Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions scripts/memlab.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,27 @@ if [[ "$BINARY" == *.exe ]] && command -v cygpath >/dev/null 2>&1 &&
# Qualify with the domain: a bare name from coreutils `whoami` resolves
# against the machine first, so on a host whose name equals the user's the
# grant lands on an empty principal.
ME="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
ME="${USERDOMAIN}\\${ME}"
fi
# Identify the account by SID, never by name. icacls resolves a bare name
# against the machine first, so a host whose name equals the user's grants
# to an empty principal (#1532); and a USERDOMAIN-qualified name is
# UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between
# account names and security IDs was done" — which fails the grant outright
# and silently leaves the tree writable by Authenticated Users. A SID has
# neither ambiguity, and the SYSTEM/Administrators grants below already use
# this form. Name lookup remains only as a fallback where PowerShell is
# unavailable.
ME="$(powershell.exe -NoProfile -NonInteractive -Command \
'[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null |
tr -d '\r\n')"
case "${ME}" in
S-1-*) ME="*${ME}" ;;
*)
ME="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
ME="${USERDOMAIN}\\${ME}"
fi
;;
esac
MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /reset /Q >/dev/null 2>&1 || true
if ! MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /inheritance:r \
/grant:r "${ME}:(OI)(CI)F" '*S-1-5-18:(OI)(CI)F' '*S-1-5-32-544:(OI)(CI)F' \
Expand Down
25 changes: 21 additions & 4 deletions scripts/run-tests-parallel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,27 @@ stamp_windows_build_dir() {
# (COMPUTERNAME=BUILD, user build) the grant lands on an empty principal
# (BUILD\) and, combined with /inheritance:r above, locks this script out
# of its own log directory.
me="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
me="${USERDOMAIN}\\${me}"
fi
# Identify the account by SID, never by name. icacls resolves a bare name
# against the machine first, so a host whose name equals the user's grants
# to an empty principal (#1532); and a USERDOMAIN-qualified name is
# UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between
# account names and security IDs was done" — which fails the grant outright
# and silently leaves the tree writable by Authenticated Users. A SID has
# neither ambiguity, and the SYSTEM/Administrators grants below already use
# this form. Name lookup remains only as a fallback where PowerShell is
# unavailable.
me="$(powershell.exe -NoProfile -NonInteractive -Command \
'[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null |
tr -d '\r\n')"
case "${me}" in
S-1-*) me="*${me}" ;;
*)
me="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
me="${USERDOMAIN}\\${me}"
fi
;;
esac
# Normalize FIRST: some runner images stamp EXPLICIT (non-inherited)
# Authenticated-Users ACEs onto the workspace tree, which /inheritance:r
# cannot strip and /grant:r does not touch (it replaces only the granted
Expand Down
25 changes: 21 additions & 4 deletions scripts/soak-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,27 @@ soak_stamp_windows_dir() {
# Qualify with the domain: a bare name from coreutils `whoami` resolves
# against the machine first, so on a host whose name equals the user's the
# grant lands on an empty principal.
me="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
me="${USERDOMAIN}\\${me}"
fi
# Identify the account by SID, never by name. icacls resolves a bare name
# against the machine first, so a host whose name equals the user's grants
# to an empty principal (#1532); and a USERDOMAIN-qualified name is
# UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between
# account names and security IDs was done" — which fails the grant outright
# and silently leaves the tree writable by Authenticated Users. A SID has
# neither ambiguity, and the SYSTEM/Administrators grants below already use
# this form. Name lookup remains only as a fallback where PowerShell is
# unavailable.
me="$(powershell.exe -NoProfile -NonInteractive -Command \
'[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null |
tr -d '\r\n')"
case "${me}" in
S-1-*) me="*${me}" ;;
*)
me="$(whoami | tr -d '\r')"
if [ -n "${USERDOMAIN:-}" ]; then
me="${USERDOMAIN}\\${me}"
fi
;;
esac
if ! output=$(MSYS2_ARG_CONV_EXCL='*' icacls "$dir_w" /reset /Q 2>&1); then
echo "FAIL: soak DACL normalize failed (dir=$dir_w user=$me)" >&2
printf '%s\n' "$output" >&2
Expand Down
84 changes: 70 additions & 14 deletions src/cli/cli.c
Original file line number Diff line number Diff line change
Expand Up @@ -152,17 +152,31 @@ int cbm_cli_exit_status_after_maintenance(int exit_status, bool maintenance_canc
return maintenance_cancelled && exit_status == EXIT_SUCCESS ? EXIT_FAILURE : exit_status;
}

/* #1537: the refusal is correct, but "sessions could not be stopped" gives the
* reader nothing to act on — which sessions? The daemon already knows every
* client pid and `daemon status` prints them, so name that remedy here rather
* than leaving people to guess (one reporter uninstalled first and still hit
* this, with no way to see what was holding on). */
static const char CLI_ACTIVATION_REFUSED_MESSAGE[] =
/* #1537. Two very different failures reached this one message: the cohort was
* BUSY (real sessions are running — the reader can close them), or the
* reservation failed outright (lock I/O, stale coordination state, permissions
* — nothing the reader can close, because nothing is running). Reporting both
* as "active sessions" sent someone hunting processes that a reboot proved did
* not exist.
*
* The remedy line must also survive the situation it prints in: an install or
* a retry after `uninstall` has no cbm on PATH, so "run codebase-memory-mcp
* daemon status" was advice the reader could not follow at exactly the moment
* they needed it. Each message now names its own condition and stays runnable. */
static const char CLI_ACTIVATION_BUSY_MESSAGE[] =
"error: active CBM sessions and operations could not be stopped safely; "
"no activation was committed.\n"
"error: run 'codebase-memory-mcp daemon status' to list the client "
"processes still holding the daemon (MCP servers started by your editor or "
"agent are the usual holders), close them, then retry.";
"error: something is still using CBM. If an editor or agent is running an "
"MCP server, close it and retry; 'codebase-memory-mcp daemon status' lists "
"the holders when a cbm binary is still installed.";
static const char CLI_ACTIVATION_REFUSED_MESSAGE[] =
"error: activation could not reserve exclusive access; no activation was "
"committed.\n"
"error: this is NOT a running-session problem — the reservation itself "
"failed (coordination lock, leftover state, or permissions). Nothing needs "
"to be closed. Check the errors above, and report this with the output of "
"'ls -la \"${CBM_CACHE_DIR:-$HOME/.cache/codebase-memory-mcp}\"' if it "
"persists.";
static const char CLI_ACTIVATION_PARTIAL_MESSAGE[] =
"error: activation stopped after one or more agent configuration or "
"cleanup operations failed; the published/current executable was kept, "
Expand Down Expand Up @@ -211,7 +225,11 @@ static void cli_activation_diagnostic(const cbm_cli_activation_ops_t *ops, const
* stop/reservation failures, which record no refusal note. */
char attributed[CBM_SZ_1K];
const char *note = cbm_activation_transaction_refusal_note();
if (diagnostic == CLI_ACTIVATION_REFUSED_MESSAGE && note && note[0]) {
/* A recorded refusal is more specific than EITHER generic message, so it
* wins over both the busy and the reservation-failure wording. */
if ((diagnostic == CLI_ACTIVATION_REFUSED_MESSAGE ||
diagnostic == CLI_ACTIVATION_BUSY_MESSAGE) &&
note && note[0]) {
(void)snprintf(attributed, sizeof(attributed),
"error: activation was refused by a filesystem safety check before any "
"change was made: %s\n"
Expand Down Expand Up @@ -245,7 +263,12 @@ int cbm_cli_activation_guard_with_ops(const cbm_cli_activation_ops_t *ops,
if (mutation_lease) {
ops->mutation_lease_release(ops->context, mutation_lease);
}
cli_activation_diagnostic(ops, CLI_ACTIVATION_REFUSED_MESSAGE);
/* 0 means the cohort was BUSY: real sessions hold it and closing them
* is the remedy. Anything else is the reservation itself failing, and
* telling that reader to close sessions sends them after processes
* that do not exist (#1537). */
cli_activation_diagnostic(ops, reserve_status == 0 ? CLI_ACTIVATION_BUSY_MESSAGE
: CLI_ACTIVATION_REFUSED_MESSAGE);
return CLI_TRUE;
}

Expand Down Expand Up @@ -1232,12 +1255,17 @@ int cbm_replace_binary(const char *path, const unsigned char *data, int len, int
static const char skill_content[] =
"---\n"
"name: codebase-memory\n"
"description: Use the codebase knowledge graph for structural code queries. "
/* #1554: the value contains "Triggers on: " — a colon-space, which YAML
* reads as a nested-mapping indicator inside an unquoted scalar. Strict
* parsers (js-yaml's load, the frontmatter reader in `npx skills`) reject
* the whole file, so the skill silently fails to load rather than loading
* wrongly. Quoting the scalar is the fix; the text itself is unchanged. */
"description: \"Use the codebase knowledge graph for structural code queries. "
"Triggers on: explore the codebase, understand the architecture, what functions exist, "
"show me the structure, who calls this function, what does X call, trace the call chain, "
"find callers of, show dependencies, impact analysis, dead code, unused functions, "
"high fan-out, refactor candidates, code quality audit, graph query syntax, "
"Cypher query examples, edge types, how to use search_graph.\n"
"Cypher query examples, edge types, how to use search_graph.\"\n"
"---\n"
"\n"
"# Codebase Memory — Knowledge Graph Tools\n"
Expand Down Expand Up @@ -3250,8 +3278,22 @@ int cbm_upsert_codex_mcp(const char *binary_path, const char *config_path) {
return CLI_ERR;
}
char block[CLI_BUF_8K];
/* #1562: Codex sanitizes the environment of stdio MCP subprocesses, passing
* through only the names listed in env_vars. Without CBM_CACHE_DIR the
* Codex-spawned server silently falls back to the DEFAULT cache while the
* account daemon uses the configured one; the two disagree and the
* handshake closes during initialization, so Codex exposes no cbm tools at
* all.
*
* The name is listed unconditionally rather than only when the variable is
* set at install time: env_vars names variables to FORWARD IF PRESENT, so
* listing it costs nothing when unset and keeps working for someone who
* sets it after installing — which install-time detection would silently
* fail to cover. */
int written = snprintf(block, sizeof(block),
CODEX_CMM_SECTION "\ncommand = \"%s\"\nargs = []\n", escaped);
CODEX_CMM_SECTION "\ncommand = \"%s\"\nargs = []\n"
"env_vars = [\"CBM_CACHE_DIR\"]\n",
escaped);
if (written < 0 || (size_t)written >= sizeof(block) ||
cbm_remove_codex_legacy_mcp(config_path) != 0) {
return CLI_ERR;
Expand Down Expand Up @@ -11482,17 +11524,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.
Expand Down
12 changes: 10 additions & 2 deletions src/cli/client_adapter.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,16 @@ 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 via
* jiti.import(path, { default: true }) and rejects anything that is not a
* function, so a named `export function register(pi)` hands the loader the
* module namespace object instead and every install fails with "Extension
* does not export a valid factory function". A Pi extension that fails to
* load takes EVERY pi command down with it — `pi doctor` included — not
* just cbm's tools (#1550). */
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]) {
Expand Down
33 changes: 31 additions & 2 deletions src/daemon/ipc.c
Original file line number Diff line number Diff line change
Expand Up @@ -1383,8 +1383,26 @@ static bool posix_directory_parent_secure(int directory_fd) {
!cbm_macos_extended_acl_fd_is_deny_only(directory_fd)) {
return false;
}
return (status.st_mode & 0022) == 0 ||
(status.st_uid == (uid_t)0 && (status.st_mode & S_ISVTX) != 0);
/* #1537: this ANCESTOR check refused any group-write bit, which is the same
* rule #1535 removed on the activation side — and the sibling that decision
* covers but that never got changed. A group-writable ~ or ~/.cache is
* ordinary (WSL2 ships 0775, so do several distro skeletons and any site
* with a shared primary group), and refusing it here made the daemon
* unusable with no way for the reader to see why.
*
* WORLD-writable is still refused: any local user could swap a path
* component. Group-writable is admitted for ancestors only — the private
* directory itself is chmod'd to 0700 and verified after this walk, so the
* thing that actually holds data stays owner-private either way. */
if ((status.st_mode & 0002) != 0) {
return status.st_uid == (uid_t)0 && (status.st_mode & S_ISVTX) != 0;
}
if ((status.st_mode & 0020) != 0) {
char mode_text[16];
(void)snprintf(mode_text, sizeof(mode_text), "%04o", (unsigned)(status.st_mode & 07777));
cbm_log_warn("daemon.private_dir_group_writable_ancestor", "mode", mode_text);
}
return true;
}

/* Validate a path transition only through the two already-open directory
Expand Down Expand Up @@ -1435,6 +1453,17 @@ static int private_directory_tree_open(const char *directory_path) {
ok = false;
} else {
ok = posix_directory_parent_secure(current_fd);
if (!ok) {
/* #1537: this branch used to leave the detail empty, so the
* caller fell back to printing errno — which NOTHING here sets.
* A reporter was handed "errno 2" (ENOENT) for a permission
* refusal and went looking for a missing file that existed.
* An unset errno is not a diagnosis; name the component. */
ipc_validation_detail_set("%s: ancestor '%s' is not a usable private-directory "
"parent (must be owned by you, not world-writable, and "
"carry no allow-ACL)",
directory_path, component);
}
bool created = ok && mkdirat(current_fd, component, 0700) == 0;
if (!created && errno != EEXIST) {
ok = false;
Expand Down
Loading
Loading