Repository navigation
docs: every --json run prints exactly one JSON object - #714
Conversation
Independent reviewNo blocking issues. The page matches shorebirdtech/shorebird#4055: there's a Should fix
Nits
|
|
lgtm |
AbhishekDoshi26
left a comment
There was a problem hiding this comment.
Checked against shorebird origin/main (#4055 merge commit f3dae09). No changes needed.
- Empty success envelope: the runner writes
{"status":"success","data":{},...}when a--jsonrun exits 0 or with no exit code and nothing else wrote an envelope (shorebird_cli_command_runner.dart). That coversinit,cache clean, and bareshorebird --json. --helpon stderr: bothShorebirdCommand.printUsageand the runner'sprintUsagego throughlogger.info, so this holds for subcommand help and top-level help.- Table: every row matches a command that calls
emitJsonSuccess.releaseandpatchalready do in v1.6.128, so moving them into the table is right.--versiongoes through the runner's ownJsonResult.success. precondition_failed:JsonErrorCode.preconditionFailedexists. Its doc comment lists "not logged in, not in a Shorebird project", which matches the table row. The caution's "Most of those" covers the ad-hoc checks that fall through toprocess_exit.- Earlier review: both points from the independent review are fixed. The caution is reworded and the
agent-instructions.txtsentence no longer repeats itself. - CI is green.
Merge timing: #4055 merged after v1.6.128 was tagged and isn't in any release yet (13 commits on main since v1.6.128). Merging this now would document the data: {} envelope before users can get it, so this should wait for the next CLI release, as the description says.
AbhishekDoshi26
left a comment
There was a problem hiding this comment.
Reviewed at 5c4f0fe.
Verdict: changes requested. One finding: on current shorebird main, two more commands return structured JSON. The table leaves them out, and the new paragraph wrongly says they print an empty data.
Checked
- Empty success envelope:
shorebird_cli_command_runner.darton shorebirdorigin/main(0049cec1) still writesJsonResult.success(data: const {}, ...)when a--jsonrun exits 0 or null and nothing else wrote an envelope. This still holds after #4097 and #4100.--help(null exit code) is covered by the same check.completionis still fast-tracked and writes no envelope; that was already accepted in the earlier review. - Existing table rows: each one matches an
emitJsonSuccesscall onorigin/main:account/{apps,orgs,whoami}_command.dartapps/{delete,rename,transfer}, plusapps listthroughAppsListCommand extends AccountAppsCommandchannels/{list,create,delete}patches/{list,info,set_track,patch_rollback_state}releases/{list,info}release_command.dart:202,490andpatch_command.dart:256,795doctor_command.dart:199andflutter_versions_list_command.dart:54--versionthrough the runner's ownJsonResult.success
- Other commands:
initandcache cleanhave noemitJsonSuccess, so they get the empty envelope, as the new paragraph says. - Error codes:
json_output.dartdefines exactlyprocess_exit,usage_error,software_error,fetch_failed,precondition_failedandinteractive_prompt_required. The table now lists all six. ThepreconditionFaileddoc comment says "not logged in, not in a Shorebird project, ...", which matches the new row.checkShorebirdInitializedinshorebird_validator.dartcovers theshorebird.yamlcase. - Caution wording: the runner maps
PreconditionFailedExittoprecondition_failedand a plain non-zero return toprocess_exit, so "Most of those" is accurate. - Release status: v1.6.128 is still the latest CLI tag, so the "land with the next CLI release" note in the description still applies.
- Agent entry points:
agent-instructions.txtis updated.- The
starlightLlmsTxtdetailsinastro.config.mjsandagent-skills/index.jsononly say "--json for machine-readable output". Neither contradicts the change, so they need no edit.
- Other pages:
account/cli.mdx,code-push/tracks.mdxandcode-push/rollback.mdxdon't contradict the change. - Merges and lint:
git merge-treeshows the branch merging cleanly with docsmain(beddc0b, which adds #727) and with #728's head. Taken together, the mergedscripting.mdxandagent-instructions.txtread consistently.- Prettier passes, cspell finds no issues in
scripting.mdx, and vale reports 0 errors, 0 warnings and 0 suggestions using main's.vale.ini. - No em-dashes in the added text.
- All internal links in the table resolve on main.
- Earlier reviews: both points from the first independent review are already addressed and are not raised again here.
Findings
-
src/content/docs/code-push/ci/scripting.mdx:54-67: the JSON command table is missingshorebird releases deleteand theshorebird metricssubcommands, so the new paragraph below it gives the wrong shape for them.Problem. The paragraph says every other command "prints a success object whose
datais empty ({})". On shorebirdmain, two commands that ship in the same next CLI release return real data:releases delete:releases_delete_command.dart:162emitsapp_id,release_id,release_version,release_deleted,platformsanddeleted.- Every
metricssubcommand:metrics_subcommand.dart:180emitsemitJsonSuccess(toJson(response)).
A script that follows this page would expect
{}from both. The 2026-10-16 heads-up from #728's review is correct per source.Evidence.
releases_command.dart:11registersReleasesDeleteCommand.shorebird_cli_command_runner.dart:100registersMetricsCommand.metrics_command.dartadds eight subcommands:active-hours,adoption,downloads,heatmap,installs,new-devices,usersandversions.- All eight inherit
run()fromMetricsSubcommand.downloadsandinstallsdo this through the abstractMetricsPatchCountCommand, which has nonameand is not registered. - Neither
releases deletenormetricsexists at the v1.6.128 tag, so both are next-release features, like the rest of this PR.
Fix. Replace the table with the version below (already run through Prettier; the column widths are unchanged).
The
metricsrow has no link for now. The metrics section (/account/cli#metrics-commands) is added by #728. If #728 lands first, link the group cell as[`shorebird metrics`](/account/cli#metrics-commands)and re-run Prettier.| Group | Commands | | ---------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | | [`shorebird account`](/account/cli#account-management-commands) | `whoami`, `apps`, `orgs` | | [`shorebird apps`](/account/cli#app-management-commands) | `list`, `rename`, `delete`, `transfer` | | [`shorebird channels`](/code-push/tracks#managing-tracks-from-the-cli) | `list`, `create`, `delete` | | [`shorebird patches`](/code-push/patch#manage-patches) | `list`, `info`, `set-track`, `rollback`, `rollforward` | | [`shorebird releases`](/code-push/release) | `list`, `info`, `delete` | | `shorebird metrics` | `active-hours`, `adoption`, `downloads`, `heatmap`, `installs`, `new-devices`, `users`, `versions` | | Other | `shorebird release`, `shorebird patch`, `shorebird doctor`, `shorebird flutter versions list`, `shorebird --version` |
Not affected. eseidel's other note, to keep #728's
--yeswording inagent-instructions.txt, needs no change in this PR. This PR does not touch the delete-confirmation line, and the two PRs merge cleanly.
AbhishekDoshi26
left a comment
There was a problem hiding this comment.
Reviewed at a5f2154.
Verdict: no findings, nits included. The finding from the last round is fixed. releases delete and the metrics subcommands are now in the JSON command table.
Checked
- The table matches the source. On shorebird
origin/main(0049cec1), every command that callsemitJsonSuccesshas a row, and every row has one:account/{apps,orgs,whoami},apps/{delete,rename,transfer}(apps listthroughAccountAppsCommand),channels/{list,create,delete},patches/{list,info,set_track,patch_rollback_state}releases/{list,info,delete}(releases_delete_command.dart:162)metrics_subcommand.dart:180release_command.dart:202,490,patch_command.dart:256,795,doctor_command.dart:199,flutter_versions_list_command.dart:54--versionthrough the runner's ownJsonResult.success
- Metrics row.
metrics_command.dart:11-18registers exactlyactive-hours,adoption,downloads,heatmap,installs,new-devices,usersandversions, and all of them emit the API response throughMetricsSubcommand.run(). The row is unlinked until #728, which adds the docs section, merges. - Empty success object.
shorebird_cli_command_runner.dart:390-393writesJsonResult.success(data: const {})when a JSON-mode run has written no envelope and exits with null or 0. That coversinit,cache cleanand--help. - Logs go to stderr. In JSON mode the runner sends human and subprocess output to stderr and binds the JSON sink to the real stdout.
- Error codes.
JsonErrorCodeinjson_output.dartdefines exactly the six codes the table lists, and thepreconditionFaileddoc comment matches the new row. - Exceptions.
shorebird apiis the only command that reserves stdout (api_command.dart:89); #726 documents it. - Agent entry points.
agent-instructions.txtmatches the page. ThestarlightLlmsTxtdetailsandagent-skills/index.jsondon't contradict it. - Consistency with main. The branch merges cleanly with docs main. The changelog entries
json-precondition-failed.mdandrelease-patch-json.mdagree with the page. - Lint. Prettier, cspell and vale are clean on the changed text, there are no em-dashes, and the table links resolve.
- Release timing. v1.6.128 is still the latest CLI release and does not contain #4055,
releases deleteormetrics, so this lands with the next CLI release.
Findings
None.
Updates the scripting docs for shorebirdtech/shorebird#4055, under which every
--jsonrun prints exactly one JSON envelope on stdout.scripting.mdx: commands without structured output now print{"status": "success", "data": {}, ...}on success, with their logs (and--helpusage) on stderr. That replaces "still print human-readable logs, check the exit code".shorebird releaseandshorebird patchmove into the JSON commands table, since they already emit JSON. The caution no longer says some failures print plain text. Pre-run check failures now reportprecondition_failed, which is added to the error-code table.agent-instructions.txt: the same change to the one-line summary, noting thatshorebird apiis the one command without--json.shorebirdtech/shorebird#4055 shipped in Shorebird 1.6.129, so this can land. On 1.6.128 and earlier, commands without JSON output still leave stdout empty on success, and the page says the empty success object needs 1.6.129 or newer.