Compare commits

...
Author SHA1 Message Date
oniisanlabsandClaude Fable 5.1 58ebf5a6de fix(codex): forward tool input that only arrives whole on an output item
The Codex Responses handler kept arguments that arrived only on
response.output_item.done (and custom tool input that arrived on
response.output_item.added) in the builder for history replay, but never
emitted them as tool-call deltas, so the runtime executed the call with an
empty argument string ("path is required").

Route every builder write through one forwardInput helper that also emits
the delta, so the builder holds exactly what the runtime has received. The
added handler forwards whole custom input, the delta handler forwards each
fragment, and the done handler forwards the final input only when nothing
reached the runtime yet, so streamed fragments are never duplicated.

Handler-level coverage spans function and custom calls, with and without
the added item, streamed or final-only, plus custom input on the added
item; a second test drives the same shapes through StreamCompletion and
zeroruntime.CollectStream. Against the unfixed handler the final-only and
input-on-added cases report an empty argument string at tool-call-end.

Closes #1194

Co-Authored-By: Claude Fable 5.1 <[email protected]>
2026-11-09 14:57:36 -08:00
JATMNandAyush7614 88ec97d563 fix(tools): publish file writes atomically instead of truncating in place (#1214)
* fix(tools): publish file writes atomically instead of truncating in place

Stage replacements before publication so failed writes preserve existing
contents, successful renames detach hard-link siblings, and creates publish
without replacing raced files. Preserve modes and contained symlinks, with
the existing validated in-place fallback when replacement is refused.

Rebuild Ayush7614's complete change from Twigpine/zero#1159 on current main.
The net patch is unchanged; original authorship is retained.

Original-commit: d97d1cf7ae
Original-commit: 9b994c1903
Original-commit: 48f65ca634
Original-PR: https://github.com/Twigpine/zero/pull/1159
Fixes: #1156

* fix(tools): preserve atomic publication when replacement is refused

Keep regular-file preimages and hard-link siblings intact after rename errors. Record published creates even when staging cleanup fails while preserving the cleanup error. Make the permission regression fixture independent of umask.

---------

Co-authored-by: Ayush7614 <[email protected]>
2026-11-09 12:25:24 -08:00
18 changed files with 1319 additions and 91 deletions

No files matched your search

@@ -0,0 +1,14 @@
package observability
import (
"os"
"github.com/Twigpine/zero/internal/pathjail"
)
// renameNoReplace publishes a crash report without replacing an existing one.
// The implementation lives in pathjail because the file tools need the same
// atomic no-replace guarantee.
func renameNoReplace(root *os.Root, oldname, newname string) error {
return pathjail.RenameNoReplace(root, oldname, newname)
}
@@ -1,19 +0,0 @@
//go:build darwin
package observability
import (
"fmt"
"os"
"golang.org/x/sys/unix"
)
func renameNoReplace(root *os.Root, oldname, newname string) error {
directory, err := root.Open(".")
if err != nil {
return fmt.Errorf("open crash directory for publication: %w", err)
}
defer directory.Close()
return unix.RenameatxNp(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_EXCL)
}
@@ -1,19 +0,0 @@
//go:build linux
package observability
import (
"fmt"
"os"
"golang.org/x/sys/unix"
)
func renameNoReplace(root *os.Root, oldname, newname string) error {
directory, err := root.Open(".")
if err != nil {
return fmt.Errorf("open crash directory for publication: %w", err)
}
defer directory.Close()
return unix.Renameat2(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_NOREPLACE)
}
+12 -2
View File
@@ -142,12 +142,22 @@ func trimTrailingSeparators(path string) string {
var randomBytes = rand.Read
// CreateTemp makes an O_EXCL temp file named prefix.<random>suffix inside dir,
// relative to handle, and returns it with its relative path.
// relative to handle, and returns it with its relative path. The file is
// created 0o600 so bytes staged in it are not readable before publication.
//
// O_EXCL rather than a fixed name: a link planted at a predictable temp path
// between the check and the create is the same race this package exists to
// close, and O_EXCL makes the kernel refuse it rather than us.
func CreateTemp(handle *os.Root, dir, prefix, suffix string) (*os.File, string, error) {
return CreateTempMode(handle, dir, prefix, suffix, 0o600)
}
// CreateTempMode is CreateTemp with an explicit create mode. The mode is
// passed to the kernel's exclusive create, so the process umask still applies;
// callers that must publish the permissions a normal create would have been
// granted should read the returned file's mode rather than assigning perm
// later, because Chmod does not consult the umask.
func CreateTempMode(handle *os.Root, dir, prefix, suffix string, perm os.FileMode) (*os.File, string, error) {
// prefix and suffix are NAME fragments, not path fragments. A separator or a
// "."/".." in either would relocate the file out of dir — a prefix of
// "../escaped", or a suffix of ".tmp/../evil" that also erases the random
@@ -164,7 +174,7 @@ func CreateTemp(handle *os.Root, dir, prefix, suffix string) (*os.File, string,
return nil, "", fmt.Errorf("generate a temporary name: %w", err)
}
relative := filepath.Join(dir, prefix+"."+hex.EncodeToString(noise)+suffix)
file, err := handle.OpenFile(relative, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600)
file, err := handle.OpenFile(relative, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm)
if err == nil {
return file, relative, nil
}
+28
View File
@@ -0,0 +1,28 @@
package pathjail
import (
"fmt"
"os"
"path/filepath"
)
// RenameNoReplace renames oldname to newname within the directory bound by
// root, and refuses to replace an existing newname with the same atomic
// guarantee the kernel gives a create's exclusive open. A caller that loses
// the race receives fs.ErrExist and nothing at newname changes.
//
// Both names must be single path components: they are resolved relative to
// root's directory handle, so a separator (or an ancestor link) must not be
// able to redirect the syscall outside the checked root. Callers with a path
// in a subdirectory open that directory with Root.OpenRoot first.
//
// Platforms whose kernel cannot publish without replacing fall back to
// ErrUnsupported, which lets the caller use a no-replace hard link instead.
func RenameNoReplace(root *os.Root, oldname, newname string) error {
for _, name := range []string{oldname, newname} {
if name == "" || name == "." || name == ".." || filepath.Base(name) != name {
return fmt.Errorf("%w: rename name %q must be a single path component", ErrEscapes, name)
}
}
return renameNoReplace(root, oldname, newname)
}
@@ -0,0 +1,22 @@
//go:build darwin
package pathjail
import (
"fmt"
"os"
"golang.org/x/sys/unix"
)
func renameNoReplace(root *os.Root, oldname, newname string) error {
directory, err := root.Open(".")
if err != nil {
return fmt.Errorf("open directory for rename: %w", err)
}
defer directory.Close()
if err := unix.RenameatxNp(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_EXCL); err != nil {
return fmt.Errorf("rename %s to %s: %w", oldname, newname, err)
}
return nil
}
@@ -0,0 +1,22 @@
//go:build linux
package pathjail
import (
"fmt"
"os"
"golang.org/x/sys/unix"
)
func renameNoReplace(root *os.Root, oldname, newname string) error {
directory, err := root.Open(".")
if err != nil {
return fmt.Errorf("open directory for rename: %w", err)
}
defer directory.Close()
if err := unix.Renameat2(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_NOREPLACE); err != nil {
return fmt.Errorf("rename %s to %s: %w", oldname, newname, err)
}
return nil
}
@@ -1,6 +1,6 @@
//go:build !darwin && !linux && !windows
package observability
package pathjail
import (
"errors"
@@ -0,0 +1,79 @@
package pathjail
import (
"errors"
"os"
"path/filepath"
"testing"
)
func openRenameRoot(t *testing.T) (*os.Root, string) {
t.Helper()
base := t.TempDir()
root, err := os.OpenRoot(base)
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() { _ = root.Close() })
return root, base
}
// A CREATE'S NAME MUST NOT BE LOST TO A RACE. The no-replace rename reports the
// collision as fs.ErrExist and leaves both files exactly as they were, so the
// caller can refuse the create instead of replacing the other writer.
func TestRenameNoReplaceRefusesAnExistingDestination(t *testing.T) {
root, base := openRenameRoot(t)
if err := os.WriteFile(filepath.Join(base, "source"), []byte("source\n"), 0o644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(base, "target"), []byte("target\n"), 0o644); err != nil {
t.Fatal(err)
}
if err := RenameNoReplace(root, "source", "target"); !errors.Is(err, os.ErrExist) {
t.Fatalf("RenameNoReplace over an existing name = %v, want os.ErrExist", err)
}
for name, want := range map[string]string{"source": "source\n", "target": "target\n"} {
got, err := os.ReadFile(filepath.Join(base, name))
if err != nil || string(got) != want {
t.Fatalf("%s = %q, err=%v; a refused rename must not move either file", name, got, err)
}
}
}
func TestRenameNoReplacePublishesAnAbsentDestination(t *testing.T) {
root, base := openRenameRoot(t)
if err := os.WriteFile(filepath.Join(base, "source"), []byte("whole\n"), 0o644); err != nil {
t.Fatal(err)
}
if err := RenameNoReplace(root, "source", "target"); err != nil {
t.Fatalf("RenameNoReplace = %v", err)
}
if got, err := os.ReadFile(filepath.Join(base, "target")); err != nil || string(got) != "whole\n" {
t.Fatalf("target = %q, err=%v", got, err)
}
if _, err := os.Stat(filepath.Join(base, "source")); !os.IsNotExist(err) {
t.Fatalf("source survived a successful rename: %v", err)
}
}
// The syscall is given a directory handle and bare names. Allowing a
// separator here would let an ancestor component resolve outside the directory
// the caller checked, so it is refused before the syscall.
func TestRenameNoReplaceRejectsPathComponents(t *testing.T) {
root, _ := openRenameRoot(t)
for _, tc := range []struct{ oldname, newname string }{
{"", "target"},
{"source", ""},
{".", "target"},
{"source", ".."},
{"dir/source", "target"},
{"source", "dir/target"},
{filepath.Join("dir", "source"), "target"},
} {
if err := RenameNoReplace(root, tc.oldname, tc.newname); !errors.Is(err, ErrEscapes) {
t.Errorf("RenameNoReplace(%q, %q) = %v, want ErrEscapes", tc.oldname, tc.newname, err)
}
}
}
@@ -1,6 +1,6 @@
//go:build windows
package observability
package pathjail
import (
"errors"
+38 -19
View File
@@ -212,15 +212,34 @@ type errorPayload struct {
// response.output_item.added, the argument deltas, and the final
// response.output_item.done.
type toolCallBuilder struct {
ID string
CallID string
Name string
ID string
CallID string
Name string
// Arguments holds exactly the input already forwarded to the runtime as
// tool-call deltas: every write goes through forwardInput, so an empty
// builder means the runtime has received nothing for this call yet. The
// done handler relies on that to forward input that only arrived whole
// (on the added or done item) without duplicating streamed fragments.
Arguments strings.Builder
Freeform bool
started bool
ended bool
}
// forwardInput appends fragment to the builder and emits the matching
// tool-call delta, keeping the builder and the runtime in step.
func (builder *toolCallBuilder) forwardInput(ctx context.Context, events chan<- zeroruntime.StreamEvent, fragment string) {
if fragment == "" {
return
}
builder.Arguments.WriteString(fragment)
providerio.SendEvent(ctx, events, zeroruntime.StreamEvent{
Type: zeroruntime.StreamEventToolCallDelta,
ToolCallID: builder.ID,
ArgumentsFragment: fragment,
})
}
// responsesState tracks in-flight tool calls and whether the terminal
// event has been emitted. It is the Responses-API equivalent of the
// chat-completions toolState in provider.go.
@@ -726,9 +745,6 @@ func (p *CodexProvider) handleOutputItemAdded(
builder.Name = event.Item.Name
}
builder.Freeform = builder.Freeform || event.Item.Type == "custom_tool_call"
if builder.Freeform && event.Item.Input != "" && builder.Arguments.Len() == 0 {
builder.Arguments.WriteString(event.Item.Input)
}
state.emitted = true
if !builder.started {
providerio.SendEvent(ctx, events, zeroruntime.StreamEvent{
@@ -740,6 +756,11 @@ func (p *CodexProvider) handleOutputItemAdded(
})
builder.started = true
}
// A custom tool call may arrive with its whole input on the added item
// instead of input deltas; the runtime needs it as a delta either way.
if builder.Freeform && builder.Arguments.Len() == 0 {
builder.forwardInput(ctx, events, event.Item.Input)
}
}
}
@@ -773,13 +794,8 @@ func (p *CodexProvider) handleToolInputDelta(
if event.CallID != "" && builder.CallID == "" {
builder.CallID = event.CallID
}
builder.Arguments.WriteString(event.Delta)
state.emitted = true
providerio.SendEvent(ctx, events, zeroruntime.StreamEvent{
Type: zeroruntime.StreamEventToolCallDelta,
ToolCallID: key,
ArgumentsFragment: event.Delta,
})
builder.forwardInput(ctx, events, event.Delta)
}
// handleOutputItemDone finalizes a tool call when the item type is
@@ -813,13 +829,6 @@ func (p *CodexProvider) handleOutputItemDone(
builder.CallID = event.Item.CallID
}
builder.Freeform = builder.Freeform || event.Item.Type == "custom_tool_call"
finalInput := event.Item.Arguments
if builder.Freeform {
finalInput = event.Item.Input
}
if finalInput != "" && builder.Arguments.Len() == 0 {
builder.Arguments.WriteString(finalInput)
}
if !builder.started {
providerio.SendEvent(ctx, events, zeroruntime.StreamEvent{
Type: zeroruntime.StreamEventToolCallStart,
@@ -830,6 +839,16 @@ func (p *CodexProvider) handleOutputItemDone(
})
builder.started = true
}
// The done item repeats the complete input. When nothing reached the
// runtime through deltas (or the added item), this is the only copy, so
// forward it before closing the call; otherwise the streamed copy stands.
if builder.Arguments.Len() == 0 {
finalInput := event.Item.Arguments
if builder.Freeform {
finalInput = event.Item.Input
}
builder.forwardInput(ctx, events, finalInput)
}
providerio.SendEvent(ctx, events, zeroruntime.StreamEvent{
Type: zeroruntime.StreamEventToolCallEnd,
ToolCallID: key,
+139
View File
@@ -903,6 +903,145 @@ func TestCodexProviderParsesResponsesToolCalls(t *testing.T) {
}
}
// The runtime only sees tool input through tool-call deltas. The Codex
// backend may deliver that input incrementally, whole on the done item, or
// (for custom tool calls) whole on the added item; whichever way it arrives,
// the deltas before tool-call-end must carry it exactly once, and the replayed
// history item must hold the same input.
func TestCodexOutputItemDoneForwardsUnstreamedArguments(t *testing.T) {
t.Parallel()
const (
functionArguments = `{"path":"README.md"}`
customInput = "*** Begin Patch\n*** Add File: hello.txt\n+hello\n*** End Patch\n"
)
cases := []struct {
name string
freeform bool
added bool // response.output_item.added precedes the input
addedInput bool // the added item already carries the whole input
streamed bool // input arrives through *_delta events
}{
{name: "function/added/streamed", added: true, streamed: true},
{name: "function/added/final-only", added: true},
{name: "function/no-added/streamed", streamed: true},
{name: "function/no-added/final-only"},
{name: "custom/added/streamed", freeform: true, added: true, streamed: true},
{name: "custom/added/final-only", freeform: true, added: true},
{name: "custom/no-added/streamed", freeform: true, streamed: true},
{name: "custom/no-added/final-only", freeform: true},
{name: "custom/input-on-added", freeform: true, added: true, addedInput: true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
state := newResponsesState()
events := make(chan zeroruntime.StreamEvent, 10)
provider := &CodexProvider{}
item := &itemPayload{Type: "function_call", ID: "item-1", CallID: "call-1", Name: "read_file", Arguments: functionArguments}
want := functionArguments
if tc.freeform {
item.Type = "custom_tool_call"
item.Name = "apply_patch"
item.Arguments = ""
item.Input = customInput
want = customInput
}
ctx := context.Background()
if tc.added {
initial := *item
if !tc.addedInput {
initial.Arguments = ""
initial.Input = ""
}
provider.handleOutputItemAdded(ctx, &responsesEvent{Item: &initial}, state, events)
}
if tc.streamed {
provider.handleToolInputDelta(ctx, &responsesEvent{ItemID: item.ID, Delta: want}, state, events, tc.freeform)
}
provider.handleOutputItemDone(ctx, &responsesEvent{Item: item}, state, events)
close(events)
var arguments strings.Builder
var starts, ends int
for event := range events {
switch event.Type {
case zeroruntime.StreamEventToolCallStart:
starts++
case zeroruntime.StreamEventToolCallDelta:
arguments.WriteString(event.ArgumentsFragment)
case zeroruntime.StreamEventToolCallEnd:
ends++
if arguments.String() != want {
t.Errorf("arguments at tool-call-end = %q, want %q", arguments.String(), want)
}
}
if event.ToolCallID != item.ID {
t.Errorf("tool call ID = %q, want %q", event.ToolCallID, item.ID)
}
}
if starts != 1 || ends != 1 {
t.Errorf("starts=%d ends=%d, want 1 each", starts, ends)
}
got := state.outputInputItems()
if len(got) != 1 {
t.Fatalf("output items = %v, want one tool call", got)
}
replayed := got[0].Arguments
if tc.freeform {
replayed = got[0].Input
}
if replayed != want {
t.Errorf("replayed history input = %q, want %q", replayed, want)
}
})
}
}
// The same cases through the runtime's collector, which is what the agent
// loop consumes: a function call whose arguments exist only on the done item
// and a custom tool call whose input exists only on the added item both reach
// the runtime with their input intact.
func TestCodexStreamCollectsToolInputDeliveredWhole(t *testing.T) {
t.Parallel()
var rec codexRequest
srv := newCodexResponsesServer(t, &rec,
`{"type":"response.output_item.added","output_index":0,"item":{"type":"function_call","id":"fc-1","call_id":"call-1","name":"read_file"}}`,
`{"type":"response.output_item.done","output_index":0,"item":{"type":"function_call","id":"fc-1","call_id":"call-1","name":"read_file","arguments":"{\"path\":\"README.md\"}"}}`,
`{"type":"response.output_item.added","output_index":1,"item":{"type":"custom_tool_call","id":"ct-1","call_id":"call-2","name":"apply_patch","input":"*** Begin Patch\n*** End Patch\n"}}`,
`{"type":"response.output_item.done","output_index":1,"item":{"type":"custom_tool_call","id":"ct-1","call_id":"call-2","name":"apply_patch","input":"*** Begin Patch\n*** End Patch\n"}}`,
`{"type":"response.completed","response":{"id":"resp-1","status":"completed","usage":{"input_tokens":3,"output_tokens":2}}}`,
)
defer srv.Close()
provider, err := NewCodexProvider(CodexOptions{
Options: Options{APIKey: "sk-test", BaseURL: srv.URL, Model: "gpt-5"},
AccountID: "acc-x",
})
if err != nil {
t.Fatalf("NewCodexProvider: %v", err)
}
stream, err := provider.StreamCompletion(context.Background(), zeroruntime.CompletionRequest{
Messages: []zeroruntime.Message{{Role: zeroruntime.MessageRoleUser, Content: "read the readme"}},
Tools: []zeroruntime.ToolDefinition{
{Name: "read_file", Parameters: map[string]any{"type": "object"}},
{Name: "apply_patch", Type: zeroruntime.ToolDefinitionFreeform},
},
})
if err != nil {
t.Fatalf("StreamCompletion: %v", err)
}
collected := zeroruntime.CollectStream(context.Background(), stream)
if collected.Error != "" {
t.Fatalf("unexpected stream error: %q", collected.Error)
}
if len(collected.ToolCalls) != 2 {
t.Fatalf("tool calls = %+v, want read_file and apply_patch", collected.ToolCalls)
}
if call := collected.ToolCalls[0]; call.Name != "read_file" || call.Arguments != `{"path":"README.md"}` || call.Freeform {
t.Errorf("function call = %+v, want read_file with the done item's arguments", call)
}
if call := collected.ToolCalls[1]; call.Name != "apply_patch" || call.Arguments != "*** Begin Patch\n*** End Patch\n" || !call.Freeform {
t.Errorf("custom call = %+v, want apply_patch with the added item's input", call)
}
}
func TestCodexProviderSendsAssistantToolCallsAsInputItems(t *testing.T) {
t.Parallel()
// When the runtime replays a prior assistant turn that issued a tool
+332 -28
View File
@@ -5,20 +5,63 @@ import (
"fmt"
"io"
"os"
"path/filepath"
"strings"
"syscall"
"github.com/Twigpine/zero/internal/pathjail"
)
var errFileChangedDuringWrite = errors.New("file changed on disk before the write committed")
// errFilePublishedWithCleanupFailure marks an error after publication. Callers
// must record the committed bytes while still reporting the cleanup failure.
var errFilePublishedWithCleanupFailure = errors.New("file published, but staging cleanup failed")
// fileWriteBeforeCommit is a deterministic test hook. Production leaves it
// nil; tests use it to replace a path after observation but before opening the
// object that will actually be mutated.
var fileWriteBeforeCommit func(path string)
// fileWriteBeforePublish is a deterministic test hook that runs after the
// replacement bytes have been staged and before the destination's identity is
// re-checked for the publish. Tests use it to install a competing writer at the
// last point before publication.
var fileWriteBeforePublish func(path string)
// fileWriteStat is a deterministic test seam for proving that the opened-file
// identity is captured before the final preimage comparison. Production uses
// the file descriptor directly.
var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat() }
// fileWriteTempWrite is a deterministic test seam for a failure while the
// replacement bytes are written to the side file, after the observed file's
// identity and preimage have been accepted. Production writes through
// io.WriteString. The failure it stands in for (a full disk, a failing
// device) is exactly the moment a truncate-then-write has already destroyed
// the observed file.
var fileWriteTempWrite = func(file *os.File, content string) error {
_, err := io.WriteString(file, content)
return err
}
// fileWriteRename publishes the staged side file over the destination.
// Tests replace it to exercise refused atomic publication deterministically.
var fileWriteRename = func(root *os.Root, oldName, newName string) error {
return root.Rename(oldName, newName)
}
// fileWriteNoReplaceRename publishes a create without replacing an existing
// name. Tests replace it to exercise the hard-link fallback that filesystems
// without a no-replace rename use.
var fileWriteNoReplaceRename = publishRootedNoReplace
// fileWriteRemove removes a staging name. Tests inject cleanup failures after
// publication to distinguish a committed create from a failed write.
var fileWriteRemove = func(root *os.Root, name string) error {
return root.Remove(name)
}
// commitRootedFileContents binds an overwrite to the file identity and bytes
// that the caller observed, opening every component through an already-open
// *os.Root descriptor rather than re-resolving the absolute path. Because the
@@ -26,11 +69,26 @@ var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat(
// swapped for a symlink that escapes the workspace between validation and this
// call is refused by the kernel instead of redirecting the write.
//
// A create uses exclusive creation THROUGH THE ROOT. An overwrite opens the
// observed object without truncation, verifies identity/content through that
// handle, then truncates and writes the same handle. relativePath is the target
// expressed relative to root; absolutePath is retained only for the test hook
// and error reporting.
// Both publishes are atomic against a concurrent reader: the replacement bytes
// are written whole to a side file in the target's directory and the target is
// swapped wholesale. A create publishes through a kernel no-replace rename, so
// losing the create race refuses instead of replacing the raced file. An
// overwrite publishes through a rename after re-checking that the destination
// still names the observed object; that check is the last operation before the
// rename, but no supported platform offers a compare-and-swap rename, so a
// writer that replaces the name between the check and the syscall can still be
// replaced. A failure at any point before the publish removes the side file
// and leaves the observed bytes standing — the caller's file outlives a write
// that could not complete, and no partial state is ever visible at the path
// itself.
//
// A final-component symlink keeps main's behaviour: the link stays a link and
// the resolved file is what is replaced. A hard link is detached by the rename
// instead of rewritten through the shared inode. If the platform refuses the
// rename, the overwrite fails and the observed file remains intact.
//
// relativePath is the target expressed relative to root; absolutePath is
// retained only for the test hooks and error reporting.
//
// expectedInfo nil means the caller observed a missing path. expectedContent
// may be nil for an existing but unreadable file; that path may still be
@@ -41,16 +99,7 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string,
}
if expectedInfo == nil {
file, err := root.OpenFile(relativePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
if err != nil {
return err
}
openedInfo, err := fileWriteStat(file)
if err != nil {
_ = file.Close()
return err
}
return writeAndVerifyRootedFileIdentity(root, relativePath, file, openedInfo, content, false)
return commitRootedFileCreate(root, relativePath, content)
}
flags := os.O_WRONLY
@@ -86,24 +135,239 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string,
return errFileChangedDuringWrite
}
}
return writeAndVerifyRootedFileIdentity(root, relativePath, file, openedInfo, content, true)
}
func writeAndVerifyRootedFileIdentity(root *os.Root, relativePath string, file *os.File, openedInfo os.FileInfo, content string, truncate bool) error {
if truncate {
if err := file.Truncate(0); err != nil {
_ = file.Close()
return err
}
if _, err := file.Seek(0, io.SeekStart); err != nil {
_ = file.Close()
return err
}
// A device, FIFO or socket is not a file this path should replace with a
// regular one, and the previous in-place write is the only publish that
// keeps its semantics. Regular files (including symlink targets) go through
// staging and rename.
if !openedInfo.Mode().IsRegular() {
return writeRootedFileInPlace(root, relativePath, file, openedInfo, expectedContent, content)
}
if _, err := io.WriteString(file, content); err != nil {
mode := openedInfo.Mode().Perm()
publishPath, err := resolveRootedFinalPath(root, relativePath)
if err != nil {
_ = file.Close()
return err
}
if publishPath != relativePath {
targetInfo, statErr := root.Stat(publishPath)
if statErr != nil || !os.SameFile(openedInfo, targetInfo) {
_ = file.Close()
return fmt.Errorf("%w: link target changed", errFileChangedDuringWrite)
}
}
if err := file.Close(); err != nil {
return err
}
return commitRootedFileOverwrite(root, absolutePath, relativePath, publishPath, openedInfo, mode, content)
}
// commitRootedFileCreate publishes a brand-new file whose absence the caller
// observed. The bytes are written whole to a private side file beside the
// target and handed to the name with a kernel no-replace rename: the create is
// either absent or the whole file, a concurrent create is refused rather than
// replaced, and a write that cannot complete leaves no file at all.
func commitRootedFileCreate(root *os.Root, relativePath, content string) error {
tempName, err := writeRootedSideFile(root, relativePath, 0o644, true, content)
if err != nil {
return err
}
if err := publishRootedCreate(root, tempName, relativePath); err != nil {
if errors.Is(err, errFilePublishedWithCleanupFailure) {
return err
}
return errors.Join(err, removeWriteSideFile(root, tempName))
}
return nil
}
// publishRootedCreate hands the staged side file to a name that must not
// already exist. RenameNoReplace is the primary primitive; a hard link is the
// no-replace fallback for filesystems whose rename cannot promise it. Neither
// fallback copies into a visible target, so a reader sees only the absent name
// or the complete file.
func publishRootedCreate(root *os.Root, tempName, relativePath string) error {
renameErr := fileWriteNoReplaceRename(root, tempName, relativePath)
if renameErr == nil {
return nil
}
if errors.Is(renameErr, os.ErrExist) {
return fmt.Errorf("creating %s: %w", relativePath, renameErr)
}
if !renameNoReplaceUnsupported(renameErr) {
return fmt.Errorf("creating %s: %w", relativePath, renameErr)
}
if linkErr := root.Link(tempName, relativePath); linkErr != nil {
return fmt.Errorf("creating %s: %w", relativePath, errors.Join(renameErr, linkErr))
}
// The staging name now shares the published inode. Remove only the name,
// without chmod-based retries that could change the published file's mode.
if cleanupErr := fileWriteRemove(root, tempName); cleanupErr != nil && !errors.Is(cleanupErr, os.ErrNotExist) {
return fmt.Errorf("creating %s: %w: %w", relativePath, errFilePublishedWithCleanupFailure, cleanupErr)
}
return nil
}
// publishRootedNoReplace performs the no-replace rename through a root handle
// bound to the target's own directory. Binding the directory first is what
// keeps the raw rename inside the workspace: a parent link swapped after the
// caller resolved the path is refused when the directory is opened, and both
// names are single components below that handle.
func publishRootedNoReplace(root *os.Root, tempName, relativePath string) error {
parent := filepath.Dir(relativePath)
if filepath.Dir(tempName) != parent {
return fmt.Errorf("staging file %s is not beside %s", tempName, relativePath)
}
directory, err := root.OpenRoot(parent)
if err != nil {
return err
}
defer directory.Close()
return pathjail.RenameNoReplace(directory, filepath.Base(tempName), filepath.Base(relativePath))
}
// renameNoReplaceUnsupported reports whether the platform or filesystem told
// us only that it cannot perform a no-replace rename, so the no-replace hard
// link is the right fallback. A collision is not an unsupported operation and
// is handled by the caller before this is consulted.
func renameNoReplaceUnsupported(err error) bool {
return errors.Is(err, errors.ErrUnsupported) || errors.Is(err, syscall.EINVAL)
}
// commitRootedFileOverwrite publishes a replacement for an object whose
// identity and preimage just passed validation: the bytes are written to a
// side file beside the target, the observed identity is re-checked against the
// path as late as the rename allows, and a rename hands the path over in one
// step. A failure while the side file is being written — ENOSPC, a failing
// device — therefore costs a removed side file, not the observed file.
//
// Refused renames fail without mutating the observed inode. Writing in place
// here would expose partial content and rewrite hard-link siblings, defeating
// the guarantees that staging provides.
func commitRootedFileOverwrite(root *os.Root, absolutePath, relativePath, publishPath string, openedInfo os.FileInfo, mode os.FileMode, content string) error {
tempName, err := writeRootedSideFile(root, publishPath, mode, false, content)
if err != nil {
return err
}
if fileWriteBeforePublish != nil {
fileWriteBeforePublish(absolutePath)
}
if err := verifyRootedPublishTarget(root, publishPath, openedInfo); err != nil {
return errors.Join(err, removeWriteSideFile(root, tempName))
}
if err := fileWriteRename(root, tempName, publishPath); err != nil {
renameErr := fmt.Errorf("writing %s: atomic replacement refused: %w", relativePath, err)
return errors.Join(renameErr, removeWriteSideFile(root, tempName))
}
return nil
}
// verifyRootedPublishTarget refuses to replace anything but the exact object
// whose identity and preimage were validated. It is deliberately the last
// operation before the rename; no portable rename offers a compare-and-swap,
// so this check is the strongest condition available before an atomic replace.
func verifyRootedPublishTarget(root *os.Root, publishPath string, openedInfo os.FileInfo) error {
info, err := root.Lstat(publishPath)
if err != nil || info.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 || !os.SameFile(openedInfo, info) {
return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite)
}
return nil
}
// resolveRootedFinalPath follows final-component symlinks inside the root and
// returns the path of the file the caller's link resolves to. Publishing over
// that path preserves the link, unlike a rename over the link itself.
func resolveRootedFinalPath(root *os.Root, relativePath string) (string, error) {
current := relativePath
for links := 0; links < 255; links++ {
info, err := root.Lstat(current)
if err != nil {
return "", err
}
if info.Mode()&os.ModeSymlink == 0 {
return current, nil
}
target, err := root.Readlink(current)
if err != nil {
return "", err
}
if filepath.IsAbs(target) || filepath.VolumeName(target) != "" {
return "", fmt.Errorf("%w: %s is an absolute link", errFileChangedDuringWrite, current)
}
next := filepath.Clean(filepath.Join(filepath.Dir(current), target))
if next == ".." || strings.HasPrefix(next, ".."+string(filepath.Separator)) {
return "", fmt.Errorf("%w: %s points outside the workspace", errFileChangedDuringWrite, current)
}
current = next
}
return "", fmt.Errorf("%w: more than 255 links at %s", errFileChangedDuringWrite, relativePath)
}
// writeRootedSideFile writes content to a fresh side file beside the target,
// relative to the root handle, and leaves it closed and ready to publish. The
// caller owns removing the side file when the publish that follows does not
// commit.
//
// umaskDerived selects the permission contract. A create asks the kernel for
// 0o644 and remembers what the process umask actually granted; the bytes are
// made owner-only while they are staged, then the granted bits are restored.
// An overwrite stages 0o600 and finally carries the observed file's permission
// bits over, so the published file looks like the one it replaced.
func writeRootedSideFile(root *os.Root, relativePath string, mode os.FileMode, umaskDerived bool, content string) (string, error) {
createMode := mode.Perm()
if !umaskDerived {
createMode = 0o600
}
temp, tempName, err := pathjail.CreateTempMode(root, filepath.Dir(relativePath), "zero-write", ".tmp", createMode)
if err != nil {
return "", fmt.Errorf("writing %s: %w", relativePath, err)
}
granted := mode.Perm()
if umaskDerived {
info, statErr := temp.Stat()
if statErr != nil {
_ = temp.Close()
_ = removeWriteSideFile(root, tempName)
return "", fmt.Errorf("writing %s: %w", relativePath, statErr)
}
granted = info.Mode().Perm()
if chmodErr := temp.Chmod(0o600); chmodErr != nil {
_ = temp.Close()
_ = removeWriteSideFile(root, tempName)
return "", fmt.Errorf("writing %s: %w", relativePath, chmodErr)
}
}
writeErr := func() error {
if err := fileWriteTempWrite(temp, content); err != nil {
return err
}
return temp.Chmod(granted.Perm())
}()
closeErr := temp.Close()
if writeErr != nil || closeErr != nil {
_ = removeWriteSideFile(root, tempName)
return "", fmt.Errorf("writing %s: %w", relativePath, errors.Join(writeErr, closeErr))
}
return tempName, nil
}
// writeRootedFileInPlace publishes through an already-validated handle. It is
// the non-regular path, so it keeps the previous truncate-and-write behaviour
// and restores the observed bytes if a write fails and the preimage is available.
func writeRootedFileInPlace(root *os.Root, relativePath string, file *os.File, openedInfo os.FileInfo, expectedContent *string, content string) error {
if err := file.Truncate(0); err != nil {
_ = file.Close()
return err
}
if _, err := file.Seek(0, io.SeekStart); err != nil {
_ = file.Close()
return err
}
if _, err := io.WriteString(file, content); err != nil {
restoreErr := restoreRootedFile(file, expectedContent)
closeErr := file.Close()
return errors.Join(err, restoreErr, closeErr)
}
if err := file.Close(); err != nil {
return err
}
@@ -113,3 +377,43 @@ func writeAndVerifyRootedFileIdentity(root *os.Root, relativePath string, file *
}
return nil
}
// restoreRootedFile puts the observed bytes back after a failed in-place
// write. The write already truncated the file, so this is best effort: the
// caller receives both the original failure and any restore failure.
func restoreRootedFile(file *os.File, preimage *string) error {
if preimage == nil {
return nil
}
if err := file.Truncate(0); err != nil {
return fmt.Errorf("restoring the preimage: %w", err)
}
if _, err := file.Seek(0, io.SeekStart); err != nil {
return fmt.Errorf("restoring the preimage: %w", err)
}
if _, err := io.WriteString(file, *preimage); err != nil {
return fmt.Errorf("restoring the preimage: %w", err)
}
return nil
}
// removeWriteSideFile removes a staged replacement that never reached (or did
// not survive) its publish, through the root handle.
func removeWriteSideFile(root *os.Root, name string) error {
err := fileWriteRemove(root, name)
if err == nil || errors.Is(err, os.ErrNotExist) {
return nil
}
// Windows refuses to delete a read-only file. The staged file only carries
// that bit after its published mode was applied; clear it through the root
// and retry. The name belongs to a random side file this commit created.
if errors.Is(err, os.ErrPermission) {
if chmodErr := root.Chmod(name, 0o600); chmodErr == nil {
err = fileWriteRemove(root, name)
if err == nil || errors.Is(err, os.ErrNotExist) {
return nil
}
}
}
return err
}
+369
View File
@@ -0,0 +1,369 @@
package tools
import (
"context"
"errors"
"io"
"os"
"path/filepath"
"runtime"
"strings"
"testing"
)
func installFileWriteRename(t *testing.T, rename func(*os.Root, string, string) error) {
t.Helper()
prior := fileWriteRename
fileWriteRename = rename
t.Cleanup(func() { fileWriteRename = prior })
}
func installFileWriteBeforePublish(t *testing.T, hook func(string)) {
t.Helper()
prior := fileWriteBeforePublish
fileWriteBeforePublish = hook
t.Cleanup(func() { fileWriteBeforePublish = prior })
}
func installFileWriteNoReplaceRename(t *testing.T, rename func(*os.Root, string, string) error) {
t.Helper()
prior := fileWriteNoReplaceRename
fileWriteNoReplaceRename = rename
t.Cleanup(func() { fileWriteNoReplaceRename = prior })
}
// A FINAL-COMPONENT SYMLINK STAYS A LINK. Main opened the target through the
// link and wrote through it, so replacing the link with a regular file leaves
// the link's target stuck on its old bytes. Publishing over the resolved path
// preserves both halves: the link is still a link, and the file it names has
// the new content.
//
// The public tools reject a symlink during path validation today, so this is
// asserted at the commit layer, which is where the publish semantics live and
// where a scoped write that resolves through an extra root reaches them.
func TestOverwriteKeepsAnInWorkspaceSymlink(t *testing.T) {
for _, tc := range []struct {
name string
chain bool
}{
{name: "direct link"},
{name: "link chain", chain: true},
} {
t.Run(tc.name, func(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "target.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
link := filepath.Join(root, "link.txt")
if err := os.Symlink("target.txt", link); err != nil {
t.Skipf("symlinks unavailable: %v", err)
}
publishPath, links := "link.txt", []string{link}
if tc.chain {
chained := filepath.Join(root, "chained.txt")
if err := os.Symlink("link.txt", chained); err != nil {
t.Fatalf("create second symlink: %v", err)
}
publishPath, links = "chained.txt", append(links, chained)
}
workspace, err := os.OpenRoot(root)
if err != nil {
t.Fatal(err)
}
defer workspace.Close()
expectedInfo, err := workspace.Stat(publishPath)
if err != nil {
t.Fatal(err)
}
preimage := "observed\n"
if err := commitRootedFileContents(workspace, filepath.Join(root, publishPath), publishPath, expectedInfo, &preimage, "zero\n"); err != nil {
t.Fatalf("symlink overwrite: %v", err)
}
for _, path := range links {
info, err := os.Lstat(path)
if err != nil {
t.Fatalf("inspect %s: %v", path, err)
}
if info.Mode()&os.ModeSymlink == 0 {
t.Fatalf("%s is no longer a symlink: mode=%v", path, info.Mode())
}
}
if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" {
t.Fatalf("link target = %q, err=%v; the link must keep pointing at the rewritten file", got, err)
}
entries, err := os.ReadDir(root)
if err != nil {
t.Fatal(err)
}
for _, entry := range entries {
if strings.HasSuffix(entry.Name(), ".tmp") {
t.Fatalf("staging file %s survived a successful publish", entry.Name())
}
}
})
}
}
// An open reader keeps the observed inode. Platforms that cannot replace the
// name while it is open must fail without rewriting that reader's bytes.
func TestOverwriteWithAnOpenReaderPreservesAtomicPublication(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
reader, err := os.Open(target)
if err != nil {
t.Fatal(err)
}
defer reader.Close()
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "content": "zero\n", "overwrite": true,
})
want := "zero\n"
if runtime.GOOS == "windows" && result.Status == StatusError && strings.Contains(result.Output, "atomic replacement refused") {
want = "observed\n"
} else if result.Status != StatusOK {
t.Fatalf("overwrite with an open reader = %s: %s", result.Status, result.Output)
}
if got, err := os.ReadFile(target); err != nil || string(got) != want {
t.Fatalf("target = %q, err=%v", got, err)
}
if got, err := io.ReadAll(reader); err != nil || string(got) != "observed\n" {
t.Fatalf("open reader = %q, err=%v; publication must preserve the observed inode", got, err)
}
}
// A refused atomic replacement must leave both names of the observed inode
// untouched. Exercise both production callers and their session baselines.
func TestOverwriteRefusedReplacementPreservesThePreimageAndHardLink(t *testing.T) {
for _, name := range []string{"write", "edit"} {
t.Run(name, func(t *testing.T) {
root := t.TempDir()
root, err := filepath.EvalSymlinks(root)
if err != nil {
t.Fatal(err)
}
target := filepath.Join(root, "existing.txt")
sibling := filepath.Join(root, "sibling.txt")
preimage := []byte("observed\n")
if err := os.WriteFile(target, preimage, 0o644); err != nil {
t.Fatal(err)
}
if err := os.Link(target, sibling); err != nil {
t.Skipf("hard links unavailable: %v", err)
}
info, err := os.Stat(target)
if err != nil {
t.Fatal(err)
}
tracker := NewFileTracker()
tracker.Record(target, preimage, info)
tracker.RecordSeenRange(target, 1, 1, 1)
installFileWriteRename(t, func(*os.Root, string, string) error {
return errors.New("replacement refused")
})
tool := NewScopedWriteFileTool(root, nil)
args := map[string]any{"path": "existing.txt", "content": "zero\n", "overwrite": true}
if name == "edit" {
tool = NewScopedEditFileTool(root, nil)
args = map[string]any{"path": "existing.txt", "old_string": "observed", "new_string": "zero"}
}
registry := NewRegistry()
registry.Register(tool)
result := registry.RunWithOptions(context.Background(), tool.Name(), args, RunOptions{FileTracker: tracker, PermissionGranted: true})
if result.Status != StatusError || !strings.Contains(result.Output, "replacement refused") {
t.Fatalf("refused overwrite = %s: %s", result.Status, result.Output)
}
for _, path := range []string{target, sibling} {
got, err := os.ReadFile(path)
if err != nil || string(got) != string(preimage) {
t.Fatalf("%s = %q, err=%v; a refused publish must preserve the shared preimage", path, got, err)
}
}
if err := tracker.CheckConflict(target, preimage); err != nil {
t.Fatalf("tracker changed after refused publication: %v", err)
}
entries, err := os.ReadDir(root)
if err != nil || len(entries) != 2 {
t.Fatalf("refused publish left staging files: %v, err=%v", entries, err)
}
})
}
}
// A competing writer that replaces the path during a refused rename keeps its
// own file. No in-place write may follow the failed publication.
func TestOverwriteRefusedRenameLeavesACompetingReplacementUntouched(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
installFileWriteRename(t, func(*os.Root, string, string) error {
preserved := target + ".preserved"
if err := os.Rename(target, preserved); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
return errors.New("replacement refused")
})
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "content": "zero\n", "overwrite": true,
})
if result.Status != StatusError || !strings.Contains(result.Output, "replacement refused") {
t.Fatalf("replaced target = %s: %s", result.Status, result.Output)
}
if got, err := os.ReadFile(target); err != nil || string(got) != "observed\n" {
t.Fatalf("replacement = %q, err=%v; a refused rename must leave the competing file untouched", got, err)
}
}
// A FILESYSTEM MAY NOT OFFER A NO-REPLACE RENAME. The hard-link fallback is
// still atomic against readers — a reader sees the absent name or the complete
// inode — and refuses a name another writer created.
func TestCreateFallsBackToAHardLinkWhenNoReplaceRenameIsUnsupported(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "created.txt")
installFileWriteNoReplaceRename(t, func(*os.Root, string, string) error {
return errors.ErrUnsupported
})
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "created.txt", "content": "zero\n",
})
if result.Status != StatusOK {
t.Fatalf("hard-link create = %s: %s", result.Status, result.Output)
}
if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" {
t.Fatalf("created file = %q, err=%v", got, err)
}
entries, err := os.ReadDir(root)
if err != nil {
t.Fatal(err)
}
if len(entries) != 1 || entries[0].Name() != "created.txt" {
t.Fatalf("root holds %d entries; the staging name must not survive the publish", len(entries))
}
}
func TestCreateHardLinkFallbackStillRefusesAnExistingName(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("other writer\n"), 0o644); err != nil {
t.Fatal(err)
}
installFileWriteNoReplaceRename(t, func(*os.Root, string, string) error {
return errors.ErrUnsupported
})
workspace, err := os.OpenRoot(root)
if err != nil {
t.Fatal(err)
}
defer workspace.Close()
if err := commitRootedFileContents(workspace, target, "existing.txt", nil, nil, "zero\n"); !errors.Is(err, os.ErrExist) {
t.Fatalf("hard-link create over an existing name = %v, want os.ErrExist", err)
}
if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" {
t.Fatalf("existing file = %q, err=%v; the fallback must not replace it", got, err)
}
entries, err := os.ReadDir(root)
if err != nil {
t.Fatal(err)
}
if len(entries) != 1 || entries[0].Name() != "existing.txt" {
t.Fatalf("root holds %d entries; a refused create must remove its staging file", len(entries))
}
}
// A REPLACEMENT INSTALLED AFTER THE BYTES WERE STAGED must not be silently
// overwritten by the publish. The identity check immediately before the rename
// is what keeps the conflict guarantee for the window that remains.
func TestOverwriteRefusesAReplacementInstalledBeforePublish(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
installFileWriteBeforePublish(t, func(path string) {
preserved := path + ".preserved"
if err := os.Rename(path, preserved); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(path, []byte("competing\n"), 0o644); err != nil {
t.Fatal(err)
}
})
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "content": "zero\n", "overwrite": true,
})
if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) {
t.Fatalf("pre-publish replacement = %s: %s", result.Status, result.Output)
}
if got, err := os.ReadFile(target); err != nil || string(got) != "competing\n" {
t.Fatalf("competing file = %q, err=%v; the publish must refuse a replacement it did not observe", got, err)
}
}
// Publication and cleanup are separate outcomes. Even when cleanup fails, the
// session must know that the new file exists and contains the requested bytes.
func TestCreateCleanupFailureRecordsThePublishedFile(t *testing.T) {
for _, cleanupErr := range []error{errors.New("injected staging cleanup failure"), os.ErrPermission} {
t.Run(cleanupErr.Error(), func(t *testing.T) {
root := t.TempDir()
root, err := filepath.EvalSymlinks(root)
if err != nil {
t.Fatal(err)
}
target := filepath.Join(root, "created.txt")
var publishedMode os.FileMode
installFileWriteNoReplaceRename(t, func(root *os.Root, oldName, _ string) error {
info, err := root.Stat(oldName)
if err != nil {
t.Fatal(err)
}
publishedMode = info.Mode().Perm()
return errors.ErrUnsupported
})
prior := fileWriteRemove
fileWriteRemove = func(*os.Root, string) error { return cleanupErr }
t.Cleanup(func() { fileWriteRemove = prior })
tracker := NewFileTracker()
registry := NewRegistry()
registry.Register(NewScopedWriteFileTool(root, nil))
result := registry.RunWithOptions(context.Background(), "write_file", map[string]any{
"path": "created.txt", "content": "zero\n",
}, RunOptions{FileTracker: tracker, PermissionGranted: true})
if result.Status != StatusError || !strings.Contains(result.Output, "published") || !strings.Contains(result.Output, cleanupErr.Error()) {
t.Fatalf("published create with cleanup failure = %s: %s", result.Status, result.Output)
}
if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" {
t.Fatalf("published content = %q, err=%v", got, err)
}
if info, err := os.Stat(target); err != nil || info.Mode().Perm() != publishedMode {
t.Fatalf("published mode changed after cleanup failure: %v, err=%v, want %v", info, err, publishedMode)
}
if version, ok := tracker.Version(target); !ok || version.Hash != HashContent([]byte("zero\n")) {
t.Fatalf("published file missing from tracker: %+v, tracked=%v", version, ok)
}
if !tracker.SeenWhole(target) {
t.Fatal("published bytes must be recorded as seen")
}
if created := tracker.CreatedFiles(); len(created) != 1 || created[0] != target {
t.Fatalf("created files = %v, want %s", created, target)
}
if len(result.ChangedFiles) != 1 || result.ChangedFiles[0] != "created.txt" {
t.Fatalf("changed files = %v", result.ChangedFiles)
}
})
}
}
+68
View File
@@ -350,3 +350,71 @@ func TestWriteFileRefusesSameNameSubstitutionWithIdenticalBytes(t *testing.T) {
t.Fatalf("substituted object = %q, err=%v; the write must not land in an object the preimage did not observe", got, err)
}
}
// AN OVERWRITTEN PATH'S NEIGHBOURS THROUGH ANOTHER NAME KEEP THEIR CONTENT.
//
// The replace-overwrite truncates and rewrites the observed file in place, and
// truncation follows the inode: a sibling reached through a hard link loses
// its content when this path's content is replaced, because both names were
// pointing at the same bytes. Publishing through a rename detaches this name
// from the shared inode instead, so the link sibling keeps what it shares.
func TestOverwriteDoesNotTruncateAHardlinkSibling(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "target.txt")
sibling := filepath.Join(root, "sibling.txt")
if err := os.WriteFile(target, []byte("shared\n"), 0o644); err != nil {
t.Fatal(err)
}
if err := os.Link(target, sibling); err != nil {
t.Skipf("hard links unavailable: %v", err)
}
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "target.txt", "content": "replaced\n", "overwrite": true,
})
if result.Status != StatusOK {
t.Fatalf("overwrite status = %s: %s", result.Status, result.Output)
}
shared, err := os.ReadFile(sibling)
if err != nil || string(shared) != "shared\n" {
t.Fatalf("hard-link sibling = %q, err=%v; replacing this name must not rewrite the bytes the sibling shares", shared, err)
}
}
// THE PUBLISHED FILE LOOKS LIKE THE FILE IT REPLACED. The permission bits are
// carried over from the file that was observed, not from the side file the
// bytes were staged in.
func TestOverwriteKeepsTheObservedPermissionBits(t *testing.T) {
// 0o640 differs from the 0o600 the side file is staged with, so a publish
// that forgot to carry the observed bits cannot pass by coincidence.
want := os.FileMode(0o640)
if runtime.GOOS == "windows" {
// A Windows file carries only the read-only bit, so a 0o640 file is
// reported as 0o666; the write bit, not the exact value, is the fact
// being pinned.
want = os.FileMode(0o666)
}
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), want); err != nil {
t.Fatal(err)
}
if err := os.Chmod(target, want); err != nil {
t.Fatal(err)
}
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "content": "zero\n", "overwrite": true,
})
if result.Status != StatusOK {
t.Fatalf("overwrite status = %s: %s", result.Status, result.Output)
}
info, err := os.Stat(target)
if err != nil {
t.Fatal(err)
}
if got := info.Mode().Perm(); got != want {
t.Fatalf("published permission bits = %o, want %o", got, want)
}
}
+56
View File
@@ -0,0 +1,56 @@
//go:build unix
package tools
import (
"context"
"os"
"path/filepath"
"syscall"
"testing"
)
// THE PUBLISHED FILE CARRIES THE PERMISSIONS THE PROCESS UMASK PRODUCES, NOT
// THE RAW MODE AN ASSIGNMENT WOULD SET.
//
// The create path opens its staging file with the same exclusive 0o644 create
// this branch of the write path always honored, and the kernel applies the
// process umask to that open. The publish must carry the granted bits — taken
// from the staging file itself — because a chmod never consults the umask: a
// process that requests tight defaults on new files would find every write it
// publishes shared with the world. Under the restrictive umask a plain mode
// assignment would publish 0o644 where the kernel granted 0o600, so the test
// fails before the granted bits are carried.
func TestCreatePublishesTheUmaskedPermissionsOfTheStagedCreate(t *testing.T) {
for name, testCase := range map[string]struct {
umask int
want os.FileMode
}{
"a restrictive umask keeps every stranger out": {umask: 0o077, want: 0o600},
"a permissive umask keeps the readable bits": {umask: 0o022, want: 0o644},
} {
t.Run(name, func(t *testing.T) {
previous := syscall.Umask(testCase.umask)
defer syscall.Umask(previous)
root := t.TempDir()
target := filepath.Join(root, "created.txt")
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "created.txt", "content": "zero\n",
})
if result.Status != StatusOK {
t.Fatalf("create status = %s: %s", result.Status, result.Output)
}
info, err := os.Stat(target)
if err != nil {
t.Fatal(err)
}
if got := info.Mode().Perm(); got != testCase.want {
t.Fatalf("published mode = %#o, want the %#o the kernel granted the staging create under this umask; a raw mode assignment would publish %#o and share the file with hands the umask excludes", got, testCase.want, os.FileMode(0o644))
}
if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "zero\n" {
t.Fatalf("published content = %q, err=%v", got, readErr)
}
})
}
}
@@ -0,0 +1,128 @@
package tools
import (
"context"
"errors"
"os"
"path/filepath"
"testing"
)
func installFileWriteTempWrite(t *testing.T, write func(*os.File, string) error) {
t.Helper()
prior := fileWriteTempWrite
fileWriteTempWrite = write
t.Cleanup(func() { fileWriteTempWrite = prior })
}
// A WRITE THAT CANNOT COMPLETE MUST NOT TAKE THE FILE IT REPLACED WITH IT.
//
// Running out of disk space part-way through a write used to be fatal to the
// preimage: the overwrite truncated the observed file first, so the failed
// write left an empty husk where a working file stood. The replacement bytes
// are now written whole to a side file, so a failure there costs the side file
// and the observed bytes survive untouched. Each case reopens the target after
// the tool has run and checks the preimage is what still stands, and that no
// side file was left behind.
func TestOverwriteFailureKeepsThePreimageStanding(t *testing.T) {
for name, run := range map[string]func(string) Result{
"write overwrite": func(root string) Result {
return NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "content": "zero\n", "overwrite": true,
})
},
"edit": func(root string) Result {
return NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "existing.txt", "old_string": "observed", "new_string": "zero",
})
},
} {
t.Run(name, func(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
// The write that stands in for ENOSPC fails after the file was
// observed, accepted and staged — the exact point the old
// truncate-then-write had nothing left to lose.
writeErr := errors.New("injected unrecoverable write failure")
installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr })
result := run(root)
if result.Status != StatusError {
t.Fatalf("failed-write overwrite = %s, want error: %s", result.Status, result.Output)
}
got, err := os.ReadFile(target)
if err != nil || string(got) != "observed\n" {
t.Fatalf("preimage = %q, err=%v; the observed file must outlive a write that failed", got, err)
}
entries, err := os.ReadDir(root)
if err != nil {
t.Fatal(err)
}
if len(entries) != 1 || entries[0].Name() != "existing.txt" {
t.Fatalf("root holds %d entries; a failed write must remove its side file", len(entries))
}
})
}
}
// A NEW FILE WHOSE WRITE FAILS MUST NOT APPEAR AT ALL. An exclusive create used
// to leave whatever the write had managed to write behind, half-written and
// complete-looking to anything that did not read past the visible length.
func TestCreateFailureCreatesNothing(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "created.txt")
writeErr := errors.New("injected unrecoverable write failure")
installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr })
result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "created.txt", "content": "zero\n",
})
if result.Status != StatusError {
t.Fatalf("failed-write create = %s, want error: %s", result.Status, result.Output)
}
if _, statErr := os.Stat(target); !os.IsNotExist(statErr) {
t.Fatalf("target = %v; a create whose bytes never made it must leave no file", statErr)
}
entries, err := os.ReadDir(root)
if err != nil {
t.Fatal(err)
}
if len(entries) != 0 {
t.Fatalf("root holds %d entries; a failed create must remove its side file", len(entries))
}
}
// AT THE LAYER THE COMMIT RUNS, the injected failure is reported as the
// failure it injected and nothing else, so a caller can distinguish a failed
// write from a refused write.
func TestCommitRootedOverwriteFailedWriteIsTheWriteError(t *testing.T) {
root := t.TempDir()
target := filepath.Join(root, "existing.txt")
if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil {
t.Fatal(err)
}
workspace, err := os.OpenRoot(root)
if err != nil {
t.Fatal(err)
}
defer workspace.Close()
info, err := os.Stat(target)
if err != nil {
t.Fatal(err)
}
preimage := "observed\n"
writeErr := errors.New("injected unrecoverable write failure")
installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr })
err = commitRootedFileContents(workspace, filepath.Join(root, "existing.txt"), "existing.txt", info, &preimage, "zero\n")
if !errors.Is(err, writeErr) {
t.Fatalf("commit error = %v, want the injected write failure", err)
}
got, readErr := os.ReadFile(target)
if readErr != nil || string(got) != "observed\n" {
t.Fatalf("preimage = %q, err=%v; the observed file must outlive a write that failed", got, readErr)
}
}
+10 -2
View File
@@ -2,6 +2,7 @@ package tools
import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
@@ -146,8 +147,9 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
if priorContentKnown {
expectedContent = &priorContent
}
if err := commitRootedFileContents(root, absolutePath, rootedRelative, priorInfo, expectedContent, content); err != nil {
return errorResult("Error writing file " + relativePath + ": " + err.Error())
commitErr := commitRootedFileContents(root, absolutePath, rootedRelative, priorInfo, expectedContent, content)
if commitErr != nil && !errors.Is(commitErr, errFilePublishedWithCleanupFailure) {
return errorResult("Error writing file " + relativePath + ": " + commitErr.Error())
}
modelKnownContent := content
// Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the
@@ -193,7 +195,13 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
if finalContentKnown {
summary += inlineDiagnostics(ctx, options, absolutePath, relativePath)
}
if commitErr != nil {
summary += "\n" + commitErr.Error()
}
result := okResult(summary)
if commitErr != nil {
result.Status = StatusError
}
result.ChangedFiles = []string{relativePath}
// Do not pretend an unreadable overwrite was a creation. The write may be
// valid, but ACP only receives an exact before/after pair we actually saw.