From 195cfb025b3e0b04e2f15fb1ba7344650a3d62c1 Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Mon, 3 Aug 2026 11:38:08 -0700 Subject: [PATCH] fix: harden project_id minting against TOML corruption; address review Several reviewers found ways the raw-text edit could produce invalid TOML. Each is now fixed and covered by a test that fails without the fix. Duplicate project_id key (Cursor bugbot, Copilot x2): - get_project_id() reports a blank or non-string value as "absent", so a file containing `project_id = ""` took the insert path and gained a second project_id line - a duplicate key, and therefore invalid TOML that no tomli-based tool could read afterwards. - _insert_project_id is now _set_project_id: it replaces an existing assignment inside [tool.crewai] instead of appending unconditionally. Table header with a trailing comment (CodeRabbit major, Cursor bugbot): - `[tool.crewai] # config` is valid TOML but failed exact string equality, so the fallback appended a second [tool.crewai] header - a redefined table, also invalid TOML, and silent because get_project_id swallows the resulting decode error. - Added _is_table_header(), which tolerates a trailing comment and does not match similar names such as [tool.crewai-extra]. Writing into malformed TOML (Cursor bugbot, Copilot): - get_or_create_project_id relied on get_project_id, which cannot distinguish "no id" from "unparsable file", so it appended to files it could not parse. - The locked path now parses explicitly and bails on a decode error, and re-parses the updated content before writing, so this feature can never be the reason a project's pyproject.toml stops parsing. Concurrency and atomicity (CodeRabbit major): - Two CLI processes could both see no id, mint different uuids, and clobber each other, leaving a caller holding an id that is not on disk. Minting now takes the existing crewai_core cross-process lock, re-reads under it, and returns the id that persists. - Writes go through a temp file in the same directory plus os.replace, so an interruption cannot truncate pyproject.toml. File mode is copied across, and the temp file is removed on failure. - os.replace only needs a writable directory, which would have let an atomic write silently overwrite a file the user marked read-only; writability is now checked explicitly so that case still returns None. Line endings (CodeRabbit): - Path.read_text/write_text normalized CRLF to LF, so minting would rewrite a CRLF-committed file entirely. Read and write now use newline="" and the inserted line ending is derived from the existing content. Default create path skipped minting (Cursor bugbot): - `crewai create crew` defaults to create_json_crew; only the --classic and flow paths minted, so most new projects had no id until a later command. Wired into create_json_crew as well. Verified all three paths now mint distinct ids. Do not mint during login (CodeRabbit major): - ToolCommand.login ran get_or_create_project_id, which is outside the sanctioned minting commands and is invoked by `crewai tools create` from a freshly scaffolded directory before the project is persisted. It now uses the read-only get_project_id. Verified login leaves pyproject.toml untouched. Not applied: Copilot asked for a console message when an id is written, in create_crew and create_flow. Minting was made deliberately silent in the previous commit, so the (id, created) tuple and the announcement are both gone by design. Tests: 32 in test_project_id.py, up from 18. New cases cover blank and non-string existing ids, three commented-header forms, similar table names, malformed input, CRLF and LF preservation, concurrent minting convergence, file-mode preservation, and temp-file cleanup. Confirmed the header and duplicate-key tests fail when the fixes are reverted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t --- lib/cli/src/crewai_cli/create_json_crew.py | 4 + lib/cli/src/crewai_cli/tools/main.py | 7 +- lib/crewai-core/src/crewai_core/project.py | 175 +++++++++++++++--- lib/crewai/tests/telemetry/test_project_id.py | 137 ++++++++++++++ 4 files changed, 300 insertions(+), 23 deletions(-) diff --git a/lib/cli/src/crewai_cli/create_json_crew.py b/lib/cli/src/crewai_cli/create_json_crew.py index 84426900e..d3aa74102 100644 --- a/lib/cli/src/crewai_cli/create_json_crew.py +++ b/lib/cli/src/crewai_cli/create_json_crew.py @@ -18,6 +18,7 @@ from crewai_cli.model_catalog import get_provider_models from crewai_cli.tui_picker import pick_many, pick_one from crewai_cli.utils import ( enable_prompt_line_editing, + get_or_create_project_id, is_dmn_mode_enabled, load_env_vars, render_template, @@ -968,6 +969,9 @@ def create_json_crew( for model in models: _setup_env(folder_path, model) + # Minted at creation so the project has a stable identity from run one. + # This is the default `crewai create crew` path, not just --classic. + get_or_create_project_id(folder_path / "pyproject.toml") initialize_if_git_available(folder_path) click.echo() diff --git a/lib/cli/src/crewai_cli/tools/main.py b/lib/cli/src/crewai_cli/tools/main.py index 3f3caf21d..7f7fcf51b 100644 --- a/lib/cli/src/crewai_cli/tools/main.py +++ b/lib/cli/src/crewai_cli/tools/main.py @@ -16,8 +16,8 @@ from crewai_cli.config import Settings from crewai_cli.constants import DEFAULT_CREWAI_ENTERPRISE_URL from crewai_cli.utils import ( build_env_with_tool_repository_credentials, - get_or_create_project_id, get_project_description, + get_project_id, get_project_name, get_project_version, read_toml, @@ -229,9 +229,12 @@ class ToolCommand(BaseCommand, PlusAPIMixin): def login(self) -> None: get_user_id = _require_get_user_id() + # Read-only: login is not one of the sanctioned minting commands, and + # `crewai tools create` calls it from inside a freshly scaffolded + # directory before the tool project is persisted. login_response = self.plus_api_client.login_to_tool_repository( user_identifier=get_user_id(), - project_id=get_or_create_project_id(), + project_id=get_project_id(), ) if login_response.status_code != 200: diff --git a/lib/crewai-core/src/crewai_core/project.py b/lib/crewai-core/src/crewai_core/project.py index f3059ebc2..fc971f1ae 100644 --- a/lib/crewai-core/src/crewai_core/project.py +++ b/lib/crewai-core/src/crewai_core/project.py @@ -3,14 +3,19 @@ from __future__ import annotations from functools import reduce +import os from pathlib import Path, PureWindowsPath +import shutil import sys +import tempfile from typing import Any import uuid from rich.console import Console import tomli +from crewai_core.lock_store import lock as store_lock + if sys.version_info >= (3, 11): import tomllib @@ -270,26 +275,68 @@ def get_or_create_project_id( The project id, or None when ``pyproject.toml`` is missing, malformed, or not writable. Best-effort - never raises. """ - existing = get_project_id(pyproject_path) - if existing: - return existing - path = Path(pyproject_path) if not path.is_file(): return None + # Cross-process lock: two CLI invocations could otherwise both see no id, + # mint different uuids, and clobber each other - leaving one caller holding + # an id that is not the one on disk. try: - content = path.read_text(encoding="utf-8") + with store_lock(_project_id_lock_name(path)): + return _get_or_create_project_id_locked(path) + except Exception: + # Lock backend unavailable; a torn write is worse than no id. + return get_project_id(path) + + +def _project_id_lock_name(path: Path) -> str: + """Return a stable lock name for a project's ``pyproject.toml``.""" + return f"file:{os.path.realpath(path)}" + + +def _get_or_create_project_id_locked(path: Path) -> str | None: + """Read-modify-write the project id while holding the lock. + + Re-reads under the lock so a concurrent minter's id is returned rather than + overwritten. + """ + try: + content = _read_preserving_newlines(path) except OSError: return None + # Parse here rather than relying on get_project_id, which reports malformed + # files and absent ids identically. Appending to a file we cannot parse + # would corrupt it further, so bail instead. + try: + pyproject_data = parse_toml(content) + except (tomli.TOMLDecodeError, ValueError): + return None + + existing = get_crewai_project_config(pyproject_data).get(_PROJECT_ID_KEY) + if isinstance(existing, str) and existing: + return existing + project_id = str(uuid.uuid4()) - updated = _insert_project_id(content, project_id) + updated = _set_project_id(content, project_id) if updated is None: return None + # Verify before writing: never leave a project with unparsable TOML because + # of this feature. try: - path.write_text(updated, encoding="utf-8") + parse_toml(updated) + except (tomli.TOMLDecodeError, ValueError): + return None + + # Checked explicitly: os.replace only needs a writable *directory*, so an + # atomic write would happily overwrite a file the user marked read-only. + if not os.access(path, os.W_OK): + return None + + try: + _write_atomically(path, updated) except OSError: # Read-only checkout, permissions, container FS - not worth failing over. return None @@ -297,44 +344,130 @@ def get_or_create_project_id( return project_id -def _insert_project_id(content: str, project_id: str) -> str | None: - """Add ``project_id`` to the ``[tool.crewai]`` table in TOML source text. +def _read_preserving_newlines(path: Path) -> str: + """Read text without translating line endings. + + ``Path.read_text`` normalizes CRLF to LF, so a later write would silently + convert a CRLF-committed file to LF and show up as a whole-file diff. + """ + with path.open("r", encoding="utf-8", newline="") as handle: + return handle.read() + + +def _write_atomically(path: Path, content: str) -> None: + """Replace ``path`` with ``content`` via a temp file in the same directory. + + An interrupted or concurrent write must never leave a truncated + ``pyproject.toml`` behind. + """ + directory = path.parent + handle = tempfile.NamedTemporaryFile( + "w", + encoding="utf-8", + newline="", + dir=directory, + prefix=f".{path.name}.", + suffix=".tmp", + delete=False, + ) + tmp_path = Path(handle.name) + try: + with handle: + handle.write(content) + handle.flush() + os.fsync(handle.fileno()) + shutil.copymode(path, tmp_path) + os.replace(tmp_path, path) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + + +def _is_table_header(line: str, table: str) -> bool: + """True if ``line`` opens ``table``, tolerating a trailing inline comment. + + ``[tool.crewai] # config`` is valid TOML. Comparing the stripped line to + the header verbatim would miss it, and the caller would then append a second + ``[tool.crewai]`` header - a duplicate table definition, which is invalid + TOML. + """ + stripped = line.strip() + if not stripped.startswith("["): + return False + closing = stripped.find("]") + if closing == -1: + return False + if stripped[: closing + 1] != table: + return False + remainder = stripped[closing + 1 :].strip() + return remainder == "" or remainder.startswith("#") + + +def _is_any_table_header(line: str) -> bool: + """True if ``line`` opens any TOML table or array-of-tables.""" + return line.lstrip().startswith("[") + + +def _project_id_key_index(lines: list[str], start: int, end: int) -> int | None: + """Return the index of an existing ``project_id`` assignment in a range.""" + for index in range(start, end): + candidate = lines[index].strip() + if not candidate or candidate.startswith("#"): + continue + key, separator, _ = candidate.partition("=") + if separator and key.strip().strip("\"'") == _PROJECT_ID_KEY: + return index + return None + + +def _set_project_id(content: str, project_id: str) -> str | None: + """Set ``project_id`` in the ``[tool.crewai]`` table of TOML source text. + + Replaces an existing ``project_id`` assignment rather than adding a second + one: a blank or non-string value reads as "absent", and appending in that + case would produce a duplicate key and therefore invalid TOML. Edits the raw text rather than round-tripping through a TOML writer so formatting, ordering, and comments in the rest of the file are preserved. Args: content: Full contents of a ``pyproject.toml``. - project_id: The id to insert. + project_id: The id to set. Returns: Updated file contents, or None if the edit could not be made safely. """ lines = content.splitlines(keepends=True) - entry = f'{_PROJECT_ID_KEY} = "{project_id}"\n' + newline = "\r\n" if "\r\n" in content else "\n" + entry = f'{_PROJECT_ID_KEY} = "{project_id}"{newline}' for index, line in enumerate(lines): - if line.strip() != "[tool.crewai]": + if not _is_table_header(line, "[tool.crewai]"): continue - # Insert at the end of the table, before the next table header, so the - # key cannot land inside a different section. - insert_at = len(lines) + # Bound the table: everything up to the next table header. + table_end = len(lines) for offset in range(index + 1, len(lines)): - if lines[offset].lstrip().startswith("["): - insert_at = offset + if _is_any_table_header(lines[offset]): + table_end = offset break + existing = _project_id_key_index(lines, index + 1, table_end) + if existing is not None: + lines[existing] = entry + return "".join(lines) + # Step back over trailing blank lines so the key stays in the table. + insert_at = table_end while insert_at > index + 1 and not lines[insert_at - 1].strip(): insert_at -= 1 - if insert_at > 0 and not lines[insert_at - 1].endswith("\n"): - lines[insert_at - 1] += "\n" + if insert_at > 0 and not lines[insert_at - 1].endswith(("\n", "\r")): + lines[insert_at - 1] += newline 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") or not content else "\n" - return f"{content}{suffix}\n[tool.crewai]\n{entry}" + suffix = "" if content.endswith(("\n", "\r")) or not content else newline + return f"{content}{suffix}{newline}[tool.crewai]{newline}{entry}" diff --git a/lib/crewai/tests/telemetry/test_project_id.py b/lib/crewai/tests/telemetry/test_project_id.py index 1eb1cb4b6..a2a95aa34 100644 --- a/lib/crewai/tests/telemetry/test_project_id.py +++ b/lib/crewai/tests/telemetry/test_project_id.py @@ -159,6 +159,143 @@ def test_blank_id_is_treated_as_absent(tmp_path): assert get_project_id(path) is None +def test_malformed_toml_is_never_written_to(tmp_path): + """Appending to a file we cannot parse would corrupt it further.""" + path = tmp_path / "pyproject.toml" + original = 'this is not [valid toml\nproject_id = "x' + path.write_text(original) + + assert get_or_create_project_id(path) is None + assert path.read_text() == original, "malformed file must be left untouched" + + +@pytest.mark.parametrize("blank", ['""', "''", '" "']) +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" + path.write_text(f'[tool.crewai]\ntype = "crew"\nproject_id = {blank}\n') + + project_id = get_or_create_project_id(path) + + content = path.read_text() + assert content.count("project_id") == 1, f"duplicate key: {content!r}" + data = parse_toml(content) # would raise on a duplicate key + assert data["tool"]["crewai"]["project_id"] == project_id + assert data["tool"]["crewai"]["type"] == "crew" + + +def test_non_string_existing_id_is_replaced(tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text("[tool.crewai]\nproject_id = 42\n") + + project_id = get_or_create_project_id(path) + + data = parse_toml(path.read_text()) + assert data["tool"]["crewai"]["project_id"] == project_id + assert isinstance(project_id, str) + + +@pytest.mark.parametrize( + "header", + [ + "[tool.crewai] # crewai config", + "[tool.crewai]# no space", + "[tool.crewai]\t# tab then comment", + ], +) +def test_table_header_with_trailing_comment_is_found(tmp_path, header): + """A commented header is valid TOML; missing it appends a duplicate table.""" + path = tmp_path / "pyproject.toml" + path.write_text(f'{header}\ntype = "crew"\n') + + project_id = get_or_create_project_id(path) + + content = path.read_text() + assert content.count("[tool.crewai]") == 1, f"duplicate table: {content!r}" + data = parse_toml(content) # would raise on a redefined table + assert data["tool"]["crewai"]["project_id"] == project_id + assert data["tool"]["crewai"]["type"] == "crew" + + +def test_similar_table_names_are_not_matched(tmp_path): + """[tool.crewai-extra] must not be mistaken for [tool.crewai].""" + path = tmp_path / "pyproject.toml" + path.write_text('[tool.crewai-extra]\nfoo = 1\n\n[tool.crewai]\ntype = "crew"\n') + + project_id = get_or_create_project_id(path) + + data = parse_toml(path.read_text()) + assert data["tool"]["crewai"]["project_id"] == project_id + assert "project_id" not in data["tool"]["crewai-extra"] + + +def test_crlf_line_endings_are_preserved(tmp_path): + """read_text/write_text would silently rewrite the whole file as LF.""" + path = tmp_path / "pyproject.toml" + path.write_bytes(b'[project]\r\nname = "x"\r\n\r\n[tool.crewai]\r\ntype = "crew"\r\n') + + project_id = get_or_create_project_id(path) + + raw = path.read_bytes() + assert b"\r\n" in raw + assert raw.count(b"\n") == raw.count(b"\r\n"), "mixed line endings introduced" + assert parse_toml(raw.decode())["tool"]["crewai"]["project_id"] == project_id + + +def test_lf_file_stays_lf(tmp_path): + path = tmp_path / "pyproject.toml" + path.write_bytes(b'[tool.crewai]\ntype = "crew"\n') + + get_or_create_project_id(path) + + assert b"\r\n" not in path.read_bytes() + + +def test_concurrent_minting_converges_on_one_id(tmp_path): + """Two processes minting at once must agree on the persisted id.""" + import threading + + path = tmp_path / "pyproject.toml" + path.write_text(CREW_PYPROJECT) + + returned: list[str | None] = [] + start = threading.Barrier(8) + + def mint() -> None: + start.wait() + returned.append(get_or_create_project_id(path)) + + threads = [threading.Thread(target=mint) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + 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" + + +def test_file_mode_is_preserved(tmp_path): + """The atomic replace must not widen permissions on pyproject.toml.""" + path = tmp_path / "pyproject.toml" + path.write_text(CREW_PYPROJECT) + path.chmod(0o600) + + get_or_create_project_id(path) + + assert path.stat().st_mode & 0o777 == 0o600 + + +def test_no_temp_files_left_behind(tmp_path): + path = tmp_path / "pyproject.toml" + path.write_text(CREW_PYPROJECT) + + get_or_create_project_id(path) + + assert [p.name for p in tmp_path.iterdir()] == ["pyproject.toml"] + + def test_ids_are_unique_across_projects(tmp_path): ids = set() for name in ("a", "b", "c"):