diff --git a/cecli/coders/base_coder.py b/cecli/coders/base_coder.py index 5a4c6c6cce2..40af87cea47 100755 --- a/cecli/coders/base_coder.py +++ b/cecli/coders/base_coder.py @@ -892,7 +892,7 @@ def get_announcements(self): rel_repo_dir = self.repo.get_rel_repo_dir() num_files = len(self.repo.get_tracked_files()) env_items.append(f"{rel_repo_dir} ({num_files:,} files)") - if num_files > 1000: + if num_files > 1000 and self.verbose: env_items.append( "Warning: For large repos, consider using --subtree-only and .cecli.ignore" ) @@ -1759,6 +1759,7 @@ async def output_task(self, preproc): except (SwitchCoderSignal, SystemExit): raise except Exception as e: + self.error_code = 1 traceback_str = traceback.format_exc() update_error_prefix(traceback_str) @@ -2759,6 +2760,7 @@ async def format_in_executor(): dict(role="assistant", content=self.multi_response_content, prefix=True) ) except Exception as err: + self.error_code = 1 self.mdstream = None lines = traceback.format_exception(type(err), err, err.__traceback__) self.io.tool_warning("".join(lines)) @@ -3253,6 +3255,12 @@ async def call_mcp_tool_from_session(self, session, tool_call): if not isinstance(arguments, dict): arguments = {} + # Some models mirror the OpenAI wire format and wrap the real params + # under a single "arguments"/"parameters"/"params" key. Unwrap so the + # server receives the actual parameters instead of rejecting the call + # with a "missing required parameter" error. + arguments = responses.coerce_tool_structure(arguments) + return await session.call_tool(name=name, arguments=arguments) async def process_tool_calls(self, tool_call_response): @@ -3508,6 +3516,7 @@ async def add_assistant_reply_to_cur_messages(self): # but response.dict() is the Pydantic V1 method name. response_dict = dict(response) except TypeError: + self.error_code = 1 self.io.tool_warning("Response parsing error.") return @@ -4277,9 +4286,21 @@ def calculate_and_show_tokens_and_cost(self, messages, completion=None): cache_hit_tokens = 0 cache_write_tokens = 0 - if completion and hasattr(completion, "usage") and completion.usage is not None: - prompt_tokens = completion.usage.prompt_tokens - completion_tokens = completion.usage.completion_tokens + if ( + completion + and nested.getter(completion, "usage.prompt_tokens") is not None + and nested.getter(completion, "usage.completion_tokens") is not None + ): + prompt_tokens = ( + nested.getter(completion.usage, "prompt_tokens", 0) + or nested.getter(completion.usage, "prompt_eval_count", 0) + or 0 + ) + completion_tokens = ( + nested.getter(completion.usage, "completion_tokens", 0) + or nested.getter(completion.usage, "eval_count", 0) + or 0 + ) cache_hit_tokens = ( getattr(completion.usage, "prompt_cache_hit_tokens", 0) or getattr(completion.usage, "cache_read_input_tokens", 0) diff --git a/cecli/helpers/agents/defaults/memorizer.md b/cecli/helpers/agents/defaults/memorizer.md index ad7311bde45..bd4e1e1d84b 100644 --- a/cecli/helpers/agents/defaults/memorizer.md +++ b/cecli/helpers/agents/defaults/memorizer.md @@ -46,7 +46,6 @@ Use these tags (and invent new ones as needed) to categorise facts: - **relationships** – how modules / systems interact with each other - **decisions** – architectural or design choices that were made and why - **entities** – important classes, functions, or data structures -- **changes** – summaries of changes made and their intent ## Tools @@ -67,10 +66,13 @@ context (e.g. compaction / yield summaries). Your job: 2. Use ReplaceFacts to insert new facts and delete the obsolete ones so the database stays clean and up-to-date. 3. Task specific details are not worth recording, focus on user intention and add facts - that would help them explain the project to another person succinctly + that would help them explain the project to another person succinctly. +4. Record notes on strategy, purpose, structure, expectations, and unintuitive/novel discoveries over project + and activity specific details. + We are trying to preserve why we took the actions we have done, not a log of the actions themselves. Start each response with an incrementing number at the beginning, e.g. "1) ...", "2) ..." -When this number hits at most 10, update what you can and yield. Do not deliberate over many turns. +Before this number hits at most 10, update what you can and yield. Do not deliberate over many turns. Important facts will be easy to search for and extract from the given context. Always prefer **concrete, reusable** facts over vague prose. diff --git a/cecli/helpers/background_commands.py b/cecli/helpers/background_commands.py index 47d4e1c41fa..988909c870f 100644 --- a/cecli/helpers/background_commands.py +++ b/cecli/helpers/background_commands.py @@ -12,6 +12,8 @@ from collections import deque from typing import Dict, List, Optional, Tuple +import xxhash + from cecli.decoding import safe_open try: @@ -419,7 +421,8 @@ def _generate_command_key(cls, command: str) -> str: Unique command key """ with cls._lock: - key = f"bg_{cls._next_id}_{hash(command) % 10000:04d}" + digest = xxhash.xxh64(command.encode("utf-8")).intdigest() + key = f"bg_{cls._next_id}_{digest % 10000:04d}" cls._next_id += 1 return key diff --git a/cecli/helpers/memory/utils.py b/cecli/helpers/memory/utils.py index e7396405947..1da6c8401d7 100644 --- a/cecli/helpers/memory/utils.py +++ b/cecli/helpers/memory/utils.py @@ -6,6 +6,7 @@ - ``add_fact(fact=..., tags=[...])`` – insert a fact with optional tags - ``remove_facts(id_facts=[...])`` – delete facts by id - ``search_facts(words=[...], tags=[...])`` – full-text search the fact store +- ``recent_tags(coder, limit=100)`` – most recently used tags + total count - ``invoke_memorizer(coder, additional_context=...)`` – fire the memorizer sub-agent with current context """ @@ -203,6 +204,51 @@ def search_facts( return results +def recent_tags(coder, limit: int = 100) -> dict: + """Return the most recently used tags plus the total tag count. + + Tags are deduplicated and ordered by the recency of the most recent + fact they are attached to (facts sorted by ``id_fact`` descending), + then truncated to *limit* entries. + + Args: + coder: The active Coder instance (used to locate the project root). + limit: Maximum number of recent tags to return (default 100). + + Returns: + A dict with keys ``tags`` (list of tag strings, most recent first) + and ``total`` (int count of all distinct tags in the store). + """ + + init_db(root=coder.root) + + conn = _get_connection(root=coder.root) + cursor = conn.cursor() + + # Tags ordered by the recency of the most recent fact they tag: + # sort facts by id descending and dedupe on the tag name. + cursor.execute( + """ + SELECT t.tag + FROM Facts f + JOIN FactTags ft ON ft.id_fact = f.id_fact + JOIN Tags t ON t.id_tag = ft.id_tag + GROUP BY t.tag + ORDER BY MAX(f.id_fact) DESC + LIMIT ? + """, + (limit,), + ) + + tags = [row["tag"] for row in cursor.fetchall()] + + cursor.execute("SELECT COUNT(*) AS total FROM Tags") + + total = cursor.fetchone()["total"] + + return {"tags": tags, "total": total} + + async def invoke_memorizer( coder, additional_context: str = "", @@ -242,6 +288,14 @@ async def invoke_memorizer( # Gather context pieces parts: list[str] = [] + # Recent tags (deduplicated, most recently used first) plus the total + # tag count — first context part so the memorizer sees active tags + recent = recent_tags(coder) + + tag_list = ", ".join(recent["tags"]) if recent["tags"] else "(none)" + + parts.append(f"## Recent Tags\n\n{tag_list}\n\n## Total Tags in DB\n\n{recent['total']}") + # Latest user message last_user = getattr(coder, "last_user_message", "") diff --git a/cecli/helpers/responses.py b/cecli/helpers/responses.py index 4eefc575393..e455d08db03 100644 --- a/cecli/helpers/responses.py +++ b/cecli/helpers/responses.py @@ -367,20 +367,24 @@ def unprefix_tool_call(tool_call): def parse_tool_arguments(args_string: str) -> dict: - """Parse tool-call arguments, merging glued ``{…}{} {…}`` object fragments.""" + """Parse tool-call arguments, merging glued ``{…}{} {…}`` object fragments. + + Also unwraps a single ``arguments``/``parameters``/``params`` wrapper key + that some models emit when mirroring the OpenAI wire format. + """ text = (args_string or "").strip() if not text: return {} try: parsed = json.loads(text) if isinstance(parsed, dict): - return parsed + return coerce_tool_structure(parsed) except json.JSONDecodeError: pass parsed = try_parse_json_value(text) if isinstance(parsed, dict): - return parsed + return coerce_tool_structure(parsed) chunks = utils.split_concatenated_json(text) if len(chunks) <= 1: @@ -388,18 +392,18 @@ def parse_tool_arguments(args_string: str) -> dict: return {} lone = try_parse_json_value(chunks[0]) if isinstance(lone, dict): - return lone + return coerce_tool_structure(lone) try: json_string = json_repair.repair_json(chunks[0], ensure_ascii=False) single = json.loads(json_string) except json.JSONDecodeError as err: return {"@error": f"Malformed JSON arguments: {err}"} - return single if isinstance(single, dict) else {} + return coerce_tool_structure(single) if isinstance(single, dict) else {} merged = merge_glued_json_objects(chunks) if merged is not None: - return merged + return coerce_tool_structure(merged) return { "@error": "Could not merge glued JSON objects: argument fragments are not all JSON objects" @@ -618,3 +622,40 @@ def _parse_bracket_arguments(payload_str: str) -> dict: arguments[key] = val_str return arguments + + +def coerce_tool_structure(args: dict) -> dict: + """Unwrap a single ``arguments``/``parameters``/``params`` wrapper key. + + Some models mirror the OpenAI wire format and emit the real params nested + under a single top-level ``arguments`` key (as a dict or a JSON-encoded + string) instead of as the params dict itself. Without normalization the + wrapper key is forwarded verbatim to the tool, which then fails required + parameter validation (e.g. MCP servers reporting "missing required + parameter"). + + Only unwraps when ``args`` has exactly one key matching one of the wrapper + names and that value is a dict (or a JSON string that parses to a dict), so + tools that legitimately declare a parameter named ``arguments`` are never + clobbered. + """ + if not isinstance(args, dict) or len(args) != 1: + return args + + for key in ("arguments", "parameters", "params"): + if key not in args: + continue + + value = args[key] + if isinstance(value, str): + try: + value = json.loads(value) + except json.JSONDecodeError: + return args + + if isinstance(value, dict): + return value + + return args + + return args diff --git a/cecli/helpers/skills.py b/cecli/helpers/skills.py index 2d1667a2bf8..7d6fbea31f4 100644 --- a/cecli/helpers/skills.py +++ b/cecli/helpers/skills.py @@ -71,7 +71,29 @@ def __init__( if default_skill_dir not in directory_paths: directory_paths = [default_skill_dir] + list(directory_paths) - self.directory_paths = [Path(p).expanduser().resolve() for p in directory_paths] + # Resolve every path and drop exact directory duplicates. + resolved_paths = [] + seen_paths = set() + for p in directory_paths: + try: + path = Path(p).expanduser().resolve() + except Exception: + continue + if path in seen_paths: + continue + seen_paths.add(path) + resolved_paths.append(path) + + # Order paths: local project dirs first, then configured, home dirs last. + git_root_path = Path(git_root).expanduser().resolve() if git_root else None + ordered = sorted( + enumerate(resolved_paths), + key=lambda item: ( + self._directory_priority(item[1], git_root_path), + item[0], + ), + ) + self.directory_paths = [path for _, path in ordered] self.include_list = set(include_list) if include_list else None self.exclude_list = set(exclude_list) if exclude_list else set() self.git_root = Path(git_root).expanduser().resolve() if git_root else None @@ -111,6 +133,35 @@ def __init__( # Save initial state from config + @staticmethod + def _directory_priority(path: Path, git_root: Optional[Path] = None) -> int: + """Return the ordering priority of a skill directory. + + Lower values are scanned first and therefore win by-name conflicts: + + 0 - local project directory (under the git root or working directory) + 1 - any other configured directory + 2 - a home directory (including the implicit ``~/.cecli/skills`` default) + """ + home = Path.home().resolve() + + # Implicit default skills dir is always treated as a home dir. + if path == (home / ".cecli" / "skills"): + return 2 + + local_anchor = git_root if git_root is not None else Path.cwd() + try: + path.relative_to(local_anchor) + return 0 + except ValueError: + pass + + try: + path.relative_to(home) + return 2 + except ValueError: + return 1 + def _get_coder(self): """Return coder via weak reference, or None if collected.""" if self._coder_ref is not None: @@ -176,6 +227,7 @@ def find_skills(self, reload: bool = False) -> List[SkillMetadata]: return self._skills_find_cache skills = [] + seen_names: set[str] = set() for directory_path in self.directory_paths: directory_path = Path(directory_path) @@ -193,6 +245,11 @@ def find_skills(self, reload: bool = False) -> List[SkillMetadata]: metadata = self._parse_skill_metadata(skill_md_path) skill_name = metadata.name + # First directory wins for duplicate skill names. + if skill_name in seen_names: + continue + seen_names.add(skill_name) + # Apply include/exclude filters if self.include_list and skill_name not in self.include_list: continue diff --git a/cecli/main.py b/cecli/main.py index 957717bec4a..92694d5e83e 100644 --- a/cecli/main.py +++ b/cecli/main.py @@ -1437,7 +1437,7 @@ def get_io(pretty): ) io.tool_output(f"Cur working dir: {Path.cwd()}") io.tool_output(f"Git working dir: {git_root}") - if args.stream and args.cache_prompts: + if args.stream and args.cache_prompts and args.verbose: io.tool_warning("Cost estimates may be inaccurate when using streaming and caching.") if args.load: await commands.execute("load", args.load) diff --git a/cecli/tools/command.py b/cecli/tools/command.py index 6a9b762e2fc..3712fcd5c0a 100644 --- a/cecli/tools/command.py +++ b/cecli/tools/command.py @@ -108,7 +108,7 @@ def _hash_command(command): if not command: return command - return xxhash.xxh64(command).hexdigest() + return xxhash.xxh64(command.encode("utf-8")).hexdigest() @classmethod async def execute( diff --git a/cecli/tools/search_facts.py b/cecli/tools/search_facts.py index e93a90f9d71..f0fba66259c 100644 --- a/cecli/tools/search_facts.py +++ b/cecli/tools/search_facts.py @@ -64,11 +64,6 @@ async def execute(cls, coder, **kwargs): try: results = search_facts(coder, words=words, tags=tags) - if not results: - # Re-search without tags as fallback - if tags: - results = search_facts(coder, words=words, tags=None) - for r in results: response.append_result( { diff --git a/cecli/website/docs/config/skills.md b/cecli/website/docs/config/skills.md index acddc7a2e7d..813c49f5bd0 100644 --- a/cecli/website/docs/config/skills.md +++ b/cecli/website/docs/config/skills.md @@ -77,6 +77,14 @@ Skills are configured through the `agent-config` parameter in the YAML configura - **`skills_includelist`**: Array of skill names to include (whitelist) - **`skills_excludelist`**: Array of skill names to exclude (blacklist) +> **Duplicate skill names**: When the same skill name is found in more than +> one configured directory, only one copy is loaded. Directories are scanned +> in priority order: local project directories (e.g. `./.cecli/skills`) first, +> then other configured directories in the order they are listed, with home +> directories (e.g. `~/skills` and the implicit `~/.cecli/skills` default) +> last. If no `skills_paths` are configured, the only directory searched is +> `~/.cecli/skills`. + Complete configuration example in YAML configuration file (`.cecli.conf.yml` or `~/.cecli.conf.yml`): ```yaml diff --git a/tests/basic/test_main.py b/tests/basic/test_main.py index 87b47b9a4f9..31a0e9423cd 100644 --- a/tests/basic/test_main.py +++ b/tests/basic/test_main.py @@ -1302,31 +1302,6 @@ def test_model_accepts_settings_attribute(dummy_io, git_temp_dir, mocker): mock_instance.set_thinking_tokens.assert_not_called() -@pytest.mark.parametrize( - "flags,should_warn", - [ - (["--stream", "--cache-prompts"], True), - (["--stream"], False), - (["--cache-prompts", "--no-stream"], False), - ], - ids=["stream_and_cache", "stream_only", "cache_only"], -) -def test_stream_cache_warning(dummy_io, git_temp_dir, mocker, flags, should_warn): - """Test warning shown only when both streaming and caching are enabled.""" - MockInputOutput = mocker.patch("cecli.io.InputOutput", autospec=True) - mock_io_instance = MockInputOutput.return_value - mock_io_instance.pretty = True - args = flags + ["--exit", "--yes-always"] - main(args, **dummy_io) - if should_warn: - mock_io_instance.tool_warning.assert_called_with( - "Cost estimates may be inaccurate when using streaming and caching." - ) - else: - for call in mock_io_instance.tool_warning.call_args_list: - assert "Cost estimates may be inaccurate" not in call[0][0] - - def test_argv_file_respects_git(dummy_io, git_temp_dir): fname = Path("not_in_git.txt") fname.touch() diff --git a/tests/basic/test_sanity_check_repo.py b/tests/basic/test_sanity_check_repo.py index 114a4de952e..e344a4e23eb 100644 --- a/tests/basic/test_sanity_check_repo.py +++ b/tests/basic/test_sanity_check_repo.py @@ -1,7 +1,9 @@ import asyncio import os import shutil +import stat import struct +import sys from unittest import mock import pytest @@ -82,6 +84,16 @@ def get_tracked_files_side_effect(): return mock_repo +def _rmtree_readonly(func, path, exc): + """Retry a failed rmtree operation after clearing the read-only attribute. + + Git marks loose object files read-only, which prevents them from being + deleted on Windows (PermissionError: [WinError 5] Access is denied). + """ + os.chmod(path, stat.S_IWRITE) + func(path) + + async def test_detached_head_state(create_repo, mock_io): repo_path, repo = create_repo # Detach the HEAD @@ -160,8 +172,14 @@ async def test_bare_repository(create_repo, mock_io, tmp_path): async def test_sanity_check_repo_with_corrupt_repo(create_repo, mock_io): repo_path, repo = create_repo - # Simulate a corrupt repository by removing the .git directory - shutil.rmtree(os.path.join(repo_path, ".git")) + # Simulate a corrupt repository by removing the .git directory. + # Git stores loose objects as read-only files, so on Windows we must + # clear the read-only attribute before they can be deleted. + git_dir = os.path.join(repo_path, ".git") + if sys.version_info >= (3, 12): + shutil.rmtree(git_dir, onexc=_rmtree_readonly) + else: + shutil.rmtree(git_dir, onerror=_rmtree_readonly) # Create the mock 'repo' object with GitError git_error = GitError("Unable to read git repository, it may be corrupt?") diff --git a/tests/tools/test_tool_arguments.py b/tests/tools/test_tool_arguments.py index ab734be40ec..3cbce3e3a97 100644 --- a/tests/tools/test_tool_arguments.py +++ b/tests/tools/test_tool_arguments.py @@ -9,6 +9,7 @@ from cecli.coders.base_coder import Coder from cecli.helpers.responses import ( _repair_local_model_json_text, + coerce_tool_structure, extract_tools_from_content_json, merge_glued_json_objects, parse_tool_arguments, @@ -298,3 +299,96 @@ def test_parse_tool_arguments_uneven_glued_objects_with_list(): # The function tries to parse, failing on the mixed glued content assert "@error" in result assert "Could not merge glued JSON objects" in result["@error"] + + +def test_coerce_tool_structure_dict_value(): + """A single 'arguments' key wrapping a dict should be unwrapped.""" + assert coerce_tool_structure({"arguments": {"query": "x"}}) == {"query": "x"} + + +def test_coerce_tool_structure_json_string_value(): + """A single 'arguments' key holding a JSON-encoded string should be unwrapped.""" + assert coerce_tool_structure({"arguments": '{"query": "x"}'}) == {"query": "x"} + + +def test_coerce_tool_structure_parameters_key(): + """'parameters' and 'params' wrappers should also be unwrapped.""" + assert coerce_tool_structure({"parameters": {"path": "."}}) == {"path": "."} + assert coerce_tool_structure({"params": {"path": "."}}) == {"path": "."} + + +def test_coerce_tool_structure_ignores_multi_key_dict(): + """A dict with other keys alongside 'arguments' is not a wrapper and is left alone.""" + wrapped = {"arguments": {"query": "x"}, "extra": 1} + assert coerce_tool_structure(wrapped) is wrapped + + +def test_coerce_tool_structure_ignores_non_dict_values(): + """Non-dict (or unparsable) wrapper values are left alone.""" + assert coerce_tool_structure({"arguments": 5}) == {"arguments": 5} + assert coerce_tool_structure({"arguments": ["a"]}) == {"arguments": ["a"]} + assert coerce_tool_structure({"arguments": "not json"}) == {"arguments": "not json"} + + +def test_coerce_tool_structure_ignores_empty(): + """An empty dict is not a wrapper.""" + assert coerce_tool_structure({}) == {} + + +def test_parse_tool_arguments_unwraps_wire_format_wrapper(): + """A model mirroring the OpenAI wire format should still bind real params.""" + assert parse_tool_arguments('{"arguments": {"query": "repo:cecli"}}') == {"query": "repo:cecli"} + + +def test_parse_tool_arguments_unwraps_wire_format_string(): + """JSON-encoded string form of the wire-format wrapper should be unwrapped.""" + assert parse_tool_arguments('{"arguments": "{\\"query\\": \\"repo:cecli\\"}"}') == { + "query": "repo:cecli" + } + + +class _FakeMcpSession: + """Minimal stand-in for an mcp ClientSession that records the call.""" + + def __init__(self): + self.calls = [] + + async def call_tool(self, name=None, arguments=None): + self.calls.append((name, arguments)) + return SimpleNamespace(content=[]) + + +def _make_mini_coder(): + class MiniCoder(Coder): + def __init__(self): + pass + + return MiniCoder.__new__(MiniCoder) + + +async def test_call_mcp_tool_from_session_unwraps_arguments_key(): + """call_mcp_tool_from_session must unwrap a wire-format wrapper before session.call_tool.""" + coder = _make_mini_coder() + session = _FakeMcpSession() + tool_call = { + "function": { + "name": "github--search_code", + "arguments": '{"arguments": "{\\"query\\": \\"repo:cecli\\"}"}', + } + } + await coder.call_mcp_tool_from_session(session, tool_call) + assert session.calls == [("github--search_code", {"query": "repo:cecli"})] + + +async def test_call_mcp_tool_from_session_passes_plain_params_through(): + """Normal params must pass through call_mcp_tool_from_session unchanged.""" + coder = _make_mini_coder() + session = _FakeMcpSession() + tool_call = { + "function": { + "name": "github--list_issues", + "arguments": '{"owner": "cecli-dev", "repo": "cecli"}', + } + } + await coder.call_mcp_tool_from_session(session, tool_call) + assert session.calls == [("github--list_issues", {"owner": "cecli-dev", "repo": "cecli"})]