Repository navigation
refactor(cli): open CLI trees confined, creating them when absent - #570
Merged
Merged
Conversation
Every CLI-side root was writable-unconfined, and the reason recorded at the one site that gave one was not access but bootstrapping: "the prefix need not exist yet -- a first install creates it -- and a confined root requires its anchor to exist." Tracing all of them (#568 step 1) found none that reads or writes outside its own tree, so the reason is the only thing keeping them unconfined. cli.OpenTree is the compound operation that answers it: create the directory when absent, then open a confined root at it. It is named for the creation so that opening keeps meaning only opening -- fsroot.OpenConfined stays a query, and a missing root stays an error. Creation runs through a root rather than os.MkdirAll, because a mode is worth nothing on windows without the access-control list fsroot applies for it: a directory made 0o700 by os.MkdirAll carries an inherited DACL and is readable by every account with access to its parent. The chain is created through a root anchored at the volume, so every component is kernel-resolved and no window opens between creating the path and opening it. A path that is not absolute on this platform is refused rather than resolved -- on windows that includes a leading separator with no volume, which is drive-relative and belongs to #392. The mode is 0o700 because the XDG Base Directory Specification says so: "If, when attempting to write a file, the destination directory is non-existent an attempt should be made to create it with permission 0700." The sites used 0o750, so this drops group access on the config, state, cache and layers trees. Owner-only is also what makes them enforceable on windows. Fourteen production sites move over: config, store, selfinstall, man, star's extension hook, and devlore-test's workspace, which needed only OpenConfined since its tree already exists. Several carried a //nolint whose reasoning was "an unconfined root holds no handle, so Close cannot fail" -- untrue of a confined root, so those now say what is actually being ignored and why. Eight test fixtures failed once their roots were confined, and all eight were reaching across trees because nothing stopped them. testFileResource called t.TempDir() itself, so a copy's source sat in a second tree; the archive fixtures wrote their tarball one level above the root they extracted into; and two tests used "/nonexistent/path" as a convenient absent value, which confined is an escape rather than a not-exist. Production has been confined all along -- plan/provider.go:488 and deploy/plan.go:387 -- so these tests were exercising a path production never takes. testFileResource now takes its directory, which puts the constraint at the call site instead of hiding it in the helper. The status document moves step 1's trace and step 2's creation rule from Open to Settled. Verified with make check (103 packages ok, 0 failures), and with build-all and vet-all across linux, darwin and windows. Refs #568.
David-Noble-at-work
added a commit
that referenced
this pull request
Aug 20, 2026
* refactor(fsroot): delete the unconfined variants #568 step 3. Two types remain -- the confined root and its scratch sibling -- and one behavior, the kernel's. Deleted: unconfinedRootReader, unconfinedRootReaderWriter, rootBase, errReadOnly, OpenUnconfined, OpenWritableUnconfined, the two unconfined Mode constants, and the tests that existed only to pin the deleted variants' behavior -- including TestApplyMode_BothWritableRootsProtect, whose surviving half duplicates TestApplyMode_PrivateFileGetsProtectedDACL. Mode, ModeConfined and Open stay as a documented vestige: op.RuntimeEnvironmentSpec.WithRoot still carries the Mode parameter, and step 5 deletes all three with it. star was the last production unconfined root, mode-dispatched, so step 2's constructor migration never saw it. It now anchors at the working directory, confined. A volume anchor was tried first and broke the generator: star scripts resolve relative paths against the session root's anchor, so anchoring at the volume redefined every relative path in every script. #571 holds the anchor question -- working directory, repository root, home, or declared roots -- open until #563's hermeticity design rules it; no current extension uses an absolute path or XDG, so wd-confined drops nothing exercised today. Confining the tests exposed a production defect in CopyDir: it handed MkdirAll a full FileMode, type bits included. os.Mkdir silently discards them; os.Root.Mkdir rejects them as an unsupported file mode, and production selfinstall has run confined since #570, so star's extension-copy hook would have failed on its first directory copy. Now Mode().Perm(). Thirteen remaining test call sites move to OpenConfined -- or cli.OpenTree where the tree must be created first -- and TestChecksumFile_NonExistent stops reaching for /nonexistent/file.txt outside its root, the same fixture defect class #570 fixed eight of. fsroot's own tests drop the variant tables and refusal suites that tested the deleted types. Found while gating and filed as #572: every provider's Makefile gen group lists node_builder.gen_test.go, which the generator no longer emits, so the groups are permanently stale and cross-GOOS make build cannot pass on any tree. Verified with make check (103 packages ok, 0 failures), make build on darwin (full regeneration, every generated file byte-identical to develop HEAD by blob SHA), and make vet under GOOS linux, darwin and windows. Refs #568, #571, #572. * fix(star): close the test Applications, and move the knowledge target inside star's root Two CI failures from confining star's session root, both the confinement collecting debts the unconfined root had been hiding. test (windows): seven TestLintCopyright failures, none of them assertions -- t.TempDir cleanup failed with "being used by another process". A confined root is a real OS handle; the lint tests chdir into the temp directory, build an Application there, and never close it, so the handle pinned the tree. The unconfined root held no handle, which is why the leak was free until now. All three test sites that build an Application now register t.Cleanup(r.Close) -- the two integration sites anchor on the repo directory and never failed, but it is the same leak. knowledge-extract: the workflow ran star from devlore-cli with --target at a sibling checkout, which a wd-confined root refuses (statat ../devlore-registry: path escapes from parent). The registry is now checked out inside the cli tree, after the star build so the nested repo cannot perturb generation, and the target and the registry steps' working directories follow it. This constrains the scenario rather than accommodating it; the Knowledge extension's own argument defaults are cross-root by design (_find_sibling in extract.star), and the ruling recorded on #571 is that star should accommodate working across roots -- the declared-roots candidate. Until #571 is answered, the target lives inside the root. Refs #568, #571. --------- Co-authored-by: David Noble <[email protected]>
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.
#568 step 2. Step 1's trace found no caller reading or writing outside its own tree, so the only thing keeping
CLI roots unconfined was bootstrapping — the one site that recorded a reason said so:
cli.OpenTreeCreates the directory when absent, then opens a confined root at it. Named for the creation, so
fsroot.OpenConfinedstays a query and a missing root stays an error.os.MkdirAll— a mode is worth nothing on Windows without the DACL fsrootapplies for it. A
0o700directory fromos.MkdirAllcarries an inherited DACL and is readable by everyaccount with access to its parent.
path and opening it.
volume — drive-relative, and Unix-literal "/" roots panic on Windows: four test fixtures, and writ's System layer TargetRoot #392's territory, not something to guess at here.
0o700, per the XDG specification: "If, when attempting to write a file, the destination directory isnon-existent an attempt should be made to create it with permission 0700." The sites used
0o750, so thisdrops group access on config, state, cache and layers — and owner-only is what makes them enforceable on
Windows.
14 production sites
config,store,selfinstall(7),man, star's extension hook, and devlore-test's workspace — the lastneeding only
OpenConfined, since its tree already exists.Several carried a
//nolintreading "an unconfined root holds no handle, so Close cannot fail." Untrue of aconfined root, so those now state what is actually being ignored and why.
8 fixtures were reaching across trees
TestIsDir/IsFile_NonExistent"/nonexistent/path"as a convenient absent value — outside the root, so confined it's an escape, not a not-existTestCopy_*(4)testFileResourcecalledt.TempDir()itself, putting the source in a second treeTestExtract_*(2)Production has been confined all along —
plan/provider.go:488,deploy/plan.go:387— so these tests wereexercising a path production never takes.
testFileResourcenow takes its directory, which puts the constraintat the call site rather than hiding it in the helper.
Verified
make check— 103 packages ok, 0 failuresmake build-all,make vet-all— linux, darwin, windowsgofmt -lover the change set — clean4.5-fsroot-variants.status.mdmoves step 1's trace and step 2's creation rule from Open to Settled.Not in this PR
The remaining test call sites still use
OpenWritableUnconfined, which still exists. They go with #568 step 3,which deletes the constructor they call — migrating them first would mean touching them twice.
Refs #568.