Repository navigation
fix: refuse relative and re-listed folders in Link checkout - #89
Merged
Merged
Conversation
Link checkout saved the typed folder as is and treated it as listed only when it was the same PathBuf. A relative folder then resolved against wherever the dashboard started, and a `..` or case-only spelling of a listed checkout added a second row. Refuse a folder that isn't a full path before anything is read, say when it doesn't exist, and compare the resolved folder with each full-path entry, so another spelling of a listed checkout writes nothing. The typed path is still what gets saved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Confidence: 8/10
Review done by claude-opus-5-5
The PR changes projects::link_checkout so it refuses a relative folder, reports a missing or unopenable folder, and compares the canonicalized folder with each absolute config.toml entry so another spelling of a listed checkout writes nothing. It also adds core tests and capture states. I found no evidenced Critical, High or Medium defect in the diff or its exact-SHA callers. The one Low finding concerns .. spellings saved as typed, and Low findings don't block approval.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 32s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 1 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 1 |
Key Findings
- ⚪ LOW — Accuracy: New `..` spellings are saved unnormalized, so on Unix they depend on the intermediate folder.
crates/dashboard-core/src/projects.rs:368: Trigger: on Linux or macOS, a user links a new checkout as/src/sub/../hs-buddy.add_repositorysaves that exact text, which the new testa_new_checkout_is_saved_as_typed_and_found_again_under_another_spellingasserts. POSIX resolvessub/..by walkingsub, so ifsubis later removed or renamed, the saved entry stops resolving and the row shows an identity error. The listed-entrycanonicalizeat line 361 then fails too, so linking the plain path adds a second row. This is inferred from path semantics and not observed at runtime. Issue #87's goal asks for 'one stable absolute path per checkout', but its acceptance criteria only forbid relative entries and the\\?\prefix, so the impact is limited to this edge case. On Windows, Win32 removes..lexically, so Windows is unaffected. (non-blocking)
Coverage
- Files reviewed:
crates/dashboard-core/src/projects.rscrates/dashboard-core/tests/projects.rscrates/dashboard-app/src/captures.rs
- Context read:
crates/dashboard-core/src/config.rscrates/dashboard-core/src/pmd.rscrates/dashboard-core/src/lib.rscrates/dashboard-app/src/manage/projects.rscrates/dashboard-app/src/projects_tests.rsREADME.mdAGENTS.mdissue #87PR #89 description
- Verified:
- Compare
base_commit.shaandmerge_base_commit.shaboth equalb6b54508e0c21ee5bc3c269ae7f7ec0e0197b94b; the last commit isc7d344790c6c96b223fe46f694055990c155ab64; the branch is 2 ahead and 0 behind is_absoluteruns before any filesystem read, so a relative folder returnsLinkError::Relativewithout loading or writing the configcanonicalizefailure maps toLinkError::Folder;Displaygives the not-found message forNotFoundand the OS error otherwise; the error text contains no sensitive values- The resolved path is used only for comparison;
add_repositorystill receives the typedcheckout, so no\\?\prefix reachesconfig.toml - The listed-entry comparison skips relative entries and entries that fail
canonicalize, then returns the listedPathBuf;add_repositorystill re-reads the file and keeps its exact-match guard - App caller
Manage::link_checkoutruns the core call on the background executor and only displays the returned path; pushing a duplicate intolinkedis harmless becauseis_linkedusesany - Capture helpers
open_dialog,set,save,settle,click,name,Folder::checkout(&str,&str)andconfig_direxist at the head SHA;dashboard_core::configis a public module - New core tests cover relative,
./, Windows\tickdownandC:tickdown, missing folder, exact path, trailing separator,.., Windows upper-case and Unix symlink spellings, and check the file stays byte-for-byte unchanged - README Link checkout text (line 220) does not contradict the new behavior, though it doesn't mention the full-path requirement
- Compare
- Not verified:
- Compilation, clippy, and CI results on Ubuntu and Windows for this head: execution forbidden, and CI status was not read
- Windows
canonicalizecase-folding and theC:tickdownand\tickdownhandling were reasoned from std semantics, not observed - Whether
open_dialogin the capture createsconfig.tomlbefore the capture'sadd_repositorycall; that helper body was not read in full - macOS case-insensitive APFS behavior of
canonicalizefor case-only spellings - Latency of
canonicalizeon unreachable network-share entries (the PR states it as a known risk)
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head c7d344790c6c96b223fe46f694055990c155ab64 • SFL v0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Risk level: 2/10 (project-dashboard)
Worked on by: claude-opus-5-5 on Laptop, for initiating Home70828f7a
Summary
Link checkout now checks the typed folder before it reads or writes anything:
std::fs::canonicalize) and compared with each full-path entry inconfig.toml. A..spelling, a case-only spelling on Windows, or a link to a listed checkout writes nothing and returns the listed path, so the table gets no second row.config.tomlnever gets a\\?\prefix.The change is in
projects::link_checkout(crates/dashboard-core/src/projects.rs).config.rsis unchanged, so #88 can follow without conflicts. No git process is started (tests/no_child_process.rsstill passes).Acceptance Criteria
config.toml:a_relative_or_missing_folder_is_not_linked_and_nothing_is_writtencoverstickdownand./tickdown, plus\tickdownandC:tickdownon Windows. The file is unchanged byte for byte. The test changes no working directory...or case-only (case-insensitive file system) spelling of a listed checkout writes nothing and adds no row:another_spelling_of_a_listed_checkout_adds_no_second_rowcoverssub/../tickdown, the upper-cased path (Windows) and a symlink (Unix). The file, with its comment, is unchanged andrepositorieskeeps one entry.linking_a_checkout_appends_it_and_keeps_every_commentandsingle_line_empty_and_missing_lists_are_edited_tooare unchanged and pass. The UI testlink_checkout_checks_the_origin_and_appends_to_config_tomlpasses too.\\?\prefix):a_new_checkout_is_saved_as_typed_and_found_again_under_another_spellingsavessub/../tickdownas typed, asserts there's no\\?\, and then finds it again from the plain path without writing.Definition of Done
dashboard-core, with behavior-named tests for the relative,.., case-only (Windows), exact and trailing-separator cases: three new tests incrates/dashboard-core/tests/projects.rs. Run against the oldlink_checkout, all three fail (3 failed); with the fix, 17/17 pass.c7d3447(see Validation).c7d3447in CI 42261815690. Local full gates remain unrun after the disk-full failure.c7d3447; one non-blocking Low note, zero unresolved threads.Validation
At head
c7d344790c6c96b223fe46f694055990c155ab64, on Mini (Linux), from agit archiveof that commit:cargo fmt --all --check: passcargo clippy --workspace --all-targets --locked -- -D warnings: passcargo test --workspace --locked: pass (dashboard-app 184 passed, 14 ignored; every dashboard-core suite passed, includingprojects17/17 andno_child_process)cargo deny --locked check advisories:advisories okOn Laptop (Windows 11, git 2.55),
cargo test -p dashboard-core --locked --test projects: 17 passed, including the case-only andC:tickdowncases. The only later edit to that file was a clippy fix (std::slice::from_ref) and an exact\\?\literal. Linux reran it at the final head.Screenshots come from the existing headless capture test, with synthetic temporary folders and fake pmd data. No window opens and no real profile is used:
CAPTURE_DIR=<abs dir> cargo test -p dashboard-app --locked captures -- --ignored --exact captures::captures(Linux headless renderer, 1100×760 at 200% device scale). The capture now opens Manage > Projects > hs-buddy, which has no checkout, and presses Link checkout three times:Relative folder
hs-buddy: refused, nothing written.Missing folder
/tmp/…/missing: says it doesn't exist./tmp/…/sub/../hs-buddy, typed after/tmp/…/hs-buddywas added toconfig.tomlbehind the open dialog: the success line names the listed path, not the typed one. The capture assertsconfig.tomlis byte-for-byte unchanged ("no second row").No recording: each state is the single frame after one button press, and the headless renderer writes still frames only.
Risks and deferred work
clippy/testrun, so it stopped before finishing. No source failure was reported. The focused Windows--test projectsrun passed before the last test-only edit (a clippy fix and an exact\\?\literal); the Windowsclippy --workspaceandtest --workspaceatc7d3447have not been run locally. Both Windows jobs in CI 42261815690 passed at the final head and supply that Windows evidence.Closes #87
🤖 Generated with Claude Code
Current-head delivery gate: all five required CI jobs passed on
c7d3447; SFL PASS names that exact head, all review threads are resolved, and effective rules require no formal approval.