From 6b801eec68d6465aad2020de438e629cead99f5c Mon Sep 17 00:00:00 2001 From: Joshua Richter Date: Fri, 28 Aug 2026 11:08:11 -0400 Subject: [PATCH] fix(env): stop an unreadable environment setting from picking a wrong default atoi and atol answer 0 for text they cannot read, and 0 is a real setting at three places in this project. So a typo, a trailing unit such as "30s", or a stray space silently chose a value nobody asked for, and nothing on screen said the setting had been dropped. src/mcp/index_supervisor.c CBM_INDEX_WORKER_TIMEOUT_S src/cli/hook_augment.c CBM_HOOK_DEADLINE_MS src/mcp/mcp.c CBM_INDEX_MAX_RESTARTS CBM_HOOK_DEADLINE_MS was the worst of the three. atoi answered 0, 0 is below HA_DEADLINE_MIN_MS, and the clamp then handed back 50 ms -- the SHORTEST deadline the setting allows, for a setting whose only purpose is to give the hook more room. The comment above that function records a hunt for hook runs that never finished, 0 of 24 real sessions, which is the exact symptom a silently-shortened deadline produces. CBM_INDEX_MAX_RESTARTS lost twice. A typo kept the default of 100, and CBM_INDEX_MAX_RESTARTS=0 -- which reads as "do not restart" to anybody who sets it -- also kept 100. The setting did the opposite of the request. CBM_INDEX_WORKER_TIMEOUT_S fell through to the 15-minute default, so a test set to give up after 30 seconds hung for 15 minutes with nothing to explain why. The fix adds one helper rather than three copies of the same check: bool cbm_env_long(const char *name, long *out); It answers true only when the variable is set, is not empty, and reads cleanly from its first character to its last. It holds no policy -- no minimum, no maximum, no default -- because the three sites disagree on all three, and a helper that guessed would be wrong at two of them. The shape is the one src/main.c:1104 already uses: an end pointer, errno, and a check that nothing was left over. It also refuses a leading blank, which strtol would otherwise step over, so " 5" is a slip rather than the number 5. Each site keeps its own rule: worker timeout an unreadable value keeps the 15-minute default AND logs the value it dropped restart cap 0 now means no restarts; an unreadable value keeps 100 AND logs the value it dropped hook deadline an unreadable value now yields HA_DEADLINE_DEFAULT_MS, not the floor. This one stays silent on purpose: the file includes no log header and writes no stderr, because its output is hook protocol. The restart-cap parse was lifted out of a very large function into a named index_restart_cap(), so it can be read and reached on its own. Four tests come with the change. The two that pin the user-visible behaviour were seen failing before the fix: FAIL tests/test_cli.c:402: ms == 50, expected HOOK_DEADLINE_DEFAULT == 2000 (with "unreadable value \"abc\" gave 50 ms" printed above it) FAIL tests/test_cli.c:434: cbm_index_restart_cap_for_testing() == 100, expected 0 == 0 After the fix, TEST_SUITES="platform cli mcp" reports 518 passed, 2 failed. The full suite reports 7635 passed, 2 failed. Both failures are in tests/test_cli.c (lines 1826 and 6802), print "error: one or more agent cleanup operations failed", and reproduce on a clean tree without this change -- they depend on the coding agents installed on the machine. make -f Makefile.cbm lint-ci passes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Joshua Richter --- src/cli/cli.h | 7 ++++ src/cli/hook_augment.c | 16 ++++++-- src/foundation/platform.c | 29 ++++++++++++++ src/foundation/platform.h | 12 ++++++ src/mcp/index_supervisor.c | 18 +++++++-- src/mcp/mcp.c | 37 +++++++++++++---- src/mcp/mcp.h | 4 ++ tests/test_cli.c | 81 ++++++++++++++++++++++++++++++++++++++ tests/test_platform.c | 75 +++++++++++++++++++++++++++++++++++ 9 files changed, 263 insertions(+), 16 deletions(-) diff --git a/src/cli/cli.h b/src/cli/cli.h index c94a8044f..41715382b 100644 --- a/src/cli/cli.h +++ b/src/cli/cli.h @@ -525,6 +525,13 @@ char *cbm_hook_augment_lifecycle_json_for(const char *input, const char *forced_ /* Thin daemon frontend support: preserve the hook's bounded stdin read and * hard fail-open deadline without constructing a local MCP/store instance. */ void cbm_hook_augment_arm_deadline(void); + +/* The in-process deadline in milliseconds, as CBM_HOOK_DEADLINE_MS resolves it. + * Exposed so a test can check what an unreadable value falls back to. POSIX + * only: the Windows path arms a fixed timer and reads no environment value. */ +#ifndef _WIN32 +int cbm_hook_augment_deadline_ms_for_testing(void); +#endif char *cbm_hook_augment_read_stdin(void); /* Pure no-op gate for the hook-client fast path (see hook_augment.c). */ bool cbm_hook_augment_input_is_noop_bash(const char *input); diff --git a/src/cli/hook_augment.c b/src/cli/hook_augment.c index 01c161be1..ccc94bfa2 100644 --- a/src/cli/hook_augment.c +++ b/src/cli/hook_augment.c @@ -20,6 +20,7 @@ #include "foundation/compat_fs.h" #include "foundation/constants.h" #include "foundation/mem.h" +#include "foundation/platform.h" #include "mcp/mcp.h" #include "pipeline/pipeline.h" #include "yyjson/yyjson.h" @@ -70,18 +71,21 @@ * hook "timeout" remains the outer backstop (and alone governs Windows, * where this whole in-process deadline block is compiled out). */ static int ha_deadline_ms(void) { - const char *env = getenv("CBM_HOOK_DEADLINE_MS"); - if (!env || !env[0]) { + /* A value this reader cannot read gets the DEFAULT, never the floor. atoi + * used to answer 0 for a typo, 0 is below the minimum, and the clamp then + * handed back the shortest deadline the setting allows — the opposite of + * what somebody raising CBM_HOOK_DEADLINE_MS is asking for. */ + long v = 0; + if (!cbm_env_long("CBM_HOOK_DEADLINE_MS", &v)) { return HA_DEADLINE_DEFAULT_MS; } - int v = atoi(env); if (v < HA_DEADLINE_MIN_MS) { return HA_DEADLINE_MIN_MS; } if (v > HA_DEADLINE_MAX_MS) { return HA_DEADLINE_MAX_MS; } - return v; + return (int)v; } static int g_ha_crumb_fd = -1; @@ -123,6 +127,10 @@ static void ha_open_crumb_log(int deadline_ms) { g_ha_crumb_len = (n > 0 && n < (int)sizeof(g_ha_crumb_msg)) ? (size_t)n : 0; } +int cbm_hook_augment_deadline_ms_for_testing(void) { + return ha_deadline_ms(); +} + void cbm_hook_augment_arm_deadline(void) { int ms = ha_deadline_ms(); ha_open_crumb_log(ms); diff --git a/src/foundation/platform.c b/src/foundation/platform.c index ce4c43dc6..14e36c0d8 100644 --- a/src/foundation/platform.c +++ b/src/foundation/platform.c @@ -8,6 +8,8 @@ #include "foundation/compat.h" #include "foundation/constants.h" #include "foundation/platform_internal.h" +#include +#include #include #include #include @@ -435,6 +437,33 @@ const char *cbm_safe_getenv(const char *name, char *buf, size_t buf_sz, const ch return NULL; } +/* See platform.h. The shape here is the one src/main.c:1104 already uses for + * --port=: an end pointer says where the read stopped, errno catches a number + * too large, and *end == '\0' catches anything left over. */ +bool cbm_env_long(const char *name, long *out) { + if (!out) { + return false; + } + char raw[CBM_SZ_64] = {0}; + if (!cbm_safe_getenv(name, raw, sizeof(raw), NULL) || !raw[0]) { + return false; + } + /* strtol skips leading blanks of its own accord, so " 5" would read as 5. + * A blank in front of a setting is a slip, not a number, so refuse it here + * rather than let strtol quietly step over it. */ + if (isspace((unsigned char)raw[0])) { + return false; + } + char *end = NULL; + errno = 0; + long value = strtol(raw, &end, CBM_DECIMAL_BASE); + if (errno != 0 || !end || end == raw || *end != '\0') { + return false; + } + *out = value; + return true; +} + /* ── Home directory (cross-platform) ───────────────────── */ const char *cbm_get_home_dir(void) { diff --git a/src/foundation/platform.h b/src/foundation/platform.h index 2511a060e..938cf5905 100644 --- a/src/foundation/platform.h +++ b/src/foundation/platform.h @@ -121,6 +121,18 @@ int cbm_default_worker_count(bool initial); * Returns NULL when the variable is unset and fallback is NULL. */ const char *cbm_safe_getenv(const char *name, char *buf, size_t buf_sz, const char *fallback); +/* Read an environment variable as a whole number. + * + * Answers true only when the variable is set, is not empty, and reads cleanly + * from its first character to its last. Anything else — a typo, a trailing + * unit such as "30s", a leading or trailing space, or a number too large for a + * long — answers false and leaves *out untouched, so the caller picks its own + * fallback and can say that it did. + * + * This exists because atoi and atol answer 0 for text they cannot read, and 0 + * is a real setting at every call site in this project. */ +bool cbm_env_long(const char *name, long *out); + /* ── Home directory ─────────────────────────────────────────────── */ /* Cross-platform home directory: tries HOME first, then USERPROFILE (Windows). diff --git a/src/mcp/index_supervisor.c b/src/mcp/index_supervisor.c index b5257dfc1..eed78f418 100644 --- a/src/mcp/index_supervisor.c +++ b/src/mcp/index_supervisor.c @@ -12,6 +12,7 @@ #include "foundation/profile.h" /* cbm_profile_active (keep worker log under CBM_PROFILE) */ #include "ui/http_server.h" /* cbm_http_server_resolve_binary_path */ +#include #include #include #include @@ -322,14 +323,23 @@ static bool supervisor_disable_requested(void) { * CBM_INDEX_WORKER_TIMEOUT_S override (seconds → ms) tightens it for tests. */ static int worker_quiet_timeout_ms(void) { enum { DEFAULT_QUIET_TIMEOUT_MS = 900000 }; /* 15 min with no progress */ + enum { MS_PER_SECOND = 1000 }; char timeout_seconds[CBM_SZ_32] = {0}; + long s = 0; + /* The upper test only stops the seconds-to-ms multiply from overflowing an + * int. It sets no policy: a longer timeout than the default is still fine. */ + if (cbm_env_long("CBM_INDEX_WORKER_TIMEOUT_S", &s) && s > 0 && s <= INT_MAX / MS_PER_SECOND) { + return (int)(s * MS_PER_SECOND); + } + /* atol used to answer 0 for a value it could not read, and 0 fell straight + * through to the 15-minute default with nothing on screen. A test set to + * give up after 30 seconds then hung for 15 minutes and nobody could see + * why. An unreadable value now says so before it is dropped. */ if (cbm_safe_getenv("CBM_INDEX_WORKER_TIMEOUT_S", timeout_seconds, sizeof(timeout_seconds), NULL) && timeout_seconds[0]) { - long s = atol(timeout_seconds); - if (s > 0) { - return (int)(s * 1000); - } + cbm_log_warn("index.supervisor.worker_timeout_ignored", "value", timeout_seconds, "action", + "using_default"); } return DEFAULT_QUIET_TIMEOUT_MS; } diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index af2092a82..9d2e2e161 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -8374,6 +8374,34 @@ cbm_mcp_supervised_result_disposition_t cbm_mcp_supervised_result_disposition( * - a contained-failure response only if even that cannot produce a clean run. * A physical CBM host never falls back to its in-process pipeline: an initial * start/protocol failure is returned as an explicit error response. */ +/* How many times a failed index worker may be re-run before the server gives + * up, as CBM_INDEX_MAX_RESTARTS sets it. Default 100. */ +static int index_restart_cap(void) { + enum { INDEX_RESTART_CAP_DEFAULT = 100 }; + long v = 0; + if (!cbm_env_long("CBM_INDEX_MAX_RESTARTS", &v)) { + /* Unset is the ordinary case and says nothing. A value that is set but + * unreadable is a person's intent being dropped, so name it. */ + char raw[CBM_SZ_64] = {0}; + if (cbm_safe_getenv("CBM_INDEX_MAX_RESTARTS", raw, sizeof(raw), NULL) && raw[0]) { + cbm_log_warn("index.restart_cap.ignored", "value", raw, "action", "using_default"); + } + return INDEX_RESTART_CAP_DEFAULT; + } + /* Zero is a real answer meaning no restarts. The old reader kept the + * default unless the number was above zero, so the one value somebody sets + * to leave the worker alone did the opposite. */ + if (v < 0 || v > INT_MAX) { + cbm_log_warn("index.restart_cap.out_of_range", "action", "using_default"); + return INDEX_RESTART_CAP_DEFAULT; + } + return (int)v; +} + +int cbm_index_restart_cap_for_testing(void) { + return index_restart_cap(); +} + static char *index_run_supervised(cbm_mcp_server_t *srv, const char *args) { invalidate_cached_store(srv); @@ -8437,14 +8465,7 @@ static char *index_run_supervised(cbm_mcp_server_t *srv, const char *args) { (void)fclose(qinit); } - int cap = 100; - const char *cap_env = getenv("CBM_INDEX_MAX_RESTARTS"); - if (cap_env && cap_env[0]) { - int v = atoi(cap_env); - if (v > 0) { - cap = v; - } - } + int cap = index_restart_cap(); char *resp = NULL; int quarantined = 0; /* files pinned + added to the quarantine list so far */ diff --git a/src/mcp/mcp.h b/src/mcp/mcp.h index d55d58f3d..38312caaf 100644 --- a/src/mcp/mcp.h +++ b/src/mcp/mcp.h @@ -290,4 +290,8 @@ void cbm_mcp_server_request_scope_end(cbm_mcp_server_t *srv); * On Windows, strips leading / from /C:/path. */ bool cbm_parse_file_uri(const char *uri, char *out_path, int out_size); +/* How many restarts a failed index worker gets, as CBM_INDEX_MAX_RESTARTS sets + * it. Exposed so a test can check what the setting resolves to. */ +int cbm_index_restart_cap_for_testing(void); + #endif /* CBM_MCP_H */ diff --git a/tests/test_cli.c b/tests/test_cli.c index b2b543d71..fe22858b8 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -367,6 +367,83 @@ static void restore_test_env(const char *name, char *saved) { } } +/* An unreadable CBM_HOOK_DEADLINE_MS must fall back to the DEFAULT budget, not + * to the shortest one the setting allows. + * + * atoi answers 0 for text it cannot read, and 0 is below HA_DEADLINE_MIN_MS, so + * the clamp used to hand back 50 ms -- the worst possible answer for a setting + * whose whole purpose is to give the hook more room. The comment above + * ha_deadline_ms records a hunt for hook runs that never finished (0 of 24 real + * sessions), which is exactly the symptom a silently-shortened deadline makes. + * + * POSIX only: the Windows path arms a fixed timer and reads no environment. */ +#ifndef _WIN32 +TEST(cli_hook_deadline_ignores_an_unreadable_value) { + enum { HOOK_DEADLINE_DEFAULT = 2000, HOOK_DEADLINE_MIN = 50, HOOK_DEADLINE_MAX = 10000 }; + char *saved = save_test_env("CBM_HOOK_DEADLINE_MS"); + + /* Positive control: a good value is still used, so a failure below is about + * the unreadable case and not about the reader being broken outright. */ + cbm_setenv("CBM_HOOK_DEADLINE_MS", "1234", 1); + ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), 1234); + + /* Unset falls back to the default. */ + cbm_unsetenv("CBM_HOOK_DEADLINE_MS"); + ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_DEFAULT); + + /* The claim: text the reader cannot read gets the default, never the floor. */ + const char *unreadable[] = {"abc", "2000ms", " 2000", "2000 ", "", "1e3"}; + for (size_t i = 0; i < sizeof(unreadable) / sizeof(unreadable[0]); i++) { + cbm_setenv("CBM_HOOK_DEADLINE_MS", unreadable[i], 1); + int ms = cbm_hook_augment_deadline_ms_for_testing(); + if (ms != HOOK_DEADLINE_DEFAULT) { + printf(" unreadable value \"%s\" gave %d ms\n", unreadable[i], ms); + } + ASSERT_EQ(ms, HOOK_DEADLINE_DEFAULT); + } + + /* Both clamps still hold for values that DO read. */ + cbm_setenv("CBM_HOOK_DEADLINE_MS", "1", 1); + ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_MIN); + cbm_setenv("CBM_HOOK_DEADLINE_MS", "999999", 1); + ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_MAX); + + restore_test_env("CBM_HOOK_DEADLINE_MS", saved); + PASS(); +} +#endif + +/* CBM_INDEX_MAX_RESTARTS=0 means no restarts. It used to mean 100 of them. + * + * The old reader kept the default unless atoi answered greater than zero, so + * the one value a person sets when they want the worker left alone did the + * opposite. A typo did the same thing, with nothing on screen either way. */ +TEST(cli_index_restart_cap_honours_zero_and_refuses_junk) { + enum { INDEX_RESTART_CAP_DEFAULT = 100 }; + char *saved = save_test_env("CBM_INDEX_MAX_RESTARTS"); + + /* Positive control: a good value is still used. */ + cbm_setenv("CBM_INDEX_MAX_RESTARTS", "7", 1); + ASSERT_EQ(cbm_index_restart_cap_for_testing(), 7); + + cbm_unsetenv("CBM_INDEX_MAX_RESTARTS"); + ASSERT_EQ(cbm_index_restart_cap_for_testing(), INDEX_RESTART_CAP_DEFAULT); + + /* The claim: zero is a real answer meaning no restarts. */ + cbm_setenv("CBM_INDEX_MAX_RESTARTS", "0", 1); + ASSERT_EQ(cbm_index_restart_cap_for_testing(), 0); + + /* Text the reader cannot read keeps the default. */ + const char *unreadable[] = {"abc", "5x", " 5", "5 ", ""}; + for (size_t i = 0; i < sizeof(unreadable) / sizeof(unreadable[0]); i++) { + cbm_setenv("CBM_INDEX_MAX_RESTARTS", unreadable[i], 1); + ASSERT_EQ(cbm_index_restart_cap_for_testing(), INDEX_RESTART_CAP_DEFAULT); + } + + restore_test_env("CBM_INDEX_MAX_RESTARTS", saved); + PASS(); +} + /* Helper: mkdirp */ static int test_mkdirp(const char *path) { char tmp[1024]; @@ -13686,6 +13763,10 @@ TEST(cli_update_only_names_an_installer_that_exists_issue1632) { SUITE(cli) { RUN_TEST(cli_update_only_names_an_installer_that_exists_issue1632); +#ifndef _WIN32 + RUN_TEST(cli_hook_deadline_ignores_an_unreadable_value); +#endif + RUN_TEST(cli_index_restart_cap_honours_zero_and_refuses_junk); RUN_TEST(cli_progress_visibility_policy); RUN_TEST(cli_raw_mcp_result_preserves_tool_error_status); RUN_TEST(cli_maintenance_cancellation_forces_failure_status); diff --git a/tests/test_platform.c b/tests/test_platform.c index 1b55df699..37299783b 100644 --- a/tests/test_platform.c +++ b/tests/test_platform.c @@ -419,6 +419,79 @@ TEST(platform_cache_dir_rejects_truncated_override) { PASS(); } +/* cbm_env_long reads a whole number, or says it could not. + * + * atoi and atol answer 0 for text they cannot read, and 0 is a real setting at + * every place this project reads a number out of the environment. So the + * helper reports whether the read worked instead of folding a failure into a + * value that looks fine. */ +TEST(platform_env_long_reads_a_clean_number) { + const char *name = "CBM_TEST_ENV_LONG"; + char *saved = getenv(name) ? strdup(getenv(name)) : NULL; + long out = 0; + + ASSERT_EQ(cbm_setenv(name, "42", 1), 0); + ASSERT_TRUE(cbm_env_long(name, &out)); + ASSERT_EQ(out, 42); + + /* Zero is a real answer, not a failure. This is the case that made + * CBM_INDEX_MAX_RESTARTS=0 mean 100 restarts. */ + ASSERT_EQ(cbm_setenv(name, "0", 1), 0); + out = 999; + ASSERT_TRUE(cbm_env_long(name, &out)); + ASSERT_EQ(out, 0); + + ASSERT_EQ(cbm_setenv(name, "-7", 1), 0); + ASSERT_TRUE(cbm_env_long(name, &out)); + ASSERT_EQ(out, -7); + + if (saved) { + (void)cbm_setenv(name, saved, 1); + free(saved); + } else { + (void)cbm_unsetenv(name); + } + PASS(); +} + +TEST(platform_env_long_refuses_what_it_cannot_read) { + const char *name = "CBM_TEST_ENV_LONG"; + char *saved = getenv(name) ? strdup(getenv(name)) : NULL; + + /* Every one of these used to answer 0 through atol. */ + const char *unreadable[] = { + "abc", "30s", " 30", "30 ", "", "1e3", + "0x10", "+ 30", "--3", "3.5", "99999999999999999999999999", + }; + for (size_t i = 0; i < sizeof(unreadable) / sizeof(unreadable[0]); i++) { + ASSERT_EQ(cbm_setenv(name, unreadable[i], 1), 0); + long out = 1234; /* a value the helper must not touch */ + if (cbm_env_long(name, &out)) { + printf(" \"%s\" was read as %ld\n", unreadable[i], out); + } + ASSERT_TRUE(!cbm_env_long(name, &out)); + ASSERT_EQ(out, 1234); + } + + /* A variable nobody set answers false too. */ + ASSERT_EQ(cbm_unsetenv(name), 0); + long out = 555; + ASSERT_TRUE(!cbm_env_long(name, &out)); + ASSERT_EQ(out, 555); + + /* A NULL destination is refused rather than written through. */ + ASSERT_EQ(cbm_setenv(name, "5", 1), 0); + ASSERT_TRUE(!cbm_env_long(name, NULL)); + + if (saved) { + (void)cbm_setenv(name, saved, 1); + free(saved); + } else { + (void)cbm_unsetenv(name); + } + PASS(); +} + #ifdef _WIN32 /* cbm_safe_getenv reads Windows' wide environment as UTF-8. Its matching * setter must update that same wide environment; _putenv_s alone interprets @@ -707,6 +780,8 @@ SUITE(platform) { RUN_TEST(platform_default_workers_env_override); RUN_TEST(platform_default_workers_env_invalid); RUN_TEST(platform_default_workers_env_unset); + RUN_TEST(platform_env_long_reads_a_clean_number); + RUN_TEST(platform_env_long_refuses_what_it_cannot_read); RUN_TEST(platform_system_info); #ifdef __linux__ RUN_TEST(cgroup_v2_cpu_quota);