mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-11-04 10:48:20 +00:00
fix(cli): stop relocating a binary a package manager owns (#1566)
`install`/`update` bundle two separable jobs: place the binary (and put its
directory on PATH), and configure the agents. For someone who installed through
mise, Homebrew or nix, the first job is not redundant — it is harmful. It drops
a SECOND copy into ~/.local/bin that shadows the managed one depending on PATH
order, and appends to a shell rc file no package manager asked us to touch:
Would install binary -> /Users/user/.local/bin/codebase-memory-mcp
Added /Users/user/.local/bin to PATH in /Users/user/.zshrc
They wanted the agent configs refreshed and nothing else. Reported by @SC-CTS.
`--skip-config` (binary, no configs) has existed for a long time; this adds its
missing mirror. Per the user's decision:
- **Infer by default.** A recognised manager path means the binary is not ours;
we configure the agents and leave the binary and PATH alone.
- **`--skip-binary` / `--force-binary`** override the inference either way.
- **`update` REFUSES** on a foreign binary and names the command that will work
(`mise upgrade`, `brew upgrade`), rather than reporting success while changing
nothing — the dishonest-success pattern this project keeps having to fix.
- **PATH is only touched when WE placed the binary**, which is a bug in its own
right regardless of the rest.
Detection is POSITIVE evidence — the path must look like a known manager's — and
the first implementation got this wrong in the tempting way: "anything outside
our install dir". The test suite failed immediately and was right to. A test
binary runs from build/, and an ordinary `install --dir=/opt/cbm` also lives
outside the default; both would be misread as foreign, and `update` would then
refuse to update an installation we own. The asymmetry settles it: a false
positive costs a working update, while a false negative merely leaves today's
behaviour for an unrecognised manager, which --skip-binary covers explicitly.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
1 parent
1649fa8344
commit
0e89ef87ba
3 files changed
+171
-2
No files matched your search
+136
-2
@@ -9225,6 +9225,71 @@ static bool cbm_detect_self_path(char *buf, size_t buf_sz, const char *home) {
|
||||
return exact;
|
||||
}
|
||||
|
||||
/* Is the running binary owned by an external package manager rather than by us?
|
||||
*
|
||||
* #1566: cbm's installer does two separable jobs — place the binary (and put its
|
||||
* directory on PATH), and configure the agents. For someone who installed
|
||||
* through mise, Homebrew or nix, the first job is not merely redundant: it drops
|
||||
* a SECOND copy into ~/.local/bin that shadows the managed one depending on PATH
|
||||
* order, and appends to a shell rc file the package manager never asked us to
|
||||
* touch. They wanted the agent configs refreshed and nothing else.
|
||||
*
|
||||
* The signal is simply that the OS says we are running from somewhere other than
|
||||
* the directory we install into. We only infer when the OS REPORTED the path
|
||||
* (exact): the ~/.local/bin fallback is a guess, and guessing here would refuse
|
||||
* to install for someone who has no binary at all. Naming the manager is
|
||||
* best-effort — the behaviour does not depend on recognising it, only the
|
||||
* wording does. */
|
||||
static const char *cli_external_manager_name(const char *self_path) {
|
||||
if (!self_path || !self_path[0]) {
|
||||
return NULL;
|
||||
}
|
||||
if (strstr(self_path, "/mise/") || strstr(self_path, "/.mise/")) {
|
||||
return "mise";
|
||||
}
|
||||
if (strstr(self_path, "/Cellar/") || strstr(self_path, "/homebrew/") ||
|
||||
strstr(self_path, "/linuxbrew/")) {
|
||||
return "Homebrew";
|
||||
}
|
||||
if (strstr(self_path, "/nix/store/")) {
|
||||
return "nix";
|
||||
}
|
||||
if (strstr(self_path, "/.asdf/")) {
|
||||
return "asdf";
|
||||
}
|
||||
if (strstr(self_path, "/.cargo/bin/")) {
|
||||
return "cargo";
|
||||
}
|
||||
return NULL;
|
||||
}
|
||||
|
||||
#ifdef CBM_CLI_ENABLE_TEST_API
|
||||
const char *cbm_cli_external_manager_name_for_testing(const char *self_path) {
|
||||
return cli_external_manager_name(self_path);
|
||||
}
|
||||
#endif
|
||||
|
||||
static bool cli_binary_is_externally_managed(const char *self_path, bool self_path_exact,
|
||||
const char *bin_dir) {
|
||||
(void)bin_dir;
|
||||
if (!self_path_exact || !self_path || !self_path[0]) {
|
||||
return false;
|
||||
}
|
||||
/* POSITIVE evidence only: the path must look like a known package manager's.
|
||||
*
|
||||
* The tempting rule — "anything outside the directory we install into" —
|
||||
* is wrong, and the test suite proved it immediately: a test binary runs
|
||||
* from build/, and a perfectly ordinary `install --dir=/opt/cbm` also lives
|
||||
* outside the default. Both would be misread as foreign, and `update` would
|
||||
* refuse to update an installation we own.
|
||||
*
|
||||
* The asymmetry decides it. A false positive REFUSES a legitimate update; a
|
||||
* false negative just leaves today's behaviour in place for an unrecognised
|
||||
* manager, which --skip-binary covers explicitly. So we only infer when we
|
||||
* can name the owner. */
|
||||
return cli_external_manager_name(self_path) != NULL;
|
||||
}
|
||||
|
||||
/* Build the agent.install.plan.v1 receipt (#388): a machine-readable list of
|
||||
* the config / instruction / skill / agent / hook files `install` WOULD write, produced by
|
||||
* running the real install dispatch in record-only mode (no mutation, no
|
||||
@@ -9355,6 +9420,10 @@ typedef struct {
|
||||
cli_binary_validator_t binary_validator;
|
||||
bool has_binary_validator;
|
||||
bool copy_binary;
|
||||
/* #1566: true when the binary is not ours to place — suppresses the PATH
|
||||
* edit too, since adding a directory to PATH for a binary we did not
|
||||
* install is exactly the unasked-for change that was complained about. */
|
||||
bool skip_binary;
|
||||
bool delete_indexes;
|
||||
bool skip_config;
|
||||
bool force;
|
||||
@@ -9429,6 +9498,14 @@ static int cli_install_activate(void *opaque) {
|
||||
return CLI_ACTIVATION_PARTIAL;
|
||||
}
|
||||
int path_rc = CLI_TRUE;
|
||||
/* #1566: only touch PATH when WE placed the binary. Appending to a shell rc
|
||||
* for a binary installed by someone else is an unasked-for change to a file
|
||||
* we do not own, and the directory we would add may hold nothing at all. */
|
||||
if (activation->skip_binary) {
|
||||
cli_activation_transaction_finalize_committed_or_fail_stop(&activation->binary_transaction,
|
||||
"install_transaction_finalize");
|
||||
return CLI_OK;
|
||||
}
|
||||
#ifdef _WIN32
|
||||
path_rc = cli_ensure_windows_user_path(activation->bin_dir,
|
||||
activation->dry_run || g_cli_activation_test_ops_set);
|
||||
@@ -9478,6 +9555,8 @@ int cbm_cmd_install(int argc, char **argv) {
|
||||
bool plan = false;
|
||||
bool reset_indexes = false;
|
||||
bool skip_config = false;
|
||||
bool skip_binary = false;
|
||||
bool force_binary = false;
|
||||
const char *requested_bin_dir = NULL;
|
||||
for (int i = 0; i < argc; i++) {
|
||||
if (strcmp(argv[i], "--dry-run") == 0) {
|
||||
@@ -9490,6 +9569,12 @@ int cbm_cmd_install(int argc, char **argv) {
|
||||
reset_indexes = true;
|
||||
} else if (strcmp(argv[i], "--skip-config") == 0) {
|
||||
skip_config = true;
|
||||
} else if (strcmp(argv[i], "--skip-binary") == 0) {
|
||||
/* #1566: the mirror of --skip-config. Configure the agents, leave
|
||||
* the binary and PATH alone. */
|
||||
skip_binary = true;
|
||||
} else if (strcmp(argv[i], "--force-binary") == 0) {
|
||||
force_binary = true;
|
||||
} else if (strncmp(argv[i], "--dir=", SLEN("--dir=")) == 0) {
|
||||
requested_bin_dir = argv[i] + SLEN("--dir=");
|
||||
if (!requested_bin_dir[0]) {
|
||||
@@ -9553,7 +9638,24 @@ int cbm_cmd_install(int argc, char **argv) {
|
||||
printf("codebase-memory-mcp install %s\n\n", CBM_VERSION);
|
||||
|
||||
char self_path[CLI_BUF_1K] = {0};
|
||||
(void)cbm_detect_self_path(self_path, sizeof(self_path), home);
|
||||
bool self_path_exact = cbm_detect_self_path(self_path, sizeof(self_path), home);
|
||||
|
||||
/* #1566: a binary owned by mise/Homebrew/nix is not ours to relocate. Infer
|
||||
* it, and let either flag override the inference in either direction. */
|
||||
bool externally_managed =
|
||||
!force_binary && cli_binary_is_externally_managed(self_path, self_path_exact, bin_dir);
|
||||
if (externally_managed || skip_binary) {
|
||||
const char *manager = cli_external_manager_name(self_path);
|
||||
if (skip_binary) {
|
||||
printf("Skipping binary placement (--skip-binary); configuring agents only.\n\n");
|
||||
} else {
|
||||
printf("Binary is managed elsewhere%s%s:\n %s\n"
|
||||
"Leaving it and your PATH untouched; configuring agents only.\n"
|
||||
" (use --force-binary to install a copy into %s anyway)\n\n",
|
||||
manager ? " by " : "", manager ? manager : "", self_path, bin_dir);
|
||||
}
|
||||
skip_binary = true;
|
||||
}
|
||||
|
||||
/* NOT stat(): on Windows it goes through the ANSI code page, so an
|
||||
* extended-length or non-ASCII target reports "absent" and a non-force
|
||||
@@ -9561,7 +9663,7 @@ int cbm_cmd_install(int argc, char **argv) {
|
||||
cbm_path_info_t target_status;
|
||||
bool target_exists = cbm_path_info_utf8(bin_target, &target_status) == 0;
|
||||
bool same_binary = cbm_same_file(self_path, bin_target);
|
||||
bool do_copy = !same_binary && (!target_exists || force);
|
||||
bool do_copy = !skip_binary && !same_binary && (!target_exists || force);
|
||||
|
||||
/* (#607) Default: preserve existing indexes. `--reset-indexes` opts into
|
||||
* the old prompt-and-delete behaviour. The helper returns 0 only when the
|
||||
@@ -9737,6 +9839,7 @@ int cbm_cmd_install(int argc, char **argv) {
|
||||
.copy_binary = do_copy,
|
||||
.delete_indexes = delete_indexes,
|
||||
.skip_config = skip_config,
|
||||
.skip_binary = skip_binary,
|
||||
.force = force,
|
||||
.dry_run = dry_run,
|
||||
};
|
||||
@@ -11564,6 +11667,37 @@ int cbm_cmd_update(int argc, char **argv) {
|
||||
"graph UI.\n");
|
||||
}
|
||||
|
||||
/* #1566: we cannot update a binary a package manager owns, and pretending
|
||||
* otherwise is the dishonest-success pattern this project keeps fixing:
|
||||
* reporting that an update ran while changing nothing leaves the user
|
||||
* convinced they are current. Refuse, and name the command that WILL work. */
|
||||
{
|
||||
char self_path[CLI_BUF_1K] = {0};
|
||||
const char *home = cbm_get_home_dir();
|
||||
bool self_exact = home && cbm_detect_self_path(self_path, sizeof(self_path), home);
|
||||
char managed_dir[CLI_BUF_1K] = {0};
|
||||
if (self_exact && home) {
|
||||
snprintf(managed_dir, sizeof(managed_dir), "%s/.local/bin", home);
|
||||
}
|
||||
if (self_exact && cli_binary_is_externally_managed(self_path, true, managed_dir)) {
|
||||
const char *manager = cli_external_manager_name(self_path);
|
||||
(void)fprintf(stderr,
|
||||
"error: this binary is managed by %s, so `update` cannot replace it:\n"
|
||||
" %s\n",
|
||||
manager ? manager : "another installer", self_path);
|
||||
if (manager && strcmp(manager, "mise") == 0) {
|
||||
(void)fprintf(stderr, " update it with: mise upgrade codebase-memory-mcp\n");
|
||||
} else if (manager && strcmp(manager, "Homebrew") == 0) {
|
||||
(void)fprintf(stderr, " update it with: brew upgrade codebase-memory-mcp\n");
|
||||
} else {
|
||||
(void)fprintf(stderr, " update it through whichever tool installed it.\n");
|
||||
}
|
||||
(void)fprintf(stderr, " to refresh only the agent configurations, run: "
|
||||
"codebase-memory-mcp install --skip-binary\n");
|
||||
return CLI_TRUE;
|
||||
}
|
||||
}
|
||||
|
||||
/* Updates run from the install script, not from this process — on every
|
||||
* platform.
|
||||
*
|
||||
|
||||
@@ -194,6 +194,13 @@ cbm_detected_agents_t cbm_detect_agents(const char *home_dir);
|
||||
/* Install or refresh every detected agent integration below home. */
|
||||
int cbm_install_agent_configs(const char *home, const char *binary_path, bool force, bool dry_run);
|
||||
|
||||
#ifdef CBM_CLI_ENABLE_TEST_API
|
||||
/* #1566: name the package manager that owns a binary path, or NULL when it is
|
||||
* not recognisably foreign. Detection must be positive evidence — see the
|
||||
* implementation comment. */
|
||||
const char *cbm_cli_external_manager_name_for_testing(const char *self_path);
|
||||
#endif
|
||||
|
||||
#ifdef CBM_CLI_ENABLE_TEST_API
|
||||
int cbm_build_qwen_hook_command_for_testing(const char *binary_path, bool windows, char *command,
|
||||
size_t command_size, char *shell, size_t shell_size);
|
||||
|
||||
@@ -12193,6 +12193,33 @@ TEST(cli_skill_frontmatter_scalars_with_colons_are_quoted_issue1554) {
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* #1566: a binary owned by mise/Homebrew/nix is not ours to relocate, and the
|
||||
* detection must key on POSITIVE evidence — a recognised manager path — not on
|
||||
* "outside our default install dir". The latter misreads an ordinary
|
||||
* `install --dir=/opt/cbm` (and every test binary) as foreign, and would then
|
||||
* REFUSE to update an installation we own. A false positive costs a working
|
||||
* update; a false negative only leaves today's behaviour for an unrecognised
|
||||
* manager, which --skip-binary covers explicitly. */
|
||||
TEST(cli_external_manager_detection_needs_positive_evidence_issue1566) {
|
||||
/* Recognised managers -> named. */
|
||||
ASSERT_NOT_NULL(cbm_cli_external_manager_name_for_testing(
|
||||
"/Users/x/.local/share/mise/installs/cbm/0.10.3/bin/codebase-memory-mcp"));
|
||||
ASSERT_NOT_NULL(cbm_cli_external_manager_name_for_testing(
|
||||
"/opt/homebrew/Cellar/codebase-memory-mcp/0.10.3/bin/codebase-memory-mcp"));
|
||||
ASSERT_NOT_NULL(
|
||||
cbm_cli_external_manager_name_for_testing("/nix/store/abc-cbm/bin/codebase-memory-mcp"));
|
||||
|
||||
/* Ours, or merely unusual, must NOT be claimed as externally managed. */
|
||||
ASSERT_NULL(
|
||||
cbm_cli_external_manager_name_for_testing("/Users/x/.local/bin/codebase-memory-mcp"));
|
||||
ASSERT_NULL(cbm_cli_external_manager_name_for_testing("/opt/cbm/codebase-memory-mcp"));
|
||||
ASSERT_NULL(cbm_cli_external_manager_name_for_testing("/usr/local/bin/codebase-memory-mcp"));
|
||||
ASSERT_NULL(cbm_cli_external_manager_name_for_testing("build/c/test-runner"));
|
||||
ASSERT_NULL(cbm_cli_external_manager_name_for_testing(""));
|
||||
ASSERT_NULL(cbm_cli_external_manager_name_for_testing(NULL));
|
||||
PASS();
|
||||
}
|
||||
|
||||
TEST(cli_update_accepts_retired_variant_flags_issue1544) {
|
||||
char *ui_argv[] = {"--dry-run", "--ui", "--yes"};
|
||||
char *standard_argv[] = {"--dry-run", "--standard", "--yes"};
|
||||
@@ -12282,6 +12309,7 @@ SUITE(cli) {
|
||||
RUN_TEST(cli_update_download_failure_does_not_quiesce_sessions);
|
||||
RUN_TEST(cli_update_already_current_does_not_quiesce_sessions);
|
||||
RUN_TEST(cli_skill_frontmatter_scalars_with_colons_are_quoted_issue1554);
|
||||
RUN_TEST(cli_external_manager_detection_needs_positive_evidence_issue1566);
|
||||
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);
|
||||
|
||||
Reference in new issue
Block a user