diff --git a/lib/cli/src/crewai_cli/eval_crew.py b/lib/cli/src/crewai_cli/eval_crew.py index 308def5d6..6ef6b5869 100644 --- a/lib/cli/src/crewai_cli/eval_crew.py +++ b/lib/cli/src/crewai_cli/eval_crew.py @@ -31,6 +31,7 @@ from crewai_core.settings import Settings from dotenv import load_dotenv, set_key import httpx from rich.console import Console +from rich.text import Text from crewai_cli.authentication.token import AuthError, get_auth_token from crewai_cli.plus_api import PlusAPI @@ -63,17 +64,22 @@ def eval_crew(run_id: str | None = None) -> None: recorded_amp = str(record.get("amp_base_url") or "").rstrip("/") if not run_id and recorded_amp and recorded_amp != client.base_url.rstrip("/"): console.print( - f"The run was traced to {recorded_amp}; evaluating at the configured AMP {client.base_url}.", + Text( + f"The run was traced to {recorded_amp}; evaluating at the configured AMP {client.base_url}." + ), style="yellow", ) started = _start_evaluation(client, execution_id) url = started.get("url") - console.print(f"Evaluating run [bold]{execution_id}[/bold]") + console.print(Text("Evaluating run ").append(execution_id, style="bold")) if url: - console.print(f"Follow it at [cyan underline]{url}[/cyan underline]") + # Appended, never interpolated: this line invites a click, so a `url` + # carrying `[link=…]` would print a trustworthy label over a hostile + # target. The style belongs to the span, not to the string. + console.print(Text("Follow it at ").append(url, style="cyan underline")) _open(url) - finished = _wait(client, str(started["id"]), url) + finished = _wait(client, started["id"], url) _print_verdict(finished, url) if finished.get("status") != "done": raise SystemExit(1) @@ -139,8 +145,10 @@ def _amp_client(trusted: set[str]) -> PlusAPI: else "would carry the login over plain HTTP" ) console.print( - f"Reading anonymously: {client.base_url} {why}. " - "Run `crewai enterprise configure ` to log in to it.", + Text( + f"Reading anonymously: {client.base_url} {why}. " + "Run `crewai enterprise configure ` to log in to it." + ), style="yellow", ) return PlusAPI() @@ -244,7 +252,22 @@ def _start_evaluation(client: PlusAPI, execution_id: str) -> dict[str, Any]: _fail(f"Could not reach AMP to start the evaluation: {error}") if response.status_code in (200, 202): payload = _payload(response) - if payload and payload.get("id"): + # The id is what every later call is made with, so a missing or + # non-string one is a protocol error and not something to carry on + # with. The url is only ever shown and opened, so a malformed one + # costs the link and nothing else: the evaluation is already running + # and its verdict is what the user came for. + if payload and isinstance(payload.get("id"), str) and payload["id"]: + if not isinstance(payload.get("url"), str): + if payload.get("url") is not None: + console.print( + Text( + "AMP answered with a report url that is not a string; " + "the link is unavailable for this run." + ), + style="yellow", + ) + payload["url"] = None return payload _fail(f"AMP answered without an evaluation id ({response.status_code}).") _refused(response, f"run {execution_id}") @@ -294,7 +317,7 @@ def _wait(client: PlusAPI, evaluation_id: str, url: str | None) -> dict[str, Any ) time.sleep(POLL_SECONDS) except KeyboardInterrupt: - console.print(f"\nStill running{where}.", style="yellow") + console.print(Text(f"\nStill running{where}."), style="yellow") raise SystemExit(130) from None @@ -315,25 +338,36 @@ def _a_grade(grade: Any) -> bool: def _print_verdict(finished: dict[str, Any], url: str | None) -> None: + """Everything printed here came over the wire, so it is composed as `Text` + and never as markup: a `Console` parses square brackets, and an area named + `[red]tasks[/red]`, a gate, an error or a URL carrying one would restyle + the line or break it. A fixed list of areas used to make that impossible; + printing what arrives does not, so the escaping is explicit instead.""" if finished.get("status") != "done": console.print( - f"Evaluation failed: {finished.get('error') or 'no reason given'}", + Text(f"Evaluation failed: {finished.get('error') or 'no reason given'}"), style="bold red", ) return verdict = finished["verdict"] # _wait let only a well-formed one through gate = str(verdict["gate"]).upper() style = {"PASSED": "bold green", "FAILED": "bold red"}.get(gate, "bold yellow") - grades = verdict.get("grades") or {} - parts = [ - f"{area} {grades[area]}/5" - if grades.get(area) is not None - else f"{area} not measured" - for area in ("goal", "quality", "process", "cost") - ] - console.print(f"Goal gate: [{style}]{gate}[/{style}] · " + " · ".join(parts)) + line = Text("Goal gate: ") + line.append(gate, style=style) + # Whatever areas the evaluation graded, in the order it sent them — never a + # fixed list. The areas are the evaluator's to name, and a client that + # printed its own would silently drop any it had not heard of while + # inventing "not measured" for ones that no longer exist. An evaluation + # that graded nothing prints the gate alone: the separator belongs to the + # segment after it, so there is never one with nothing behind it. + for area, grade in (verdict.get("grades") or {}).items(): + line.append(" · ") + line.append( + f"{area} {grade}/5" if grade is not None else f"{area} not measured" + ) + console.print(line) if url: - console.print(f"Full report: {url}") + console.print(Text(f"Full report: {url}")) def _open(url: str) -> None: @@ -373,5 +407,8 @@ def _refused(response: httpx.Response, subject: str) -> None: def _fail(message: str) -> None: - console.print(message, style="bold red") + # `Text`, because most of what reaches here is AMP's own sentence and a + # `Console` parses square brackets. No caller relies on markup; the colour + # comes from `style`. + console.print(Text(message), style="bold red") raise SystemExit(1) diff --git a/lib/cli/tests/test_eval_crew.py b/lib/cli/tests/test_eval_crew.py index 4c91e3fe3..8a5a345aa 100644 --- a/lib/cli/tests/test_eval_crew.py +++ b/lib/cli/tests/test_eval_crew.py @@ -95,6 +95,119 @@ def test_the_last_run_is_evaluated_the_url_opened_and_the_verdict_printed(projec assert "Goal gate: PASSED" in out and "goal 5/5" in out and "cost not measured" in out +def test_the_verdict_prints_whatever_areas_the_evaluation_graded(project, monkeypatch, capsys): + # The areas are the evaluator's to name. A client printing its own list + # would drop the ones it had not heard of and invent "not measured" for + # ones that no longer exist — which is what happens the moment the + # evaluation's vocabulary moves ahead of an installed CLI. + directory, _ = project + record_last_run(directory) + graded = done(grades={"goal": 5, "tasks": 3, "agents": 4, "tools": None}) + install(monkeypatch, FakeAMP(statuses=[httpx.Response(200, json={"id": "ev-1", "status": "running"}), graded])) + + eval_module.eval_crew() + + out = capsys.readouterr().out + # The whole segment, in order: asserting the parts one by one would pass + # even if this path sorted them or printed its own list. + assert "Goal gate: PASSED · goal 5/5 · tasks 3/5 · agents 4/5 · tools not measured" in out + assert "quality" not in out and "process" not in out + + +def test_an_areas_name_is_printed_literally_never_as_markup(project, monkeypatch, capsys): + # Every part of this line came over the wire, and a Console parses square + # brackets. A fixed list of areas made that impossible; printing what + # arrives does not, so an area named `[red]tasks[/red]` must show its own + # brackets rather than restyling the verdict. + directory, _ = project + record_last_run(directory) + graded = done(grades={"[red]tasks[/red]": 4, "[bold]goal": 5}) + install(monkeypatch, FakeAMP(statuses=[httpx.Response(200, json={"id": "ev-1", "status": "running"}), graded])) + + eval_module.eval_crew() + + out = capsys.readouterr().out + assert "[red]tasks[/red] 4/5" in out + assert "[bold]goal 5/5" in out + + +def test_the_follow_link_cannot_be_retargeted_by_the_url_amp_sends(project, monkeypatch, capsys): + # This line invites a click, so a `url` carrying `[link=…]` would print a + # trustworthy label over a hostile target. It is the worst place in this + # command to let markup through. + directory, _ = project + record_last_run(directory) + hostile = "[link=http://attacker.test/]https://app.crewai.com/e/ev-1[/link]" + created = httpx.Response(202, json={"id": "ev-1", "url": hostile, "status": "queued"}) + install(monkeypatch, FakeAMP(create=created, statuses=[done()])) + + eval_module.eval_crew() + + out = capsys.readouterr().out + assert "[link=http://attacker.test/]" in out # printed, not followed + + +def test_a_url_that_is_not_a_string_costs_the_link_and_nothing_else(project, monkeypatch, capsys): + # Composing the line means appending the url rather than interpolating it, + # and `Text.append` wants a string. A malformed one must not become a + # traceback: the evaluation is already running and its verdict is what the + # user came for, so the link is dropped and the run carries on. + directory, opened = project + record_last_run(directory) + created = httpx.Response(202, json={"id": "ev-1", "url": ["not", "a", "string"], "status": "queued"}) + install(monkeypatch, FakeAMP(create=created, statuses=[done()])) + + eval_module.eval_crew() + + out = capsys.readouterr().out + assert "report url that is not a string" in out # said, not swallowed + assert "Goal gate: PASSED" in out # and the verdict still arrives + assert opened == [] # nothing was handed to a browser + assert "Follow it at" not in out + + +def test_an_id_that_is_not_a_string_is_a_protocol_error(project, monkeypatch, capsys): + # The id is what every later call is made with, so there is nothing to + # carry on with — unlike the url, which is only ever shown. + directory, _ = project + record_last_run(directory) + created = httpx.Response(202, json={"id": {"oops": 1}, "url": URL, "status": "queued"}) + install(monkeypatch, FakeAMP(create=created)) + + with pytest.raises(SystemExit): + eval_module.eval_crew() + + assert "without an evaluation id" in capsys.readouterr().out + + +def test_amps_refusal_is_printed_literally_too(project, monkeypatch, capsys): + # The same defect on the refusal path: AMP's own sentence reaches a Console. + directory, _ = project + record_last_run(directory) + refusal = httpx.Response(404, json={"error": "trace_not_found", "message": "No spans for [id]"}) + install(monkeypatch, FakeAMP(create=refusal)) + + with pytest.raises(SystemExit): + eval_module.eval_crew() + + assert "No spans for [id]" in capsys.readouterr().out + + +def test_an_evaluation_that_graded_nothing_prints_the_gate_alone(project, monkeypatch, capsys): + # A well-formed verdict may carry no grades at all, and a fixed list of + # areas used to hide that: there was always something after the separator. + directory, _ = project + record_last_run(directory) + graded = done(grades={}) + install(monkeypatch, FakeAMP(statuses=[httpx.Response(200, json={"id": "ev-1", "status": "running"}), graded])) + + eval_module.eval_crew() + + out = capsys.readouterr().out + assert "Goal gate: PASSED" in out + assert "Goal gate: PASSED ·" not in out # no separator with nothing after it + + def test_run_names_another_execution_and_an_anonymous_caller_sends_no_token(project, monkeypatch, capsys): directory, _ = project record_last_run(directory)