feat: bind workflow findings to source evidence - #1000
Conversation
Closes #938 Derive typed clean, detected, or unknown assessments from acquired GitHub Actions run/job metadata, preserve canonical provenance through IssueOps and control-plane scan detail, and add fail-closed fixtures, coverage, and product traceability docs.
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughGitHub Actions의 보안 워크플로 run/job 결과를 검증하는 source-bound evidence 수집기를 추가했습니다. 수집기는 SHA-256, freshness, typed assessment를 생성하고, 정상화된 finding과 IssueOps 및 control plane 상세 조회에 증거를 보존합니다. Changes소스 바운드 워크플로 증거
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to This PR can incorrectly treat stale or unbound workflow jobs as valid evidence, crash on malformed metadata, and publish inconclusive results as confirmed workflow failures. These paths can create false security findings or bypass safe evidence handling, so the current head is not ready to merge until the validation and publication issues are fixed. Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant collect_findings
participant acquire_workflow_evidence
participant IssueOps
participant ControlPlane
GitHubActions->>collect_findings: workflow run/job 결과 제공
collect_findings->>acquire_workflow_evidence: repository, run, job 전달
acquire_workflow_evidence->>acquire_workflow_evidence: 필드, freshness, 중복, 결과 검증
acquire_workflow_evidence-->>collect_findings: source evidence와 typed assessment 반환
collect_findings->>IssueOps: 증거가 포함된 finding 게시
collect_findings->>ControlPlane: canonical evidence를 scan detail에 저장
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@opencode-agent @cwl-noema-review please review the exact current head 782c7ba for Issue #938. This is a protected review request only; no approval or merge authorization is implied. Please report findings and required changes on this exact head. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add the public Issue #815 OpenCode run and sibling success job as source fixtures with independent outcome oracles and traceability.
|
@opencode-agent @cwl-noema-review please review the exact current head 5a217b5 for Issue #938. This is a protected review request only; no approval or merge authorization is implied. Please report findings and required changes on this exact head. |
|
@opencode-agent @cwl-noema-review please review the exact current head 2d86881 for Issue #938, including the source-evidence acquisition ordering, strict run/job identity validation, job-completion freshness precedence, fail-closed publication path, and regression fixtures. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent @cwl-noema-review please review the exact current head b1ec29b for Issue #938, including the configured lookback-to-freshness propagation and historical --run-url replay regression. |
opencode-agent
Bot
added
area: ci-cd
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
| workflow = _text(run.get("name")) or _text(job.get("workflow_name")) | ||
| job_name = _text(job.get("name")) | ||
| if not is_security_name(workflow, job_name): | ||
| return _unknown( | ||
| repository, acquired_at, "unknown-detector-family", revision=revision | ||
| ) |
There was a problem hiding this comment.
📝 Info: acquire uses a two-argument is_security_name while collector uses three; effectively equivalent
collect_findings gates jobs with is_security_name(run.get("name"), job.get("workflow_name"), job.get("name")), while acquire_workflow_evidence recomputes with is_security_name(workflow, job_name) where workflow = _text(run.get("name")) or _text(job.get("workflow_name")) (source_evidence.py). Since is_security_name is variadic and joins all names, dropping job.workflow_name when run.name is truthy could theoretically diverge. In practice GitHub sets run name and job workflow_name to the same workflow name, so the security-term set is unchanged and the divergence only ever fails closed (excludes, never falsely includes). Not a bug, but the duplicated classification logic is a maintenance hazard if the two call sites drift.
Was this helpful? React with 👍 or 👎 to provide feedback.
| finding = build_source_bound_finding( | ||
| repo, | ||
| run, | ||
| job, | ||
| now=collection_now, | ||
| seen_artifact_refs=seen_artifact_refs, | ||
| max_age_hours=args.lookback_hours, | ||
| ) |
There was a problem hiding this comment.
📝 Info: run-url replay now bounded by lookback freshness
The --run-url replay path now enforces source freshness against --lookback-hours (default 48h), so replaying an older run yields stale-source-evidence and publishes nothing. Operators validating historical runs must raise --lookback-hours to cover the run's age.
Was this helpful? React with 👍 or 👎 to provide feedback.
| age = (acquired - observed).total_seconds() | ||
| if age < 0 or age > max_age_hours * 3600: | ||
| return _unknown( | ||
| repository, | ||
| acquired_at, | ||
| "stale-source-evidence", | ||
| revision=revision, | ||
| observed_at=observed_at, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Source freshness now uses job completed_at, diverging from run-selection cutoff
collect_findings selects candidate runs using the run's updated_at/created_at against cutoff (collect_org_security_failures.py), but acquire_workflow_evidence computes freshness from job.completed_at or run.updated_at or run.created_at (source_evidence.py) with the same max_age_hours. A run whose overall updated_at is recent (e.g. because other jobs re-ran) but whose relevant security job completed longer ago than the lookback window will now be dropped as stale-source-evidence, whereas the old collector would have published it. This is a deliberate tightening consistent with the ADR, but worth confirming it does not silently drop still-relevant failures.
Was this helpful? React with 👍 or 👎 to provide feedback.
| causes = job.get("failure_causes") | ||
| if causes is not None and (not isinstance(causes, list) or len(causes) != 1): | ||
| return _unknown( | ||
| repository, | ||
| acquired_at, | ||
| "ambiguous-cause-order", | ||
| revision=revision, | ||
| observed_at=observed_at, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Empty failure_causes list is classified as ambiguous-cause-order
The cause-order guard rejects any failure_causes value that is not a single-element list: causes is not None and (not isinstance(causes, list) or len(causes) != 1) (source_evidence.py). An empty list ([]) therefore yields unknown/ambiguous-cause-order rather than being treated as "no cause reported". Since GitHub REST job payloads do not natively include failure_causes, this only matters if a caller supplies it; behavior is fail-closed so it is not a correctness bug, but the empty-list case may be surprising.
Was this helpful? React with 👍 or 👎 to provide feedback.
| finding = build_source_bound_finding( | ||
| repo, | ||
| run, | ||
| job, | ||
| now=collection_now, | ||
| seen_artifact_refs=seen_artifact_refs, | ||
| max_age_hours=args.lookback_hours, | ||
| ) | ||
| assessment = finding.get("source_evidence", {}).get( | ||
| "assessment", {} | ||
| ) | ||
| if assessment.get("status") != "detected": | ||
| continue |
There was a problem hiding this comment.
🔍 Collector now files issues only for job conclusion 'failure', dropping cancelled/timed_out/action_required security jobs
Previously collect_findings appended a finding for every failure-class job (is_failure matches failure/cancelled/timed_out/action_required). Now build_source_bound_finding only returns a full finding when the derived assessment is detected, and acquire_workflow_evidence maps only job conclusion == "failure" to detected (source_evidence.py); cancelled/timed_out/action_required become unknown-detector-result and are skipped at collect_org_security_failures.py. This means cancelled/timed-out/action-required security workflow jobs no longer generate any issue at all. Note the diagnosis() branches in issueops.py that special-case those conclusions are now effectively dead for the collector path. This appears intended per ADR-0007 ("The collector publishes only detected assessments"), but it is a meaningful coverage narrowing worth confirming against the docs that insist non-completion must not be collapsed into a passing state.
Was this helpful? React with 👍 or 👎 to provide feedback.
| result = { | ||
| "schema": SCHEMA, | ||
| "detector_family": DETECTOR_FAMILY, | ||
| "atomic_cause": ATOMIC_CAUSE, | ||
| "control_obligation": CONTROL_OBLIGATION, | ||
| "probe_ref": PROBE_REF, | ||
| "acquirer_ref": ACQUIRER_REF, | ||
| "source_identity": { | ||
| "repository": repository, | ||
| "revision": revision, | ||
| "artifact_ref": artifact_ref, | ||
| "artifact_sha256": artifact_sha256, | ||
| "observed_at": observed_at, | ||
| "acquired_at": acquired_at, | ||
| }, | ||
| "assessment": { | ||
| "status": status, | ||
| "reason": reason, | ||
| "confirmed_vulnerability": False, | ||
| }, | ||
| } | ||
| result["evidence_digest"] = _sha256(result) | ||
| return result |
There was a problem hiding this comment.
📝 Info: evidence_digest correctly excludes itself from the hash
_evidence computes result["evidence_digest"] = _sha256(result) (source_evidence.py) after building the envelope but before inserting the digest key, so the digest deterministically covers all fields except itself. artifact_sha256 is separately derived from _source_payload, which selects only bounded run/job/step metadata and ignores any caller-injected assessment/source_artifact_sha256 keys on the raw job dict — verified by the mutation tests. This confirms the PR's claim that caller-provided assessment/digest fields cannot influence the derived hash.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
the
enhancement
ChatGPT Codex Connector
1 participant
Closes #938
Summary
detectedassessments to the security-failure issue path--lookback-hoursas the source freshness bound, including explicit--run-urlhistorical replaysSemantics
A detected result means the security workflow control failed; it is not a confirmed application vulnerability. Caller-provided assessment and digest fields are ignored. Unknown or inconclusive source evidence is not published as a security finding. Freshness remains fail-closed against the explicitly configured lookback window.
Verification
acquire_workflow_evidencepath at b1ec29b; result was detected/security-workflow-job-failure with artifact SHA-256d5f63667ab6254d8c6be26bf6998cb22e7465be5289f53fd1d10413f7d5fb6a9; no raw logs were used