feat: populate assessment plan and step identity in evaluation logs#579
Conversation
8f8f4d4 to
0279571
Compare
7813ef1 to
2a7f806
Compare
|
@jpower432 @gvauter Testing result following the Review Hints in the PR description Compliance Scan Report: policies/test-opa-policyGenerated: 2026-06-11T17:17:34Z Control: container-resource-limits
check-resource-limits
Control: container-run-as-nonroot
check-run-as-nonroot
Evaluation Logmetadata:
id: policies/test-opa-policy
type: EvaluationLog
gemara-version: v1.0.0
description: Compliance scan evaluation log
author:
id: complytime
name: complytime
type: Software
uri: https://github.com/complytime/complyctl
result: Passed
evaluations:
- name: container-resource-limits
result: Passed
message: 1 of 1 targets passed
control:
reference-id: policies/test-opa-policy
entry-id: container-resource-limits
assessment-logs:
- requirement:
reference-id: policies/test-opa-policy
entry-id: check-resource-limits
plan:
reference-id: policies/test-opa-policy
entry-id: check-resource-limits
description: 1 of 1 targets passed
result: Passed
message: 1 of 1 targets passed
applicability:
- default
steps:
- "complypacks/test-opa-complypack@sha256:18260d6f99bf60dd7fba9c68fe8b1a0fd28c6db2dd44423f42511262b5e21f65#test-deployment.yaml"
steps-executed: 1
start: "2026-06-11T17:17:34Z"
confidence-level: High
- name: container-run-as-nonroot
result: Passed
message: 1 of 1 targets passed
control:
reference-id: policies/test-opa-policy
entry-id: container-run-as-nonroot
assessment-logs:
- requirement:
reference-id: policies/test-opa-policy
entry-id: check-run-as-nonroot
plan:
reference-id: policies/test-opa-policy
entry-id: check-run-as-nonroot
description: 1 of 1 targets passed
result: Passed
message: 1 of 1 targets passed
applicability:
- default
steps:
- "complypacks/test-opa-complypack@sha256:18260d6f99bf60dd7fba9c68fe8b1a0fd28c6db2dd44423f42511262b5e21f65#test-deployment.yaml"
steps-executed: 1
start: "2026-06-11T17:17:34Z"
confidence-level: High
target:
id: test-k8s-deployment
name: test-k8s-deployment
type: Software
|
gvauter
left a comment
There was a problem hiding this comment.
DevPod E2E verified: full OPA pipeline (get → generate → scan) produces correct evaluation log output with plan: field populated and steps: containing digest-pinned complypack identity strings. All CI checks pass.
One minor consistency note flagged inline — non-blocking.
This review was generated by /review-pr (AI-assisted).
7232297 to
0645189
Compare
363f43b to
6158b29
Compare
|
@jpower432 @gvauter see the output here with the feedback addressed. Compliance Scan Report: policies/test-opa-policyGenerated: 2026-06-12T13:12:47Z Control: container-resource-limits
check-resource-limits
Control: container-run-as-nonroot
check-run-as-nonroot
Evaluation Logmetadata:
id: policies/test-opa-policy
type: EvaluationLog
gemara-version: v1.0.0
description: Compliance scan evaluation log
author:
id: complytime
name: complytime
type: Software
uri: https://github.com/complytime/complyctl
result: Passed
evaluations:
- name: container-resource-limits
result: Passed
message: 1 of 1 targets passed
control:
reference-id: policies/test-opa-policy
entry-id: container-resource-limits
assessment-logs:
- requirement:
reference-id: policies/test-opa-policy
entry-id: check-resource-limits
plan:
reference-id: policies/test-opa-policy
entry-id: check-resource-limits
description: 1 of 1 targets passed
result: Passed
message: 1 of 1 targets passed
applicability:
- default
steps:
- "complypacks/test-opa-complypack@sha256:18260d6f99bf60dd7fba9c68fe8b1a0fd28c6db2dd44423f42511262b5e21f65#test-deployment.yaml"
steps-executed: 1
start: "2026-06-12T13:12:47Z"
confidence-level: High
- name: container-run-as-nonroot
result: Passed
message: 1 of 1 targets passed
control:
reference-id: policies/test-opa-policy
entry-id: container-run-as-nonroot
assessment-logs:
- requirement:
reference-id: policies/test-opa-policy
entry-id: check-run-as-nonroot
plan:
reference-id: policies/test-opa-policy
entry-id: check-run-as-nonroot
description: 1 of 1 targets passed
result: Passed
message: 1 of 1 targets passed
applicability:
- default
steps:
- "complypacks/test-opa-complypack@sha256:18260d6f99bf60dd7fba9c68fe8b1a0fd28c6db2dd44423f42511262b5e21f65#test-deployment.yaml"
steps-executed: 1
start: "2026-06-12T13:12:47Z"
confidence-level: High
target:
id: test-k8s-deployment
name: test-k8s-deployment
type: Software
|
marcusburghardt
left a comment
There was a problem hiding this comment.
Well-structured PR with thorough test coverage, complete OpenSpec artifacts, and clean CI. The shadow struct approach for step identity serialization is well-motivated and documented in the design doc. Two items worth noting below.
Commit trailers (MEDIUM): Commit c0fde116 is missing the Signed-off-by trailer required by the constitution. Commit 73853dd6 has a name mismatch between author ("Hannah Braswell") and sign-off ("Heather Braswell"). These can be fixed with an interactive rebase.
This review was generated by /review-pr (AI-assisted).
Thanks for this catch. I will fix this immediately. |
6158b29 to
82c16c8
Compare
marcusburghardt
left a comment
There was a problem hiding this comment.
Commit trailer issues from the prior review round are resolved. All GitHub Actions CI checks pass. Two MEDIUM items remain as follow-up suggestions (swapped warning labels in reverseMap, growing parameter count on processScanOutput) -- neither affects correctness.
This review was generated by /review-pr (AI-assisted).
gvauter
left a comment
There was a problem hiding this comment.
All 5 sub-issues from the spec are addressed with comprehensive test coverage.
The resolvedMappings struct cleanly prevents parameter-transposition bugs.
Both prior reviewer concerns (gvauter, jpower432) are resolved in the code.
All CI checks pass.
This review was generated by /review-pr (AI-assisted).
The scan command builds gemara.EvaluationLog from provider.ScanResponse, but the conversion from provider.Step (a data struct) to gemara.AssessmentStep (a function type) was missing. The evaluation log YAML always showed steps: [] regardless of what providers sent. Add providerStepToGemara() to wrap each provider.Step into a gemara.AssessmentStep closure that returns the step's pre-computed result, message, and the assessment-level confidence. Add providerStepsToGemara() to convert a slice of provider steps. Update providerToGemaraAssessment() to populate the Steps field on the returned gemara.AssessmentLog. Signed-off-by: Hannah Braswell <hbraswel@redhat.com>
The evaluation log YAML emitted by complyctl scan had two identity gaps after PR complytime#575 fixed the structural step closure bug: 1. The plan field on gemara.AssessmentLog was never populated, breaking the requirement-to-plan traceability chain 2. Steps serialized as Go function pointer names instead of meaningful identifiers linking back to the checks that were run No proto changes, no go-gemara changes. Provider-side Step.Name population is tracked separately in complytime-providers. Closes: complytime#578 Assisted-by: Claude (Anthropic, Claude Opus 4.6) Signed-off-by: Hannah Braswell <hbraswel@redhat.com>
Signed-off-by: Hannah Braswell <hbraswel@redhat.com>
Addresses review feedback from jpower432: the previous resolveComplypackRef() re-derived the OCI reference from cache state and assumed one complypack per evaluator. Replace with per-evaluator ref map threaded from config+state. Changes: - Replace resolveComplypackRef() with buildComplypackRefs() returning map[string]string (evaluatorID -> repo@digest) built from state.Complypacks - Add extractReqToEvaluator() to map requirementID -> evaluatorID from evaluator groups, enabling per-assessment complypack ref resolution - Thread both maps through executeScanPhase -> runScanAndReport -> processScanOutput -> buildEvaluators -> NewEvaluator - Change Evaluator.complypackRef string to complypackRefs map + reqToEvaluator map with resolveComplypackRef(requirementID) method for per-assessment lookup - Update toSerializable() to resolve complypack ref per assessment log This correctly handles multiple evaluators with different complypacks per policy without re-reading cache state. Signed-off-by: Hannah Braswell <hbraswel@redhat.com>
Signed-off-by: Hannah Braswell <hbraswel@redhat.com>
9ebddc8 to
4e3ebb9
Compare
Replace two separate maps (complypackRefs evaluatorID->ref and reqToEvaluator reqID->evaluatorID) with a single pre-resolved reqToComplypackRef map (reqID->ref). The resolution chain is composed in buildReqToComplypackRef() in the scan pipeline, keeping the Evaluator free of evaluator-ID routing concerns. This simplifies the Evaluator constructor from 6 to 5 params, reduces resolveComplypackRef to a direct map lookup, and adds a collision warning when multiple complypacks share an evaluator-id. Assisted-by: Claude (Anthropic, Claude Opus 4.6) Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
…refs refactor: pre-resolve complypack refs upstream of Evaluator
Description
The evaluation log YAML emitted by complyctl scan had two identity gaps after PR #575 fixed the structural step closure bug:
gemara.AssessmentLogwas never populated, breaking the requirement-to-plan traceability chaingithub.com/complytime/complyctl/internal/output.providerStepsToGemara.providerStepToGemara.func1)instead of meaningful identifiers linking back to the checks that were run.Notes
The step summary included the Go function pointer names rather than the identifiers from the
.regofiles in the complypackcomplytime-mapping.jsonassociate the gemara requirement-ids (in the Policy assessment plan) to its.regonamespace id equivalent (i.e., the assessment plan testable condition related to its actual testable.regofile executable checks). By pointing to the correct step execution link they can be accessed by their sha256 once updated in subsequent versions.Changes
reqToPlanandcomplypackReffields to Evaluator struct. Thread plan ID reverse map and complypack OCI ref from scan pipeline throughprocessScanOutput/buildEvaluatorsintoNewEvaluatorproviderToGemaraAssessmentusing reqToPlan lookup from the dependency graphgemara.EvaluationLoghierarchy withSteps []stringinstead ofSteps []AssessmentStep(function type)formatStepIdentity()to produce'{oci-ref}#{step-name}'format with bare step name fallback when nocomplypackis configuredAssessmentLogpointer;toSerializable()converts to shadow structWrite()to marshal shadow struct instead of gemara structUnchanged
protochangesgo-gemarachangesRelated Issues
Closes #578
Supersedes #573
Review Hints
Testing Steps
Prerequisites: A GitHub Codespace from the opsx/eval-log-plan-step-identity branch on hbraswelrh/complyctl.
Check each assessment-logs entry in the YAML for:
"complypacks/test-opa-complypack@sha256:...#test-deployment.yaml"(was previouslysteps: []or a Go function pointer name like ...providerStepToGemara.func1)Example of correct output:
Note: Step 2 requires the companion PR hbraswelrh/complytime-providers#opsx/fix-synthetic-step-names (https://github.com/hbraswelrh/complytime-providers/compare/opsx/fix-synthetic-step-names) which populates steps on synthetic passing assessments in the OPA provider. Without it, steps will be steps: [] for requirements that passed all checks.
Assisted-by: Claude (Anthropic, Claude Opus 4.6)