fix: fail closed on cwd-relative organize targets - #225
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes대상 폴더 경로 검증
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change makes relative organize targets fail closed while preserving supported rooted targets; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
opencode-agent
Bot
added
priority: medium
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn resolve_target_folder(template: &str, home: &Path, local: &str) -> Option<PathBuf> { | ||
| let resolved_template = template.replace("{class}", local); | ||
| if resolved_template == "~" { | ||
| return home.is_absolute().then(|| home.to_path_buf()); | ||
| } | ||
| if let Some(relative) = resolved_template.strip_prefix("~/") { | ||
| if !home.is_absolute() { | ||
| return None; | ||
| } | ||
| let mut folder_path = home.to_path_buf(); | ||
| for component in Path::new(relative).components() { | ||
| match component { | ||
| Component::Normal(segment) => folder_path.push(segment), | ||
| _ => return None, | ||
| } | ||
| } | ||
| return Some(folder_path); | ||
| } | ||
|
|
||
| let folder_path = PathBuf::from(resolved_template); | ||
| folder_path.is_absolute().then_some(folder_path) | ||
| } |
There was a problem hiding this comment.
📝 Info: Old tilde substitution replaced first tilde anywhere
The prior template.replacen('~', ...) expanded the first tilde anywhere in the template, so an absolute target like /opt/~archive/{class} was corrupted. The new resolve_target_folder only expands an exact ~ or leading ~/ and preserves literal tildes in absolute targets, fixing this.
Was this helpful? React with 👍 or 👎 to provide feedback.
| windows-organize-paths: | ||
| runs-on: windows-latest | ||
| steps: | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # stable | ||
| - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2 | ||
| with: | ||
| workspaces: src-tauri | ||
| - name: Windows organize absolute-path regressions | ||
| run: cargo test --manifest-path src-tauri/Cargo.toml --lib organize::tests |
There was a problem hiding this comment.
🔍 Windows CI job installs no system deps
The new windows-organize-paths job runs cargo test --lib organize::tests on windows-latest with no system-dep install step, unlike the ubuntu job. Building the src-tauri lib pulls in tauri build dependencies; confirm it compiles on a bare windows-latest runner or the job fails before the organize tests run.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Current-head update: pushed merge commit
Hosted checks are now invalidated/restarted for exact head |
|
Exact-head update: 06d1d2e (style-only rustfmt for touched src-tauri/src/organize.rs after main merge). Local evidence at this head: rustfmt --check and git diff --check passed; focused organize tests 21 passed; Windows home-resolution contract 1 passed; actionlint passed. Hosted checks are queued/in progress with no failures observed. PR remains draft and protected review gates are still required; no merge claimed. |
|
Exact-head update: 3715a5a (test: use platform absolute homes in organize regressions). Hosted Windows failed only because two organize tests supplied POSIX |
|
Superseded by current-main successor PR #260 ( |
1 participant
Purpose
Prevent user-editable ontology
dm:targetFoldervalues from producing process-working-directory-relative move plans. DiskSage preview/execute destinations must be rooted and stable rather than depending on whichever CWD launched the desktop process.Exact current state
9580804a9c1df6d2ba01d564fb1f1aa684636c6e.main:be418bac9bfbdc8aa41b1a698713833e3dbccfdc.src-tauri/src/organize.rsplus the dedicated Windows organize-path test job in.github/workflows/test.yml.31942644718, Security Scan31942644683, and SAST Semgrep31942644737completedsuccess.31942644656also completedsuccess, including the dedicated Windows organize-path job, but this older Test workflow still used the predecessor Rust-cache contract that may restoresrc-tauri/target. Canonical exact-evidence owner ci: require exact-head production coverage evidence #156 has since proven stale compiled-target contamination and repaired it withcache-targets: false; therefore this earlier Test success is not treated as final trustworthy exact-head evidence for integration.resolve_homefallback and provided Windows absolute-home evidence; this branch independently provided Windows organize-path evidence.Test-first repair
The original RED proved
dm:targetFolder "relative/{class}"could produce a CWD-relative move destination. The current implementation addsresolve_target_folderand fails closed unless an ontology target is either:~/~/...home-relative target whose supplied home is itself absolute and whose suffix contains only normal path components.The branch also rejects parent traversal in home-relative targets and named-user-like
~other/...values, preserves literal tildes inside absolute targets, uses platform-appropriate absolute fixtures, and keeps existing{class}substitution behavior. No filesystem mutation authority was added or broadened.Ownership / dependency order
commands.rsabsolute-home resolution contract and has already repaired its relative fallback.cache-targets: falsestale-target repair. Do not duplicate that workflow repair here; after ci: require exact-head production coverage evidence #156's evidence contract reaches the integration lineage, reacquire this PR's exact Test evidence on the then-current head/base.Required before merge
Keep Draft until the unchanged integration head has trustworthy current Test evidence under the non-stale target-cache contract, Release/Security/SAST and every applicable central workflow pass, every valid current-head finding/thread is resolved, fresh protected-main ancestry remains current, repository-wide exact 100% owned-production region/statement-equivalent, branch, function, and line coverage is satisfied without exclusions or threshold weakening, and live independent-review governance is satisfied. Pending, queued, skipped-required, neutral-required, failed, stale, predecessor, synthetic, diagnostic-only, status-only, model-only, author-only, cached-target, or infrastructure-only evidence is non-passing.