diff --git a/CHANGELOG.md b/CHANGELOG.md index ec20c2e..8179ea8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and versions are tracked in the repo-root `VERSION` file. ### Fixed +- Detect JSON capture without running Click callbacks, callable defaults, type + converters, or close hooks a second time; respect option-value arity so a + payload equal to `--json` remains human output. - Preserve explicit application identities losslessly while using collision-resistant, path-safe runtime namespace components. - Give `BatteriesIncludedConfigLoader.cli_name` a documented identity role by diff --git a/docs/json-contracts.md b/docs/json-contracts.md index 625f98c..962148b 100644 --- a/docs/json-contracts.md +++ b/docs/json-contracts.md @@ -26,6 +26,11 @@ in memory and rolls the remainder to a temporary file, so both temporary-disk use and finalization memory remain bounded. The temporary file is removed when the invocation ends. +The mode check respects Click option arity: a value such as +`--payload --json` does not activate JSON when `--json` is the payload. It does +not run consumer callbacks, defaults, type converters, or close hooks as a +second parse before the real invocation. + If a command exceeds the limit, base-cli emits one `base-cli.error` envelope with `code: "capture_limit"` and exit code `1`; it never silently truncates the captured text. Use the NDJSON contract for larger record sets. diff --git a/lib/python/base_cli/_app_core.py b/lib/python/base_cli/_app_core.py index 54f088b..b1f1435 100644 --- a/lib/python/base_cli/_app_core.py +++ b/lib/python/base_cli/_app_core.py @@ -126,6 +126,7 @@ class _InvocationState: options_parsed: bool = False attached_completion: bool = False json_output: bool = False + output_router: Any = None @dataclass(frozen=True) @@ -303,6 +304,10 @@ def _capture_standard_options(standard: dict[str, Any], owner_app: App) -> None: state.quiet = bool(standard.get("quiet")) state.json_output = bool(standard.get("json")) state.options_parsed = True + router = state.output_router + resolve_json_output = getattr(router, "resolve_json_output", None) + if callable(resolve_json_output): + resolve_json_output(state.json_output) def _capture_effective_output_options( diff --git a/lib/python/base_cli/_run.py b/lib/python/base_cli/_run.py index 09dc442..c875eec 100644 --- a/lib/python/base_cli/_run.py +++ b/lib/python/base_cli/_run.py @@ -17,7 +17,6 @@ _INVOCATION_ARGV, _INVOCATION_MAIN_BYPASS, _INVOCATION_STATE, - _LIFECYCLE_CAPTURE_META_KEY, DISPLAY_COMMAND_ENV, App, _InvocationState, @@ -88,6 +87,99 @@ def close(self) -> None: self._stream.close() +class _DeferredJsonCapture(io.TextIOBase): + """Buffer parser-time stdout until Click resolves the lifecycle output mode.""" + + encoding = "utf-8" + errors = "strict" + + def __init__(self, stream: TextIO, limit_bytes: int) -> None: + super().__init__() + self._stdout = stream + self._limit_bytes = limit_bytes + self._bytes_written = 0 + self._mode: bool | None = None + self._stream = cast( + TextIO, + tempfile.SpooledTemporaryFile( + max_size=min(limit_bytes, 1_048_576), + mode="w+", + encoding="utf-8", + newline="", + ), + ) + + @property + def pending(self) -> bool: + return self._mode is None + + @property + def json_output(self) -> bool: + return self._mode is True + + def resolve_json_output(self, enabled: bool) -> None: + if self._mode is not None: + if self._mode == enabled: + return + if not self._mode and enabled: + raise RuntimeError("JSON output mode was resolved after stdout had been released.") + if enabled: + self._mode = True + if self._bytes_written > self._limit_bytes: + raise JsonCaptureLimitError( + f"JSON stdout exceeded the {_format_bytes(self._limit_bytes)} limit; use NDJSON for large record sets." + ) + return + + self._stream.flush() + self._stream.seek(0) + while chunk := self._stream.read(64 * 1024): + self._stdout.write(chunk) + self._stdout.flush() + self._mode = False + + def write(self, value: str) -> int: + if self._mode is False: + return self._stdout.write(value) + encoded_size = len(value.encode("utf-8")) + if self._mode is True and self._bytes_written + encoded_size > self._limit_bytes: + raise JsonCaptureLimitError( + f"JSON stdout exceeded the {_format_bytes(self._limit_bytes)} limit; use NDJSON for large record sets." + ) + written = self._stream.write(value) + self._bytes_written += encoded_size + return written + + def flush(self) -> None: + if self._mode is False: + self._stdout.flush() + else: + self._stream.flush() + + def read(self, size: int | None = -1) -> str: + return self._stream.read(-1 if size is None else size) + + def seek(self, offset: int, whence: int = io.SEEK_SET) -> int: + return self._stream.seek(offset, whence) + + def tell(self) -> int: + return self._stream.tell() + + def isatty(self) -> bool: + if self._mode is True: + return False + try: + return bool(self._stdout.isatty()) + except (AttributeError, OSError, ValueError): + return False + + def close(self) -> None: + try: + super().close() + finally: + self._stream.close() + + def _format_bytes(value: int) -> str: if value % 1_048_576 == 0: return f"{value // 1_048_576} MiB" @@ -112,6 +204,15 @@ def run_app( return ExitCode.FAILURE args = list(sys.argv[1:] if argv is None else argv) + command = app.click_command + click = dialect_for_command(command) + preliminary_json = _json_requested( + args, + app.lifecycle_options, + default_map=_command_default_map(command), + command=command, + prog_name=app.name, + ) leading_debug, leading_quiet = _leading_output_flags( args, app.lifecycle_options, @@ -123,7 +224,7 @@ def run_app( debug_option=_primary_lifecycle_declaration( app.lifecycle_options.debug, ), - json_output=_json_requested(args, app.lifecycle_options), + json_output=bool(preliminary_json), ) state_token = _INVOCATION_STATE.set(state) output_capture: TextIO | None = None @@ -131,22 +232,24 @@ def run_app( try: display_command = app.profile.display_command() invocation_argv = _effective_invocation_argv(app, args, display_command) - command = app.click_command - click = dialect_for_command(command) - if not state.json_output: - state.json_output = _json_requested( - args, - app.lifecycle_options, - default_map=_command_default_map(command), - command=command, - prog_name=display_command or app.name, - ) + json_decision = _json_requested( + args, + app.lifecycle_options, + default_map=_command_default_map(command), + command=command, + prog_name=display_command or app.name, + ) + state.json_output = bool(json_decision) invocation_token = _INVOCATION_ARGV.set(invocation_argv) try: bypass_token = _INVOCATION_MAIN_BYPASS.set(command) - # Capture only an active JSON invocation. Human and NDJSON - # paths retain the real stdout stream and its flush behavior. - output_capture = _new_json_capture() if state.json_output else None + if json_decision is None: + output_capture = cast(TextIO, _DeferredJsonCapture(sys.stdout, _MAX_JSON_CAPTURE_BYTES)) + state.output_router = output_capture + else: + # Human and NDJSON paths retain the real stdout stream; + # only a resolved JSON invocation uses the bounded spool. + output_capture = _new_json_capture() if state.json_output else None try: if output_capture is None: result = command.main( @@ -162,6 +265,11 @@ def run_app( standalone_mode=False, ) finally: + if isinstance(output_capture, _DeferredJsonCapture): + if output_capture.pending: + output_capture.resolve_json_output(state.json_output) + if not output_capture.json_output: + output_capture = None _reset_context_var(_INVOCATION_MAIN_BYPASS, bypass_token) finally: _reset_context_var(_INVOCATION_ARGV, invocation_token) @@ -258,11 +366,20 @@ def _json_requested( default_map: Mapping[str, Any] | None = None, command: Any | None = None, prog_name: str | None = None, -) -> bool: +) -> bool | None: option = lifecycle_options.json if option is None: return False + if command is not None: + return _click_lifecycle_value( + command, + args, + option, + prog_name, + default_map=default_map, + ) + positive_declarations, negative_declarations = _lifecycle_flag_declarations(option) explicit_value: bool | None = None for argument in args: @@ -277,16 +394,6 @@ def _json_requested( if explicit_value is not None: return explicit_value - # Once the command object is available, let Click resolve the option. Its - # parser knows about auto_envvar_prefix, nested default maps, callable - # defaults, and the complete boolean environment grammar (including `t` - # and `y`). This is used only for the pre-invocation capture decision; the - # real command is still parsed and invoked exactly once below. - if command is not None: - click_value = _click_lifecycle_value(command, args, option, prog_name) - if click_value is not None: - return click_value - if option.envvar is not None: envvars = (option.envvar,) if isinstance(option.envvar, str) else option.envvar if any(os.environ.get(name, "").lower() in {"1", "true", "yes", "on"} for name in envvars): @@ -296,6 +403,8 @@ def _json_requested( value = default_map.get(key) if isinstance(value, bool): return value + if callable(option.default): + return None return option.default is True @@ -304,86 +413,132 @@ def _click_lifecycle_value( args: list[str], option: LifecycleOption, prog_name: str | None, + *, + default_map: Mapping[str, Any] | None = None, ) -> bool | None: - """Resolve a lifecycle flag with the owning Click command parser.""" + """Inspect Click's raw parsers without processing parameter values. + + ``make_context`` is intentionally avoided: parsing a temporary context + executes user parameter types, defaults, callbacks, lazy group resolvers, + and close hooks. The low-level parser only tokenizes values and option + arity, which is enough to decide whether stdout needs JSON capture. + """ - contexts: list[Any] = [] + click = dialect_for_command(command) + destination = option.name or _option_destination(option) + selected: tuple[int, int, bool] | None = None + unresolved: tuple[int, int] | None = None current_command = command current_args = list(args) - current_context: Any | None = None + parent_context: Any | None = None + info_name = prog_name or getattr(command, "name", None) or "cli" + depth = 0 try: - current_context = command.make_context( - prog_name, - current_args, - resilient_parsing=True, - ) - contexts.append(current_context) - while current_args: - resolve_command = getattr(current_command, "resolve_command", None) - if not callable(resolve_command): - break - command_name, next_command, remaining = resolve_command( - current_context, - _remaining_context_args(current_context), - ) - if command_name is None or next_command is None: - break - next_context = next_command.make_context( - command_name, - remaining, - parent=current_context, + for _ in range(64): + context_settings = dict(getattr(current_command, "context_settings", None) or {}) + context_settings.update( resilient_parsing=True, + allow_extra_args=True, + ignore_unknown_options=True, ) - contexts.append(next_context) - current_command = next_command - current_context = next_context - current_args = _remaining_context_args(current_context) - - destination = option.name or _option_destination(option) - for context in reversed(contexts): - params = getattr(context, "params", {}) - value = params.get(destination) if isinstance(params, Mapping) else None - if isinstance(value, bool): - return value + if parent_context is None and default_map is not None: + context_settings["default_map"] = default_map + context = click.Context( + current_command, + parent=parent_context, + info_name=info_name, + **context_settings, + ) + parameters = current_command.get_params(context) + parser = current_command.make_parser(context) + parsed_values, remaining, _parameter_order = parser.parse_args(list(current_args)) + parameter = next((candidate for candidate in parameters if candidate.name == destination), None) + + if parameter is not None: + parsed_value = parsed_values.get(destination) + if isinstance(parsed_value, bool): + selected = _prefer_json_value(selected, 4, depth, parsed_value) + + try: + environment_value = parameter.value_from_envvar(context) + if environment_value is not None: + converted = parameter.type_cast_value(context, environment_value) + if isinstance(converted, bool): + selected = _prefer_json_value(selected, 3, depth, converted) + except Exception: + # The real Click parse owns diagnostics for malformed + # environment values; preflight only needs a safe hint. + pass + + parameter_default = getattr(parameter, "default", None) + if callable(parameter_default): + unresolved = _prefer_json_source(unresolved, 1, depth) + elif isinstance(parameter_default, bool) or parameter_default is None: + selected = _prefer_json_value( + selected, + 1, + depth, + bool(parameter_default), + ) + context_default_map = getattr(context, "default_map", None) - if isinstance(context_default_map, Mapping): - mapped_value = context_default_map.get(destination) + if isinstance(context_default_map, Mapping) and destination in context_default_map: + mapped_value = context_default_map[destination] if isinstance(mapped_value, bool): - return mapped_value - meta = getattr(context, "meta", {}) - captures = meta.get(_LIFECYCLE_CAPTURE_META_KEY) if isinstance(meta, Mapping) else None - if isinstance(captures, Mapping): - for captured in captures.values(): - if not isinstance(captured, Mapping): - continue - raw = captured.get("json") - raw_value = getattr(raw, "value", None) - if isinstance(raw_value, bool): - return raw_value + selected = _prefer_json_value(selected, 2, depth, mapped_value) + elif callable(mapped_value): + unresolved = _prefer_json_source(unresolved, 2, depth) + + if not remaining or not isinstance(getattr(current_command, "commands", None), Mapping): + break + command_name = remaining[0] + commands = current_command.commands + next_command = commands.get(command_name) + normalize = getattr(context, "token_normalize_func", None) + if next_command is None and callable(normalize): + next_command = commands.get(normalize(command_name)) + if next_command is None: + # A lazy group may resolve this name during the real dispatch. + # Defer only successful stdout until Click supplies the actual + # lifecycle value; never call get_command speculatively. + resolver = getattr(type(current_command), "get_command", None) + group_type = getattr(click, "Group", None) + base_resolver = getattr(group_type, "get_command", None) + if callable(resolver) and resolver is not base_resolver: + unresolved = _prefer_json_source(unresolved, 4, depth + 1) + break + parent_context = context + current_command = next_command + current_args = remaining[1:] + info_name = command_name + depth += 1 except (Exception, SystemExit): - # Invalid command lines still need the lightweight explicit-token - # detector above so Click can render its normal machine error. A - # resilient parse may not be able to resolve a leaf command; in that - # case retain the existing fallback behavior. + # The real parser owns malformed-command diagnostics. Keep the best + # known source without running consumer parsing hooks here. + pass + + if unresolved is not None and (selected is None or unresolved >= selected[:2]): return None - finally: - for context in reversed(contexts): - close = getattr(context, "close", None) - if callable(close): - close() - return None - - -def _remaining_context_args(context: Any) -> list[str]: - """Return unparsed group/command arguments without Click deprecation warnings.""" - - values = getattr(context, "__dict__", {}) - if isinstance(values, Mapping): - protected = values.get("_protected_args", values.get("protected_args", ())) - else: - protected = () - args = getattr(context, "args", ()) - return [*protected, *args] + return False if selected is None else selected[2] + + +def _prefer_json_value( + selected: tuple[int, int, bool] | None, + rank: int, + depth: int, + value: bool, +) -> tuple[int, int, bool]: + candidate = (rank, depth, value) + return candidate if selected is None or candidate[:2] >= selected[:2] else selected + + +def _prefer_json_source( + selected: tuple[int, int] | None, + rank: int, + depth: int, +) -> tuple[int, int]: + candidate = (rank, depth) + return candidate if selected is None or candidate >= selected else selected def _option_destination(option: LifecycleOption) -> str: diff --git a/tests/test_json_contracts.py b/tests/test_json_contracts.py index 388475a..b6d4459 100644 --- a/tests/test_json_contracts.py +++ b/tests/test_json_contracts.py @@ -259,11 +259,17 @@ def main(ctx: base_cli.Context) -> None: self.assertEqual(envelope["details"]["stdout"], "hello from auto env\n") def test_json_click_parser_handles_callable_defaults(self) -> None: + default_calls: list[bool] = [] + + def default_json() -> bool: + default_calls.append(True) + return True + app = base_cli.App( name="json-callable-default", log_to_file=False, lifecycle_options=base_cli.LifecycleOptions( - json=base_cli.LifecycleOption("--json", default=lambda: True), + json=base_cli.LifecycleOption("--json", default=default_json), ), ) @@ -275,6 +281,7 @@ def main(ctx: base_cli.Context) -> None: with tempfile.TemporaryDirectory() as home: result = base_cli.testing.invoke(app, [], home=Path(home)) self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(default_calls, [True]) envelope = json.loads(result.stdout) self.assertEqual(envelope["details"]["stdout"], "hello from default\n") diff --git a/tests/test_run_json_preflight.py b/tests/test_run_json_preflight.py new file mode 100644 index 0000000..598a0ee --- /dev/null +++ b/tests/test_run_json_preflight.py @@ -0,0 +1,194 @@ +from __future__ import annotations + +import importlib.util +import json +import tempfile +import unittest +from pathlib import Path +from typing import Any + +import base_cli + + +@unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") +class RunJsonPreflightTests(unittest.TestCase): + def _attached_click_command(self, counters: dict[str, Any], *, lazy: bool = False) -> Any: + import click + + class CountingType(click.ParamType): + name = "counted" + + def convert(self, value: Any, param: Any, ctx: Any) -> Any: + del param, ctx + counters["conversions"] = counters.get("conversions", 0) + 1 + return value + + def default_payload() -> str: + counters["defaults"] = counters.get("defaults", 0) + 1 + return "default" + + def record_payload(ctx: Any, _parameter: Any, value: str) -> str: + counters["callbacks"] = counters.get("callbacks", 0) + 1 + ctx.call_on_close(lambda: counters.__setitem__("close_hooks", counters.get("close_hooks", 0) + 1)) + return value + + @click.command("status") + @click.option( + "--payload", + "-p", + default=default_payload, + type=CountingType(), + callback=record_payload, + ) + @click.option("--pair", nargs=2, type=(str, str)) + @click.argument("tail", required=False) + def status(payload: str, pair: tuple[str, str] | None, tail: str | None) -> None: + counters["commands"] = counters.get("commands", 0) + 1 + click.echo(f"PAYLOAD={payload};PAIR={pair};TAIL={tail}") + + class LazyGroup(click.Group): + def get_command(self, ctx: Any, cmd_name: str) -> Any: + counters["lazy_resolutions"] = counters.get("lazy_resolutions", 0) + 1 + return super().get_command(ctx, cmd_name) + + group_class = LazyGroup if lazy else click.Group + group = group_class(name="probe", commands={"status": status}) + app = base_cli.App( + name="probe", + log_to_file=False, + lifecycle_options=base_cli.LifecycleOptions( + json=base_cli.LifecycleOption("--json/--no-json"), + ), + ) + return app.attach(group) + + def _native_app(self) -> base_cli.App: + app = base_cli.App( + name="probe", + log_to_file=False, + lifecycle_options=base_cli.LifecycleOptions( + json=base_cli.LifecycleOption("--json/--no-json"), + ), + ) + + @app.subcommand() + @base_cli.option("--payload", "-p", default="default") + @base_cli.option("--pair", nargs=2, type=(str, str)) + @base_cli.argument("tail", required=False) + def status( + ctx: base_cli.Context[Any, Any, Any], + payload: str, + pair: tuple[str, str] | None, + tail: str | None, + ) -> None: + del ctx + print(f"PAYLOAD={payload};PAIR={pair};TAIL={tail}") + + return app + + def test_human_invocation_runs_callbacks_defaults_converters_and_close_hooks_once(self) -> None: + counters: dict[str, Any] = {} + command = self._attached_click_command(counters) + + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(command, ["status"], home=Path(home)) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn("PAYLOAD=default", result.stdout) + self.assertEqual(counters, {"defaults": 1, "conversions": 1, "callbacks": 1, "commands": 1, "close_hooks": 1}) + + def test_json_invocation_runs_callbacks_defaults_converters_and_close_hooks_once(self) -> None: + counters: dict[str, Any] = {} + command = self._attached_click_command(counters) + + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(command, ["--json", "status"], home=Path(home)) + + self.assertEqual(result.exit_code, 0, result.output) + envelope = json.loads(result.stdout) + self.assertEqual(envelope["details"]["stdout"].split(";", maxsplit=1)[0], "PAYLOAD=default") + self.assertEqual(counters, {"defaults": 1, "conversions": 1, "callbacks": 1, "commands": 1, "close_hooks": 1}) + + def test_early_parse_errors_do_not_run_consumer_callbacks_during_mode_detection(self) -> None: + for args, expect_json in ( + (["status", "--unknown"], False), + (["--json", "status", "--unknown"], True), + ): + with self.subTest(args=args): + counters: dict[str, Any] = {} + command = self._attached_click_command(counters) + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(command, args, home=Path(home)) + + self.assertEqual(result.exit_code, 2) + self.assertEqual(counters, {}) + if expect_json: + envelope = json.loads(result.stdout) + self.assertEqual(envelope["schema"], "base-cli.error") + self.assertIn("No such option", envelope["message"]) + else: + self.assertEqual(result.stdout, "") + self.assertIn("No such option", result.stderr) + + def test_lazy_command_resolver_is_called_only_by_real_dispatch(self) -> None: + counters: dict[str, Any] = {} + command = self._attached_click_command(counters, lazy=True) + + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(command, ["status"], home=Path(home)) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(counters["lazy_resolutions"], 1) + self.assertEqual(counters["commands"], 1) + self.assertEqual(counters["callbacks"], 1) + self.assertEqual(counters["close_hooks"], 1) + + def test_option_values_and_flags_do_not_false_activate_json_mode(self) -> None: + equivalent_payload_outputs: list[str] = [] + cases = ( + (["status", "--payload", "--json"], "PAYLOAD=--json"), + (["status", "--payload=--json"], "PAYLOAD=--json"), + (["status", "--pair", "first", "--json"], "PAIR=('first', '--json')"), + (["status", "-p--json"], "PAYLOAD=--json"), + (["status", "--", "--json"], "TAIL=--json"), + ) + for args, expected in cases: + with self.subTest(args=args): + app = self._native_app() + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(app, args, home=Path(home)) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn(expected, result.stdout) + self.assertFalse(result.stdout.lstrip().startswith("{"), result.stdout) + if args in ( + ["status", "--payload", "--json"], + ["status", "--payload=--json"], + ["status", "-p--json"], + ): + equivalent_payload_outputs.append(result.stdout) + + self.assertEqual(len(set(equivalent_payload_outputs)), 1) + + def test_root_leaf_and_negated_json_flags_follow_click_precedence(self) -> None: + cases = ( + (["--json", "status"], True), + (["status", "--json"], True), + (["--json", "status", "--no-json"], False), + (["--no-json", "status", "--json"], True), + ) + for args, expect_json in cases: + with self.subTest(args=args): + app = self._native_app() + with tempfile.TemporaryDirectory() as home: + result = base_cli.testing.invoke(app, args, home=Path(home)) + + self.assertEqual(result.exit_code, 0, result.output) + if expect_json: + self.assertEqual(json.loads(result.stdout)["schema"], "base-cli.output") + else: + self.assertTrue(result.stdout.startswith("PAYLOAD="), result.stdout) + + +if __name__ == "__main__": + unittest.main()