fix(windows): build resolvable file URIs for artifacts and environments - #918
Merged
Conversation
Artifact and environment URIs were built as url.URL{Scheme: "file", Path:
path}. On Windows that renders C:\dir\result.txt as
file://C:%5Cdir%5Cresult.txt: the drive letter becomes the URI host and every
separator is percent-encoded. Nothing could resolve it — filepathFromURI
rejects a URI with a host, so artifact lookup failed outright.
This was reported earlier as cosmetic, on the grounds that these are recorded
identifiers rather than something opened. That was wrong: the managed
execution plane test opens exactly this URI, and it failed with
open C:%5CUsers%5C...%5Cresult.txt: The system cannot find the file
specified
Add internal/fileuri and use it at all four build sites and the one parse
site. New makes the path absolute, converts to forward slashes, and
guarantees the leading slash a file URI needs ahead of a drive letter. Path
reverses it, dropping that slash only when a drive letter follows.
Nothing recorded is invalidated. On unix an absolute path already starts with
a slash and ToSlash is a no-op, so old and new URIs are byte-identical; on
Windows the old form never resolved, so there is nothing to stay compatible
with. Path rejects it explicitly rather than guessing.
releasedArtifact now returns an error, since building a URI can fail.
The tarsserver helper that reads these URIs used to strip "file://" and treat
the rest as a path, which is how it accepted a malformed URI and then failed
at open() instead of at parse. It now resolves them the way production does.
Windows: internal/executionplane and internal/workerprotocol lose two
failures and gain none, and the managed execution plane test passes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI's diff-coverage gate failed at 74.6%, and the uncovered lines were almost entirely the error branches this change had just added: four callers each handling a failure from fileuri.New that no test can reach. The error was never real. New made the path absolute with filepath.Abs, which fails only when the working directory cannot be determined, and every caller already holds an absolute path. An unreachable branch in four places is the signal that it does not belong in the signature. New now returns a string. releasedArtifact goes back to returning one value too. The change is smaller than before it: 39 changed coverable lines rather than 59, at 97.4% covered rather than 74.6%, because the lines that went away were the ones nothing could exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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
Artifact and environment URIs were built as
url.URL{Scheme: "file", Path: path}. On Windows that rendersC:\dir\result.txtasfile://C:%5Cdir%5Cresult.txt— the drive letter becomes the URI host and every separator is percent-encoded. Nothing could resolve it:filepathFromURIrejects a URI with a host, so artifact lookup failed outright.I previously reported this as cosmetic, on the grounds that these are recorded identifiers rather than something opened. That was wrong. The managed execution plane test opens exactly this URI:
Adds
internal/fileuriand uses it at all four build sites and the one parse site:executionplane/artifact_collector.goexecutionplane/worktree_environment.goexecutionplane/local_environment.goworkerprotocol/artifact_quarantine.goNewmakes the path absolute, converts to forward slashes, and guarantees the leading slash a file URI needs ahead of a drive letter.Pathreverses it, dropping that slash only when a drive letter follows.Compatibility
Nothing recorded is invalidated:
ToSlashis a no-op, so old and new URIs are byte-identical. Existing records keep resolving.Pathrejects it explicitly rather than guessing at it, and there is a test case for that.releasedArtifactnow returns an error, since building a URI can fail.The test that hid this
filepathFromArtifactURIin the tarsserver tests stripped"file://"and treated the rest as a path. That is why a malformed URI got as far asopen()instead of failing at parse. It now resolves URIs the way production does.Test plan
go build ./...for windows, linux, and darwingo veton the touched packages under Windows and Linuxinternal/fileuri— round trip, URI shape, relative paths, and five rejection cases./scripts/windows_test.sh— exit 0TestManagedWorkExecutionPlaneRunsLifecycleAndPreservesSourcepasses on Windows (was failing)Measured against the pre-change tree on Windows, comparing normalized failure sets so run-to-run timings do not pollute the diff:
executionplane+workerprotocolNo new failures. The two that now pass are
TestArtifactSanitizersAndURIContracts— the URI contract test itself — andTestFileArtifactCollectorCapturesTrackedAndUntrackedGitPatch. The remaining 21 are the pre-existing Windows issues that keep both packages on the excluded list (symlinks, POSIX file modes, ssh/container policy).🤖 Generated with Claude Code