From 73e5ebc500613f9f143dc5fb1658d16b6574058c Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Tue, 3 Nov 2026 14:59:20 -0800 Subject: [PATCH 1/3] fix: keep global options given before the subcommand The root parser and every subparser share the same global options, so the subparser's defaults overwrote values given before the subcommand: `tabcmd --server X --language de login` lost both. Build the subcommand copy of the global options with defaults suppressed so a value from either position is kept, and a value after the subcommand still wins. Co-Authored-By: Claude Code --- tabcmd/execution/parent_parser.py | 13 +++-- tests/parsers/test_global_options_position.py | 49 +++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/parsers/test_global_options_position.py diff --git a/tabcmd/execution/parent_parser.py b/tabcmd/execution/parent_parser.py index ad3173f5..0da7ec3d 100644 --- a/tabcmd/execution/parent_parser.py +++ b/tabcmd/execution/parent_parser.py @@ -21,7 +21,10 @@ """ -def parent_parser_with_global_options(): +def parent_parser_with_global_options(suppress_defaults: bool = False): + """Global options are accepted both before and after the subcommand. + The copy used by the subcommand parsers must not set defaults: argparse would otherwise + overwrite a value given before the subcommand with the default.""" parser = argparse.ArgumentParser(usage=argparse.SUPPRESS, add_help=False) parser._optionals.title = _("tabcmdparser.global.behaviors") @@ -139,6 +142,9 @@ def parent_parser_with_global_options(): metavar="", help=_("tabcmd.global.help.page_size"), ) + if suppress_defaults: + for action in parser._actions: + action.default = argparse.SUPPRESS return parser @@ -148,6 +154,7 @@ class ParentParser: def __init__(self): self.global_options = parent_parser_with_global_options() + self.subcommand_global_options = parent_parser_with_global_options(suppress_defaults=True) self.root = argparse.ArgumentParser( prog="tabcmd", description="tabcmd -- " + _("tabcmd.howto"), @@ -173,7 +180,7 @@ def connect_commands(self): def include(self, command): additional_parser = self.subparsers.add_parser( - command.name, help=command.description, parents=[self.global_options] + command.name, help=command.description, parents=[self.subcommand_global_options] ) additional_parser._optionals.title = _("tabcmdparser.global.connections") # This line is where we actually set each parser to call the correct command @@ -184,7 +191,7 @@ def include(self, command): # Help isn't added like the others because it has to have access to the rest, to get their args def include_help(self): additional_parser = self.subparsers.add_parser( - "help", help=_("tabcmdparser.help.description"), parents=[self.global_options] + "help", help=_("tabcmdparser.help.description"), parents=[self.subcommand_global_options] ) additional_parser._optionals.title = _("tabcmdparser.global.connections") additional_parser.set_defaults(func=Help(self)) diff --git a/tests/parsers/test_global_options_position.py b/tests/parsers/test_global_options_position.py new file mode 100644 index 00000000..1e221843 --- /dev/null +++ b/tests/parsers/test_global_options_position.py @@ -0,0 +1,49 @@ +import unittest +from unittest import mock + +from tabcmd.commands.auth.login_command import LoginCommand +from .common_setup import * + +commandname: str = "login" + + +@mock.patch("sys.argv", None) +class GlobalOptionsPositionTest(ParserTest): + """Global options must mean the same thing before and after the subcommand.""" + + @classmethod + def setUpClass(cls): + cls.parser_under_test = initialize_test_pieces(commandname, LoginCommand) + + def test_global_options_before_command_are_kept(self): + args = self.parser_under_test.parse_args( + ["--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de", commandname] + ) + assert args.server == "the-server", args + assert args.site_name == "the-site", args + assert args.token_name == "n", args + assert args.language == "de", args + + def test_global_options_after_command_are_kept(self): + args = self.parser_under_test.parse_args( + [commandname, "--server", "the-server", "--site", "the-site", "--token-name", "n", "--language", "de"] + ) + assert args.server == "the-server", args + assert args.site_name == "the-site", args + assert args.token_name == "n", args + assert args.language == "de", args + + def test_option_after_command_wins_over_option_before(self): + args = self.parser_under_test.parse_args(["--server", "first", commandname, "--server", "second"]) + assert args.server == "second", args + + def test_defaults_apply_when_option_not_given(self): + args = self.parser_under_test.parse_args([commandname]) + assert args.server is None, args + assert args.language is None, args + assert args.site_name == "", args + assert args.logging_level == "INFO", args + + +if __name__ == "__main__": + unittest.main() From 64d50cf4c226e166cb7e22d3be5ab2fa0103448e Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Tue, 3 Nov 2026 15:52:51 -0800 Subject: [PATCH 2/3] Test global options in both positions across real commands Covers every global option before and after the subcommand, boolean flag defaults, converters, the full default set, and the help command. Co-Authored-By: Claude Code --- tests/parsers/test_global_options_position.py | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/parsers/test_global_options_position.py b/tests/parsers/test_global_options_position.py index 1e221843..ef08fdd5 100644 --- a/tests/parsers/test_global_options_position.py +++ b/tests/parsers/test_global_options_position.py @@ -1,7 +1,9 @@ +import argparse import unittest from unittest import mock from tabcmd.commands.auth.login_command import LoginCommand +from tabcmd.execution.parent_parser import ParentParser, parent_parser_with_global_options from .common_setup import * commandname: str = "login" @@ -45,5 +47,104 @@ def test_defaults_apply_when_option_not_given(self): assert args.logging_level == "INFO", args +# every global option, with a value where it takes one; each must mean the same thing in either position +GLOBAL_OPTIONS = [ + ["-s", "the-server"], + ["-t", "the-site"], + ["-u", "the-user"], + ["--token-name", "the-token-name"], + ["-p", "the-password"], + ["--token-value", "the-token-value"], + ["--password-file", "the-file"], + ["--token-file", "the-file"], + ["--no-prompt"], + ["-c", "the-cert"], + ["--no-certcheck"], + ["--no-cookie"], + ["-x", "host:1"], + ["--no-proxy"], + ["--timeout", "30"], + ["--continue-if-exists"], + ["--country", "DE"], + ["--language", "DE"], + ["-l", "debug"], + ["--query-page-size", "5"], +] + +# commands that parse with the given arguments and nothing else +COMMANDS = [["login"], ["logout"], ["listsites"], ["get", "/views/a/b"], ["creategroup", "the-group"]] + + +def real_parser() -> argparse.ArgumentParser: + manager = ParentParser() + manager.connect_commands() + manager.include_help() + return manager.get_root_parser() + + +def parse(parser: argparse.ArgumentParser, args: list) -> dict: + values = vars(parser.parse_args(args)) + values.pop("func") + return values + + +class GlobalOptionsRealCommandsTest(unittest.TestCase): + """The same checks against the parser the CLI builds, not a single mocked command.""" + + parser: argparse.ArgumentParser + + @classmethod + def setUpClass(cls): + cls.parser = real_parser() + + def test_every_global_option_means_the_same_before_and_after_the_command(self): + for command in COMMANDS: + defaults = parse(self.parser, command) + for option in GLOBAL_OPTIONS: + with self.subTest(command=command[0], option=option): + before = parse(self.parser, option + command) + after = parse(self.parser, command + option) + assert before == after, (before, after) + assert before != defaults, "{} had no effect".format(option) + + def test_option_values_before_the_command_are_converted(self): + values = parse( + self.parser, ["-l", "debug", "--language", "DE", "--country", "FR", "--query-page-size", "5", "login"] + ) + assert values["logging_level"] == "DEBUG", values + assert values["language"] == "de", values + assert values["country"] == "fr", values + assert values["query_page_size"] == 5, values + + def test_boolean_flags_default_to_false_and_are_kept_before_the_command(self): + flags = { + "--no-prompt": "no_prompt", + "--no-certcheck": "no_certcheck", + "--no-cookie": "no_cookie", + "--no-proxy": "no_proxy", + "--continue-if-exists": "continue_if_exists", + } + defaults = parse(self.parser, ["login"]) + for flag, name in flags.items(): + with self.subTest(flag=flag): + assert defaults[name] is False, defaults + assert parse(self.parser, [flag, "login"])[name] is True + + def test_every_global_default_is_present_for_every_command(self): + expected = {a.dest for a in parent_parser_with_global_options()._actions if a.default is not argparse.SUPPRESS} + assert len(expected) >= 19, expected + for command in COMMANDS: + with self.subTest(command=command[0]): + missing = expected - set(parse(self.parser, command)) + assert not missing, missing + + def test_help_command_accepts_global_options_in_either_position(self): + defaults = parse(self.parser, ["help"]) + assert defaults["server"] is None, defaults + assert defaults["logging_level"] == "INFO", defaults + assert parse(self.parser, ["-s", "the-server", "help"])["server"] == "the-server" + assert parse(self.parser, ["help", "-s", "the-server"])["server"] == "the-server" + + if __name__ == "__main__": unittest.main() From bf7b4465bf0e7f38bc7a773ff6be3bbbc8bd246e Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 6 Nov 2026 22:01:09 -0800 Subject: [PATCH 3/3] Reject --proxy together with --no-proxy Global options can be given before and after the subcommand, and argparse only enforces mutually exclusive groups within one parser, so 'tabcmd --no-proxy login --proxy host:1' kept both and --no-proxy silently won. Check the pair in Session, where the connection is configured. Co-Authored-By: Claude Code --- tabcmd/commands/auth/session.py | 2 ++ .../locales/en/tabcmd_messages_en.properties | 1 + tests/commands/test_session.py | 25 +++++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/tabcmd/commands/auth/session.py b/tabcmd/commands/auth/session.py index eb8a0448..4f01e2cf 100644 --- a/tabcmd/commands/auth/session.py +++ b/tabcmd/commands/auth/session.py @@ -81,6 +81,8 @@ def _update_session_data(self, args): self.no_prompt = args.no_prompt # have to set this on every call? self.certificate = args.certificate or self.certificate self.no_certcheck = args.no_certcheck # have to set this on every call? + if args.no_proxy and args.proxy: + Errors.exit_with_error(self.logger, _("session.errors.proxy_conflict")) self.no_proxy = args.no_proxy # have to set this on every call? self.proxy = args.proxy or self.proxy self.timeout = self.timeout_as_integer(self.logger, args.timeout, self.timeout) diff --git a/tabcmd/locales/en/tabcmd_messages_en.properties b/tabcmd/locales/en/tabcmd_messages_en.properties index d9b5f57f..cad45868 100644 --- a/tabcmd/locales/en/tabcmd_messages_en.properties +++ b/tabcmd/locales/en/tabcmd_messages_en.properties @@ -122,6 +122,7 @@ session.auto_site_login=Auto-sign in to site: {0} session.connecting=Connecting to the server... session.continuing_session=Continuing previous session session.errors.missing_arguments=Cannot sign in because of missing arguments: {0} +session.errors.proxy_conflict=Cannot use --proxy together with --no-proxy. Pass only one of them. session.errors.password_file_not_found=Password file not found: '{0}'. Check the path passed to --password-file. session.errors.token_file_not_found=Token file not found: '{0}'. Check the path passed to --token-file. session.errors.script_no_password=Tabcmd was run from a script or IDE but no password was provided. A password must be provided as an argument to tabcmd in order to log in. diff --git a/tests/commands/test_session.py b/tests/commands/test_session.py index bd14bf31..661aec23 100644 --- a/tests/commands/test_session.py +++ b/tests/commands/test_session.py @@ -199,6 +199,31 @@ def test__create_new_username_credential_succeeds_from_args(self, mock_pass): auth = active_session._create_new_credential(test_args.password, Session.PASSWORD_CRED_TYPE) +class ProxyOptionTests(unittest.TestCase): + def test_proxy_with_no_proxy_is_rejected(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = "host:1" + test_args.no_proxy = True + with self.assertRaises(SystemExit): + Session()._update_session_data(test_args) + + def test_proxy_alone_is_accepted(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = "host:1" + test_args.no_proxy = False + mock_session = Session() + mock_session._update_session_data(test_args) + assert mock_session.proxy == "host:1" + + def test_no_proxy_alone_is_accepted(self): + test_args = Namespace(**vars(args_to_mock)) + test_args.proxy = None + test_args.no_proxy = True + mock_session = Session() + mock_session._update_session_data(test_args) + assert mock_session.no_proxy is True + + class PromptingTests(unittest.TestCase): def test_show_prompt_if_user_didnt_say(self): test_args = Namespace(**vars(args_to_mock))