fix(noema): don't treat a deleted file's expected head-content 404 as a review-blocking error - #1524
fix(noema): don't treat a deleted file's expected head-content 404 as a review-blocking error#1524seonghobae wants to merge 1 commit into
Conversation
…as a review-blocking error Noema's required review check on #1486 (which deletes fuzz/fuzz_opencode_normalize_output.py) refused to complete a real verdict, citing "File content unavailable due to HTTP 404 error" as a high-severity finding and returning only COMMENT. Root cause: fetch_changed_file_paths() discarded each file's PR `status` field, so changed_file_context() fetched every changed file's content at the PR's own head SHA — including files whose status is "removed", which by definition cannot exist at head and always 404. changed_file_context() then reported that expected 404 with the same "Unavailable ... error" phrasing used for genuine anomalies, and the LLM reviewer reasonably read it as a real data-integrity problem serious enough to block a full review. Fix: add fetch_changed_files() to return each file's (path, status) from the same `gh api pulls/{n}/files` response, and special-case status "removed" in changed_file_context(): instead of probing head_sha (guaranteed 404), fetch the file's pre-deletion content at the PR's base ref (now carried via PR_QUERY's baseRefOid) so the reviewer sees what's being deleted. A genuine head-content fetch failure on a still-existing file is unchanged. fetch_changed_file_paths() itself is untouched and still serves its other caller (inspect_and_review's changed_paths for validate_substantive_verdict). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Required
|
|
Contextual-Orchestrator를 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
|
@opencode-agent review Review exact head |
|
Fresh protected-main revalidation after #1546: @opencode-agent fix and review |
|
Current-main compare remains restricted to |
Summary
Noema's required review check produced a real, live false-positive block on
ContextualWisdomLab/.github#1486(which deletesfuzz/fuzz_opencode_normalize_output.py, PR-status"removed"pergh api repos/ContextualWisdomLab/.github/pulls/1486/files). Instead of a real APPROVE/REQUEST_CHANGES verdict, Noema returned:Root cause:
fetch_changed_file_paths()(scripts/ci/noema_review_gate.py) only extracted each changed file'sfilenamefromgh api .../pulls/{number}/files, discarding thestatusfield the same response already includes (added/modified/removed/renamed).changed_file_context()then unconditionally calledfetch_head_file_content()for every changed file at the PR's own head SHA — but a file whose PR status is"removed"cannot exist at head by definition, so this fetch was guaranteed to 404, every time, for every PR that deletes any file.fetch_head_file_contentraised on the 404, andchanged_file_context'sexcept RuntimeErrorwrapped it as"Unavailable from head content API: {reason}"— phrasing indistinguishable from a genuine anomaly. The LLM reviewer, seeing that framing, reasonably (but wrongly) treated an entirely expected file-deletion scenario as a high-severity data-integrity problem and refused a full review. Since this org's hollow-path-audit lineage regularly deletes dead-code files, this could affect any such PR's required Noema review, not just #1486.Fix
fetch_changed_files(repo, number) -> list[tuple[str, str]], which extracts(filename, status)pairs from the samegh api pulls/{number}/filesresponse (--jq '.[] | .filename + "\t" + .status').fetch_changed_file_paths()itself is untouched — it keeps its existinglist[str]return type and its other caller (inspect_and_review'schanged_paths, used forvalidate_substantive_verdict's adversarial-probe location checks) needed no change.changed_file_context()now takes each file's status fromfetch_changed_files(). For a"removed"file it no longer probeshead_sha(guaranteed 404, zero signal). Instead — per this repo's own evidence-gated review philosophy (CLAUDE.md: OpenCode/Noema approval requires concrete, observed evidence, not the absence of a signal) — it fetches the file's pre-deletion content at the PR's base ref via the newremoved_file_context_section()helper, so the reviewer can actually judge whether the deletion is safe.PR_QUERYnow also fetchesbaseRefOidalongside the existingheadRefOid, sobuild_review_context()can pass the base SHA through with no extra API call. If no base SHA is available, or the base-ref fetch itself fails, a clear non-alarming note is emitted instead (no "Unavailable from head content API" framing)."added"files are unaffected (they exist at head, no special-casing needed)."renamed"files are also unaffected — GitHub's Files API reports their current (post-rename)filename/status, which already worked correctly againsthead_shabefore this change."modified"file whose head-content fetch actually fails) is unchanged and still surfaces as"Unavailable from head content API: {reason}".Developer experience
tests/test_noema_review_gate.py:test_fetch_changed_files_parses_path_and_status— unit coverage for the new(path, status)parser.test_fetch_changed_file_paths_parses_plain_filenames— keeps the untouched original function's real body covered now thatchanged_file_contextno longer calls through it.test_changed_file_context_removed_file_uses_base_content_not_head_error— reproduces PR chore(fuzz): remove dead duplicate fuzz target #1486's exact scenario (aremovedfuzz file) and asserts the output contains neither the old "Unavailable from head content API" framing nor a generic error, and instead surfaces the file's pre-deletion content.test_changed_file_context_removed_file_without_base_sha,test_changed_file_context_removed_file_base_fetch_failure,test_changed_file_context_removed_file_base_content_empty— the three non-happy-path branches of the new removed-file handling.test_changed_file_context_non_removed_head_fetch_failure_is_unchanged— pins that a genuine head-content failure on a file that still exists keeps the original error message.test_build_review_context_forwards_base_sha_to_changed_file_context— confirmsbuild_review_contextthreadspr["baseRefOid"]through.coverage run -m pytest tests -q && coverage report --show-missing: 2134 passed, 1 skipped, 21 subtests passed; 100% coverage repo-wide, includingscripts/ci/noema_review_gate.py.interrogate:RESULT: PASSED (minimum: 100.0%, actual: 100.0%).*-hashes.txtfiles touched.User experience
Test plan
coverage run -m pytest tests -q— 2134 passed, 1 skipped, 21 subtests passedcoverage report --show-missing— 100% overall, 100% onscripts/ci/noema_review_gate.pyinterrogate— 100%, PASSED🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code