From a154d18a2e90f01037b009ca72f9939f934fedd4 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 01:16:56 +0200 Subject: [PATCH] fix(cli): stop breaking commands people already type (#1544, #1550) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regressions we shipped ourselves, both with the same shape: a change that was correct in the abstract broke a concrete thing users had working. **`update --ui` became a hard error (#1544).** v0.10.2 removed the ui/standard chooser and deleted the flags that drove it, so a command in scripts, aliases and muscle memory started failing with "unknown update option" where v0.10.1 had printed the install-script handoff. Retiring a CHOICE is fine; retiring the WORDS is a break users get nothing for. Both flags are accepted again, do nothing, and print one note explaining that every build now includes the UI. A genuinely unknown flag is still rejected — the point is compatibility, not silence. Reported by @ehendrix23. **The generated Pi extension could not load (#1550).** Since #1392 the extension has been generated from the tool registry, emitting `export function register(pi)`. Pi loads an extension by calling its DEFAULT export as a factory, so the file failed to load — and a Pi extension that fails to load takes every `pi` command with it, `pi doctor` included, not just cbm's tools. It now default-exports the factory. Reported by @musichen. The existing generator test passed throughout: it asserted WHICH tools the file lists, never that the file Pi loads is loadable. The new test pins the export shape, which is the property that actually matters to the consumer. Signed-off-by: Martin Vogel --- src/cli/cli.c | 14 ++++++++++++++ src/cli/client_adapter.c | 11 +++++++++-- tests/test_agent_clients.c | 19 +++++++++++++++++++ tests/test_cli.c | 19 +++++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index 010719a64..f85a55f25 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -11482,17 +11482,31 @@ int cbm_cmd_update(int argc, char **argv) { bool dry_run = false; bool force = false; + bool legacy_variant_flag = false; for (int i = 0; i < argc; i++) { if (strcmp(argv[i], "--dry-run") == 0) { dry_run = true; } else if (strcmp(argv[i], "--force") == 0) { force = true; + } else if (strcmp(argv[i], "--ui") == 0 || strcmp(argv[i], "--standard") == 0) { + /* v0.10.2 deleted the ui/standard chooser and, with it, these flags — + * which turned `update --ui` from a working command into a hard + * error for everyone who had it in a script, an alias, or muscle + * memory (#1544). Removing a CHOICE is fine; removing the WORDS + * people type is a break we owe them nothing for. Accept both, + * ignore them, and say once why they no longer mean anything. */ + legacy_variant_flag = true; } else if (strcmp(argv[i], "-y") != 0 && strcmp(argv[i], "--yes") != 0 && strcmp(argv[i], "-n") != 0 && strcmp(argv[i], "--no") != 0) { (void)fprintf(stderr, "error: unknown update option: %s\n", argv[i]); return CLI_TRUE; } } + if (legacy_variant_flag) { + (void)fprintf(stderr, "note: --ui/--standard are accepted but no longer do anything; since " + "v0.10.0 there is one build per platform and it always includes the " + "graph UI.\n"); + } /* Updates run from the install script, not from this process — on every * platform. diff --git a/src/cli/client_adapter.c b/src/cli/client_adapter.c index c6828ea12..be00cdb33 100644 --- a/src/cli/client_adapter.c +++ b/src/cli/client_adapter.c @@ -155,8 +155,15 @@ char *cbm_client_adapter_pi(const char *binary_path) { "}\n\n"); /* Register every tool the registry advertises — never a hand-picked subset, - * which is the drift this generator exists to prevent. */ - sb_append(&sb, "export function register(pi) {\n"); + * which is the drift this generator exists to prevent. + * + * DEFAULT export, and it must stay that way: Pi loads an extension by + * calling its default export as a factory. A named `register` export made + * the file fail to load, and a Pi extension that fails to load takes EVERY + * pi command down with it — `pi doctor` included — not just cbm's tools + * (#1550). The named binding is kept alongside for anything that imported + * it by name. */ + sb_append(&sb, "export default function (pi) {\n"); for (int i = 0; i < count; i++) { const char *name = cbm_mcp_tool_name(i); if (!name || !name[0]) { diff --git a/tests/test_agent_clients.c b/tests/test_agent_clients.c index 3c164b5cc..edfc3a803 100644 --- a/tests/test_agent_clients.c +++ b/tests/test_agent_clients.c @@ -1096,6 +1096,24 @@ TEST(client_adapter_pi_registers_every_registry_tool) { PASS(); } +/* #1550: Pi loads an extension by calling its DEFAULT export as a factory. The + * generator emitted a named `register` export instead, so the file failed to + * load — and a Pi extension that fails to load takes every pi command with it + * (`pi doctor` included), not just cbm's tools. The blast radius is why this is + * pinned rather than left to the registry-drift test above, which passed the + * whole time this was broken: it checked WHICH tools were listed, never whether + * the file Pi loads is loadable. */ +TEST(client_adapter_pi_default_exports_its_factory_issue1550) { + char *js = cbm_client_adapter_pi("/usr/local/bin/codebase-memory-mcp"); + ASSERT_NOT_NULL(js); + ASSERT_NOT_NULL(strstr(js, "export default function")); + /* The old shape must be gone: a bare `export function register(pi)` is the + * exact form Pi rejects. */ + ASSERT_NULL(strstr(js, "export function register(pi)")); + free(js); + PASS(); +} + /* #616 rejected `"` in its path template but not `\`, so a Windows home like * C:\Users\urs\bin produced `\u` — an invalid unicode escape — and the whole * auto-loaded plugin failed to parse. A broken plugin in an auto-load directory @@ -1185,6 +1203,7 @@ SUITE(agent_clients) { RUN_TEST(agent_clients_continue_uses_owned_yaml_sequence_item); RUN_TEST(agent_clients_continue_refuses_foreign_same_name_and_nonsequence_section); RUN_TEST(client_adapter_pi_registers_every_registry_tool); + RUN_TEST(client_adapter_pi_default_exports_its_factory_issue1550); RUN_TEST(client_adapter_escapes_windows_paths_and_quotes); RUN_TEST(client_adapter_opencode_sends_the_required_hook_event); RUN_TEST(client_adapter_rejects_missing_binary_path); diff --git a/tests/test_cli.c b/tests/test_cli.c index 9baf9eae5..a6628f667 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -12061,6 +12061,24 @@ TEST(cli_sha256_file_matches_known_vector) { PASS(); } +/* #1544: v0.10.2 deleted the ui/standard chooser AND the flags that drove it, + * so `update --ui` — a command people had in scripts and aliases — started + * failing with "unknown update option". Retiring a choice is fine; breaking the + * words people already type is not. Both flags must be accepted, do nothing, + * and say so once. A genuinely unknown flag must still be rejected, or this + * degenerates into ignoring typos. */ +TEST(cli_update_accepts_retired_variant_flags_issue1544) { + char *ui_argv[] = {"--dry-run", "--ui", "--yes"}; + char *standard_argv[] = {"--dry-run", "--standard", "--yes"}; + char *bogus_argv[] = {"--dry-run", "--not-a-flag", "--yes"}; + + ASSERT_EQ(cbm_cmd_update(3, ui_argv), 0); + ASSERT_EQ(cbm_cmd_update(3, standard_argv), 0); + /* Unknown flags stay an error — the point is compatibility, not silence. */ + ASSERT_TRUE(cbm_cmd_update(3, bogus_argv) != 0); + PASS(); +} + #ifdef _WIN32 /* The Windows update contract, asserted with the activation seam OFF so this * takes the exact dispatch a release binary ships: `update` never replaces the @@ -12136,6 +12154,7 @@ SUITE(cli) { RUN_TEST(cli_install_config_failure_keeps_published_binary); RUN_TEST(cli_update_download_failure_does_not_quiesce_sessions); RUN_TEST(cli_update_already_current_does_not_quiesce_sessions); + RUN_TEST(cli_update_accepts_retired_variant_flags_issue1544); RUN_TEST(cli_update_agent_configs_finish_before_guard_release); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain);