From 98e87397a6be58bd3b0a68d7794720ef3caee706 Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Mon, 3 Aug 2026 13:22:35 -0700 Subject: [PATCH] fix: never create [tool.crewai], treat whitespace ids as absent, harden test `crewai run` could rewrite unrelated projects (Cursor bugbot, high): - get_or_create_project_id ran before the cwd was established as a CrewAI project, and _set_project_id appended a [tool.crewai] table when none existed. Any directory with a pyproject.toml could therefore gain one - including on `crewai run --definition`, which may otherwise succeed. - _set_project_id no longer creates the table; it returns None when [tool.crewai] is absent, so a key is only ever added to a table the project already declares. The templates all ship the table, so no create path needs the old fallback. - The minting call in run_crew moved after the --definition early return, so an explicit-flow run does not touch the cwd at all. - Presence is checked, not truthiness: an empty [tool.crewai] is still a CrewAI marker, and get_crewai_project_config returns {} both for that and for an absent table. - Verified an unrelated project's pyproject.toml is byte-identical after a mint attempt. Whitespace-only project_id accepted as valid (CodeRabbit): - `project_id = " "` is truthy, so it was returned as an identity and would have propagated into login payloads and tracing context. It also meant the '" "' parameter of the replacement test asserted nothing. - Added _usable_project_id, which strips before deciding, used by both get_project_id and the locked mint path. Concurrency test could hang CI (CodeRabbit, major): - Neither the barrier nor the joins had timeouts, so a thread dying early or blocking on the lock would hang the job rather than fail it. The result count was also unchecked, so a dead thread still passed. - Added timeouts, an explicit liveness assertion, a result-count assertion, a lock around the shared result list, and corrected the docstring: this covers the read-modify-write race with threads, not the cross-process backend. Tests: 35, up from 32. New coverage for the absent-table refusal and three whitespace forms; the blank-id replacement case now asserts a real uuid replaced the blank value. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t --- lib/cli/src/crewai_cli/run_crew.py | 13 +++-- lib/crewai-core/src/crewai_core/project.py | 48 +++++++++++++--- lib/crewai/tests/telemetry/test_project_id.py | 56 +++++++++++++++---- 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/lib/cli/src/crewai_cli/run_crew.py b/lib/cli/src/crewai_cli/run_crew.py index 0f903a513..05dfe0bd1 100644 --- a/lib/cli/src/crewai_cli/run_crew.py +++ b/lib/cli/src/crewai_cli/run_crew.py @@ -618,10 +618,6 @@ def run_crew( or declarative (JSON) crew. Layered over the definition's own defaults; missing required values are prompted for interactively. """ - # Backfills projects created before project_id existed. Only here, in a - # command the user explicitly invoked - never from the SDK during kickoff. - get_or_create_project_id() - # --definition is a pure override: run that flow directly. if definition is not None: _run_explicit_declarative_flow( @@ -632,6 +628,15 @@ def run_crew( return pyproject_data = read_toml() + + # Backfills projects created before project_id existed. Only here, in a + # command the user explicitly invoked - never from the SDK during kickoff. + # Placed after the --definition early return so an explicit-flow run does + # not touch the cwd; get_or_create_project_id itself refuses to act unless + # [tool.crewai] is already present, so an unrelated project is never + # rewritten. + get_or_create_project_id() + if json_crew_definition := configured_project_json_crew(pyproject_data): # Declarative (JSON) crews resolve inputs the same way flows do: --inputs # layers over the crew's declared defaults, missing {placeholder}s are diff --git a/lib/crewai-core/src/crewai_core/project.py b/lib/crewai-core/src/crewai_core/project.py index fc971f1ae..80b4596f4 100644 --- a/lib/crewai-core/src/crewai_core/project.py +++ b/lib/crewai-core/src/crewai_core/project.py @@ -251,8 +251,31 @@ def get_project_id(pyproject_path: str | Path = "pyproject.toml") -> str | None: except (OSError, tomli.TOMLDecodeError): return None - project_id = get_crewai_project_config(pyproject_data).get(_PROJECT_ID_KEY) - return project_id if isinstance(project_id, str) and project_id else None + return _usable_project_id(get_crewai_project_config(pyproject_data)) + + +def _has_crewai_table(pyproject_data: dict[str, Any]) -> bool: + """True if ``[tool.crewai]`` exists, even when empty. + + Distinguishes "declared but empty" from "absent", which + :func:`get_crewai_project_config` cannot: it returns ``{}`` for both. + """ + tool_config = pyproject_data.get("tool") + return isinstance(tool_config, dict) and isinstance(tool_config.get("crewai"), dict) + + +def _usable_project_id(crewai_config: dict[str, Any]) -> str | None: + """Return the configured id if it is usable as an identifier. + + Whitespace-only values are treated as absent: they are truthy in Python but + are not an identity, and would otherwise propagate into login payloads and + tracing context. + """ + project_id = crewai_config.get(_PROJECT_ID_KEY) + if not isinstance(project_id, str): + return None + stripped = project_id.strip() + return stripped or None def get_or_create_project_id( @@ -314,10 +337,21 @@ def _get_or_create_project_id_locked(path: Path) -> str | None: except (tomli.TOMLDecodeError, ValueError): return None - existing = get_crewai_project_config(pyproject_data).get(_PROJECT_ID_KEY) - if isinstance(existing, str) and existing: + crewai_config = get_crewai_project_config(pyproject_data) + existing = _usable_project_id(crewai_config) + if existing: return existing + # Only ever add a key to an existing [tool.crewai] table. Creating the table + # would rewrite the pyproject.toml of any directory that merely happens to + # have one, which `crewai run` could otherwise do before it has established + # that the cwd is a CrewAI project at all. + # + # Presence, not truthiness: an empty `[tool.crewai]` table is still a CrewAI + # marker, and get_crewai_project_config returns {} for both cases. + if not _has_crewai_table(pyproject_data): + return None + project_id = str(uuid.uuid4()) updated = _set_project_id(content, project_id) if updated is None: @@ -468,6 +502,6 @@ def _set_project_id(content: str, project_id: str) -> str | None: lines.insert(insert_at, entry) return "".join(lines) - # No [tool.crewai] table: append one rather than guessing where it belongs. - suffix = "" if content.endswith(("\n", "\r")) or not content else newline - return f"{content}{suffix}{newline}[tool.crewai]{newline}{entry}" + # No [tool.crewai] table. Never create one: that would let this feature + # rewrite the pyproject.toml of a directory that is not a CrewAI project. + return None diff --git a/lib/crewai/tests/telemetry/test_project_id.py b/lib/crewai/tests/telemetry/test_project_id.py index a2a95aa34..e24aa725c 100644 --- a/lib/crewai/tests/telemetry/test_project_id.py +++ b/lib/crewai/tests/telemetry/test_project_id.py @@ -88,7 +88,6 @@ def test_comments_and_formatting_are_preserved(tmp_path): [ ('[project]\nname = "x"\n\n[tool.crewai]\ntype = "crew"\n', "table then EOF"), ('[tool.crewai]\ntype = "crew"', "no trailing newline"), - ('[project]\nname = "x"\n', "no tool.crewai table"), ('[project]\nname = "x"\n[tool.crewai]\n[other]\na = 1\n', "empty table"), ( '[tool.crewai]\ntype = "crew"\n\n\n[build-system]\nrequires = []\n', @@ -121,6 +120,20 @@ def test_id_does_not_leak_into_a_neighbouring_table(tmp_path): assert "project_id" not in data["build-system"] +def test_absent_tool_crewai_table_is_never_created(tmp_path): + """Refuse to mint rather than rewrite a non-CrewAI project's pyproject.toml. + + `crewai run` in any directory that merely happens to have a pyproject.toml + must not gain a [tool.crewai] table as a side effect. + """ + path = tmp_path / "pyproject.toml" + original = '[project]\nname = "unrelated"\n' + path.write_text(original) + + assert get_or_create_project_id(path) is None + assert path.read_text() == original, "unrelated project was modified" + + def test_missing_file_is_not_an_error(tmp_path): assert get_or_create_project_id(tmp_path / "nope.toml") is None @@ -152,9 +165,15 @@ def test_get_project_id_never_creates_anything(pyproject): assert pyproject.read_text() == before -def test_blank_id_is_treated_as_absent(tmp_path): +@pytest.mark.parametrize("blank", ['""', "' '", '"\\t"']) +def test_blank_or_whitespace_id_is_treated_as_absent(tmp_path, blank): + """Whitespace is truthy in Python but is not an identity. + + Accepting it would propagate a useless value into login payloads and + tracing context. + """ path = tmp_path / "pyproject.toml" - path.write_text('[tool.crewai]\ntype = "crew"\nproject_id = ""\n') + path.write_text(f'[tool.crewai]\ntype = "crew"\nproject_id = {blank}\n') assert get_project_id(path) is None @@ -169,7 +188,7 @@ def test_malformed_toml_is_never_written_to(tmp_path): assert path.read_text() == original, "malformed file must be left untouched" -@pytest.mark.parametrize("blank", ['""', "''", '" "']) +@pytest.mark.parametrize("blank", ['""', "''", '" "', '"\\t\\t"']) def test_blank_existing_id_is_replaced_not_duplicated(tmp_path, blank): """A blank id reads as absent; appending would make a duplicate key.""" path = tmp_path / "pyproject.toml" @@ -182,6 +201,7 @@ def test_blank_existing_id_is_replaced_not_duplicated(tmp_path, blank): data = parse_toml(content) # would raise on a duplicate key assert data["tool"]["crewai"]["project_id"] == project_id assert data["tool"]["crewai"]["type"] == "crew" + assert uuid.UUID(project_id), "must mint a real id, not keep the blank one" def test_non_string_existing_id_is_replaced(tmp_path): @@ -252,28 +272,42 @@ def test_lf_file_stays_lf(tmp_path): def test_concurrent_minting_converges_on_one_id(tmp_path): - """Two processes minting at once must agree on the persisted id.""" + """Concurrent minters must all return the id that ends up on disk. + + Uses threads in one process, so it covers the read-modify-write race rather + than the cross-process lock backend itself. + """ import threading + workers = 8 path = tmp_path / "pyproject.toml" path.write_text(CREW_PYPROJECT) returned: list[str | None] = [] - start = threading.Barrier(8) + results_lock = threading.Lock() + # Timed out rather than unbounded: a thread dying before the barrier, or + # blocking on the lock, would otherwise hang CI instead of failing. + start = threading.Barrier(workers, timeout=30) def mint() -> None: start.wait() - returned.append(get_or_create_project_id(path)) + project_id = get_or_create_project_id(path) + with results_lock: + returned.append(project_id) - threads = [threading.Thread(target=mint) for _ in range(8)] + threads = [threading.Thread(target=mint) for _ in range(workers)] for thread in threads: thread.start() for thread in threads: - thread.join() + thread.join(timeout=30) + + assert not [t for t in threads if t.is_alive()], "thread did not finish in time" + assert len(returned) == workers, f"only {len(returned)}/{workers} threads returned" persisted = parse_toml(path.read_text())["tool"]["crewai"]["project_id"] - assert len(set(returned)) == 1, f"callers disagreed: {set(returned)}" - assert returned[0] == persisted, "returned an id that is not on disk" + assert set(returned) == {persisted}, ( + f"callers disagreed with disk: returned={set(returned)} persisted={persisted}" + ) def test_file_mode_is_preserved(tmp_path):