Skip to content

fix(parser): escape unescaped '%' in help text for Python 3.14 argpar… #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
YangAn-microsoft wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(parser): escape unescaped '%' in help text for Python 3.14 argpar… #299

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions knack/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ def create_global_parser(cli_ctx=None):
def _add_argument(obj, arg):
""" Only pass valid argparse kwargs to argparse.ArgumentParser.add_argument """
argparse_options = {name: value for name, value in arg.options.items() if name in ARGPARSE_SUPPORTED_KWARGS}

# Python 3.14+ validates help strings at add_argument() time by running
# `help_string % params` (see https://github.com/python/cpython/pull/124899).
# Any bare '%' in help text causes ValueError/TypeError/KeyError since '%' is
# the Python old-style string formatting operator. CLI authors are not expected
# to escape '%' in help strings (no such requirement exists in docs), so we
# transparently escape '%' to '%%' before argparse validation and restore it
# afterward so users see the original unescaped text in help output.
help_string = argparse_options.get('help')
if help_string and '%' in help_string:
argparse_options['help'] = help_string.replace('%', '%%')

if arg.options_list:
scrubbed_options_list = []
for item in arg.options_list:
Expand All @@ -61,13 +73,22 @@ def __new__(cls, *args, **kwargs):
setattr(option, 'deprecate_info', item)
item = option
scrubbed_options_list.append(item)
return obj.add_argument(*scrubbed_options_list, **argparse_options)

if 'required' in argparse_options:
del argparse_options['required']
if 'metavar' not in argparse_options:
argparse_options['metavar'] = '<{}>'.format(argparse_options['dest'].upper())
return obj.add_argument(**argparse_options)
param = obj.add_argument(*scrubbed_options_list, **argparse_options)
else:
if 'required' in argparse_options:
del argparse_options['required']
if 'metavar' not in argparse_options:
argparse_options['metavar'] = '<{}>'.format(argparse_options['dest'].upper())
param = obj.add_argument(**argparse_options)

# Restore the original unescaped help text so the CLI help renderer displays
# the correct single '%' characters to the user. Knack's help renderer reads
# action.help directly (not via argparse's _expand_help), so without this
# restore users would see '%%' literally in help output.
if help_string and '%' in help_string:
param.help = help_string
Comment on lines +84 to +89
Copy link
Copy Markdown
Member

@jiasli jiasli Apr 21, 2026

Choose a reason for hiding this comment

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

It may not be a good solution to hack into an argparse.Action, as it may cause other unexpected side effects or failures as argparse actually expects % in the help message to be escaped.

Would it be possible to use help_string % params like argparse when printing the help message?

Copy link
Copy Markdown
Author

@YangAn-microsoft YangAn-microsoft Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback. I agree this area needs to be conservative.

I evaluated applying argparse-style help interpolation in Knack, but I believe that would be a broader behavior change, not just a percent-sign fix.

  1. It introduces a new contract for all command modules (both azure-cli and extensions): bare percent signs in help text become invalid unless escaped.
  2. It enables interpolation semantics such as %(default)s, %(prog)s, %(metavar)s, which can change current output and introduce new KeyError/TypeError failure modes for malformed templates.
  3. It would require coordinated updates across multiple teams. I scanned azure-cli-extensions and found existing bare-percent help text patterns that would need updates if Knack switched to argparse-style interpolation.
  • networkcloud: 6 hits (for example, 50%, percentage text)
  • databox: 4 hits (password character set includes %)
  • datamigration: 3 hits (150%, %LocalAppData%)
  • containerapp: 2 hits (maximum %, URL with %2C)
  • cosmosdb-preview: 1 hit ((cn=%s))
  • machinelearningservices: 1 hit (100%)
  • storagesync: 1 hit (50%)

On the argparse.Action concern specifically: the current fix is intentionally scoped to help text only. It does not modify parsing semantics such as type, nargs, choices, required, defaults, or validators. It only escapes percent signs to pass Python 3.14 add_argument validation, then restores original help text so displayed help remains unchanged.

Given that, I believe the current approach is the lowest-risk compatibility fix for Python 3.14 without forcing broad migrations and contract sync.

Copy link
Copy Markdown
Collaborator

@bebound bebound Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

What's the cost of changing it to _expand_help? If %% is the right way, we should not restore to a deprecated method.

Knack's help renderer reads action.help directly (not via argparse's _expand_help)

Copy link
Copy Markdown
Author

@YangAn-microsoft YangAn-microsoft Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the question. The cost of switching to _expand_help is ecosystem migration and rollout risk.

  • It changes the contract for help text: literal % must be escaped as %%.

  • That means existing help strings with unescaped % (in both azure-cli and extensions) need to be updated.

  • This cannot be treated as an isolated Knack change. It needs coordinated rollout with the help-text updates.

  • If rollout is not coordinated:

    • help text updated first, Knack unchanged: behavior can differ from current rendering expectations
    • Knack changed first, help text not updated: exceptions can occur from unescaped %

I agree extension owners may not need direct notification if the check is enforced by test/CI, because unescaped % should fail validation and be caught. But that still means this is a breaking contract change and requires planned migration, not a narrow compatibility fix.

That is why I treated the current patch as a compatibility fix and considered _expand_help migration a separate, broader change.

Copy link
Copy Markdown
Member

@jiasli jiasli Apr 23, 2026

Choose a reason for hiding this comment

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

As this PR is not changing how knack supports %-formatting (not supported). Some problems may occur:

  1. If the help string contains %% as argparse requested, it will be printed as %%, not %.
  2. If the help string contains %(...), it will not be %-formatted.

An alternative solution:

  1. If there is %( or %% in th help string, leave the string as is.
  2. If not, escape % to %%, like AWS CLI did (Add support for Python 3.14 aws/aws-cli#9790).

The bottom line is that knack should support %-formatting when printing the help message, thus not rejecting correct usages according to argparse.


return param

@staticmethod
def _expand_prefixed_files(args):
Expand Down