From 7658954140b98c1c3e56e9429c3613dccc3afe04 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 26 Sep 2026 02:26:33 +0200 Subject: [PATCH 1/7] fix(artifact): write and detect artifacts through wide paths on Windows (#1171) With persistence=true, indexing a repository under a CJK path on Windows made the worker exit nonzero and no .codebase-memory/graph.db.zst was written, while an ASCII junction to the same directory worked. Root cause: write_file_atomic() -- used for artifact.json, graph.db.zst and the import temp db -- opened its temp file with the raw CRT fopen() and published it with MoveFileExA(). Both go through the ANSI code page, so a repo path containing characters outside it cannot be opened. The same file had two more ANSI calls on the same flow: cbm_artifact_exists() used stat() on the repo path, and the import published the cache db with CRT rename(), which is ANSI and also fails when the destination exists. Fix: use the repo's UTF-8 helpers -- cbm_fopen() for the temp file, cbm_rename_replace() (MoveFileExW, write-through, replace-existing) for both publishes, and a cbm_fopen() probe for the non-empty existence check. rename_temp now reports errno on every platform (cbm_rename_replace translates the Win32 error). Test: artifact::artifact_roundtrip_non_ascii_paths exports from a CJK repo directory, checks cbm_artifact_exists(), and imports twice into a CJK cache directory (the second import replaces the existing db). Signed-off-by: Martin Vogel --- src/pipeline/artifact.c | 32 ++++++++++++++++--------------- tests/test_artifact.c | 42 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/pipeline/artifact.c b/src/pipeline/artifact.c index 2dd368c28..9594c8a09 100644 --- a/src/pipeline/artifact.c +++ b/src/pipeline/artifact.c @@ -185,7 +185,10 @@ static int write_file_atomic(const char *path, const char *data, size_t len, return CBM_NOT_FOUND; } - FILE *fp = fopen(tmp, "wb"); + /* cbm_fopen / cbm_rename_replace: wide paths on Windows. The ANSI CRT fopen + * and MoveFileExA mangled a repo path outside the active code page (a CJK + * checkout), so every persistent export failed there (#1171). */ + FILE *fp = cbm_fopen(tmp, "wb"); if (!fp) { file_error_set(out_err, "open_temp", errno); return CBM_NOT_FOUND; @@ -207,22 +210,14 @@ static int write_file_atomic(const char *path, const char *data, size_t len, return CBM_NOT_FOUND; } -#ifdef _WIN32 - /* MoveFileEx replace approach suggested by @Ayush7Ranjan in #492. */ - if (!MoveFileExA(tmp, path, MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH)) { - DWORD saved_error = GetLastError(); - cbm_unlink(tmp); - file_error_set(out_err, "rename_temp", (int)saved_error); - return CBM_NOT_FOUND; - } -#else - if (rename(tmp, path) != 0) { + /* Replace-existing rename (MoveFileExW write-through on Windows, the + * approach suggested by @Ayush7Ranjan in #492); errno carries the cause. */ + if (cbm_rename_replace(tmp, path) != 0) { int saved_errno = errno; cbm_unlink(tmp); file_error_set(out_err, "rename_temp", saved_errno); return CBM_NOT_FOUND; } -#endif return 0; } @@ -1188,7 +1183,7 @@ int cbm_artifact_import(const char *repo_path, const char *cache_db_path) { * stale WAL next to the cache path would be replayed on top of the * imported file at the next open (#897). */ cbm_remove_db_sidecars(cache_db_path); - if (rename(tmp_path, cache_db_path) != 0) { + if (cbm_rename_replace(tmp_path, cache_db_path) != 0) { cbm_log_error("artifact.import", "err", "rename_to_cache"); cbm_unlink(tmp_path); return CBM_NOT_FOUND; @@ -1218,8 +1213,15 @@ bool cbm_artifact_exists(const char *repo_path) { char zst_path[CBM_SZ_4K]; artifact_path(zst_path, sizeof(zst_path), repo_path, CBM_ARTIFACT_FILENAME); - struct stat st; - if (stat(zst_path, &st) != 0 || st.st_size == 0) { + /* Present and non-empty. cbm_fopen, not stat(): the ANSI CRT stat misses a + * repo path outside the active Windows code page (#1171). */ + FILE *fp = cbm_fopen(zst_path, "rb"); + if (!fp) { + return false; + } + int first = fgetc(fp); + (void)fclose(fp); + if (first == EOF) { return false; } diff --git a/tests/test_artifact.c b/tests/test_artifact.c index a7d96cdd9..6206463e3 100644 --- a/tests/test_artifact.c +++ b/tests/test_artifact.c @@ -334,6 +334,47 @@ TEST(artifact_gitattributes_created) { PASS(); } +/* #1171: a repository (and a cache directory) under a non-ASCII path must + * export, report the artifact present, and import — also onto an existing + * cache db. On Windows the atomic writer used the ANSI CRT fopen + MoveFileExA + * and the existence check the ANSI stat, so CJK characters outside the active + * code page made every step fail. The bytes are escaped UTF-8 for a CJK repo + * directory name and a CJK cache directory name (kept ASCII in source). */ +#define ART_CJK_REPO "\xe6\x8c\x81\xe4\xb9\x85\xe5\x8c\x96\xe5\xa4\x8d\xe7\x8e\xb0" +#define ART_CJK_CACHE "\xe5\xaf\xbc\xe5\x85\xa5" +TEST(artifact_roundtrip_non_ascii_paths) { + setup_artifact_test(); + create_test_db(g_db); + + char repo[1024]; + snprintf(repo, sizeof(repo), "%s/cbm-" ART_CJK_REPO, g_tmpdir); + ASSERT_TRUE(cbm_mkdir_p(repo, 0755)); + + ASSERT_EQ(cbm_artifact_export(g_db, repo, "test-proj", CBM_ARTIFACT_FAST), 0); + char zst[1024]; + snprintf(zst, sizeof(zst), "%s/.codebase-memory/graph.db.zst", repo); + ASSERT_TRUE(cbm_file_exists(zst)); + ASSERT_TRUE(cbm_artifact_exists(repo)); + + char cache_dir[1024]; + snprintf(cache_dir, sizeof(cache_dir), "%s/" ART_CJK_CACHE, g_tmpdir); + ASSERT_TRUE(cbm_mkdir_p(cache_dir, 0755)); + char import_db[1024]; + snprintf(import_db, sizeof(import_db), "%s/imported.db", cache_dir); + ASSERT_EQ(cbm_artifact_import(repo, import_db), 0); + /* Re-import replaces the existing cache db (the second-session path). */ + ASSERT_EQ(cbm_artifact_import(repo, import_db), 0); + + cbm_store_t *s = cbm_store_open_path(import_db); + ASSERT_NOT_NULL(s); + ASSERT_EQ(cbm_store_count_nodes(s, "test-proj"), 2); + ASSERT_EQ(cbm_store_count_edges(s, "test-proj"), 1); + cbm_store_close(s); + + cleanup_dir(g_tmpdir); + PASS(); +} + TEST(artifact_export_rename_failure_logs_specific_error) { setup_artifact_test(); create_test_db(g_db); @@ -1102,6 +1143,7 @@ SUITE(artifact) { RUN_TEST(artifact_schema_version_mismatch); RUN_TEST(artifact_import_missing); RUN_TEST(artifact_gitattributes_created); + RUN_TEST(artifact_roundtrip_non_ascii_paths); RUN_TEST(artifact_export_rename_failure_logs_specific_error); RUN_TEST(pipeline_persistence_export_failure_returns_error); RUN_TEST(artifact_import_rejects_size_mismatch); From 431778c0ae420a3eedb9a795d96f77959d569029 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 17:33:05 +0200 Subject: [PATCH 2/7] fix(extract): a decorator with arguments but no path is not a route Distilled from #1245 by Andrew Hundt (a slice of d15071c9), with a different rule than upstream chose. Python's unittest.mock is used as a decorator: @patch("subprocess.run") def test_runs(self, run): ... decorator_method_name maps a bare `patch` to the HTTP method PATCH, and try_route_from_decorator_call accepts a receiver-less decorator call. When extract_route_path_from_args then finds nothing path-shaped, the function falls back to "/". So every mock-decorated test function became a Route handler, PATCH "/", and every one of those was wrong. Upstream fixed it with a has_receiver heuristic. That still admits @mock.patch("os.getcwd") (it has a receiver) and would reject the bare framework decorators Litestar and BlackSheep use, @get("/x"). The rule here is about the arguments, not the callee: a decorator call that HAS arguments but none of them path-shaped is not a route. The "/" default survives only for a zero-argument call such as @app.route(), which is the one case where "/" is what the framework means. No test on main relied on the default for a call with non-path arguments; checked before changing it. RED before the fix: extract_python_mock_patch_is_not_route FAIL tests/test_extraction.c:4023: mocked->route_path is not NULL extract_python_bare_decorator_route_rules FAIL tests/test_extraction.c:4063: mocked->route_path is not NULL The second test also pins the two shapes that must keep working: bare @get("/health") still yields a GET route, and zero-argument @app.route() still yields "/" ANY -- those assertions passed before the fix and after. GREEN after: extraction 351 passed; extraction pipeline edge_types_probe route_canon cross_repo infrascan lang_contract 754 passed, with the CALLS-breadth contract at 54 languages / 0 failures. The single remaining red in both runs is extract_spill_round_trip_keeps_every_field refusing to spill onto a disk below its 10 GB floor -- this host, not this change. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- internal/cbm/extract_defs.c | 9 ++++- tests/test_extraction.c | 73 +++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 49543e59a..3fb279a11 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -1605,7 +1605,11 @@ static bool try_drf_action_decorator(CBMArena *a, TSNode dchild, const char *sou } // Try to extract a route from a single decorator call node. -// Returns true if a route method was found (even with fallback path "/"). +// Returns true if a route method was found. The fallback path "/" applies only +// to a zero-argument call (`@app.route()`): a call that HAS arguments but none +// path-shaped is not a route -- unittest.mock's `@patch("subprocess.run")` +// shares its name with the HTTP verb and used to mint a PATCH "/" handler for +// every mocked test function (distilled from PR #1245). static bool try_route_from_decorator_call(CBMArena *a, TSNode dchild, const char *source, const char **out_path, const char **out_method) { TSNode fn = ts_node_child_by_field_name(dchild, TS_FIELD("function")); @@ -1630,6 +1634,9 @@ static bool try_route_from_decorator_call(CBMArena *a, TSNode dchild, const char *out_method = method; return true; } + if (ts_node_named_child_count(args) > 0) { + return false; + } } *out_path = "/"; *out_method = method; diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 50a0db788..6ded6d282 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -4303,6 +4303,77 @@ TEST(extract_java_method_annotations_issue382) { PASS(); } +/* Distilled from PR #1245 (Andrew Hundt): unittest.mock's `@patch("x")` is a + * bare decorator whose name collides with the HTTP verb, and its only argument + * is a dotted target, not a path. The "/" fallback then minted a Route handler + * PATCH "/" for every mocked test function. Rule: a decorator call WITH + * arguments but no path-shaped one is not a route. */ +TEST(extract_python_mock_patch_is_not_route) { + CBMFileResult *r = extract("from unittest.mock import patch\n\n" + "@patch(\"subprocess.run\")\n" + "def test_cmd(mock_run):\n" + " pass\n\n" + "@app.patch(\"/items/{id}\")\n" + "def update_item():\n" + " pass\n", + CBM_LANG_PYTHON, "t", "test_routes.py"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + + const CBMDefinition *mocked = find_def_by_name(r, "test_cmd"); + ASSERT_NOT_NULL(mocked); + ASSERT_NULL(mocked->route_path); + ASSERT_NULL(mocked->route_method); + + const CBMDefinition *route = find_def_by_name(r, "update_item"); + ASSERT_NOT_NULL(route); + ASSERT_STR_EQ(route->route_path, "/items/{id}"); + ASSERT_STR_EQ(route->route_method, "PATCH"); + + cbm_free_result(r); + PASS(); +} + +/* Companion to the mock-patch case: the narrowing must not cost real routes. + * A receiver-less framework decorator with a path-shaped argument (Litestar / + * BlackSheep `@get("/x")`) stays a route, `@mock.patch("x")` (receiver, but + * no path) is not one, and a zero-argument `@app.route()` keeps the "/" + * default main emits today. */ +TEST(extract_python_bare_decorator_route_rules) { + CBMFileResult *r = extract("from litestar import get\n" + "from unittest import mock\n\n" + "@get(\"/health\")\n" + "def health():\n" + " pass\n\n" + "@mock.patch(\"os.getcwd\")\n" + "def test_cwd(mock_cwd):\n" + " pass\n\n" + "@app.route()\n" + "def index():\n" + " pass\n", + CBM_LANG_PYTHON, "t", "routes.py"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + + const CBMDefinition *health = find_def_by_name(r, "health"); + ASSERT_NOT_NULL(health); + ASSERT_STR_EQ(health->route_path, "/health"); + ASSERT_STR_EQ(health->route_method, "GET"); + + const CBMDefinition *mocked = find_def_by_name(r, "test_cwd"); + ASSERT_NOT_NULL(mocked); + ASSERT_NULL(mocked->route_path); + ASSERT_NULL(mocked->route_method); + + const CBMDefinition *index = find_def_by_name(r, "index"); + ASSERT_NOT_NULL(index); + ASSERT_STR_EQ(index->route_path, "/"); + ASSERT_STR_EQ(index->route_method, "ANY"); + + cbm_free_result(r); + PASS(); +} + /* ── ArkTS (HarmonyOS .ets) ─────────────────────────────────────── */ TEST(arkts_component_struct) { @@ -8813,6 +8884,8 @@ SUITE(extraction) { RUN_TEST(js_index_module_qn_not_collide_with_folder); RUN_TEST(python_regular_module_qn_unchanged); RUN_TEST(extract_java_method_annotations_issue382); + RUN_TEST(extract_python_mock_patch_is_not_route); + RUN_TEST(extract_python_bare_decorator_route_rules); RUN_TEST(arkts_component_struct); RUN_TEST(arkts_exported_struct_decorators); RUN_TEST(arkts_member_decorators); From 6eb33534f68b29f36c012b18a0236b1bd6b300c4 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 25 Sep 2026 18:37:16 +0200 Subject: [PATCH 3/7] fix(daemon): explain foreign-owned cache ancestors and warn on network filesystems (#1717) A cache root whose resolved path runs through a directory owned by another non-root account (typical on shared/HPC filesystems where ~/.cache is a symlink into a shared tree) is refused by the private-directory walk. The refusal itself is correct: the owner of an ancestor can rename or replace everything below it after the walk, so it could redirect the cache. But posix_directory_transition_secure() set no validation detail, so the caller printed "ancestry component validation failed (errno 17)" -- a stale EEXIST from mkdirat() that named neither the directory nor the reason. - The refusal now names the directory, its owner uid and account name, the caller's uid, and the remedy (a location with a self/root-owned ancestry, e.g. via CBM_CACHE_DIR). The path is shortened from the left so the remedy always fits the 384-byte detail buffer. - On Linux the account name is read from /etc/passwd, not getpwuid(). The release binary is statically linked against glibc, and getpwuid() there loads the host's NSS modules at runtime, which can crash on a glibc version mismatch (a real risk on sssd/LDAP cluster hosts). Accounts that exist only in a directory service are shown by uid. - New advice-only check: on Linux, a cache root on NFS, CIFS, SMB2, Lustre or GPFS (statfs f_type) logs one daemon.cache_root_network_fs warning per process. SQLite WAL is unreliable on network filesystems. Nothing is refused. - docs/CONFIGURATION.md gains a "Shared / HPC filesystems" section. The ownership policy is unchanged, and there is still no override (same stance as #1830). Test seams: one seam reports a directory (matched by dev/ino) as foreign-owned, and another supplies the statfs magic. The refusal, the message and the once-per-process warning are therefore testable without root and on every platform. Refs #1717 Signed-off-by: Martin Vogel --- docs/CONFIGURATION.md | 27 ++++++ src/daemon/ipc.c | 209 +++++++++++++++++++++++++++++++++++++++- src/daemon/ipc.h | 17 ++++ src/main.c | 2 + tests/test_daemon_ipc.c | 207 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+), 4 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 024f37e93..aa17e5005 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -235,6 +235,33 @@ be indexed and later returned. And the credential list is a denylist, so it raises the cost of a mistake rather than closing the class — a directory it does not name is permitted. +### Shared / HPC filesystems + +On clusters and other shared machines, home directories often live on a network +filesystem (NFS, Lustre, GPFS, SMB/CIFS), and `~/.cache` is sometimes a symlink +into a shared tree. Three rules keep CBM working there: + +- **Put the cache and the rendezvous on local storage.** Set `CBM_CACHE_DIR` (indexes, + `_config.db`) and, if needed, `CBM_RUNTIME_DIR` (the daemon rendezvous) to a + local disk you own, for example `/local/scratch/$USER/cbm` or `/tmp/$USER-cbm`. + Set the same values in your MCP client's environment and your shell. +- **Do not route the cache into another account's tree.** A symlinked `~/.cache` is + resolved and allowed, but every directory on the *resolved* path must be owned + by you or by root. If a parent such as `/shared/fs` belongs to another account, + that account can rename or replace anything below it, so CBM refuses the path + and names the directory and its owner: + + ```text + ... (cache-private) - '/shared/fs' is owned by uid 1234 (alice), not by you (uid 1000) or root; ... + ``` + + This check has no override. Choose a location with a safe ancestry instead. +- **Avoid network filesystems for the cache.** The indexes are SQLite databases in + WAL mode, which needs shared-memory locking that network filesystems do not + provide reliably. On Linux, CBM logs one `daemon.cache_root_network_fs` warning + at startup when the cache root is on NFS, Lustre, GPFS, CIFS or SMB2. The cache + still works there, but it can be slow, and a database can be corrupted. + ## 5. Agent and Editor Integration Files The `install` command can also write MCP entries and instruction blocks into agent/editor config files such as Claude Code, Codex, Gemini, VS Code, Cursor, Zed, and others. diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index eb87e43be..0ea819542 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -21,6 +21,9 @@ #include #include #include +#if defined(__linux__) +#include +#endif enum { CBM_DAEMON_IPC_SEND_TIMEOUT_MS = 5000, @@ -54,6 +57,76 @@ const char *cbm_daemon_ipc_validation_detail(void) { return ipc_validation_detail_buffer; } +/* #1717: a cache root on a network filesystem (a shared HPC home, an NFS or + * SMB mount) is ACCEPTED -- this is advice, never a refusal. SQLite's WAL mode + * relies on shared-memory locking that network filesystems do not provide + * reliably, so indexes there can be slow or corrupt. Detection is statfs + * f_type, Linux only; other platforms never warn. */ +static const char *ipc_network_fs_name(unsigned long magic) { + static const struct { + unsigned long magic; + const char *name; + } kinds[] = { + {0x6969UL, "NFS"}, {0xFF534D42UL, "CIFS"}, {0xFE534D42UL, "SMB2"}, + {0x0BD00BD0UL, "Lustre"}, {0x47504653UL, "GPFS"}, + }; + for (size_t index = 0; index < sizeof(kinds) / sizeof(kinds[0]); index++) { + if (kinds[index].magic == magic) { + return kinds[index].name; + } + } + return NULL; +} + +static atomic_bool g_ipc_network_cache_warned; +#ifdef CBM_ENABLE_TEST_SEAMS +static bool g_ipc_fs_magic_override_active; +static unsigned long g_ipc_fs_magic_override; +void cbm_daemon_ipc_set_fs_magic_for_test(bool active, unsigned long magic) { + g_ipc_fs_magic_override_active = active; + g_ipc_fs_magic_override = magic; + atomic_store(&g_ipc_network_cache_warned, false); +} +#endif + +static bool ipc_path_fs_magic(const char *path, unsigned long *magic_out) { +#ifdef CBM_ENABLE_TEST_SEAMS + if (g_ipc_fs_magic_override_active) { + *magic_out = g_ipc_fs_magic_override; + return true; + } +#endif +#if defined(__linux__) + struct statfs status; + if (statfs(path, &status) != 0) { + return false; + } + /* f_type is a signed word on some ABIs; the magics are 32-bit. */ + *magic_out = (unsigned long)status.f_type & 0xFFFFFFFFUL; + return true; +#else + (void)path; + (void)magic_out; + return false; +#endif +} + +bool cbm_daemon_ipc_warn_if_network_cache_root(const char *cache_root) { + unsigned long magic = 0; + const char *kind = NULL; + if (!cache_root || !cache_root[0] || !ipc_path_fs_magic(cache_root, &magic) || + !(kind = ipc_network_fs_name(magic))) { + return false; + } + if (atomic_exchange(&g_ipc_network_cache_warned, true)) { + return false; /* one warning per process */ + } + cbm_log_warn("daemon.cache_root_network_fs", "path", cache_root, "fs", kind, "reason", + "sqlite_wal_unreliable_on_network_fs", "hint", + "set CBM_CACHE_DIR to local storage (docs/CONFIGURATION.md)"); + return true; +} + static cbm_daemon_ipc_listen_failure_t ipc_listen_failure; /* The failure recorder and its reset are only reached from the POSIX socket @@ -401,6 +474,9 @@ int cbm_daemon_ipc_wait_pending(const cbm_ipc_pending_ops_t *ops, uint32_t timeo #include #include #include +#if !defined(__linux__) +#include +#endif #include #include #include @@ -1788,14 +1864,138 @@ static bool posix_directory_parent_secure(int directory_fd) { * never chmod'd or ACL-rewritten. The euid-only enforcement that stops a * squatted private directory lives in the created/final/snapshot checks, not * here — this only walks ancestors. */ -static bool posix_directory_transition_secure(int parent_fd, int child_fd) { +#ifdef CBM_ENABLE_TEST_SEAMS +/* #1717 seam: report one directory (by dev/ino) as owned by a foreign uid, so + * the foreign-owned-ancestor refusal and its diagnostic are testable without + * root (an unprivileged test cannot chown a directory to another account). */ +static bool g_posix_foreign_owner_active; +static dev_t g_posix_foreign_owner_dev; +static ino_t g_posix_foreign_owner_ino; +static uid_t g_posix_foreign_owner_uid; +bool cbm_daemon_ipc_posix_set_foreign_owned_dir_for_test(const char *directory_path, + unsigned long foreign_uid) { + struct stat status; + g_posix_foreign_owner_active = false; + if (!directory_path || lstat(directory_path, &status) != 0 || !S_ISDIR(status.st_mode)) { + return false; + } + g_posix_foreign_owner_dev = status.st_dev; + g_posix_foreign_owner_ino = status.st_ino; + g_posix_foreign_owner_uid = (uid_t)foreign_uid; + g_posix_foreign_owner_active = true; + return true; +} +void cbm_daemon_ipc_posix_clear_foreign_owned_dir_for_test(void) { + g_posix_foreign_owner_active = false; +} +#endif + +static uid_t posix_directory_observed_owner(const struct stat *status) { +#ifdef CBM_ENABLE_TEST_SEAMS + if (g_posix_foreign_owner_active && status->st_dev == g_posix_foreign_owner_dev && + status->st_ino == g_posix_foreign_owner_ino) { + return g_posix_foreign_owner_uid; + } +#endif + return status->st_uid; +} + +enum { POSIX_OWNER_NAME_CAP = 33, POSIX_PASSWD_LINE_CAP = 1024 }; + +/* The account name for uid, or "" when it cannot be named. Diagnostic only. + * + * Linux reads /etc/passwd directly instead of calling getpwuid(): the Linux + * release binary is statically linked against glibc, where getpwuid() loads the + * HOST's NSS modules at runtime -- a version-mismatch crash hazard exactly on + * the sssd/LDAP hosts (#1717's shared HPC filesystems) this message serves. + * A directory-service account is then shown by uid only. */ +static void posix_owner_name(uid_t uid, char out[POSIX_OWNER_NAME_CAP]) { + out[0] = '\0'; +#if defined(__linux__) + FILE *passwd = cbm_fopen("/etc/passwd", "r"); + if (!passwd) { + return; + } + char line[POSIX_PASSWD_LINE_CAP]; + while (fgets(line, sizeof(line), passwd)) { + char *name_end = strchr(line, ':'); + char *password_end = name_end ? strchr(name_end + 1, ':') : NULL; + if (!password_end || name_end == line) { + continue; + } + char *uid_end = NULL; + errno = 0; + unsigned long line_uid = strtoul(password_end + 1, &uid_end, 10); + if (errno == 0 && uid_end != password_end + 1 && *uid_end == ':' && + line_uid == (unsigned long)uid) { + (void)snprintf(out, POSIX_OWNER_NAME_CAP, "%.*s", (int)(name_end - line), line); + break; + } + } + (void)fclose(passwd); +#else + struct passwd entry; + struct passwd *found = NULL; + char storage[POSIX_PASSWD_LINE_CAP]; + if (getpwuid_r(uid, &entry, storage, sizeof(storage), &found) == 0 && found && found->pw_name) { + (void)snprintf(out, POSIX_OWNER_NAME_CAP, "%s", found->pw_name); + } +#endif +} + +/* The foreign-owned-ancestor refusal. The remedy is the part the reader acts + * on, so it must survive the fixed detail buffer: the path is shortened from + * the LEFT ("...") to fit, which keeps the refusing component at its end. */ +static void posix_foreign_owner_detail_set(const char *child_path, uid_t owner) { + char name[POSIX_OWNER_NAME_CAP]; + posix_owner_name(owner, name); + char owner_text[64]; + if (name[0]) { + (void)snprintf(owner_text, sizeof(owner_text), "uid %ld (%s)", (long)owner, name); + } else { + (void)snprintf(owner_text, sizeof(owner_text), "uid %ld", (long)owner); + } + char tail[sizeof(ipc_validation_detail_buffer)]; + int tail_length = snprintf(tail, sizeof(tail), + "' is owned by %s, not by you (uid %ld) or root; that account " + "controls this path. Use a location whose parent directories are " + "yours or root's (e.g. set CBM_CACHE_DIR)", + owner_text, (long)geteuid()); + if (tail_length < 0) { + return; + } + size_t used = (size_t)tail_length + 1; /* the opening quote */ + size_t room = used < sizeof(ipc_validation_detail_buffer) + ? sizeof(ipc_validation_detail_buffer) - 1 - used + : 0; + size_t path_length = strlen(child_path); + const char *ellipsis = ""; + const char *shown = child_path; + if (path_length > room) { + ellipsis = "..."; + shown = child_path + path_length - (room > 3 ? room - 3 : 0); + } + ipc_validation_detail_set("'%s%s%s", ellipsis, shown, tail); +} + +static bool posix_directory_transition_secure(int parent_fd, int child_fd, const char *child_path) { struct stat parent; struct stat child; if (parent_fd < 0 || child_fd < 0 || !posix_directory_parent_secure(parent_fd) || fstat(parent_fd, &parent) != 0 || fstat(child_fd, &child) != 0 || !S_ISDIR(parent.st_mode) || !S_ISDIR(child.st_mode) || - !posix_directory_ancestor_owner_trusted(parent.st_uid) || - !posix_directory_ancestor_owner_trusted(child.st_uid)) { + !posix_directory_ancestor_owner_trusted(posix_directory_observed_owner(&parent))) { + return false; + } + uid_t child_owner = posix_directory_observed_owner(&child); + if (!posix_directory_ancestor_owner_trusted(child_owner)) { + /* #1717: this refusal used to set no detail, so the caller printed + * "ancestry component validation failed (errno 17)" -- a stale EEXIST + * from the mkdirat() on the existing component, naming neither the + * directory nor the reason. The policy is unchanged: an account that + * owns an ancestor can rename or replace everything below it after + * this walk, so it could redirect the private directory. Name it. */ + posix_foreign_owner_detail_set(child_path, child_owner); return false; } return true; @@ -1870,7 +2070,8 @@ static int private_directory_tree_open(const char *directory_path) { : -1; struct stat status; ok = next_fd >= 0 && fd_set_cloexec(next_fd) && fstat(next_fd, &status) == 0 && - S_ISDIR(status.st_mode) && posix_directory_transition_secure(current_fd, next_fd); + S_ISDIR(status.st_mode) && + posix_directory_transition_secure(current_fd, next_fd, path); if (ok && created) { ok = status.st_uid == geteuid() && fchmod(next_fd, 0700) == 0 && cbm_macos_extended_acl_fd_clear(next_fd) && diff --git a/src/daemon/ipc.h b/src/daemon/ipc.h index effe6a852..f8195f3d5 100644 --- a/src/daemon/ipc.h +++ b/src/daemon/ipc.h @@ -92,6 +92,17 @@ bool cbm_daemon_ipc_private_directory_secure(const char *directory_path); * append it to their error messages; policy decisions never read it. */ const char *cbm_daemon_ipc_validation_detail(void); +/* #1717: advice, never a refusal. When cache_root lives on a network + * filesystem (Linux statfs: NFS, CIFS, SMB2, Lustre, GPFS), log one + * `daemon.cache_root_network_fs` warning per process pointing at local + * storage. Returns true only when this call emitted that warning. */ +bool cbm_daemon_ipc_warn_if_network_cache_root(const char *cache_root); +#ifdef CBM_ENABLE_TEST_SEAMS +/* Report `magic` as the cache root's statfs f_type on every platform (active), + * or restore real detection; either way re-arm the once-per-process warning. */ +void cbm_daemon_ipc_set_fs_magic_for_test(bool active, unsigned long magic); +#endif + /* This process's most recent listener-publication failure: the stage that * refused, the errno the failing step reported (0 when the step reported * none), and the artifact path it was operating on ("" when no path applies). @@ -127,6 +138,12 @@ bool cbm_daemon_ipc_win_sid_trusted_for_testing(void *sid); * so they can be tested without a chown-able overflow-owned directory. */ void cbm_daemon_ipc_posix_set_ancestor_overflow_uid_for_test(bool active, unsigned long overflow_uid); +/* #1717 seam (POSIX): report the directory at directory_path (matched by + * dev/ino) as owned by foreign_uid during the ancestor walk. False when the + * path is not a directory. Clear restores real ownership. */ +bool cbm_daemon_ipc_posix_set_foreign_owned_dir_for_test(const char *directory_path, + unsigned long foreign_uid); +void cbm_daemon_ipc_posix_clear_foreign_owned_dir_for_test(void); bool cbm_daemon_ipc_posix_uid_map_is_single_uid_for_test(const char *uid_map, unsigned long euid); bool cbm_daemon_ipc_posix_ancestor_stat_ok_for_test(unsigned long owner, unsigned int mode, unsigned long euid, bool overflow_active, diff --git a/src/main.c b/src/main.c index fd3370c04..3f179fffe 100644 --- a/src/main.c +++ b/src/main.c @@ -1455,6 +1455,8 @@ static main_build_identity_status_t main_build_identity(cbm_daemon_build_identit if (!cbm_daemon_ipc_private_directory_secure(canonical_cache)) { return MAIN_BUILD_IDENTITY_CACHE_PRIVATE; } + /* #1717: advice only -- a network-filesystem cache root is accepted. */ + (void)cbm_daemon_ipc_warn_if_network_cache_root(canonical_cache); /* Every cache consumer in this process must use the exact path whose * fingerprint joins the account-wide cohort. Keeping an original symlink * spelling in the environment would let a later retarget move storage diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index 97257506c..35ad5433c 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -49,6 +49,7 @@ #include #include #include +#include #include #ifdef __linux__ #include /* #1830 userns real smoke: unshare(CLONE_NEWUSER). */ @@ -5274,6 +5275,147 @@ TEST(daemon_ipc_posix_wsl_drvfs_refusal_names_remedy_issue1687) { PASS(); } +/* #1717: an ancestor owned by ANOTHER non-root account (a shared filesystem + * whose parent directory belongs to a colleague) is refused -- that account can + * rename or replace everything below it, so the policy stays. What was wrong is + * the message: the refusal set no detail, and the caller printed + * "ancestry component validation failed (errno 17)", a stale EEXIST from + * mkdirat() that named neither the directory nor the reason. The reporter + * could not tell which directory, or why. The refusal must name the refusing + * directory, its owner uid, and the remedy. The seam reports one directory as + * foreign-owned because an unprivileged test cannot chown to another account. */ +enum { IPC_TEST_FOREIGN_MAX_DEPTH = 16 }; + +typedef struct { + bool paths_ok; + bool seam_set; + bool refused; + bool cache_created; + bool secured_without_seam; + size_t foreign_path_length; + char detail[512]; +} ipc_test_foreign_result_t; + +/* Build //shared-fs, mark shared-fs as owned by + * foreign_uid through the seam, and ask for <...>/shared-fs/cache. Deeper + * chains produce a path long enough that the detail must be shortened. */ +static void ipc_test_foreign_ancestor_run(const char *tag, int depth, unsigned long foreign_uid, + ipc_test_foreign_result_t *result) { + char parent[TEST_PATH_CAP]; + char chain[IPC_TEST_FOREIGN_MAX_DEPTH + 1][TEST_PATH_CAP]; + char cache[TEST_PATH_CAP]; + int made = 0; + memset(result, 0, sizeof(*result)); + result->paths_ok = depth <= IPC_TEST_FOREIGN_MAX_DEPTH && ipc_test_parent_new(parent, tag); + const char *base = parent; + for (int level = 0; result->paths_ok && level <= depth; level++) { + const char *leaf = level == depth ? "shared-fs" : "level-component-24-chars"; + int w = snprintf(chain[level], sizeof(chain[level]), "%s/%s", base, leaf); + result->paths_ok = w > 0 && w < (int)sizeof(chain[level]) && mkdir(chain[level], 0755) == 0; + made += result->paths_ok ? 1 : 0; + base = chain[level]; + } + if (result->paths_ok) { + int c = snprintf(cache, sizeof(cache), "%s/cache", chain[depth]); + result->paths_ok = c > 0 && c < (int)sizeof(cache); + result->foreign_path_length = strlen(chain[depth]); + } + if (result->paths_ok) { + result->seam_set = + cbm_daemon_ipc_posix_set_foreign_owned_dir_for_test(chain[depth], foreign_uid); + } + if (result->seam_set) { + result->refused = !cbm_daemon_ipc_private_directory_secure(cache); + const char *why = cbm_daemon_ipc_validation_detail(); + (void)snprintf(result->detail, sizeof(result->detail), "%s", why ? why : ""); + struct stat probe; + result->cache_created = lstat(cache, &probe) == 0; + cbm_daemon_ipc_posix_clear_foreign_owned_dir_for_test(); + /* Control: with real ownership the same path is accepted, so the + * refusal above is the foreign owner and nothing else. */ + result->secured_without_seam = cbm_daemon_ipc_private_directory_secure(cache); + (void)rmdir(cache); + } + cbm_daemon_ipc_posix_clear_foreign_owned_dir_for_test(); + while (made > 0) { + (void)rmdir(chain[--made]); + } + if (depth <= IPC_TEST_FOREIGN_MAX_DEPTH) { + ipc_test_remove_flat_dir(parent); + } +} + +TEST(daemon_ipc_posix_foreign_owned_ancestor_refusal_names_directory_issue1717) { + /* A uid no account resolves to: the detail names the uid alone. */ + unsigned long foreign_uid = (unsigned long)geteuid() + 4242UL; + ASSERT_TRUE(getpwuid((uid_t)foreign_uid) == NULL); /* precondition */ + ipc_test_foreign_result_t result; + ipc_test_foreign_ancestor_run("posix-foreign-ancestor", 0, foreign_uid, &result); + char owner_text[64]; + (void)snprintf(owner_text, sizeof(owner_text), "owned by uid %lu,", foreign_uid); + + ASSERT_TRUE(result.paths_ok); + ASSERT_TRUE(result.seam_set); + ASSERT_TRUE(result.refused); + ASSERT_TRUE(!result.cache_created); /* nothing is created below a foreign ancestor */ + ASSERT_NOT_NULL(strstr(result.detail, "/shared-fs'")); + ASSERT_NOT_NULL(strstr(result.detail, owner_text)); + ASSERT_NOT_NULL(strstr(result.detail, "CBM_CACHE_DIR")); + ASSERT_TRUE(strstr(result.detail, "errno") == NULL); + ASSERT_TRUE(result.secured_without_seam); + PASS(); +} + +/* #1717: the owner is named when the account resolves. uid 1 is a local system + * account in /etc/passwd on macOS and on the Linux images we test ("daemon" on + * both; Alpine calls it "bin"), so the expected name is taken from the same + * local database rather than hardcoded. */ +TEST(daemon_ipc_posix_foreign_owned_ancestor_names_owner_account_issue1717) { + const unsigned long named_uid = 1; + struct passwd *expected = getpwuid((uid_t)named_uid); + if (!expected || !expected->pw_name || !expected->pw_name[0]) { + SKIP_PLATFORM("uid 1 has no local account name on this system"); + } + char owner_text[96]; + (void)snprintf(owner_text, sizeof(owner_text), "owned by uid %lu (%s),", named_uid, + expected->pw_name); + ipc_test_foreign_result_t result; + ipc_test_foreign_ancestor_run("posix-foreign-named", 0, named_uid, &result); + + ASSERT_TRUE(result.paths_ok); + ASSERT_TRUE(result.seam_set); + ASSERT_TRUE(result.refused); + ASSERT_NOT_NULL(strstr(result.detail, owner_text)); + ASSERT_NOT_NULL(strstr(result.detail, "/shared-fs'")); + ASSERT_TRUE(result.secured_without_seam); + PASS(); +} + +/* #1717: the detail buffer is fixed (384 bytes). A long path must not push the + * remedy out: the path is shortened from the left, so the refusing component + * and the whole remedy survive. */ +TEST(daemon_ipc_posix_foreign_owned_ancestor_long_path_keeps_remedy_issue1717) { + const unsigned long named_uid = 1; + ipc_test_foreign_result_t result; + ipc_test_foreign_ancestor_run("posix-foreign-long", IPC_TEST_FOREIGN_MAX_DEPTH, named_uid, + &result); + static const char remedy[] = + "Use a location whose parent directories are yours or root's (e.g. set CBM_CACHE_DIR)"; + + ASSERT_TRUE(result.paths_ok); + ASSERT_TRUE(result.foreign_path_length > 384); /* the input really overflows */ + ASSERT_TRUE(result.refused); + ASSERT_TRUE(strlen(result.detail) < 384); + ASSERT_TRUE(strncmp(result.detail, "'...", 4) == 0); + ASSERT_NOT_NULL(strstr(result.detail, "/shared-fs' is owned by uid 1")); + size_t detail_length = strlen(result.detail); + ASSERT_TRUE(detail_length >= sizeof(remedy) - 1); + ASSERT_STR_EQ(result.detail + detail_length - (sizeof(remedy) - 1), remedy); + PASS(); +} +#endif + +#ifdef CBM_ENABLE_TEST_SEAMS /* #1830: /proc/self/uid_map single-uid detection. A single-uid map is exactly * one line " 1" whose inside id is our euid; anything else — * the init map, a count other than 1, a foreign inside id, extra lines, or junk @@ -5496,6 +5638,65 @@ static void ipc_test_log_capture_sink(const char *line) { ipc_test_log_capture[ipc_test_log_capture_used] = '\0'; } +#ifdef CBM_ENABLE_TEST_SEAMS +/* #1717: a cache root on a network filesystem (shared HPC home, NFS/SMB mount) + * is ACCEPTED, but SQLite WAL is unreliable there, so startup logs ONE warning + * with the local-storage advice. The seam supplies the statfs f_type on every + * platform (real detection is Linux-only), so the mapping and the + * once-per-process rule are bound deterministically here. */ +static bool ipc_test_network_warning_emitted(unsigned long magic, const char *fs_name) { + ipc_test_log_capture_used = 0; + ipc_test_log_capture[0] = '\0'; + cbm_daemon_ipc_set_fs_magic_for_test(true, magic); + cbm_log_set_sink_ex(ipc_test_log_capture_sink, CBM_LOG_SINK_REPLACE); + bool warned = cbm_daemon_ipc_warn_if_network_cache_root("/shared/fs/me/.cache/cbm"); + cbm_log_set_sink(NULL); + char expected[32]; + (void)snprintf(expected, sizeof(expected), "fs=%s", fs_name); + const char *line = strstr(ipc_test_log_capture, "daemon.cache_root_network_fs"); + return warned && line && strstr(line, expected) && strstr(line, "CBM_CACHE_DIR") && + strstr(line, "path=/shared/fs/me/.cache/cbm"); +} + +TEST(daemon_ipc_network_cache_root_warns_once_issue1717) { + CBMLogLevel saved_level = cbm_log_get_level(); + cbm_log_set_level(CBM_LOG_WARN); + bool nfs = ipc_test_network_warning_emitted(0x6969UL, "NFS"); + bool cifs = ipc_test_network_warning_emitted(0xFF534D42UL, "CIFS"); + bool smb2 = ipc_test_network_warning_emitted(0xFE534D42UL, "SMB2"); + bool lustre = ipc_test_network_warning_emitted(0x0BD00BD0UL, "Lustre"); + bool gpfs = ipc_test_network_warning_emitted(0x47504653UL, "GPFS"); + + /* Once per process: a second startup check stays silent. */ + cbm_daemon_ipc_set_fs_magic_for_test(true, 0x6969UL); + bool first = cbm_daemon_ipc_warn_if_network_cache_root("/nfs/cache"); + bool second = cbm_daemon_ipc_warn_if_network_cache_root("/nfs/cache"); + + /* Local filesystems (ext4, xfs, tmpfs) never warn. */ + cbm_daemon_ipc_set_fs_magic_for_test(true, 0xEF53UL); + bool ext4 = cbm_daemon_ipc_warn_if_network_cache_root("/home/me/.cache/cbm"); + cbm_daemon_ipc_set_fs_magic_for_test(true, 0x58465342UL); + bool xfs = cbm_daemon_ipc_warn_if_network_cache_root("/home/me/.cache/cbm"); + cbm_daemon_ipc_set_fs_magic_for_test(true, 0x01021994UL); + bool tmpfs = cbm_daemon_ipc_warn_if_network_cache_root("/home/me/.cache/cbm"); + + cbm_daemon_ipc_set_fs_magic_for_test(false, 0); + cbm_log_set_level(saved_level); + + ASSERT_TRUE(nfs); + ASSERT_TRUE(cifs); + ASSERT_TRUE(smb2); + ASSERT_TRUE(lustre); + ASSERT_TRUE(gpfs); + ASSERT_TRUE(first); + ASSERT_TRUE(!second); + ASSERT_TRUE(!ext4); + ASSERT_TRUE(!xfs); + ASSERT_TRUE(!tmpfs); + PASS(); +} +#endif + /* #1828: a full /tmp made every daemon start die at pending publication, and * the only durable trace was `daemon.ipc.listen_failed stage=pending_publication` * -- no syscall, no errno, no path. The reporter needed hours (and a wrong @@ -5599,6 +5800,9 @@ SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_posix_world_writable_ancestor_still_refused_issue1537); #ifdef CBM_ENABLE_TEST_SEAMS RUN_TEST(daemon_ipc_posix_wsl_drvfs_refusal_names_remedy_issue1687); + RUN_TEST(daemon_ipc_posix_foreign_owned_ancestor_refusal_names_directory_issue1717); + RUN_TEST(daemon_ipc_posix_foreign_owned_ancestor_names_owner_account_issue1717); + RUN_TEST(daemon_ipc_posix_foreign_owned_ancestor_long_path_keeps_remedy_issue1717); RUN_TEST(daemon_ipc_posix_uid_map_single_uid_parse_issue1830); RUN_TEST(daemon_ipc_posix_overflow_ancestor_tolerated_only_in_single_uid_ns_issue1830); RUN_TEST(daemon_ipc_posix_overflow_uid_is_never_cached_issue1830); @@ -5633,6 +5837,9 @@ SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_posix_rejects_non_socket_and_symlink_endpoints); #ifndef _WIN32 RUN_TEST(daemon_ipc_listen_failure_names_errno_and_path); +#ifdef CBM_ENABLE_TEST_SEAMS + RUN_TEST(daemon_ipc_network_cache_root_warns_once_issue1717); +#endif #endif #endif } From 402dd7c59b7287416361edac782fd642ac027cc6 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Thu, 1 Oct 2026 19:59:02 +0200 Subject: [PATCH 4/7] fix(artifact): create .gitattributes through the UTF-8 path layer Follow-up to the #1171 fix. Exporting an artifact from a repository under a CJK path on Windows wrote graph.db.zst and artifact.json, but the .codebase-memory/.gitattributes file carrying the merge=ours protection was silently missing. Root cause: ensure_gitattributes() created the file with the raw POSIX open(O_WRONLY | O_CREAT | O_EXCL). On Windows that is the ANSI CRT, so a repo path with characters outside the active code page fails with ENOENT. The failure was only logged, and the warning itself was malformed: it passed printf-style arguments to the key/value logger, which printed the literal "msg=artifact.gitattributes.open_path=%s_err=%s =". Fix: create the file with cbm_fopen(path, "wbx") (_wfopen on Windows). The "x" mode keeps the O_CREAT | O_EXCL create-only-if-absent semantics, so an existing (possibly user-edited) .gitattributes is never rewritten. Binary mode writes the same LF bytes on every platform. The warning now uses the logger's key/value form (path=..., err=...). Test: artifact::artifact_roundtrip_non_ascii_paths now reads .gitattributes back from the CJK repo through cbm_fopen and asserts the merge=ours line. It also replaces the file with user content, exports again, and asserts the content is unchanged. On the Windows VM the assertion failed 3 of 3 without the fix and passes 3 of 3 with it. The no-overwrite check fails if the mode loses its "x". Refs #1171 Signed-off-by: Martin Vogel --- src/pipeline/artifact.c | 43 +++++++++++++++++++++-------------------- tests/test_artifact.c | 39 +++++++++++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/pipeline/artifact.c b/src/pipeline/artifact.c index 9594c8a09..b1a1ac6c7 100644 --- a/src/pipeline/artifact.c +++ b/src/pipeline/artifact.c @@ -772,30 +772,31 @@ static void ensure_gitattributes(const char *repo_path) { char ga_path[CBM_SZ_4K]; artifact_path(ga_path, sizeof(ga_path), repo_path, ".gitattributes"); - /* Atomic create-only-if-absent: O_EXCL closes the TOCTOU window - * between checking existence and writing. If the file exists, open - * fails with EEXIST and we leave it untouched. */ - int fd = open(ga_path, O_WRONLY | O_CREAT | O_EXCL, 0644); - if (fd < 0) { - if (errno != EEXIST) { - cbm_log_warn("artifact.gitattributes.open path=%s err=%s", ga_path, strerror(errno)); + /* Atomic create-only-if-absent: the "x" mode (O_CREAT|O_EXCL) closes the + * TOCTOU window between checking existence and writing. If the file + * exists, the open fails with EEXIST and we leave it untouched. + * cbm_fopen, not open(): the ANSI CRT open() cannot create the file under + * a repo path outside the active code page on Windows, which silently + * dropped the merge=ours protection there (#1171). */ + errno = 0; + FILE *fp = cbm_fopen(ga_path, "wbx"); + if (!fp) { + int open_errno = errno; + if (open_errno != EEXIST) { + cbm_log_warn("artifact.gitattributes.open", "path", ga_path, "err", + strerror(open_errno)); } /* fall through to merge driver setup either way */ } else { - FILE *fp = fdopen(fd, "w"); - if (fp) { - /* Order matters: attributes apply left to right and the `binary` - * macro expands to `-diff -merge -text`, so a trailing `binary` - * unsets `merge=ours` and the conflict prevention this file - * exists for never engages. The macro must come first. */ - (void)fputs("# Auto-generated by codebase-memory-mcp\n" - "# Prevent merge conflicts on compressed artifact\n" CBM_ARTIFACT_FILENAME - " binary merge=ours\n", - fp); - (void)fclose(fp); - } else { - (void)close(fd); - } + /* Order matters: attributes apply left to right and the `binary` + * macro expands to `-diff -merge -text`, so a trailing `binary` + * unsets `merge=ours` and the conflict prevention this file + * exists for never engages. The macro must come first. */ + (void)fputs("# Auto-generated by codebase-memory-mcp\n" + "# Prevent merge conflicts on compressed artifact\n" CBM_ARTIFACT_FILENAME + " binary merge=ours\n", + fp); + (void)fclose(fp); } /* Best-effort: configure merge driver */ diff --git a/tests/test_artifact.c b/tests/test_artifact.c index 6206463e3..8bbec658d 100644 --- a/tests/test_artifact.c +++ b/tests/test_artifact.c @@ -334,12 +334,30 @@ TEST(artifact_gitattributes_created) { PASS(); } +/* Read a small file through the UTF-8 path layer (wide on Windows), so a + * non-ASCII path is never misread by the test's own ANSI CRT call. Returns the + * byte count, or 0 when the file cannot be opened or is empty. */ +static size_t read_small_file_utf8(const char *path, char *buf, size_t cap) { + buf[0] = '\0'; + FILE *fp = cbm_fopen(path, "rb"); + if (!fp) { + return 0; + } + size_t rd = fread(buf, 1, cap - 1, fp); + (void)fclose(fp); + buf[rd] = '\0'; + return rd; +} + /* #1171: a repository (and a cache directory) under a non-ASCII path must * export, report the artifact present, and import — also onto an existing * cache db. On Windows the atomic writer used the ANSI CRT fopen + MoveFileExA * and the existence check the ANSI stat, so CJK characters outside the active - * code page made every step fail. The bytes are escaped UTF-8 for a CJK repo - * directory name and a CJK cache directory name (kept ASCII in source). */ + * code page made every step fail. The same export also writes the + * .gitattributes merge=ours protection, which ensure_gitattributes created + * with the ANSI CRT open(): on Windows it was silently missing. The bytes are + * escaped UTF-8 for a CJK repo directory name and a CJK cache directory name + * (kept ASCII in source). */ #define ART_CJK_REPO "\xe6\x8c\x81\xe4\xb9\x85\xe5\x8c\x96\xe5\xa4\x8d\xe7\x8e\xb0" #define ART_CJK_CACHE "\xe5\xaf\xbc\xe5\x85\xa5" TEST(artifact_roundtrip_non_ascii_paths) { @@ -356,6 +374,23 @@ TEST(artifact_roundtrip_non_ascii_paths) { ASSERT_TRUE(cbm_file_exists(zst)); ASSERT_TRUE(cbm_artifact_exists(repo)); + char ga[1024]; + snprintf(ga, sizeof(ga), "%s/.codebase-memory/.gitattributes", repo); + char ga_content[512]; + ASSERT_TRUE(read_small_file_utf8(ga, ga_content, sizeof(ga_content)) > 0); + ASSERT_NOT_NULL(strstr(ga_content, CBM_ARTIFACT_FILENAME " binary merge=ours")); + + /* Create-only-if-absent: a re-export never rewrites an existing + * .gitattributes the user may have edited. */ + static const char user_ga[] = "# user-owned\n"; + FILE *uga = cbm_fopen(ga, "wb"); + ASSERT_NOT_NULL(uga); + ASSERT_TRUE(fputs(user_ga, uga) >= 0); + ASSERT_EQ(fclose(uga), 0); + ASSERT_EQ(cbm_artifact_export(g_db, repo, "test-proj", CBM_ARTIFACT_FAST), 0); + ASSERT_EQ(read_small_file_utf8(ga, ga_content, sizeof(ga_content)), strlen(user_ga)); + ASSERT_STR_EQ(ga_content, user_ga); + char cache_dir[1024]; snprintf(cache_dir, sizeof(cache_dir), "%s/" ART_CJK_CACHE, g_tmpdir); ASSERT_TRUE(cbm_mkdir_p(cache_dir, 0755)); From e066515190a1d1747edb23274c3f1333b80102be Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 28 Sep 2026 19:34:20 +0200 Subject: [PATCH 5/7] fix(routes): mint Laravel routes written without a leading slash Why: Laravel accepts a route URI with or without its leading slash and trims '/' from both ends, so Route::get('users', ...) serves /users and Route::get('', ...) the group root. Most Laravel apps write it that way. Both route passes only minted a Route for a first argument starting with '/', so Firefly III's API registrations (246 distinct routes in routes/api.php) had no Route at all, and neither did laravel.io's PUT/DELETE API routes or 62 of its 67 registrations. Fix (extract_calls.c, PHP only; other languages' path gates are untouched): - A recognised Laravel registration gets its literal URI normalised to the slashed form before the gates: 'users' -> /users, '' -> /, 'users/' -> /users. It is recognised by an HTTP verb (get/post/put/patch/delete) called on the Route facade - directly, or at the root of a registrar chain such as Route::middleware('auth')->get(...) - or on $router. The literal `Route` scope is the same one the #952 callee qualification accepts. Any other call keeps its argument: $request->get('name'), Cache::get('key'), config(), a collection's or session's get(), $this->router->get(), and a test client's $this->get('users'). - The normalised path composes with the enclosing in-file prefix groups the same way as #952. The attribute-array group form Route::group(['prefix' => 'v1/autocomplete', ...], function () {...}), which Firefly III's whole API uses, now contributes its prefix too; without it those routes would mint as /accounts instead of /v1/autocomplete/accounts. - A slashless registration whose action is a string ('index' inside a Route::controller() group, 'UserController@store') is minted without a handler. The string does not say which class owns the method, and resolving it could only guess: on krayin/laravel-crm, 'update' and 'destroy' bound functions in a bundled chart.js, and 'store' bound ActivityController for /login. Proof (release CLI built from this tree vs origin/main 1f9b4db1): - Routes minted by route registration: firefly-iii 30 -> 651, laravel.io 39 -> 81, krayin/laravel-crm 5 -> 132. An independent recomputation of every composed path from the route files matches exactly: firefly routes/api.php 246/246 and routes/web.php 390/390, laravel.io web 64/64 and api 3/3, with 0 missing and 0 extra. - Samples checked against source: GET /v1/autocomplete/accounts and DELETE /v1/accounts/{account} (firefly), PUT /articles/{article} (laravel.io api), PUT /admin/articles/{article}/approve (laravel.io web), and PUT /leads/edit/{id} (krayin). - No false Routes: every new route-registration edge comes from a Route:: registrar in a route file. The pre-existing test-client and JS-client route edges are identical to main. HANDLES stays at 0 on all three corpora, so no handler is guessed. Tests (edge_types_probe): routes_laravel_slashless_issue1146 and routes_laravel_slashless_parallel_issue1146 assert the exact method+path set: facade, facade chain, $router, chain and array prefix groups, '' and a trailing slash. routes_laravel_slashless_no_junk_issue1146 covers the non-route string calls. routes_laravel_slashless_no_guessed_handlers_issue1146 checks string actions against JS and PHP decoys. RED on origin/main 3x, green, RED on revert (the guessed-handler test is also RED with only the handler gate removed). edge_types_probe, php_lsp, pipeline, parallel, route_canon and cross_repo pass (731); make lint-ci passes. Refs #1146 Signed-off-by: Martin Vogel --- internal/cbm/extract_calls.c | 152 +++++++++++++++++++++++++++++++- tests/test_edge_types_probe.c | 159 ++++++++++++++++++++++++++++++++++ 2 files changed, 309 insertions(+), 2 deletions(-) diff --git a/internal/cbm/extract_calls.c b/internal/cbm/extract_calls.c index f14efb766..a8b97c639 100644 --- a/internal/cbm/extract_calls.c +++ b/internal/cbm/extract_calls.c @@ -1763,6 +1763,59 @@ static const char *php_chain_prefix_arg(CBMArena *a, TSNode call_node, const cha return NULL; } +/* The value of a plain PHP string literal ('...' or "..." with no + * interpolation or escapes), "" for an empty one; NULL for anything else. */ +static const char *php_plain_string_value(CBMArena *a, TSNode node, const char *source) { + const char *kind = ts_node_type(node); + if (strcmp(kind, "string") != 0 && strcmp(kind, "encapsed_string") != 0) { + return NULL; + } + uint32_t nc = ts_node_named_child_count(node); + for (uint32_t i = 0; i < nc; i++) { + if (strcmp(ts_node_type(ts_node_named_child(node, i)), "string_content") != 0) { + return NULL; + } + } + return nc == 0 ? "" : cbm_node_text(a, ts_node_named_child(node, 0), source); +} + +/* The first argument's value node of a PHP call (unwrapping `argument`). */ +static TSNode php_first_arg_value(TSNode call_node) { + TSNode none = {0}; + TSNode args = ts_node_child_by_field_name(call_node, TS_FIELD("arguments")); + if (ts_node_is_null(args) || ts_node_named_child_count(args) == 0) { + return none; + } + TSNode arg = ts_node_named_child(args, 0); + if (strcmp(ts_node_type(arg), "argument") == 0) { + return ts_node_named_child_count(arg) == SKIP_ONE ? ts_node_named_child(arg, 0) : none; + } + return arg; +} + +/* The literal 'prefix' of an attribute-array group (#1146): + * Route::group(['prefix' => 'v1/autocomplete', 'as' => ...], function () {...}), + * the form Firefly III's whole API is written in. */ +static const char *php_group_array_prefix(CBMArena *a, TSNode group_call, const char *source) { + TSNode attrs = php_first_arg_value(group_call); + if (ts_node_is_null(attrs) || strcmp(ts_node_type(attrs), "array_creation_expression") != 0) { + return NULL; + } + uint32_t nc = ts_node_named_child_count(attrs); + for (uint32_t i = 0; i < nc; i++) { + TSNode el = ts_node_named_child(attrs, i); + if (strcmp(ts_node_type(el), "array_element_initializer") != 0 || + ts_node_named_child_count(el) != PAIR_LEN) { + continue; + } + const char *key = php_plain_string_value(a, ts_node_named_child(el, 0), source); + if (key && strcmp(key, "prefix") == 0) { + return php_plain_string_value(a, ts_node_named_child(el, SKIP_ONE), source); + } + } + return NULL; +} + static const char *php_group_prefix_for_call(CBMArena *a, TSNode node, const char *source) { const char *parts[PHP_PREFIX_PARTS_MAX]; int part_count = 0; @@ -1785,7 +1838,14 @@ static const char *php_group_prefix_for_call(CBMArena *a, TSNode node, const cha strcmp(ts_node_type(p), "scoped_call_expression") == 0)) { TSNode gname = ts_node_child_by_field_name(p, TS_FIELD("name")); char *gtxt = ts_node_is_null(gname) ? NULL : cbm_node_text(a, gname, source); - if (gtxt && strcmp(gtxt, "group") == 0) { + const char *array_prefix = gtxt && strcmp(gtxt, "group") == 0 + ? php_group_array_prefix(a, p, source) + : NULL; + if (array_prefix) { + if (part_count < PHP_PREFIX_PARTS_MAX) { + parts[part_count++] = array_prefix; /* inner-first */ + } + } else if (gtxt && strcmp(gtxt, "group") == 0) { /* Scan the receiver chain for prefix('...'). */ TSNode recv = ts_node_child_by_field_name(p, TS_FIELD("object")); for (int hops = 0; hops < PHP_GROUP_WALK_MAX && !ts_node_is_null(recv); @@ -1843,6 +1903,81 @@ static const char *php_group_prefix_for_call(CBMArena *a, TSNode node, const cha return pos ? cbm_arena_strndup(a, buf, pos) : NULL; } +/* A Laravel route registrar receiver (#1146): the Route facade — also at the + * root of a registrar chain, Route::middleware('auth')->get(...) — or the + * $router instance route files are loaded with (RouteServiceProvider, Lumen). + * The same literal `Route` scope the #952 callee qualification accepts. */ +static bool php_is_laravel_registrar(CBMArena *a, TSNode call_node, const char *source) { + TSNode cur = call_node; + for (int hops = 0; hops < PHP_GROUP_WALK_MAX && !ts_node_is_null(cur); hops++) { + const char *kind = ts_node_type(cur); + if (strcmp(kind, "scoped_call_expression") == 0) { + TSNode scope = ts_node_child_by_field_name(cur, TS_FIELD("scope")); + const char *st = ts_node_is_null(scope) ? NULL : cbm_node_text(a, scope, source); + return st && strcmp(st, "Route") == 0; + } + if (strcmp(kind, "variable_name") == 0) { + const char *vt = cbm_node_text(a, cur, source); + return vt && strcmp(vt, "$router") == 0; + } + if (strcmp(kind, "member_call_expression") != 0) { + return false; + } + cur = ts_node_child_by_field_name(cur, TS_FIELD("object")); + } + return false; +} + +/* Laravel accepts a route URI with or without its leading slash and trims + * '/' from both ends: Route::get('users', ...) serves /users, and '' is the + * group root. The route passes only mint a Route for a path that starts with + * '/', so give a recognised registration's literal URI its slash here, where + * the first argument is still at hand (#1146). Returns NULL — leave the call + * alone — for any other call, any non-literal URI, or one already slashed: + * `$request->get('name')`, `Cache::get('key')` and a collection's get('k') + * keep their argument. */ +static const char *php_laravel_route_uri(CBMArena *a, TSNode call_node, const char *source) { + static const char *const verbs[] = {"get", "post", "put", "patch", "delete", NULL}; + TSNode mname = ts_node_child_by_field_name(call_node, TS_FIELD("name")); + const char *verb = ts_node_is_null(mname) ? NULL : cbm_node_text(a, mname, source); + bool is_verb = false; + for (int i = 0; verb && verbs[i] && !is_verb; i++) { + is_verb = strcmp(verb, verbs[i]) == 0; + } + if (!is_verb || !php_is_laravel_registrar(a, call_node, source)) { + return NULL; + } + TSNode arg0 = php_first_arg_value(call_node); + const char *uri = ts_node_is_null(arg0) ? NULL : php_plain_string_value(a, arg0, source); + if (!uri || uri[0] == '/') { + return NULL; + } + size_t len = strlen(uri); + while (len > 0 && uri[len - SKIP_ONE] == '/') { + len--; + } + return cbm_arena_sprintf(a, "/%.*s", (int)len, uri); +} + +/* Is a Laravel registration's action (its last argument) a string — 'index' + * inside a Route::controller() group, 'UserController@show'? Such an action + * does not say which class owns the method, so resolving it could only guess a + * same-named function anywhere in the repo. */ +static bool php_route_action_is_string(TSNode call_node) { + TSNode args = ts_node_child_by_field_name(call_node, TS_FIELD("arguments")); + uint32_t nc = ts_node_is_null(args) ? 0 : ts_node_named_child_count(args); + if (nc < PAIR_LEN) { + return false; + } + TSNode action = ts_node_named_child(args, nc - SKIP_ONE); + if (strcmp(ts_node_type(action), "argument") == 0 && + ts_node_named_child_count(action) == SKIP_ONE) { + action = ts_node_named_child(action, 0); + } + const char *kind = ts_node_type(action); + return strcmp(kind, "string") == 0 || strcmp(kind, "encapsed_string") == 0; +} + static bool is_nested_verilog_call_wrapper(CBMLanguage lang, TSNode node) { if (lang != CBM_LANG_VERILOG || strcmp(ts_node_type(node), "subroutine_call") != 0) { return false; @@ -3941,6 +4076,16 @@ CBMInvocationDescriptor handle_calls(CBMExtractCtx *ctx, TSNode node, const CBML } if (!ts_node_is_null(args)) { call.first_string_arg = extract_url_or_topic_arg(ctx, args, call.callee_name); + /* #1146: a Laravel registration's slashless URI ('users', '') + * becomes '/users' / '/' so it mints and composes below. */ + const char *laravel_uri = NULL; + if (ctx->language == CBM_LANG_PHP && call.callee_name && + cbm_service_pattern_route_method(call.callee_name) != NULL) { + laravel_uri = php_laravel_route_uri(ctx->arena, node, ctx->source); + if (laravel_uri) { + call.first_string_arg = laravel_uri; + } + } /* #952: routes registered inside Laravel `prefix()->group()` * closures must carry the composed path — the resolve passes * only see the flat CBMCall, so the enclosing chain can only @@ -3959,7 +4104,10 @@ CBMInvocationDescriptor handle_calls(CBMExtractCtx *ctx, TSNode node, const CBML : cbm_arena_strndup(ctx->arena, gp, strlen(gp)); } } - if (call.first_string_arg && call.first_string_arg[0] == '/') { + /* A slashless registration's string action would only be + * guessed at (#1146): mint its Route without a handler. */ + if (call.first_string_arg && call.first_string_arg[0] == '/' && + !(laravel_uri && php_route_action_is_string(node))) { call.second_arg_name = extract_handler_arg(ctx, args); } if (ctx->language == CBM_LANG_OBJECTSCRIPT_UDL || diff --git a/tests/test_edge_types_probe.c b/tests/test_edge_types_probe.c index 5ed449b8d..30541db2c 100644 --- a/tests/test_edge_types_probe.c +++ b/tests/test_edge_types_probe.c @@ -922,6 +922,161 @@ TEST(routes_laravel_withrouting_parallel_subdir_issue1146) { PASS(); } +/* Assert the exact Route set as "METHOD /path" strings: the method comes from + * the Route QN ("__route__PUT__/articles/{}"), the path from the node name + * (the raw composed path). The count must match too, so a junk Route fails. */ +static int et_route_set_exact(const EtFile *files, int nfiles, const char **want, int parallel) { + EtProj lp; + cbm_store_t *store = + parallel ? et_index_parallel(&lp, files, nfiles) : et_index_files(&lp, files, nfiles); + cbm_node_t *nodes = NULL; + int node_count = 0; + int wanted = 0; + int found[ET_ROUTE_ASSERT_MAX] = {0}; + while (want[wanted] && wanted < ET_ROUTE_ASSERT_MAX) { + wanted++; + } + int ok = store != NULL && + cbm_store_find_nodes_by_label(store, lp.project, "Route", &nodes, &node_count) == + CBM_STORE_OK && + node_count == wanted; + for (int ni = 0; store && ni < node_count; ni++) { + char got[512]; + const char *qn = nodes[ni].qualified_name ? nodes[ni].qualified_name : ""; + const char *m = strncmp(qn, "__route__", 9) == 0 ? qn + 9 : qn; + const char *end = strstr(m, "__"); + snprintf(got, sizeof(got), "%.*s %s", end ? (int)(end - m) : 0, m, + nodes[ni].name ? nodes[ni].name : ""); + int matched = 0; + for (int wi = 0; wi < wanted; wi++) { + if (!found[wi] && strcmp(got, want[wi]) == 0) { + found[wi] = matched = 1; + break; + } + } + if (!matched) { + ok = 0; + fprintf(stderr, " [ET-ROUTESET] unexpected %s\n", got); + } + } + for (int wi = 0; wi < wanted; wi++) { + if (!found[wi]) { + ok = 0; + fprintf(stderr, " [ET-ROUTESET] missing %s\n", want[wi]); + } + } + if (!ok) { + fprintf(stderr, " [ET-ROUTESET] FAIL (%s path) expected=%d actual=%d\n", + parallel ? "parallel" : "sequential", wanted, node_count); + } + cbm_store_free_nodes(nodes, node_count); + et_cleanup(&lp, store); + return ok; +} + +/* #1146: Laravel writes most route URIs without the leading slash — + * Route::get('users', ...) serves /users, Route::put('{article}', ...) inside + * prefix('articles') serves /articles/{article}, and '' is the group root. + * Both route passes only minted a Route for a first argument starting with + * '/', so Firefly III's API (258 registrations) and laravel.io's PUT/DELETE + * API routes had no Route at all. Covers the facade, a facade chain + * (Route::middleware()->get), the $router instance, the chain-form and the + * array-form prefix group (Route::group(['prefix' => ...], fn)), and a + * trailing slash. */ +static const EtFile et_laravel_slashless[] = { + {"routes/api.php", + "group(function () {\n" + " Route::get('', 'ArticlesController@index');\n" + " Route::put('{article}', 'ArticlesController@update');\n" + " Route::delete('{article}', 'ArticlesController@delete');\n" + "});\n" + "Route::group(['prefix' => 'v1/autocomplete', 'as' => 'api.v1.'], static function (): void {\n" + " Route::get('accounts', ['uses' => 'AccountController@accounts', 'as' => 'accounts']);\n" + "});\n" + "Route::middleware('auth')->get('dashboard', 'DashboardController@show');\n" + "$router->get('lumen/users', 'UserController@index');\n"}}; + +static const char *et_laravel_slashless_routes[] = {"GET /users", + "POST /users", + "GET /articles", + "PUT /articles/{article}", + "DELETE /articles/{article}", + "GET /v1/autocomplete/accounts", + "GET /dashboard", + "GET /lumen/users", + NULL}; + +TEST(routes_laravel_slashless_issue1146) { + ASSERT_TRUE(et_route_set_exact(et_laravel_slashless, 1, et_laravel_slashless_routes, 0)); + PASS(); +} + +TEST(routes_laravel_slashless_parallel_issue1146) { + ASSERT_TRUE(et_route_set_exact(et_laravel_slashless, 1, et_laravel_slashless_routes, 1)); + PASS(); +} + +/* The string action of a slashless registration — 'index' inside a + * Route::controller() group, 'UserController@store' — does not say which + * class owns the method, so resolving it could only guess a same-named + * function anywhere in the repo (on krayin/laravel-crm 'update' and 'destroy' + * bound a chart.js function). Such routes are minted without a guessed + * HANDLES edge. Decoys: a JS function and a PHP method per action name. */ +TEST(routes_laravel_slashless_no_guessed_handlers_issue1146) { + static const char *want[] = {"GET /leads", "PUT /leads/edit/{id}", "POST /users", NULL}; + static const EtFile f[] = { + {"routes/web.php", + "prefix('leads')->group(function () {\n" + " Route::get('', 'index');\n" + " Route::put('edit/{id}', 'update');\n" + "});\n" + "Route::post('users', 'UserController@store');\n"}, + {"resources/js/chart.js", + "function index() { return 1; }\nfunction update() { return 2; }\n"}, + {"app/Http/Controllers/ActivityController.php", + "get('name');\n" + " $q = $request->query->get('q');\n" + " $v = Cache::get('users.count');\n" + " $c = config('app.name');\n" + " $k = collect(['a' => 1])->get('a');\n" + " $s = session()->get('key');\n" + " $r = $this->router->get('status');\n" + " $p = $this->post('login', []);\n" + " return $this->get('users');\n" + " }\n}\n"}}; + ASSERT_TRUE(et_route_set_exact(f, 1, want, 0)); + PASS(); +} + /* Rails (Ruby) — ActionDispatch router. The handler MUST be passed as a bare * identifier (not the idiomatic `to: 'list_items'` string, which extract_handler_arg * cannot capture). mapper.get resolves by name to the Mapper#get method whose QN @@ -2138,6 +2293,10 @@ SUITE(edge_types_probe) { RUN_TEST(routes_laravel_routeserviceprovider_control_issue1146); RUN_TEST(routes_laravel_withrouting_nonliteral_prefix_issue1146); RUN_TEST(routes_laravel_withrouting_parallel_subdir_issue1146); + RUN_TEST(routes_laravel_slashless_issue1146); + RUN_TEST(routes_laravel_slashless_parallel_issue1146); + RUN_TEST(routes_laravel_slashless_no_guessed_handlers_issue1146); + RUN_TEST(routes_laravel_slashless_no_junk_issue1146); RUN_TEST(handles_rails_ruby); RUN_TEST(handles_actix_rust); From 14ed8ce38a537a3ce14cb8957d6418ef9a460bae Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 25 Sep 2026 18:37:16 +0200 Subject: [PATCH 6/7] fix(coverage): one line counter for a source buffer (#1967) Three places in internal/cbm/cbm.c counted the lines of the same source buffer with two different conventions. cbm_count_lines (parse_unusable 80% threshold) ignored a final newline, while the inline counter that sizes the preprocessed-line map (orig_lines) and cbm_line_offsets (the recovery-gap walker) counted every newline, so on any file ending in a newline -- nearly every file -- they disagreed by one. Nothing produced a wrong answer: every consumer of the larger count either clamps to it or sees the phantom last line as blank. The risk was the next reader picking up one convention without knowing the other existed. Add cbm_source_line_count() as the single helper, with the convention documented beside it: a '\n' terminates a line and opens no new one; an empty buffer counts as 1 line. All three call sites use it; the old static cbm_count_lines is gone. Test: parse_coverage::source_line_count_one_convention pins the convention (trailing newline, no final newline, CRLF, empty buffer, blank last line, src_len bound). Signed-off-by: Martin Vogel --- internal/cbm/cbm.c | 48 ++++++++++++++++++------------------- internal/cbm/cbm.h | 5 ++++ tests/test_parse_coverage.c | 18 ++++++++++++++ 3 files changed, 46 insertions(+), 25 deletions(-) diff --git a/internal/cbm/cbm.c b/internal/cbm/cbm.c index 4d01d0c69..aae9d64ba 100644 --- a/internal/cbm/cbm.c +++ b/internal/cbm/cbm.c @@ -1433,6 +1433,26 @@ static void cbm_mark_pp_error_rows(TSNode n, uint8_t *rows, uint32_t row_count, } } +/* The ONE line counter for a source buffer (#1967). Every caller that asks + * "how many lines does this file have" uses it, so the coverage report never + * holds two answers for the same buffer. + * + * Convention: a '\n' TERMINATES a line; it does not open a new one. So the + * count is the number of '\n' that have at least one byte after them, plus + * one. "a\nb" and "a\nb\n" both have 2 lines (the trailing newline adds no + * phantom empty line, matching what an editor shows); "a\nb" without a final + * newline still counts its last line. An empty buffer counts as 1 line, because + * tree-sitter still reports row 0 for it and 1-based line maps index line 1. */ +uint32_t cbm_source_line_count(const char *src, int src_len) { + uint32_t n = 1; + for (int i = 0; i + 1 < src_len; i++) { + if (src[i] == '\n') { + n++; + } + } + return n; +} + /* Recovery subtraction (#963): tree-sitter error recovery plus the * ERROR-descending def walker often still extract constructs INSIDE a failed * region (verified: a function in an #ifdef-split ERROR region and even a @@ -1450,12 +1470,7 @@ static void cbm_mark_pp_error_rows(TSNode n, uint8_t *rows, uint32_t row_count, * Now the uncovered gaps are reported instead, and a gap holding only blank, * comment or preprocessor lines is not a miss at all. */ static uint32_t *cbm_line_offsets(const char *src, int src_len, uint32_t *out_lines) { - uint32_t lines = 1; - for (int i = 0; i < src_len; i++) { - if (src[i] == '\n') { - lines++; - } - } + uint32_t lines = cbm_source_line_count(src, src_len); uint32_t *offs = (uint32_t *)cbm_alloc(CBM_MEM_CLASS_EXTRACT, (size_t)(lines + 1) * sizeof(uint32_t)); if (!offs) { @@ -2060,18 +2075,6 @@ static void cbm_refine_regions_with_pp_lines(cbm_error_regions_t *regs, const ui * this repo covers 25.5% of its file, and the next widest 3.9%. */ #define CBM_UNUSABLE_PCT 80 -/* Number of 1-based lines in `src`. A file that does not end with a newline - * still has a last line, so the count is separators plus one. */ -static uint32_t cbm_count_lines(const char *src, int src_len) { - uint32_t n = 1; - for (int i = 0; i < src_len; i++) { - if (src[i] == '\n' && i + 1 < src_len) { - n++; - } - } - return n; -} - /* Serialize collected regions as "start-end,start-end,...", with a trailing * ",+" when the cap threw N ranges away. * @@ -2745,12 +2748,7 @@ static CBMFileResult *extract_file_ex_body(const char *source, int source_len, C * total loss (root is ERROR), because then it vouches for * nothing and there is no refinement to make. */ if (strcmp(ts_node_type(pp_root), "ERROR") != 0) { - uint32_t orig_lines = 1; - for (int ci = 0; ci < source_len; ci++) { - if (source[ci] == '\n') { - orig_lines++; - } - } + uint32_t orig_lines = cbm_source_line_count(source, source_len); uint8_t *map = (uint8_t *)cbm_arena_alloc(a, (size_t)orig_lines + 2); int exp_lines = preprocessed->expanded_line_count; uint8_t *bad_rows = @@ -2946,7 +2944,7 @@ static CBMFileResult *extract_file_ex_body(const char *source, int source_len, C * so the report can say "read the source" instead. See * parse_unusable in cbm.h for which files land here and why. */ if (regs.count == 1 && regs.dropped == 0) { - uint32_t total = cbm_count_lines(source, source_len); + uint32_t total = cbm_source_line_count(source, source_len); uint32_t span = regs.ends[0] - regs.starts[0] + 1; if (total > 0 && span * 100 >= total * CBM_UNUSABLE_PCT) { result->parse_unusable = true; diff --git a/internal/cbm/cbm.h b/internal/cbm/cbm.h index dc89a6b2d..7f1aad43e 100644 --- a/internal/cbm/cbm.h +++ b/internal/cbm/cbm.h @@ -920,6 +920,11 @@ uint64_t cbm_usage_field_lookup_test_work(void); uint64_t cbm_usage_slow_parent_fallback_test_count(void); #endif +// Number of 1-based lines in a source buffer. The single line-count convention +// for coverage reporting (#1967): a trailing '\n' ends the last line and opens +// no new one; an empty buffer counts as 1 line. See the definition in cbm.c. +uint32_t cbm_source_line_count(const char *src, int src_len); + // Toggle C/C++ preprocessor Macro-node extraction (#375). The pipeline enables // it only for full/advanced index modes (it dominates extraction on macro-dense // codebases). Default ON. Set before extraction; read-only during. diff --git a/tests/test_parse_coverage.c b/tests/test_parse_coverage.c index 09caeaa82..5dbc38e4a 100644 --- a/tests/test_parse_coverage.c +++ b/tests/test_parse_coverage.c @@ -1615,7 +1615,25 @@ TEST(cs_malformed_conditional_remains_partial_issue1748) { PASS(); } +/* #1967: every line counter over one source buffer answers the same + * question the same way. A trailing newline ends the last line; it does not + * open a new one. */ +TEST(source_line_count_one_convention) { + ASSERT_EQ(cbm_source_line_count("", 0), 1u); + ASSERT_EQ(cbm_source_line_count("a", 1), 1u); + ASSERT_EQ(cbm_source_line_count("a\n", 2), 1u); + ASSERT_EQ(cbm_source_line_count("\n", 1), 1u); + ASSERT_EQ(cbm_source_line_count("a\nb", 3), 2u); + ASSERT_EQ(cbm_source_line_count("a\nb\n", 4), 2u); + ASSERT_EQ(cbm_source_line_count("a\n\n", 3), 2u); + ASSERT_EQ(cbm_source_line_count("a\r\nb\r\n", 6), 2u); + /* src_len bounds the count, not a NUL terminator. */ + ASSERT_EQ(cbm_source_line_count("a\nb\nc", 2), 1u); + PASS(); +} + SUITE(parse_coverage) { + RUN_TEST(source_line_count_one_convention); RUN_TEST(c_ifdef_split_brace_sets_parse_incomplete); RUN_TEST(c_ifdef_split_brace_neighbors_still_extracted); RUN_TEST(c_error_range_points_at_failed_region); From c5a3c6eae944bb863bd9310faf6a995f4a7365d9 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 18:16:02 +0200 Subject: [PATCH 7/7] fix(routes): register FastAPI and Starlette WebSocket decorators Distilled from #1245 by Andrew Hundt (a slice of 1ac3c136). The route-registration suffix table knew .add_api_websocket_route but not the decorator forms FastAPI and Starlette actually use: @router.websocket("/ws") @app.websocket_route("/events") so a WebSocket endpoint produced no Route node on either resolver path and was invisible to the graph. Two entries join the table, both recorded as ANY like the existing WebSocket entry -- a socket upgrade is not an HTTP verb. Both are needed: matching is a pure suffix compare and .route does not match ...websocket_route, whose tail is _route. Upstream also changed pass_calls.c, pass_parallel.c and pipeline_internal.h. None of that is taken: main already carries the #952 sequential unresolved-callee route fallback its July base lacked, so the table entries are the only missing piece. RED before the fix, each form recorded on its own (the harness stops at the first failing assert, so the two were swapped and re-run): pipeline_python_websocket_decorators_register_routes FAIL tests/test_pipeline.c:6163: count_nodes_named(s, project, "/ws") (0) not >= 1 (1) FAIL tests/test_pipeline.c:6163: count_nodes_named(s, project, "/events") (0) not >= 1 (1) The @app.get("/health") control above those lines passed before and after. GREEN after: pipeline 282 passed; pipeline parallel extraction edge_types_probe route_canon infrascan 779 passed, 0 failed. The test drives the sequential resolver (one file). The parallel resolver reads the same table through cbm_service_pattern_route_method and its suite is green; a CBM_WORKERS=4 twin of this test is a small follow-up. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- internal/cbm/service_patterns.c | 5 ++++ tests/test_pipeline.c | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/internal/cbm/service_patterns.c b/internal/cbm/service_patterns.c index 6da74d478..2626b52ca 100644 --- a/internal/cbm/service_patterns.c +++ b/internal/cbm/service_patterns.c @@ -577,6 +577,11 @@ static const method_suffix_t route_reg_suffixes[] = { {".add_route", "ANY"}, {".add_api_route", "ANY"}, {".add_api_websocket_route", "ANY"}, + /* FastAPI/Starlette WebSocket decorators (`@router.websocket('/ws')`, + * `@app.websocket_route('/events')`): not an HTTP verb, so ANY — mirrors + * .add_api_websocket_route above (#1245). */ + {".websocket", "ANY"}, + {".websocket_route", "ANY"}, {NULL, NULL}, }; diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 44bb49e9c..889bffbe1 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -7487,6 +7487,56 @@ TEST(pipeline_capitalized_axios_import_is_http_client_sequential) { return capitalized_axios_case(0); } +/* FastAPI/Starlette WebSocket route registration (distilled from #1245). The + * route-registration suffix table knew `.add_api_websocket_route` but not the + * decorator forms `.websocket` / `.websocket_route`, so a WebSocket endpoint + * registered via `@router.websocket('/ws')` produced no Route node at all — + * the endpoint was invisible to the graph. `@app.get('/health')` is the + * positive control (already a route suffix). Fewer than 50 files exercises + * pass_calls.c; the same suffix table drives the parallel resolver. */ +TEST(pipeline_python_websocket_decorators_register_routes) { + char tmp[256]; + snprintf(tmp, sizeof(tmp), "/tmp/cbm_py_ws_routes_XXXXXX"); + if (!cbm_mkdtemp(tmp)) { + FAIL("tmpdir"); + } + + write_temp_file(tmp, "app.py", + "from fastapi import APIRouter, FastAPI, WebSocket\n\n" + "app = FastAPI()\n" + "router = APIRouter()\n\n" + "@app.get('/health')\n" + "def health():\n" + " return {'ok': True}\n\n" + "@router.websocket('/ws')\n" + "async def ws_endpoint(websocket: WebSocket):\n" + " await websocket.accept()\n\n" + "@app.websocket_route('/events')\n" + "async def events_endpoint(websocket: WebSocket):\n" + " await websocket.accept()\n"); + + char db_path[512]; + snprintf(db_path, sizeof(db_path), "%s/py_ws.db", tmp); + cbm_pipeline_t *p = cbm_pipeline_new(tmp, db_path, CBM_MODE_FULL); + ASSERT_NOT_NULL(p); + ASSERT_EQ(cbm_pipeline_run(p), 0); + const char *project = cbm_pipeline_project_name(p); + + cbm_store_t *s = cbm_store_open_path(db_path); + ASSERT_NOT_NULL(s); + + /* Positive control: the HTTP decorator mints its Route node. */ + ASSERT_GTE(count_nodes_named(s, project, "/health"), 1); + /* The two WebSocket decorator forms must mint Route nodes too. */ + ASSERT_GTE(count_nodes_named(s, project, "/ws"), 1); + ASSERT_GTE(count_nodes_named(s, project, "/events"), 1); + + cbm_store_close(s); + cbm_pipeline_free(p); + th_rmtree(tmp); + PASS(); +} + /* Python bare-call local-binding suppression, sequential path. The bare-call * counterpart of the receiver guard above: `run` is a PARAMETER, so `run()` * cannot be the module-level `run` and must not bind SatoriLive.run. @@ -17568,6 +17618,7 @@ SUITE(pipeline) { RUN_TEST(pipeline_tsjs_receiver_parallel_keeps_service_edges); RUN_TEST(pipeline_capitalized_axios_import_is_http_client); RUN_TEST(pipeline_capitalized_axios_import_is_http_client_sequential); + RUN_TEST(pipeline_python_websocket_decorators_register_routes); RUN_TEST(pipeline_python_receiver_parallel_suppresses_weak_method_edges); RUN_TEST(pipeline_python_bare_local_binding_suppresses_weak_edge); RUN_TEST(pipeline_python_bare_local_binding_parallel_suppresses_weak_edge);