fix(variant): never adopt an identity reached by a truncated path - #274
Open
tobymurray wants to merge 3 commits into
Open
fix(variant): never adopt an identity reached by a truncated path#274tobymurray wants to merge 3 commits into
tobymurray wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds directory support to the in-memory host filesystem, documents and tests its enumeration semantics, and strengthens VariantConfig candidate-path handling with discovery tests. ChangesFilesystem and variant testing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Tests/Host/support/KernelTestDoubles.cpp (3)
498-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dir(nullptr)returns a root handle whilefile(nullptr)returns null.
file()returns an emptyunique_ptrfor a null path.dir()maps a null path to"", which names the root, so the caller receives a usable handle to the root directory. Align the two, or document the difference.♻️ Proposed change
std::unique_ptr<SDK::Interface::IDirectory> InMemoryFileSystem::dir(const char* path) { - return std::make_unique<InMemoryDirectory>(*this, path != nullptr ? path : ""); + if (path == nullptr) { + return {}; // matches file(): a null path yields no handle + } + return std::make_unique<InMemoryDirectory>(*this, path); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Host/support/KernelTestDoubles.cpp` around lines 498 - 501, Update InMemoryFileSystem::dir to return an empty unique_ptr when path is nullptr, matching file() semantics; retain InMemoryDirectory construction for non-null paths.
475-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mkdir()accepts a path that already names a file.
mkdir()never checksfiles. If a test seedsa.txtand then callsmkdir("a.txt"), the same name is recorded as a file and as a directory.snapshot()then emits two entries with the same name, one withisDirfalse and one withisDirtrue. A real backend rejects thatmkdir. A guard keeps the fake from producing a listing that the device cannot produce.♻️ Proposed guard
bool InMemoryFileSystem::mkdir(const char* path) { if (path == nullptr) { return false; } + // A file already occupying the name makes the directory impossible, as + // on device. + const std::string requested = normalizeDir(path); + auto existing = files.find(requested); + if (existing != files.end() && existing->second.exists) { + return false; + } // Creates parents too, and succeeds when the directory already exists -- // both per IFileSystem::mkdir's contract. - for (std::string dir = normalizeDir(path); !dir.empty(); dir = parentOf(dir)) { + for (std::string dir = requested; !dir.empty(); dir = parentOf(dir)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Host/support/KernelTestDoubles.cpp` around lines 475 - 487, Update InMemoryFileSystem::mkdir to check whether the normalized requested path already exists in files before inserting it or its parents; return false for an existing file, while preserving the current behavior for existing directories and parent creation.
368-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider invalidating the enumeration state in
setPath().
setPath()changesmPathbut keepsmOpen,mEntries, andmCursor. If a caller callssetPath()while the handle is open,readNext()continues to return entries of the previous directory. A real backend cannot show that combination. Clearing the cursor state, or rejecting the call while open, keeps the fake honest.♻️ Proposed change
void InMemoryFileSystem::InMemoryDirectory::setPath(const char* path) { mPath = path != nullptr ? path : ""; + // A retargeted handle must not keep serving the previous directory's + // snapshot. + mOpen = false; + mEntries.clear(); + mCursor = 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Host/support/KernelTestDoubles.cpp` around lines 368 - 371, Update InMemoryDirectory::setPath() to invalidate the existing enumeration state when changing mPath: close the handle and clear mEntries and mCursor, or reject the path change while mOpen is true. Ensure subsequent readNext() calls cannot return entries from the previous directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/Host/support/KernelTestDoubles_test.cpp`:
- Around line 86-99: Update the MkdirMakesAnEmptyDirectoryEnumerable test to
assert that the directory handle’s open() call succeeds directly before checking
listDir(fs, "Debug").empty(). Keep the empty-vector assertion for verifying
enumeration of a successfully opened empty directory, and apply the same change
to the corresponding test block noted in the comment.
In `@Tests/Host/support/KernelTestDoubles.cpp`:
- Around line 581-600: Update objectInfo() so both its file and directory
branches assign baseNameOf(path) to item.name, matching readNext() and FatFs
f_stat behavior. Keep the existing metadata assignments unchanged.
---
Nitpick comments:
In `@Tests/Host/support/KernelTestDoubles.cpp`:
- Around line 498-501: Update InMemoryFileSystem::dir to return an empty
unique_ptr when path is nullptr, matching file() semantics; retain
InMemoryDirectory construction for non-null paths.
- Around line 475-487: Update InMemoryFileSystem::mkdir to check whether the
normalized requested path already exists in files before inserting it or its
parents; return false for an existing file, while preserving the current
behavior for existing directories and parent creation.
- Around line 368-371: Update InMemoryDirectory::setPath() to invalidate the
existing enumeration state when changing mPath: close the handle and clear
mEntries and mCursor, or reject the path change while mOpen is true. Ensure
subsequent readNext() calls cannot return entries from the previous directory.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 665d7d28-c3e6-48c4-92dc-ae2e732e96d7
📒 Files selected for processing (7)
Docs/unit-testing.mdLibs/Source/Variant/VariantConfig.cppTests/Host/CMakeLists.txtTests/Host/support/KernelTestDoubles.cppTests/Host/support/KernelTestDoubles.hppTests/Host/support/KernelTestDoubles_test.cppTests/Host/variant/VariantConfig_test.cpp
tobymurray
force-pushed
the
feat/inmemory-directory
branch
from
August 8, 2026 17:49
c1ec0a0 to
98f41e6
Compare
tobymurray
changed the title
tobymurray
force-pushed
the
feat/inmemory-directory
branch
2 times, most recently
from
August 8, 2026 18:36
c4bc173 to
602b565
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/Host/support/KernelTestDoubles_test.cpp (1)
587-645: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a rename whose destination canonicalises to the source.
The suite covers a rename onto a different existing file at Line 642, but not a rename onto another spelling of the source itself. That case currently deletes the file and reports success. See the fix proposed on
Tests/Host/support/KernelTestDoubles.cppLines 675-696.💚 Suggested addition
// Overwriting a plain file is still allowed, as std::rename does it. EXPECT_TRUE(fs.rename("src.txt", "blocker.txt")); EXPECT_EQ(fs.readFile("blocker.txt"), "SRC"); EXPECT_FALSE(fs.exist("src.txt")); + + // The same object under another spelling: a no-op success, not a deletion. + EXPECT_TRUE(fs.rename("blocker.txt", "/blocker.txt")); + EXPECT_TRUE(fs.exist("blocker.txt")); + EXPECT_EQ(fs.readFile("blocker.txt"), "SRC");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Host/support/KernelTestDoubles_test.cpp` around lines 587 - 645, Add a test in RenameAndCopyRefuseADestinationThatCannotHoldAFile or a nearby rename test covering a destination spelling that canonicalises to the source path, such as a trailing-slash or equivalent normalized form. Assert rename reports success without deleting the source, and verify the file remains readable and exists afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/Host/support/KernelTestDoubles.cpp`:
- Around line 675-696: Update InMemoryFileSystem::rename to return true
immediately when the canonical source and target keys are identical, without
modifying files. For different keys, move the source entry to target and erase
the source by canonical key rather than iterator, avoiding invalidation from
potential unordered_map rehashing.
---
Nitpick comments:
In `@Tests/Host/support/KernelTestDoubles_test.cpp`:
- Around line 587-645: Add a test in
RenameAndCopyRefuseADestinationThatCannotHoldAFile or a nearby rename test
covering a destination spelling that canonicalises to the source path, such as a
trailing-slash or equivalent normalized form. Assert rename reports success
without deleting the source, and verify the file remains readable and exists
afterward.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eff058f3-74b2-4fa9-b940-23223240d01f
📒 Files selected for processing (7)
Docs/unit-testing.mdLibs/Header/SDK/Interfaces/IFileSystem.hppLibs/Source/Variant/VariantConfig.cppTests/Host/support/KernelTestDoubles.cppTests/Host/support/KernelTestDoubles.hppTests/Host/support/KernelTestDoubles_test.cppTests/Host/variant/VariantConfig_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Docs/unit-testing.md
- Libs/Source/Variant/VariantConfig.cpp
tobymurray
force-pushed
the
feat/inmemory-directory
branch
3 times, most recently
from
August 8, 2026 19:37
e751d5a to
dbf120a
Compare
tobymurray
changed the title
tobymurray
force-pushed
the
feat/inmemory-directory
branch
from
August 8, 2026 19:54
dbf120a to
96d93cc
Compare
`InMemoryFileSystem`'s `IDirectory` is a stub whose `readNext()` always returns false, so SDK code that walks a directory has its loop body unreachable from a test. `VariantConfig`'s sandbox-root scan is the only such caller in the tree, and it has no host tests. `InMemoryDirectory` enumerates the flat `files` map as a hierarchy: a path's parent is everything before its last '/', so seeding "a/b/c.txt" makes "a" and "a/b" enumerable with no mkdir call. Subdirectories come back with `isDir` set, so an `if (item.isDir) continue;` guard is exercised rather than silently dead. Both files and subdirectories are reported, and a listing never repeats a name whatever mix of spellings and implied or explicit directories produced it. Enumeration is sorted by name. The backing store is an unordered_map, so anything depending on order -- picking the "first" matching entry, say -- would otherwise be a coin flip between runs. Real FAT enumerates in directory-entry order, so this is a guarantee the fake makes for the sake of reproducible tests, not one the device makes. `readNext()` is a cursor over a snapshot taken at `open()`, re-taken on an explicit rewind, matching POSIX `rewinddir`. Entries added mid-scan do not retroactively appear until then. Around that: `mkdir()` creates parents and refuses a name a file already holds, `IDirectory::create()` is the simulator's single non-recursive `::mkdir` and does not invent parents, `remove()` takes an empty directory and reports whether it removed anything, and `objectInfo()` fills `name` with the leaf rather than the path handed in -- the convention every `readNext()` already follows. The deliberate divergences from a real backend are listed on the class and mirrored in Docs/unit-testing.md, alongside a note on proving a scan is live: a test expecting "nothing was found" cannot otherwise tell a correct decision from an enumerator that never ran.
A fake is only as useful as its refusals. This one grants several that no
backend grants, so a test can build a state the device cannot reach and then
write assertions against it. Each was settled by running the same call
sequence against the simulator's Mock::FileSystem, which the host tests
already build, rather than against an assumption about what POSIX says.
One name could be both a file and a directory. `mkdir()` and
`IDirectory::create()` already refused a name a file held, but a write-mode
`open()` and `seedFile()` would plant a file over an existing directory, or
underneath another file. `canHoldFileAt()` now states that rule once and
`rename()` and `copy()` apply it to their destination too, standing in for
the backend's EISDIR and ENOTDIR. `seedFile()` returns bool to report the
refusal.
`rename()` and `copy()` took a tombstone as a source. Both looked the entry
up without checking `exists`, so a path this filesystem had already removed
still counted: `rename("gone.txt", "live.txt")` returned true and moved the
cleared entry onto live.txt, destroying a live file with a call that should
have been a no-op returning false. `remove()` reads that flag already; these
are the same rule. Their destination is checked as well -- the root, an
existing directory and a path under a file are refused, where a destination
of "" previously filed the entry under the root's own key and lost it --
while overwriting a plain file still works, as `std::rename` does it.
A source and destination that canonicalise to one key are the no-op success
`std::rename` makes them, rather than an entry moved onto itself, left
unspecified, and then erased. `rename()` erases by key rather than through
an iterator, which an intervening rehash may have invalidated.
`close()` could not fail. It returned true for a handle never opened, a file
never seeded, and a directory that does not exist, which left every
`EXPECT_TRUE(h->close())` an assertion with no failing mode. The simulator's
`File::close()` and `Directory::close()` both answer false there.
`IFile::setPath()` left an open handle open. `rename()` may keep one open,
being the same file under a new name, and migrates the open-handle count to
match; `setPath()` names a different file, so the count cannot follow.
Leaving it open stranded the old path's bucket and made the eventual
`close()` decrement the new path's from zero, underflowing size_t and taking
the leak instrumentation with it, while writes silently retargeted.
`IDirectory::setPath()` invalidates its scan for the same reason.
`canonicalPath()` collapses runs of '/', so "a//b" and "a/b" are one object
as on POSIX and FatFs. Splitting them left an empty path segment, which
enumerated as an entry with no name -- something no backend can show, and
which no caller could join back onto its parent.
`IDirectory::create()` on the root answers true, where `::mkdir` gives EEXIST
and the simulator reports success once it confirms the target is a directory.
Each refusal has a test that fails when it is dropped. Two of the additions
pin contracts that nothing currently violates -- the rehash path and copying
an object onto itself -- and say so, rather than implying coverage they do
not carry. The divergence lists on the class and in Docs/unit-testing.md are
reconciled and now record the boundary both listing guarantees stop at: a
name too long for ObjectInfo::name comes back clipped, so it names nothing,
and two names differing only past that capacity come back as one repeated
entry.
VariantConfig's sandbox-root scan decides what users see their activity called. It builds each candidate path from an enumerated name, and ObjectInfo::name is as wide as a whole path, so "/" + name need not fit in one. A clipped path can name a different, existing file; if that file is an alias, the app adopts its name and its FIT sport and comes up calling itself the wrong thing. The length is measured before the path is built, so there is nothing to truncate. Recovering it from snprintf's return would fix the behaviour but not the diagnostic: -Wformat-truncation fires on a "/%s" whose source is as wide as its destination whatever the caller does with the result, and that is the -Os -Wall cmake/una-app.cmake applies to every app build. Verified gone for both host g++ and arm-none-eabi-g++. The candidate buffer lives inside the scan loop, so an entry whose path is declined names nothing rather than inheriting what the last entry left behind. Ordering the conditions so the stale path is never read would work today and is too fragile a thing to rest "never open the wrong .uapp" on. The buffer is not the root cause and this does not pretend to fix it: ObjectInfo::name is as wide as a whole path, so a maximal name can never be joined onto a directory and still fit. Any future caller that scans and joins meets the same wall, so the sizing relationship is spelled out on the field. Also adds the suite this file never had. The mixed-directory determinism rules had no coverage at all: a real .uapp beside an alias means classic (tested with the real app both first and last in enumeration order, since which is seen first decides whether a candidate is adopted before being discarded), an unreadable candidate counts as real, non-apps and directories are skipped, and a malformed alias -- unknown payload version, unknown schema, bad JSON, zero-length config -- falls back to defaults rather than bricking the launch. Most of these expect the classic app, which is also the answer a scan that saw nothing would give, so each asserts the sandbox root really enumerates what was seeded before trusting the verdict. The truncation test carries a positive control: the same alias payload, at the longest name that does fit, is adopted -- so length is the only thing separating the two cases, and the test cannot pass merely because the file was never reached.
tobymurray
force-pushed
the
feat/inmemory-directory
branch
from
August 8, 2026 20:06
96d93cc to
62ed35a
Compare
tobymurray
marked this pull request as ready for review
August 8, 2026 23:47
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.
The bug
VariantConfigcan adopt the wrong app's identity. Its sandbox-root scan builds each candidate path from an enumerated name, andObjectInfo::nameis as wide as a whole path, so"/" + nameneed not fit in one. A clipped path can name a different, existing file, and if that file is an alias the app comes up calling itself by another activity's name and logging that activity's FIT sport.Measuring the name before building the path fixes it, and also clears the
-Wformat-truncationthis line has been earning at the-Os -Wallevery app build uses. Recovering the length fromsnprintf's return would have fixed the behaviour but not the diagnostic, which is about the call rather than what the caller does with its result.Why the test double is in here
The bug was invisible because nothing could reach it.
InMemoryFileSystem'sIDirectoryis a stub whosereadNext()always returns false, so code that walks a directory has its loop body unreachable from a test, and this scan is the only such caller in the tree. It had no host tests at all.Making the fake enumerate is the first commit. The second is the one worth arguing about: once tests can lean on the fake, its habit of permitting states the device cannot reach stops being harmless. It would let one name be both a file and a directory, accept a removed path as a rename source and destroy whatever sat at the destination while returning true, and never fail a
close(). Every refusal added there has a test that fails when the refusal is dropped, and each was settled by running the same call sequence against the simulator'sMock::FileSystemrather than against assumptions about what POSIX says.Risk
One change reaches past the fake's own tests:
close()now returns false for a handle that is not open, matching the simulator and FatFs. Fourteen production sites propagate that return, and none currently reachesclose()on an unopened handle.Summary by CodeRabbit
Bug Fixes
Documentation
Tests