diff --git a/src/algolia_agent/cli.py b/src/algolia_agent/cli.py index e898773..61f6460 100644 --- a/src/algolia_agent/cli.py +++ b/src/algolia_agent/cli.py @@ -290,6 +290,29 @@ def cmd_create(client: AlgoliaAgentClient, args: argparse.Namespace): # only deviations are reported. Fields absent from this map are reported conservatively. _TOOL_FIELD_DEFAULTS = {"mode": "static", "allowUnlistedIndices": False} +# Fields that identify a tool or are regenerated by the API, so never a real change: +# name/type identify it, indices and description are diffed separately (the API composes +# the tool description from the index descriptions). +_TOOL_IDENTITY = {"name", "type", "indices", "description"} + + +def _omitted_field_note(field: str, curr_val) -> str: + """Describe what happens to a field the payload leaves out. + + The API replaces the tool object, so an omitted field takes its default. Name that + default when it is known rather than just saying "default", which reads as though + the current value were the default. + """ + default = _TOOL_FIELD_DEFAULTS.get(field, _MISSING) + if default is _MISSING: + return f"{_fmt(curr_val)} would be dropped from the payload" + return f"{_fmt(curr_val)} would revert to its default of {_fmt(default)}" + + +def _tool_key(tool: dict) -> str: + """Identity of a tool within the tools array.""" + return tool.get("type") or tool.get("name") or "" + def _fmt(value) -> str: """Render a diff value: JSON for objects/arrays, repr for scalars.""" @@ -397,10 +420,6 @@ def _diff(current: dict, new_payload: dict) -> list[str]: # that silently drops an existing tool is invisible. Only fields present in the new # payload are compared (same noise-avoidance as searchControls); nested index data # and descriptions are diffed above, so they're excluded here. - def _tool_key(t): - return t.get("type") or t.get("name") or "" - - _tool_identity = {"name", "type", "indices", "description"} curr_tools = {_tool_key(t): t for t in current.get("tools", [])} new_tools = {_tool_key(t): t for t in new_payload.get("tools", [])} @@ -411,7 +430,7 @@ def _tool_key(t): elif key not in new_tools: tool_lines.append(f" - {key}") else: - new_fields = {k: v for k, v in new_tools[key].items() if k not in _tool_identity} + new_fields = {k: v for k, v in new_tools[key].items() if k not in _TOOL_IDENTITY} for k in sorted(new_fields): changed, curr_shown = _field_changed(curr_tools[key].get(k), new_fields[k]) if changed: @@ -422,13 +441,15 @@ def _tool_key(t): # reverts to its API default — verified: mode "dynamic" became "static" and # allowUnlistedIndices true became false. Report those, or the dry-run stays # silent while the update turns settings off. "description" is excluded via - # _tool_identity because the API regenerates it from the index descriptions. - for k in sorted(set(curr_tools[key]) - set(new_fields) - _tool_identity): + # _TOOL_IDENTITY because the API regenerates it from the index descriptions. + for k in sorted(set(curr_tools[key]) - set(new_fields) - _TOOL_IDENTITY): curr_val = curr_tools[key][k] if curr_val is None or curr_val == _TOOL_FIELD_DEFAULTS.get(k, _MISSING): continue # already at its default; omitting it changes nothing + default = _TOOL_FIELD_DEFAULTS.get(k, _MISSING) + becomes = "dropped" if default is _MISSING else _fmt(default) tool_lines.append( - f" ~ {key}.{k}: {_fmt(curr_val)} → default (not sent)" + f" ~ {key}.{k}: {_fmt(curr_val)} → {becomes} (not sent)" ) if tool_lines: lines.append(" tools:") @@ -454,6 +475,55 @@ def _tool_key(t): return lines +def _removals(current: dict, new_payload: dict) -> list[str]: + """Describe losses the config model has no way to ask for. + + PATCH replaces objects rather than merging them, so anything absent from the + payload is destroyed. agent-config.json cannot express a second tool, a tool-level + field like mode, or per-index searchControls, so losing those is never something the + user asked for — it is the config model being narrower than the API. + + Index membership is deliberately excluded: the config names its indices, so + dropping one is an explicit choice rather than an accident. + """ + out = [] + + curr_tools = {_tool_key(t): t for t in current.get("tools", [])} + new_tools = {_tool_key(t): t for t in new_payload.get("tools", [])} + + for key in sorted(set(curr_tools) - set(new_tools)): + out.append(f" - the {key!r} tool would be deleted") + + for key in sorted(set(curr_tools) & set(new_tools)): + dropped = set(curr_tools[key]) - set(new_tools[key]) - _TOOL_IDENTITY + for field in sorted(dropped): + val = curr_tools[key][field] + if val is None or val == _TOOL_FIELD_DEFAULTS.get(field, _MISSING): + continue # already at its default; omitting it changes nothing + out.append(f" - {key}.{field}: {_omitted_field_note(field, val)}") + + curr_sc = { + i["index"]: i.get("searchControls") + for t in current.get("tools", []) + for i in t.get("indices", []) + } + new_sc = { + i["index"]: i.get("searchControls") + for t in new_payload.get("tools", []) + for i in t.get("indices", []) + } + for idx in sorted(curr_sc): + if idx in new_sc and curr_sc[idx] and not new_sc[idx]: + out.append(f" - searchControls on {idx!r} would be wiped") + + new_cfg = new_payload.get("config") + if new_cfg is not None: + for k in sorted(set(current.get("config") or {}) - set(new_cfg)): + out.append(f" - config key {k!r} would be removed") + + return out + + def cmd_update(client: AlgoliaAgentClient, args: argparse.Namespace): current = client.get_agent(args.agent_id) @@ -544,6 +614,21 @@ def cmd_update(client: AlgoliaAgentClient, args: argparse.Namespace): print("\n No changes detected.") return + # The API replaces objects rather than merging, so a config narrower than the + # agent silently deletes the difference. Refuse rather than destroy. + removals = _removals(current, new_payload) + if removals and not getattr(args, "force", False): + source = f"{config_path}" if config_path else "the fields you supplied" + raise SystemExit( + "ERROR: this update would remove configuration the agent currently has:\n" + + "\n".join(removals) + + "\n\nThe Agent Studio API replaces these fields instead of merging them, so\n" + f"anything missing from the payload is lost. {source} cannot express them,\n" + "which is why they are absent.\n\n" + "Either add the values to your config, or pass --force to accept the removals.\n" + "Run the same command with --dry-run to see the full diff first." + ) + agent = client.update_agent(args.agent_id, new_payload) if args.json: @@ -889,6 +974,8 @@ def build_parser() -> argparse.ArgumentParser: help="Template variable substitution (repeatable)") update_p.add_argument("--publish", action="store_true", help="Publish the agent after updating") + update_p.add_argument("--force", action="store_true", + help="Proceed even if the update would remove existing configuration") update_p.add_argument("--dry-run", action="store_true", help="Show what would change without making API calls") update_p.add_argument("--json", action="store_true", help="Output JSON") diff --git a/tests/test_cli.py b/tests/test_cli.py index c359395..cfb841e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1311,14 +1311,30 @@ def test_diff_reports_tool_fields_that_revert_to_default(): }]) changes = _diff(current, new_payload) assert " tools:" in changes - reverts = [line for line in changes if "default (not sent)" in line] + reverts = [line for line in changes if "(not sent)" in line] assert len(reverts) == 2, reverts - assert any("mode" in line and "dynamic" in line for line in reverts) - assert any("allowUnlistedIndices" in line for line in reverts) + # The actual default is named, not just the word "default". + assert any("mode" in line and "'dynamic' → 'static'" in line for line in reverts) + assert any("allowUnlistedIndices" in line and "True → False" in line for line in reverts) # The API regenerates tool description, so it must not be reported. assert not any("description" in line for line in reverts) +def test_diff_says_dropped_when_the_default_is_unknown(): + """Only mode and allowUnlistedIndices have known defaults; be honest about the rest.""" + current = _agent_with_tools([{ + "type": "algolia_search_index", "name": "algolia_search_index", + "someFutureField": 42, + "indices": [{"index": "products", "description": "d"}], + }]) + new_payload = _agent_with_tools([{ + "type": "algolia_search_index", "name": "algolia_search_index", + "indices": [{"index": "products", "description": "d"}], + }]) + changes = _diff(current, new_payload) + assert any("someFutureField: 42 → dropped (not sent)" in line for line in changes) + + def test_diff_ignores_null_tool_fields_not_sent(): """A field already null loses nothing by being omitted.""" current = _agent_with_tools([{ @@ -1331,3 +1347,180 @@ def test_diff_ignores_null_tool_fields_not_sent(): "indices": [{"index": "products", "description": "d"}], }]) assert not _diff(current, new_payload) + + +# ── destructive-update guard (#14 phase 1) ──────────────────────────────────── + +def _rich_current_agent(): + """An agent carrying everything agent-config.json cannot express.""" + return { + "id": "agent-uuid", "name": "Rich Agent", "model": "gemini-2.5-flash", + "instructions": "Old instructions.", "status": "published", + "providerId": "provider-uuid", + "tools": [ + {"name": "algolia_search_index", "type": "algolia_search_index", + "mode": "dynamic", "allowUnlistedIndices": True, + "indices": [{"index": "products", "description": "Product catalog.", + "searchControls": {"hitsPerPage": {"exposed": True, "default": 7}}}]}, + {"name": "algolia_display_results", "type": "algolia_display_results", + "isTerminal": False, "maxGroups": 3}, + ], + "config": {"suggestions": {"enabled": True}, "memory": {"enabled": True}, + "max_iterations": 15}, + } + + +def _narrow_payload(): + """What build_tool() + cmd_update produce from a config naming only the index.""" + return { + "name": "Rich Agent", "providerId": "provider-uuid", "model": "gemini-2.5-flash", + "instructions": "Old instructions.", "status": "published", + "tools": [{"name": "algolia_search_index", "type": "algolia_search_index", + "indices": [{"index": "products", "description": "Product catalog."}]}], + "config": {"suggestions": {"enabled": True}}, + } + + +def test_removals_lists_every_loss_the_config_cannot_express(): + from algolia_agent.cli import _removals + + out = _removals(_rich_current_agent(), _narrow_payload()) + joined = "\n".join(out) + assert "'algolia_display_results' tool would be deleted" in joined + assert "algolia_search_index.mode: 'dynamic' would revert to its default of 'static'" in joined + assert "algolia_search_index.allowUnlistedIndices" in joined + assert "searchControls on 'products' would be wiped" in joined + assert "config key 'memory'" in joined + assert "config key 'max_iterations'" in joined + # 'suggestions' is being sent, so it is not a loss. + assert "'suggestions'" not in joined + + +def test_removals_empty_for_a_faithful_payload(): + from algolia_agent.cli import _removals + + current = _rich_current_agent() + assert _removals(current, dict(current)) == [] + + +def test_removals_ignores_fields_already_at_their_default(): + from algolia_agent.cli import _removals + + current = _rich_current_agent() + current["tools"][0]["mode"] = "static" + current["tools"][0]["allowUnlistedIndices"] = False + out = "\n".join(_removals(current, _narrow_payload())) + assert "mode" not in out + assert "allowUnlistedIndices" not in out + + +def test_removals_does_not_flag_index_membership(): + """Dropping an index is expressible in config, so it is a choice, not an accident.""" + from algolia_agent.cli import _removals + + current = _rich_current_agent() + current["tools"][0]["indices"].append({"index": "products_asc", "description": "Asc."}) + out = "\n".join(_removals(current, _narrow_payload())) + assert "products_asc" not in out + + +def _guard_args(tmp_path, extra=None): + prompt = tmp_path / "PROMPT.md" + prompt.write_text("Old instructions.") + config = tmp_path / "config.json" + config.write_text(json.dumps({ + "name": "Rich Agent", "model": "gemini-2.5-flash", + "instructions": str(prompt), "index": "products", + "index_description": "Product catalog.", + "config": {"suggestions": {"enabled": True}}, + })) + return build_parser().parse_args( + ["update", "agent-uuid", "--config", str(config)] + (extra or []) + ) + + +def test_update_refuses_a_destructive_payload(tmp_path, capsys): + from algolia_agent.cli import cmd_update + + mock_client = MagicMock() + mock_client.get_agent.return_value = _rich_current_agent() + with pytest.raises(SystemExit) as exc: + cmd_update(mock_client, _guard_args(tmp_path)) + msg = str(exc.value) + assert "would remove configuration" in msg + assert "algolia_display_results" in msg + assert "--force" in msg + mock_client.update_agent.assert_not_called() + + +def test_update_proceeds_with_force(tmp_path): + from algolia_agent.cli import cmd_update + + mock_client = MagicMock() + mock_client.get_agent.return_value = _rich_current_agent() + mock_client.update_agent.return_value = {"id": "agent-uuid", "name": "Rich Agent", "status": "published"} + cmd_update(mock_client, _guard_args(tmp_path, ["--force"])) + mock_client.update_agent.assert_called_once() + + +def test_dry_run_is_not_blocked_by_the_guard(tmp_path, capsys): + """--dry-run writes nothing, so it must still report rather than refuse.""" + from algolia_agent.cli import cmd_update + + mock_client = MagicMock() + mock_client.get_agent.return_value = _rich_current_agent() + cmd_update(mock_client, _guard_args(tmp_path, ["--dry-run"])) + out = capsys.readouterr().out + assert "DRY RUN" in out + assert "algolia_display_results" in out + mock_client.update_agent.assert_not_called() + + +def test_update_allowed_when_nothing_is_removed(tmp_path): + """A plain agent with nothing extra must still be updatable without --force.""" + from algolia_agent.cli import cmd_update + + mock_client = MagicMock() + mock_client.get_agent.return_value = _make_current_agent() + mock_client.update_agent.return_value = {"id": "agent-uuid", "name": "Old Agent", "status": "draft"} + prompt = tmp_path / "PROMPT.md" + prompt.write_text("New instructions.") + config = tmp_path / "config.json" + config.write_text(json.dumps({ + "name": "Old Agent", "model": "gemini-2.5-flash", "instructions": str(prompt), + "index": "products", "index_description": "Product catalog.", + "replicas": [{"index": "products_price_asc", "description": "Sorted by price ascending."}], + })) + args = build_parser().parse_args(["update", "agent-uuid", "--config", str(config)]) + cmd_update(mock_client, args) + mock_client.update_agent.assert_called_once() + + +def test_removals_names_the_supplied_config_path(tmp_path): + """update accepts --config with any path, so the message must not hard-code one.""" + from algolia_agent.cli import cmd_update + + mock_client = MagicMock() + mock_client.get_agent.return_value = _rich_current_agent() + prompt = tmp_path / "PROMPT.md" + prompt.write_text("Old instructions.") + config = tmp_path / "my-custom-name.json" + config.write_text(json.dumps({ + "name": "Rich Agent", "model": "gemini-2.5-flash", "instructions": str(prompt), + "index": "products", "index_description": "Product catalog.", + "config": {"suggestions": {"enabled": True}}, + })) + args = build_parser().parse_args(["update", "agent-uuid", "--config", str(config)]) + with pytest.raises(SystemExit) as exc: + cmd_update(mock_client, args) + assert "my-custom-name.json" in str(exc.value) + assert "agent-config.json" not in str(exc.value) + + +def test_removals_says_dropped_for_unknown_default(): + from algolia_agent.cli import _removals + + current = _rich_current_agent() + current["tools"][0]["someFutureField"] = 42 + out = "\n".join(_removals(current, _narrow_payload())) + assert "someFutureField: 42 would be dropped from the payload" in out