Skip to content

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

Open
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/env-int-parse-strict
Open

fix(env): stop an unreadable environment setting from picking a wrong default#1880
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/env-int-parse-strict

Conversation

@CaptainMittens

@CaptainMittens CaptainMittens commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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.

… 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) <noreply@anthropic.com>
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
@CaptainMittens

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A malformed CBM_* environment value silently picks a wrong setting at three sites

1 participant