Granting all-files access only moves the mirror root at the next cold start - #132
Merged
Conversation
computeStorageTarget() adopted a new default root only when the current one was missing, but ensureExternalStorage() has already created it, so once projectPath was set the fallback branch was dead for the rest of the process. Granting all-files access left the running session writing to the old private root, and the switch surfaced only at the next cold start, where the earlier mirrors no longer sat under the listed root. The choice moves to StoragePaths.resolveRoot, which is blind to the root in use by construction. The same function also erased a persisted BasePath as soon as it could not be vetted; it is now kept and merely unused for that run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Review of the previous commit found two consequences of recomputing on every resume. initRootPath deliberately leaks a buffer per call, on the assumption that re-init is rare, and the missing-directory warning is a toast that would repeat for as long as the base stayed unusable. Both now run only when the resolved root actually changes. resolveRoot drops its baseIsDirectory parameter and derives the fact itself: a boxed tri-state Boolean sitting next to a primitive boolean transposes silently, and the caller was computing something the helper already had. The tests move from asserting on source text to driving the helper against a real filesystem, which covers the cases a text match could not tell apart. Verified by mutation: hardcoding the verdict, erasing under the literal key, ungating the native re-init, and dropping the directory check each fail exactly one test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The decision that re-points the root on a resume was only asserted as source text, and the re-init assertion still passed with initRootPath moved just outside the guarded block. rootMoved() and defaultRoot() move into StoragePaths, so plain JUnit drives both the gate and the public-versus-private path policy. What is still read from the source is now brace-scoped, and the erasure check asks that the settings are never edited rather than listing two spellings of remove(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The gate test matched the literal head `if (StoragePaths.rootMoved(`, so a mutant spelled `rootMoved(projectPath, projectPath)` kept the text intact, never fired, and left the native log root permanently wrong. Inverting the condition only died by accident, and lifting the body into a helper failed a test that had nothing to say about behaviour. The block becomes StoragePaths.applyRootMove, taking the two roots and a RootMoveActions for the warning, the native re-init and the suggestion refresh, so the test records which calls happened with which arguments. While here: defaultRoot took `shared` first where its neighbour isWritable takes it last, four same-typed File params one line apart. Both now read the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
… site The recorder tests judge applyRootMove itself, so deleting its one caller leaves them green while the native log root goes back to being wrong. A text check cannot prove ordering or nesting, which is why the old block matcher went; it can prove the call is spelled somewhere, which is all this asks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Comments and Javadoc only, no behaviour change. The note on getDefaultHTTrackPath had grown into a paragraph about private-directory side effects, which is not what the line below it does. applyRootMove documented two of its four parameters where the rest of the file documents all of them, and the interface method it takes was the one parameter in StoragePaths without final. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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
1 participant
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.
computeStorageTarget()adopted a new default root only when the current one was missing. ButensureExternalStorage()has already created it, so onceprojectPathwas set that branch was dead for the rest of the process: granting all-files access left the running session writing to the old private root, and the move showed up at the next cold start, by which point the earlier mirrors sat outside the root being listed. The decision now sits inStoragePaths.resolveRoot, which takes no argument for the root in use and so cannot prefer it; that also fixes the legacy API<=29 grant callback, which recomputed through the same dead branch.The same function erased a persisted
BasePathas soon as it could not be vetted, so an unmounted volume or a revoked grant lost the setting for good. It is now kept and simply unused for that run. Every resume re-resolves, and the two things that must not repeat are gated onStoragePaths.rootMoved:initRootPathleaks a buffer per call by design, and the missing-directory warning is a toast.The public-versus-private path choice moved to
StoragePaths.defaultRoottoo, so plain JUnit now drives what used to be asserted against the source text. A move's side effects run throughStoragePaths.applyRootMove, and the tests record the calls rather than search for them: granting access moves the root, the first resolution counts as a move, and an unchanged root does not, however many resumes happen.One surface stays uncovered, deliberately. The stub
android.jarputscomputeStorageTargetbeyond a unit test's reach, so nothing asserts what the anonymousRootMoveActionsat that call site does; a source-text check only confirms the call is there at all. Swapping the bodies ofwarnMissingDirectoryandrefreshProjectSuggestionsleaves the whole suite green, the miswiring plain in the bytecode: a user whose base directory is missing would get no warning, and a toast for nothing once the root moved. HardcodingmissingDirto false, or inverting theloadedSuccessfully()guard, survive the same way. Catching any of it needs assertions on the source text, and the first rename breaks those.Closes #128