Added umount command and unmount() API

[opencode-agent]

Implementation complete. Here's a summary of the changes:

Files modified:

• fs/src/fs/org/jnode/fs/service/FileSystemService.java - Added unmount() method declaration
• fs/src/fs/org/jnode/fs/service/def/FileSystemAPIImpl.java - Implemented unmount(): removes mount point, flushes if not read-only, unregisters from VirtualFS, closes filesystem, re-adds on failure
• fs/src/fs/org/jnode/fs/service/def/FileSystemPlugin.java - Added unmount() delegation with path normalization

File created:

• fs/src/commands/org/jnode/fs/command/UnmountCommand.java - Shell command with umount alias

Plugin descriptor updated:

• fs/descriptors/org.jnode.fs.command.xml - Added umount alias and syntax

The build succeeds (sh build.sh tests).

Closes #607

opencode session  |  github run

[LSantha]

/oc review

You are reviewing a pull request for the orchestrator. Your final line must be exactly one of:
Verdict: approve
Verdict: request-changes

Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next orchestrator phase.

[opencode-agent]

Here's my review of PR #611:

---

Review of PR #611: Added umount command and unmount() API

🟡 Unrelated change — fs/descriptors/org.jnode.fs.command.xml:65

The sync command's empty syntax description was changed from "Flush all mounted filesystems to disk" to "Flush all mounted filesystems". This is unrelated to the umount feature and removes useful detail. Either revert this or make it a separate commit.

🟡 Missing argument description — fs/descriptors/org.jnode.fs.command.xml:72

The umount syntax's <argument> lacks a description attribute, unlike every other command in this file (e.g., mount's arguments all have descriptions):

<argument argLabel="directory"/>  <!-- missing description -->

Should be:

<argument argLabel="directory" description="the mount point to unmount"/>

🟡 No unit test for unmount

FileSyncCommandTest exists for the sync command (testing mount point resolution). No corresponding test was added for the unmount path — at minimum, the rollback behavior (re-adds to mountPoints on IOException) and the IllegalArgumentException for non-mount-point paths should be tested. The existing SyncCommandTest provides a good template.

🟢 Partial rollback edge case — FileSystemAPIImpl.java:485-494

If fs.close() throws IOException after vfs.unregisterFileSystem() succeeds, the mount point is restored in mountPoints but the VirtualFS entry is already removed. This leaves an inconsistent state where getMountPoints() returns the entry but isMount() returns false. Low probability, but worth noting for a future improvement.

Self-check pass

• No Java 1.6 violations (no lambdas, diamond, try-with-resources)
• US-ASCII encoding
• LGPL 2.1 header present
• No code comments added (Javadoc is fine)
• Build succeeds

---

Verdict: request-changes

opencode session  |  github run

[LSantha]

/oc fix Address review feedback.

[opencode-agent]

All review feedback addressed:

1. Reverted unrelated change - Restored "to disk" in sync command description (fs/descriptors/org.jnode.fs.command.xml:65)

2. Added missing argument description - Added description="the mount point to unmount" to umount syntax (fs/descriptors/org.jnode.fs.command.xml:72)

3. Added unit tests - Created fs/src/test/org/jnode/fs/service/def/FileSystemAPIImplTest.java with 6 tests:

   • testUnmountNonExistentMountPointThrowsIllegalArgument - verifies IllegalArgumentException for non-mount-point paths
   • testUnmountWithoutLeadingSlash - verifies path normalization
   • testUnmountReadOnlyFsSkipsFlush - verifies close() is still called for read-only filesystems
   • testUnmountRollbackOnCloseFailure - verifies mount point is restored when close() throws IOException
   • testUnmountRemovesMountPoint - verifies successful unmount
   • testUnmountMultipleMountPoints - verifies sequential unmounts

4. Added test to build suite - Registered the new test in fs/build-tests.xml

opencode session  |  github run

[LSantha]

/oc review

You are reviewing a pull request for the orchestrator. Your final line must be exactly one of:
Verdict: approve
Verdict: request-changes

Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next orchestrator phase.

[opencode-agent]

Review of PR #611: umount command and unmount() API

Critical Issues

1. unmount() rollback is incomplete — FileSystemAPIImpl.java:485-494

If vfs.unregisterFileSystem() succeeds but fs.close() fails, the catch block only restores the mount point in mountPoints — but the VFS entry is already gone. This leaves an inconsistent state where getMountPoints() shows the mount but isMount() returns false. Fix: reorder to flush → close → unregister.

2. Test mock setup is broken — FileSystemAPIImplTest.java:53

mock(VirtualFS.class) returns null from getRootEntry(), causing mount() to throw FileNotFoundException. All mount-then-unmount tests will fail at runtime. Fix: use new VirtualFS(mock(Device.class)) instead.

Medium Issues

• Double flush — FileSystemAPIImpl.java:486-488 calls flush() explicitly, but AbstractFileSystem.close() already flushes internally. Redundant.
• No test for flush failure rollback — only close() failure is tested.
• No verification of vfs.unregisterFileSystem() in successful unmount tests.

Low

• Unused assertNotNull import at FileSystemAPIImplTest.java:34.

---

Verdict: request-changes

opencode session  |  github run

[LSantha]

/oc fix Address review feedback.

[opencode-agent]

All review feedback addressed:

1. Fixed unmount() rollback (FileSystemAPIImpl.java:476-493)

• Reordered to close → unregister (removed explicit flush since AbstractFileSystem.close() flushes internally)
• Now if close() fails, mount point is restored before VFS is touched, preventing inconsistent state

2. Fixed test mock setup (FileSystemAPIImplTest.java:50-56)

• Changed from mock(VirtualFS.class) to new VirtualFS(mock(Device.class))
• Added when(device.getShortDescription()).thenReturn("mock-device") stub
• Tests now properly mount/unmount via real VirtualFS

3. Removed double flush

• Eliminated redundant explicit flush call

4. Cleaned up imports

• Removed unused assertNotNull import

5. All 6 tests pass:

• testUnmountNonExistentMountPointThrowsIllegalArgument
• testUnmountWithoutLeadingSlash
• testUnmountReadOnlyFsClosesAndUnregisters (renamed from SkipsFlush)
• testUnmountRollbackOnCloseFailure
• testUnmountRemovesMountPoint
• testUnmountMultipleMountPoints

opencode session  |  github run

[LSantha]

/oc review

You are reviewing a pull request for the orchestrator. Your final line must be exactly one of:
Verdict: approve
Verdict: request-changes

Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next orchestrator phase.

[opencode-agent]

Review of PR #611: umount command and unmount() API

Summary

The PR implements an umount shell command and unmount() API for JNode. After the two previous fix rounds, the code is in good shape.

Findings

🟢 FileSystemAPIImpl.java:476-493 — unmount() implementation is correct:

• Path normalization (adds leading /) mirrors mount()
• mountPoints.remove() + null check → proper IllegalArgumentException
• Ordering is close → unregister — if close() fails, mount point is restored before VFS is touched, preventing inconsistent state
• No redundant explicit flush — AbstractFileSystem.close() already calls flush() for non-readOnly filesystems (verified at AbstractFileSystem.java:119-122)

🟢 FileSystemAPIImplTest.java — Test coverage is solid:

• 6 tests covering: non-existent path, path normalization, read-only FS, rollback on close failure, successful unmount, multiple unmounts
• Mock setup with new VirtualFS(mock(Device.class)) works correctly since VirtualFS is a concrete class with a Device constructor

🟢 UnmountCommand.java — Clean shell command. Validates mount point existence via fss.isMount() before calling unmount(), with proper error output.

🟢 org.jnode.fs.command.xml — Sync command description restored ("to disk" is back). umount syntax includes description attribute on the argument. Matches the pattern of other commands.

🟢 build-tests.xml — Test registered in the build suite.

🟢 Self-check:

• No Java 1.6 violations (no lambdas, diamond, try-with-resources)
• US-ASCII encoding
• LGPL 2.1 headers present
• No code comments added
• No debug output

No issues found. The implementation is correct and well-tested.

Verdict: approve

opencode session  |  github run

[LSantha]

Agent review passed. Awaiting human approval via native GitHub PR Review UI.