Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow
Semantic Versioning where the repository publishes a release.

## [Unreleased]
- Close a 99% `scripts/ci` coverage regression on protected main: merged #1546 added an
uncovered `live_head_matches` helper, an uncovered no-active/no-stale-runs fall-through in
`prepare_autofix_slot`, and an uncovered "current-head autofix run is already queued or
running" wait path in `pr_review_fix_scheduler.py::inspect_pr`, while the pre-existing
conflicted-draft and conflicted-unauthorized `inspect_pr` returns and the REST
`fetch_workflow_names_by_check_suite_rest` pagination/name-filtering/permission-denied paths
in `pr_review_merge_scheduler.py` remained untested. Every PR rebasing onto main inherited
this failure via the `coverage-evidence` required check regardless of its own diff; this adds
test-only coverage for all of the above with no production code change.
- Avoid redundant merge-scheduler wakes when the trusted receipt predicate
already finds a substantive exact-head OpenCode verdict. Missing, stale, or
fallback-only evidence still dispatches review work, while receipt lookup or
Expand Down
48 changes: 48 additions & 0 deletions docs/product-technical-gap-baseline.md
Original file line number Diff line number Diff line change
Expand Up @@ -2344,6 +2344,54 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr
"today" reference. Landed in the same PR (`#1463`) as the streaming revert,
not split out, since the revert is unsafe without it.

## 2026-09-01 post-#1546 `scripts/ci` coverage regression on protected main: root-caused and closed

**Context**: `#1546` (merged, exact head `5686de41660d51a7a7f22b8840dfa6ccfe5ff3f1`) reconciled
unbounded exact-head review agents and, as part of a 90-line expansion of
`scripts/ci/pr_review_fix_scheduler.py`, added a `live_head_matches` helper, a no-active/no-stale
fall-through branch in `prepare_autofix_slot`, and an "already queued or running" wait branch in
`inspect_pr` — none of which any test exercised directly. This compounded a narrower, older gap in
the same file (`inspect_pr`'s conflicted-draft and conflicted-unauthorized returns) and in
`scripts/ci/pr_review_merge_scheduler.py::fetch_workflow_names_by_check_suite_rest` (pagination,
missing-suite-id/blank-name filtering, non-access-error propagation), first found and attempted in
now-closed, unmerged `#1547`/`#1551`/`#1554` — none of whose evidence or diffs transferred here;
this pass re-derived the current gap from a clean `origin/main` clone rather than assuming those
predecessors were still accurate against `#1546`'s shifted line numbers and new branches. Verified
directly: `coverage report --show-missing` on unmodified `main` showed
`scripts/ci/pr_review_fix_scheduler.py` at 97% (missing 116-121, 459->466, 495, 503, 546) and
`scripts/ci/pr_review_merge_scheduler.py` at 99% (missing 1003, 1008->1005, 1012) — total repo-wide
99%, below the `pyproject.toml` `fail_under = 100` gate. Because `opencode-review-dispatch.yml`'s
`coverage-evidence` job measures the **merged** PR tree (base + head) and hard-fails below 100%,
every PR rebasing onto main inherited this failure regardless of its own diff — org-wide impact,
not scoped to one PR.
Comment thread
seonghobae marked this conversation as resolved.

**Fix**: `#1567` (test-only, no production code) adds direct unit coverage for `live_head_matches`
(case-insensitive match, mismatch, malformed-payload paths), `prepare_autofix_slot`'s empty-run
fall-through, the `inspect_pr` conflicted-draft/conflicted-unauthorized/already-queued cases, and
the `fetch_workflow_names_by_check_suite_rest` pagination/filtering/error-propagation paths.
Verified on the fix commit (`db106d50f2134ece147bc5318e389aeb124d198c`): `coverage run -m pytest
tests -q` (2251 passed, 1 skipped, 21 subtests), `coverage report` (repo-wide 100%, both files
individually 100% statement and 100% branch), `interrogate` (100.0%).

**Devin Review raised a false positive on the fix itself**, claiming
`test_live_head_matches_compares_case_insensitively_and_fails_closed` left non-object-payload,
non-string-SHA, and wrong-length-SHA branches uncovered. Re-verified against the actual gate rather
than accepted at face value: `live_head_matches` has exactly one `if` statement (two arcs, both
exercised by the committed test), and its final `return (isinstance(...) and len(...) == 40 and
...)` is a single boolean expression with no `if`/`else` of its own — `coverage.py`'s branch mode
(what `fail_under = 100` actually measures here) tracks control-flow arcs between statements, not
sub-clause condition coverage within one expression. The cited cases are additional test
thoroughness, not something the gate is currently failing on; confirmed by a full-suite run on the
exact same head showing both files at 100% branch coverage with zero missing branches. Replied with
this evidence on the review thread and did not widen the PR's diff for a claim that does not hold
against this repo's own tooling.

**One test in the full suite remains a known, pre-existing flake**, unrelated to this change:
`tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate`
intermittently exits 141 (SIGPIPE) under full-suite parallel load; reproduces identically on
unmodified `origin/main` and passes cleanly in file isolation. Not remediated here — out of scope
for a coverage-gap-only PR, and not itself a coverage regression.

## 5. 실행 루프와 고객의 다음 행동

각 hourly pass는 아래 순서를 유지한다.
Expand Down
17 changes: 1 addition & 16 deletions scripts/ci/noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,24 +474,9 @@ def review_thread_context(pr: dict[str, Any]) -> str:
return "\n".join(lines)


def load_codegraph_context() -> str:
"""Load optional precomputed CodeGraph context for structural review evidence."""
path = os.environ.get("NOEMA_CODEGRAPH_CONTEXT_PATH", "").strip()
if not path:
return ""
try:
with open(path, encoding="utf-8") as handle:
return truncate_text(handle.read(), MAX_REVIEW_CONTEXT_CHARS)
except OSError as exc:
return f"CodeGraph context unavailable: {exc}"


def build_review_context(repo: str, number: int, pr: dict[str, Any]) -> str:
"""Build bounded non-diff context for the Noema reviewer."""
sections: list[str] = []
codegraph = load_codegraph_context()
if codegraph:
sections.append("## CodeGraph context\n" + codegraph)
threads = review_thread_context(pr)
if threads:
sections.append("## Prior review threads\n" + threads)
Expand Down Expand Up @@ -958,7 +943,7 @@ def call_llm(
"content": "\n".join(
[
"You are Noema, an independent pull request reviewer for ContextualWisdomLab.",
"Review the PR diff plus the additional changed-file, review-thread, and CodeGraph context for correctness, security, maintainability, and behavioral regressions.",
"Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.",
"Return only JSON with this shape:",
json.dumps(
{
Expand Down
15 changes: 2 additions & 13 deletions tests/test_noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -1378,7 +1378,7 @@ def test_current_actor_rejects_unbound_action_identity(monkeypatch, actor, insta
noema.current_actor()


def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch, tmp_path):
def test_review_context_builders_include_threads_and_files(monkeypatch):
assert noema.truncate_text("abc", 10) == "abc"
assert "truncated 2 characters" in noema.truncate_text("abcdef", 4)
assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "")
Expand All @@ -1405,9 +1405,6 @@ def fake_run(args, stdin=None):
raise AssertionError(args)

monkeypatch.setattr(noema, "run", fake_run)
codegraph_path = tmp_path / "codegraph.md"
codegraph_path.write_text("call graph: src/a.py -> tests", encoding="utf-8")
monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(codegraph_path))
pr = make_pr(
headRefOid="head sha",
reviewThreads={
Expand All @@ -1431,8 +1428,6 @@ def fake_run(args, stdin=None):

context = noema.build_review_context("owner/repo", 7, pr)

assert "## CodeGraph context" in context
assert "call graph: src/a.py -> tests" in context
assert "Thread open at src/a.py:3" in context
assert "reviewer: check call site" in context
assert "### src/a.py" in context
Expand All @@ -1442,13 +1437,7 @@ def fake_run(args, stdin=None):
assert any("/files" in call[2] for call in calls)


def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, tmp_path):
monkeypatch.delenv("NOEMA_CODEGRAPH_CONTEXT_PATH", raising=False)
assert noema.load_codegraph_context() == ""

monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(tmp_path / "missing.md"))
assert "CodeGraph context unavailable" in noema.load_codegraph_context()

def test_review_context_reports_omitted_files(monkeypatch):
paths = [f"src/file_{index}.py" for index in range(noema.MAX_CONTEXT_FILES + 1)]
monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths)
monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: "x")
Expand Down
1 change: 1 addition & 0 deletions tests/test_opencode_required_verdict_regression.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate(
elif [[ "$*" == *"/pulls/7/reviews"* ]]; then
printf '[%s]' "$FAKE_REVIEWS"
elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then
cat >/dev/null
Comment thread
seonghobae marked this conversation as resolved.
printf 'dispatch\n' >>"$DISPATCH_CALLS"
fi
""",
Expand Down
56 changes: 56 additions & 0 deletions tests/test_pr_review_fix_scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,40 @@ def test_prepare_autofix_slot_preserves_new_head_workers_after_head_advance(monk
workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY,
dry_run=False,
) is None


def test_prepare_autofix_slot_returns_directly_with_no_active_or_stale_runs(monkeypatch):
"""An empty Actions run list needs no reconciliation and skips cancellation."""
monkeypatch.setattr(fix, "run_json", lambda _args: {"workflow_runs": []})
monkeypatch.setattr(
fix,
"force_cancel_workflow_runs",
lambda *_args: pytest.fail("no stale runs must not attempt cancellation"),
)

assert fix.prepare_autofix_slot(
"owner/repo",
make_pr(),
workflow=fix.DEFAULT_AUTOFIX_WORKFLOW,
workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY,
dry_run=False,
) is False


def test_live_head_matches_compares_case_insensitively_and_fails_closed(monkeypatch):
"""Live head lookup normalizes case and rejects malformed or mismatched payloads."""
head = "a" * 40

monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": head.upper()}})
assert fix.live_head_matches("owner/repo", make_pr(headRefOid=head))

monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "b" * 40}})
assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head))

monkeypatch.setattr(fix, "run_json", lambda _args: {"nothead": {}})
assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head))
Comment thread
seonghobae marked this conversation as resolved.


def test_terminal_failed_check_triggers_rca_without_prior_opencode_review():
"""Exact-head check evidence can start RCA without a circular review prerequisite."""
pr = make_pr(
Expand Down Expand Up @@ -1318,6 +1352,13 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch):
assert fix.inspect_pr("owner/repo", make_pr(headRepository={"nameWithOwner": "fork/repo"}), args)[1] == (
"external PR head is not writable by repository workflow credentials",
)
assert fix.inspect_pr(
"owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args
) == ("skip", ("draft PR",))
assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == (
"skip",
("merge conflict is not authorized for repair",),
)

monkeypatch.setattr(fix, "needs_autofix", lambda pr: (False, ()))
assert fix.inspect_pr("owner/repo", make_pr(), args) == (
Expand All @@ -1329,6 +1370,21 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch):
monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [{"body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->"}])
assert fix.inspect_pr("owner/repo", make_pr(), args) == ("wait", ("recent autofix marker exists for this head",))

assert fix.inspect_pr(
"owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args
) == ("skip", ("draft PR",))
assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == (
"skip",
("merge conflict is not authorized for repair",),
)

monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [])
monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: True)
assert fix.inspect_pr("owner/repo", make_pr(), args) == (
"wait",
("current-head autofix run is already queued or running",),
)

pr1 = make_pr(number=1)
pr2 = make_pr(number=2)
monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr1, pr2])
Expand Down
70 changes: 70 additions & 0 deletions tests/test_pr_review_fix_scheduler_rest_workflow_identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,73 @@ def fake_api(path: str) -> Any:
assert merge.is_strix_context(context)
assert merge.strix_evidence_state(pr) == expected_state
assert fix.current_head_failed_checks(pr) == ()


def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100(
monkeypatch: Any,
) -> None:
"""A first page of exactly 100 runs must fetch a second page and merge both."""
head_sha = "e" * 40
page1 = [
{"check_suite_id": i, "name": f"workflow-{i}"} for i in range(100)
]
page2 = [{"check_suite_id": 100, "name": "workflow-100"}]
calls: list[str] = []

def fake_api(path: str) -> Any:
"""Return deterministic paginated workflow-run fixtures."""
calls.append(path)
if path.endswith("page=1"):
return {"workflow_runs": page1}
if path.endswith("page=2"):
return {"workflow_runs": page2}
raise AssertionError(f"unexpected path {path}")

monkeypatch.setattr(merge, "gh_api_json", fake_api)

names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha)

assert names == {i: f"workflow-{i}" for i in range(101)}
assert calls == [
f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=1",
f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=2",
]


def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id_or_name(
monkeypatch: Any,
) -> None:
"""A run with no check-suite id or a blank name must not populate the map."""
head_sha = "f" * 40

def fake_api(path: str) -> Any:
"""Return workflow runs that exercise incomplete-identity filtering."""
return {
"workflow_runs": [
{"check_suite_id": None, "name": "orphaned run"},
{"check_suite_id": 900, "name": ""},
{"check_suite_id": 901, "name": "kept run"},
]
}

monkeypatch.setattr(merge, "gh_api_json", fake_api)

names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha)

assert names == {901: "kept run"}


def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors(
monkeypatch: Any,
) -> None:
"""A page-fetch failure unrelated to integration access must fail closed."""
head_sha = "0" * 40

def fake_api(path: str) -> Any:
"""Simulate a non-access REST failure that must propagate."""
raise RuntimeError("gh: HTTP 502 (exhausted retries)")

monkeypatch.setattr(merge, "gh_api_json", fake_api)

with pytest.raises(RuntimeError, match="HTTP 502"):
merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha)
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,7 @@ def test_noema_review_context_includes_locations_bodies_and_all_sections(
assert "src/runtime.py:7" in rendered
assert "reviewer: Fix this" in rendered

monkeypatch.setattr(noema, "load_codegraph_context", lambda: "graph")
monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "files")
context = noema.build_review_context("owner/repo", 1, pr)
assert "CodeGraph context" in context
assert "Prior review threads" in context
assert "Changed file context" in context
1 change: 0 additions & 1 deletion tests/test_repository_branch_coverage_reporting_edges.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ def test_noema_small_diff_and_empty_context_branches(
rendered_context = noema.review_thread_context(pr)
assert rendered_context == "- Thread open at src/runtime.py:\n - reviewer: note"

monkeypatch.setattr(noema, "load_codegraph_context", lambda: "")
monkeypatch.setattr(noema, "review_thread_context", lambda _pr: "")
monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "")
assert noema.build_review_context("owner/repo", 1, pr) == ""
Expand Down
Loading