Skip to content

fix(mv): accept -f/--force - #136

Merged
mkmeral merged 1 commit into
strands-agents:mainfrom
ferdingler:fix/mv-accept-force
Oct 6, 2026
Merged

mkmeral merged 1 commit into
strands-agents:mainfrom
ferdingler:fix/mv-accept-force

Conversation

@ferdingler

Copy link
Copy Markdown
Contributor

Description

The mv command rejected -f/--force as an invalid option. mv already overwrites an existing destination without prompting, which is what -f means, so the flag is now accepted and changes nothing. The Strands harness web_fetch tool (Python and TypeScript) ends its command with mv -f OUT.part OUT, and agents write mv -f routinely.

mv --help now lists the option.

Related Issues

Closes #134

Documentation PR

N/A

Type of Change

Bug fix

Testing

Added mv_force_overwrites and mv_long_force_overwrites integration tests, which check that mv -f and mv --force overwrite an existing destination. Also ran mv -f and mv --help in the strands-shell binary.

  • I ran the relevant test suites for the bindings I touched (cargo test --workspace --all-targets, pytest tests/python, npm test)
  • If I touched Rust, I ran cargo fmt and cargo clippy

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

mv already overwrites an existing destination without prompting, so
-f/--force is accepted as a no-op. The Strands harness web_fetch tool
ends its command with `mv -f OUT.part OUT`, and agents write `mv -f`
routinely.

Closes strands-agents#134
@ferdingler
ferdingler requested a review from a team as a code owner October 6, 2026 00:00
@ferdingler
ferdingler requested a review from pgrayy October 6, 2026 00:00
@mkmeral

mkmeral commented Oct 6, 2026

Copy link
Copy Markdown
Member

@strandly-the-agent review pls

@strandly-the-agent strandly-the-agent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Correct and minimal: -f/--force becomes a no-op and overwrite really is already the default on both filesystem paths, so the new HELP text is honest. One non-blocking docs follow-up in harness-sdk.

🟡 Docs drift (other repo, non-blocking): strands-agents/harness-sdk site/src/content/docs/user-guide/shell/commands.mdx:53 says "mv | … no -f, -i, -n, or -v." — stale once this merges. PR says Documentation PR: N/A; a one-line edit there (drop -f) would close the loop.

✅ What was verified (head 5926965)
  • Overwrite is the default on both paths: src/vfs.rs:466-485 removes an existing destination entry before re-inserting (// If destination exists, remove it first); src/vfs_kernel.rs:673 routes host↔host renames to std::fs::rename, which replaces on Unix. Exercised the host bind path manually with mv -f OUT.part OUT — exit 0, OUT contains new, OUT.part gone (the exact web_fetch pattern from #134).
  • Tests prove the fix: tests/shell_integration.rs:1815-1824 create the destination with old first and use ;, so a failing mv surfaces as old. cargo test --test shell_integration mv_ → 4 passed on this branch; the two new tests fail on origin/main (left: "old", right: "new").
  • CI Gate: success on 5926965.
  • -i / -n still error (mv: invalid option '-i') — the right call; silently accepting those would clobber data.
  • HELP style matches precedent: src/commands/grep.rs:18 documents a no-op flag with (default) the same way; Usage: mv [-f] … mirrors rm's [-rf].
Appendix — non-blocking (2)
  • ⚪ No mv_help test, though the PR body claims --help lists the flag and rm_help exists (tests/shell_integration.rs:5701). Optional — only ~11/34 commands have one. If you want it: expect!(mv_help, "mv --help | head -n 1", "Usage: mv [-f] SOURCE... DEST");
  • ⚪ The "I have reviewed and understand every line of code" checklist box is the one left unticked.

Reviewed by strandly-the-agent — an AI agent; please treat as input for a human reviewer.

@mkmeral
mkmeral merged commit e2b4c02 into strands-agents:main Oct 6, 2026
30 checks passed
@ferdingler
ferdingler deleted the fix/mv-accept-force branch October 6, 2026 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mv: accept -f/--force

3 participants