Repository navigation
overlay: require the attr tools instead of silently merging directories - #12
Merged
Merged
Conversation
overlay_is_opaque() treated a missing getfattr as "this directory is not
opaque". That is not a safe default. Without attr installed, a directory
the agent deleted and recreated was merged into rather than replaced, so
files the agent had removed reappeared in the working tree after the
write-back, and the summary showed the replacement as a plain addition
because it reads the same marker. The user approved a changeset that did
not describe what they were getting.
Reproduced on a tree containing mydir/old.txt, with the agent running
`rm -rf mydir && mkdir mydir && echo new > mydir/new.txt`:
without attr summary: "A mydir/new.txt"
after write-back: new.txt AND old.txt
with attr summary: "R mydir/" and "A mydir/new.txt"
after write-back: new.txt
Require getfattr and setfattr up front, next to the bubblewrap version
check, and name the package to install. Refusing to start is better than
writing back something that does not match what the agent did. The guard
inside overlay_is_opaque is dropped with it, so a missing tool can no
longer be mistaken for an answer.
Only --overlay is affected; every other mode still needs nothing beyond
bwrap.
Tests: the overlay tier now also asserts that a replaced directory drops
the old contents and is reported as R, and 30-options asserts the refusal
when getfattr is masked from PATH. CI installs attr in both jobs and fails
loudly if it is absent, so the tier cannot silently skip into a green run.
The bubblewrap version check runs before the attr check, so on a host with an older bwrap the wrapper fails with the version message and the test was asserting against the wrong error.
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.
overlay_is_opaque()treated a missinggetfattras "this directory is not opaque". That isn't a safe default.Without
attrinstalled, a directory the agent deleted and recreated was merged into rather than replaced. Files the agent had removed reappeared in the working tree after the write-back, and the summary showed the replacement as a plain addition, because it reads the same marker. So the user approved a changeset that didn't describe what they were actually getting.Reproduced on a tree containing
mydir/old.txt, agent runningrm -rf mydir && mkdir mydir && echo new > mydir/new.txt:attrA mydir/new.txtnew.txtandold.txtattrR mydir/+A mydir/new.txtnew.txtThe fix: require
getfattrandsetfattrup front, next to the existing bubblewrap version check, naming the package to install. Refusing to start beats writing back something that doesn't match what the agent did. The guard insideoverlay_is_opaquegoes with it, so a missing tool can no longer be mistaken for an answer.Only
--overlayis affected. Every other mode still needs nothing beyondbwrap.Tests: the overlay tier now asserts that a replaced directory drops the old contents and is reported as
R;30-optionsasserts the refusal whengetfattris masked from PATH. Mutation-checked — reverting the fix fails that test.CI: installs
attrin both jobs and fails loudly if it's absent, so the overlay tier can't silently skip into a green run that tested nothing. That's how this went unnoticed: the tier was skipping.Verified locally against bubblewrap 0.11.0 with attr present: 87 passed, 0 failed, 3 skipped (only uninstalled agent binaries).