fix(osv): keep base scan results across fork checkout - #1257
Conversation
Copy a non-empty old-results.json into RUNNER_TEMP before the head checkout and restore it before compare, including scanner output under source/, so a zero-finding fork PR cannot fail on an empty base file.
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughOSV 보안 스캔이 base와 fork head 결과를 ChangesOSV 결과 격리
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟠 High · up to The workflow preserves base OSV results across fork checkouts, but the current head still carries a high-impact integrity risk because fork-provided result files may influence vulnerability comparison, potentially allowing findings to be bypassed. Cleanup can also fail before scanning when result paths are directories, so merge should wait for these issues to be addressed. Sequence Diagram(s)sequenceDiagram
participant OSVScan
participant RUNNER_TEMP
participant HeadCheckout
participant Workspace
participant Reporter
OSVScan->>RUNNER_TEMP: base 결과 캡처 및 검증
HeadCheckout->>Workspace: fork head checkout 수행
OSVScan->>RUNNER_TEMP: head 결과 캡처 및 검증
RUNNER_TEMP->>Workspace: base·head 결과 materialize
Workspace->>Reporter: 검증된 결과 전달
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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.
![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.
Copy OSV results only into RUNNER_TEMP, unlink any root-owned workspace copy before restore, and discard checkout-provided JSON before each scan so a fork cannot plant reporter input. Observed quality failure was permission denied while overwriting old-results.json.
|
@opencode-agent @cwl-noema-review current-head review for Root cause of the previous |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent @cwl-noema-review current-head review for OSV capture remains in |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Independent current-head review request for exact HEAD Gap G-03: required Strix still fail-closes 0-vuln complete scans on |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
20d72bc838d7f91b74ce01bb4de16d07144fa270. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/32644456722/job/97206450237)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/32644456722/job/97206450237)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: security-scan.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: security-scan.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file: CHANGELOG.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: CHANGELOG.md"]
R2 --> V2["required checks"]
Evidence --> S3["Docs: osv-cross-fork-result-isolation.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: osv-cross-fork-result-isolation.md"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_required_workflow_queue_contract.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_required_workflow_queue_contract.py"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: security-scan.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: security-scan.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file: CHANGELOG.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: CHANGELOG.md"]
R2 --> V2["required checks"]
Evidence --> S3["Docs: osv-cross-fork-result-isolation.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: osv-cross-fork-result-isolation.md"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_required_workflow_queue_contract.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_required_workflow_queue_contract.py"]
R4 --> V4["targeted test run"]
|
…-results-runner-temp # Conflicts: # .github/workflows/security-scan.yml # CHANGELOG.md
|
Resolved the stale merge conflict against current Conflicts (2 files, both mechanical — no design decision involved):
Test evidence (worktree, head
Pushed as merge commit Generated by Claude Code |
| dest="${RUNNER_TEMP}/osv-old-results.json" | ||
| rm -f "${dest}" | ||
| umask 077 | ||
| for candidate in old-results.json source/old-results.json; do | ||
| if [ -f "${candidate}" ] && [ ! -L "${candidate}" ] && [ -s "${candidate}" ]; then | ||
| /usr/bin/sudo --non-interactive /usr/bin/cat -- "${candidate}" | /usr/bin/tee "${dest}" >/dev/null | ||
| test -s "${dest}" | ||
| test -O "${dest}" | ||
| echo "Preserved OSV base output from ${candidate}" | ||
| exit 0 | ||
| fi | ||
| done |
There was a problem hiding this comment.
📝 Info: Failed scans cannot reuse stale captures
rm -f clears each temporary capture before validation. Scan-root cleanup also prevents a failed scan from reusing checkout-provided results.
Was this helpful? React with 👍 or 👎 to provide feedback.
| umask 077 | ||
| for candidate in old-results.json source/old-results.json; do | ||
| if [ -f "${candidate}" ] && [ ! -L "${candidate}" ] && [ -s "${candidate}" ]; then | ||
| /usr/bin/sudo --non-interactive /usr/bin/cat -- "${candidate}" | /usr/bin/tee "${dest}" >/dev/null | ||
| test -s "${dest}" | ||
| test -O "${dest}" | ||
| echo "Preserved OSV base output from ${candidate}" |
There was a problem hiding this comment.
📝 Info: Root-owned output remains consumable
Privileged cat reads the scanner result, while unprivileged tee creates the private capture. Later steps therefore retain readable, runner-owned evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
| old="${RUNNER_TEMP}/osv-old-results.json" | ||
| new="${RUNNER_TEMP}/osv-new-results.json" | ||
| test -s "${old}" | ||
| test -s "${new}" | ||
| rm -f old-results.json new-results.json | ||
| cp "${old}" old-results.json | ||
| cp "${new}" new-results.json |
There was a problem hiding this comment.
📝 Info: Reporter inputs avoid partial materialization
Both captures pass validation before workspace inputs are replaced. A missing result preserves external evidence and prevents a partial comparison.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # CHANGELOG.md
Stale base resolved — merged current
|
Two resolutions, one of which the conflict markers did not surface: 1. CHANGELOG.md (marked conflict) — kept both sides, this branch's entry above main's, matching the file's newest-first convention. 2. tests/test_required_workflow_queue_contract.py (NO conflict marker) — the automatic merge of the import block kept this branch's `import stat` but dropped its `import shlex`, while `shlex.quote` remains in use at the security-scan transfer assertion. The merge therefore produced a tree that auto-merged cleanly and failed three tests with `NameError: name 'shlex' is not defined`. Restored the import. Isolated before fixing: the three tests pass on this branch's pre-merge head and do not exist on main, so the breakage was introduced by the merge, not pre-existing on either side. Verified after resolution: 2893 passed, 1 skipped, coverage 100%, interrogate 100%; zero conflict markers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added
bug
ChatGPT Codex Connector
2 participants
Why
Fork PRs such as ContextualWisdomLab/governance-risk-compliance#51 scanned clean (OSV exit 0, zero findings) and then failed
Require OSV scan outputbecauseold-results.jsonwas empty or missing after the head checkout replaced the workspace.PR #1209 already isolates checkouts under
source/. This follow-up copies a non-empty base result into${RUNNER_TEMP}/osv-old-results.jsonbefore the fork checkout and restores it before compare, including when the scanner writessource/old-results.json.Missing output after restore remains a hard failure. A zero-finding head scan does not skip base comparison.
Verification
pytest tests/test_required_workflow_queue_contract.py::test_security_scan_preserves_base_output_across_cross_fork_checkoutosv-scanon exact head1a8f90dd15f37ffc86b8a0efd217a8b2812e5f99succeeded after re-run against current main (#1209). This PR hardens that contract so the empty-file failure cannot recur.Summary by CodeRabbit
버그 수정
문서