mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-08-10 08:21:54 +00:00
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user