fix(review): 오프라인 커버리지와 증거 결합을 통합 - #604
Conversation
![pr-auto-rebase[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
seonghobae
changed the title
|
Current-head governance correction: |
seonghobae
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR reprioritizes the OpenCode review model candidate order to try GPT-4.1 first (based on observed reliability/performance), while keeping other models as ordered fallbacks, and aligns this ordering across workflow + Python + Strix contracts. It also hardens trusted execution boundaries by preventing GitHub Actions command injection from coverage logs and by failing closed on non-regular or symlink entries in the materialized PR worktree.
Changes:
- Reorders
OPENCODE_MODEL_CANDIDATESto start withgithub-models/openai/gpt-4.1and updates corresponding assertions. - Adds stop-command enveloping for replaying untrusted coverage summaries (collision-safe token).
- Adds fail-closed validation of tracked file modes and symlink presence in the PR worktree; cleans
.codegraphstate before indexing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_opencode_agent_contract.py | Updates ordering expectations and adds regression tests for stop-command replay + symlink rejection contracts. |
| scripts/ci/test_strix_quick_gate.sh | Updates Strix gate assertions to match the new model candidate ordering. |
| .github/workflows/opencode-review.yml | Reorders model candidates and introduces new security checks (stop-commands envelope, worktree entry validation, .codegraph cleanup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dismissed as stale: this review targets an older head; current-head evidence remains subject to independent review.
…quence-after-604 # Conflicts: # .github/workflows/opencode-review.yml # requirements-strix-ci.txt # scripts/ci/test_strix_quick_gate.sh # tests/test_opencode_agent_contract.py
seonghobae
changed the title
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough커버리지 소스의 ref·아카이브·워크트리 검증이 강화되고, 재구성된 SHA가 측정과 게이트에 사용됩니다. 증거 citation 복구, 로그 replay 보호, 모델 타임아웃, 의존성 해시 잠금 및 관련 계약 테스트가 추가되었습니다. ChangesCoverage source materialization
Coverage execution and protected output
Receipt-verified evidence rebinding
Provider timeout and retry-budget controls
Trusted runtime dependency locking
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/opencode-review.yml:2061
- The PR worktree symlink scan is implemented as
find | grepinside anifcondition. Withset -euo pipefail, failures in condition lists still do not terminate the step, so an unexpectedfinderror would skip this guard and allow trusted processing to continue. Since this check is explicitly a trust-boundary validation, fail closed on any scan error and only accept when the scan succeeds and finds no symlinks.
if find -P "$OPENCODE_SOURCE_WORKDIR" -mindepth 1 -type l -print -quit | grep -q .; then
echo "::error::PR worktree contains a symbolic link; refusing trusted review processing."
exit 1
fi
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/run_opencode_review_model_pool.sh (1)
552-552: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win모든 후보가 skip되면 즉시 종료해야 합니다.
should_skip_model_candidate가 전부 걸러지면dead_candidate_reasons가 비어alive_candidates가 0이 되지 않고,deadline도 0인 채로 남습니다. 이 상태에서max_cycles=0이면 cycle이 계속 반복될 수 있으니, 실제 실행 가능한 후보가 0개인 경우에는 바로 종료하거나 skip된 후보를 종료 판단에 포함해 주세요.🤖 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 `@scripts/ci/run_opencode_review_model_pool.sh` at line 552, Update the candidate-selection and termination flow around should_skip_model_candidate, alive_candidates, dead_candidate_reasons, and deadline so that when every candidate is skipped, the run immediately exits instead of continuing cycles with max_cycles=0 and deadline=0; include skipped candidates in the no-runnable-candidates decision or terminate directly when alive_candidates is zero.
🧹 Nitpick comments (5)
scripts/ci/test_strix_quick_gate.sh (2)
617-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win들여쓰기 10칸에 고정된 정규식은 재포맷 시 검사가 무력화됩니다.
^[[:space:]]{10}cat "\$CODEGRAPH_EVIDENCE_FILE"[[:space:]]*$는 워크플로 스텝의 현재 들여쓰기 깊이에 의존합니다. 스텝이 다른 블록으로 이동하거나 재들여쓰기되면 금지 패턴이 다시 도입되어도 조용히 통과합니다.^[[:space:]]*cat "\$CODEGRAPH_EVIDENCE_FILE"[[:space:]]*$처럼 깊이에 무관한 형태를 권합니다.♻️ 제안 수정
- assert_file_not_matches "$workflow_file" '^[[:space:]]{10}cat "\$CODEGRAPH_EVIDENCE_FILE"[[:space:]]*$' "opencode review does not replay assembled PR-derived CodeGraph evidence into the command channel" + assert_file_not_matches "$workflow_file" '^[[:space:]]*cat "\$CODEGRAPH_EVIDENCE_FILE"[[:space:]]*$' "opencode review does not replay assembled PR-derived CodeGraph evidence into the command channel"🤖 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 `@scripts/ci/test_strix_quick_gate.sh` at line 617, Update the regular expression in the assert_file_not_matches check for "$CODEGRAPH_EVIDENCE_FILE" to allow any amount of leading whitespace instead of requiring exactly 10 spaces, while preserving the existing command and end-of-line matching.
90-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win파일이 없으면 조용히 통과합니다.
assert_file_not_matches는[ -f "$file_path" ]가 거짓일 때 아무 것도 기록하지 않으므로, 경로 오타나 파일 이름 변경이 있으면 금지 패턴 검사가 사라진 줄 모르고 게이트가 통과합니다. 부정 어서션에서는 대상 파일 부재 자체를 실패로 처리하는 편이 안전합니다.♻️ 제안 수정
assert_file_not_matches() { local file_path="$1" local pattern="$2" local message="$3" - if [ -f "$file_path" ] && grep -Eq -- "$pattern" "$file_path"; then + if [ ! -f "$file_path" ]; then + record_failure "$message (missing file '$file_path')" + return + fi + if grep -Eq -- "$pattern" "$file_path"; then record_failure "$message (unexpected pattern '$pattern')" fi }🤖 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 `@scripts/ci/test_strix_quick_gate.sh` around lines 90 - 98, Update assert_file_not_matches so a missing target file records a failure instead of silently passing, using record_failure with a clear missing-file message; preserve the existing unexpected-pattern failure behavior when the file exists..github/workflows/opencode-review.yml (1)
174-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동일한 trusted-ref 결정 로직이 3중 복제되었습니다.
이 블록은 470-508행, 1855-1893행과 논리가 동일하지만 이번 사본만
/usr/bin/python3 -I를 쓰고 나머지 둘은python3를 사용합니다. 신뢰 경계 로직이 갈라질 위험이 있으니 composite action이나 공유 스크립트로 추출하고 인터프리터 호출 방식을 통일하는 것을 권합니다.🤖 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 @.github/workflows/opencode-review.yml around lines 174 - 212, Extract the duplicated trusted-ref resolution logic from the three workflow blocks, including the step identified by coverage_source_trusted, into one shared composite action or script. Update all callers to use that shared implementation and standardize the interpreter invocation, replacing the inconsistent /usr/bin/python3 -I versus python3 usage while preserving the existing validation and fallback behavior.scripts/ci/opencode_review_normalize_output.py (1)
693-793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win파일 간 매직 스트링 결합으로 인한 잠재적 취약점.
Line 746의
rejection == "must cite the exact probe path and positive line"비교가scripts/ci/adversarial_evidence.py의adversarial_evidence_rejection_reason이 반환하는 정확한 문자열에 하드코딩으로 결합되어 있습니다. 현재는if path and path_citation is None: suffix = " and positive line" if line is not None else ""return f"must cite the exact probe path{suffix}"조합으로 값이 정확히 일치하지만, 향후adversarial_evidence.py의 메시지 문구가 변경되면 이 조건이 조용히(예외 없이) 매칭되지 않게 되어 재바인딩 기능이 fail-closed 상태로 영구히 비활성화됩니다. 보안 상 fail-closed라 즉각적인 위험은 아니지만, 회귀를 조기에 잡기 어렵습니다.
adversarial_evidence.py에 sentinel 상수(또는 판별 헬퍼 함수)를 정의해 두 파일이 이를 공유하도록 리팩터링을 권장합니다.♻️ 제안 리팩터링
# scripts/ci/adversarial_evidence.py +MISSING_PATH_AND_LINE_CITATION = "must cite the exact probe path and positive line" + def adversarial_evidence_rejection_reason( evidence: str, path: str, line: int | None = None, ) -> str | None: ... if path and path_citation is None: suffix = " and positive line" if line is not None else "" - return f"must cite the exact probe path{suffix}" + return ( + MISSING_PATH_AND_LINE_CITATION + if suffix + else "must cite the exact probe path" + )# scripts/ci/opencode_review_normalize_output.py - rejection = adversarial_evidence_rejection_reason(repaired_evidence, path, line) - if rejection == "must cite the exact probe path and positive line": + rejection = adversarial_evidence_rejection_reason(repaired_evidence, path, line) + if rejection == adversarial_evidence.MISSING_PATH_AND_LINE_CITATION:🤖 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 `@scripts/ci/opencode_review_normalize_output.py` around lines 693 - 793, Replace the literal rejection-string comparison in repair_adversarial_probe_evidence_bindings with a shared sentinel constant or predicate defined by adversarial_evidence.py and imported here. Ensure the shared symbol identifies the “must cite the exact probe path and positive line” condition, preserving the existing repair and fail-closed behavior while preventing silent coupling to message wording.tests/test_opencode_model_pool_runner.py (1)
1050-1077: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGPT-5/DeepSeek runtime cap 테스트 중복.
test_github_deepseek_runtime_cap_preserves_queue_budget이 기존test_github_gpt5_runtime_cap_preserves_queue_budget(1024-1047)과 provider 이름·env 변수명만 다를 뿐 구조가 거의 동일합니다. PR 목표상 향후 DeepSeek V3·GPT-5.6 Luna·GPT-5/o3·DeepSeek R1 등 추가 provider별 유사 테스트가 늘어날 가능성이 높으므로,pytest.mark.parametrize로 통합하면 유지보수가 쉬워집니다.♻️ 제안 리팩터링
-def test_github_gpt5_runtime_cap_preserves_queue_budget(tmp_path: Path) -> None: - ... -def test_github_deepseek_runtime_cap_preserves_queue_budget(tmp_path: Path) -> None: - ... +@pytest.mark.parametrize( + ("model_candidate", "cap_env", "cap_seconds"), + [ + ("github-models/openai/gpt-5", "OPENCODE_GITHUB_GPT5_RUN_TIMEOUT_SECONDS", 3), + ( + "github-models/deepseek/deepseek-v3-0324", + "OPENCODE_GITHUB_DEEPSEEK_RUN_TIMEOUT_SECONDS", + 2, + ), + ], +) +def test_provider_runtime_cap_preserves_queue_budget( + tmp_path: Path, model_candidate: str, cap_env: str, cap_seconds: int +) -> None: + result = run_failed_model( + tmp_path, + model_candidates=model_candidate, + extra_env={cap_env: str(cap_seconds), "OPENCODE_RUN_TIMEOUT_SECONDS": "9"}, + ) + assert result.returncode == 1 + ...🤖 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 `@tests/test_opencode_model_pool_runner.py` around lines 1050 - 1077, 통합된 provider별 runtime cap 테스트로 중복을 제거하세요. 기존 test_github_gpt5_runtime_cap_preserves_queue_budget와 test_github_deepseek_runtime_cap_preserves_queue_budget를 pytest.mark.parametrize 기반의 단일 테스트로 합치고, 모델 후보와 provider별 환경 변수명 및 기대 로그를 매개변수화해 각 provider에 대한 동일한 검증을 유지하세요.
🤖 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 @.github/workflows/opencode-review.yml:
- Around line 3802-3804: Update the DeepSeek timeout configuration used by
cap_model_run_timeout() so the 300-second cap applies only to endpoints known to
return no usable provider detail, not the primary
github-models/deepseek/deepseek-v3-0324 reviewer. Preserve the primary
candidate’s normal 5400-second cadence or assign it a more generous limit, while
retaining bounded fallback behavior for the affected endpoints.
---
Outside diff comments:
In `@scripts/ci/run_opencode_review_model_pool.sh`:
- Line 552: Update the candidate-selection and termination flow around
should_skip_model_candidate, alive_candidates, dead_candidate_reasons, and
deadline so that when every candidate is skipped, the run immediately exits
instead of continuing cycles with max_cycles=0 and deadline=0; include skipped
candidates in the no-runnable-candidates decision or terminate directly when
alive_candidates is zero.
---
Nitpick comments:
In @.github/workflows/opencode-review.yml:
- Around line 174-212: Extract the duplicated trusted-ref resolution logic from
the three workflow blocks, including the step identified by
coverage_source_trusted, into one shared composite action or script. Update all
callers to use that shared implementation and standardize the interpreter
invocation, replacing the inconsistent /usr/bin/python3 -I versus python3 usage
while preserving the existing validation and fallback behavior.
In `@scripts/ci/opencode_review_normalize_output.py`:
- Around line 693-793: Replace the literal rejection-string comparison in
repair_adversarial_probe_evidence_bindings with a shared sentinel constant or
predicate defined by adversarial_evidence.py and imported here. Ensure the
shared symbol identifies the “must cite the exact probe path and positive line”
condition, preserving the existing repair and fail-closed behavior while
preventing silent coupling to message wording.
In `@scripts/ci/test_strix_quick_gate.sh`:
- Line 617: Update the regular expression in the assert_file_not_matches check
for "$CODEGRAPH_EVIDENCE_FILE" to allow any amount of leading whitespace instead
of requiring exactly 10 spaces, while preserving the existing command and
end-of-line matching.
- Around line 90-98: Update assert_file_not_matches so a missing target file
records a failure instead of silently passing, using record_failure with a clear
missing-file message; preserve the existing unexpected-pattern failure behavior
when the file exists.
In `@tests/test_opencode_model_pool_runner.py`:
- Around line 1050-1077: 통합된 provider별 runtime cap 테스트로 중복을 제거하세요. 기존
test_github_gpt5_runtime_cap_preserves_queue_budget와
test_github_deepseek_runtime_cap_preserves_queue_budget를 pytest.mark.parametrize
기반의 단일 테스트로 합치고, 모델 후보와 provider별 환경 변수명 및 기대 로그를 매개변수화해 각 provider에 대한 동일한 검증을
유지하세요.
🪄 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: Pro Plus
Run ID: 4d2ec5a4-3914-4115-b8cd-d4dae89e5217
📒 Files selected for processing (10)
.github/workflows/opencode-review.ymlrequirements-opencode-review-ci-hashes.txtrequirements-opencode-review-ci.txtrequirements-strix-ci.txtscripts/ci/opencode_review_normalize_output.pyscripts/ci/run_opencode_review_model_pool.shscripts/ci/test_strix_quick_gate.shtests/test_opencode_agent_contract.pytests/test_opencode_model_pool_runner.pytests/test_opencode_python_dependency_lock.py
변경 사항
통합 범위
검증
전체 Strix 셸 하네스는 로컬 pyenv shim 재생성 경합 때문에 별도 환경 실패가 발생했으며, PR CI의 격리된 러너에서 다시 검증합니다.
Summary by CodeRabbit
보안 및 안정성 개선
CI 및 커버리지 개선
의존성 및 테스트