Repository navigation
Conversation
An empty string environment variable (VAR= from a shell wrapper, CI template, or .env file) hit int() at module import and killed every subcommand with an uncaught ValueError (tirth8205#912). os.environ.get(NAME, default) only returns the default when the variable is absent, not when it is present but empty. A single _int_env() helper in constants.py now treats empty/whitespace as unset, parses valid integers, and warns with the variable name before falling back to the default for invalid values. All module-level int(os.environ.get(...)) sites across constants, changes, and incremental now use it. Seven regression tests cover the helper's five branches plus subprocess import with empty CRG_GIT_TIMEOUT and CRG_MAX_IMPACT_NODES.
code-review-graph reviewOverall risk: 0.30 (LOW) — 11 changed function(s)/class(es), 2 affected flow(s), 2 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~20,797 fewer tokens (~65%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
|
The helper is the right idea and it does fix the reported command — The sweep stops less than half way, and #912's own scope list is not covered. The PR body says "All module-level
No clamping, so the crash moves rather than disappears. Empty → default is right, and invalid → warning + default is a reasonable call. But out-of-range values pass through untouched, and two of them are actively harmful:
Minor: the tests fail on the parent as a collection error ( Gates are green: 2992 passed, ruff and mypy clean, 13/13 checks. |
|
Changes required: an empty documented numeric setting still prevents the module from importing. Reproduce with |
|
Checks pass and the code looks right. Missing: tests for the edge cases it introduces.
Every new branch in the code should have a test, including the failure branch. Disagree with any of these? Say so here first. PRs now target |
|
This no longer merges into
Worth knowing before you resolve:
git fetch origin
git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git pushI have not reviewed the change itself yet. That comes once it merges and the checks run against the merged state, since staging has moved a long way and the result is what matters. |
|
Thank you for this PR. I tested empty values for each setting, on your branch and on This fix is already on On your branch, So I will not merge this PR. Thank you again for your help. |
Summary
An empty string environment variable (
VAR=from a shell wrapper, CI template, or.envfile) hitint()at module import and killed every subcommand with an uncaughtValueError(#912).os.environ.get(NAME, default)only returns the default when the variable is absent, not when it is present but empty.A single
_int_env()helper inconstants.pynow:All module-level
int(os.environ.get(...))sites now use it:constants.py:CRG_MAX_IMPACT_NODES,CRG_MAX_IMPACT_DEPTH,CRG_MAX_BFS_DEPTH,CRG_MAX_SEARCH_RESULTSchanges.py:CRG_GIT_TIMEOUT,CRG_MAX_CHANGED_FUNCSincremental.py:CRG_PARSE_WORKERS,CRG_MODULE_SCAN_DEPTH,CRG_MODULE_SCAN_MAX_DIRS,CRG_GIT_TIMEOUTTesting
New
tests/test_int_env.py(7 tests):test_unset_returns_default/test_empty_string_returns_default/test_whitespace_returns_default— the helper's three "fall back" branchestest_valid_integer— normal parsingtest_invalid_integer_warns_and_returns_default— warning includes the variable nametest_empty_git_timeout_does_not_crash_import/test_empty_max_impact_nodes_does_not_crash_import— subprocess verification that the original reproduction commands now succeedFixes #912