From 15c4d698fb22996bac5c1bb202dc585b4c91aa84 Mon Sep 17 00:00:00 2001 From: Chuck Meyer Date: Wed, 26 Aug 2026 20:19:38 -0400 Subject: [PATCH 1/2] feat(update): refuse updates that would delete existing configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of #14. The API replaces objects rather than merging, so a payload narrower than the agent silently deletes the difference. agent-config.json cannot express a second tool, tool-level fields like mode, or per-index searchControls, so those losses are never something the user asked for — they are the config model being narrower than the API. _removals() reports exactly those inexpressible losses and cmd_update refuses unless --force. Index membership is deliberately excluded: the config names its indices, so dropping one is a choice rather than an accident. Fields already at their API default are skipped, since omitting those changes nothing. --dry-run is not blocked; it writes nothing and its job is to report. _tool_key and _TOOL_IDENTITY move to module scope so the guard and _diff share one definition instead of duplicating the rules. Verified end to end against a live throwaway agent: without --force the update is refused and the agent is left byte-for-byte intact; with --force every warned-about loss actually occurs, so the warnings are accurate rather than defensive. Co-Authored-By: Claude Opus 5 (1M context) --- src/algolia_agent/cli.py | 85 ++++++++++++++++++++-- tests/test_cli.py | 147 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 7 deletions(-) diff --git a/src/algolia_agent/cli.py b/src/algolia_agent/cli.py index e898773..0255529 100644 --- a/src/algolia_agent/cli.py +++ b/src/algolia_agent/cli.py @@ -290,6 +290,16 @@ 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 _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 +407,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 +417,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,8 +428,8 @@ 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 @@ -454,6 +460,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} would revert to its default (now {_fmt(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 +599,20 @@ 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): + 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" + "anything missing from the payload is lost. agent-config.json cannot express\n" + "them, 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 +958,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..cf1fbe8 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1331,3 +1331,150 @@ 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" in joined and "dynamic" 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() From 701bd84a308cb1c4b58ecedb5b8dbed691956ca4 Mon Sep 17 00:00:00 2001 From: Chuck Meyer Date: Wed, 26 Aug 2026 20:26:53 -0400 Subject: [PATCH 2/2] fix(update): name the actual default and the actual config file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on this PR. The refusal message hard-coded "agent-config.json", but update takes --config with any path and can run with no config file at all, so the message was sometimes simply wrong. It now names the path in use, or "the fields you supplied" when there is no config file. "would revert to its default (now 'dynamic')" put the word default next to the current value, reading as though dynamic were the default. Both defaults are known, so name them: "'dynamic' would revert to its default of 'static'". For a field with no known default, say it would be dropped rather than inventing one — which also keeps the message honest as the API grows fields we have not probed. Extended the same phrasing to the matching _diff line, slightly beyond the review's scope: it carried the identical ambiguity from the identical source, and two different wordings for one phenomenon is worse than either. Co-Authored-By: Claude Opus 5 (1M context) --- src/algolia_agent/cli.py | 24 +++++++++++++++--- tests/test_cli.py | 54 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/algolia_agent/cli.py b/src/algolia_agent/cli.py index 0255529..61f6460 100644 --- a/src/algolia_agent/cli.py +++ b/src/algolia_agent/cli.py @@ -296,6 +296,19 @@ def cmd_create(client: AlgoliaAgentClient, args: argparse.Namespace): _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 "" @@ -433,8 +446,10 @@ def _diff(current: dict, new_payload: dict) -> list[str]: 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:") @@ -485,7 +500,7 @@ def _removals(current: dict, new_payload: dict) -> list[str]: 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} would revert to its default (now {_fmt(val)})") + out.append(f" - {key}.{field}: {_omitted_field_note(field, val)}") curr_sc = { i["index"]: i.get("searchControls") @@ -603,12 +618,13 @@ def cmd_update(client: AlgoliaAgentClient, args: argparse.Namespace): # 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" - "anything missing from the payload is lost. agent-config.json cannot express\n" - "them, which is why they are absent.\n\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." ) diff --git a/tests/test_cli.py b/tests/test_cli.py index cf1fbe8..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([{ @@ -1371,7 +1387,7 @@ def test_removals_lists_every_loss_the_config_cannot_express(): 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" in joined and "dynamic" 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 @@ -1478,3 +1494,33 @@ def test_update_allowed_when_nothing_is_removed(tmp_path): 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