Skip to content

Commit dce5c05

Browse files
committed
fix(cli): stop breaking commands people already type (#1544, #1550)
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 <martin.vogel.tech@gmail.com>
1 parent 7f23a66 commit dce5c05

4 files changed

Lines changed: 61 additions & 2 deletions

File tree

‎src/cli/cli.c‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11482,17 +11482,31 @@ int cbm_cmd_update(int argc, char **argv) {
1148211482

1148311483
bool dry_run = false;
1148411484
bool force = false;
11485+
bool legacy_variant_flag = false;
1148511486
for (int i = 0; i < argc; i++) {
1148611487
if (strcmp(argv[i], "--dry-run") == 0) {
1148711488
dry_run = true;
1148811489
} else if (strcmp(argv[i], "--force") == 0) {
1148911490
force = true;
11491+
} else if (strcmp(argv[i], "--ui") == 0 || strcmp(argv[i], "--standard") == 0) {
11492+
/* v0.10.2 deleted the ui/standard chooser and, with it, these flags —
11493+
* which turned `update --ui` from a working command into a hard
11494+
* error for everyone who had it in a script, an alias, or muscle
11495+
* memory (#1544). Removing a CHOICE is fine; removing the WORDS
11496+
* people type is a break we owe them nothing for. Accept both,
11497+
* ignore them, and say once why they no longer mean anything. */
11498+
legacy_variant_flag = true;
1149011499
} else if (strcmp(argv[i], "-y") != 0 && strcmp(argv[i], "--yes") != 0 &&
1149111500
strcmp(argv[i], "-n") != 0 && strcmp(argv[i], "--no") != 0) {
1149211501
(void)fprintf(stderr, "error: unknown update option: %s\n", argv[i]);
1149311502
return CLI_TRUE;
1149411503
}
1149511504
}
11505+
if (legacy_variant_flag) {
11506+
(void)fprintf(stderr, "note: --ui/--standard are accepted but no longer do anything; since "
11507+
"v0.10.0 there is one build per platform and it always includes the "
11508+
"graph UI.\n");
11509+
}
1149611510

1149711511
/* Updates run from the install script, not from this process — on every
1149811512
* platform.

‎src/cli/client_adapter.c‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,15 @@ char *cbm_client_adapter_pi(const char *binary_path) {
155155
"}\n\n");
156156

157157
/* Register every tool the registry advertises — never a hand-picked subset,
158-
* which is the drift this generator exists to prevent. */
159-
sb_append(&sb, "export function register(pi) {\n");
158+
* which is the drift this generator exists to prevent.
159+
*
160+
* DEFAULT export, and it must stay that way: Pi loads an extension by
161+
* calling its default export as a factory. A named `register` export made
162+
* the file fail to load, and a Pi extension that fails to load takes EVERY
163+
* pi command down with it — `pi doctor` included — not just cbm's tools
164+
* (#1550). The named binding is kept alongside for anything that imported
165+
* it by name. */
166+
sb_append(&sb, "export default function (pi) {\n");
160167
for (int i = 0; i < count; i++) {
161168
const char *name = cbm_mcp_tool_name(i);
162169
if (!name || !name[0]) {

‎tests/test_agent_clients.c‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,6 +1096,24 @@ TEST(client_adapter_pi_registers_every_registry_tool) {
10961096
PASS();
10971097
}
10981098

1099+
/* #1550: Pi loads an extension by calling its DEFAULT export as a factory. The
1100+
* generator emitted a named `register` export instead, so the file failed to
1101+
* load — and a Pi extension that fails to load takes every pi command with it
1102+
* (`pi doctor` included), not just cbm's tools. The blast radius is why this is
1103+
* pinned rather than left to the registry-drift test above, which passed the
1104+
* whole time this was broken: it checked WHICH tools were listed, never whether
1105+
* the file Pi loads is loadable. */
1106+
TEST(client_adapter_pi_default_exports_its_factory_issue1550) {
1107+
char *js = cbm_client_adapter_pi("/usr/local/bin/codebase-memory-mcp");
1108+
ASSERT_NOT_NULL(js);
1109+
ASSERT_NOT_NULL(strstr(js, "export default function"));
1110+
/* The old shape must be gone: a bare `export function register(pi)` is the
1111+
* exact form Pi rejects. */
1112+
ASSERT_NULL(strstr(js, "export function register(pi)"));
1113+
free(js);
1114+
PASS();
1115+
}
1116+
10991117
/* #616 rejected `"` in its path template but not `\`, so a Windows home like
11001118
* C:\Users\urs\bin produced `\u` — an invalid unicode escape — and the whole
11011119
* auto-loaded plugin failed to parse. A broken plugin in an auto-load directory
@@ -1185,6 +1203,7 @@ SUITE(agent_clients) {
11851203
RUN_TEST(agent_clients_continue_uses_owned_yaml_sequence_item);
11861204
RUN_TEST(agent_clients_continue_refuses_foreign_same_name_and_nonsequence_section);
11871205
RUN_TEST(client_adapter_pi_registers_every_registry_tool);
1206+
RUN_TEST(client_adapter_pi_default_exports_its_factory_issue1550);
11881207
RUN_TEST(client_adapter_escapes_windows_paths_and_quotes);
11891208
RUN_TEST(client_adapter_opencode_sends_the_required_hook_event);
11901209
RUN_TEST(client_adapter_rejects_missing_binary_path);

‎tests/test_cli.c‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12061,6 +12061,24 @@ TEST(cli_sha256_file_matches_known_vector) {
1206112061
PASS();
1206212062
}
1206312063

12064+
/* #1544: v0.10.2 deleted the ui/standard chooser AND the flags that drove it,
12065+
* so `update --ui` — a command people had in scripts and aliases — started
12066+
* failing with "unknown update option". Retiring a choice is fine; breaking the
12067+
* words people already type is not. Both flags must be accepted, do nothing,
12068+
* and say so once. A genuinely unknown flag must still be rejected, or this
12069+
* degenerates into ignoring typos. */
12070+
TEST(cli_update_accepts_retired_variant_flags_issue1544) {
12071+
char *ui_argv[] = {"--dry-run", "--ui", "--yes"};
12072+
char *standard_argv[] = {"--dry-run", "--standard", "--yes"};
12073+
char *bogus_argv[] = {"--dry-run", "--not-a-flag", "--yes"};
12074+
12075+
ASSERT_EQ(cbm_cmd_update(3, ui_argv), 0);
12076+
ASSERT_EQ(cbm_cmd_update(3, standard_argv), 0);
12077+
/* Unknown flags stay an error — the point is compatibility, not silence. */
12078+
ASSERT_TRUE(cbm_cmd_update(3, bogus_argv) != 0);
12079+
PASS();
12080+
}
12081+
1206412082
#ifdef _WIN32
1206512083
/* The Windows update contract, asserted with the activation seam OFF so this
1206612084
* takes the exact dispatch a release binary ships: `update` never replaces the
@@ -12136,6 +12154,7 @@ SUITE(cli) {
1213612154
RUN_TEST(cli_install_config_failure_keeps_published_binary);
1213712155
RUN_TEST(cli_update_download_failure_does_not_quiesce_sessions);
1213812156
RUN_TEST(cli_update_already_current_does_not_quiesce_sessions);
12157+
RUN_TEST(cli_update_accepts_retired_variant_flags_issue1544);
1213912158
RUN_TEST(cli_update_agent_configs_finish_before_guard_release);
1214012159
RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index);
1214112160
RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain);

0 commit comments

Comments
 (0)