Skip to content

fix(cli): add test and fix for argv readonly handling#2473

Open
Zenvila wants to merge 1 commit intoWerWolv:masterfrom
Zenvila:fix-argv-bug
Open

fix(cli): add test and fix for argv readonly handling#2473
Zenvila wants to merge 1 commit intoWerWolv:masterfrom
Zenvila:fix-argv-bug

Conversation

@Zenvila
Copy link
Copy Markdown

@Zenvila Zenvila commented Oct 2, 2025

Summary

This PR introduces a failing test for the argv bug related to readonly handling in the CLI,
and provides a fix to ensure proper argument parsing.

Changes

  • Added new test case in tests/cli/test_cli_readonly.cpp to reproduce the bug.
  • Implemented fix in CLI initialization to correctly handle readonly argv.
  • Updated CMake configuration to include the new test.

Motivation

Previously, the CLI would incorrectly handle readonly cases with argv,
leading to unexpected behavior. This PR ensures that the issue is reproducible
via a test and then properly resolved in the code.

Testing

  • Verified the new test fails before the fix.
  • Confirmed the test passes after the fix.
  • All existing tests still pass.

Related

Fixes #

@Zenvila
Copy link
Copy Markdown
Author

Zenvila commented Oct 3, 2025

Kindly review the request

@Zman350x
Copy link
Copy Markdown
Contributor

Zman350x commented Oct 4, 2025

Is there a reason this CLI flag was given its own parser instead of just being registered as a subcommand the same way every other CLI flag is handled? And unless I'm reading your PR wrong, you're implementing a whole new feature here (read-only mode), not just adding a test for an existing feature, so why is this marked as a test/fix instead of a feature?

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.

2 participants