From 5ac9c052d88f8af2d83d41728386ec30a414049f Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Sun, 20 Sep 2026 01:51:13 -0700 Subject: [PATCH] =?UTF-8?q?fix(cli):=20crewai=20eval=20=E2=80=94=20guard?= =?UTF-8?q?=20the=20project,=20survive=20AMP=20blips,=20name=20the=20right?= =?UTF-8?q?=20subject,=20read=20the=20whole=20record?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings, each reproduced before the fix: - The run-it-now offer wrote CREWAI_TRACING_ENABLED into ./.env before checking the directory is a crewAI project, then run_crew() died on a missing pyproject.toml with a traceback. Now: no pyproject.toml → one sentence, exit 1, nothing written. - httpx errors (AMP unreachable, a timeout) surfaced as tracebacks. Now the start says "Could not reach AMP to start the evaluation: …"; while waiting, an unreachable AMP or a 5xx is retried up to POLL_RETRIES consecutive times, then reported with the URL — the evaluation keeps running server-side either way. - The POST now carries a 120 s timeout (AMP reads the run's spans inside it), the poll 30 s. - A 200 whose body has no known status (a non-dict, no status, a status outside queued/running/done/failed) polled forever. Now it stops with the status it saw and the URL. - A 404 without a JSON message read "AMP holds no run " while polling. _refused takes "run " / "evaluation " and says "AMP answered 404 for " — AMP's own message still wins. - The record's amp_base_url was ignored; the CLI now evaluates the run at the AMP it was traced to. --run keeps the configured AMP. - The post-run explanation names the third cause: a crewai older than the version that records the last run. Tests for each, plus the previously untested paths: Ctrl-C exits 130, a 2xx without an id, a refusal mid-poll, DMN opens no browser, --run skips the offer. 51 passed in lib/cli/tests (eval + plus_api). Co-Authored-By: Claude Fable 5.1 --- lib/cli/src/crewai_cli/eval_crew.py | 81 ++++++++++++---- lib/cli/src/crewai_cli/plus_api.py | 14 ++- lib/cli/tests/test_eval_crew.py | 137 ++++++++++++++++++++++++++-- lib/cli/tests/test_plus_api.py | 3 +- 4 files changed, 204 insertions(+), 31 deletions(-) diff --git a/lib/cli/src/crewai_cli/eval_crew.py b/lib/cli/src/crewai_cli/eval_crew.py index 56ec9e8e8..258449ade 100644 --- a/lib/cli/src/crewai_cli/eval_crew.py +++ b/lib/cli/src/crewai_cli/eval_crew.py @@ -38,17 +38,23 @@ console = Console() LAST_RUN_FILE = Path(".crewai") / "last_run.json" TRACING_ENV_VAR = "CREWAI_TRACING_ENABLED" POLL_SECONDS = 3.0 +POLL_RETRIES = 5 # consecutive unreachable / 5xx polls before giving up; the evaluation keeps running FINISHED = {"done", "failed"} +STATUSES = {"queued", "running"} | FINISHED def eval_crew(run_id: str | None = None) -> None: """Evaluate the last traced run of this project, or the run RUN_ID.""" get_or_create_project_id() - execution_id = run_id or last_run_id() + record = read_last_run() or {} + execution_id = run_id or record.get("execution_id") if execution_id is None: execution_id = _run_now_or_explain() + record = read_last_run() or {} - client = PlusAPI(api_key=saved_login()) + # The record names the AMP the run was traced to; a run named by hand goes to the configured AMP. + amp_base_url = None if run_id else record.get("amp_base_url") + client = PlusAPI(api_key=saved_login(), base_url=amp_base_url or None) started = _start_evaluation(client, execution_id) url = started.get("url") console.print(f"Evaluating run [bold]{execution_id}[/bold]") @@ -62,17 +68,18 @@ def eval_crew(run_id: str | None = None) -> None: raise SystemExit(1) -def last_run_id(directory: Path | None = None) -> str | None: - """The execution id crewAI recorded for the project's last traced run.""" +def read_last_run(directory: Path | None = None) -> dict[str, Any] | None: + """The record crewAI wrote for the project's last traced run (execution_id, + tier, started_at, finished_at, amp_base_url), or None.""" path = (directory or Path.cwd()) / LAST_RUN_FILE try: loaded = json.loads(path.read_text(encoding="utf-8")) except (OSError, ValueError): return None - if not isinstance(loaded, dict): + if not isinstance(loaded, dict) or not loaded.get("execution_id"): return None - execution_id = loaded.get("execution_id") - return str(execution_id) if execution_id else None + loaded["execution_id"] = str(loaded["execution_id"]) + return loaded def saved_login() -> str | None: @@ -85,6 +92,11 @@ def saved_login() -> str | None: def _run_now_or_explain() -> str: """No traced run recorded here: offer to turn tracing on and run the crew now.""" + if not Path("pyproject.toml").is_file(): + _fail( + "No crewAI project here (no pyproject.toml). Run `crewai eval` from the project's " + "directory, or name a run: `crewai eval --run EXECUTION_ID`." + ) steps = ( "No traced run is recorded in this project. Turn tracing on and run the crew, " f"then come back:\n 1. add {TRACING_ENV_VAR}=true to .env\n 2. crewai run\n 3. crewai eval" @@ -103,15 +115,16 @@ def _run_now_or_explain() -> str: from crewai_cli.run_crew import run_crew run_crew() - execution_id = last_run_id() - if execution_id is None: + record = read_last_run() + if record is None: console.print( - "The run finished but no trace was recorded: the run may have failed, or sharing " - "the trace was declined. Run the crew again and accept when asked, then `crewai eval`.", + "The run finished but no trace was recorded: the run may have failed, sharing the " + "trace was declined, or this project's crewai is older than the version that records " + f"the last run ({LAST_RUN_FILE}). Run the crew again and accept when asked, then `crewai eval`.", style="bold red", ) raise SystemExit(1) - return execution_id + return str(record["execution_id"]) def _enable_tracing() -> None: @@ -127,30 +140,58 @@ def _enable_tracing() -> None: def _start_evaluation(client: PlusAPI, execution_id: str) -> dict[str, Any]: - response = client.create_evaluation(execution_id) + try: + response = client.create_evaluation(execution_id) + except httpx.HTTPError as error: + _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"): return payload _fail(f"AMP answered without an evaluation id ({response.status_code}).") - _refused(response, execution_id) + _refused(response, f"run {execution_id}") raise AssertionError("unreachable") def _wait(client: PlusAPI, evaluation_id: str, url: str | None) -> dict[str, Any]: """Poll until the evaluation is done or failed; Ctrl-C leaves it running.""" console.print("Waiting for the verdict…", style="dim") + where = f" at {url}" if url else "" + subject = f"evaluation {evaluation_id}" + misses = ( + 0 # AMP unreachable or answering 5xx: a blip is retried, a streak is reported + ) try: while True: - response = client.get_evaluation(evaluation_id) + try: + response = client.get_evaluation(evaluation_id) + except httpx.HTTPError as error: + misses += 1 + if misses >= POLL_RETRIES: + _fail( + f"Could not reach AMP while waiting ({error}); the evaluation keeps running{where}." + ) + time.sleep(POLL_SECONDS) + continue + if response.status_code >= 500: + misses += 1 + if misses >= POLL_RETRIES: + _refused(response, subject) + time.sleep(POLL_SECONDS) + continue if response.status_code != 200: - _refused(response, evaluation_id) + _refused(response, subject) + misses = 0 payload = _payload(response) or {} - if payload.get("status") in FINISHED: + status = payload.get("status") + if status in FINISHED: return payload + if status not in STATUSES: + _fail( + f"AMP answered without a known evaluation status ({status!r}); follow it{where or ' on AMP'}." + ) time.sleep(POLL_SECONDS) except KeyboardInterrupt: - where = f" at {url}" if url else "" console.print(f"\nStill running{where}.", style="yellow") raise SystemExit(130) from None @@ -193,7 +234,7 @@ def _payload(response: httpx.Response) -> dict[str, Any] | None: def _refused(response: httpx.Response, subject: str) -> None: - """AMP's own words when it sent them, then exit 1.""" + """AMP's own words when it sent them, then exit 1. SUBJECT is "run " or "evaluation ".""" payload = _payload(response) or {} message = str(payload.get("message") or "").strip() error = str(payload.get("error") or "") @@ -204,7 +245,7 @@ def _refused(response: httpx.Response, subject: str) -> None: f"{message or 'AMP refused the credential'}. Log in with `crewai login` and try again." ) if response.status_code == 404: - _fail(message or f"AMP holds no run {subject}.") + _fail(message or f"AMP answered 404 for {subject}.") if response.status_code == 429: retry = response.headers.get("Retry-After") _fail( diff --git a/lib/cli/src/crewai_cli/plus_api.py b/lib/cli/src/crewai_cli/plus_api.py index ea3733d8b..f03661e5b 100644 --- a/lib/cli/src/crewai_cli/plus_api.py +++ b/lib/cli/src/crewai_cli/plus_api.py @@ -23,16 +23,26 @@ class PlusAPI(_CorePlusAPI): """ EVALUATIONS_RESOURCE = f"{_CorePlusAPI.TRACING_RESOURCE}/evaluations" + # AMP reads the run's spans from Wharf inside the POST; a large run takes a while. + EVALUATION_START_TIMEOUT = 120.0 + EVALUATION_POLL_TIMEOUT = 30.0 def create_evaluation(self, execution_id: str) -> httpx.Response: """Ask AMP to evaluate the traced run EXECUTION_ID (crewai eval).""" return self._make_request( - "POST", self.EVALUATIONS_RESOURCE, json={"execution_id": execution_id} + "POST", + self.EVALUATIONS_RESOURCE, + json={"execution_id": execution_id}, + timeout=self.EVALUATION_START_TIMEOUT, ) def get_evaluation(self, evaluation_id: str) -> httpx.Response: """The evaluation's status and, once done, its verdict.""" - return self._make_request("GET", f"{self.EVALUATIONS_RESOURCE}/{evaluation_id}") + return self._make_request( + "GET", + f"{self.EVALUATIONS_RESOURCE}/{evaluation_id}", + timeout=self.EVALUATION_POLL_TIMEOUT, + ) def _make_multipart_request( self, diff --git a/lib/cli/tests/test_eval_crew.py b/lib/cli/tests/test_eval_crew.py index b0b50109b..642a804da 100644 --- a/lib/cli/tests/test_eval_crew.py +++ b/lib/cli/tests/test_eval_crew.py @@ -8,6 +8,7 @@ from pathlib import Path from click.testing import CliRunner import httpx import pytest +from rich.console import Console from crewai_cli import eval_crew as eval_module from crewai_cli.cli import eval_command @@ -47,6 +48,7 @@ def done(gate="passed", grades=None): @pytest.fixture def project(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) + monkeypatch.setattr(eval_module, "console", Console(width=240)) # one sentence per line in the captured output monkeypatch.setattr(eval_module, "get_or_create_project_id", lambda: None) monkeypatch.setattr(eval_module, "saved_login", lambda: "login-token") monkeypatch.setattr(eval_module.time, "sleep", lambda seconds: None) @@ -56,13 +58,18 @@ def project(tmp_path, monkeypatch): return tmp_path, opened -def record_last_run(directory: Path, execution_id: str = EXECUTION_ID) -> None: +def record_last_run(directory: Path, execution_id: str = EXECUTION_ID, **fields) -> None: (directory / ".crewai").mkdir(exist_ok=True) - (directory / ".crewai" / "last_run.json").write_text(json.dumps({"execution_id": execution_id, "tier": "ephemeral"})) + record = {"execution_id": execution_id, "tier": "ephemeral", "amp_base_url": "https://amp.test", **fields} + (directory / ".crewai" / "last_run.json").write_text(json.dumps(record)) def install(monkeypatch, amp: FakeAMP) -> FakeAMP: - monkeypatch.setattr(eval_module, "PlusAPI", lambda api_key=None: (setattr(amp, "api_key", api_key), amp)[1]) + def build(api_key=None, base_url=None): + amp.api_key, amp.base_url = api_key, base_url + return amp + + monkeypatch.setattr(eval_module, "PlusAPI", build) return amp @@ -75,6 +82,7 @@ def test_the_last_run_is_evaluated_the_url_opened_and_the_verdict_printed(projec out = capsys.readouterr().out assert amp.api_key == "login-token" + assert amp.base_url == "https://amp.test" # the AMP the run was traced to, off the record assert amp.calls == [("create", EXECUTION_ID), ("get", "ev-1"), ("get", "ev-1")] assert opened == [URL] assert EXECUTION_ID in out and URL in out @@ -90,6 +98,7 @@ def test_run_names_another_execution_and_an_anonymous_caller_sends_no_token(proj eval_module.eval_crew(run_id="other-run") assert amp.api_key is None + assert amp.base_url is None # a run named by hand goes to the configured AMP, not the record's assert amp.calls[0] == ("create", "other-run") assert "Goal gate: FAILED" in capsys.readouterr().out @@ -113,6 +122,8 @@ def test_a_failed_evaluation_exits_one_with_amps_reason(project, monkeypatch, ca "already read once without an account"), (httpx.Response(401, json={"error": "bad_credentials", "message": "Bad credentials"}), "Bad credentials. Log in with `crewai login`"), (httpx.Response(404, json={"error": "trace_not_found", "message": "No spans recorded for execution 6f31"}), "No spans recorded for execution 6f31"), + (httpx.Response(404, text="Page not found"), f"AMP answered 404 for run {EXECUTION_ID}."), + (httpx.Response(202, json={"url": URL}), "AMP answered without an evaluation id (202)."), (httpx.Response(429, json={"error": "rate_limit_exceeded", "message": "Too many requests"}, headers={"Retry-After": "60"}), "Too many requests — retry after 60s"), (httpx.Response(503, json={"error": "service_unavailable", "message": "Wharf could not list the spans"}), "AMP answered 503: Wharf could not list the spans"), (httpx.Response(500, text="boom"), "AMP answered 500."), @@ -131,7 +142,110 @@ def test_amps_refusals_are_printed_in_its_words_and_exit_one(project, monkeypatc assert opened == [] +def test_amp_unreachable_at_the_start_is_a_sentence_not_a_traceback(project, monkeypatch, capsys): + directory, _ = project + record_last_run(directory) + amp = install(monkeypatch, FakeAMP()) + monkeypatch.setattr(amp, "create_evaluation", lambda execution_id: (_ for _ in ()).throw(httpx.ConnectError("connection refused"))) + + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + + assert exit_.value.code == 1 + assert "Could not reach AMP to start the evaluation: connection refused" in capsys.readouterr().out + + +def test_while_waiting_a_blip_is_retried_and_a_streak_is_reported(project, monkeypatch, capsys): + directory, _ = project + record_last_run(directory) + amp = install(monkeypatch, FakeAMP(statuses=[httpx.Response(502), httpx.Response(503, json={"error": "service_unavailable", "message": "crew-optimize is down"}), done()])) + + eval_module.eval_crew() # two bad polls, then the verdict + + assert "Goal gate: PASSED" in capsys.readouterr().out + assert amp.calls.count(("get", "ev-1")) == 3 + + record_last_run(directory) + amp = install(monkeypatch, FakeAMP(statuses=[httpx.Response(503, json={"error": "service_unavailable", "message": "crew-optimize is down"})] * eval_module.POLL_RETRIES)) + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + assert exit_.value.code == 1 + assert "AMP answered 503: crew-optimize is down" in capsys.readouterr().out + assert amp.calls.count(("get", "ev-1")) == eval_module.POLL_RETRIES + + amp = install(monkeypatch, FakeAMP()) + monkeypatch.setattr(amp, "get_evaluation", lambda evaluation_id: (_ for _ in ()).throw(httpx.ReadTimeout("timed out"))) + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + assert exit_.value.code == 1 + assert f"Could not reach AMP while waiting (timed out); the evaluation keeps running at {URL}." in capsys.readouterr().out + + +def test_a_refusal_mid_poll_names_the_evaluation_and_an_unknown_status_stops_the_wait(project, monkeypatch, capsys): + directory, _ = project + record_last_run(directory) + install(monkeypatch, FakeAMP(statuses=[httpx.Response(404, text="gone")])) + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + assert exit_.value.code == 1 and "AMP answered 404 for evaluation ev-1." in capsys.readouterr().out + + for odd in (httpx.Response(200, json={"id": "ev-1", "status": "cancelled"}), httpx.Response(200, json=[]), httpx.Response(200, text="")): + install(monkeypatch, FakeAMP(statuses=[httpx.Response(200, json={"id": "ev-1", "status": "queued"}), odd])) + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + assert exit_.value.code == 1 + assert f"AMP answered without a known evaluation status" in capsys.readouterr().out + + +def test_ctrl_c_leaves_the_evaluation_running_and_exits_130(project, monkeypatch, capsys): + directory, _ = project + record_last_run(directory) + amp = install(monkeypatch, FakeAMP()) + monkeypatch.setattr(amp, "get_evaluation", lambda evaluation_id: (_ for _ in ()).throw(KeyboardInterrupt())) + + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + + assert exit_.value.code == 130 + assert f"Still running at {URL}." in capsys.readouterr().out + + +def test_dmn_mode_prints_the_url_but_opens_no_browser(project, monkeypatch, capsys): + directory, opened = project + record_last_run(directory) + monkeypatch.setattr(eval_module, "is_dmn_mode_enabled", lambda: True) + install(monkeypatch, FakeAMP(statuses=[done()])) + + eval_module.eval_crew() + + assert opened == [] and URL in capsys.readouterr().out + + +def test_run_skips_the_offer_when_nothing_is_recorded(project, monkeypatch, capsys): + monkeypatch.setattr(eval_module.click, "confirm", lambda *args, **kwargs: pytest.fail("no offer with --run")) + amp = install(monkeypatch, FakeAMP(statuses=[done()])) + + eval_module.eval_crew(run_id="named-run") + + assert amp.calls[0] == ("create", "named-run") + + +def test_outside_a_crewai_project_nothing_is_written_and_it_says_so(project, monkeypatch, capsys): + directory, _ = project + monkeypatch.setattr(eval_module.sys.stdin, "isatty", lambda: True) + monkeypatch.setattr(eval_module.click, "confirm", lambda *args, **kwargs: pytest.fail("no offer outside a project")) + amp = install(monkeypatch, FakeAMP()) + + with pytest.raises(SystemExit) as exit_: + eval_module.eval_crew() + + assert exit_.value.code == 1 + assert "No crewAI project here (no pyproject.toml)" in capsys.readouterr().out + assert not (directory / ".env").exists() and amp.calls == [] + + def test_without_a_traced_run_and_no_terminal_it_explains_and_exits(project, monkeypatch, capsys): + (project[0] / "pyproject.toml").write_text("[project]\nname = 'demo'\n") monkeypatch.setattr(eval_module, "is_dmn_mode_enabled", lambda: True) amp = install(monkeypatch, FakeAMP()) @@ -146,6 +260,7 @@ def test_without_a_traced_run_and_no_terminal_it_explains_and_exits(project, mon def test_without_a_traced_run_it_offers_to_turn_tracing_on_and_run_the_crew(project, monkeypatch, capsys): directory, _ = project + (directory / "pyproject.toml").write_text("[project]\nname = 'demo'\n") monkeypatch.setattr(eval_module.sys.stdin, "isatty", lambda: True) monkeypatch.setattr(eval_module.click, "confirm", lambda *args, **kwargs: True) ran: list[str] = [] @@ -170,6 +285,7 @@ def test_without_a_traced_run_it_offers_to_turn_tracing_on_and_run_the_crew(proj def test_declining_the_offer_exits_cleanly_with_the_steps(project, monkeypatch, capsys): + (project[0] / "pyproject.toml").write_text("[project]\nname = 'demo'\n") monkeypatch.setattr(eval_module.sys.stdin, "isatty", lambda: True) monkeypatch.setattr(eval_module.click, "confirm", lambda *args, **kwargs: False) amp = install(monkeypatch, FakeAMP()) @@ -182,6 +298,7 @@ def test_declining_the_offer_exits_cleanly_with_the_steps(project, monkeypatch, def test_a_run_that_leaves_no_trace_behind_is_explained(project, monkeypatch, capsys): + (project[0] / "pyproject.toml").write_text("[project]\nname = 'demo'\n") monkeypatch.setattr(eval_module.sys.stdin, "isatty", lambda: True) monkeypatch.setattr(eval_module.click, "confirm", lambda *args, **kwargs: True) import crewai_cli.run_crew as run_crew_module @@ -193,7 +310,8 @@ def test_a_run_that_leaves_no_trace_behind_is_explained(project, monkeypatch, ca eval_module.eval_crew() assert exit_.value.code == 1 - assert "no trace was recorded" in capsys.readouterr().out + out = capsys.readouterr().out + assert "no trace was recorded" in out and "older than the version that records the last run" in out def test_the_cli_command_maps_to_the_implementation(monkeypatch): @@ -207,10 +325,13 @@ def test_the_cli_command_maps_to_the_implementation(monkeypatch): assert "Evaluate the last traced run" in runner.invoke(eval_command, ["--help"]).output -def test_last_run_id_reads_the_record_crewai_writes(tmp_path): - assert eval_module.last_run_id(tmp_path) is None +def test_read_last_run_reads_the_record_crewai_writes(tmp_path): + assert eval_module.read_last_run(tmp_path) is None (tmp_path / ".crewai").mkdir() (tmp_path / ".crewai" / "last_run.json").write_text("not json") - assert eval_module.last_run_id(tmp_path) is None + assert eval_module.read_last_run(tmp_path) is None + (tmp_path / ".crewai" / "last_run.json").write_text(json.dumps({"tier": "ephemeral"})) + assert eval_module.read_last_run(tmp_path) is None record_last_run(tmp_path) - assert eval_module.last_run_id(tmp_path) == EXECUTION_ID + record = eval_module.read_last_run(tmp_path) + assert record is not None and record["execution_id"] == EXECUTION_ID and record["amp_base_url"] == "https://amp.test" diff --git a/lib/cli/tests/test_plus_api.py b/lib/cli/tests/test_plus_api.py index 8f683a80f..e40cfdbc2 100644 --- a/lib/cli/tests/test_plus_api.py +++ b/lib/cli/tests/test_plus_api.py @@ -29,6 +29,7 @@ class TestPlusAPI(unittest.TestCase): "POST", "/crewai_plus/api/v1/tracing/evaluations", json={"execution_id": "6f31fe1a-20bd-4bfe-a011-25d6b9341f62"}, + timeout=120.0, # AMP reads the run's spans inside this request ) self.assertEqual(response, mock_response) @@ -40,7 +41,7 @@ class TestPlusAPI(unittest.TestCase): response = self.api.get_evaluation("ev-1") mock_make_request.assert_called_once_with( - "GET", "/crewai_plus/api/v1/tracing/evaluations/ev-1" + "GET", "/crewai_plus/api/v1/tracing/evaluations/ev-1", timeout=30.0 ) self.assertEqual(response, mock_response)