Skip to content

Commit 4b2c9cd

Browse files
joaomdmouraclaude
andcommitted
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
1 parent 24b36d7 commit 4b2c9cd

3 files changed

Lines changed: 95 additions & 22 deletions

File tree

lib/cli/src/crewai_cli/run_crew.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -618,10 +618,6 @@ def run_crew(
618618
or declarative (JSON) crew. Layered over the definition's own
619619
defaults; missing required values are prompted for interactively.
620620
"""
621-
# Backfills projects created before project_id existed. Only here, in a
622-
# command the user explicitly invoked - never from the SDK during kickoff.
623-
get_or_create_project_id()
624-
625621
# --definition is a pure override: run that flow directly.
626622
if definition is not None:
627623
_run_explicit_declarative_flow(
@@ -632,6 +628,15 @@ def run_crew(
632628
return
633629

634630
pyproject_data = read_toml()
631+
632+
# Backfills projects created before project_id existed. Only here, in a
633+
# command the user explicitly invoked - never from the SDK during kickoff.
634+
# Placed after the --definition early return so an explicit-flow run does
635+
# not touch the cwd; get_or_create_project_id itself refuses to act unless
636+
# [tool.crewai] is already present, so an unrelated project is never
637+
# rewritten.
638+
get_or_create_project_id()
639+
635640
if json_crew_definition := configured_project_json_crew(pyproject_data):
636641
# Declarative (JSON) crews resolve inputs the same way flows do: --inputs
637642
# layers over the crew's declared defaults, missing {placeholder}s are

lib/crewai-core/src/crewai_core/project.py

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,31 @@ def get_project_id(pyproject_path: str | Path = "pyproject.toml") -> str | None:
251251
except (OSError, tomli.TOMLDecodeError):
252252
return None
253253

254-
project_id = get_crewai_project_config(pyproject_data).get(_PROJECT_ID_KEY)
255-
return project_id if isinstance(project_id, str) and project_id else None
254+
return _usable_project_id(get_crewai_project_config(pyproject_data))
255+
256+
257+
def _has_crewai_table(pyproject_data: dict[str, Any]) -> bool:
258+
"""True if ``[tool.crewai]`` exists, even when empty.
259+
260+
Distinguishes "declared but empty" from "absent", which
261+
:func:`get_crewai_project_config` cannot: it returns ``{}`` for both.
262+
"""
263+
tool_config = pyproject_data.get("tool")
264+
return isinstance(tool_config, dict) and isinstance(tool_config.get("crewai"), dict)
265+
266+
267+
def _usable_project_id(crewai_config: dict[str, Any]) -> str | None:
268+
"""Return the configured id if it is usable as an identifier.
269+
270+
Whitespace-only values are treated as absent: they are truthy in Python but
271+
are not an identity, and would otherwise propagate into login payloads and
272+
tracing context.
273+
"""
274+
project_id = crewai_config.get(_PROJECT_ID_KEY)
275+
if not isinstance(project_id, str):
276+
return None
277+
stripped = project_id.strip()
278+
return stripped or None
256279

257280

258281
def get_or_create_project_id(
@@ -314,10 +337,21 @@ def _get_or_create_project_id_locked(path: Path) -> str | None:
314337
except (tomli.TOMLDecodeError, ValueError):
315338
return None
316339

317-
existing = get_crewai_project_config(pyproject_data).get(_PROJECT_ID_KEY)
318-
if isinstance(existing, str) and existing:
340+
crewai_config = get_crewai_project_config(pyproject_data)
341+
existing = _usable_project_id(crewai_config)
342+
if existing:
319343
return existing
320344

345+
# Only ever add a key to an existing [tool.crewai] table. Creating the table
346+
# would rewrite the pyproject.toml of any directory that merely happens to
347+
# have one, which `crewai run` could otherwise do before it has established
348+
# that the cwd is a CrewAI project at all.
349+
#
350+
# Presence, not truthiness: an empty `[tool.crewai]` table is still a CrewAI
351+
# marker, and get_crewai_project_config returns {} for both cases.
352+
if not _has_crewai_table(pyproject_data):
353+
return None
354+
321355
project_id = str(uuid.uuid4())
322356
updated = _set_project_id(content, project_id)
323357
if updated is None:
@@ -468,6 +502,6 @@ def _set_project_id(content: str, project_id: str) -> str | None:
468502
lines.insert(insert_at, entry)
469503
return "".join(lines)
470504

471-
# No [tool.crewai] table: append one rather than guessing where it belongs.
472-
suffix = "" if content.endswith(("\n", "\r")) or not content else newline
473-
return f"{content}{suffix}{newline}[tool.crewai]{newline}{entry}"
505+
# No [tool.crewai] table. Never create one: that would let this feature
506+
# rewrite the pyproject.toml of a directory that is not a CrewAI project.
507+
return None

lib/crewai/tests/telemetry/test_project_id.py

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ def test_comments_and_formatting_are_preserved(tmp_path):
8888
[
8989
('[project]\nname = "x"\n\n[tool.crewai]\ntype = "crew"\n', "table then EOF"),
9090
('[tool.crewai]\ntype = "crew"', "no trailing newline"),
91-
('[project]\nname = "x"\n', "no tool.crewai table"),
9291
('[project]\nname = "x"\n[tool.crewai]\n[other]\na = 1\n', "empty table"),
9392
(
9493
'[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):
121120
assert "project_id" not in data["build-system"]
122121

123122

123+
def test_absent_tool_crewai_table_is_never_created(tmp_path):
124+
"""Refuse to mint rather than rewrite a non-CrewAI project's pyproject.toml.
125+
126+
`crewai run` in any directory that merely happens to have a pyproject.toml
127+
must not gain a [tool.crewai] table as a side effect.
128+
"""
129+
path = tmp_path / "pyproject.toml"
130+
original = '[project]\nname = "unrelated"\n'
131+
path.write_text(original)
132+
133+
assert get_or_create_project_id(path) is None
134+
assert path.read_text() == original, "unrelated project was modified"
135+
136+
124137
def test_missing_file_is_not_an_error(tmp_path):
125138
assert get_or_create_project_id(tmp_path / "nope.toml") is None
126139

@@ -152,9 +165,15 @@ def test_get_project_id_never_creates_anything(pyproject):
152165
assert pyproject.read_text() == before
153166

154167

155-
def test_blank_id_is_treated_as_absent(tmp_path):
168+
@pytest.mark.parametrize("blank", ['""', "' '", '"\\t"'])
169+
def test_blank_or_whitespace_id_is_treated_as_absent(tmp_path, blank):
170+
"""Whitespace is truthy in Python but is not an identity.
171+
172+
Accepting it would propagate a useless value into login payloads and
173+
tracing context.
174+
"""
156175
path = tmp_path / "pyproject.toml"
157-
path.write_text('[tool.crewai]\ntype = "crew"\nproject_id = ""\n')
176+
path.write_text(f'[tool.crewai]\ntype = "crew"\nproject_id = {blank}\n')
158177

159178
assert get_project_id(path) is None
160179

@@ -169,7 +188,7 @@ def test_malformed_toml_is_never_written_to(tmp_path):
169188
assert path.read_text() == original, "malformed file must be left untouched"
170189

171190

172-
@pytest.mark.parametrize("blank", ['""', "''", '" "'])
191+
@pytest.mark.parametrize("blank", ['""', "''", '" "', '"\\t\\t"'])
173192
def test_blank_existing_id_is_replaced_not_duplicated(tmp_path, blank):
174193
"""A blank id reads as absent; appending would make a duplicate key."""
175194
path = tmp_path / "pyproject.toml"
@@ -182,6 +201,7 @@ def test_blank_existing_id_is_replaced_not_duplicated(tmp_path, blank):
182201
data = parse_toml(content) # would raise on a duplicate key
183202
assert data["tool"]["crewai"]["project_id"] == project_id
184203
assert data["tool"]["crewai"]["type"] == "crew"
204+
assert uuid.UUID(project_id), "must mint a real id, not keep the blank one"
185205

186206

187207
def test_non_string_existing_id_is_replaced(tmp_path):
@@ -252,28 +272,42 @@ def test_lf_file_stays_lf(tmp_path):
252272

253273

254274
def test_concurrent_minting_converges_on_one_id(tmp_path):
255-
"""Two processes minting at once must agree on the persisted id."""
275+
"""Concurrent minters must all return the id that ends up on disk.
276+
277+
Uses threads in one process, so it covers the read-modify-write race rather
278+
than the cross-process lock backend itself.
279+
"""
256280
import threading
257281

282+
workers = 8
258283
path = tmp_path / "pyproject.toml"
259284
path.write_text(CREW_PYPROJECT)
260285

261286
returned: list[str | None] = []
262-
start = threading.Barrier(8)
287+
results_lock = threading.Lock()
288+
# Timed out rather than unbounded: a thread dying before the barrier, or
289+
# blocking on the lock, would otherwise hang CI instead of failing.
290+
start = threading.Barrier(workers, timeout=30)
263291

264292
def mint() -> None:
265293
start.wait()
266-
returned.append(get_or_create_project_id(path))
294+
project_id = get_or_create_project_id(path)
295+
with results_lock:
296+
returned.append(project_id)
267297

268-
threads = [threading.Thread(target=mint) for _ in range(8)]
298+
threads = [threading.Thread(target=mint) for _ in range(workers)]
269299
for thread in threads:
270300
thread.start()
271301
for thread in threads:
272-
thread.join()
302+
thread.join(timeout=30)
303+
304+
assert not [t for t in threads if t.is_alive()], "thread did not finish in time"
305+
assert len(returned) == workers, f"only {len(returned)}/{workers} threads returned"
273306

274307
persisted = parse_toml(path.read_text())["tool"]["crewai"]["project_id"]
275-
assert len(set(returned)) == 1, f"callers disagreed: {set(returned)}"
276-
assert returned[0] == persisted, "returned an id that is not on disk"
308+
assert set(returned) == {persisted}, (
309+
f"callers disagreed with disk: returned={set(returned)} persisted={persisted}"
310+
)
277311

278312

279313
def test_file_mode_is_preserved(tmp_path):

0 commit comments

Comments
 (0)