fix(cli): copy and sign the built binary on install instead of re-signing the stale target

[halindrome]

Summary

install (especially with --force) only ad-hoc-signed whatever binary already sat at ~/.local/bin/codebase-memory-mcp and never copied the binary the operator ran install from. An operator who built from source and ran install -y --force was silently left with the OLD binary at the target — running stale code while believing they had upgraded.

This makes install mirror what update already does for the binary swap.

What changed

• Copy then sign the target. install now detects the running binary, copies it to the canonical target ~/.local/bin/codebase-memory-mcp, makes it executable, and ad-hoc signs the target — not whatever the operator happened to launch.
• Same-file guard. cbm_copy_file opens the destination "wb" before reading the source, so copying a file onto itself would truncate it to zero. A new cbm_same_file() check skips the copy when the running binary already is the target (the normal release-install case), preserving prior behavior.
• Safe overwrite semantics. A different pre-existing binary at the target is replaced only with --force or an interactive confirmation. --dry-run reports the intended swap and touches nothing.
• Configs point at the target. Agent configs are now written pointing at the install target (matching update).

Secondary issue (linker-signed manual copies)

The copy+sign path also resolves the secondary report: a freshly clang-built arm64 binary is adhoc, linker-signed (flags=0x20002) and is Killed: 9 when spawned by another process (e.g. an MCP host), even though it runs fine from a shell. Re-signing ad-hoc (flags=0x2) makes it launchable — install now does this for the binary it places, so the manual cp + codesign --force --sign - workaround is no longer required.

Implementation

• src/cli/cli.c — new cbm_same_file() and cbm_copy_binary_to_target() helpers; cbm_cmd_install() now does copy → sign(target) → configs(target).
• src/cli/cli.h — declare cbm_copy_binary_to_target().
• tests/test_cli.c — two regression tests: cli_install_copies_binary_to_target_issue472 (fresh + stale-overwrite copy, exec bit) and cli_install_same_file_guard_issue472 (self-copy must not truncate, incl. a symlink/same-inode case).

Test coverage note

The regression tests target the extracted cbm_copy_binary_to_target() seam rather than cbm_cmd_install() directly. A full cbm_cmd_install() invocation is deliberately avoided in unit tests because it has process-global side effects (it SIGTERMs other codebase-memory-mcp instances) and HOME/PATH/config coupling that don't isolate cleanly in the suite. As a result the --force/prompt branch, the --dry-run "would" messaging, and the config-target redirection are validated by reading/review rather than automated tests; the copy + same-file-guard logic (the actual bug) is covered by the helper-level tests.

Validation

• scripts/build.sh — clean (-Werror).
• clang-format — clean on all changed files.
• scripts/test.sh — 5605 passed; the single remaining failure (cli_hook_gate_script_no_predictable_tmp_issue384) is pre-existing on main (confirmed identical baseline without this change) and unrelated to this PR. Both new issue472 tests pass.

Closes #472

🤖 Generated with Claude Code

[halindrome]

QA Round 1 — PR #473

Verdict: APPROVE WITH NITS. The fix correctly resolves #472: install now detects the running binary, copies it to the canonical target (~/.local/bin/codebase-memory-mcp[.exe]), sets the exec bit, ad-hoc-signs the target, and points agent configs at the target — faithfully mirroring the established cbm_cmd_update copy→sign→configs pattern. The self-truncation guard (cbm_same_file via st_dev/st_ino) is sound, dry-run is properly gated, and the two regression tests exercise the core hazards. No blockers or majors. The findings below are minor/advisory: a couple of edge cases the tests don't cover and one defensible-but-worth-noting return CLI_TRUE on copy failure. The logic is correct as written.

Findings

Severity	Location	Finding	Recommendation
advisory	cli.c (Step 1c/1d)	Ordering vs update is correct. Kill-instances (SIGTERM via pgrep -x) runs before the copy. On Unix this does not interfere: cbm_copy_file opens the source (the running binary, still alive — it's the current process) read-only and creates the dest fresh; killing other server processes does not affect the running CLI's own executable file. cbm_kill_other_instances excludes self (getpid() skip / Win PID filter). No race.	None.
advisory	cli.c cbm_same_file	Same-file guard is robust to non-canonical cbm_detect_self_path. On macOS _NSGetExecutablePath may return a non-canonical/symlinked path, but cbm_same_file calls stat() on both operands, and stat resolves symlinks/../. before comparing st_dev+st_ino. So the dev/ino comparison correctly detects "same file" regardless of path form. Hardlinks/symlinks to the same inode are also correctly treated as same-file (copy skipped) — the safe choice here.	None — hypothesis confirmed.
minor	cli.c (copy-failure path)	Copy failure returns CLI_TRUE (exit 1), aborting before signing/configs. Defensible (a failed binary copy is fatal — continuing would point configs at a stale/absent binary), and matches update's convention. No leak: no fds/heap held at that point.	Keep. Optionally note in the error message that configs were not updated.
minor	cli.c (chmod)	chmod failure is silently ignored ((void)chmod). Acceptable: if the file was just created/copied by the same user, chmod failure is near-impossible, and the binary is still present. Consistent with the codebase's other (void)chmod/(void)fchmod sites.	Keep.
minor	test_cli.c cli_install_same_file_guard_issue472	The same-file test passes the identical path string (cbm_copy_binary_to_target(path, path)). It does not cover the more realistic case the guard exists for: two different path strings resolving to the same inode (e.g. a symlink, or ./self vs absolute) — exactly what a non-canonical _NSGetExecutablePath vs the hardcoded bin_target could produce. The dev/ino logic handles it, but it's untested.	Add a case using symlink() (or relative vs absolute path) to the same file; assert the copy is skipped and content survives.
minor	test suite	Neither test exercises cbm_cmd_install itself — so the force/prompt branch, dry-run messaging, config-target redirection, and macOS sign gating are uncovered by automated tests. Given the kill-instances side effect (SIGTERMs real processes) and HOME/network coupling, avoiding a full cbm_cmd_install invocation in unit tests is a reasonable limitation.	Acceptable; note the gap in the PR description. Helper-level coverage is the right seam.
advisory	cli.c (Step 1d comment)	Comment claims linker-signed flags=0x20002 vs ad-hoc flags=0x2. Not load-bearing and broadly accurate for arm64 linker vs ad-hoc signatures.	None.
advisory	cli.c / cli.h	cbm_same_file is static while cbm_copy_binary_to_target is exported as the regression surface. Asymmetric but intentional — the guard is tested indirectly through the exported wrapper. cbm_normalize_path_sep (used only in the #ifdef _WIN32 branch) is declared in platform.h and always compiled, so no unused-symbol/missing-decl warning on either platform.	None.

Detail on the items that matter

Semantic equivalence with update. update signs bin_dest unconditionally after download and returns early on dry-run, so it needs no !dry_run guard around signing; install cannot return early (it still does PATH/config work) so it correctly wraps the sign block in if (!dry_run). Both repoint configs: update passes literal true; install passes its force variable, set to true after a prompt approval so cbm_install_agent_configs overwrites existing configs — intended behavior, not a side-effect bug. User-facing messages match update's wording.

Force/prompt/-y interaction — no silent-stale path remains. For "a different binary already exists at target":

• install --force / -y --force: force=true → prompt skipped → copies. ✔ (the install --force re-signs the existing binary in place instead of replacing it; manual copies are linker-signed and killed when spawned #472 bug, now fixed)
• install -y (no --force): enters prompt branch → auto-yes → do_copy=true; force=true → copies. ✔
• interactive yes: copies, sets force=true so configs repoint. ✔
• interactive no: copy skipped, configs point at the existing target the user chose to keep (message is accurate). ✔

When self_path already is bin_target, cbm_same_file short-circuits the block and signing still runs on the target — preserving the original re-sign-in-place behavior.

Dry-run. Copy block prints "Would install …" and performs no mkdir/copy; the macOS sign block is gated by if (!dry_run); configs are invoked with dry_run=true so they don't write. No mutation occurs.

Windows. bin_target gets .exe; chmod is #ifndef _WIN32; cbm_same_file's Windows branch normalizes both paths and strcmps them. cbm_detect_self_path on Windows calls GetModuleFileNameA then cbm_normalize_path_sep, and bin_target is built with forward slashes, so the two strings are comparable. (The Windows string-compare guard is weaker than dev/ino, but a redundant copy there is harmless since the source is the live process file opened read-only.)

Required changes

None. Recommended (non-blocking): add a symlink/relative-path case to cli_install_same_file_guard_issue472, and note the cbm_cmd_install-level coverage gap in the PR description.

[halindrome]

QA Round 2 — PR #473

Verdict: APPROVE WITH NITS. The fix is correct and complete for the bug it targets (#472). End-to-end the new flow is sound: it detects the running binary (cbm_detect_self_path, with a safe fallback to the target path on detection failure), guards self-copy via a real dev/ino comparison (cbm_same_file), copies + chmod 0755 + ad-hoc-signs the target (not the running file), and points configs at the target. Ordering relative to cbm_cmd_update is fine — cbm_kill_other_instances() runs before the copy, so a target binary in use by an MCP host is stopped before it's overwritten. The --dry-run/--force/prompt matrix has no remaining silent-stale path: when the target is absent, do_copy is unconditionally true; when present without --force, the operator is prompted; signing and copy are both correctly gated on !dry_run. The round-1 symlink/same-inode test is present, correct, registered in the runner, and genuinely exercises a distinct path string (self-link → self) resolving to one inode. No blockers or majors; one minor robustness note and two advisory items below.

Severity	Location	Finding	Recommendation
Minor	cbm_copy_binary_to_target → cbm_copy_file	Copy uses cbm_copy_file, which fopen(dst,"wb") truncates the target inode in place before reading src. If the copy fails partway (read error, ENOSPC), the target is left truncated/corrupt — unlike cbm_replace_binary, which unlinks first to get a fresh inode. The pre-copy cbm_kill_other_instances() removes the running-binary risk, but partial-write corruption on a failed copy is still possible.	Acceptable for an interactive installer (error is printed, operator re-runs). If hardening is desired, write to bin_target.tmp then rename() into place for atomic replacement. Not required to merge.
Advisory	read_test_file (test helper)	Returns a static char buf[8192] (shared, not fresh per call). Current tests read→assert→read→assert sequentially, so the reuse is benign here.	Leave as-is; flagged so a future test that holds two read_test_file results live simultaneously doesn't get bitten.
Advisory	cbm_cmd_install (stat→copy)	TOCTOU between stat(bin_target) and the copy, and the "~/.local/bin exists as a non-directory" case. Both are benign: the only thing between stat and copy is an interactive prompt, and a non-dir parent makes cbm_mkdir_p/cbm_copy_file fail with a printed error and return CLI_TRUE (no crash). Both are pre-existing conditions, not introduced by this PR.	No change.

Notes on items specifically probed

• Configs pointing at a non-existent target after a decline: not reachable. The decline branch only executes when target_exists == true, so configs always point at a binary that exists. When the target is absent, do_copy is forced true with no prompt, so it's always created before configs are written.
• force = true after prompt approval: force is a function-local parsed from argv; mutating it is intentional and propagates to cbm_install_agent_configs(…, force, …) so an approved binary replacement also force-refreshes configs. Coherent; the caller is unaffected.
• Symlink test correctness: uses two distinct path strings to the same inode; on non-Windows it asserts the file content survives (no truncation). The #ifndef _WIN32 guard is appropriate since symlink() and the st_dev/st_ino semantics are POSIX-only — Windows uses the normalized-string compare branch instead.
• Comment accuracy / style: the inline comments accurately describe behavior (truncation rationale, sign-the-target rationale, Killed:9 flags). CLI_OCTAL_PERM = 0755 matches the test's S_IXUSR assertion. Style is consistent with surrounding code.

Required changes

(none)