Merge pipeline-required-tasks from ruleData config#1786
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds rule-data overrides for ChangesPipeline required-task overrides
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant RuleData
participant TektonPipeline
participant ReleaseTaskChecks
RuleData->>TektonPipeline: provide pipeline-required-tasks overrides
TektonPipeline->>TektonPipeline: merge base and configured mappings
TektonPipeline->>ReleaseTaskChecks: validate merged required-task data
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoMerge pipeline-required-tasks from ruleData configuration
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
15 rules 1. Union can break validation
|
![qodo-for-conforma[bot]](https://avatars.githubusercontent.com/in/3609154?s=60&v=4){kind=small}
| default _prt_base := {} | ||
|
|
||
| _prt_base := data["pipeline-required-tasks"] | ||
|
|
||
| default _prt_config := {} | ||
|
|
||
| _prt_config := data["rule_data__configuration__"]["pipeline-required-tasks"] | ||
|
|
||
| pipeline_required_tasks := object.union(_prt_base, _prt_config) |
There was a problem hiding this comment.
1. Union can break validation 🐞 Bug ☼ Reliability
tekton.pipeline_required_tasks is computed via object.union over two unvalidated data values, and tasks.rego now validates/iterates over this computed value; if either input is a defined non-object, evaluation may fail before _data_errors can emit the intended structured format error. This turns a user configuration mistake into a hard policy failure instead of a tasks.data_provided result with actionable diagnostics.
Agent Prompt
## Issue description
`tekton.pipeline_required_tasks` is built with `object.union(_prt_base, _prt_config)` where both operands come directly from `data[...]`. If either value exists but is not an object, downstream rules (schema validation and `some key, entries in ...` iteration) may error out before producing the intended structured `_data_errors` output.
## Issue Context
This PR changed tasks validation to use `tekton.pipeline_required_tasks`, which requires `tekton.pipeline_required_tasks` to *always* evaluate safely even for malformed user-provided data.
## Fix Focus Areas
- policy/lib/tekton/pipeline.rego[11-19]
- policy/release/tasks/tasks.rego[442-456]
- policy/release/tasks/tasks.rego[483-495]
## Proposed fix
1) In `policy/lib/tekton/pipeline.rego`, split raw vs safe values:
- Read `_prt_base_raw` and `_prt_config_raw` using `object.get(...)` to avoid unsafe nested indexing.
- Create `_prt_base_obj` / `_prt_config_obj` that coerce to `{}` unless `is_object(...)`.
- Compute `pipeline_required_tasks := object.union(_prt_base_obj, _prt_config_obj)`.
2) In `policy/release/tasks/tasks.rego`, preserve actionable errors by validating the *raw* sources too:
- Run `j.validate_schema` against `_prt_base_raw` and `_prt_config_raw` (or add explicit `not is_object(...)` checks) so non-object inputs generate `tasks.data_provided` messages rather than crashing evaluation.
- Keep the existing merged validation/iteration against `tekton.pipeline_required_tasks` once the merge is guaranteed safe.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 `@policy/lib/tekton/pipeline.rego`:
- Around line 17-19: Guard the inputs used by pipeline_required_tasks before
calling object.union: validate or default _prt_base and _prt_config so both are
objects, including malformed or missing pipeline-required-tasks values. Preserve
valid configuration merging while allowing tasks.rego to emit its structured
schema error for invalid input instead of failing during policy evaluation.
🪄 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: 5287c6ab-e174-40a7-b839-bc39f170f284
📒 Files selected for processing (2)
policy/lib/tekton/pipeline.regopolicy/release/tasks/tasks.rego
| _prt_config := data["rule_data__configuration__"]["pipeline-required-tasks"] | ||
|
|
||
| pipeline_required_tasks := object.union(_prt_base, _prt_config) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
command -v opa >/dev/null || {
echo "OPA is required"
exit 1
}
if opa eval --format=pretty 'object.union([], {})' >/dev/null 2>&1; then
echo "Expected object.union to reject a non-object operand"
exit 1
fi
echo "object.union rejects non-object operands as expected"Repository: conforma/policy
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== policy/lib/tekton/pipeline.rego ==\n'
wc -l policy/lib/tekton/pipeline.rego
sed -n '1,120p' policy/lib/tekton/pipeline.rego
printf '\n== references to pipeline-required-tasks ==\n'
rg -n 'pipeline-required-tasks|rule_data__configuration__|object.union' policy -S
printf '\n== relevant task validation files ==\n'
fd -a 'tasks.rego|pipeline.rego' policyRepository: conforma/policy
Length of output: 21153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== policy/release/tasks/tasks.rego: validation and callers ==\n'
sed -n '430,520p' policy/release/tasks/tasks.rego
printf '\n---\n'
sed -n '1,120p' policy/pipeline/required_tasks/required_tasks.rego
printf '\n== policy/lib/tekton/pipeline_test.rego: coverage around malformed inputs ==\n'
sed -n '1,260p' policy/lib/tekton/pipeline_test.regoRepository: conforma/policy
Length of output: 14967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== rule_data configuration access and schema handling ==\n'
sed -n '1,260p' policy/lib/rule_data/rule_data.rego
printf '\n---\n'
rg -n 'rule_data__configuration__|validate_schema|schema' policy/lib/rule_data policy -S
printf '\n== pipeline required task tests for malformed config ==\n'
rg -n 'unexpected format|rule_data__configuration__|pipeline-required-tasks' policy/release/tasks/tasks_test.rego policy/pipeline/required_tasks/required_tasks_test.rego policy/lib/tekton/pipeline_test.rego -SRepository: conforma/policy
Length of output: 46065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== policy/lib/rule_data/rule_data.rego ==\n'
wc -l policy/lib/rule_data/rule_data.rego
sed -n '1,240p' policy/lib/rule_data/rule_data.rego
printf '\n== tests referencing malformed rule_data__configuration__ ==\n'
rg -n 'rule_data__configuration__ as|unexpected format|Data .* unexpected format' policy -SRepository: conforma/policy
Length of output: 35748
🌐 Web query:
OPA Rego object lookup on non-object type error missing object key semantics
💡 Result:
In OPA Rego, attempting to perform an object lookup (e.g., obj.key or obj[key]) on a value that is not an object, or referencing a non-existent key, typically leads to an "undefined" state rather than a standard runtime exception that crashes the policy [1]. When this occurs in a policy evaluation, the rule or expression containing the lookup evaluates to "undefined," which effectively behaves like a failure in many contexts (e.g., it causes the rule body to not be satisfied) [2][1]. Regarding the specific error messages and semantics: 1. Type Errors: If the OPA type checker can determine that a lookup is being performed on a non-object type (or an invalid key), it may throw a "rego_type_error" at compile time or during analysis, stating that the reference is undefined [3][4]. 2. Undefined vs. False: A common point of confusion is that accessing a key whose value is explicitly false is not the same as accessing a missing key [5][6]. Because Rego uses unification, expressions like obj[key] succeed if the lookup yields a value, including false [6]. However, if you use a pattern that interprets the result as a boolean or participates in a generator (e.g., {k | obj[k]}), a false value may be ignored or cause the expression to fail because the evaluation does not unify to true [5][6]. To check for key existence regardless of the value (even if false), it is idiomatic to use an assignment to a wildcard: _ = obj[key] [7][5][6]. 3. Safe Access: To avoid "undefined" results when a key might be missing, you can use the built-in function object.get(object, key, default) [4][2]. This function allows you to specify a default value if the key does not exist, preventing the entire expression from evaluating to "undefined" [4]. In summary, "non-object type" or "undefined ref" errors usually indicate that your policy is attempting to traverse a path that does not exist in the data provided [4][1]. Using object.get or explicit existence checks via _ = obj[key] is the recommended approach for writing robust policies that handle optional or missing data [2][5].
Citations:
- 1: Default deny when accessing undefined key open-policy-agent/conftest#1164
- 2: Improve support for handling undefined inside of queries open-policy-agent/opa#2345
- 3: Referencing non-existing property of empty object passes type check, while non-empty object does not open-policy-agent/opa#7275
- 4: https://sangkeon.github.io/opaguide/chap4/builtinfunction.html
- 5: https://blog.kenev.net/en/posts/check-if-key-exists-in-object-in-rego-42pp/
- 6: https://stackoverflow.com/questions/76044732/opa-doesnt-find-key-if-its-value-is-false
- 7: Request for an
object.key_exists(object, key)built-in function open-policy-agent/opa#5232
🌐 Web query:
OPA object.union non-object operand type error undefined behavior
💡 Result:
In Open Policy Agent (OPA), the object.union and object.union_n built-in functions are strictly designed to operate on objects [1][2]. Passing non-object types (such as strings, arrays, or numbers) as operands to these functions typically results in a runtime error rather than "undefined behavior" in the sense of uncontrolled memory corruption, though older or specific versions may exhibit unexpected behavior like panics [3]. Key technical points regarding these errors include: 1. Expected Behavior: The object.union function expects both arguments to be objects [1][2]. If a non-object is provided, OPA will generally throw a runtime error because the function cannot perform an asymmetric recursive union on incompatible types. 2. Historical Runtime Panics: In some versions of OPA, passing an array containing non-object elements to object.union_n could trigger a runtime panic (specifically a nil pointer dereference) [3]. This is a bug related to unchecked error handling within the function's internal implementation [3]. 3. Type Checking: While many OPA built-in functions have historically been documented with an any return type, they strictly expect object inputs [4]. Efforts have been made to improve OPA's type checker to catch these mismatches at compile time rather than relying on runtime failures [4]. 4. Distinction from Merge Conflicts: Developers occasionally encounter "object merge conflict" errors. This is distinct from a type error; it occurs during recursive merging when the function encounters incompatible structures that cannot be unified (e.g., trying to merge a primitive value into an existing object key) [5]. If you are encountering unexpected behavior, it is recommended to ensure that all data passed to object.union or object.union_n is validated as an object using internal checks or by ensuring your input data structures conform to the expected schema before the built-in is called.
Citations:
- 1: https://www.openpolicyagent.org/docs/policy-reference/builtins/object
- 2: https://v0-57-1--opa-docs.netlify.app/policy-reference/
- 3: panic when
object.union_n()called on an array with non-object elements open-policy-agent/opa#5073 - 4: Dubious
anyreturn type definition in several built-in functions open-policy-agent/opa#8692 - 5: [wasm] error when calling eval: object merge conflict open-policy-agent/opa#2194
Guard the rule-data merge in policy/lib/tekton/pipeline.rego:17-19. object.union only accepts objects, so a malformed data["pipeline-required-tasks"] or data["rule_data__configuration__"]["pipeline-required-tasks"] value can fail policy evaluation before tasks.rego emits the structured schema error. Validate or default both inputs before merging.
🤖 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 `@policy/lib/tekton/pipeline.rego` around lines 17 - 19, Guard the inputs used
by pipeline_required_tasks before calling object.union: validate or default
_prt_base and _prt_config so both are objects, including malformed or missing
pipeline-required-tasks values. Preserve valid configuration merging while
allowing tasks.rego to emit its structured schema error for invalid input
instead of failing during policy evaluation.
04e4673 to
d704179
Compare
|
🤖 Finished Review · ✅ Success · Started 5:06 PM UTC · Completed 5:21 PM UTC |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsMedium
Low
Labels: PR fixes a bug where ruleData pipeline-required-tasks was silently ignored |
![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
requires-manual-review
The policy now merges pipeline-required-tasks from both data sources and the ruleData configuration (rule_data__configuration__). This allows users to extend pipeline-required-tasks via the ruleData field in their policy config without needing a separate data source file. Uses the same lib_rule_data pattern as trusted_tasks merging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d704179 to
9b01177
Compare
|
🤖 Review · |
Document the non-obvious interaction between rule_data.get() defaults, object.union safety, and override precedence — knowledge needed by anyone adding a new data-source/ruleData merge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 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 `@design/rule-data-merge-pattern.md`:
- Around line 36-38: Clarify in the rule_data merge documentation that the
defaults map, including the {} default in rule_data.rego, applies only when the
key is absent and does not protect against explicitly supplied arrays or other
invalid types. Require validating present values or applying a safe object
fallback before object.union, preventing invalid data from silently producing
zero violations and errors.
🪄 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: ba1678e9-e875-4bc9-8bcc-2de34e6a1e24
📒 Files selected for processing (1)
design/rule-data-merge-pattern.md
| This means a misconfigured data source that provides an array instead of an object will | ||
| cause policy rules to silently stop evaluating, producing zero violations and zero errors. | ||
| The `{}` default in rule_data.rego is the primary defense against this. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clarify that {} does not protect against wrong-typed values.
The defaults map is consulted only when the key is absent. If a data source explicitly supplies an array, rule_data.get(...) still returns that array and object.union remains undefined. Document this limitation and require type validation or a safe fallback for present, invalid values; otherwise this can still fail open with zero policy violations.
🤖 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 `@design/rule-data-merge-pattern.md` around lines 36 - 38, Clarify in the
rule_data merge documentation that the defaults map, including the {} default in
rule_data.rego, applies only when the key is absent and does not protect against
explicitly supplied arrays or other invalid types. Require validating present
values or applying a safe object fallback before object.union, preventing
invalid data from silently producing zero violations and errors.
Fixes opa-check strict mode: assigned var attestation unused. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Review · |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Review · |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Finished Review · ❌ Failure · Started 7:53 PM UTC · Completed 8:10 PM UTC |
|
This was created from a slack conversation, so I still need to create a story. |
1 participant
What:
Merge
pipeline-required-tasksfrom the ruleData configuration into the pipeline required tasks used by the tasks policy. This adds apipeline_required_tasksrule inpipeline.regothat unions data from bothdata["pipeline-required-tasks"](data sources) anddata["rule_data__configuration__"]["pipeline-required-tasks"](ruleData), and updates all consumers inpipeline.regoandtasks.regoto use it.Why:
Previously,
pipeline-required-tasksspecified in theruleDatafield of a policy configuration was silently ignored. The ec CLI places ruleData underdata.rule_data__configuration__, but the tasks policy accesseddata["pipeline-required-tasks"]directly — bypassing the ruleData namespace entirely. This meant users could not extend pipeline-required-tasks via their policy config and had to provide a separate data source file instead.Tickets: