fix(devcontainer): use 0o755 for feature directories (missing execute bit)#507
fix(devcontainer): use 0o755 for feature directories (missing execute bit)#507
Conversation
…s can actually walk into them MkdirAll was called with 0o644 (rw-r--r--) which drops the execute bit that directories need to be traversable. Harmless on the in-memory filesystem used in tests, but on a real filesystem non-root container users couldn't access feature install scripts. Fixes #506
There was a problem hiding this comment.
Pull request overview
This PR fixes feature extraction directory permissions in devcontainer compilation so non-root users can traverse feature directories on real filesystems, addressing issue #506.
Changes:
- Update the features root directory
MkdirAllpermission from0o644to0o755. - Update each per-feature extraction directory
MkdirAllpermission from0o644to0o755.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…context The featuresDir creation already wraps its error, but the per-feature directory creation returned the raw error. When this fails in the field you'd get a bare permission/path error with no indication of which feature or directory caused it.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Correct fix, well-scoped. Two constants changed, two-sentence description, no scope creep. The permission values now match every other production MkdirAll in the codebase (all use 0o755, 0o750, or 0o700).
One concern (P2), two adjacent nits (P4). The P2 is that the fix ships without proof it works: no test asserts the permission value, memfs doesn't enforce permissions, and the integration test runs as root. Five of seven reviewers flagged this independently.
"Revert the fix, all tests stay green. The bug described in #506 has no regression test." -- Bisky
The two P4s are pre-existing issues adjacent to the changed lines, not introduced by this PR.
🤖 This review was automatically generated with Coder Agents.
Extends TestCompileWithFeatures/WithoutBuildContexts to stat the features root and per-feature directories after Compile and verify they have 0o755 permissions. memfs preserves the mode argument, so reverting the fix back to 0o644 will fail this assertion.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 2. F1 (P2, missing regression test) addressed in 3304543 with permission assertions. F2 (P4, unwrapped error) addressed in 670d34b with descriptive wrapping. Both look good.
F3 (P4, featureSha naming) is unresolved: thread was closed with no reply and no code change. This is a pre-existing nit, not introduced by this PR. A brief acknowledgment (e.g. "won't fix, pre-existing") would unblock the next review round. Holding further review until then.
🤖 This review was automatically generated with Coder Agents.
MkdirAllcalls forfeaturesDirand per-featurefeatureDirfrom0o644to0o755.Fixes #506