fix(cli): don't let an agent-config failure abort uninstall; follow user-owned symlinked configs (#1954) - #2110
Open
DeusData wants to merge 6 commits into
Open
fix(cli): don't let an agent-config failure abort uninstall; follow user-owned symlinked configs (#1954)#2110DeusData wants to merge 6 commits into
DeusData wants to merge 6 commits into
Conversation
A single agent configuration the editors refuse to touch (the reporter's ~/.cursor/mcp.json was a symlink into a dotfiles checkout) aborted the uninstall activation BEFORE index and binary removal. `uninstall -y` exited 1 and left a 300 MB executable plus the whole cache behind — the data-loss half of #1954 — while the transcript still read "removed". Agent-config failures are now collected instead of gating: every cleanup runs, the indexes and the executable go, and the command ends with one list of the files that could not be cleaned (agent, operation, path, observed target) and a non-zero exit. The existing per-error stderr line is unchanged. Two tests encoded the old hostage contract ("binary must survive failed cleanup") and move to the new one. The new test drives `uninstall -y` against a sandbox HOME with a dangling ~/.cursor/mcp.json link and asserts binary + index gone, exit non-zero, and the file named on stderr; on main it fails at ASSERT(binary_gone). (#1954) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Every config editor (JSON-like, TOML, YAML, text) opens with O_NOFOLLOW and refuses symlinks, so a link planted at ~/.cursor/mcp.json cannot point a privileged writer at another file. Users keep exactly those files as symlinks into a dotfiles checkout; the blanket refusal made install and uninstall fail on them and, before the previous commit, turned uninstall into data loss (#1954). Decision C: a symlink the INVOKING user owns, resolving to a regular file the same user owns, is followed — as an explicit opt-in of the agent-configuration writers only. cbm_install_agent_configs and the uninstall activation register the user's configuration roots (HOME, XDG_CONFIG_HOME) for the duration of their work; a link whose own directory is outside every registered root, or any link seen by another editor caller, stays refused, so repository paths never follow. One shared helper (src/cli/config_edit_path.c) runs the race-safe sequence: lstat(link) → owner == invoking uid (a root process follows root-owned links only) → realpath → the target's parent directory opened as a dirfd (O_DIRECTORY|O_NOFOLLOW; owned by the invoking user or root, not writable by others unless sticky) → openat(dirfd, base, O_NOFOLLOW) → fstat: regular file, same owner, link count 1, and the same (dev, ino) as stat(realpath). Each editor reads through that validated descriptor, stages its temp file with openat(dirfd, …, O_CREAT|O_EXCL|O_NOFOLLOW), runs its pre-publish snapshot comparison against the resolved path, and commits with renameat on the pinned dirfd, so the link itself is never replaced. Every other link is refused with an observable reason (surfaced in the uninstall diagnostics): another owner, root over a non-root link, dangling, a directory or special target, a hard-linked or swapped target, a foreign or world-writable parent. Whether the target must also stay under a root is a pending user decision behind CBM_CONFIG_EDIT_TARGET_UNDER_ROOT (default: anywhere the user owns). Windows never follows reparse points. Tests: each editor suite gains a same-owner link edited in place under the opt-in (link preserved and still pointing at the same target, temp file staged beside the target, bytes identical to the same edit on a plain file, no stray temp files) and keeps its refusal cases; the JSON-like suite adds no-opt-in, outside-root, dangling, directory, identity-skew and world-writable-parent refusals. The foreign-owner and root-run refusals move the observer through a test seam (a test cannot create a foreign-owned link without root; a real root run demotes the link first). test_cli.c drives `uninstall -y` end to end through a symlinked ~/.cursor/mcp.json. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…mlinked-config # Conflicts: # tests/test_cli.c
…ws build config_edit_path.c's edit_path_note is defined before the '#ifndef _WIN32' follow-resolver block but only called inside it, so the Windows build compiled it dead and failed under -Werror,-Wunused-function (pr-smoke windows-latest). Windows never follows a symlinked config, so guard the recorder with the same condition as its callers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cli_fd_capture_begin/end use POSIX fd redirection (dup/dup2/tmpfile) and are called only by the uninstall tests, which are themselves #ifndef _WIN32. Defined at top level, the helpers were compiled but unused on Windows, tripping -Werror,-Wunused-function on the test-windows CLANG64 leg. Wrap the struct and both helpers in #ifndef _WIN32 to match their callers. Test-only; no production change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
#1954's config-symlink follow support stored its opted-in roots in a _Thread_local char[4][4096] -- 16 KiB in the binary's static TLS block. On glibc/x86-64 that block is carved out of every thread's stack, including the parent-death watchdog thread, created with a deliberately tiny 64 KiB stack (PARENT_WATCHDOG_STACK_SIZE, main.c). The extra 16 KiB pushed that thread's pthread_create past the static-TLS + TCB + guard budget, so it failed with EINVAL: client_start_parent_watchdog() returned false and the stdio server exited before answering initialize. The parent-death watchdog regression test (#406/#407, scripts/test.sh Step 5) went red on every Linux x86-64 leg while macOS/arm64 stayed green -- its TLS is not stack-carved. Move the buffer off the static TLS block: keep a _Thread_local POINTER (8 bytes) and calloc the 16 KiB lazily on first add, freeing it in follow_clear(). Per-thread isolation and the exact API/semantics are unchanged; the watchdog thread's static-TLS budget is restored. main.c and the watchdog path are untouched. Evidence (local, Linux x86-64 gcc under QEMU + Linux arm64): - PT_TLS MemSiz 59,889 B -> 43,505 B (-16,384 B, exactly the array). - parent-watchdog test 10/10 red -> 10/10 green; red again on revert. - origin/main 10/10 green on the same env (regression is PR-specific). - config editor + cli suites: 522/522 ASan/UBSan (macOS), 523/523 ASan/LeakSanitizer (Linux arm64), no leak. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
added
bug
This was referenced Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the executable/index-removal half of #1954, and follows user-owned symlinked config files.
Unit 1 — uninstall is no longer held hostage.
cli_uninstall_activateaborted on any agent-config cleanup error before index/binary removal, so one refusedmcp.jsonleft a ~300 MB binary and the cache behind. Order is now: run agent cleanups (collecting failures) → remove indexes → remove the binary → exit non-zero listing the configs that could not be cleaned, with the honest per-file reason. RED-first test intests/test_cli.c(a dangling~/.cursor/mcp.json: binary + index gone, rc≠0, stderr names the file).Unit 2 — follow user-owned symlinked config files (decision C). The four config editors open
O_NOFOLLOW, so a stow/dotfiles~/.cursor/mcp.json -> …was refused outright. A new opt-in (cbm_config_edit_path) lets the agent-config install/uninstall paths follow a symlink only when: it is rooted under HOME/XDG_CONFIG_HOME; the link is owned by the invoking user (root only over root-owned); and the resolved target is a regular file the user owns, reached race-safely (lstat → realpath → parent dirfdO_DIRECTORY|O_NOFOLLOW, owner-checked and not other-writable unless sticky →openat O_NOFOLLOW→ fstatS_ISREG/uid/nlink==1/(dev,ino) match). Writes commit viarenameaton the dirfd so the link survives (the target is edited, never replaced). Other-owner links, dangling links, link-to-directory, world-writable parents, and every non-agent-config caller stay refused. Target may live anywhere the user owns (under-HOME restriction available behind a constant, default off).Local verification (macOS):
config_json_like config_text_edit config_yaml_edit config_toml_edit agent_clients platform220/0,cli309/0; lint-format + no-suppress clean; cppcheck on changed files clean. Not covered here (recorded follow-ups): the honest past-tenseremoved …lines and theNOT_APPLICABLEmapping (unit1954-report-lines).🤖 Generated with Claude Code