diff --git a/CHANGELOG.md b/CHANGELOG.md index 43020db98e..848cec87a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Exclude models with declared non-text input requirements from the Contextual + Orchestrator review catalog before cost, privacy, and account selection. + Plain-text Chat Completions, Responses, and structured-output review paths + remain eligible, while the existing multi-hour per-model review budgets are + unchanged (#1415). - 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/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index f115ef2b88..f6d2bbffd5 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -105,6 +105,29 @@ def _has_text_output(model: object) -> bool: return not modalities or "text" in {str(modality).casefold() for modality in modalities} +def _is_text_review_candidate( + model: object, + *, + is_general_chat_agent_model_id: Any, + requires_non_text_input: Any, +) -> bool: + """Return whether discovery evidence permits plain-text review traffic. + + Unknown input modality remains eligible because absence of catalog evidence + is not evidence of a non-text requirement. Any declared non-text input is + excluded conservatively before cost, privacy, or account selection can + admit the route. Chat and structured-output capability metadata otherwise + remain untouched for the gateway's Responses and Chat Completions paths. + """ + model_id = getattr(model, "model_id", "") + input_modalities = getattr(model, "input_modalities", ()) or () + return bool( + is_general_chat_agent_model_id(model_id) + and _has_text_output(model) + and not requires_non_text_input(input_modalities) + ) + + _DISCOVERY_DIAGNOSTICS_COMPLETE_SENTINEL = "discovery_diagnostics_complete" @@ -796,7 +819,10 @@ def main(argv: list[str] | None = None) -> int: args = parser.parse_args(argv) from contextual_orchestrator.credentials import get_credential - from contextual_orchestrator.chat_capability import is_general_chat_agent_model_id + from contextual_orchestrator.chat_capability import ( + is_general_chat_agent_model_id, + requires_non_text_input, + ) from contextual_orchestrator.model_discovery import discover_all_models, free_discovered_models from contextual_orchestrator.orchestrator import ModelClient, TaskOrchestrator, load_agents from contextual_orchestrator.review_gateway import ( @@ -834,8 +860,11 @@ def main(argv: list[str] | None = None) -> int: free_route_identities = frozenset(_route_identity(model) for model in free_models) selected_models = [] for model in routable_discovered: - model_id = getattr(model, "model_id", "") - if not is_general_chat_agent_model_id(model_id) or not _has_text_output(model): + if not _is_text_review_candidate( + model, + is_general_chat_agent_model_id=is_general_chat_agent_model_id, + requires_non_text_input=requires_non_text_input, + ): continue if args.pool == "free" and _route_identity(model) not in free_route_identities: continue diff --git a/tests/test_contextual_orchestrator_review_sidecar_contract.py b/tests/test_contextual_orchestrator_review_sidecar_contract.py index 0a63356dad..80c1f4e449 100644 --- a/tests/test_contextual_orchestrator_review_sidecar_contract.py +++ b/tests/test_contextual_orchestrator_review_sidecar_contract.py @@ -279,7 +279,9 @@ def test_launcher_registers_secrets_into_the_kv_once() -> None: def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: """Discovery, price evidence, and serving come from the vendored library.""" text = _read(LAUNCHER) - assert "from contextual_orchestrator.chat_capability import is_general_chat_agent_model_id" in text + assert "from contextual_orchestrator.chat_capability import (" in text + assert "is_general_chat_agent_model_id," in text + assert "requires_non_text_input," in text assert "from contextual_orchestrator.model_discovery import discover_all_models, free_discovered_models" in text assert "routable_discovered = _routable_discovered_models(discovered)" in text assert "free_discovered_models(routable_discovered)" in text @@ -287,8 +289,9 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert 'getattr(model, "output_modalities", None)' in text assert 'isinstance(modalities, str)' in text assert '"text" in {str(modality).casefold() for modality in modalities}' in text - assert "not _has_text_output(model)" in text - assert 'model_id = getattr(model, "model_id", "")' in text + assert "_is_text_review_candidate(" in text + assert 'input_modalities = getattr(model, "input_modalities", ()) or ()' in text + assert "not requires_non_text_input(input_modalities)" in text launcher = runpy.run_path(str(LAUNCHER)) has_text_output = launcher["_has_text_output"] @@ -323,6 +326,61 @@ def test_launcher_uses_orchestrator_discovery_and_governed_pools() -> None: assert "from scripts.ci import zdr_policy" in text +def test_launcher_admits_only_plain_text_review_candidates() -> None: + """Review discovery keeps text protocols while excluding multimodal rows.""" + launcher = runpy.run_path(str(LAUNCHER)) + is_candidate = launcher["_is_text_review_candidate"] + + def general_chat(model_id: str) -> bool: + return model_id.startswith("chat-") + + def needs_non_text(modalities: object) -> bool: + return any(str(value).casefold() != "text" for value in modalities) + + compatible = SimpleNamespace( + model_id="chat-structured", + input_modalities=("text",), + output_modalities=("text",), + capabilities=("chat", "response_format"), + ) + unknown_input = SimpleNamespace( + model_id="chat-undocumented", + input_modalities=(), + output_modalities=("text",), + ) + multimodal = SimpleNamespace( + model_id="chat-multimodal", + input_modalities=("text", "image"), + output_modalities=("text",), + ) + media_output = SimpleNamespace( + model_id="chat-media-output", + input_modalities=("text",), + output_modalities=("image",), + ) + + assert is_candidate( + compatible, + is_general_chat_agent_model_id=general_chat, + requires_non_text_input=needs_non_text, + ) + assert is_candidate( + unknown_input, + is_general_chat_agent_model_id=general_chat, + requires_non_text_input=needs_non_text, + ) + assert not is_candidate( + multimodal, + is_general_chat_agent_model_id=general_chat, + requires_non_text_input=needs_non_text, + ) + assert not is_candidate( + media_output, + is_general_chat_agent_model_id=general_chat, + requires_non_text_input=needs_non_text, + ) + + def test_launcher_wraps_catalog_for_vendored_load_agents() -> None: """Persist the catalog envelope expected by the pinned orchestrator loader.""" text = _read(LAUNCHER) diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 3b4416bdc3..0120a4a108 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -1142,6 +1142,16 @@ 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", + _approved_dirty_pr(isDraft=True), + args, + ) == ("skip", ("draft PR",)) + monkeypatch.setattr(fix, "needs_conflict_resolution", lambda pr, **kwargs: (False, ())) + assert fix.inspect_pr("owner/repo", _approved_dirty_pr(), 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_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index c24cfb05f9..2c96afd0b4 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -10,6 +10,45 @@ from scripts.ci import pr_review_merge_scheduler as merge +def test_workflow_name_rest_fallback_paginates_and_skips_incomplete_rows( + monkeypatch: Any, +) -> None: + """Workflow identity pagination ignores rows without a complete binding.""" + calls: list[str] = [] + + def fake_api(path: str) -> Any: + calls.append(path) + if path.endswith("&page=1"): + return { + "workflow_runs": [ + {"check_suite_id": 7, "name": "Required OpenCode Review"}, + *({"check_suite_id": None, "name": ""} for _ in range(99)), + ] + } + return {"workflow_runs": []} + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + assert merge.fetch_workflow_names_by_check_suite_rest( + "owner/repo", "a" * 40 + ) == {7: "Required OpenCode Review"} + assert len(calls) == 2 + assert "page=2" in calls[-1] + + +def test_workflow_name_rest_fallback_propagates_unexpected_errors( + monkeypatch: Any, +) -> None: + """Only permission denial degrades to unknown workflow identity.""" + def fail(_path: str) -> Any: + raise RuntimeError("unexpected transport failure") + + monkeypatch.setattr(merge, "gh_api_json", fail) + + with pytest.raises(RuntimeError, match="unexpected transport failure"): + merge.fetch_workflow_names_by_check_suite_rest("owner/repo", "b" * 40) + + def test_rest_fallback_preserves_renamed_opencode_workflow_identity( monkeypatch: Any, ) -> None: