From fe1f8f83e4da37843a8a549c4f931bc37a6f9169 Mon Sep 17 00:00:00 2001 From: mark42 Date: Sat, 22 Aug 2026 12:09:44 +0900 Subject: [PATCH 1/2] fix(windows): build resolvable file URIs for artifacts and environments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/executionplane/artifact_collector.go | 16 ++-- internal/executionplane/local_environment.go | 8 +- .../executionplane/worktree_environment.go | 8 +- internal/fileuri/fileuri.go | 61 +++++++++++++++ internal/fileuri/fileuri_test.go | 77 +++++++++++++++++++ .../tarsserver/work_scheduler_runtime_test.go | 12 +-- .../workerprotocol/artifact_quarantine.go | 15 ++-- 7 files changed, 177 insertions(+), 20 deletions(-) create mode 100644 internal/fileuri/fileuri.go create mode 100644 internal/fileuri/fileuri_test.go diff --git a/internal/executionplane/artifact_collector.go b/internal/executionplane/artifact_collector.go index ef039b18..94621eec 100644 --- a/internal/executionplane/artifact_collector.go +++ b/internal/executionplane/artifact_collector.go @@ -8,13 +8,13 @@ import ( "encoding/json" "fmt" "mime" - "net/url" "os" "path/filepath" "sort" "strings" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" ) const ( @@ -323,18 +323,22 @@ func writeCollectedArtifact(root, kind, name string, raw []byte) (CollectedArtif if mediaType == "" { mediaType = "application/octet-stream" } + uri, err := fileuri.New(path) + if err != nil { + return CollectedArtifact{}, err + } return CollectedArtifact{ - Kind: kind, Name: filepath.ToSlash(name), URI: (&url.URL{Scheme: "file", Path: path}).String(), + Kind: kind, Name: filepath.ToSlash(name), URI: uri, Digest: fmt.Sprintf("sha256:%x", digest[:]), MediaType: mediaType, SizeBytes: int64(len(raw)), }, nil } func filepathFromURI(raw string) (string, error) { - parsed, err := url.Parse(raw) - if err != nil || parsed.Scheme != "file" || parsed.Host != "" { - return "", fmt.Errorf("executionplane: invalid file artifact URI") + path, err := fileuri.Path(raw) + if err != nil { + return "", fmt.Errorf("executionplane: invalid file artifact URI: %w", err) } - return filepath.FromSlash(parsed.Path), nil + return path, nil } var _ ArtifactCollector = (*FileArtifactCollector)(nil) diff --git a/internal/executionplane/local_environment.go b/internal/executionplane/local_environment.go index be4c7205..2126f936 100644 --- a/internal/executionplane/local_environment.go +++ b/internal/executionplane/local_environment.go @@ -4,12 +4,12 @@ import ( "context" "encoding/json" "fmt" - "net/url" "os" "path/filepath" "strings" "time" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/proofverifier" "github.com/devlikebear/tars/internal/workstore" ) @@ -85,9 +85,13 @@ func (provider *LocalEnvironmentProvider) Sync(ctx context.Context, environment } metadata, _ := json.Marshal(map[string]any{"artifact_digests": json.RawMessage(artifactDigests)}) now := provider.now().UTC() + uri, err := fileuri.New(recovered.RootDir) + if err != nil { + return EnvironmentSnapshot{}, err + } return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: (&url.URL{Scheme: "file", Path: recovered.RootDir}).String(), + URI: uri, MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/executionplane/worktree_environment.go b/internal/executionplane/worktree_environment.go index 1160e11c..f55c3f39 100644 --- a/internal/executionplane/worktree_environment.go +++ b/internal/executionplane/worktree_environment.go @@ -6,7 +6,6 @@ import ( "crypto/sha256" "encoding/json" "fmt" - "net/url" "os" "os/exec" "path/filepath" @@ -14,6 +13,7 @@ import ( "time" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/proofverifier" "github.com/devlikebear/tars/internal/workstore" ) @@ -159,9 +159,13 @@ func (provider *ManagedWorktreeProvider) Sync(ctx context.Context, environment E "git_diff_digest": digestRaw(diff), "git_status_entries": bytes.Count(status, []byte{0}), }) now := provider.now().UTC() + uri, err := fileuri.New(recovered.RootDir) + if err != nil { + return EnvironmentSnapshot{}, err + } return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: (&url.URL{Scheme: "file", Path: recovered.RootDir}).String(), + URI: uri, MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/fileuri/fileuri.go b/internal/fileuri/fileuri.go new file mode 100644 index 00000000..d554bbf4 --- /dev/null +++ b/internal/fileuri/fileuri.go @@ -0,0 +1,61 @@ +// Package fileuri converts between local paths and file: URIs. +// +// It exists because url.URL{Scheme: "file", Path: path} is wrong on Windows: +// `C:\dir\artifact.txt` renders as file://C:%5Cdir%5Cartifact.txt, where the +// drive letter becomes the URI host and every separator is percent-encoded. +// Such a URI is not resolvable — the parser here rejects it, and a naive +// consumer that strips the scheme gets a path that cannot be opened. +// +// A file URI needs forward slashes and a leading one ahead of the drive +// letter: file:///C:/dir/artifact.txt. On unix the absolute path already +// starts with a slash, so both forms agree and existing URIs stay valid. +package fileuri + +import ( + "fmt" + "net/url" + "path/filepath" + "strings" +) + +// New returns the file: URI for path, which is made absolute first so the +// result identifies the same file from any working directory. +func New(path string) (string, error) { + absolute, err := filepath.Abs(strings.TrimSpace(path)) + if err != nil { + return "", fmt.Errorf("fileuri: resolve %q: %w", path, err) + } + uriPath := filepath.ToSlash(absolute) + if !strings.HasPrefix(uriPath, "/") { + uriPath = "/" + uriPath + } + return (&url.URL{Scheme: "file", Path: uriPath}).String(), nil +} + +// Path returns the local path a file: URI refers to. +// +// A URI with a host is rejected rather than guessed at: it is either a remote +// UNC reference this code does not handle, or the malformed shape New exists +// to avoid. +func Path(raw string) (string, error) { + parsed, err := url.Parse(strings.TrimSpace(raw)) + if err != nil { + return "", fmt.Errorf("fileuri: parse %q: %w", raw, err) + } + if parsed.Scheme != "file" { + return "", fmt.Errorf("fileuri: %q is not a file URI", raw) + } + if parsed.Host != "" { + return "", fmt.Errorf("fileuri: %q has a host", raw) + } + uriPath := parsed.Path + // file:///C:/dir becomes /C:/dir when parsed; the leading slash is part of + // the URI grammar, not of the Windows path. + if len(uriPath) > 2 && uriPath[0] == '/' && uriPath[2] == ':' { + uriPath = uriPath[1:] + } + if uriPath == "" { + return "", fmt.Errorf("fileuri: %q has no path", raw) + } + return filepath.FromSlash(uriPath), nil +} diff --git a/internal/fileuri/fileuri_test.go b/internal/fileuri/fileuri_test.go new file mode 100644 index 00000000..5a9d5bf7 --- /dev/null +++ b/internal/fileuri/fileuri_test.go @@ -0,0 +1,77 @@ +package fileuri + +import ( + "path/filepath" + "strings" + "testing" +) + +// TestRoundTripPreservesPath is the property every caller depends on: a path +// turned into a URI and back must name the same file. +func TestRoundTripPreservesPath(t *testing.T) { + want := filepath.Join(t.TempDir(), "nested", "artifact.txt") + + uri, err := New(want) + if err != nil { + t.Fatalf("New: %v", err) + } + got, err := Path(uri) + if err != nil { + t.Fatalf("Path(%q): %v", uri, err) + } + if got != want { + t.Fatalf("round trip = %q, want %q", got, want) + } +} + +// TestNewHasNoHostAndForwardSlashes pins the shape that was wrong on Windows, +// where url.URL{Scheme: "file", Path: path} put the drive letter in the host +// and percent-encoded every separator. +func TestNewHasNoHostAndForwardSlashes(t *testing.T) { + uri, err := New(filepath.Join(t.TempDir(), "artifact.txt")) + if err != nil { + t.Fatalf("New: %v", err) + } + if !strings.HasPrefix(uri, "file:///") { + t.Fatalf("uri = %q, want a file:/// prefix", uri) + } + if strings.Contains(uri, "%5C") || strings.Contains(uri, `\`) { + t.Fatalf("uri = %q, want forward slashes only", uri) + } +} + +func TestNewResolvesRelativePaths(t *testing.T) { + t.Chdir(t.TempDir()) + + uri, err := New("artifact.txt") + if err != nil { + t.Fatalf("New: %v", err) + } + path, err := Path(uri) + if err != nil { + t.Fatalf("Path: %v", err) + } + if !filepath.IsAbs(path) { + t.Fatalf("path = %q, want an absolute path", path) + } + if filepath.Base(path) != "artifact.txt" { + t.Fatalf("path = %q, want it to end at the requested file", path) + } +} + +func TestPathRejectsUnusableURIs(t *testing.T) { + cases := map[string]string{ + "other scheme": "https://example.test/artifact.txt", + "host present": "file://example.test/artifact.txt", + "no path": "file://", + "not a uri": "::not a uri::", + "malformed old": `file://C:%5Cdir%5Cartifact.txt`, + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + if got, err := Path(raw); err == nil { + t.Fatalf("Path(%q) = %q, want an error", raw, got) + } + }) + } +} diff --git a/internal/tarsserver/work_scheduler_runtime_test.go b/internal/tarsserver/work_scheduler_runtime_test.go index 2b12f2a0..4344e949 100644 --- a/internal/tarsserver/work_scheduler_runtime_test.go +++ b/internal/tarsserver/work_scheduler_runtime_test.go @@ -18,6 +18,7 @@ import ( "github.com/devlikebear/tars/internal/agentruntime" "github.com/devlikebear/tars/internal/config" "github.com/devlikebear/tars/internal/executionplane" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/session" "github.com/devlikebear/tars/internal/workerprotocol" "github.com/devlikebear/tars/internal/workscheduler" @@ -639,12 +640,13 @@ func shellQuoteForTest(value string) string { return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" } +// filepathFromArtifactURI resolves an artifact URI the way production does. +// +// It used to strip "file://" and treat the rest as a path, which quietly +// accepted the malformed URIs this code used to produce on Windows and handed +// back a percent-encoded string that could not be opened. func filepathFromArtifactURI(raw string) (string, error) { - const prefix = "file://" - if !strings.HasPrefix(raw, prefix) { - return "", os.ErrInvalid - } - return strings.TrimPrefix(raw, prefix), nil + return fileuri.Path(raw) } func TestBuildNativeWorkExecutionPlaneSelectsHonestEnvironment(t *testing.T) { diff --git a/internal/workerprotocol/artifact_quarantine.go b/internal/workerprotocol/artifact_quarantine.go index 26cf2569..a0330ab4 100644 --- a/internal/workerprotocol/artifact_quarantine.go +++ b/internal/workerprotocol/artifact_quarantine.go @@ -13,6 +13,7 @@ import ( "unicode/utf8" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/secrets" ) @@ -190,7 +191,7 @@ func (quarantine *ArtifactQuarantine) release(ctx context.Context, placementID s if digestBytes(existing) != artifact.Digest { return ReleasedArtifact{}, fmt.Errorf("%w: accepted artifact %q changed digest", ErrManifestMismatch, artifact.Name) } - return releasedArtifact(destination, artifact), nil + return releasedArtifact(destination, artifact) } else if !os.IsNotExist(err) { return ReleasedArtifact{}, fmt.Errorf("workerprotocol: inspect accepted artifact: %w", err) } @@ -212,14 +213,18 @@ func (quarantine *ArtifactQuarantine) release(ctx context.Context, placementID s if err := os.Chmod(destination, 0o600); err != nil { return ReleasedArtifact{}, fmt.Errorf("workerprotocol: secure accepted artifact: %w", err) } - return releasedArtifact(destination, artifact), nil + return releasedArtifact(destination, artifact) } -func releasedArtifact(path string, artifact WireArtifact) ReleasedArtifact { +func releasedArtifact(path string, artifact WireArtifact) (ReleasedArtifact, error) { + uri, err := fileuri.New(path) + if err != nil { + return ReleasedArtifact{}, err + } return ReleasedArtifact{ - Name: artifact.Name, URI: (&url.URL{Scheme: "file", Path: path}).String(), + Name: artifact.Name, URI: uri, Digest: artifact.Digest, MediaType: artifact.MediaType, SizeBytes: int64(len(artifact.Data)), - } + }, nil } type DefaultArtifactScanner struct{} From 29d1cfffbdfa9a128b9d2a678390de79438d44a7 Mon Sep 17 00:00:00 2001 From: mark42 Date: Sat, 22 Aug 2026 12:20:25 +0900 Subject: [PATCH 2/2] refactor(fileuri): drop an error that cannot happen 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 --- internal/executionplane/artifact_collector.go | 6 +---- internal/executionplane/local_environment.go | 6 +---- .../executionplane/worktree_environment.go | 6 +---- internal/fileuri/fileuri.go | 22 ++++++++++++------- internal/fileuri/fileuri_test.go | 15 +++---------- .../workerprotocol/artifact_quarantine.go | 14 +++++------- 6 files changed, 25 insertions(+), 44 deletions(-) diff --git a/internal/executionplane/artifact_collector.go b/internal/executionplane/artifact_collector.go index 94621eec..78bcdb96 100644 --- a/internal/executionplane/artifact_collector.go +++ b/internal/executionplane/artifact_collector.go @@ -323,12 +323,8 @@ func writeCollectedArtifact(root, kind, name string, raw []byte) (CollectedArtif if mediaType == "" { mediaType = "application/octet-stream" } - uri, err := fileuri.New(path) - if err != nil { - return CollectedArtifact{}, err - } return CollectedArtifact{ - Kind: kind, Name: filepath.ToSlash(name), URI: uri, + Kind: kind, Name: filepath.ToSlash(name), URI: fileuri.New(path), Digest: fmt.Sprintf("sha256:%x", digest[:]), MediaType: mediaType, SizeBytes: int64(len(raw)), }, nil } diff --git a/internal/executionplane/local_environment.go b/internal/executionplane/local_environment.go index 2126f936..0cf89446 100644 --- a/internal/executionplane/local_environment.go +++ b/internal/executionplane/local_environment.go @@ -85,13 +85,9 @@ func (provider *LocalEnvironmentProvider) Sync(ctx context.Context, environment } metadata, _ := json.Marshal(map[string]any{"artifact_digests": json.RawMessage(artifactDigests)}) now := provider.now().UTC() - uri, err := fileuri.New(recovered.RootDir) - if err != nil { - return EnvironmentSnapshot{}, err - } return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: uri, + URI: fileuri.New(recovered.RootDir), MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/executionplane/worktree_environment.go b/internal/executionplane/worktree_environment.go index f55c3f39..9a2f9a5a 100644 --- a/internal/executionplane/worktree_environment.go +++ b/internal/executionplane/worktree_environment.go @@ -159,13 +159,9 @@ func (provider *ManagedWorktreeProvider) Sync(ctx context.Context, environment E "git_diff_digest": digestRaw(diff), "git_status_entries": bytes.Count(status, []byte{0}), }) now := provider.now().UTC() - uri, err := fileuri.New(recovered.RootDir) - if err != nil { - return EnvironmentSnapshot{}, err - } return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: uri, + URI: fileuri.New(recovered.RootDir), MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/fileuri/fileuri.go b/internal/fileuri/fileuri.go index d554bbf4..8d3d6d6e 100644 --- a/internal/fileuri/fileuri.go +++ b/internal/fileuri/fileuri.go @@ -18,18 +18,24 @@ import ( "strings" ) -// New returns the file: URI for path, which is made absolute first so the -// result identifies the same file from any working directory. -func New(path string) (string, error) { - absolute, err := filepath.Abs(strings.TrimSpace(path)) - if err != nil { - return "", fmt.Errorf("fileuri: resolve %q: %w", path, err) +// New returns the file: URI for path. +// +// It takes no error because it cannot fail for its callers. path is made +// absolute so the URI identifies the same file from any working directory, +// and filepath.Abs only fails when the working directory cannot be +// determined — unreachable here, since every caller already holds an absolute +// path. Returning an error anyway would put a branch in four callers that no +// test could ever reach. +func New(path string) string { + resolved := strings.TrimSpace(path) + if absolute, err := filepath.Abs(resolved); err == nil { + resolved = absolute } - uriPath := filepath.ToSlash(absolute) + uriPath := filepath.ToSlash(resolved) if !strings.HasPrefix(uriPath, "/") { uriPath = "/" + uriPath } - return (&url.URL{Scheme: "file", Path: uriPath}).String(), nil + return (&url.URL{Scheme: "file", Path: uriPath}).String() } // Path returns the local path a file: URI refers to. diff --git a/internal/fileuri/fileuri_test.go b/internal/fileuri/fileuri_test.go index 5a9d5bf7..5edb5d09 100644 --- a/internal/fileuri/fileuri_test.go +++ b/internal/fileuri/fileuri_test.go @@ -11,10 +11,7 @@ import ( func TestRoundTripPreservesPath(t *testing.T) { want := filepath.Join(t.TempDir(), "nested", "artifact.txt") - uri, err := New(want) - if err != nil { - t.Fatalf("New: %v", err) - } + uri := New(want) got, err := Path(uri) if err != nil { t.Fatalf("Path(%q): %v", uri, err) @@ -28,10 +25,7 @@ func TestRoundTripPreservesPath(t *testing.T) { // where url.URL{Scheme: "file", Path: path} put the drive letter in the host // and percent-encoded every separator. func TestNewHasNoHostAndForwardSlashes(t *testing.T) { - uri, err := New(filepath.Join(t.TempDir(), "artifact.txt")) - if err != nil { - t.Fatalf("New: %v", err) - } + uri := New(filepath.Join(t.TempDir(), "artifact.txt")) if !strings.HasPrefix(uri, "file:///") { t.Fatalf("uri = %q, want a file:/// prefix", uri) } @@ -43,10 +37,7 @@ func TestNewHasNoHostAndForwardSlashes(t *testing.T) { func TestNewResolvesRelativePaths(t *testing.T) { t.Chdir(t.TempDir()) - uri, err := New("artifact.txt") - if err != nil { - t.Fatalf("New: %v", err) - } + uri := New("artifact.txt") path, err := Path(uri) if err != nil { t.Fatalf("Path: %v", err) diff --git a/internal/workerprotocol/artifact_quarantine.go b/internal/workerprotocol/artifact_quarantine.go index a0330ab4..3a34a86f 100644 --- a/internal/workerprotocol/artifact_quarantine.go +++ b/internal/workerprotocol/artifact_quarantine.go @@ -191,7 +191,7 @@ func (quarantine *ArtifactQuarantine) release(ctx context.Context, placementID s if digestBytes(existing) != artifact.Digest { return ReleasedArtifact{}, fmt.Errorf("%w: accepted artifact %q changed digest", ErrManifestMismatch, artifact.Name) } - return releasedArtifact(destination, artifact) + return releasedArtifact(destination, artifact), nil } else if !os.IsNotExist(err) { return ReleasedArtifact{}, fmt.Errorf("workerprotocol: inspect accepted artifact: %w", err) } @@ -213,18 +213,14 @@ func (quarantine *ArtifactQuarantine) release(ctx context.Context, placementID s if err := os.Chmod(destination, 0o600); err != nil { return ReleasedArtifact{}, fmt.Errorf("workerprotocol: secure accepted artifact: %w", err) } - return releasedArtifact(destination, artifact) + return releasedArtifact(destination, artifact), nil } -func releasedArtifact(path string, artifact WireArtifact) (ReleasedArtifact, error) { - uri, err := fileuri.New(path) - if err != nil { - return ReleasedArtifact{}, err - } +func releasedArtifact(path string, artifact WireArtifact) ReleasedArtifact { return ReleasedArtifact{ - Name: artifact.Name, URI: uri, + Name: artifact.Name, URI: fileuri.New(path), Digest: artifact.Digest, MediaType: artifact.MediaType, SizeBytes: int64(len(artifact.Data)), - }, nil + } } type DefaultArtifactScanner struct{}