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