diff --git a/.github/workflows/agent-mention-router.yml b/.github/workflows/agent-mention-router.yml index 43fb163975..a109c8a97c 100644 --- a/.github/workflows/agent-mention-router.yml +++ b/.github/workflows/agent-mention-router.yml @@ -23,6 +23,8 @@ jobs: && ( contains(github.event.comment.body, '@cwl-noema-review') || contains(github.event.comment.body, '@opencode-agent') + || contains(github.event.comment.body, '/opencode') + || contains(github.event.comment.body, '/oc') ) concurrency: group: review-agent-mention-router-local-${{ github.repository }} diff --git a/docs/automation/review-agent-comment-invocation.md b/docs/automation/review-agent-comment-invocation.md index a886caa967..926249b563 100644 --- a/docs/automation/review-agent-comment-invocation.md +++ b/docs/automation/review-agent-comment-invocation.md @@ -1,13 +1,13 @@ # Review-agent comment invocation -Updated: 2026-08-22 +Updated: 2026-09-01 ## Purpose Trusted ContextualWisdomLab maintainers can invoke the existing review planes from a pull-request conversation: - `@cwl-noema-review` requests the independent Noema review. -- `@opencode-agent` requests a bounded current-head OpenCode review only; the invocation itself disables branch updates, automatic merge, and direct merge. +- `@opencode-agent` (or upstream OpenCode's own `/opencode`/`/oc` comment triggers, accepted as aliases of the same request) requests a bounded current-head OpenCode review only; the invocation itself disables branch updates, automatic merge, and direct merge. The router never checks out or executes pull-request-controlled code. It reads live PR metadata, binds the request to the current head SHA and base branch, and dispatches the already deployed central workflows in `ContextualWisdomLab/.github`. diff --git a/scripts/ci/agent_mention_router.py b/scripts/ci/agent_mention_router.py old mode 100755 new mode 100644 index ee9232ebd5..498b885c27 --- a/scripts/ci/agent_mention_router.py +++ b/scripts/ci/agent_mention_router.py @@ -15,13 +15,49 @@ CENTRAL_AUTOMATION_REPOSITORY = "ContextualWisdomLab/.github" TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# "opencode-agent" also accepts /opencode and /oc: upstream OpenCode's own +# GitHub Action documents those as its trigger phrases +# (https://open-code.ai/en/docs/github), and this repo's dispatch pipeline +# accepts them as aliases of the same @opencode-agent request rather than +# forcing commenters to learn a locally-invented mention instead. +# +# None of the three alternatives below may be preceded by a bare "/": a +# preceding slash almost always means the match is embedded in a URL path +# (e.g. https://opencode.ai/docs, https://youtube.com/@opencode-agent) or an +# ordinary path segment (docs/@opencode-agent), not a deliberate trigger. The +# one deliberate exception is a maintainer separating both supported agent +# requests with a bare slash and no space (@cwl-noema-review/@opencode-agent). +# That case is matched as one combined literal — "@cwl-noema-review/@opencode-agent" +# — guarded by the same left-boundary exclusion as the standalone +# "@opencode-agent" alternative. A boundary check on the trailing slash alone +# is not enough: it would still fire for invalid pasted text where +# "@cwl-noema-review" is itself embedded in a larger token (e.g. +# foo@cwl-noema-review/@opencode-agent, docs/@cwl-noema-review/@opencode-agent) +# without checking that the Noema mention has a valid left boundary of its own. +# The bare /opencode and /oc forms additionally exclude a preceding "=": a +# URL query string (?next=/opencode, ?redirect=/oc) shares the same "not +# preceded by a word character" shape as a deliberate standalone command. +# +# The shared trailing boundary after any of the three alternatives also +# excludes a following "/": without it, a root-relative path continuation +# right after the alias (/oc/config, /opencode/docs, @opencode-agent/config, +# @cwl-noema-review/@opencode-agent/foo) still matched, since the alias text +# itself is a complete, valid match and nothing in the original trailing +# lookahead treated "/" as a word character. This mirrors the +# leading-boundary "/" exclusion already applied above and closes the same +# false-positive class from the trailing side, for every alternative rather +# than only the bare /opencode and /oc forms. MENTION_PATTERNS = { "cwl-noema-review": re.compile( r"(? None: assert module.exact_mentions("@opencode-agent-evil @cwl-noema-review2") == () +@pytest.mark.parametrize( + "body", + [ + "/opencode please re-review", + "/oc please re-review", + "kicking off /oc", + "/OC", + "/OpenCode", + ], +) +def test_exact_mentions_accepts_slash_opencode_aliases(body: str) -> None: + """Upstream OpenCode's own /opencode and /oc trigger phrases also dispatch.""" + + module = load_module() + assert module.exact_mentions(body) == ("opencode-agent",) + + +def test_exact_mentions_accepts_at_mention_after_a_slash_separator() -> None: + """A slash used to separate two agent requests must not swallow the @mention. + + Devin/owner review regression on #1537, across three rounds: + + 1. Excluding a preceding ``/`` from the lookbehind to reject + documentation-link false positives (see + ``test_exact_mentions_rejects_slash_opencode_substrings``) was + originally applied to the whole ``@opencode-agent|/opencode|/oc`` + alternation, so a maintainer separating both requested agents with a + bare slash and no space (``@cwl-noema-review/@opencode-agent``) + silently lost the OpenCode request. + 2. Simply exempting the ``@`` form from the slash exclusion reopened the + same false-positive class for ``/@opencode-agent`` embedded in an + arbitrary URL or path segment. + 3. Recognizing ``/@opencode-agent`` only when the slash is immediately + preceded by the other pattern's exact literal mention text + (``@cwl-noema-review``) checked only the boundary of the trailing + slash, not whether that ``@cwl-noema-review`` occurrence itself has a + valid left boundary, so invalid pasted text such as + ``foo@cwl-noema-review/@opencode-agent`` still dispatched OpenCode + (see ``test_exact_mentions_rejects_invalid_separator_prefixes``). + + The final pattern matches the whole separator form + (``@cwl-noema-review/@opencode-agent``) as one literal, guarded by the + same left-boundary exclusion as the standalone ``@opencode-agent`` + alternative. + """ + + module = load_module() + assert module.exact_mentions("@cwl-noema-review/@opencode-agent") == ( + "cwl-noema-review", + "opencode-agent", + ) + + +@pytest.mark.parametrize( + "body", + [ + "foo@cwl-noema-review/@opencode-agent", + "docs/@cwl-noema-review/@opencode-agent", + "user.name@cwl-noema-review/@opencode-agent", + ], +) +def test_exact_mentions_rejects_invalid_separator_prefixes(body: str) -> None: + """The combined separator literal must not fire when embedded in a larger token. + + Fifth-round finding on #1537, reported directly by the repository owner + (not a review bot): the separator alternative + ``(?<=@cwl-noema-review)/@opencode-agent`` only checked the literal text + immediately before the slash, not whether that ``@cwl-noema-review`` + occurrence itself has a valid left boundary. Pasted text embedding the + Noema mention inside a larger token — a preceding word + (``foo@cwl-noema-review/@opencode-agent``), a path segment + (``docs/@cwl-noema-review/@opencode-agent``), or an email-like local part + (``user.name@cwl-noema-review/@opencode-agent``) — still dispatched an + unintended OpenCode review. The fix matches the whole + ``@cwl-noema-review/@opencode-agent`` literal with the same left-boundary + exclusion as the standalone ``@opencode-agent`` alternative, so it no + longer fires unless the combined mention itself starts at a valid + boundary. Some of these inputs still independently match the unrelated, + pre-existing ``cwl-noema-review`` pattern (e.g. a preceding ``/`` is not + excluded there); that pattern predates this PR and is out of scope for + this fix, so only the OpenCode dispatch is asserted here. + """ + + module = load_module() + assert "opencode-agent" not in module.exact_mentions(body) + + +@pytest.mark.parametrize( + "body", + [ + "the /occupied seat", + "visit /oceanography for more", + "see /opencode-docs for the guide", + "check out https://opencode.ai/docs for more info", + "see http://open-code.ai/en/docs/github", + "share this: https://youtube.com/@opencode-agent", + "see docs/@opencode-agent for the config file", + "visit https://example.com/?next=/opencode for the redirect", + "visit https://example.com/?next=/oc for the redirect", + ], +) +def test_exact_mentions_rejects_slash_opencode_substrings(body: str) -> None: + """A longer token merely starting with /oc or /opencode is not a mention. + + Includes a URL whose path component happens to embed ``/opencode`` right + after the scheme's own ``//`` (Devin review finding on #1537): the prior + lookbehind excluded a preceding letter/digit/underscore/hyphen but not a + preceding ``/``, so a documentation link like ``https://opencode.ai`` + satisfied it and could launch an unintended review. Also includes a + second-round Devin finding on the same PR: restoring plain recognition of + ``@opencode-agent`` after a bare slash (so a maintainer could write + ``@cwl-noema-review/@opencode-agent`` with no space) reopened the same + class of false positive for ``/@opencode-agent`` embedded in an arbitrary + URL or path segment, since both share the exact same "word char, then + slash, then the mention" shape as the deliberate separator case. A third + finding (CodeRabbit, same PR) noted the slash-preceded exclusion for the + bare ``/opencode``/``/oc`` forms did not also exclude a preceding ``=``, + so a URL query string such as ``?next=/opencode`` or ``?next=/oc`` still + matched. + """ + + module = load_module() + assert module.exact_mentions(body) == () + + +@pytest.mark.parametrize( + "body", + [ + "/oc/config", + "/opencode/docs", + "@opencode-agent/config", + "@cwl-noema-review/@opencode-agent/foo", + ], +) +def test_exact_mentions_rejects_trailing_path_continuation(body: str) -> None: + """A root-relative path continuation right after the alias is not a mention. + + Sixth-round finding on #1537's successor PR (Devin): the shared trailing + boundary after all three ``opencode-agent`` alternatives excluded a + following letter, digit, underscore, or hyphen but not a following + ``/``, so a root-relative path glued directly onto the alias — ``/oc`` + followed by ``/config``, ``/opencode`` followed by ``/docs``, or even + ``@opencode-agent`` or the ``@cwl-noema-review/@opencode-agent`` + separator followed by ``/config`` or ``/foo`` — still matched as a + complete, valid mention, since nothing treated the alias text itself as + incomplete just because a slash continued right after it. The fix adds + ``/`` to the shared trailing exclusion, mirroring the leading-boundary + ``/`` exclusion already applied to each alternative from the other side. + """ + + module = load_module() + assert "opencode-agent" not in module.exact_mentions(body) + + @pytest.mark.parametrize( "payload", [ diff --git a/tests/test_agent_mention_router_slash_path_regression.py b/tests/test_agent_mention_router_slash_path_regression.py new file mode 100644 index 0000000000..e10bc224e1 --- /dev/null +++ b/tests/test_agent_mention_router_slash_path_regression.py @@ -0,0 +1,39 @@ +"""Regression coverage for root-relative OpenCode slash-command lookalikes.""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from types import ModuleType + +ROOT = Path(__file__).resolve().parents[1] +MODULE_PATH = ROOT / "scripts" / "ci" / "agent_mention_router.py" + + +def load_module() -> ModuleType: + """Load the production mention router from its script path.""" + + module_name = "agent_mention_router_slash_path_regression" + spec = importlib.util.spec_from_file_location(module_name, MODULE_PATH) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + spec.loader.exec_module(module) + return module + + +def test_root_relative_paths_do_not_dispatch_opencode_aliases() -> None: + """Path-like suffixes must not be accepted as standalone slash commands.""" + + module = load_module() + assert module.exact_mentions("/oc/config") == () + assert module.exact_mentions("/opencode/docs") == () + + +def test_standalone_slash_aliases_remain_supported() -> None: + """The path guard must preserve both documented standalone aliases.""" + + module = load_module() + assert module.exact_mentions("/oc") == ("opencode-agent",) + assert module.exact_mentions("/opencode please review") == ("opencode-agent",) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8f8047ff10..0e5d30805b 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -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 printf 'dispatch\n' >>"$DISPATCH_CALLS" fi """, diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 9860eeaec7..daeefae573 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -177,6 +177,92 @@ 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_same_head_with_no_stale_workers(monkeypatch): + """No stale workers means the cancellation branch is never entered.""" + head = "a" * 40 + monkeypatch.setattr( + fix, + "run_json", + lambda _args: { + "workflow_runs": [ + { + "id": 1, + "status": "in_progress", + "display_title": f"PR Review Autofix owner/repo#7@{head}", + } + ] + }, + ) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("there is no stale worker to cancel"), + ) + monkeypatch.setattr( + fix, + "live_head_matches", + lambda *_args: pytest.fail( + "staleness is only ever checked when a stale worker exists" + ), + ) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(headRefOid=head), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) + + +def test_live_head_matches_compares_the_live_head_to_the_cached_snapshot(monkeypatch): + """The real head-matching implementation reads GitHub's current PR head. + + Every other test in this module monkeypatches ``live_head_matches`` away, + so its own body (the ``gh api`` read, the payload-shape guard, and the + case-insensitive comparison) was never exercised by the suite at all. + """ + pr = make_pr(headRefOid="a" * 40) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "A" * 40}}) + assert fix.live_head_matches("owner/repo", pr) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "b" * 40}}) + assert not fix.live_head_matches("owner/repo", pr) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "short"}}) + assert not fix.live_head_matches("owner/repo", pr) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": None}}) + assert not fix.live_head_matches("owner/repo", pr) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": "not-a-dict"}) + assert not fix.live_head_matches("owner/repo", pr) + + monkeypatch.setattr(fix, "run_json", lambda _args: "not-a-dict") + assert not fix.live_head_matches("owner/repo", pr) + + +def test_inspect_pr_reports_active_autofix_worker_without_dispatch(monkeypatch): + """An already-running current-head worker waits instead of re-dispatching.""" + args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main"]) + monkeypatch.setattr(fix, "needs_autofix", lambda _pr: (True, ("review",))) + monkeypatch.setattr(fix, "issue_comments", lambda _repo, _number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: True) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda *_args, **_kwargs: pytest.fail("an active worker must not be redispatched"), + ) + + assert fix.inspect_pr("owner/repo", make_pr(), args) == ( + "wait", + ("current-head autofix run is already queued or running",), + ) + + 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( diff --git a/tests/test_scheduler_1541_coverage_regressions.py b/tests/test_scheduler_1541_coverage_regressions.py new file mode 100644 index 0000000000..3d03ce56db --- /dev/null +++ b/tests/test_scheduler_1541_coverage_regressions.py @@ -0,0 +1,102 @@ +"""Regression coverage for the scheduler branches introduced by PR #1541.""" + +from __future__ import annotations + +import pytest + +from scripts.ci import pr_review_fix_scheduler as fix +from scripts.ci import pr_review_merge_scheduler as merge + + +def _pr(**overrides: object) -> dict[str, object]: + """Return a minimal same-repository pull-request fixture.""" + value: dict[str, object] = { + "number": 7, + "isDraft": False, + "baseRefName": "main", + "baseRefOid": "b" * 40, + "headRefName": "feature", + "headRefOid": "a" * 40, + "headRepository": {"nameWithOwner": "owner/repo"}, + "mergeStateStatus": "CLEAN", + "reviews": {"nodes": []}, + "reviewThreads": {"nodes": []}, + } + value.update(overrides) + return value + + +def test_conflicted_draft_skips_before_repair_authority() -> None: + """A conflicted draft remains a draft skip rather than an RCA dispatch.""" + args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main"]) + + assert fix.inspect_pr( + "owner/repo", + _pr(isDraft=True, mergeStateStatus="DIRTY"), + args, + ) == ("skip", ("draft PR",)) + + +def test_conflicted_unapproved_pr_fails_closed_without_repair_authority() -> None: + """A conflict without explicit unreviewed-repair authority stays closed.""" + args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main"]) + + assert fix.inspect_pr( + "owner/repo", + _pr(mergeStateStatus="DIRTY"), + args, + ) == ("skip", ("merge conflict is not authorized for repair",)) + + +def test_workflow_name_rest_fallback_paginates_and_filters_rows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Workflow identity pagination keeps only rows with usable names.""" + page_one = [{"check_suite_id": index, "name": f"workflow-{index}"} for index in range(99)] + page_one.append({"check_suite_id": 99, "name": ""}) + calls: list[str] = [] + + def fake_api(path: str) -> dict[str, object]: + calls.append(path) + if path.endswith("page=1"): + return {"workflow_runs": page_one} + return {"workflow_runs": [{"check_suite_id": 100, "name": "opencode-review"}]} + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", "a" * 40) + + assert names[0] == "workflow-0" + assert 99 not in names + assert names[100] == "opencode-review" + assert calls == [ + f"repos/owner/repo/actions/runs?head_sha={'a' * 40}&per_page=100&page=1", + f"repos/owner/repo/actions/runs?head_sha={'a' * 40}&per_page=100&page=2", + ] + + +def test_workflow_name_rest_fallback_treats_permission_denial_as_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An inaccessible Actions inventory returns an empty fail-closed map.""" + + def fake_api(_path: str) -> dict[str, object]: + raise RuntimeError("Resource not accessible by integration") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + assert merge.fetch_workflow_names_by_check_suite_rest("owner/repo", "a" * 40) == {} + + +def test_workflow_name_rest_fallback_propagates_unrelated_failures( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Transport failures other than permission denial remain visible.""" + + def fake_api(_path: str) -> dict[str, object]: + 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", "a" * 40)