Fix mkdir dedupeName for existing directories#3215
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes FSService.mkdir so dedupeName is honored when the target path already exists as a directory (creating a suffixed directory like hello (1)), while keeping the existing idempotent behavior when dedupeName is false. It also adds regression tests for both modern and legacy FS controllers to ensure the hello → hello (1) behavior.
Changes:
- Update
FSService.mkdirto dedupe directory names when the target directory already exists anddedupeNameis enabled. - Preserve idempotent mkdir behavior when
dedupeNameis disabled and the target directory already exists. - Add modern + legacy controller regression tests covering the dedupe behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/backend/services/fs/FSService.ts |
Adjusts mkdir conflict handling so existing directories trigger dedupe when requested. |
src/backend/controllers/fs/LegacyFSController.test.ts |
Adds regression coverage for legacy mkdir dedupe behavior (hello → hello (1)). |
src/backend/controllers/fs/FSController.test.ts |
Adds regression coverage for modern mkdir dedupe behavior (hello → hello (1)). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (existing) { | ||
| if (existing.isDir) { | ||
| // A directory already exists at path: idempotent success. | ||
| return existing; | ||
| } | ||
| if (input.overwrite) { | ||
| if (input.dedupeName) { | ||
| name = await this.#findDedupedName(parent, name); | ||
| } else { | ||
| // A directory already exists at path: idempotent success. | ||
| return existing; | ||
| } |
There was a problem hiding this comment.
i see the check in the controller, copilot just dumb at bricks, lgtm
xsourabhsharma
force-pushed
the
fix-mkdir-dedupename-directory
branch
from
45d65bc to
9ee93fe
Compare
Salazareo
left a comment
Salazareo
left a comment
There was a problem hiding this comment.
looks good to me, but just gonna get copilot to scan in case I missed anything
|
@Salazareo Thanks for checking. I also verified this locally on the PR branch: the FSController/LegacyFSController mkdir suites pass, ClaudeProvider.test.ts passes, and npm run build:ts passes. The red CI job looks unrelated to this mkdir change. The GitHub log shows the backend tests passed, then Vitest reported an unhandled teardown rejection from src/backend/drivers/ai-chat/providers/claude/ClaudeProvider.test.ts. I think rerunning the failed backend job is the right next step. |
|
merged @xsourabhsharma thanks for the contribution! |
Summary
mkdirsodedupeNamecreates a suffixed directory when the target directory already exists.mkdirbehavior whendedupeNameis false.hello->hello (1)and the parent-write ACL guard.Root cause
FSService.mkdirreturned an existing directory before consideringdedupeName, so duplicate directory creation never reached the dedupe path.Review follow-up
Copilot flagged that deduping an existing directory creates a sibling path, so target-path authorization alone could be too weak for app-owned AppData roots. The controllers now require parent write access when
dedupeNamecollides with an existing target before callingFSService.mkdir.Tests
npm run test:backend -- src/backend/controllers/fs/FSController.test.ts src/backend/controllers/fs/LegacyFSController.test.tsnpm run build:tsCloses #3201