fix(env): stop an unreadable environment setting from picking a wrong default

[CaptainMittens]

What this fixes

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.

Site	Setting	What a typo did
src/cli/hook_augment.c	CBM_HOOK_DEADLINE_MS	returned the shortest deadline the setting allows
src/mcp/mcp.c	CBM_INDEX_MAX_RESTARTS	kept 100, and so did =0
src/mcp/index_supervisor.c	CBM_INDEX_WORKER_TIMEOUT_S	kept the 15-minute default

1. CBM_HOOK_DEADLINE_MS — a typo bought the shortest possible deadline

int v = atoi(env);
if (v < HA_DEADLINE_MIN_MS) {
    return HA_DEADLINE_MIN_MS;
}

atoi answers 0, 0 is below HA_DEADLINE_MIN_MS, and the clamp handed
back 50 ms — for a setting whose only purpose is to give the hook more
room. The comment directly 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.

2. CBM_INDEX_MAX_RESTARTS — the setting did the opposite of the request

int v = atoi(cap_env);
if (v > 0) {
    cap = v;
}

Two ways to lose. 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.

3. CBM_INDEX_WORKER_TIMEOUT_S — a 30-second test hung for 15 minutes

The comment above this override says it exists to tighten the timeout for
tests. atol answered 0 for 30s or a stray space, 0 failed the > 0
test, and the 15-minute default came back with nothing to explain it.

The fix

One helper, not 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 for --port=: an end
pointer, errno, and a check that nothing was left over. It adds one thing on
top — a refusal of a leading blank, because strtol steps over blanks of its
own accord, so " 5" would otherwise read as the number 5. The cli suite
caught that during this work.

Each site keeps its own rule:

Site	New behaviour on text it cannot read
worker timeout	keeps the 15-minute default AND logs the value it dropped
restart cap	0 now means no restarts; unreadable keeps 100 AND logs the dropped value
hook deadline	yields HA_DEADLINE_DEFAULT_MS (2000), never the floor

hook_augment.c stays silent on purpose: it 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.

Tests

Four tests. The two that pin the user-visible behaviour were seen failing
before the fix:

cli_hook_deadline_ignores_an_unreadable_value   unreadable value "abc" gave 50 ms
  FAIL tests/test_cli.c:402: ms == 50, expected HOOK_DEADLINE_DEFAULT == 2000
cli_index_restart_cap_honours_zero_and_refuses_junk
  FAIL tests/test_cli.c:434: cbm_index_restart_cap_for_testing() == 100, expected 0 == 0

Each carries a positive control first — a good value is still read — so a later
failure points at the unreadable case and not at a reader broken outright.

Test	File
platform_env_long_reads_a_clean_number	tests/test_platform.c
platform_env_long_refuses_what_it_cannot_read	tests/test_platform.c
cli_hook_deadline_ignores_an_unreadable_value	tests/test_cli.c
cli_index_restart_cap_honours_zero_and_refuses_junk	tests/test_cli.c

Two _for_testing wrappers reach the statics, following the convention already
used 25 times in src/cli/cli.h. The deadline wrapper is POSIX-only, matching
the #ifndef _WIN32 block the function lives in.

Checks run

Command	Result
make -f Makefile.cbm test-focused TEST_SUITES="platform cli mcp"	518 passed, 2 failed, 6 skipped
make -f Makefile.cbm lint-ci	=== CI linters passed === — exit 0
make -f Makefile.cbm cbm	exit 0
make -f Makefile.cbm test	7635 passed, 2 failed, 8 skipped

The two 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.

How this was found

By scanning for siblings of the parse bug fixed in #1875 — "a parse reports
success while the input stays unread". #1877 fixed the confidence pair from the
same scan. This is the environment-variable group.

Checklist

• Signed off with git commit -s (DCO)
• make -f Makefile.cbm test run
• make -f Makefile.cbm lint-ci run
• New behaviour covered by a test

Fixes #1981

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[CaptainMittens]

Same shape as #1922: an input the code cannot read becomes a plausible value, and nothing downstream can tell it from a real one.

Here it is atoi and atol answering 0, and 0 is a real setting at all three sites. The one worth a second look is CBM_HOOK_DEADLINE_MS. An unreadable value reads as 0, falls below the floor, and the clamp hands 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 symptom a silently-shortened deadline produces.

CBM_INDEX_MAX_RESTARTS=0 has the same flavour: it reads as "do not restart" to anyone who sets it, and kept the default of 100.

This is one of four from a single scan for siblings of #1875's shape — #1875, #1877, #1880, #1881. Context for the set, and a question about the two still unfiled, is on #1877.

[DeusData]

Thank you for the detailed reproduce-first analysis and for keeping policy at the individual call sites instead of hiding it in the shared parser. This changes hook deadlines, worker timeout handling, and restart policy across the CLI and MCP paths, so we need more time to review compatibility and failure behavior before making a decision. No additional information is requested from you at this stage.

[DeusData]

Approved. Three real bugs, and the first one is nastier than the table conveys.

I checked your headline evidence and it is exact. The comment above ha_deadline_ms says augmentation "never appeared in real sessions (0/24 observed)" because the original 300 ms budget self-terminated on cold starts. And then:

int v = atoi(env);
if (v < HA_DEADLINE_MIN_MS) { return HA_DEADLINE_MIN_MS; }   /* 50 */

So a typo in the one setting that exists to make that budget more generous hands back 50 ms — six times shorter than the 300 ms already documented as too short. Someone raising the deadline because their hook never fires would make it strictly worse and see nothing.

cbm_env_long holding no policy is the right call and I verified it does. No minimum, no maximum, no default — it only answers whether the text read cleanly. You are right that a helper which guessed would be wrong at two of the three sites; the worker timeout keeps 15 minutes, the restart cap keeps 100, and the hook deadline yields 2000, and those are genuinely three different answers.

And CBM_INDEX_MAX_RESTARTS=0 now meaning no restarts is the fix I would have missed. A typo keeping the default is bad; an explicit 0 keeping 100 is the setting doing the opposite of what it says.

One thing I chased and it comes out clean. cbm_env_long reads into a 64-byte buffer, so I checked what happens to a longer value: platform_copy_environment_value returns NULL and empties the buffer when length >= buf_sz rather than truncating, so cbm_safe_getenv fails, cbm_env_long returns false, and the site keeps its documented default. Fail-closed, no silent truncation into a wrong-but-parseable number. Worth stating because the alternative would have been a real hole.

Smaller things done right: the leading-blank refusal (strtol steps over blanks, so " 5" would read as 5) with the note that the cli suite caught it; positive controls first in each test so a later failure points at the unreadable case rather than a reader broken outright; hook_augment.c staying silent on purpose because its output is hook protocol; and lifting the restart-cap parse into a named index_restart_cap() so it can be reached at all. Your _for_testing count is right too — cli.h has exactly 25.

Status: cleared, reviewed, approved. main moved twice today (broken, repaired by #1993, then #1703 merged), so I will update this branch and re-run before merging. Nothing needed from you.

Working the bug class from #1875 rather than the bug — confidence pair in #1877, timestamps in #1881, environment here — is why these three read as one piece of work instead of three coincidences.

[CaptainMittens]

Rebased onto c38aa35c and pushed as 6b801eec. The clean merge-tree simulation held — one commit replayed, no conflicts.

Posting the local test result too, because it looks alarming and is not. If you run this branch's suite yourself you will see failures, and I would rather you had the comparison up front than spend time on it.

On this machine, same worktree, same TEST_SUITES="mcp cli":

Tree	Result
origin/main at c38aa35c	536 passed, 6 failed
this branch at 6b801eec	542 passed, 2 failed

The branch fails fewer than the base it sits on, and its two are a strict subset of main's six. They are the same two test functions — this branch adds 81 lines to tests/test_cli.c, which shifts both sites by 77:

Test	on main	on branch
cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index	1749	1826
cli_agent_client_registry_routes_plan_install_and_uninstall	6725	6802

Main's other four are tests/test_mcp.c:12521 and :12601 (index recovery, child killed by signal 9 on the alarm), plus tests/test_cli.c:9390 and :9491.

Two machine conditions explain them, and neither exists on your runners:

OpenCode is installed here. The failing runs print Detected agents: OpenCode … from inside the test's own fixture. cbm_find_cli searches /usr/local/bin and /opt/homebrew/bin, so agent detection escapes the fixture's HOME and PATH. Three of main's six print that line — 6725, 9390 and 9491.

No permanent daemon is running. The agent-client install/uninstall and cohort-drain assertions cannot drain a cohort cleanly without one, which is why the failing set is not even stable between runs on the same tree.

I have opened #2019 for the first of those, which fixes the 6725/6802 case only. Its description says plainly that 9390 and 9491 are the same class and untouched — the right fix for each depends on what its own assertion means, and I did not want to guess that on your behalf.

Nothing needed from you on this thread. #1880 itself is unchanged apart from the rebase.