fix: validate time_range_minutes from CLI args in run.py#207
Open
Sanjays2402 wants to merge 1 commit into
Open
fix: validate time_range_minutes from CLI args in run.py#207Sanjays2402 wants to merge 1 commit into
Sanjays2402 wants to merge 1 commit into
Conversation
The CLI-argument path in _load_request_from_args_or_env called int(sys.argv[3]) directly, so passing a non-integer third argument crashed with an unhandled ValueError and non-positive values (0, -5) were silently accepted. The environment-variable path already validated both cases. Factor the parse-and-validate into a helper and use it for both paths so the documented invocation (python -m sre_agent.run <log_group> <service> <minutes>) fails cleanly on bad input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix inconsistent handling of
time_range_minutesinsre_agent/run.pybetween the CLI-argument and environment-variable code paths. The CLI path now validates the value the same way the env path does.Why
The documented invocation in
DEVELOPMENT.mdis:…optionally with a third positional argument for minutes. In
_load_request_from_args_or_env(src/sre_agent/run.py:L26), the CLI branch callsint(sys.argv[3])directly, while the env branch (L38-L46) already wraps the parse intry/except ValueErrorand rejects<= 0.Two real issues follow:
ValueError: invalid literal for int() with base 10: 'abc'with a full traceback instead of the friendly"time_range_minutes must be an integer."message the env path prints.python -m sre_agent.run foo bar 0and... -5are accepted on the CLI path, even though the env path rejects both. The same-shape validation insrc/sre_agent/cli/mode/remote/aws/ecs/steps.py:L402-L409also rejects<= 0, so the CLI path is the outlier.Reproduction (before this patch):
How
_parse_time_range_minutes(raw: str) -> intand call it from both the CLI-args branch and the env branch.time_range_minutesso they read naturally for bothsys.argv[3]andTIME_RANGE_MINUTES.int(raw)withraw > 0returns the same value as before.Extra
No new dependencies.
Checklist
Testing
tests/test_run_args.pycovering: valid input, non-integer input, zero, negatives, and a round-trip through_load_request_from_args_or_envfor both failure modes and the happy path.uv run pytest tests/): 8 passed (1 pre-existing + 7 new).ruff check --config=ruff.tomlandruff format --config=ruff.toml --check: clean.uv run mypy --config-file=mypy.ini $(find src/sre_agent -name '*.py'):Success: no issues found in 94 source files.Type of change
Bug fix.