Repository navigation
Conversation
Capture subprocess output as bytes and decode it in the calling thread so invalid output returns a failed StepResult on Unix and Windows. Preserve locale decoding and universal newlines, and keep JSON output strict. Add real-process regressions for stdout/stderr decoding, valid Unicode, and workflow continue_on_error behavior. Update two subprocess mocks for binary capture. Assisted-by: OpenAI Codex (model: not exposed by the runtime, autonomous)
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The decoding-error handler discards the real exit code and already-decoded output needed by downstream workflow logic.
1 open issue
What changed in this PR
Makes undecodable shell output a structured step failure so workflow failure handling, including continue_on_error, can apply.
Changes:
- Capture output as bytes and decode locally, preserving locale encoding and newline normalization.
- Add real-process regressions for decoding failures, Unicode, JSON output, and workflow continuation.
- Update existing subprocess mocks to return bytes.
| File | Description |
|---|---|
| tests/workflows/test_shell_step.py | Adds decoding and workflow-continuation regression coverage. |
| tests/test_workflows.py | Aligns timeout-test mocks with binary capture. |
| src/specify_cli/workflows/step/shell/__init__.py | Decodes captured bytes and returns structured decoding failures. |
Review effort: Max
Comment on lines
+108
to
+113
| error = f"Shell command output could not be decoded: {exc}" | ||
| return StepResult( | ||
| status=StepStatus.FAILED, | ||
| error=error, | ||
| output={"exit_code": -1, "stdout": "", "stderr": str(exc)}, | ||
| ) |
Collaborator
|
Please address Copilot feedback |
mnriem
added
the
triage-must-have
This branch has not been deployed
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.
Description
A shell command can emit bytes that cannot be decoded with the locale encoding. Currently,
subprocess.run(..., text=True)lets that decoding error escape beforeShellStepreturns aStepResult. A workflow therefore crashes instead of recording a failed step, andcontinue_on_error: truecannot take effect.This change captures stdout/stderr as bytes and decodes them strictly in the calling thread, returning a failed step with a decoding diagnostic when necessary. Doing the decoding here also avoids relying on catching exceptions raised inside subprocess reader threads on Windows. The existing locale encoding, universal-newline behavior, and strict JSON parsing are preserved.
The regression tests use real child processes to cover invalid bytes on either stream, text/JSON output, valid Unicode and newline normalization, and both values of
continue_on_error. Two existing subprocess mocks now return bytes to match binary capture.Reproduction
In a UTF-8 environment, a shell step running the following command reproduces the problem:
python -c "import os; os.write(1, bytes([255]))"Place that step before another shell step and set
continue_on_error: true. Before this change, aUnicodeDecodeErrorescapes; afterward, the first step is recorded as failed and the next step runs. Withoutcontinue_on_error, the workflow ends with a structured failure.Testing
Automated validation on macOS, Python 3.13.9, pytest 8.4.2, against baseline
27d14e20c187c1f2916540ea201ecbdc9f8ed588:TestInitStep::test_failed_init_surfaces_inits_own_messagefails because this environment cannot resolve the external catalog host; the same failure was reproduced against the original source.--helpsmoke check passed.Commands, run from the worktree with
PYTHONPATH=src:.venv/bin/python -m pytest tests/workflows/test_shell_step.py tests/test_workflows.py::TestShellStep tests/test_workflows.py::TestContinueOnError -q .venv/bin/python -m pytest tests/test_workflows.py tests/workflows -q --deselect=tests/test_workflows.py::TestInitStep::test_failed_init_surfaces_inits_own_message .venv/bin/python -c 'from specify_cli import main; main()' --helpValidation limitations: network access prevented a fresh dependency installation, so these checks used available preinstalled dependencies in a worktree virtual environment. The full repository suite, supported dependency matrix, and native Windows execution were not run. This changes workflow shell execution; no slash-command templates or generated PowerShell code change.
AI Disclosure
AI disclosure: OpenAI Codex in ChatGPT Work Mode acted autonomously on behalf of @gavenren. The exact model identifiers and reasoning-effort settings were not exposed by the runtime and cannot be determined. AI performed investigation, implementation, regression-test generation and execution, independent agent review, and authored this PR description and the commit. No human line-by-line review or manual testing is claimed. The commit includes an
Assisted-by:trailer.