fix(cli): crewai eval — the credential goes only to the configured AMP; an explicit yes before running the crew; a done answer needs a well-formed verdict

Review findings (CodeRabbit, the PR gate): - The record's amp_base_url was handed to PlusAPI beside the saved login, so a modified .crewai/last_run.json could send the token to any origin. The client is now built from the configured AMP only (CREWAI_PLUS_URL, the saved settings, app.crewai.com); the project's .env is loaded first, as `crewai run` loads it, so the configured AMP is the one the run was traced to. A record naming another address gets a one-line note and no credential. - The offer to turn tracing on and run the crew defaults to no and says the .env change stays; Enter no longer spends a crew run. - A `done` answer whose verdict is missing or malformed (no gate, grades not an object, a grade not an int or null) is a protocol error, exit 1, instead of an INCONCLUSIVE line with exit 0 or an AttributeError. Tests for each: a foreign origin in the record with a saved token, the .env-loaded same-AMP case, seven malformed verdicts, the prompt's text and default. 59 passed (eval + plus_api); ruff and mypy clean. Co-Authored-By: Claude Fable 5.1 <[email protected]>
2026-10-12 02:12:31 +00:00 · 2026-09-20 10:08:27 -07:00
parent 5ac9c052d8
commit 3f21d108d5
2 changed files with 86 additions and 13 deletions
@@ -24,7 +24,7 @@ from typing import Any
 import webbrowser

 import click
-from dotenv import set_key
+from dotenv import load_dotenv, set_key
 import httpx
 from rich.console import Console

@@ -46,15 +46,22 @@ 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()
+    _load_project_env()
    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 {}

-    # 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)
+    # The credential goes to the CONFIGURED AMP only (CREWAI_PLUS_URL, the saved settings,
+    # app.crewai.com) — never to an address read off a file in the project.
+    client = PlusAPI(api_key=saved_login())
+    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}.",
+            style="yellow",
+        )
    started = _start_evaluation(client, execution_id)
    url = started.get("url")
    console.print(f"Evaluating run [bold]{execution_id}[/bold]")
@@ -68,6 +75,13 @@ def eval_crew(run_id: str | None = None) -> None:
        raise SystemExit(1)


+def _load_project_env() -> None:
+    """The project's .env, as `crewai run` loads it — so CREWAI_PLUS_URL here is the one the run used."""
+    env_file = Path.cwd() / ".env"
+    if env_file.is_file():
+        load_dotenv(env_file, override=True)
+
+
 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."""
@@ -105,8 +119,9 @@ def _run_now_or_explain() -> str:
        console.print(steps, style="yellow")
        raise SystemExit(1)
    if not click.confirm(
-        "No traced run is recorded in this project. Turn tracing on and run the crew now?",
-        default=True,
+        f"No traced run is recorded in this project. Turn tracing on ({TRACING_ENV_VAR}=true "
+        "stays in .env) and run the crew now?",
+        default=False,
    ):
        console.print(steps, style="yellow")
        raise SystemExit(0)
@@ -184,6 +199,10 @@ def _wait(client: PlusAPI, evaluation_id: str, url: str | None) -> dict[str, Any
            misses = 0
            payload = _payload(response) or {}
            status = payload.get("status")
+            if status == "done" and not _well_formed_verdict(payload.get("verdict")):
+                _fail(
+                    f"AMP answered done without a verdict (protocol error); follow it{where or ' on AMP'}."
+                )
            if status in FINISHED:
                return payload
            if status not in STATUSES:
@@ -196,6 +215,19 @@ def _wait(client: PlusAPI, evaluation_id: str, url: str | None) -> dict[str, Any
        raise SystemExit(130) from None


+def _well_formed_verdict(verdict: Any) -> bool:
+    """`{"gate": "<word>", "grades": {area: 1..5 | null}}` — anything else is a protocol error."""
+    return (
+        isinstance(verdict, dict)
+        and isinstance(verdict.get("gate"), str)
+        and isinstance(verdict.get("grades"), dict)
+        and all(
+            grade is None or isinstance(grade, int)
+            for grade in verdict["grades"].values()
+        )
+    )
+
+
 def _print_verdict(finished: dict[str, Any], url: str | None) -> None:
    if finished.get("status") != "done":
        console.print(
@@ -203,8 +235,8 @@ def _print_verdict(finished: dict[str, Any], url: str | None) -> None:
            style="bold red",
        )
        return
-    verdict = finished.get("verdict") or {}
-    gate = str(verdict.get("gate") or "inconclusive").upper()
+    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 = [
@@ -64,9 +64,9 @@ def record_last_run(directory: Path, execution_id: str = EXECUTION_ID, **fields)
    (directory / ".crewai" / "last_run.json").write_text(json.dumps(record))


-def install(monkeypatch, amp: FakeAMP) -> FakeAMP:
+def install(monkeypatch, amp: FakeAMP, configured_amp: str = "https://amp.test") -> FakeAMP:
    def build(api_key=None, base_url=None):
-        amp.api_key, amp.base_url = api_key, base_url
+        amp.api_key, amp.base_url = api_key, base_url or configured_amp
        return amp

    monkeypatch.setattr(eval_module, "PlusAPI", build)
@@ -82,7 +82,6 @@ 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
@@ -98,7 +97,6 @@ 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

@@ -142,6 +140,46 @@ def test_amps_refusals_are_printed_in_its_words_and_exit_one(project, monkeypatc
    assert opened == []


+def test_the_credential_goes_only_to_the_configured_amp_never_to_an_address_off_the_record(project, monkeypatch, capsys):
+    directory, _ = project
+    record_last_run(directory, amp_base_url="https://evil.example/steal")
+    amp = install(monkeypatch, FakeAMP(statuses=[done()]), configured_amp="https://app.crewai.com")
+
+    eval_module.eval_crew()
+
+    assert amp.api_key == "login-token" and amp.base_url == "https://app.crewai.com"  # PlusAPI got no base_url
+    out = capsys.readouterr().out
+    assert "The run was traced to https://evil.example/steal; evaluating at the configured AMP https://app.crewai.com." in out
+
+    # The project's .env is what `crewai run` traced with, so it is loaded first: same AMP, no note.
+    (directory / ".env").write_text("CREWAI_PLUS_URL=https://amp.test\n")
+    record_last_run(directory, amp_base_url="https://amp.test/")
+    install(monkeypatch, FakeAMP(statuses=[done()]))
+    monkeypatch.delenv("CREWAI_PLUS_URL", raising=False)
+    eval_module.eval_crew()
+    assert eval_module.os.environ["CREWAI_PLUS_URL"] == "https://amp.test"
+    assert "was traced to" not in capsys.readouterr().out
+
+
+@pytest.mark.parametrize(
+    "verdict",
+    [None, "passed", [], {"gate": "passed"}, {"gate": None, "grades": {}}, {"gate": "passed", "grades": "5/5"}, {"gate": "passed", "grades": {"goal": "five"}}],
+)
+def test_a_done_answer_without_a_well_formed_verdict_is_a_protocol_error(project, monkeypatch, capsys, verdict):
+    directory, _ = project
+    record_last_run(directory)
+    body = {"id": "ev-1", "status": "done", "url": URL}
+    if verdict is not None:
+        body["verdict"] = verdict
+    install(monkeypatch, FakeAMP(statuses=[httpx.Response(200, json=body)]))
+
+    with pytest.raises(SystemExit) as exit_:
+        eval_module.eval_crew()
+
+    assert exit_.value.code == 1
+    assert f"AMP answered done without a verdict (protocol error); follow it at {URL}." in capsys.readouterr().out
+
+
 def test_amp_unreachable_at_the_start_is_a_sentence_not_a_traceback(project, monkeypatch, capsys):
    directory, _ = project
    record_last_run(directory)
@@ -262,7 +300,8 @@ def test_without_a_traced_run_it_offers_to_turn_tracing_on_and_run_the_crew(proj
    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)
+    prompts: list[tuple] = []
+    monkeypatch.setattr(eval_module.click, "confirm", lambda text, **kwargs: prompts.append((text, kwargs)) or True)
    ran: list[str] = []

    def fake_run_crew() -> None:
@@ -279,6 +318,8 @@ def test_without_a_traced_run_it_offers_to_turn_tracing_on_and_run_the_crew(proj
    eval_module.eval_crew()

    assert ran == ["run"]
+    text, kwargs = prompts[0]
+    assert "CREWAI_TRACING_ENABLED=true stays in .env" in text and kwargs == {"default": False}  # an explicit yes
    assert "CREWAI_TRACING_ENABLED=true" in (directory / ".env").read_text()
    assert amp.calls[0] == ("create", "fresh-run")
    assert "Tracing is on for this project" in capsys.readouterr().out