fix(workflows): route 'workflow status --json' errors to stderr#3520
Open
jawwad-ali wants to merge 2 commits into
Open
fix(workflows): route 'workflow status --json' errors to stderr#3520jawwad-ali wants to merge 2 commits into
jawwad-ali wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Routes workflow status JSON-mode lookup errors to stderr, preserving clean stdout for machine-readable output.
Changes:
- Uses
_error_console(json_output)for status lookup errors. - Adds a regression test for missing runs.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/_commands.py |
Routes status errors appropriately. |
tests/test_workflows.py |
Tests stderr routing for missing runs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
Collaborator
|
Please address Copilot feedback |
The workflow_status run_id error paths (FileNotFoundError -> 'Run not found', ValueError -> invalid run) used the stdout console and fired before the json_output branch, so 'specify workflow status <bad-id> --json' wrote a Rich-rendered error to stdout and corrupted the JSON stream a consumer would json.loads(). Route both through _error_console(json_output) so they go to stderr under --json, matching the sibling 'workflow run'/'workflow resume' commands (which use the identical RunState.load try/except) and the documented stdout-purity contract. Test asserts the not-found error appears on stderr and stdout stays empty under --json (fails before: the error was on stdout). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on purity The stderr-routing fix reroutes both the FileNotFoundError and ValueError run_id handlers, but the test only exercised FileNotFoundError — a regression of the ValueError path back to stdout would have gone uncaught. Add a ValueError case (RunState.load raising) asserting the same stderr-only / empty-stdout behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
77743e8 to
de544bc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
specify workflow status <run_id> --jsonprinted its lookup errors to stdout, corrupting the machine-readable JSON stream.In
workflow_status, the tworun_iderror handlers used the stdoutconsoleand fire before theif json_output:branch:So a consumer doing
json.loads(stdout)onspecify workflow status bad-id --jsongets a Rich-rendered error line instead of parseable JSON.Fix
Route both handlers through the existing
_error_console(json_output)helper so they go to stderr under--json. This mirrors the sibling commands in the same file —workflow run(err.print(...), line ~1083) andworkflow resume, which does the identicalRunState.loadtry/except with the byte-identicalRun not found: {run_id}message routed to stderr (line ~1188) — and the documented stdout-purity contract on_emit_workflow_json/_error_console. One file, minimal change; text output path is unchanged.Test
test_status_json_not_found_error_goes_to_stderrasserts that under--jsonthe not-found error appears on stderr and stdout stays empty (no partial JSON). Fails before the fix (the error was on stdout), passes after. Existingstatusboundary tests unchanged.🤖 This PR was written with the assistance of Claude Code (AI). The bug was self-found and the fix/tests were verified locally (fail-before / pass-after).