Honor Windows token owner for private files#6
Merged
Conversation
Windows can assign newly created filesystem objects to the process token owner rather than the token user SID, especially for elevated/admin tokens on hosted runners. The daemon runtime store was rejecting directories and runtime records that the same process had just created, which broke consumers that rely on safefileio for private runtime state. Validate private directories and current-user files against the current token owner SID while continuing to grant the token user explicit DACL access. This matches Windows ownership semantics without weakening the non-reparse and restricted-DACL checks. Validation: go test ./...; go vet ./...; GOOS=windows GOARCH=amd64 go test -c ./safefileio -o /tmp/kit-safefileio-windows.test.exe; GOOS=windows GOARCH=amd64 go test -c ./daemon -o /tmp/kit-daemon-windows.test.exe 🤖 Generated with [OpenAI Codex](https://openai.com/codex) Co-authored-by: OpenAI Codex <noreply@openai.com>
roborev: Combined Review (
|
Kit did not have repository-level CI beyond the existing GitHub security workflows, which left PRs without native Windows test coverage for filesystem and daemon behavior. Add a small workflow modeled after the sibling kenn-io repositories so every PR builds, tests, and vets on Linux, macOS, and Windows with pinned official actions and read-only checkout credentials. The hygiene job checks that module metadata remains tidy. Running that locally with Go 1.26.3 required marking golang.org/x/mod and golang.org/x/sys as direct requirements, so include that metadata-only adjustment with the workflow instead of letting the new job fail on its first run. Validation: go mod tidy && git diff --exit-code -- go.mod go.sum; go build ./...; go test ./...; go vet ./... Generated with OpenAI Codex Co-authored-by: OpenAI Codex <noreply@openai.com>
roborev: FailSummary verdict: One medium-severity Windows ownership regression should be addressed before merging. Medium
Review type: | Agent: codex | Job: 19402 |
Adding native Windows CI exposed a few assumptions in the git helpers and tests that only held on Unix-like systems. Git reports some paths with slash separators on Windows, and the test fake-git shim was a shell script that Windows never executed. Normalize worktree-list paths before returning them and make the tests compare cleaned paths and use platform-native null device and fake command behavior. This keeps the new Windows lane meaningful without relaxing the CI coverage. Validation: go mod tidy && git diff --exit-code -- go.mod go.sum; go test ./...; go vet ./... Generated with OpenAI Codex Co-authored-by: OpenAI Codex <noreply@openai.com>
Remote and Actions-based Windows validation surfaced an elevation boundary that the token-owner fix did not fully cover. The runtime path is derived from the token user SID, but Windows may assign object ownership to either the token user or token owner depending on how the process is launched. Accept both SIDs during private directory and current-user file validation while continuing to grant DACL access to the token user. That keeps elevated and non-elevated runs for the same account compatible without weakening the reparse-point or restricted-DACL checks. Validation: go test ./...; go vet ./...; GOOS=windows GOARCH=amd64 go test -c ./safefileio -o /tmp/kit-safefileio-windows.test.exe; GOOS=windows GOARCH=amd64 go test -c ./daemon -o /tmp/kit-daemon-windows.test.exe Generated with OpenAI Codex Co-authored-by: OpenAI Codex <noreply@openai.com>
The new Ubuntu CI lane exposed that a one millisecond per-probe timeout was too tight for hosted runners, even when the next local httptest server was healthy. That made the test assert scheduler timing rather than Discover's behavior. Make the slow probe block until its request context is canceled and use a timeout that still exercises the per-probe skip path while leaving room for the fast local probe to complete. This keeps the coverage meaningful and removes the CI race. Validation: go test ./daemon -run TestDiscoverSkipsPerProbeTimeouts -count=20; go test ./...; go vet ./... Generated with OpenAI Codex Co-authored-by: OpenAI Codex <noreply@openai.com>
The new CI matrix was still using plain go test output, which is harder to scan in GitHub logs when a package fails. Match the middleman workflow style by running tests through gotestsum with pkgname-and-test-fails formatting while keeping the raw JSON event stream available as a per-runner file. Record gotestsum as a Go tool dependency so CI uses a pinned version through the module rather than installing an implicit latest binary. Validation: go tool gotestsum --format pkgname-and-test-fails --jsonfile=/tmp/kit-gotestsum.json -- ./...; go vet ./... Generated with OpenAI Codex Co-authored-by: OpenAI Codex <noreply@openai.com>
roborev: Combined Review (
|
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.
Summary
Validation
Windows verification note
This repository does not include a visible Windows Go test workflow to dispatch. \Dependency Graph active 286880732
CodeQL active 286880707 only shows Dependency Graph and CodeQL, so I could cross-compile the Windows test binaries locally but could not run the Windows-only tests in GitHub Actions from this repo.