Skip to content

fix: make JSON capture preflight side-effect free - #369

Open
codeforester wants to merge 2 commits into
mainfrom
bug/338-20260918-side-effect-free-json-probe
Open

codeforester wants to merge 2 commits into
mainfrom
bug/338-20260918-side-effect-free-json-probe

Conversation

@codeforester

Copy link
Copy Markdown
Contributor

Summary

  • Inspect Click option arity with its raw parser rather than creating and closing resilient contexts, avoiding duplicate consumer callbacks, callable defaults, type conversions, lazy resolution, and close hooks.
  • Preserve deferred callable-default behavior and nested default-map, auto-envvar, root/leaf, and negated-flag precedence.
  • Keep flag-like option values as human output, including multi-value options, attached short values, and -- positional data.

Validation

  • Full pytest suite passed.
  • Repository-wide Ruff, format, mypy, generated API reference, and diff checks passed.

Fixes #338
Fixes #339

finally:
if isinstance(output_capture, _DeferredJsonCapture):
if output_capture.pending:
output_capture.resolve_json_output(state.json_output)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correctness (reproduced): when a Click parse/validation error occurs before the leaf command's wrapper runs, the deferred JSON-capture fallback uses the stale preflight guess instead of the value Click's real parse already computed, silently downgrading a JSON-configured invocation to plain-text output. Reproduced: a subcommand with a required argument, invoked without it and without an explicit --json flag, emits a plain-text usage error instead of the base-cli.error JSON envelope main correctly produces for the same scenario.

parent_context = context
current_command = next_command
current_args = remaining[1:]
info_name = command_name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correctness (reproduced): the subcommand-walk in _click_lifecycle_value sets the child Click Context's info_name to the literal, unnormalized command-line token instead of the name Click's own resolve_command would use after token_normalize_func. With a group using token_normalize_func=str.lower and a default_map keyed by canonical name, invoking with a differently-cased token makes the preflight miss the default_map entry and return a firm-but-wrong False, causing output_capture to be set to None (no capture) while the real parse later resolves json=True — real stdout leaks to the terminal outside the JSON envelope.

args = list(sys.argv[1:] if argv is None else argv)
command = app.click_command
click = dialect_for_command(command)
preliminary_json = _json_requested(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Efficiency: run_app now unconditionally runs the full Click-parser-based preflight (_click_lifecycle_value, which builds a click.Context and calls get_params/make_parser().parse_args() per nested command level) twice per invocation with identical args/command/default_map — the two calls always produce the same result, so the first pass is pure waste. main only performed this resolution once, often skipped via a cheap fast path.

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.

bug: preserve stdout when --json is another option value bug: avoid executing Click callbacks during JSON capture preflight

1 participant