fix(shell): report correct illegal option character in mv command#26750
fix(shell): report correct illegal option character in mv command#26750
Conversation
The mv command was reporting "-" for all unrecognized flags instead of the actual flag character. For example, `mv -T` would show "mv: illegal option -- -" instead of "mv: illegal option -- T". The bug was in parseFlag() where the else branch returned a hardcoded "-" instead of the actual unrecognized character from the flag string. Fixes #26749 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Updated 12:17 AM PT - Feb 5th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26750That installs a local version of the PR into your bun-26750 --bun |
WalkthroughFlag parsing in the mv builtin was changed to track the index of each short flag and return the specific invalid option character on error; regression tests were added to verify the precise "mv: illegal option -- " messages for several invalid-flag scenarios. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26749.test.ts`:
- Around line 12-15: Add an explicit assertion for the process exit code in the
catch block: after decoding stderr and the existing
expect(stderr).toContain(...) add an assertion on the captured ShellError (const
err = e as Bun.$.ShellError) such as expect(err.code).toBe(<expectedExitCode>)
or at minimum expect(typeof err.code).toBe("number") &&
expect(err.code).not.toBe(0); apply the same pattern to the other related test
cases to make exit-code verification explicit.
Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Address code review feedback to verify exit codes in catch blocks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Addressed the review feedback - added |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26749.test.ts`:
- Around line 7-65: Replace the four nearly identical tests with a single
parameterized test using test.each: create an array of cases like [["-T","T"],
["-X","X"], ["-fX","X"], ["-vZ","Z"]] and write test.each(cases)("mv reports
correct illegal option for %s", async (flag, expectedChar) => { $.throws(true);
try { await Bun.$`mv ${flag} ./a ./b`; expect.unreachable("should have thrown");
} catch (e: unknown) { const err = e as Bun.$.ShellError; const stderr = new
TextDecoder().decode(err.stderr); expect(stderr).toContain(`mv: illegal option
-- ${expectedChar}`); expect(err.exitCode).not.toBe(0); } finally { $.nothrow();
} }); ensure you reference the existing Bun.$ invocation and the
$.throws/$.nothrow usage so behavior stays identical.
Address code review feedback to reduce duplication by using parameterized tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Refactored the tests to use |
Summary
mvcommand reporting incorrect character in illegal option error messagesBefore:
mv -T ./a ./bshowsmv: illegal option -- -After:
mv -T ./a ./bshowsmv: illegal option -- TTest plan
test/regression/issue/26749.test.ts-T,-X,-fX, and-vZflagsFixes #26749
🤖 Generated with Claude Code