Standardize CLI output and terminal behavior#267
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8d398eea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _record_value(value: object) -> object: | ||
| if value is None: | ||
| return "" | ||
| if isinstance(value, (str, int, float, bool)): | ||
| return value | ||
| return json.dumps(value, sort_keys=True, default=_json_default) |
There was a problem hiding this comment.
When sidemantic query returns common warehouse scalar types such as DATE, TIMESTAMP, or DECIMAL, the default CSV path now routes each non-primitive through json.dumps here, which raises TypeError because _json_default does not handle datetime.date, datetime.datetime, or decimal.Decimal. The previous csv.writer path stringified these values, so queries selecting date dimensions or decimal metrics will fail instead of emitting results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32744ea8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| debug=bool(debug), | ||
| quiet=bool(quiet) if quiet is not None else bool(configured and configured.quiet), | ||
| verbose=bool(verbose) if verbose is not None else bool(configured and configured.verbose), | ||
| requested_format=output_format if output_format is not None else (configured.format if configured else None), |
There was a problem hiding this comment.
When a project has cli.format: csv (or table/jsonl), this stores that default in requested_format, so later sidemantic info --json reaches resolve_output_format(json_output=True) and exits with --json cannot be combined... instead of honoring the explicit command flag. This violates the documented flag > env > project-config precedence and breaks existing --json invocations as soon as a non-JSON CLI default is added to the project.
Useful? React with 👍 / 👎.
| sidemantic validate | ||
| sidemantic validate ./models --verbose | ||
| """ | ||
| output_format = resolve_output_format(json_output=json_output) |
There was a problem hiding this comment.
The new --format json path is resolved here, but if validate_directory() raises while loading/parsing a model (e.g. malformed YAML), the handler below still checks only json_output, so sidemantic validate --format json produces no stdout JSON and only an stderr error whereas --json returns the JSON failure payload. Scripts using the documented format alias or SIDEMANTIC_FORMAT=json lose machine-readable validation failures.
Useful? React with 👍 / 👎.
f32744e to
d1c8210
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c8210552
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import csv | ||
| import sys | ||
|
|
||
| records = [dict(zip(columns, row, strict=True)) for row in rows] |
There was a problem hiding this comment.
When a query returns duplicate column names, this converts each row to a dict before rendering, so later duplicate names overwrite earlier values and CSV/table output repeats the last value for every duplicate column. For example, legal SQL like SELECT 1 AS x, 2 AS x now emits x,x followed by 2,2, whereas the previous positional csv.writer path preserved 1,2, so default sidemantic query output can silently corrupt results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e1acda8a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state.plain: | ||
| if json_output or requested not in {None, "table"}: | ||
| raise InvocationError("--plain cannot be combined with --format csv, json, or jsonl") |
There was a problem hiding this comment.
When a default format comes from SIDEMANTIC_FORMAT or cli.format and the user passes --plain, this check still treats the lower-precedence default as an active --format and exits, e.g. SIDEMANTIC_FORMAT=json sidemantic info --plain fails instead of producing plain output. The inverse also happens with a configured/env plain default and an explicit --json/--format json, so one-off command flags cannot override defaults despite the documented flag > env > project-config precedence.
Useful? React with 👍 / 👎.
|
|
||
| selected = output_format or resolve_output_format() | ||
| rendered = render_records(records, columns=columns, output_format=selected, json_value=json_value) | ||
| emit_result(rendered) |
There was a problem hiding this comment.
When a JSONL-producing command has no records, render_records(..., output_format="jsonl") returns an empty string, but this still writes a newline. Commands such as preagg recommend --format jsonl with no recommendations therefore emit a blank JSONL line, which is not a JSON object and causes line-by-line parsers that call json.loads(line) to fail.
Useful? React with 👍 / 👎.
| except Exception as e: | ||
| if engine == "rust" or not fallback: | ||
| if json_output: | ||
| if output_format == "json": |
There was a problem hiding this comment.
When validate --engine rust fails before the Python validation path and the requested output is csv, jsonl, or plain, this branch emits only a stderr error because it special-cases json and bypasses emit_records. For example, sidemantic validate --engine rust --format jsonl produces no stdout payload, so scripts lose the machine-readable failure report that the later parse-error path now preserves.
Useful? React with 👍 / 👎.
8e1acda to
26d6650
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d66508aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else: | ||
| emit_result(rewritten_sql) |
There was a problem hiding this comment.
When a caller explicitly requests sidemantic query --dry-run --format csv, this fallback still writes the raw rewritten SQL instead of using the standard record renderer. Any rewritten SQL containing commas or newlines is therefore not parseable as the documented CSV format, while the adjacent JSON/JSONL branches do produce a structured sql record.
Useful? React with 👍 / 👎.
| emit_records( | ||
| [{"valid": not errors, "spec": str(spec), "tabs": len(document.tabs), "charts": chart_count}], | ||
| output_format=output_format, | ||
| json_value=payload, |
There was a problem hiding this comment.
When dashboard validate finds errors and the requested structured format is jsonl, csv, table, or --plain, the emitted record omits the errors list and json_value is only used by the JSON renderer. The command then exits 1 with no actionable error messages in the selected output, unlike the legacy --json payload and the main validate command's error records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9767e5a373
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def _json_default(value: object) -> Any: | ||
| if isinstance(value, Decimal): | ||
| return float(value) |
There was a problem hiding this comment.
When query --format json or jsonl returns a DECIMAL that is outside binary-float precision, this coerces it through float() and silently changes the value; for example 9007199254740993::DECIMAL(38,0) is emitted as 9007199254740992.0. Since these formats are intended for automation and decimal metrics commonly need exact values, serialize Decimal without lossy conversion (for example as a string) rather than rounding it here.
Useful? React with 👍 / 👎.
9767e5a to
c3505a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3a3bf7f7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return value | ||
| if isinstance(value, (Decimal, date, datetime, time, Path)): | ||
| return str(value) | ||
| return json.dumps(value, sort_keys=True, default=_json_default) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier scalar fix: when a query returns a BINARY/BLOB/bytea column (DuckDB returns BLOB as bytes), default CSV/table/plain rendering reaches this json.dumps fallback and raises TypeError: Object of type bytes is not JSON serializable. The previous csv.writer path stringified these values, so selecting binary columns now makes even default sidemantic query fail instead of emitting results.
Useful? React with 👍 / 👎.
| if selected_plain and selected_format not in {None, "table"}: | ||
| if format_priority > plain_priority: | ||
| selected_plain = False | ||
| elif plain_priority > format_priority: | ||
| selected_format = None |
There was a problem hiding this comment.
The remaining precedence case is --format table: if SIDEMANTIC_PLAIN=1 or cli.plain: true is set and the user passes --format table, this condition skips conflict resolution because table is allowed, so state.plain remains true and the command still emits TSV. That leaves an explicit command flag unable to override a lower-precedence plain default, contrary to the documented flag > env > project-config order.
Useful? React with 👍 / 👎.
6f73e1e to
78390a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78390a29ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| rewritten_sql = rewriter.rewrite(sql) | ||
| typer.echo(rewritten_sql) | ||
| if output_format in {"csv", "json", "jsonl"}: |
There was a problem hiding this comment.
When sidemantic query ... --dry-run is run without an explicit --format, resolve_output_format(default="csv") returns csv, so this new branch now emits a CSV document with a sql header instead of the raw rewritten SQL. That breaks the existing documented compile-only workflow and any script piping the dry-run output into a SQL client; only explicit --format csv/json/jsonl should use the record renderer while the default dry-run path should continue writing raw SQL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3027b5deb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cli_state().requested_format is not None or cli_state().plain: | ||
| resolve_output_format() |
There was a problem hiding this comment.
When SIDEMANTIC_FORMAT or cli.format is set to csv, json, or jsonl, this root-level resolution runs for every command and sets cli_state().machine_output=True. Unstructured commands that only surface operational details through emit_diagnostic—for example server api/server postgres startup URLs and auth status—do not emit any formatted stdout replacement, so those diagnostics are silently dropped just because a default format was configured for other structured commands. Resolve the format, or at least flip machine-output mode, only inside commands that actually render structured records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee6fbf6147
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| global _loaded_config, _project_context | ||
|
|
||
| cli_state().reset(debug=debug) | ||
| format_explicit = ctx.get_parameter_source("output_format") is click.core.ParameterSource.COMMANDLINE |
There was a problem hiding this comment.
In any environment that satisfies the published dependency floor (typer>=0.9.0), Typer 0.9.0 allows Click 7.1.1 while this source-tracking API is a Click 8 API (Click docs, Typer 0.9 metadata). With Click 7.x installed, this first callback line raises AttributeError before every non-help CLI command, so the package needs a Click 8+/newer Typer constraint or a fallback that avoids get_parameter_source.
Useful? React with 👍 / 👎.
| if not progress_enabled(): | ||
| yield | ||
| return | ||
| from rich.console import Console |
There was a problem hiding this comment.
When progress is enabled (interactive stderr, not quiet/plain/machine/CI), this lazy import now runs, but the project still permits Typer 0.9.0, whose base metadata lists rich only under extras such as all (Typer 0.9 metadata). A valid install without Rich will hit ModuleNotFoundError: rich before convert, server startup, or preagg progress work starts, so declare Rich/newer Typer or fall back when it is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca6edac4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "click>=8.0.0", | ||
| "typer>=0.9.0", |
There was a problem hiding this comment.
Fresh evidence beyond the earlier Click 7 finding: this new Click floor still leaves the adjacent typer>=0.9.0 able to resolve to Typer 0.9 with Click 8.2/8.3 in environments that already constrain Typer. Typer's own release notes say 0.15.4 pinned Click <8.2, 0.16.0 added Click 8.2 compatibility, and 0.18.0 restored Click 8.3 compatibility (https://typer.tiangolo.com/release-notes/), so a dependency set still allowed here can crash while rendering help/errors before users can recover. Please either raise the Typer floor to a compatible version or cap Click accordingly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d81510d644
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| output_format: str = typer.Option( | ||
| None, | ||
| "--format", |
There was a problem hiding this comment.
Because --format is now a root option and is hoisted after subcommands, sidemantic explain ... --format csv or --format jsonl is accepted, but explain_sql_command still unconditionally calls emit_json(...), so stdout remains a JSON object rather than the requested format. In scripts that pass an explicit format, this silently violates the selected output contract; either honor the standard formats for explain or reject non-JSON formats for that command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0356f76f1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state.plain: | ||
| raise InvocationError("explain does not support --plain; use --format json") | ||
| if state.requested_format not in {None, "json"}: | ||
| raise InvocationError("explain supports only --format json") |
There was a problem hiding this comment.
Fresh evidence beyond the earlier explicit-format case: this guard also sees defaults from SIDEMANTIC_FORMAT, cli.format, SIDEMANTIC_PLAIN, or cli.plain, so a project with cli.format: csv or cli.plain: true makes a bare sidemantic explain ... exit 2 even though the user did not explicitly request an unsupported output mode and explain has always emitted JSON. Please gate the rejection on format_explicit/plain_explicit, or ignore unsupported defaults for this JSON-only command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b51eb3de2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| envvar="SIDEMANTIC_CONFIG", | ||
| help="Path to config file (sidemantic.yaml)", | ||
| ), | ||
| output_format: str = typer.Option( |
There was a problem hiding this comment.
When a caller passes --format to a command that never calls resolve_output_format (for example sidemantic rewrite ... --format xml or server subcommands), this root option accepts the arbitrary string and the command silently ignores it, so an invalid/documented-unsupported format can still succeed with raw output or start a server. Since the CLI docs say --format accepts only table, csv, json, or jsonl, validate the root option centrally or reject explicit formats in commands that do not implement formatted output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c749200491
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| rewritten_sql = rewriter.rewrite(sql) | ||
| typer.echo(rewritten_sql) | ||
| if cli_state().requested_format is not None and output_format in {"csv", "json", "jsonl"}: |
There was a problem hiding this comment.
Fresh evidence beyond the earlier default-dry-run comment: requested_format is also populated from SIDEMANTIC_FORMAT and cli.format, so a project or shell default like SIDEMANTIC_FORMAT=csv makes a bare sidemantic query ... --dry-run emit a CSV/JSON record instead of the raw rewritten SQL. That still breaks compile-only scripts that do not pass --format and pipe dry-run output into a SQL client; this branch should use the explicit format source rather than any configured default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a017635ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output_format: str = typer.Option( | ||
| None, | ||
| "--format", | ||
| envvar="SIDEMANTIC_FORMAT", | ||
| metavar="table|csv|json|jsonl", |
There was a problem hiding this comment.
Fresh evidence beyond the prior invalid-format comment: the current central validation only rejects unknown values, so valid explicit requests still reach commands that never call resolve_output_format. For example, sidemantic rewrite ... --format json succeeds and prints raw SQL instead of JSON, and server subcommands accept --format json while starting normally, so scripts can receive non-JSON despite an explicit output flag; reject valid-but-unsupported formats for those commands or honor them.
Useful? React with 👍 / 👎.
| return json.dumps(records, indent=2, sort_keys=True, default=_json_default) | ||
| return "\n".join(json.dumps(record, sort_keys=True, default=_json_default) for record in records) |
There was a problem hiding this comment.
When a query result contains a non-finite float such as DuckDB SELECT 'NaN'::DOUBLE or an infinity from a floating metric, this keeps json.dumps's default allow_nan=True behavior and emits bare NaN/Infinity. Those tokens are not valid JSON/JSONL for strict parsers, which breaks the new machine-readable query --format json and jsonl contract; convert these values to null/strings or otherwise sanitize them before dumping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16cff095af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| error_type = InvocationError if usage else OperationalError | ||
| raise error_type(sanitize(exc)) from exc if isinstance(exc, Exception) else None |
There was a problem hiding this comment.
When a command catches a Typer/Click usage error and routes it through fail()—for example query catches _resolve_connection()'s BadParameter for --connection used together with --db—this default branch reclassifies it as an OperationalError, so the process exits 1 instead of the new documented exit 2 for invalid invocations. Treat click.ClickException/typer.BadParameter as invocation errors here or let them propagate so scripts can distinguish bad arguments from runtime failures.
Useful? React with 👍 / 👎.
0494a0f to
af00280
Compare
1 participant
Adds standard table, CSV, JSON, and JSON Lines output; global presentation and environment options; terminal-aware color and progress; improved help; and behavioral contract coverage. Updates CLI documentation and the bundled modeler skill for the new interfaces.