mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-09-22 19:06:25 +00:00
fix(cli): print whatever areas the evaluation graded (#7701)
`crewai eval` printed a fixed tuple of area names. The areas belong to the evaluation, not to the client, so a fixed list does two wrong things the moment the evaluator's vocabulary moves ahead of an installed CLI: it drops every area it has not heard of, and it invents "not measured" for ones that no longer exist. A user would see one real grade and three phantom blanks, with no sign that anything had been dropped. It now prints the areas it was sent, in the order they arrived. The verdict is still validated exactly as before — a grade is an int in 1..5 or null — so a malformed payload is still a protocol error rather than something printed. This lands before the evaluator's own change so that an installed CLI keeps working through the rollout rather than after it.
This commit is contained in:
@@ -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 <url>` to log in to it.",
|
||||
Text(
|
||||
f"Reading anonymously: {client.base_url} {why}. "
|
||||
"Run `crewai enterprise configure <url>` 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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user