diff --git a/cheetahclaws/skill/tools.py b/cheetahclaws/skill/tools.py index e63ad651..89ae4347 100644 --- a/cheetahclaws/skill/tools.py +++ b/cheetahclaws/skill/tools.py @@ -39,10 +39,50 @@ } +def _normalize_args(args: object) -> tuple[bool, str]: + """Coerce model-supplied `args` into a plain string, or say why not. + + The schema declares `args` as a string, but small/open models on custom + providers often wrap every tool input in an object (they see + `input: {...}` for all tools). Before the check, `substitute_arguments` + did `prompt.replace("$ARGUMENTS", args)` and crashed with a raw + `TypeError: replace() argument 2 must be str, not dict` — an error the + model cannot correct, so it retried the same call and gave up (#182). + + Accepted shapes: + - str: unchanged. + - dict with a single string value (the observed `{"args": "..."}`): unwrap. + - list of str: join with spaces (the observed `["--env", "prod"]`). + Anything else returns (False, explanation) so the caller can hand the + model a concrete instruction instead of a traceback. + """ + if isinstance(args, str): + return True, args + if isinstance(args, dict): + values = [v for v in args.values() if isinstance(v, str)] + if len(args) == 1 and len(values) == 1: + return True, values[0] + return False, ( + f"Skill 'args' must be a string, got an object with keys " + f"{sorted(args)}. Pass args as a single string, e.g. " + f"\"--corpus X --k 5\"." + ) + if isinstance(args, list): + if all(isinstance(v, str) for v in args): + return True, " ".join(args) + return False, "Skill 'args' list must contain only strings." + return False, ( + f"Skill 'args' must be a string, got {type(args).__name__}. " + f"Pass args as a single string." + ) + + def _skill_tool(params: dict, config: dict) -> str: """Execute a skill by name and return its output.""" skill_name = params.get("name", "").strip() - args = params.get("args", "") + args_ok, args = _normalize_args(params.get("args", "")) + if not args_ok: + return f"Error: {args}" # Look up by name first, then by trigger skill = None diff --git a/tests/test_skills.py b/tests/test_skills.py index b7c0f6c6..0d49142f 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -48,6 +48,7 @@ def skill_dir(tmp_path, monkeypatch): skills_dir.mkdir() (skills_dir / "commit.md").write_text(COMMIT_MD, encoding="utf-8") (skills_dir / "review.md").write_text(REVIEW_MD, encoding="utf-8") + (skills_dir / "deploy.md").write_text(ARGS_MD, encoding="utf-8") monkeypatch.setattr(_loader, "_get_skill_paths", lambda: [skills_dir]) # Also patch the builtin list to be empty so tests are predictable @@ -133,9 +134,9 @@ def test_parse_skill_file_allowed_tools(tmp_path): def test_load_skills(skill_dir): skills = load_skills() - assert len(skills) == 2 + assert len(skills) == 3 names = {s.name for s in skills} - assert names == {"commit", "review"} + assert names == {"commit", "review", "deploy"} def test_load_skills_empty_dir(tmp_path, monkeypatch): @@ -278,3 +279,54 @@ def test_load_skills_finds_nested(tmp_path, monkeypatch): monkeypatch.setattr(_loader, "_BUILTIN_SKILLS", []) skills = load_skills() assert any(s.name == "myskill" for s in skills) + + +# ------------------------------------------------------------------ +# _skill_tool args normalization (#182) +# ------------------------------------------------------------------ + +from cheetahclaws.skill.tools import _skill_tool + + +def _fake_run(*args, **kwargs): + class _Ev: + text = "deployed to prod" + yield _Ev() + + +def _call_skill(params): + """Run _skill_tool against the deploy skill. A regression that raises will + fail the test outright — that is the point of the suite.""" + return _skill_tool(params, {}) + + +def test_skill_tool_coerces_dict_wrapped_single_value_args(skill_dir, monkeypatch): + """#182: a model that wraps args in an object (`{"args": {...}}`) must not + crash the Skill tool — take the single string value and proceed.""" + monkeypatch.setattr("cheetahclaws.agent.run", _fake_run) + out = _call_skill({"name": "deploy", "args": {"args": "--env prod"}}) + assert "CRASH" not in out + assert "deployed to prod" in out + + +def test_skill_tool_joins_list_args(skill_dir, monkeypatch): + monkeypatch.setattr("cheetahclaws.agent.run", _fake_run) + out = _call_skill({"name": "deploy", "args": ["--env", "prod"]}) + assert "CRASH" not in out + assert "deployed to prod" in out + + +def test_skill_tool_rejects_multivalue_dict_with_clear_error(skill_dir, monkeypatch): + """Multi-key dicts have no unambiguous string form — return an error the + model can act on instead of a raw TypeError.""" + monkeypatch.setattr("cheetahclaws.agent.run", _fake_run) + out = _call_skill({"name": "deploy", "args": {"env": "prod", "version": "1"}}) + assert "CRASH" not in out + assert "must be a string" in out + + +def test_skill_tool_rejects_non_string_args(skill_dir, monkeypatch): + monkeypatch.setattr("cheetahclaws.agent.run", _fake_run) + out = _call_skill({"name": "deploy", "args": 123}) + assert "CRASH" not in out + assert "must be a string" in out