Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate review comments remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates argbash to fail with status 1 when it cannot write the requested output file.
Changes:
- Adds output-write failure handling.
- Adds unwritable-output regression coverage.
- Updates generated artifacts and
ChangeLog.
File summaries
| File | Summary |
|---|---|
tests/regressiontests/Makefile |
Registers the regression test. |
tests/regressiontests/make/tests/tests-base.m4 |
Adds the test; moderate comment requests asserting status 1 (1 vote). |
src/argbash.m4 |
Adds write-failure handling; critical comment requests propagating recursive generation failures (1 vote). |
ChangeLog |
Documents the bug fix. |
bin/argbash |
Updates the generated executable. |
Review details
Suppressed comments (1)
tests/regressiontests/make/tests/tests-base.m4:349
- This regression only uses
reverse, which considers any non-zero status a pass. Since the PR contract is specifically exit status 1, a regression returning 2 would still pass; assert the captured command status equals 1 in addition to matching the diagnostic.
ERROR="write the output" $(REVERSE) $(ARGBASH_EXEC) $(TESTDIR)/test-simple.m4 -o $(TESTDIR)/nonexistent-dir/out.sh
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if test "$outfname" != '-' | ||
| then | ||
| printf "%s\\n" "$output" > "$outfname" | ||
| printf "%s\\n" "$output" > "$outfname" || die "Couldn't write the output to '$outfname'." 1 |
Author
There was a problem hiding this comment.
Addressed: the recursive generation of the parsing code file is now checked and argbash dies with a message if it fails. The unwritable-output test also asserts the exit status 1 now.
A failed redirection to the output file only produced a shell diagnostic, argbash still exited with status 0, so callers such as make carried on with a stale or missing output. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2
gdevenyi
force-pushed
the
fix/report-unwritable-output
branch
from
September 13, 2026 02:21
8ef94d5 to
1dff421
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.
A failed redirection to the output file (for example a non-existent directory, or a file owned by another user) only produced a shell diagnostic -
argbashstill exited with status 0, so callers such asmakecarried on with a stale or missing output.argbashnow dies with a clear message and exit status 1.New regression test
test-unwritable-output.🤖 Generated with Claude Code
https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2