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/CHANGELOG.md b/CHANGELOG.md index 43020db98e..67f3f26f1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Accept upstream OpenCode's own `/opencode` and `/oc` trigger phrases as aliases of the local + `@opencode-agent` mention in `agent_mention_router.py`'s `MENTION_PATTERNS` (and, transitively, + `agent_mention_sweep.py`'s scheduled organization sweep, which imports the same matcher), plus the + `agent-mention-router.yml` pre-filter. A commenter following OpenCode's public GitHub Action docs + (which document `/opencode`/`/oc`, not this org's locally-invented `@opencode-agent` mention) now + successfully dispatches the same request instead of silently triggering nothing. +- Fix the `@cwl-noema-review/@opencode-agent` bare-slash separator alternative in + `agent_mention_router.py`'s `MENTION_PATTERNS`: it previously checked only that the slash + immediately preceding `/@opencode-agent` was preceded by the literal text `@cwl-noema-review`, + without checking that occurrence's own left boundary, so invalid pasted text embedding the Noema + mention in a larger token (`foo@cwl-noema-review/@opencode-agent`, + `docs/@cwl-noema-review/@opencode-agent`) still dispatched an unintended OpenCode review. The + alternative now matches the whole `@cwl-noema-review/@opencode-agent` literal under the same + left-boundary exclusion as the standalone `@opencode-agent` alternative. - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. 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 index ee9232ebd5..b67b8a9212 100755 --- a/scripts/ci/agent_mention_router.py +++ b/scripts/ci/agent_mention_router.py @@ -15,13 +15,39 @@ 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. 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( "payload", [ diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 3b4416bdc3..90a0169f86 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -1142,6 +1142,13 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): """Inspect and queue logic report skip, wait, dispatch-limit, and errors.""" args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main"]) assert fix.inspect_pr("owner/repo", make_pr(isDraft=True), args) == ("skip", ("draft PR",)) + 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",), + ) assert fix.inspect_pr("owner/repo", make_pr(baseRefName="develop"), args)[1][0].startswith("base branch") wildcard_args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "*"]) monkeypatch.setattr(fix, "needs_autofix", lambda pr: (False, ())) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 919566aeb2..a3e9547e63 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -717,6 +717,54 @@ def fake_api(path): sched.fetch_all_pr_reviews_rest("owner/repo", 7) +def test_fetch_workflow_names_by_check_suite_rest_paginates_and_filters_incomplete_rows(monkeypatch): + """The REST workflow-name lookup must paginate and skip unusable rows.""" + page1_runs = [{"check_suite_id": i, "name": f"workflow-{i}"} for i in range(99)] + page1_runs.append({"check_suite_id": 99, "name": ""}) + calls = [] + + def fake_api(path): + calls.append(path) + if path.endswith("page=1"): + return {"workflow_runs": page1_runs} + return {"workflow_runs": [{"check_suite_id": 100, "name": "opencode-review"}]} + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + names = sched.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_fetch_workflow_names_by_check_suite_rest_returns_empty_map_when_resource_inaccessible(monkeypatch): + """A denied Actions read must fail closed to an empty map, not raise.""" + + def fake_api(path): + raise RuntimeError("Resource not accessible by integration") + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + assert sched.fetch_workflow_names_by_check_suite_rest("owner/repo", "a" * 40) == {} + + +def test_fetch_workflow_names_by_check_suite_rest_propagates_other_failures(monkeypatch): + """A transient REST failure unrelated to permissions must not be swallowed.""" + + def fake_api(path): + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + with pytest.raises(RuntimeError, match="HTTP 502"): + sched.fetch_workflow_names_by_check_suite_rest("owner/repo", "a" * 40) + + def test_fetch_pr_pagination_recovers_independent_approval_past_100_reviews(monkeypatch): """End-to-end regression for the reported bug: a genuine independent APPROVED review made early in a PR's life must still satisfy