Skip to content

fix: require rooted ontology organize destinations - #260

Merged
seonghobae merged 9 commits into
mainfrom
fix/organize-rooted-target-current
Aug 25, 2026
Merged

fix: require rooted ontology organize destinations#260
seonghobae merged 9 commits into
mainfrom
fix/organize-rooted-target-current

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject relative, named-user tilde, and parent-traversal ontology destinations before emitting a MovePlan
  • preserve literal tildes inside absolute destinations and require an absolute supplied home for ~/ expansion
  • add ADR-0010, changelog traceability, and a Windows public-boundary regression contract

Validation

  • CARGO_BUILD_JOBS=2 cargo test --manifest-path src-tauri/Cargo.toml --lib organize::tests → 21 passed
  • CARGO_BUILD_JOBS=2 cargo test --manifest-path src-tauri/Cargo.toml --test organize_target_paths_windows → 0 tests on macOS (Windows-only cases compile at the public boundary)
  • rustfmt --edition 2021 --check src-tauri/tests/organize_target_paths_windows.rs
  • git diff --check

This is the current-main successor for stale draft PR #225, whose hosted test failure was an unrelated concurrent projection test on the old base. No force push or protected-branch bypass is used.


Open in Devin Review

Summary by CodeRabbit

  • 버그 수정

    • 파일 정리 대상 경로 검증을 강화했습니다.
    • 절대 경로와 정확한 ~/~/ 홈 경로만 허용하며, 상대 경로·사용자명 기반 틸드·상위 경로 탐색은 거부합니다.
    • Windows에서도 홈 경로가 올바른 절대 경로로 확장됩니다.
    • 절대 경로 내부의 리터럴 틸드는 그대로 보존됩니다.
  • 문서

    • 정리 대상 경로 정책과 보안 고려 사항을 문서화했습니다.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

Next included review available in 20 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 74341533-2fbc-40ef-920c-9916963d2eb8

📥 Commits

Reviewing files that changed from the base of the PR and between 69f88d2 and 3ecf1a1.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • docs/architecture/adr/0010-rooted-organize-destinations.md
  • docs/architecture/adr/README.md
  • src-tauri/src/organize.rs
📝 Walkthrough

Walkthrough

organize 대상 경로가 절대 경로 또는 정확한 홈 토큰인지 검증하도록 변경했습니다. 잘못된 경로는 이동 계획에서 제외합니다. Windows 회귀 테스트, CI 단계, ADR 및 변경 기록을 추가했습니다.

Changes

루트 기반 organize 대상 경로

Layer / File(s) Summary
대상 경로 해석과 검증
src-tauri/src/organize.rs, docs/architecture/adr/0010-rooted-organize-destinations.md, docs/architecture/adr/README.md, CHANGELOG.md
resolve_target_folder가 절대 경로와 정확한 ~ 또는 ~/... 경로만 허용합니다. 상대 경로, 부모 디렉터리 탐색, 명명된 사용자 틸드, 상대 홈 경로는 거부합니다. 관련 테스트와 ADR 및 변경 기록을 추가했습니다.
Windows 회귀 검증과 CI 연결
src-tauri/tests/organize_target_paths_windows.rs, .github/workflows/test.yml
Windows 테스트가 홈 상대 경로를 네이티브 절대 경로로 확장하고 상대 경로를 빈 계획으로 처리하는지 검증합니다. CI가 Rust 캐시와 해당 테스트 대상을 실행합니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 69f88

Absolute ontology destinations containing .. can still reach move planning instead of being rejected, leaving a concrete path-validation correctness gap that should be fixed before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. (4 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 온톨로지 organize 대상 경로에 루트 경로를 요구하는 주요 변경을 정확하고 간결하게 설명합니다.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. (4 skipped: 4 unsupported.)

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/organize-rooted-target-current

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head follow-up (after remote-safe fixture correction):

  • Exact head: 8412958da5bbc0a73a0c395a8b33ca3400d8de58
  • Base: 7eb131c4b4ac628226b262d6406a663fc7dc632f
  • The Windows integration fixture now passes the resolved target directly ({target}), avoiding accidental duplicate /{class} expansion; this preserves the public contract for ~/Media/{class} and relative-path rejection.
  • Local verification: cargo test --manifest-path src-tauri/Cargo.toml --test organize_target_paths_windows (macOS: 0 Windows-gated tests, compile succeeded), rustfmt --edition 2021 --check src-tauri/tests/organize_target_paths_windows.rs, git diff --check passed.
  • Hosted checks were freshly queued for this exact head; no failure is reported yet. Merge remains blocked pending required checks and independent approval.

coderabbitai[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review follow-up:

  • Exact head: c4dad65204956c853d7ca8a3098ac3ccb38c1688
  • Base: 7eb131c4b4ac628226b262d6406a663fc7dc632f
  • Addressed the remaining Devin finding at the shared resolver: absolute destinations containing Component::ParentDir are now rejected, with a regression test; the Windows fixture correction remains in the same branch.
  • Local verification: cargo test --manifest-path src-tauri/Cargo.toml --lib organize::tests → 22 passed; Windows-gated integration target compiled on macOS (0 runtime tests); git diff --check passed. The repository has pre-existing rustfmt differences outside this narrow change, so full-file rustfmt was not used as a claim.
  • Review threads are outdated for the new head; no current unresolved thread remains. Hosted checks were requeued for this exact head; merge remains blocked pending required checks and independent approval.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head follow-up:

  • Exact head: 1d3e393126d60e08941076005c96d9a0a6a0c69d
  • The first hosted Windows run exposed a real test-boundary compile failure: the external integration test imported private crate modules (E0603). The contract now runs inside organize::tests on Windows, so no internal module is made public; the obsolete integration file and failing command were removed.
  • The same fix adds native Windows ~\ home-token expansion while retaining fail-closed relative/traversal handling.
  • Local verification: cargo test --manifest-path src-tauri/Cargo.toml --lib organize::tests → 22 passed; git diff --check passed.
  • The Devin backslash-token note is addressed at this head; no current unresolved review thread remains. Hosted checks were requeued for this exact head.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review follow-up:

  • Exact head: 041cce66fc6eb737ced0bf2c31aca29004f5a970
  • Fixed the new Devin Windows bug: the workflow now filters cargo test --lib organize::tests::windows_, so Unix-path unit cases cannot execute on Windows; the three Windows-specific cases still compile and run there.
  • Local verification: the same filter compiles successfully on macOS (0 platform-gated tests) and git diff --check passed.
  • The two informational threads were verified as intentional/inert and resolved with rationale; the prior private-module compile failure and native separator issue remain fixed. Hosted checks were requeued for this exact head.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review follow-up:

  • Exact head remains 041cce66fc6eb737ced0bf2c31aca29004f5a970 (base 7eb131c4b4ac628226b262d6406a663fc7dc632f).
  • The latest informational CurDir note was verified against Rust Path::components() normalization; no semantic change is needed, and an incorrect rejection test was not retained. The thread is resolved with rationale.
  • Local cargo test --manifest-path src-tauri/Cargo.toml --lib organize::tests remains 22 passed; git diff --check passed. No current unresolved review thread remains; hosted checks are still queued/in progress.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Hosted-check update for exact head 041cce66fc6eb737ced0bf2c31aca29004f5a970: windows-home-resolution completed SUCCESS (job 97834797298), confirming the scoped Windows filter and native path tests pass. No failed terminal check is currently reported; the remaining release/security/review jobs are queued or in progress, and independent approval is still required.

@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 14:22
@seonghobae

Copy link
Copy Markdown
Contributor Author

Protected auto-merge is enabled for exact head 041cce66fc6eb737ced0bf2c31aca29004f5a970 using squash. No gate was bypassed: the PR remains OPEN/BLOCKED pending required hosted Checks and independent review approval; GitHub will merge only when those protections are satisfied.

@seonghobae
seonghobae marked this pull request as draft August 25, 2026 16:15
auto-merge was automatically disabled August 25, 2026 16:15

Pull request was converted to draft

@seonghobae
seonghobae marked this pull request as ready for review August 25, 2026 17:11
@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 17:11
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@seonghobae
seonghobae marked this pull request as draft August 25, 2026 17:29
auto-merge was automatically disabled August 25, 2026 17:29

Pull request was converted to draft

@seonghobae
seonghobae marked this pull request as ready for review August 25, 2026 19:03
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 19:03
devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae merged commit 198fea8 into main Aug 25, 2026
26 of 27 checks passed
@seonghobae
seonghobae deleted the fix/organize-rooted-target-current branch August 25, 2026 22:03
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.

1 participant