From 7883507db6779a20bcfad398c8d6594f4206aa9b Mon Sep 17 00:00:00 2001 From: Brian McNaboe Date: Wed, 19 Aug 2026 10:14:08 -0400 Subject: [PATCH 1/7] fix(hooks): clear repository git environment for tests Signed-off-by: Brian McNaboe --- scripts/hooks/pre-commit | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts/hooks/pre-commit b/scripts/hooks/pre-commit index fa42d92ae4..7b5e1a4fff 100755 --- a/scripts/hooks/pre-commit +++ b/scripts/hooks/pre-commit @@ -17,4 +17,13 @@ echo "pre-commit: security audit (source-level)..." scripts/security-audit.sh echo "pre-commit: building and running tests..." -make -j$(sysctl -n hw.ncpu 2>/dev/null || nproc) -f Makefile.cbm test +( + # Git exports repository-local variables to hooks. The test suite creates + # independent repositories and linked worktrees, so keep the commit's + # index and object paths out of those fixtures. + while IFS= read -r variable; do + unset "$variable" + done < <(git rev-parse --local-env-vars) + + make -j"$(sysctl -n hw.ncpu 2>/dev/null || nproc)" -f Makefile.cbm test +) From 3604d6cf66c3d3099cec0b0b9a2f6da7f15e9e88 Mon Sep 17 00:00:00 2001 From: Brian McNaboe Date: Wed, 19 Aug 2026 10:35:16 -0400 Subject: [PATCH 2/7] fix(tests): scale daemon runtime sanitizer timeout Signed-off-by: Brian McNaboe --- tests/test_daemon_runtime.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/test_daemon_runtime.c b/tests/test_daemon_runtime.c index 5f41d914e8..b650b5efb7 100644 --- a/tests/test_daemon_runtime.c +++ b/tests/test_daemon_runtime.c @@ -21,6 +21,7 @@ #include "foundation/compat_thread.h" #include "foundation/log.h" #include "foundation/platform.h" +#include "foundation/sanitized.h" #include "pipeline/pipeline.h" #include "store/store.h" @@ -3550,10 +3551,18 @@ TEST(daemon_runtime_disconnect_cancels_blocked_non_index_child_and_preserves_oth SKIP_PLATFORM("requires a queryable copied process image"); #else enum { +#if CBM_SANITIZED + /* This readiness wait is a liveness backstop, not the behavior under + * test. The copied instrumented runner can take several seconds to + * reach main on macOS, especially while the parallel gate is busy. */ + CHILD_READY_BOUND_MS = 60000, + REQUEST_TIMEOUT_MS = 90000, +#else CHILD_READY_BOUND_MS = 5000, + REQUEST_TIMEOUT_MS = 15000, +#endif CHILD_CANCEL_BOUND_MS = 3000, CHILD_CLEANUP_BOUND_MS = 5000, - REQUEST_TIMEOUT_MS = 15000, }; const char *old_cache = getenv("CBM_CACHE_DIR"); const char *old_path = getenv("PATH"); From c314dea4c1409a3a68fb33706b49e272809b5079 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 19:29:00 +0200 Subject: [PATCH 3/7] fix(service-patterns): match library ids on identifier boundaries Distilled from #1245 by Andrew Hundt (d15071c9, the match_qn half). match_qn was a raw strstr(qn, library_id). Short ids fired inside unrelated words, and a service kind REPLACES the plain CALLS edge, so a false match both invents a service edge and deletes a real call: "gin." in plugin. -> route registration; any "/"-prefixed first argument then minted a Route node "nconf" in encryptionconfig -> CONFIGURES for every call into the package "tonic" in Monotonic -> gRPC; unparseable, so NO edge at all "express" in expression -> route registration "get_env" in get_envelope, get_environ -> CONFIGURES "dio" "surf" "curl" "resty" "phin" "hyper" "treq" "rocket" in studio, surface, curly, restyle, dolphin, hyperbolic, streq, rocketmq The rule (qn_hit_on_boundary; every occurrence of an id is tried, not only the first). A separator is any character that is not an ASCII letter or digit. before: start of string | separator | the id itself starts with a separator ("@trpc/server") | the id starts with an uppercase letter -- a capital opens a new CamelCase word whatever precedes it (AsyncHttpClient, IHttpClientFactory, NSURLSession) after: end of string | separator | the id itself ends in a separator ("gin.") | uppercase letter (GuzzleHttp, FeignClient, KafkaProducer) | digit (urllib2, Mint.HTTP2, amqp091-go) Rejected: a lowercase- or digit-initial id glued to a preceding letter or digit, and any id continued by a lowercase letter. Two deliberate differences from the upstream hunk: upstream's before-rule needed a GuzzleHttp table entry and would have dropped IHttpClientFactory and NSURLSession, and without the digit after-boundary urllib2 and amqp091-go stop matching. All three match on main today. No recall is traded for this. No boundary rule can tell "grequests" from "myrequests", so a library whose own name glues a prefix or suffix onto another id gets an explicit entry with the kind (and broker) it had on main -- 47 of them, found mechanically by running main's matcher and the new one over 173 candidate names and listing every one that went from a kind to NONE: HTTP grequests txrequests redaxios gaxios libcurl curlpp curlcpp hyperlocal guzzlehttp ASYNC aiokafka pykafka rskafka librdkafka rdkafkacpp aioamqp amqpstorm pamqp pyamqp amqprs amqpcpp pynats jnats aiomqtt amqtt hbmqtt umqtt mqttools emqtt rumqtt gomqtt libmosquitto mosquittopp pubsublite gocelery CONFIG wgetenv getenvb qgetenv dotenvx phpdotenv ROUTE_REG apiflask honox fasthttprouter GRAPHQL gqlparser gqlgenc aiogqlc Left at NONE on purpose: names that are not clients of the id they contain and were misclassified on main -- hypercorn, hyperlink, hyperopt, openresty, gqlalchemy, celeryconfig, kafkacat, gnatsd, pypubsub. One visible reclassification: hyperium/tonic was HTTP through "hyper" inside "hyperium" and is now gRPC through "tonic". RED, production reverted and the final tests kept: infrascan_service_pattern_match_rejects_ids_inside_words FAIL tests/test_infrascan.c:105: svc_case_mismatches(cases) == 28, expected 0 == 0 4 passed, 1 failed All 28 negatives mismatch on main. The 47 glued-library positives are RED the other way round: against the boundary rule without the entries. Each positive uses a name containing no second id, so it binds to its own entry -- "aiokafka.AIOKafkaProducer" would have passed through "KafkaProducer" and proven nothing. GREEN: infrascan 5, pipeline 281, parallel 74, extraction 350, edge_types_probe 59, route_canon 11, lang_contract 41, cross_repo 8, registry 64, mcp 318 (4 Windows-only skips). 0 failed. Effect on real code -- production binaries, main 92abefa3 against this change. main against main differs by 0 edges and 0 nodes on all three corpora, so every delta below is the change: django CONFIGURES 76 -> 74; both calls return as CALLS (geom.get_envelope, WSGIRequestHandler.get_environ) typescript CONFIGURES 12,026 -> 12,024; one bogus Route "/*type*/" gone with its CALLS and HANDLES; 3 calls return as CALLS kubernetes CONFIGURES 7,962 -> 7,609: all 353 are "nconf" inside encryptionconfig / authorizationconfig / authenticationconfig, and all 353 return as CALLS. 8 bogus Routes gone -- os.ReadFile of the serviceaccount namespace file, t.Run("/TwoWay"), "/tmp/test" -- with 26 CALLS, 2 HANDLES and 21 TESTS edges that pointed at them. 374 plain CALLS restored; 8 of them (validate.Monotonic and friends) had NO edge at all on main. GRPC_CALLS 60 -> 60. HTTP_CALLS 398 -> 407: nine calls main swallowed as route registrations now reach the existing arg_url heuristic like every other package. 88 of the 408 removed service-derived edges were read at the call site -- all of django and typescript, 82 of 402 in kubernetes: 88 false positives, 0 true positives lost. None of the three corpora uses a glued-name library, so the 47 entries are covered by the unit test only. Known limits: "_" is a separator, so optical_fiber.len still matches "fiber." exactly as requests_get matches "requests" by design; a lowercase id followed by a capital matches (kafkaProducer), which also admits a dioXide-style name; a user wrapper glued without a separator (mygetenv) is no longer classified; a glued-name library missing from the list is NONE until someone adds it. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- internal/cbm/service_patterns.c | 124 ++++++++++++++++++++-- tests/test_infrascan.c | 183 +++++++++++++++++++++++++++++++- 2 files changed, 295 insertions(+), 12 deletions(-) diff --git a/internal/cbm/service_patterns.c b/internal/cbm/service_patterns.c index 0dd4048771..c2425e09e1 100644 --- a/internal/cbm/service_patterns.c +++ b/internal/cbm/service_patterns.c @@ -140,6 +140,20 @@ static const lib_pattern_t http_libraries[] = { {"socket.http", CBM_SVC_HTTP, NULL}, {"resty.http", CBM_SVC_HTTP, NULL}, + /* Glued-name libraries. match_qn needs an identifier boundary around an + * id, so a library whose own name glues a lowercase prefix/suffix onto + * another id ("grequests", "curlpp") is listed explicitly: no boundary + * rule can tell "grequests" from "myrequests". */ + {"grequests", CBM_SVC_HTTP, NULL}, /* Python: gevent + requests */ + {"txrequests", CBM_SVC_HTTP, NULL}, /* Python: Twisted + requests */ + {"redaxios", CBM_SVC_HTTP, NULL}, /* JS: axios-compatible fetch client */ + {"gaxios", CBM_SVC_HTTP, NULL}, /* JS: Google's axios-style client */ + {"libcurl", CBM_SVC_HTTP, NULL}, /* C, node-libcurl */ + {"curlpp", CBM_SVC_HTTP, NULL}, /* C++ libcurl wrapper */ + {"curlcpp", CBM_SVC_HTTP, NULL}, /* C++ libcurl wrapper */ + {"hyperlocal", CBM_SVC_HTTP, NULL}, /* Rust: hyper over unix sockets */ + {"guzzlehttp", CBM_SVC_HTTP, NULL}, /* PHP: Guzzle's composer vendor */ + {NULL, CBM_SVC_NONE, NULL}, }; @@ -263,6 +277,33 @@ static const lib_pattern_t async_libraries[] = { {"dapr.clients.grpc", CBM_SVC_ASYNC, "dapr"}, {"DaprClient", CBM_SVC_ASYNC, "dapr"}, + /* Glued-name libraries (see the note in http_libraries). */ + {"aiokafka", CBM_SVC_ASYNC, "kafka"}, + {"pykafka", CBM_SVC_ASYNC, "kafka"}, + {"librdkafka", CBM_SVC_ASYNC, "kafka"}, + {"rdkafkacpp", CBM_SVC_ASYNC, "kafka"}, + {"rskafka", CBM_SVC_ASYNC, "kafka"}, + {"aioamqp", CBM_SVC_ASYNC, "rabbitmq"}, + {"amqpstorm", CBM_SVC_ASYNC, "rabbitmq"}, + {"pamqp", CBM_SVC_ASYNC, "rabbitmq"}, + {"pyamqp", CBM_SVC_ASYNC, "rabbitmq"}, + {"amqprs", CBM_SVC_ASYNC, "rabbitmq"}, + {"amqpcpp", CBM_SVC_ASYNC, "rabbitmq"}, + {"pynats", CBM_SVC_ASYNC, "nats"}, + {"jnats", CBM_SVC_ASYNC, "nats"}, + {"aiomqtt", CBM_SVC_ASYNC, "mqtt"}, + {"amqtt", CBM_SVC_ASYNC, "mqtt"}, + {"hbmqtt", CBM_SVC_ASYNC, "mqtt"}, + {"umqtt", CBM_SVC_ASYNC, "mqtt"}, + {"mqttools", CBM_SVC_ASYNC, "mqtt"}, + {"emqtt", CBM_SVC_ASYNC, "mqtt"}, + {"rumqtt", CBM_SVC_ASYNC, "mqtt"}, + {"gomqtt", CBM_SVC_ASYNC, "mqtt"}, + {"libmosquitto", CBM_SVC_ASYNC, "mqtt"}, + {"mosquittopp", CBM_SVC_ASYNC, "mqtt"}, + {"pubsublite", CBM_SVC_ASYNC, "pubsub"}, + {"gocelery", CBM_SVC_ASYNC, "celery"}, + {NULL, CBM_SVC_NONE, NULL}, }; @@ -308,6 +349,13 @@ static const lib_pattern_t config_libraries[] = { {"Application.get_env", CBM_SVC_CONFIG, NULL}, {"Application.fetch_env", CBM_SVC_CONFIG, NULL}, + /* Glued-name accessors (see the note in http_libraries). */ + {"wgetenv", CBM_SVC_CONFIG, NULL}, /* Windows CRT: _wgetenv, _wgetenv_s */ + {"getenvb", CBM_SVC_CONFIG, NULL}, /* Python: os.getenvb */ + {"qgetenv", CBM_SVC_CONFIG, NULL}, /* Qt */ + {"dotenvx", CBM_SVC_CONFIG, NULL}, /* JS: @dotenvx/dotenvx */ + {"phpdotenv", CBM_SVC_CONFIG, NULL}, /* PHP: vlucas/phpdotenv */ + {NULL, CBM_SVC_NONE, NULL}, }; @@ -377,6 +425,11 @@ static const lib_pattern_t route_reg_libraries[] = { {"akka.http.scaladsl.server", CBM_SVC_ROUTE_REG, NULL}, {"play.api.routing", CBM_SVC_ROUTE_REG, NULL}, + /* Glued-name frameworks (see the note in http_libraries). */ + {"apiflask", CBM_SVC_ROUTE_REG, NULL}, /* Python: Flask-based */ + {"honox", CBM_SVC_ROUTE_REG, NULL}, /* JS: Hono meta-framework */ + {"fasthttprouter", CBM_SVC_ROUTE_REG, NULL}, /* Go: httprouter for fasthttp */ + {NULL, CBM_SVC_NONE, NULL}, }; @@ -448,6 +501,11 @@ static const lib_pattern_t graphql_libraries[] = { {"async-graphql", CBM_SVC_GRAPHQL, NULL}, {"juniper", CBM_SVC_GRAPHQL, NULL}, + /* Glued-name libraries (see the note in http_libraries). */ + {"gqlparser", CBM_SVC_GRAPHQL, NULL}, /* Go: vektah/gqlparser */ + {"gqlgenc", CBM_SVC_GRAPHQL, NULL}, /* Go: gqlgen client generator */ + {"aiogqlc", CBM_SVC_GRAPHQL, NULL}, /* Python: asyncio GraphQL client */ + {NULL, CBM_SVC_NONE, NULL}, }; @@ -534,17 +592,67 @@ static const method_suffix_t method_suffixes[] = { /* ── Matching implementation ───────────────────────────────────── */ -/* Check if any library identifier appears as a substring in the QN. - * Case-sensitive: "requests" matches "project.venv.requests.api.get" - * but not "Requests". Library names are specific enough to avoid - * false positives even with substring matching. */ +static bool qn_is_lower(char ch) { + return ch >= 'a' && ch <= 'z'; +} + +static bool is_digit_char(char ch) { + return ch >= '0' && ch <= '9'; +} + +static bool qn_is_alnum(char ch) { + return qn_is_lower(ch) || is_digit_char(ch) || (ch >= 'A' && ch <= 'Z'); +} + +/* True when the occurrence of `id` at `hit` sits on identifier boundaries. + * + * A raw substring match let short ids fire inside unrelated words: "gin." + * in "plugin.", "dio" in "studio", "surf" in "surface", "express" in + * "expression". Every character that is not an ASCII letter or digit is a + * separator ('.', '/', '\\', ':', '_', '-', '$', '@', ...). + * + * BEFORE: start of string, a separator, an id that itself starts with a + * separator ("@trpc/server"), or an id that starts with an + * uppercase letter — a capital opens a new CamelCase word whatever + * precedes it ("AsyncHttpClient", "IHttpClientFactory", + * "NSURLSession"). Rejected: a lowercase/digit-initial id glued to + * a letter or digit ("plugin." / "studio" / "myrequests"). + * AFTER: end of string, a separator, an id that itself ends in a + * separator ("gin."), an uppercase letter (CamelCase glue: + * "GuzzleHttp", "FeignClient", "KafkaProducer", "kafkaProducer") + * or a digit (version suffix: "urllib2", "amqp091-go"). + * Rejected: a lowercase letter continuing the word ("surface", + * "curly", "expression"). */ +static bool qn_hit_on_boundary(const char *qn, const char *hit, const char *id, size_t id_len) { + char first = id[0]; + char last = id[id_len - 1]; + if (hit > qn && qn_is_alnum(hit[-1]) && (qn_is_lower(first) || is_digit_char(first))) { + return false; + } + if (qn_is_alnum(last) && qn_is_lower(hit[id_len])) { + return false; + } + return true; +} + +/* Check if any library identifier appears in the QN on identifier + * boundaries (see qn_hit_on_boundary). Case-sensitive: "requests" matches + * "project.venv.requests.api.get" and "svc.requests_get" but neither + * "Requests" nor "myrequests". */ static const lib_pattern_t *match_qn(const char *qn, const lib_pattern_t *patterns) { if (!qn || !qn[0]) { return NULL; } for (int i = 0; patterns[i].library_id != NULL; i++) { - if (strstr(qn, patterns[i].library_id) != NULL) { - return &patterns[i]; + const char *id = patterns[i].library_id; + size_t id_len = strlen(id); + if (id_len == 0) { + continue; + } + for (const char *hit = strstr(qn, id); hit != NULL; hit = strstr(hit + 1, id)) { + if (qn_hit_on_boundary(qn, hit, id, id_len)) { + return &patterns[i]; + } } } return NULL; @@ -574,10 +682,6 @@ static bool contains_segment(const char *path, const char *segment) { return false; } -static bool is_digit_char(char ch) { - return ch >= '0' && ch <= '9'; -} - static bool has_http_route_marker(const char *path) { if (starts_with_segment(path, "api") || starts_with_segment(path, "apis") || starts_with_segment(path, "graphql") || starts_with_segment(path, "health") || diff --git a/tests/test_infrascan.c b/tests/test_infrascan.c index cd7df945d0..a9bf1aef94 100644 --- a/tests/test_infrascan.c +++ b/tests/test_infrascan.c @@ -1,11 +1,11 @@ #include "test_framework.h" #include "graph_buffer/graph_buffer.h" #include "pipeline/pipeline_internal.h" +#include "service_patterns.h" #include #include - -bool cbm_service_pattern_is_http_route_literal(const char *literal, const char *callee_name); +#include static int has_data_flow(cbm_gbuf_t *gb, int64_t source_id, int64_t target_id) { const cbm_gbuf_edge_t **edges = NULL; @@ -48,6 +48,183 @@ TEST(infrascan_http_route_literal_guard_rejects_filesystem_paths) { PASS(); } +/* ── Service-pattern QN boundaries (distilled from PR #1245) ───────── + * A library id must sit on an identifier boundary inside the QN. A raw + * substring match classified "proj.plugin.loader" as gin ("gin.") and + * "proj.studio.x" as Dio ("dio"), which minted bogus Route / HTTP_CALLS + * edges. The rule must stay open for CamelCase glue ("GuzzleHttp", + * "FeignClient", "KafkaProducer", "IHttpClientFactory") and for version + * suffixes ("urllib2", "amqp091-go"). */ +typedef struct { + const char *qn; + cbm_svc_kind_t want; +} svc_case_t; + +static int svc_case_mismatches(const svc_case_t *cases) { + int bad = 0; + for (int i = 0; cases[i].qn != NULL; i++) { + cbm_svc_kind_t got = cbm_service_pattern_match(cases[i].qn); + if (got != cases[i].want) { + printf(" service-pattern mismatch: \"%s\" -> %d, expected %d\n", cases[i].qn, (int)got, + (int)cases[i].want); + bad++; + } + } + return bad; +} + +TEST(infrascan_service_pattern_match_rejects_ids_inside_words) { + static const svc_case_t cases[] = { + /* "gin." inside plugin. */ + {"proj.plugins.autorun.tests.test_plugin._dispatch", CBM_SVC_NONE}, + {"proj.plugin.loader", CBM_SVC_NONE}, + /* "chi." / "echo." / "fiber." */ + {"proj.hibachi.grill", CBM_SVC_NONE}, + {"sound.reecho.apply", CBM_SVC_NONE}, + {"textile.microfiber.weave", CBM_SVC_NONE}, + /* "dio" inside studio / radio */ + {"proj.studio.x", CBM_SVC_NONE}, + {"radio.tune", CBM_SVC_NONE}, + /* "surf" / "resty" / "curl" / "phin" / "hyper" / "sling" / "needle" / "treq" */ + {"app.surface.draw", CBM_SVC_NONE}, + {"ui.restyle.apply", CBM_SVC_NONE}, + /* control: "forestry" spells "restr", never contained "resty" */ + {"pkg.forestry.trees", CBM_SVC_NONE}, + {"util.curly.brace", CBM_SVC_NONE}, + {"sea.dolphin.swim", CBM_SVC_NONE}, + {"math.hyperbolic.tanh", CBM_SVC_NONE}, + {"farm.gosling.walk", CBM_SVC_NONE}, + {"sewing.needles.sort", CBM_SVC_NONE}, + {"util.streq", CBM_SVC_NONE}, + /* "requests" glued to a lowercase prefix (upstream's case) */ + {"proj.myrequests.client.get", CBM_SVC_NONE}, + /* "express" / "rocket" / "tonic" / "nconf" */ + {"ast.expression.eval", CBM_SVC_NONE}, + {"mq.rocketmq.send", CBM_SVC_NONE}, + {"geo.tectonic.plates", CBM_SVC_NONE}, + {"app.unconfigured.warn", CBM_SVC_NONE}, + {"cli.runconfig.load", CBM_SVC_NONE}, + {"aws.s3express.session", CBM_SVC_NONE}, + /* not clients of the id they contain: ASGI server, URL library, + * nginx platform, Memgraph OGM, Celery settings module, a word */ + {"hypercorn.run.serve", CBM_SVC_NONE}, + {"hyperlink.URL.from_text", CBM_SVC_NONE}, + {"openresty.core.request", CBM_SVC_NONE}, + {"gqlalchemy.Memgraph.execute", CBM_SVC_NONE}, + {"celeryconfig.broker_url", CBM_SVC_NONE}, + {"kafkaesque.story.tell", CBM_SVC_NONE}, + {NULL, CBM_SVC_NONE}, + }; + ASSERT_EQ(svc_case_mismatches(cases), 0); + PASS(); +} + +TEST(infrascan_service_pattern_match_keeps_real_library_qns) { + static const svc_case_t cases[] = { + /* Route registration: ids that end in a separator, own segment */ + {"github.com/gin-gonic/gin.Default", CBM_SVC_ROUTE_REG}, + {"proj.gin.router.GET", CBM_SVC_ROUTE_REG}, + {"gin.GET", CBM_SVC_ROUTE_REG}, + {"chi.NewRouter", CBM_SVC_ROUTE_REG}, + {"github.com/labstack/echo.New", CBM_SVC_ROUTE_REG}, + {"fiber.New", CBM_SVC_ROUTE_REG}, + {"proj.express.router.get", CBM_SVC_ROUTE_REG}, + {"@hapi/hapi.server.route", CBM_SVC_ROUTE_REG}, + /* HTTP: own segment / separator-joined wrapper */ + {"proj.venv.requests.api.get", CBM_SVC_HTTP}, + {"proj.service.requests_get", CBM_SVC_HTTP}, + {"package:dio/dio.dart", CBM_SVC_HTTP}, + {"dio.Dio.get", CBM_SVC_HTTP}, + {"pkg.net.curl.get", CBM_SVC_HTTP}, + {"curl_exec", CBM_SVC_HTTP}, + {"surf.get", CBM_SVC_HTTP}, + {"hyper.Client.request", CBM_SVC_HTTP}, + {"phin.promisified", CBM_SVC_HTTP}, + {"needle.get", CBM_SVC_HTTP}, + {"github.com/go-resty/resty.New", CBM_SVC_HTTP}, + {"github.com/dghubble/sling.New", CBM_SVC_HTTP}, + /* HTTP: CamelCase glue after the id */ + {"GuzzleHttp\\Client", CBM_SVC_HTTP}, + {"proj.GuzzleHttp.Client.get", CBM_SVC_HTTP}, + {"org.springframework.cloud.openfeign.FeignClientBuilder.build", CBM_SVC_HTTP}, + {"com.acme.net.HttpClientFactory.create", CBM_SVC_HTTP}, + /* HTTP: CamelCase glue before the id */ + {"System.Net.Http.IHttpClientFactory.CreateClient", CBM_SVC_HTTP}, + {"org.asynchttpclient.AsyncHttpClient.prepareGet", CBM_SVC_HTTP}, + {"Foundation.NSURLSession.dataTask", CBM_SVC_HTTP}, + /* HTTP: version suffix after the id */ + {"urllib2.urlopen", CBM_SVC_HTTP}, + {"Mint.HTTP2.request", CBM_SVC_HTTP}, + /* Async */ + {"com.acme.events.KafkaProducer.send", CBM_SVC_ASYNC}, + {"this.kafkaProducer.send", CBM_SVC_ASYNC}, + {"org.apache.kafka.clients.producer.KafkaProducer.send", CBM_SVC_ASYNC}, + {"github.com/rabbitmq/amqp091-go.Channel.Publish", CBM_SVC_ASYNC}, + {"github.com/nats-io/nats.go.Conn.Publish", CBM_SVC_ASYNC}, + /* Config */ + {"os.getenv", CBM_SVC_CONFIG}, + {"os.Getenv", CBM_SVC_CONFIG}, + /* Glued-name libraries: the library's own name glues a lowercase + * prefix/suffix onto another id, so each has an explicit table entry. + * Every QN below contains NO other id — only the entry can match. */ + {"grequests.map", CBM_SVC_HTTP}, + {"txrequests.Session.send", CBM_SVC_HTTP}, + {"redaxios.create", CBM_SVC_HTTP}, + {"gaxios.request", CBM_SVC_HTTP}, + {"libcurl.version", CBM_SVC_HTTP}, + {"node-libcurl.Easy.perform", CBM_SVC_HTTP}, + {"curlpp.Easy.perform", CBM_SVC_HTTP}, + {"curlcpp.easy.perform", CBM_SVC_HTTP}, + {"hyperlocal.UnixConnector.call", CBM_SVC_HTTP}, + {"guzzlehttp/psr7.Utils.streamFor", CBM_SVC_HTTP}, + {"aiokafka.helpers.create_ssl_context", CBM_SVC_ASYNC}, + {"pykafka.topic.Topic.get_producer", CBM_SVC_ASYNC}, + {"librdkafka.rd_version", CBM_SVC_ASYNC}, + {"rdkafkacpp.version", CBM_SVC_ASYNC}, + {"rskafka.client.partition.write", CBM_SVC_ASYNC}, + {"aioamqp.connect", CBM_SVC_ASYNC}, + {"amqpstorm.Connection.channel", CBM_SVC_ASYNC}, + {"pamqp.frame.marshal", CBM_SVC_ASYNC}, + {"kombu.transport.pyamqp.Transport.establish_connection", CBM_SVC_ASYNC}, + {"amqprs.channel.basic_publish", CBM_SVC_ASYNC}, + {"amqpcpp.TcpChannel.publish", CBM_SVC_ASYNC}, + {"pynats.client.send", CBM_SVC_ASYNC}, + {"jnats.options.build", CBM_SVC_ASYNC}, + {"aiomqtt.client.publish", CBM_SVC_ASYNC}, + {"amqtt.session.deliver", CBM_SVC_ASYNC}, + {"hbmqtt.session.deliver", CBM_SVC_ASYNC}, + {"umqtt.simple.publish", CBM_SVC_ASYNC}, + {"mqttools.client.publish", CBM_SVC_ASYNC}, + {"emqtt.publish", CBM_SVC_ASYNC}, + {"rumqtt.client.publish", CBM_SVC_ASYNC}, + {"github.com/256dpi/gomqtt/client.Publish", CBM_SVC_ASYNC}, + {"libmosquitto.publish", CBM_SVC_ASYNC}, + {"mosquittopp.publish", CBM_SVC_ASYNC}, + {"google.cloud.pubsublite.admin.create_topic", CBM_SVC_ASYNC}, + {"gocelery.client.delay", CBM_SVC_ASYNC}, + {"_wgetenv", CBM_SVC_CONFIG}, + {"_wgetenv_s", CBM_SVC_CONFIG}, + {"os.getenvb", CBM_SVC_CONFIG}, + {"qgetenv", CBM_SVC_CONFIG}, + {"@dotenvx/dotenvx.config", CBM_SVC_CONFIG}, + {"vlucas/phpdotenv.load", CBM_SVC_CONFIG}, + {"apiflask.scaffold.get", CBM_SVC_ROUTE_REG}, + {"honox.factory.createRoute", CBM_SVC_ROUTE_REG}, + {"fasthttprouter.Router.GET", CBM_SVC_ROUTE_REG}, + {"github.com/vektah/gqlparser/v2.LoadSchema", CBM_SVC_GRAPHQL}, + {"github.com/Yamashou/gqlgenc.client.Post", CBM_SVC_GRAPHQL}, + {"aiogqlc.client.execute", CBM_SVC_GRAPHQL}, + {NULL, CBM_SVC_NONE}, + }; + ASSERT_EQ(svc_case_mismatches(cases), 0); + ASSERT_STR_EQ(cbm_service_pattern_broker("aiokafka.helpers.create_ssl_context"), "kafka"); + ASSERT_STR_EQ(cbm_service_pattern_broker("umqtt.simple.publish"), "mqtt"); + ASSERT_STR_EQ(cbm_service_pattern_broker("com.acme.events.KafkaProducer.send"), "kafka"); + ASSERT_STR_EQ(cbm_service_pattern_broker("github.com/rabbitmq/amqp091-go.Channel.Publish"), + "rabbitmq"); + PASS(); +} + TEST(infrascan_route_nodes_skip_bad_http_url_paths) { cbm_gbuf_t *gb = cbm_gbuf_new("test", "/tmp/cbm_infrascan_route_guard"); ASSERT_NOT_NULL(gb); @@ -128,6 +305,8 @@ TEST(infrascan_http_calls_join_matching_handler_route) { SUITE(infrascan) { RUN_TEST(infrascan_http_route_literal_guard_rejects_filesystem_paths); + RUN_TEST(infrascan_service_pattern_match_rejects_ids_inside_words); + RUN_TEST(infrascan_service_pattern_match_keeps_real_library_qns); RUN_TEST(infrascan_route_nodes_skip_bad_http_url_paths); RUN_TEST(infrascan_http_calls_join_matching_handler_route); } From 343d7dd5e47b817999098605370f0eab706675f0 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 19:45:31 +0200 Subject: [PATCH 4/7] fix(extract): a C-family function's return type keeps its pointer and qualifiers Distilled from #1245 by Andrew Hundt (c51e5cb0, the extract_defs.c half; the mcp.c and token-reduction parts are not taken). For C-family functions return_type was the text of the declaration's `type` node alone. In these grammars the `*`, `&` and `&&` live on the declarator and cv-qualifiers are sibling nodes, so const char *get_name(void) published char robj **lookupKeys(...) published robj Text &operator=(const Text &) published Text and no C or C++ function in any index had a pointer return type at all. The C/C++ resolver's return-type parser already strips a leading const/volatile and handles trailing `*`, `&`, `&&` -- branches that were unreachable for functions, so every `Foo *f()` was registered as returning Foo by value. c_declared_return_type() renders the declared type at both sites (functions and methods) for C, C++, CUDA, GLSL, HLSL, ISPC, Slang and Objective-C -- the list resolve_func_name_c_family uses: so `const char *`, `char **`, `Text &`, `Text &&`, `Text *&`, `volatile unsigned long *`; a pointer-level qualifier follows its star (`char *const *`); east-const `char const *` normalises to `const char *`. With no qualifier and no marker the old text is returned unchanged. It measures, then writes into one exact-size arena allocation; the declarator walk is a plain child chain, no recursion. def.return_types is untouched. Two deliberate differences from the upstream hunk. Upstream copied every type_qualifier node, which in these grammars includes constexpr, _Noreturn, mutable and __extension__ -- `constexpr int f()` would have become `constexpr int`. Only const, volatile, restrict, __restrict, __restrict__ and _Atomic are kept, and both cases are asserted unchanged. And the walk stops at the first declarator that is neither pointer nor reference, so `int (*f(void))(int)` stays `int` rather than becoming a wrong `int *`. RED, production reverted and the final tests kept: c_function_return_type_preserves_pointer_and_qualifier FAIL tests/test_extraction.c:1005: "char" != "const char *" cpp_method_return_type_preserves_pointer_and_qualifier FAIL tests/test_extraction.c:1072: "char" != "const char *" 351 passed, 2 failed c_function_return_type_plain_unchanged is the control: it passes before and after. GREEN: extraction 353, c_lsp 762, lang_contract 41, pipeline 281. Effect on real code -- Redis, production binaries, old against new. 39,132 nodes in both. 1,442 Function nodes change, in return_type only: void -> void * (270), char -> const char * (101), unsigned char -> unsigned char * (91), char -> char * (81), robj -> robj * (57), ... Functions with a `*` in return_type: 0 -> 1,440. With one worker the complete edge set apart from SEMANTICALLY_RELATED is byte-identical old against new across six runs (three each) -- CALLS, USAGE, WRITES, IMPORTS and the rest do not move. SEMANTICALLY_RELATED goes 350 -> 325 because that pass tokenises the return-type string into its type vector. One thing measured rather than assumed. With several workers, which `dict.h` Redis' src/dict.c imports (src/ or deps/hiredis/) depends on worker merge order. That is main's defect, not this change's: unmodified main picked src/dict.h in 2 of 10 runs and the hiredis header in 8; with one worker both binaries always pick src/dict.h. This change picked src/dict.h in 6 of 10. At ten runs a side that gap is not separable from chance (Fisher exact, two-sided p = 0.17), and a shifted rate would be unsurprising for a timing-dependent pick; it is recorded here rather than explained. The include-target fix is a separate change (#2227). Known limits, all pre-existing and out of scope: struct and class FIELDS have the same defect (`const char *data;` publishes char); C prototypes and declaration-only C++ methods are not extracted as definitions, so they are not reached; where a macro is misparsed as the type (`LUA_API const char *f()`), the base was already wrong and now reads `const LUA_API *`; the resolver's parser does not yet understand a pointer-level qualifier or a leading _Atomic/restrict and falls back to a named-type lookup for those, as it effectively did for every pointer return before. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- internal/cbm/extract_defs.c | 157 +++++++++++++++++++++++++++++++++++- tests/test_extraction.c | 98 ++++++++++++++++++++++ 2 files changed, 253 insertions(+), 2 deletions(-) diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 93da084be1..916f1136f9 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -3477,6 +3477,159 @@ static TSNode find_function_params(TSNode func_node, CBMLanguage lang) { return params; } +/* ── C-family declared return type ────────────────────────────────── + * The C-family grammars split a declared return type three ways: the `type` + * field (`char`), sibling type_qualifier nodes (`const`), and the + * pointer/reference declarators wrapping the function declarator (`*`). Taking + * only the `type` field published `const char *get_name(void)` as "char". + * + * Canonical spelling: leading cv-qualifiers in source order, the base type text + * verbatim, then one space and the declarator markers outermost-first with no + * space between them — `const char *`, `char **`, `Text &`, `Text *&`. A + * qualifier on a pointer level follows its `*` and is separated from the next + * marker by a space: `char *const *`. A qualifier written after the base type + * (`char const *`) is normalized to the leading position. */ + +/* Output sink: measures when buf is NULL, writes otherwise. Rendering twice + * sizes the arena allocation exactly without a second copy of the logic. */ +typedef struct { + char *buf; + size_t len; +} c_rt_out_t; + +static void c_rt_put(c_rt_out_t *out, const char *text, size_t n) { + if (out->buf) { + memcpy(out->buf + out->len, text, n); + } + out->len += n; +} + +static void c_rt_put_str(c_rt_out_t *out, const char *text) { + c_rt_put(out, text, strlen(text)); +} + +static void c_rt_put_node(c_rt_out_t *out, TSNode node, const char *source) { + uint32_t start = ts_node_start_byte(node); + uint32_t end = ts_node_end_byte(node); + if (end > start) { + c_rt_put(out, source + start, (size_t)(end - start)); + } +} + +/* type_qualifier also covers keywords that are not part of the type + * (`constexpr`, `_Noreturn`, `mutable`, `__extension__`, …). Keep only the ones + * that are, so `constexpr int f()` still returns "int". */ +static bool is_c_return_cv_qualifier(TSNode node, const char *source) { + static const char *const kept[] = {"const", "volatile", "restrict", "__restrict", + "__restrict__", "_Atomic", NULL}; + if (strcmp(ts_node_type(node), "type_qualifier") != 0) { + return false; + } + uint32_t start = ts_node_start_byte(node); + uint32_t end = ts_node_end_byte(node); + size_t len = end > start ? (size_t)(end - start) : 0; + for (const char *const *k = kept; *k; k++) { + if (strlen(*k) == len && memcmp(source + start, *k, len) == 0) { + return true; + } + } + return false; +} + +static bool is_c_declarator_lang(CBMLanguage lang) { + return lang == CBM_LANG_C || lang == CBM_LANG_CPP || lang == CBM_LANG_CUDA || + lang == CBM_LANG_GLSL || lang == CBM_LANG_HLSL || lang == CBM_LANG_ISPC || + lang == CBM_LANG_SLANG || lang == CBM_LANG_OBJC; +} + +/* Render the canonical return type into `out`; returns how many qualifiers and + * markers were added around the base type (0 = the base text alone is already + * the whole type). The declarator walk is one strict child chain, so it is + * O(depth) with no recursion and needs no depth cap. It stops at the first node + * that is neither a pointer nor a reference declarator: for a function returning + * a function pointer (`int (*f(void))(int)`) that is the outer + * function_declarator, which leaves the base type as it was. */ +static size_t c_rt_render(c_rt_out_t *out, TSNode func_node, TSNode type_node, TSNode declarator, + const char *source) { + size_t added = 0; + uint32_t decl_start = ts_node_start_byte(declarator); + uint32_t nc = ts_node_named_child_count(func_node); + for (uint32_t i = 0; i < nc; i++) { + TSNode ch = ts_node_named_child(func_node, i); + if (ts_node_start_byte(ch) >= decl_start) { + break; + } + if (is_c_return_cv_qualifier(ch, source)) { + c_rt_put_node(out, ch, source); + c_rt_put_str(out, " "); + added++; + } + } + c_rt_put_node(out, type_node, source); + + bool need_space = true; + TSNode decl = declarator; + while (!ts_node_is_null(decl)) { + const char *dk = ts_node_type(decl); + bool is_ref = strcmp(dk, "reference_declarator") == 0; + if (!is_ref && strcmp(dk, "pointer_declarator") != 0) { + break; + } + if (need_space) { + c_rt_put_str(out, " "); + need_space = false; + } + /* A reference_declarator opens with its `&` / `&&` token. */ + TSNode marker = ts_node_child(decl, 0); + if (is_ref && !ts_node_is_null(marker) && !ts_node_is_named(marker)) { + c_rt_put_node(out, marker, source); + } else { + c_rt_put_str(out, is_ref ? "&" : "*"); + } + added++; + uint32_t dn = ts_node_named_child_count(decl); + for (uint32_t i = 0; i < dn; i++) { + TSNode q = ts_node_named_child(decl, i); + if (is_c_return_cv_qualifier(q, source)) { + c_rt_put_node(out, q, source); + need_space = true; + added++; + } + } + /* tree-sitter-cpp/-cuda give a reference_declarator's inner declarator no + * `declarator` field (see find_c_params); it is the one named child. */ + TSNode inner = ts_node_child_by_field_name(decl, TS_FIELD("declarator")); + if (ts_node_is_null(inner) && is_ref && dn > 0) { + inner = ts_node_named_child(decl, 0); + } + decl = inner; + } + return added; +} + +/* Declared return type of a C-family function/method node whose `type` field is + * `type_node`. Any other language, and any type with nothing around its base + * type, gets the base type text exactly as before. */ +static char *c_declared_return_type(CBMExtractCtx *ctx, TSNode func_node, TSNode type_node) { + CBMArena *a = ctx->arena; + TSNode declarator = ts_node_child_by_field_name(func_node, TS_FIELD("declarator")); + if (!is_c_declarator_lang(ctx->language) || ts_node_is_null(declarator)) { + return cbm_node_text(a, type_node, ctx->source); + } + c_rt_out_t out = {NULL, 0}; + if (c_rt_render(&out, func_node, type_node, declarator, ctx->source) == 0) { + return cbm_node_text(a, type_node, ctx->source); + } + out.buf = (char *)cbm_arena_alloc(a, out.len + NULL_TERM); + if (!out.buf) { + return cbm_node_text(a, type_node, ctx->source); + } + out.len = 0; + (void)c_rt_render(&out, func_node, type_node, declarator, ctx->source); + out.buf[out.len] = '\0'; + return out.buf; +} + // C++: resolve trailing return type (auto f() -> Type) on a declarator node. // Updates def->return_type and def->return_types if trailing type found. static void resolve_cpp_trailing_return(CBMArena *a, TSNode func_node, const char *source, @@ -3712,7 +3865,7 @@ static void extract_func_def(CBMExtractCtx *ctx, TSNode node, const CBMLangSpec for (const char **f = rt_fields; *f; f++) { TSNode rt = ts_node_child_by_field_name(func_node, *f, (uint32_t)strlen(*f)); if (!ts_node_is_null(rt)) { - def.return_type = cbm_node_text(a, rt, ctx->source); + def.return_type = c_declared_return_type(ctx, func_node, rt); def.return_types = extract_return_types(a, rt, ctx->source, ctx->language); break; } @@ -4964,7 +5117,7 @@ static void push_method_def(CBMExtractCtx *ctx, TSNode child, TSNode class_node, for (const char **f = rt_fields; *f; f++) { TSNode rt = ts_node_child_by_field_name(child, *f, (uint32_t)strlen(*f)); if (!ts_node_is_null(rt)) { - def.return_type = cbm_node_text(a, rt, ctx->source); + def.return_type = c_declared_return_type(ctx, child, rt); break; } } diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 8d9bf742b0..331aa54f3f 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -977,6 +977,65 @@ TEST(c_struct) { PASS(); } +/* return_type of the first definition named `name`; NULL when there is no such + * definition or it carries no return type. */ +static const char *def_return_type(CBMFileResult *r, const char *name) { + for (int i = 0; i < r->defs.count; i++) { + if (strcmp(r->defs.items[i].name, name) == 0) { + return r->defs.items[i].return_type; + } + } + return NULL; +} + +/* PR #1245: the C grammar splits a declared return type across the `type` node, + * sibling type_qualifier nodes and the pointer_declarator chain wrapping the + * function declarator. Taking only the `type` node published `const char *` as + * "char". Canonical spelling: qualifiers, base type, one space, then the + * declarator markers unspaced (`const char *`, `char **`). */ +TEST(c_function_return_type_preserves_pointer_and_qualifier) { + CBMFileResult *r = extract("static const char *text_end(const char *text) { return text; }\n" + "char **table(void) { return 0; }\n" + "char const *east_const(void) { return 0; }\n" + "const struct Point *find_point(void) { return 0; }\n" + "volatile unsigned long *reg(void) { return 0; }\n" + "char *const *frozen(void) { return 0; }\n", + CBM_LANG_C, "t", "returns.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "text_end"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "table"), "char **"); + ASSERT_STR_EQ(def_return_type(r, "east_const"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "find_point"), "const struct Point *"); + ASSERT_STR_EQ(def_return_type(r, "reg"), "volatile unsigned long *"); + ASSERT_STR_EQ(def_return_type(r, "frozen"), "char *const *"); + cbm_free_result(r); + PASS(); +} + +/* Guard for the fix above: a return type with no qualifier and no declarator + * marker is already correct and must come out byte-identical. */ +TEST(c_function_return_type_plain_unchanged) { + CBMFileResult *r = extract("int scalar(void) { return 0; }\n" + "void nothing(void) {}\n" + "unsigned long wide(void) { return 0; }\n" + "struct Point make_point(void) { struct Point p; return p; }\n" + "static inline size_t count(void) { return 0; }\n" + "_Noreturn void die(void) { for (;;) {} }\n", + CBM_LANG_C, "t", "plain.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "scalar"), "int"); + ASSERT_STR_EQ(def_return_type(r, "nothing"), "void"); + ASSERT_STR_EQ(def_return_type(r, "wide"), "unsigned long"); + ASSERT_STR_EQ(def_return_type(r, "make_point"), "struct Point"); + ASSERT_STR_EQ(def_return_type(r, "count"), "size_t"); + /* _Noreturn parses as a type_qualifier but is not part of the type. */ + ASSERT_STR_EQ(def_return_type(r, "die"), "void"); + cbm_free_result(r); + PASS(); +} + /* --- C++ --- */ TEST(cpp_class) { CBMFileResult *r = extract( @@ -990,6 +1049,42 @@ TEST(cpp_class) { PASS(); } +/* PR #1245, C++ side: in-class methods go through a separate extraction path + * from free functions, and C++ adds reference markers (`&`, `&&`) whose + * reference_declarator carries no `declarator` field. */ +TEST(cpp_method_return_type_preserves_pointer_and_qualifier) { + CBMFileResult *r = extract("class Text {\n" + "public:\n" + " const char *end() { return nullptr; }\n" + " char **table() { return nullptr; }\n" + " Text &self() { return *this; }\n" + " const Text &cself() const { return *this; }\n" + " Text *&slot() { return next_; }\n" + " Text *next_;\n" + " int width() const { return 0; }\n" + " constexpr int square(int x) const { return x * x; }\n" + "};\n" + "const Text &shared() { static Text t; return t; }\n" + "Text &&moved(Text &t) { return static_cast(t); }\n" + "const char *Text::c_str() const { return nullptr; }\n", + CBM_LANG_CPP, "t", "text.cpp"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "end"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "table"), "char **"); + ASSERT_STR_EQ(def_return_type(r, "self"), "Text &"); + ASSERT_STR_EQ(def_return_type(r, "cself"), "const Text &"); + ASSERT_STR_EQ(def_return_type(r, "slot"), "Text *&"); + ASSERT_STR_EQ(def_return_type(r, "width"), "int"); + /* constexpr parses as a type_qualifier but is not part of the type. */ + ASSERT_STR_EQ(def_return_type(r, "square"), "int"); + ASSERT_STR_EQ(def_return_type(r, "shared"), "const Text &"); + ASSERT_STR_EQ(def_return_type(r, "moved"), "Text &&"); + ASSERT_STR_EQ(def_return_type(r, "c_str"), "const char *"); + cbm_free_result(r); + PASS(); +} + /* ═══════════════════════════════════════════════════════════════════ * Group C: Scripting / Dynamic Languages * ═══════════════════════════════════════════════════════════════════ */ @@ -8206,8 +8301,11 @@ SUITE(extraction) { RUN_TEST(go_interface); RUN_TEST(zig_function); RUN_TEST(c_function); + RUN_TEST(c_function_return_type_preserves_pointer_and_qualifier); + RUN_TEST(c_function_return_type_plain_unchanged); RUN_TEST(c_struct); RUN_TEST(cpp_class); + RUN_TEST(cpp_method_return_type_preserves_pointer_and_qualifier); /* Scripting */ RUN_TEST(python_function); From a041794f8e51ddf42342899be7b2ac26f951d777 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 22 Sep 2026 01:12:51 +0200 Subject: [PATCH 5/7] fix(daemon): losing the runtime-directory creation race is not a failure on Windows Several processes first-starting together against a runtime directory that does not exist yet -- test-windows-guards' section_cold_storm, or a host that launches more than one MCP server on its very first run -- all observe the final path component absent. One CreateDirectoryW wins; the others get ERROR_ALREADY_EXISTS. win_private_directory_tree_secure() treated that as a failure and recorded no validation detail, so every loser exited with a bare secure CLI coordination could not be created (endpoint) The function called right after the walk, win_runtime_directory_secure(), has always tolerated ERROR_ALREADY_EXISTS for the same directory, and the POSIX walk tolerates EEXIST at the same point; only the Windows walk in front of them did not. Seen four times on 2026-09-21 on unrelated PRs (#1768 three attempts in a row, #2140, #1999, #808). The racy lines date from July; the guard began exercising them on 2026-09-03, when each guard section was given its own empty CBM_RUNTIME_DIR, so the final component is now absent at storm time on every run. The walk now treats ERROR_ALREADY_EXISTS from its own CreateDirectoryW as "the directory I wanted exists". Nothing is trusted because of that: an ancestor still goes through win_directory_component_secure(), and the final component through win_runtime_directory_secure(), which refuses a non-directory or reparse point and enforces owner and DACL. Every refusal on this path now names its component and its rule. The walk reports the Windows error when it can neither create nor inspect a component, and win_runtime_directory_secure() says whether the path could not be created, cannot be inspected, exists but is not a directory, or is a reparse point. One helper owns the wide-to-UTF-8 conversion for these messages and the existing ancestor message now uses it too, so src/daemon/ipc.c stays at its memory-core baseline. Deterministic reproduction, no threads and no timing: a test seam fires in the walk between "component observed absent" and CreateDirectoryW, and the test plays the process that wins the creation. daemon_ipc_windows_private_directory_survives_lost_creation_race daemon_ipc_windows_private_directory_refuses_and_names_a_planted_file Verification. macOS arm64 (ASan+UBSan): build clean, daemon_ipc 53 passed (2 Windows-only skips), daemon_bootstrap 28 passed, lint-memory-core unchanged (ipc.c stays at 146 raw sites). The two new tests are Windows-only and have NOT been run locally: the Windows VM was down when this was written, so the RED run on the seam-only tree and the GREEN run with this fix are delegated to CI's windows-latest legs (test-windows shards, test-windows-guards) by an explicit maintainer decision. If the guard suite's cold storm still fails with this in place, the attribution above is wrong and this reverts. Signed-off-by: Martin Vogel --- src/daemon/ipc.c | 88 ++++++++++++++++++++++++--- src/daemon/ipc_internal.h | 11 ++++ tests/test_daemon_ipc.c | 125 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 7 deletions(-) diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index a92befb080..3917a7a05e 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -214,6 +214,31 @@ static void ipc_startup_gate_run(void) { } #endif +#ifdef _WIN32 +static cbm_daemon_ipc_win_directory_create_hook_fn g_win_directory_create_hook_for_test; +static void *g_win_directory_create_hook_context_for_test; +#endif + +void cbm_daemon_ipc_win_directory_create_hook_set_for_test( + cbm_daemon_ipc_win_directory_create_hook_fn hook, void *context) { +#ifdef _WIN32 + g_win_directory_create_hook_context_for_test = context; + g_win_directory_create_hook_for_test = hook; +#else + (void)hook; + (void)context; +#endif +} + +#ifdef _WIN32 +static void ipc_win_directory_create_hook_run(const wchar_t *path) { + cbm_daemon_ipc_win_directory_create_hook_fn hook = g_win_directory_create_hook_for_test; + if (hook) { + hook(path, g_win_directory_create_hook_context_for_test); + } +} +#endif + bool cbm_daemon_ipc_windows_legacy_names(const char *canonical_runtime_parent, const char *instance_key, char pipe_out[CBM_DAEMON_IPC_WINDOWS_NAME_CAP], @@ -3994,6 +4019,16 @@ static char *wide_to_utf8(const wchar_t *value) { return utf8; } +/* Record ": " as the validation detail, with the wide path + * rendered as UTF-8. One place owns the conversion buffer, so every refusal on + * the directory walk can name its component without each call site carrying + * its own allocation. */ +static void ipc_validation_detail_set_for_path(const wchar_t *path, const char *message) { + char *path_utf8 = wide_to_utf8(path); + ipc_validation_detail_set("%s: %s", path_utf8 ? path_utf8 : "", message); + free(path_utf8); +} + static wchar_t *wide_copy(const wchar_t *value) { if (!value) { return NULL; @@ -4861,13 +4896,28 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { return false; } bool created = CreateDirectoryW(runtime_dir, &security.directory_attributes) != 0; - if (!created && GetLastError() != ERROR_ALREADY_EXISTS) { + DWORD create_error = created ? ERROR_SUCCESS : GetLastError(); + if (!created && create_error != ERROR_ALREADY_EXISTS) { + char message[96]; + (void)snprintf(message, sizeof(message), + "could not create the directory (Windows error %lu)", + (unsigned long)create_error); + ipc_validation_detail_set_for_path(runtime_dir, message); win_security_destroy(&security); return false; } DWORD attributes = GetFileAttributesW(runtime_dir); if (attributes == INVALID_FILE_ATTRIBUTES || (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0 || (attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { + /* What already exists at this path is adopted only if it is a plain + * directory. Say which rule refused it: the walk in front of this + * function tolerates losing a creation race, so this check is what + * stands between that tolerance and a planted file or junction. */ + const char *rule = attributes == INVALID_FILE_ATTRIBUTES ? "cannot be inspected" + : (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0 + ? "exists but is not a directory" + : "is a reparse point (junction or symlink)"; + ipc_validation_detail_set_for_path(runtime_dir, rule); win_security_destroy(&security); return false; } @@ -5021,8 +5071,35 @@ static bool win_private_directory_tree_secure(const wchar_t *directory_path) { DWORD attributes = GetFileAttributesW(path); if (attributes == INVALID_FILE_ATTRIBUTES) { DWORD error = GetLastError(); - ok = (error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND) && - CreateDirectoryW(path, &security.directory_attributes) != 0; + bool absent = error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND; + if (absent) { + ipc_win_directory_create_hook_run(path); + /* Several processes first-starting together all observe + * this component absent; one CreateDirectoryW wins and + * the rest get ERROR_ALREADY_EXISTS. Losing that race is + * not a failure -- the directory this process wanted now + * exists, exactly as if it had been there all along. It + * is NOT trusted for that: an ancestor still goes through + * win_directory_component_secure() below, and the final + * component through win_runtime_directory_secure(), which + * refuses a non-directory or reparse point and enforces + * owner and DACL. The POSIX walk tolerates EEXIST at the + * same point for the same reason. */ + if (CreateDirectoryW(path, &security.directory_attributes) == 0) { + error = GetLastError(); + absent = error == ERROR_ALREADY_EXISTS; + } + } + ok = absent; + if (!ok) { + /* Never a bare "(endpoint)": name the component and the + * Windows error that refused it. */ + char message[96]; + (void)snprintf(message, sizeof(message), + "could not create or inspect the directory (Windows error %lu)", + (unsigned long)error); + ipc_validation_detail_set_for_path(path, message); + } } /* Ancestors are observe-only and must already be secure. The * final current-user directory is intentionally handled below by @@ -5034,10 +5111,7 @@ static bool win_private_directory_tree_secure(const wchar_t *directory_path) { * at this walk position; the helper set the inner rule. */ char inner[384]; (void)snprintf(inner, sizeof(inner), "%s", ipc_validation_detail_buffer); - char *component_utf8 = wide_to_utf8(path); - ipc_validation_detail_set( - "%s: %s", component_utf8 ? component_utf8 : "", inner); - free(component_utf8); + ipc_validation_detail_set_for_path(path, inner); } } } diff --git a/src/daemon/ipc_internal.h b/src/daemon/ipc_internal.h index 4b7cf1fd5e..f481b7378a 100644 --- a/src/daemon/ipc_internal.h +++ b/src/daemon/ipc_internal.h @@ -110,4 +110,15 @@ void cbm_daemon_ipc_posix_record_write_failure_set_for_test(int errno_value); typedef void (*cbm_daemon_ipc_startup_gate_fn)(void *context); void cbm_daemon_ipc_startup_gate_set_for_test(cbm_daemon_ipc_startup_gate_fn gate, void *context); +/* Deterministic-interleaving seam for the Windows private-directory walk: + * fires after a path component was observed ABSENT and before this process + * tries to create it. A test plays the concurrent process that wins that + * creation, which pins the cold-start race (N processes first-starting + * against a runtime directory that does not exist yet) by construction + * instead of by thread timing. `path` is the NUL-terminated component path at + * the walk position. No-op off Windows. */ +typedef void (*cbm_daemon_ipc_win_directory_create_hook_fn)(const wchar_t *path, void *context); +void cbm_daemon_ipc_win_directory_create_hook_set_for_test( + cbm_daemon_ipc_win_directory_create_hook_fn hook, void *context); + #endif /* CBM_DAEMON_IPC_INTERNAL_H */ diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index b451f9bb32..815f630a62 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -882,6 +882,129 @@ TEST(daemon_ipc_windows_private_directory_ace_is_inheritable) { PASS(); } +typedef struct { + int calls; + bool plant_file; /* false: play a racer that creates the DIRECTORY first */ + bool planted; +} ipc_test_win_create_racer_t; + +/* Plays the concurrent process that wins the creation of a path component the + * walk has just observed absent. Runs inside the walk, at the exact point the + * race lives, so the interleaving is pinned by construction: no threads, no + * timing. */ +static void ipc_test_win_create_racer(const wchar_t *path, void *context) { + ipc_test_win_create_racer_t *racer = context; + racer->calls++; + if (racer->calls != 1) { + return; + } + if (racer->plant_file) { + HANDLE file = + CreateFileW(path, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); + racer->planted = file != INVALID_HANDLE_VALUE; + if (racer->planted) { + (void)CloseHandle(file); + } + } else { + racer->planted = CreateDirectoryW(path, NULL) != 0; + } +} + +/* Cold-start race (test-windows-guards section_cold_storm, 2026-09-21; the + * same thing a user gets when a host launches several MCP servers on its very + * first run): N processes first-start against a runtime directory that does + * not exist yet. All of them observe the final component absent, one + * CreateDirectoryW wins, and every other process gets ERROR_ALREADY_EXISTS. + * The walk treated that as a failure and recorded no detail, so the losers + * exited with a bare "secure CLI coordination could not be created + * (endpoint)". Losing the creation race is not a failure: the directory this + * process wanted now exists, and the owner / DACL / not-a-reparse-point checks + * that follow are exactly the ones a pre-existing directory gets. The POSIX + * walk has always tolerated EEXIST here. */ +TEST(daemon_ipc_windows_private_directory_survives_lost_creation_race) { + char parent[TEST_PATH_CAP] = {0}; + char cache[TEST_PATH_CAP] = {0}; + bool paths_ok = false; + bool secured = false; + bool owner_only = false; + ipc_test_win_create_racer_t racer = {0}; + + if (ipc_test_parent_new(parent, "win-create-race")) { + int written = snprintf(cache, sizeof(cache), "%s/cache", parent); + paths_ok = written > 0 && written < (int)sizeof(cache); + } + if (paths_ok) { + cbm_daemon_ipc_win_directory_create_hook_set_for_test(ipc_test_win_create_racer, &racer); + secured = cbm_daemon_ipc_private_directory_secure(cache); + cbm_daemon_ipc_win_directory_create_hook_set_for_test(NULL, NULL); + } + if (secured) { + /* The racer created the directory with an inherited DACL; the winner's + * directory must still end up owner-only, like any adopted one. */ + PACL dacl = NULL; + PSECURITY_DESCRIPTOR descriptor = NULL; + ACL_SIZE_INFORMATION information; + memset(&information, 0, sizeof(information)); + if (GetNamedSecurityInfoA(cache, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, + &dacl, NULL, &descriptor) == ERROR_SUCCESS && + dacl && + GetAclInformation(dacl, &information, sizeof(information), AclSizeInformation)) { + owner_only = information.AceCount == 1; + } + if (descriptor) { + (void)LocalFree(descriptor); + } + } + + (void)RemoveDirectoryA(cache); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(paths_ok); + /* The seam sat on the path and the racer really won: without these two the + * test could pass without ever exercising the race. */ + ASSERT_EQ(racer.calls, 1); + ASSERT_TRUE(racer.planted); + ASSERT_TRUE(secured); + ASSERT_TRUE(owner_only); + PASS(); +} + +/* The tolerance above must not admit anything: when what appeared at the path + * is NOT a directory, the walk still refuses -- and it says why. A bare + * "(endpoint)" is what four reporters in #1533/#1574 were left with; every + * refusal on this path names the component and the rule. */ +TEST(daemon_ipc_windows_private_directory_refuses_and_names_a_planted_file) { + char parent[TEST_PATH_CAP] = {0}; + char cache[TEST_PATH_CAP] = {0}; + char detail[512] = {0}; + bool paths_ok = false; + bool refused = false; + ipc_test_win_create_racer_t racer = {.plant_file = true}; + + if (ipc_test_parent_new(parent, "win-create-race-file")) { + int written = snprintf(cache, sizeof(cache), "%s/cache", parent); + paths_ok = written > 0 && written < (int)sizeof(cache); + } + if (paths_ok) { + cbm_daemon_ipc_win_directory_create_hook_set_for_test(ipc_test_win_create_racer, &racer); + refused = !cbm_daemon_ipc_private_directory_secure(cache); + cbm_daemon_ipc_win_directory_create_hook_set_for_test(NULL, NULL); + const char *why = cbm_daemon_ipc_validation_detail(); + (void)snprintf(detail, sizeof(detail), "%s", why ? why : ""); + } + + (void)DeleteFileA(cache); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(paths_ok); + ASSERT_EQ(racer.calls, 1); + ASSERT_TRUE(racer.planted); + ASSERT_TRUE(refused); + ASSERT_NOT_NULL(strstr(detail, "cache")); + ASSERT_NOT_NULL(strstr(detail, "not a directory")); + PASS(); +} + TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor) { char parent[TEST_PATH_CAP] = {0}; char add_only[TEST_PATH_CAP] = {0}; @@ -5361,6 +5484,8 @@ SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_windows_default_endpoint_ignores_temp_environment); RUN_TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl); RUN_TEST(daemon_ipc_windows_private_directory_ace_is_inheritable); + RUN_TEST(daemon_ipc_windows_private_directory_survives_lost_creation_race); + RUN_TEST(daemon_ipc_windows_private_directory_refuses_and_names_a_planted_file); RUN_TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor); RUN_TEST(daemon_ipc_windows_sid_trust_accepts_local_admin_rejects_foreign_500); RUN_TEST(daemon_ipc_windows_legacy_bridge_covers_handoff_and_lifetime); From aaa72fbe60ad341325e9c944e32cf67d98eca387 Mon Sep 17 00:00:00 2001 From: Lowpower <3133734+Lowpower@users.noreply.github.com> Date: Thu, 10 Sep 2026 03:06:03 +0000 Subject: [PATCH 6/7] feat(cli): point top-level cli --help at --format json CLI_USAGE described outer --json but did not mention tool-level --format json. Add a help-only pointer that tools which accept format support --format tree|json, and distinguish payload JSON from the full MCP envelope. No new flags, env vars, or output-default changes. Fixes #2102 Signed-off-by: Lowpower <3133734+Lowpower@users.noreply.github.com> --- src/cli/cli.h | 12 ++++++++++++ src/main.c | 8 +------- tests/test_cli.c | 16 ++++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/cli/cli.h b/src/cli/cli.h index cf7f6fa443..a1b749bb08 100644 --- a/src/cli/cli.h +++ b/src/cli/cli.h @@ -27,6 +27,18 @@ const char *cbm_cli_get_version(void); /* ── CLI tool arguments (flags / --args-file / --help) ────────── */ +/* Top-level `cli --help` text printed by run_cli() in src/main.c. + * Documents tool-level --format without adding a session-wide flag (#2102). */ +#define CBM_CLI_USAGE \ + "Usage: codebase-memory-mcp cli [--quiet] [--progress] [--verbose] [--json] " \ + "[json_args]\n" \ + " --quiet Show errors only; cannot combine with --progress or outer --verbose\n" \ + " --progress Show lifecycle progress even when stderr is redirected\n" \ + " --verbose Include informational logs (preserves CBM_LOG_LEVEL=debug)\n" \ + " --json Print the raw MCP result envelope\n" \ + " Tools that accept format support --format tree|json (default: tree).\n" \ + " --format json prints payload JSON; outer --json prints the full MCP envelope.\n" + /* Convert `--flag value` / `--flag=value` / bare-boolean `--flag` arguments for * a tool into a JSON arguments object string, using the tool's input_schema to * type values (string/integer/boolean) and to collect repeated flags into diff --git a/src/main.c b/src/main.c index 9b91f205c1..65e041217b 100644 --- a/src/main.c +++ b/src/main.c @@ -675,13 +675,7 @@ static bool client_start_parent_watchdog(DWORD initial_ppid) { /* ── CLI mode ───────────────────────────────────────────────────── */ -#define CLI_USAGE \ - "Usage: codebase-memory-mcp cli [--quiet] [--progress] [--verbose] [--json] " \ - "[json_args]\n" \ - " --quiet Show errors only; cannot combine with --progress or outer --verbose\n" \ - " --progress Show lifecycle progress even when stderr is redirected\n" \ - " --verbose Include informational logs (preserves CBM_LOG_LEVEL=debug)\n" \ - " --json Print the raw MCP result envelope\n" +#define CLI_USAGE CBM_CLI_USAGE /* `cli --help`; wording lives in cli.h (#2102) */ /* Extract text content from MCP tool result envelope and print it. * MCP results: {"content":[{"type":"text","text":"..."}],"isError":...} diff --git a/tests/test_cli.c b/tests/test_cli.c index 211ec63555..2ca51ab2f7 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -15336,6 +15336,21 @@ TEST(cli_print_tool_help_issue680) { PASS(); } +/* #2102: top-level `cli --help` (CLI_USAGE) must point at tool-level + * `--format json` and distinguish it from outer `--json`. This is help-only: + * the usage synopsis still has no session-wide --format / CBM_CLI_FORMAT. */ +TEST(cli_usage_points_to_tool_format_json_issue2102) { + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "--format tree|json")); + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "payload JSON")); + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "full MCP envelope")); + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "--json Print the raw MCP result envelope")); + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, + "cli [--quiet] [--progress] [--verbose] [--json] ")); + ASSERT_NULL(strstr(CBM_CLI_USAGE, "[--format")); + ASSERT_NULL(strstr(CBM_CLI_USAGE, "CBM_CLI_FORMAT")); + PASS(); +} + /* #1359: `cli ` with no argument-bearing token used to slurp stdin to EOF * for ANY non-terminal stdin. The ordinary automation caller never sends that * EOF — Node's child_process.spawn defaults to stdio:['pipe','pipe','pipe'] and @@ -16376,6 +16391,7 @@ SUITE(cli) { RUN_TEST(cli_build_args_json_key_equals_value_issue680); RUN_TEST(cli_build_args_json_bad_positional_errors_issue680); RUN_TEST(cli_print_tool_help_issue680); + RUN_TEST(cli_usage_points_to_tool_format_json_issue2102); /* Stdin argument gate (#1359) */ RUN_TEST(cli_zero_argument_tool_never_reads_stdin_issue1359); From 2f557f53986976b2df1eb2a1a540c9d48cf91a8e Mon Sep 17 00:00:00 2001 From: Lowpower <3133734+Lowpower@users.noreply.github.com> Date: Thu, 10 Sep 2026 03:17:17 +0000 Subject: [PATCH 7/7] style(cli): clang-format CBM_CLI_USAGE and help-text test Signed-off-by: Lowpower <3133734+Lowpower@users.noreply.github.com> --- src/cli/cli.h | 14 +++++++------- tests/test_cli.c | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cli/cli.h b/src/cli/cli.h index a1b749bb08..5522edef06 100644 --- a/src/cli/cli.h +++ b/src/cli/cli.h @@ -29,14 +29,14 @@ const char *cbm_cli_get_version(void); /* Top-level `cli --help` text printed by run_cli() in src/main.c. * Documents tool-level --format without adding a session-wide flag (#2102). */ -#define CBM_CLI_USAGE \ +#define CBM_CLI_USAGE \ "Usage: codebase-memory-mcp cli [--quiet] [--progress] [--verbose] [--json] " \ - "[json_args]\n" \ - " --quiet Show errors only; cannot combine with --progress or outer --verbose\n" \ - " --progress Show lifecycle progress even when stderr is redirected\n" \ - " --verbose Include informational logs (preserves CBM_LOG_LEVEL=debug)\n" \ - " --json Print the raw MCP result envelope\n" \ - " Tools that accept format support --format tree|json (default: tree).\n" \ + "[json_args]\n" \ + " --quiet Show errors only; cannot combine with --progress or outer --verbose\n" \ + " --progress Show lifecycle progress even when stderr is redirected\n" \ + " --verbose Include informational logs (preserves CBM_LOG_LEVEL=debug)\n" \ + " --json Print the raw MCP result envelope\n" \ + " Tools that accept format support --format tree|json (default: tree).\n" \ " --format json prints payload JSON; outer --json prints the full MCP envelope.\n" /* Convert `--flag value` / `--flag=value` / bare-boolean `--flag` arguments for diff --git a/tests/test_cli.c b/tests/test_cli.c index 2ca51ab2f7..01fb79ec8d 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -15344,8 +15344,8 @@ TEST(cli_usage_points_to_tool_format_json_issue2102) { ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "payload JSON")); ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "full MCP envelope")); ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "--json Print the raw MCP result envelope")); - ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, - "cli [--quiet] [--progress] [--verbose] [--json] ")); + ASSERT_NOT_NULL(strstr(CBM_CLI_USAGE, "cli [--quiet] [--progress] [--verbose] [--json] " + "")); ASSERT_NULL(strstr(CBM_CLI_USAGE, "[--format")); ASSERT_NULL(strstr(CBM_CLI_USAGE, "CBM_CLI_FORMAT")); PASS();