Skip to content

Fail fast on unknown shell run arguments#1490

Closed
biefan wants to merge 1 commit intoAzure:mainfrom
biefan:reject-unknown-shell-run-arguments
Closed

Fail fast on unknown shell run arguments#1490
biefan wants to merge 1 commit intoAzure:mainfrom
biefan:reject-unknown-shell-run-arguments

Conversation

@biefan
Copy link
Contributor

@biefan biefan commented Mar 17, 2026

Summary

  • reject unknown pyrit_shell run arguments instead of logging a warning and continuing
  • keep the existing shell error path so invalid run input is shown to the user immediately
  • add regression coverage for unknown shell run arguments

Problem

parse_run_arguments() currently logs unknown tokens and keeps parsing. In shell mode that means a typo can be silently ignored while the rest of the command still runs.

For example:

  • run demo --bogus 123 --max-retries 2

currently logs warnings for --bogus and 123, but still returns a parsed command with max_retries=2.

That turns invalid CLI input into a silent partial execution instead of a clear parse error.

Testing

  • .venv/bin/pytest tests/unit/cli/test_frontend_core.py -q
  • .venv/bin/pytest tests/unit/cli/test_pyrit_shell.py -q -k parse_error

@biefan biefan force-pushed the reject-unknown-shell-run-arguments branch from 6076fd4 to eaf604a Compare March 17, 2026 03:57
@hannahwestra25 hannahwestra25 self-assigned this Mar 17, 2026
else:
logger.warning(f"Unknown argument: {parts[i]}")
i += 1
raise ValueError(f"Unknown argument: {parts[i]}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see where this is in code, but hoping this doesn't terminate the process, just re-type. If so that's ideal :)

@rlundeen2
Copy link
Contributor

rlundeen2 commented Mar 19, 2026

Fixed it in another pr because we had to consolidate another fix. Closing this one although this is independently a good fix.

See: #1489

@rlundeen2 rlundeen2 closed this Mar 19, 2026
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.

3 participants