diff --git a/src/kiro_crew/dashboard/handlers/__init__.py b/src/kiro_crew/dashboard/handlers/__init__.py index 0612b9dc3e1..4dd8b8e7b96 100644 --- a/src/kiro_crew/dashboard/handlers/__init__.py +++ b/src/kiro_crew/dashboard/handlers/__init__.py @@ -60,6 +60,7 @@ def sel(): _get_config_lock, _installed_agent_config, api_agent_config, + api_agent_create, api_agent_detail, api_agents_installed, api_capability_agents_install, diff --git a/src/kiro_crew/dashboard/handlers/agents.py b/src/kiro_crew/dashboard/handlers/agents.py index 32cd9a10551..23d89f14814 100644 --- a/src/kiro_crew/dashboard/handlers/agents.py +++ b/src/kiro_crew/dashboard/handlers/agents.py @@ -1040,6 +1040,323 @@ async def api_slash_commands(request: web.Request) -> web.Response: ) +#: A template name has to be safe as a bare filename stem (``.json`` inside +#: the agents dir) AND usable as a kiro-cli ``--agent`` argument. The character +#: class excludes the separators and ``..`` that a traversal would need, so no +#: separate traversal check is required to keep the write inside the dir — the +#: post-join containment assert below is belt and braces, not the primary guard. +_VALID_AGENT_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") + +_MAX_AGENT_NAME_LEN = 64 +_MAX_AGENT_DESCRIPTION_LEN = 500 +#: Generous by design: a system prompt is prose and some are genuinely long. +#: The cap exists so one request cannot write an unbounded file, not to express +#: an opinion about prompt length. +_MAX_AGENT_PROMPT_LEN = 100_000 + + +def _managed_agent_filenames() -> tuple[str, ...]: + """The agent spec filenames Kiro Crew itself writes and rewrites. + + Read through ``agent_files`` rather than duplicated as literals so adding a + managed spec there automatically protects it here — both from being shadowed + by a new template of the same name and from prompt/description edits that + the next install would silently revert. + """ + from kiro_crew.agent_files import OWNED_KIRO_AGENT_FILES + + return OWNED_KIRO_AGENT_FILES + + +def _reserved_agent_names() -> set[str]: + """Names a new template may not take. + + The managed specs' own stems (a template called ``kirocrew`` would either + collide with the file or shadow it by ``name``), plus ``default`` — the + built-in agent that has no config file and is special-cased by + :func:`api_agent_detail`. + """ + return {Path(f).stem for f in _managed_agent_filenames()} | {"default"} + + +#: The spec a blank new template starts from. +#: +#: ``tools`` is the surface the agent CAN reach; ``allowedTools`` is the subset +#: auto-approved without asking. The split is the whole security story of a +#: template, so a created one starts with a useful tool surface but auto-approves +#: only side-effect-free reads — an agent that can edit files or run a command +#: has to ask the first time. Kiro Crew's own privileged MCP servers +#: (``@kirocrew-core`` and friends: spawn, cron, computer use) are deliberately +#: absent; a user who wants that surface duplicates ``kirocrew`` instead of +#: starting blank, which is an explicit act rather than a default. +_BLANK_TEMPLATE_TOOLS = ( + "execute_bash", + "fs_read", + "fs_write", + "code", + "grep", + "glob", + "web_fetch", + "web_search", +) +_BLANK_TEMPLATE_ALLOWED_TOOLS = ("fs_read", "code", "grep", "glob") + + +def _blank_template_spec() -> dict[str, Any]: + """A minimal, schema-clean kiro agent spec for a from-scratch template. + + Only fields kiro-cli understands: it rejects a spec with unknown fields and + then resolves no agent at all, so the safest new spec is a small one. + """ + return { + "tools": list(_BLANK_TEMPLATE_TOOLS), + "allowedTools": list(_BLANK_TEMPLATE_ALLOWED_TOOLS), + "resources": ["file://.kiro/steering/**/*.md"], + } + + +def _validate_new_agent_name(raw: Any) -> tuple[str, str, str]: + """Return ``(name, code, message)`` for a requested template name. + + ``code`` is empty when the name is acceptable. The machine-readable code is + what the dashboard branches on; the prose is advisory (RFC 9457 3.1.3). + """ + if not isinstance(raw, str): + return "", "name_required", "name must be a string" + name = raw.strip() + if not name: + return "", "name_required", "name is required" + if len(name) > _MAX_AGENT_NAME_LEN: + return ( + "", + "name_too_long", + f"name must be at most {_MAX_AGENT_NAME_LEN} characters", + ) + if not _VALID_AGENT_NAME_RE.match(name): + return ( + "", + "invalid_agent_name", + "name may contain only letters, digits, dot, dash and underscore, " + "and must start with a letter or digit", + ) + if name in _reserved_agent_names(): + return "", "agent_name_reserved", f"'{name}' is reserved" + return name, "", "" + + +def _validate_spec_text(raw: Any, field: str, limit: int) -> tuple[str, str, str]: + """Return ``(value, code, message)`` for a free-text spec field.""" + if not isinstance(raw, str): + return "", f"{field}_invalid", f"{field} must be a string" + if len(raw) > limit: + return "", f"{field}_too_long", f"{field} must be at most {limit} characters" + return raw, "", "" + + +def _read_agent_specs(agents_dir: Path) -> list[tuple[Path, dict[str, Any]]]: + """Every parseable spec in *agents_dir*, as ``(path, data)`` pairs. + + Unreadable and non-object files are skipped rather than raising: the dir is + shared with other tools, and one bad file must not block creating a template. + """ + out: list[tuple[Path, dict[str, Any]]] = [] + try: + candidates = sorted(agents_dir.glob("*.json")) + except OSError: + return out + for path in candidates: + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError): + continue + if isinstance(data, dict): + out.append((path, data)) + return out + + +def _clone_source_spec( + specs: list[tuple[Path, dict[str, Any]]], source: str +) -> dict[str, Any] | None: + """The spec to copy for a ``from`` request, matched the way kiro-cli resolves. + + Both the declared ``name`` and the file stem are accepted because a + package-installed agent's file is ``-.json`` while its + identity is the ``name`` field. + """ + for path, data in specs: + if spec_str(data, "name") == source or path.stem == source: + return data + return None + + +async def api_agent_create(request: web.Request) -> web.Response: + """POST /api/agents/detail — create a new agent template. + + Writes ``.json`` into the agents dir, either from a conservative blank + baseline or as a copy of an existing template (``from``). + + ``tools``, ``allowedTools`` and ``toolsSettings`` are deliberately NOT + accepted from the request body. They are the privilege surface — the + auto-approve list and the bash deny patterns — so a create call cannot mint + a spec that auto-approves everything. A copy inherits them from a spec that + already exists on disk; a blank template gets the read-only baseline. Editing + them is a separate, deliberate capability this endpoint does not grant. + """ + from kiro_crew.agent import kiro_agents_dir_path # noqa: F811 + + try: + body = await request.json() + except (json.JSONDecodeError, ValueError): + return web.json_response({"error": "invalid JSON", "code": "invalid_json"}, status=400) + if not isinstance(body, dict): + return web.json_response( + {"error": "body must be a JSON object", "code": "invalid_body"}, status=400 + ) + + def _denied(code: str, message: str) -> web.Response: + """A 400 that is also recorded — a rejected create is worth an audit line.""" + _sel().log_api_access( + caller=request.get("user", "dashboard"), + operation="agent_template.create", + outcome="denied", + source="dashboard", + resources=str(body.get("name", ""))[:_MAX_AGENT_NAME_LEN], + error=code, + ) + return web.json_response({"error": message, "code": code}, status=400) + + name, code, message = _validate_new_agent_name(body.get("name")) + if code: + return _denied(code, message) + description, code, message = _validate_spec_text( + body.get("description", ""), "description", _MAX_AGENT_DESCRIPTION_LEN + ) + if code: + return _denied(code, message) + prompt, code, message = _validate_spec_text( + body.get("prompt", ""), "prompt", _MAX_AGENT_PROMPT_LEN + ) + if code: + return _denied(code, message) + raw_from = body.get("from", "") + if not isinstance(raw_from, str): + return _denied("invalid_from", "from must be a string") + clone_from = raw_from.strip() + + state: DashboardState = request.app["state"] + agents_dir = kiro_agents_dir_path() + target = agents_dir / f"{name}.json" + # The name regex already excludes every separator, so this cannot fail; it is + # here so a future loosening of the regex trips this instead of writing + # outside the agents dir. + if target.parent.resolve() != agents_dir.resolve(): + return _denied("invalid_agent_name", "invalid name") + + async with _get_config_lock(): + specs = await asyncio.to_thread(_read_agent_specs, agents_dir) + # Reject a name any EXISTING spec already answers to, not just a file-name + # collision: kiro-cli resolves by the ``name`` field, so a second spec + # declaring a package agent's name makes which one wins a coin flip. + for path, data in specs: + if spec_str(data, "name") == name or path.stem == name: + _sel().log_api_access( + caller=request.get("user", "dashboard"), + operation="agent_template.create", + outcome="denied", + source="dashboard", + resources=name, + error="agent_template_exists", + ) + return web.json_response( + {"error": f"'{name}' already exists", "code": "agent_template_exists"}, + status=409, + ) + + if clone_from: + source = _clone_source_spec(specs, clone_from) + if source is None: + return web.json_response( + { + "error": f"template '{clone_from}' not found", + "code": "source_template_not_found", + }, + status=404, + ) + spec: dict[str, Any] = json.loads(json.dumps(source)) + else: + spec = _blank_template_spec() + spec["name"] = name + if description: + spec["description"] = description + else: + spec.pop("description", None) + if prompt: + spec["prompt"] = prompt + elif not clone_from: + spec.pop("prompt", None) + # Kiro Crew bookkeeping that older specs may carry; kiro-cli rejects + # unknown fields and drops the whole agent, so a copy must not inherit it. + spec.pop("model_managed", None) + spec.pop("cc_model", None) + + payload = json.dumps(spec, indent=2) + "\n" + + def _write() -> str: + """Create the file exclusively; returns an error code or ''.""" + agents_dir.mkdir(parents=True, exist_ok=True) + try: + # O_EXCL, not a write after the collision scan above: the scan and + # the write are two steps, and the kernel is the only party that + # can make "create only if absent" atomic against a concurrent + # POST or another tool writing the same path. + fd = os.open(target, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError: + return "agent_template_exists" + except OSError as exc: + logger.warning("Failed to create agent template %s: %s", target, exc) + return "agent_template_write_failed" + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(payload) + except OSError as exc: + logger.warning("Failed to write agent template %s: %s", target, exc) + target.unlink(missing_ok=True) + return "agent_template_write_failed" + return "" + + write_code = await asyncio.to_thread(_write) + + if write_code: + _sel().log_api_access( + caller=request.get("user", "dashboard"), + operation="agent_template.create", + outcome="failure", + source="dashboard", + resources=name, + error=write_code, + ) + if write_code == "agent_template_exists": + return web.json_response( + {"error": f"'{name}' already exists", "code": write_code}, status=409 + ) + return web.json_response( + {"error": "could not write agent template", "code": write_code}, status=500 + ) + + # The list_agents() cache keys on a (count, newest-mtime-ns) signature, which + # a write inside the current mtime granularity would not move. + clear_list_agents_cache() + state.push_refresh("agents") + _sel().log_api_access( + caller=request.get("user", "dashboard"), + operation="agent_template.create", + outcome="success", + source="dashboard", + resources=f"{name} (from={clone_from})" if clone_from else name, + ) + return web.json_response({"ok": True, "name": name}) + + async def api_agent_detail(request: web.Request) -> web.Response: """GET/DELETE/PATCH /api/agents/detail/{name} — view, delete, or update agent config.""" name = request.match_info["name"] @@ -1089,6 +1406,38 @@ async def api_agent_detail(request: web.Request) -> web.Response: {"error": f"at most {MAX_AGENT_SKILLS} skills per agent"}, status=400, ) + # ``prompt`` and ``description`` are validated here, with + # ``skills``, so a bad value is rejected before any state + # mutation rather than half-applying a combined PATCH. + text_edits: dict[str, str] = {} + for field, limit in ( + ("prompt", _MAX_AGENT_PROMPT_LEN), + ("description", _MAX_AGENT_DESCRIPTION_LEN), + ): + if field not in patch_body: + continue + if f.name in _managed_agent_filenames(): + # Both fields are rewritten from the shipped defaults + # on every install and at boot self-heal, so accepting + # the edit would show a save that silently reverts. + return web.json_response( + { + "error": ( + f"{f.stem} is managed by Kiro Crew; its " + f"{field} cannot be edited" + ), + "code": "agent_template_managed", + }, + status=400, + ) + value, code, message = _validate_spec_text( + patch_body[field], field, limit + ) + if code: + return web.json_response( + {"error": message, "code": code}, status=400 + ) + text_edits[field] = value mapped: list[str] = [] loop = asyncio.get_running_loop() async with _get_config_lock(): @@ -1141,6 +1490,14 @@ async def api_agent_detail(request: web.Request) -> web.Response: # kiro-cli rejects unknown fields and drops the agent. data.pop("model_managed", None) data.pop("cc_model", None) + # An empty string means "no value", not the literal "": + # the key is dropped so the spec stays minimal and reads + # back through spec_str() as absent. + for field, value in text_edits.items(): + if value: + data[field] = value + else: + data.pop(field, None) f.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") # The list_agents() cache keys on a (count, newest-mtime-ns) # signature; two writes inside the same mtime granularity @@ -1148,7 +1505,13 @@ async def api_agent_detail(request: web.Request) -> web.Response: clear_list_agents_cache() state.push_refresh("agents") return web.json_response( - {"ok": True, "model": data.get("model", ""), "skills": mapped} + { + "ok": True, + "model": data.get("model", ""), + "skills": mapped, + "prompt": spec_str(data, "prompt"), + "description": spec_str(data, "description"), + } ) # ``skills`` / ``unmanaged_skills`` are computed, response-only # views of ``resources`` — never written back into the spec @@ -1170,6 +1533,12 @@ async def api_agent_detail(request: web.Request) -> web.Response: "model": spec_model(data), "skills": keys, "unmanaged_skills": unmanaged_uris, + # Whether Kiro Crew owns this spec and rewrites it on + # install. Served rather than re-derived client-side so + # the editor disables exactly the fields PATCH refuses, + # instead of keeping a second copy of the owned-file list + # in TypeScript that would drift from agent_files.py. + "managed": f.name in _managed_agent_filenames(), } ) except (json.JSONDecodeError, OSError): @@ -1178,7 +1547,9 @@ async def api_agent_detail(request: web.Request) -> web.Response: if name == "default": if request.method != "GET": return web.json_response({"error": "cannot modify built-in default agent"}, status=400) - return web.json_response({"name": "default", "model": ""}) + # No file to edit, so it is managed by definition — the editor must not + # offer fields whose PATCH the branch above already refuses. + return web.json_response({"name": "default", "model": "", "managed": True}) return web.json_response({"error": "not found"}, status=404) diff --git a/src/kiro_crew/dashboard/server.py b/src/kiro_crew/dashboard/server.py index 2d9c6c66dc7..eeba7e990f0 100644 --- a/src/kiro_crew/dashboard/server.py +++ b/src/kiro_crew/dashboard/server.py @@ -2335,6 +2335,7 @@ async def _wf_nudge_authorizer( app.router.add_get("/api/agents/detail/{name}", handlers.api_agent_detail) app.router.add_patch("/api/agents/detail/{name}", handlers.api_agent_detail) app.router.add_delete("/api/agents/detail/{name}", handlers.api_agent_detail) + app.router.add_post("/api/agents/detail", handlers.api_agent_create) # KiroCrew Agent CRUD app.router.add_get("/api/agents", handlers.api_kirocrew_agents) app.router.add_get( diff --git a/test/test_agent_template_create.py b/test/test_agent_template_create.py new file mode 100644 index 00000000000..8b252aaef1f --- /dev/null +++ b/test/test_agent_template_create.py @@ -0,0 +1,533 @@ +"""Tests for authoring agent templates from the dashboard. + +Two capabilities, both previously absent — the agents dir could only be written +by a package install, an app, or by hand: + +* CREATE — ``POST /api/agents/detail`` writes ``.json``, either from a + conservative blank baseline or as a copy of an existing template. +* EDIT — ``PATCH /api/agents/detail/{name}`` accepts ``prompt`` and + ``description``, so a created template can be corrected without hand-editing + JSON. + +The privilege surface (``tools`` / ``allowedTools`` / ``toolsSettings``) is +deliberately not writable through either verb; the tests below pin that. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from aiohttp import web + +from kiro_crew.agent_discovery import clear_list_agents_cache +from kiro_crew.dashboard.handlers.agents import ( + _BLANK_TEMPLATE_ALLOWED_TOOLS, + api_agent_create, + api_agent_detail, +) + + +@pytest.fixture(autouse=True) +def _no_agent_cache(): + clear_list_agents_cache() + yield + clear_list_agents_cache() + + +def _request(method: str, body: dict | list | None = None, name: str = "") -> MagicMock: + request = MagicMock(spec=web.Request) + request.method = method + request.match_info = {"name": name} + request.app = {"state": MagicMock()} + request.get = lambda key, default=None: default + + async def _json(): + if body is None: + raise json.JSONDecodeError("no body", "", 0) + return body + + request.json = _json + return request + + +async def _create(agents_dir: Path, body: dict | list | None) -> web.Response: + with patch("kiro_crew.agent.KIRO_AGENTS_DIR", agents_dir): + return await api_agent_create(_request("POST", body)) + + +async def _patch(agents_dir: Path, name: str, body: dict) -> web.Response: + with patch("kiro_crew.agent.KIRO_AGENTS_DIR", agents_dir): + return await api_agent_detail(_request("PATCH", body, name=name)) + + +async def _get(agents_dir: Path, name: str) -> web.Response: + with patch("kiro_crew.agent.KIRO_AGENTS_DIR", agents_dir): + return await api_agent_detail(_request("GET", None, name=name)) + + +def _spec(resp: web.Response) -> dict: + return json.loads(resp.body.decode("utf-8")) + + +@pytest.fixture +def agents_dir(tmp_path: Path) -> Path: + d = tmp_path / "agents" + d.mkdir() + return d + + +# ── CREATE: the blank baseline ── + + +class TestCreateBlank: + @pytest.mark.asyncio + async def test_writes_a_named_spec(self, agents_dir): + resp = await _create(agents_dir, {"name": "researcher"}) + + assert resp.status == 200 + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert written["name"] == "researcher" + + @pytest.mark.asyncio + async def test_auto_approves_only_read_only_tools(self, agents_dir): + """The blank baseline's whole security story: a useful tool surface, but + nothing with a side effect is pre-approved.""" + await _create(agents_dir, {"name": "researcher"}) + + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert set(written["allowedTools"]) == set(_BLANK_TEMPLATE_ALLOWED_TOOLS) + assert "execute_bash" in written["tools"] + assert "execute_bash" not in written["allowedTools"] + assert "fs_write" not in written["allowedTools"] + + @pytest.mark.asyncio + async def test_omits_kirocrew_mcp_servers(self, agents_dir): + """Kiro Crew's own MCP surface (spawn / cron / computer use) is opt-in via + a copy of ``kirocrew``, never a blank template's default.""" + await _create(agents_dir, {"name": "researcher"}) + + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert not [t for t in written["tools"] if t.startswith("@kirocrew")] + + @pytest.mark.asyncio + async def test_description_and_prompt_are_stored(self, agents_dir): + await _create( + agents_dir, + {"name": "researcher", "description": "Digs through papers", "prompt": "Be rigorous."}, + ) + + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert written["description"] == "Digs through papers" + assert written["prompt"] == "Be rigorous." + + @pytest.mark.asyncio + async def test_absent_optional_fields_are_omitted_not_empty(self, agents_dir): + """kiro-cli reads the spec; an empty-string prompt is not the same as no + prompt, and a minimal spec is the one least likely to be rejected.""" + await _create(agents_dir, {"name": "researcher"}) + + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert "prompt" not in written + assert "description" not in written + + @pytest.mark.asyncio + async def test_tools_in_the_body_are_ignored(self, agents_dir): + """The privilege surface is not settable through create — otherwise one + call could mint a template that auto-approves everything.""" + await _create( + agents_dir, + { + "name": "researcher", + "tools": ["execute_bash"], + "allowedTools": ["execute_bash", "fs_write"], + "toolsSettings": {"execute_bash": {"deniedCommands": []}}, + }, + ) + + written = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert "execute_bash" not in written["allowedTools"] + assert "toolsSettings" not in written + + +# ── CREATE: name validation ── + + +class TestCreateNameValidation: + @pytest.mark.asyncio + async def test_missing_name_is_rejected(self, agents_dir): + resp = await _create(agents_dir, {}) + assert resp.status == 400 + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "name", + [ + "../escape", + "sub/dir", + "back\\slash", + "..", + ".hidden", + "has space", + "-leading-dash", + "sym*bol", + ], + ) + async def test_unsafe_names_are_rejected(self, agents_dir, name): + resp = await _create(agents_dir, {"name": name}) + + assert resp.status == 400 + assert list(agents_dir.iterdir()) == [] + + @pytest.mark.asyncio + async def test_traversal_writes_nothing_outside_the_agents_dir(self, agents_dir, tmp_path): + await _create(agents_dir, {"name": "../../pwned"}) + + assert not (tmp_path / "pwned.json").exists() + assert not (tmp_path.parent / "pwned.json").exists() + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "name", ["kirocrew", "kirocrew-lite", "kirocrew-knowledge", "default"] + ) + async def test_managed_and_builtin_names_are_reserved(self, agents_dir, name): + resp = await _create(agents_dir, {"name": name}) + + assert resp.status == 400 + assert not (agents_dir / f"{name}.json").exists() + + @pytest.mark.asyncio + async def test_overlong_name_is_rejected(self, agents_dir): + resp = await _create(agents_dir, {"name": "a" * 65}) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_non_string_name_is_rejected(self, agents_dir): + resp = await _create(agents_dir, {"name": {"id": "x"}}) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_non_object_body_is_rejected(self, agents_dir): + resp = await _create(agents_dir, ["name"]) + assert resp.status == 400 + + @pytest.mark.asyncio + async def test_invalid_json_is_rejected(self, agents_dir): + resp = await _create(agents_dir, None) + assert resp.status == 400 + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "body,code", + [ + ({}, "name_required"), + ({"name": "../escape"}, "invalid_agent_name"), + ({"name": "kirocrew"}, "agent_name_reserved"), + ({"name": "a" * 65}, "name_too_long"), + ({"name": "ok", "prompt": {"bad": 1}}, "prompt_invalid"), + ({"name": "ok", "description": "d" * 501}, "description_too_long"), + ({"name": "ok", "from": 7}, "invalid_from"), + ], + ) + async def test_every_rejection_carries_a_machine_readable_code( + self, agents_dir, body, code + ): + """The dashboard branches on ``code``; the prose is advisory and localized + away, so an un-coded rejection is untranslatable by construction.""" + resp = await _create(agents_dir, body) + + assert resp.status == 400 + assert _spec(resp)["code"] == code + + +# ── CREATE: collisions ── + + +class TestCreateCollisions: + @pytest.mark.asyncio + async def test_existing_filename_conflicts(self, agents_dir): + (agents_dir / "researcher.json").write_text('{"name": "researcher"}', encoding="utf-8") + + resp = await _create(agents_dir, {"name": "researcher"}) + assert resp.status == 409 + assert _spec(resp)["code"] == "agent_template_exists" + + @pytest.mark.asyncio + async def test_name_claimed_by_a_package_spec_conflicts(self, agents_dir): + """kiro-cli resolves an agent by its ``name`` field, so a second spec + declaring a package agent's name makes which one wins a coin flip — the + filename being free is not enough.""" + (agents_dir / "somepkg-reviewer.json").write_text( + '{"name": "reviewer"}', encoding="utf-8" + ) + + resp = await _create(agents_dir, {"name": "reviewer"}) + + assert resp.status == 409 + assert not (agents_dir / "reviewer.json").exists() + + @pytest.mark.asyncio + async def test_an_unparseable_neighbour_does_not_block_creation(self, agents_dir): + (agents_dir / "broken.json").write_text("{not json", encoding="utf-8") + + resp = await _create(agents_dir, {"name": "researcher"}) + assert resp.status == 200 + + @pytest.mark.asyncio + async def test_existing_file_is_never_overwritten(self, agents_dir): + (agents_dir / "researcher.json").write_text( + '{"name": "researcher", "prompt": "original"}', encoding="utf-8" + ) + + await _create(agents_dir, {"name": "researcher", "prompt": "replacement"}) + + kept = json.loads((agents_dir / "researcher.json").read_text(encoding="utf-8")) + assert kept["prompt"] == "original" + + @pytest.mark.asyncio + async def test_a_file_appearing_after_the_scan_is_not_clobbered(self, agents_dir): + """The TOCTOU window the exclusive create closes. + + Reading the dir and writing the file are two steps. A concurrent POST for + the same name — or another tool writing the same path — lands in between, + and only the kernel can make "create only if absent" atomic. Stubbing the + scan to report an empty dir is exactly what the loser of that race sees, + so this reaches the write with the file already present. + """ + victim = agents_dir / "researcher.json" + victim.write_text('{"name": "researcher", "prompt": "original"}', encoding="utf-8") + + with patch( + "kiro_crew.dashboard.handlers.agents._read_agent_specs", return_value=[] + ): + resp = await _create(agents_dir, {"name": "researcher", "prompt": "replacement"}) + + assert resp.status == 409 + assert json.loads(victim.read_text(encoding="utf-8"))["prompt"] == "original" + + +# ── CREATE: copying an existing template ── + + +class TestCreateFromCopy: + @pytest.fixture + def source(self, agents_dir) -> Path: + path = agents_dir / "base.json" + path.write_text( + json.dumps( + { + "name": "base", + "description": "the original", + "prompt": "original prompt", + "model": "some-model", + "tools": ["execute_bash", "fs_read", "@kirocrew-core"], + "allowedTools": ["fs_read", "@kirocrew-core"], + "mcpServers": {"kirocrew-core": {"command": "mcp", "args": []}}, + "resources": ["skill://~/.kiro/skills/babysit/SKILL.md"], + "toolsSettings": {"execute_bash": {"deniedCommands": ["rm -rf /*"]}}, + } + ), + encoding="utf-8", + ) + return path + + @pytest.mark.asyncio + async def test_copy_inherits_the_privilege_surface(self, agents_dir, source): + """A copy is how a user gets a privileged tool surface: inherited from a + spec that already exists on disk, not assembled from a request body.""" + await _create(agents_dir, {"name": "clone", "from": "base"}) + + written = json.loads((agents_dir / "clone.json").read_text(encoding="utf-8")) + assert written["tools"] == ["execute_bash", "fs_read", "@kirocrew-core"] + assert written["allowedTools"] == ["fs_read", "@kirocrew-core"] + assert written["toolsSettings"]["execute_bash"]["deniedCommands"] == ["rm -rf /*"] + assert written["mcpServers"] == {"kirocrew-core": {"command": "mcp", "args": []}} + assert written["resources"] == ["skill://~/.kiro/skills/babysit/SKILL.md"] + + @pytest.mark.asyncio + async def test_copy_takes_the_new_name(self, agents_dir, source): + await _create(agents_dir, {"name": "clone", "from": "base"}) + + written = json.loads((agents_dir / "clone.json").read_text(encoding="utf-8")) + assert written["name"] == "clone" + + @pytest.mark.asyncio + async def test_supplied_prompt_overrides_the_copied_one(self, agents_dir, source): + await _create(agents_dir, {"name": "clone", "from": "base", "prompt": "mine"}) + + written = json.loads((agents_dir / "clone.json").read_text(encoding="utf-8")) + assert written["prompt"] == "mine" + + @pytest.mark.asyncio + async def test_source_is_left_untouched(self, agents_dir, source): + await _create(agents_dir, {"name": "clone", "from": "base", "prompt": "mine"}) + + original = json.loads(source.read_text(encoding="utf-8")) + assert original["name"] == "base" + assert original["prompt"] == "original prompt" + + @pytest.mark.asyncio + async def test_copy_resolves_a_package_spec_by_its_name_field(self, agents_dir): + (agents_dir / "somepkg-reviewer.json").write_text( + json.dumps({"name": "reviewer", "tools": ["fs_read"]}), encoding="utf-8" + ) + + resp = await _create(agents_dir, {"name": "myreviewer", "from": "reviewer"}) + + assert resp.status == 200 + written = json.loads((agents_dir / "myreviewer.json").read_text(encoding="utf-8")) + assert written["tools"] == ["fs_read"] + + @pytest.mark.asyncio + async def test_unknown_source_is_a_404(self, agents_dir): + resp = await _create(agents_dir, {"name": "clone", "from": "nope"}) + + assert resp.status == 404 + assert _spec(resp)["code"] == "source_template_not_found" + assert not (agents_dir / "clone.json").exists() + + @pytest.mark.asyncio + async def test_bookkeeping_keys_are_not_inherited(self, agents_dir): + """kiro-cli rejects unknown fields and then resolves no agent at all, so a + copy of an older spec must not carry Kiro Crew's sidecar keys.""" + (agents_dir / "legacy.json").write_text( + json.dumps({"name": "legacy", "model_managed": True, "cc_model": "x"}), + encoding="utf-8", + ) + + await _create(agents_dir, {"name": "clone", "from": "legacy"}) + + written = json.loads((agents_dir / "clone.json").read_text(encoding="utf-8")) + assert "model_managed" not in written + assert "cc_model" not in written + + +# ── EDIT: prompt and description ── + + +class TestPatchText: + @pytest.fixture + def template(self, agents_dir) -> Path: + path = agents_dir / "researcher.json" + path.write_text( + json.dumps({"name": "researcher", "prompt": "old", "description": "old desc"}), + encoding="utf-8", + ) + return path + + @pytest.mark.asyncio + async def test_prompt_is_written(self, agents_dir, template): + resp = await _patch(agents_dir, "researcher", {"prompt": "new prompt"}) + + assert resp.status == 200 + assert json.loads(template.read_text(encoding="utf-8"))["prompt"] == "new prompt" + assert _spec(resp)["prompt"] == "new prompt" + + @pytest.mark.asyncio + async def test_description_is_written(self, agents_dir, template): + resp = await _patch(agents_dir, "researcher", {"description": "new desc"}) + + assert resp.status == 200 + assert json.loads(template.read_text(encoding="utf-8"))["description"] == "new desc" + + @pytest.mark.asyncio + async def test_empty_value_drops_the_key(self, agents_dir, template): + await _patch(agents_dir, "researcher", {"prompt": ""}) + + assert "prompt" not in json.loads(template.read_text(encoding="utf-8")) + + @pytest.mark.asyncio + async def test_a_file_prompt_can_be_replaced_with_inline_text(self, agents_dir): + """A copied template inherits ``prompt: file://…`` from its source; the + editor has to be able to detach it.""" + path = agents_dir / "clone.json" + path.write_text( + json.dumps({"name": "clone", "prompt": "file:///somewhere/prompt.md"}), + encoding="utf-8", + ) + + await _patch(agents_dir, "clone", {"prompt": "inline now"}) + + assert json.loads(path.read_text(encoding="utf-8"))["prompt"] == "inline now" + + @pytest.mark.asyncio + async def test_non_string_prompt_is_rejected(self, agents_dir, template): + resp = await _patch(agents_dir, "researcher", {"prompt": {"file": "x"}}) + + assert resp.status == 400 + assert json.loads(template.read_text(encoding="utf-8"))["prompt"] == "old" + + @pytest.mark.asyncio + async def test_overlong_prompt_is_rejected(self, agents_dir, template): + resp = await _patch(agents_dir, "researcher", {"prompt": "x" * 100_001}) + + assert resp.status == 400 + assert json.loads(template.read_text(encoding="utf-8"))["prompt"] == "old" + + @pytest.mark.asyncio + async def test_a_rejected_combined_patch_applies_nothing(self, agents_dir, template): + """Validation runs before any mutation, so a bad prompt cannot leave a + half-applied model change behind.""" + resp = await _patch( + agents_dir, "researcher", {"model": "new-model", "prompt": {"bad": 1}} + ) + + assert resp.status == 400 + written = json.loads(template.read_text(encoding="utf-8")) + assert "model" not in written + assert written["prompt"] == "old" + + @pytest.mark.asyncio + @pytest.mark.parametrize("field", ["prompt", "description"]) + async def test_managed_specs_refuse_the_edit(self, agents_dir, field): + """Kiro Crew rewrites these fields on every install, so accepting the edit + would show the user a save that silently reverts.""" + path = agents_dir / "kirocrew.json" + path.write_text( + json.dumps({"name": "kirocrew", "prompt": "file://p.md", "description": "d"}), + encoding="utf-8", + ) + + resp = await _patch(agents_dir, "kirocrew", {field: "hijacked"}) + + assert resp.status == 400 + assert _spec(resp)["code"] == "agent_template_managed" + assert json.loads(path.read_text(encoding="utf-8"))[field] != "hijacked" + + @pytest.mark.asyncio + async def test_managed_specs_still_accept_a_model_patch(self, agents_dir): + """Only the rewritten fields are refused; the model pin is a real sidecar + -backed setting and must keep working.""" + path = agents_dir / "kirocrew.json" + path.write_text(json.dumps({"name": "kirocrew"}), encoding="utf-8") + + resp = await _patch(agents_dir, "kirocrew", {"model": "some-model"}) + + assert resp.status == 200 + assert json.loads(path.read_text(encoding="utf-8"))["model"] == "some-model" + + +# ── The managed flag the editor gates on ── + + +class TestManagedFlag: + @pytest.mark.asyncio + async def test_a_created_template_is_not_managed(self, agents_dir): + await _create(agents_dir, {"name": "researcher"}) + + resp = await _get(agents_dir, "researcher") + assert _spec(resp)["managed"] is False + + @pytest.mark.asyncio + async def test_a_kirocrew_owned_spec_is_managed(self, agents_dir): + (agents_dir / "kirocrew.json").write_text('{"name": "kirocrew"}', encoding="utf-8") + + resp = await _get(agents_dir, "kirocrew") + assert _spec(resp)["managed"] is True + + @pytest.mark.asyncio + async def test_the_builtin_default_is_managed(self, agents_dir): + resp = await _get(agents_dir, "default") + assert _spec(resp)["managed"] is True diff --git a/website/src/api/client.ts b/website/src/api/client.ts index 4c5ba5156d7..aa7336fd33a 100644 --- a/website/src/api/client.ts +++ b/website/src/api/client.ts @@ -1219,6 +1219,7 @@ export const api = { // Agents agentsInstalled: () => fetch('/api/agents/installed').then(j), agentDetail: (name: string) => fetch('/api/agents/detail/' + encodeURIComponent(name)).then(j), + agentCreate: (body: object) => fetch('/api/agents/detail', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body) }).then(j), agentPatch: (name: string, body: object) => fetch('/api/agents/detail/' + encodeURIComponent(name), { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body) }).then(j), agentDelete: (name: string) => fetch('/api/agents/detail/' + encodeURIComponent(name), { method: 'DELETE' }).then(j), // KiroCrew agents diff --git a/website/src/components/AgentTemplateCreateDialog.tsx b/website/src/components/AgentTemplateCreateDialog.tsx new file mode 100644 index 00000000000..0c76c866852 --- /dev/null +++ b/website/src/components/AgentTemplateCreateDialog.tsx @@ -0,0 +1,142 @@ +import { useEffect, useState } from 'react' +import { api } from '../api/client' +import { Btn, SendBtn, Input } from './ui' +import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogBody, DialogFooter } from './ui/dialog' +import SimpleSelect from './SimpleSelect' +import InfoTip from './InfoTip' + +import { i18nT } from '../i18n/t' +/** + * The one authoring surface for a kiro agent template (`~/.kiro/agents/.json`). + * + * Both entry points share it: "New template" opens it with no source, and + * "Duplicate" opens it with `initialFrom` set. They differ only in that one + * field, and a second component for the copy case would have to keep the same + * name validation, the same conflict messages, and the same tool-surface + * explainer in sync. + * + * The dialog deliberately does NOT offer `tools` / `allowedTools` / + * `deniedCommands`. Those are the privilege surface — the auto-approve list and + * the bash deny patterns — and the create endpoint refuses them from a request + * body for that reason. A blank template gets a read-only auto-approve baseline; + * copying a template inherits whatever that one was already trusted with. + */ +export default function AgentTemplateCreateDialog({ + open, + templates, + initialFrom, + onClose, + onCreated, +}: { + open: boolean + /** Names of installed templates offered as a starting point. */ + templates: string[] + /** Preselected source, for the Duplicate entry point. */ + initialFrom?: string + onClose: () => void + onCreated: (name: string) => void +}) { + const [name, setName] = useState('') + const [description, setDescription] = useState('') + const [prompt, setPrompt] = useState('') + const [from, setFrom] = useState(initialFrom || '') + const [error, setError] = useState('') + const [submitting, setSubmitting] = useState(false) + + // Reset on each open so a dismissed attempt does not resurface half-typed, + // and so Duplicate always reflects the row the user actually clicked. + useEffect(() => { + if (!open) return + setName(initialFrom ? `${initialFrom}-copy` : '') + setDescription('') + setPrompt('') + setFrom(initialFrom || '') + setError('') + setSubmitting(false) + }, [open, initialFrom]) + + const submit = async () => { + setError('') + const n = name.trim() + if (!n) { setError(i18nT('components.agentTemplateCreateDialog.name_is_required')); return } + setSubmitting(true) + try { + const body: Record = { name: n } + if (description.trim()) body.description = description.trim() + if (prompt.trim()) body.prompt = prompt + if (from) body.from = from + const r: { ok?: boolean; name?: string; error?: string } = await api.agentCreate(body) + // The server owns the rules the form cannot check (reserved names, a name + // already claimed by a package spec), so its message is shown verbatim + // rather than replaced with a guess. + if (r.error) { setError(r.error); setSubmitting(false); return } + onCreated(r.name || n) + } catch (e) { + setError(e instanceof Error ? e.message : i18nT('components.agentTemplateCreateDialog.failed_to_create_template')) + } finally { + setSubmitting(false) + } + } + + return ( + { if (!next) onClose() }}> + + + {initialFrom ? i18nT('components.agentTemplateCreateDialog.duplicate_agent_template') : i18nT('components.agentTemplateCreateDialog.new_agent_template')} + + +
+
+
+ {/* Native input associated via htmlFor+id; label-has-for's nesting requirement is a false positive. */} + {/* eslint-disable-next-line jsx-a11y/label-has-for */} + + +
+ setName(e.target.value)} autoFocus /> +
+
+
+ {i18nT('components.agentTemplateCreateDialog.start_from')} + +
+ +
+
+ {/* eslint-disable-next-line jsx-a11y/label-has-for */} + + setDescription(e.target.value)} /> +
+
+
+ {/* eslint-disable-next-line jsx-a11y/label-has-for */} + + +
+