diff --git a/cmd/entire/cli/checkpoint/opf_span_cache.go b/cmd/entire/cli/checkpoint/opf_span_cache.go new file mode 100644 index 0000000000..e144236a10 --- /dev/null +++ b/cmd/entire/cli/checkpoint/opf_span_cache.go @@ -0,0 +1,152 @@ +package checkpoint + +import ( + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "strings" + "time" + + "github.com/entireio/cli/cmd/entire/cli/gitdir" + "github.com/entireio/cli/cmd/entire/cli/jsonutil" + "github.com/entireio/cli/cmd/entire/cli/osroot" + "github.com/entireio/cli/redact" + + "github.com/go-git/go-git/v6" +) + +// OPFSpanCacheDirName holds OPF scan results between the background scan that +// produces them and the rewrite that applies them. Like RedactCacheDirName it +// sits in the git common dir, where nothing is walked into a checkpoint tree, +// and every entry is derived data: deleting the directory only costs a rescan. +const OPFSpanCacheDirName = "entire-opf-cache" + +// OPFSpanCacheMaxAge bounds how long an entry is kept after it was written. +// A checkpoint is normally rewritten and pushed within minutes of its scan, so +// an entry this old belongs to content that has shipped or been abandoned. +const OPFSpanCacheMaxAge = 30 * 24 * time.Hour + +// opfSpanCache is the git-common-dir implementation of redact.OPFSpanCache. +// Keys are hex SHA-256 digests produced by the redact package, so every entry +// name is safe by construction; I/O still goes through the shared root. +type opfSpanCache struct { + root *os.Root +} + +// OPFSpanCacheForRepo opens the cache in repo's git common dir. It resolves the +// directory from the repository itself, never from the process's working +// directory, so a caller holding one repo can never read or write another's +// scan results. +func OPFSpanCacheForRepo(repo *git.Repository) (redact.OPFSpanCache, error) { + _, commonDir, err := repositoryDirs(repo) + if err != nil { + return nil, err + } + return OPFSpanCacheAt(commonDir) +} + +// OPFSpanCacheAt opens the cache under gitCommonDir, creating its directory. +func OPFSpanCacheAt(gitCommonDir string) (redact.OPFSpanCache, error) { + root, err := gitdir.OpenAt(gitCommonDir) + if err != nil { + return nil, fmt.Errorf("open git common dir: %w", err) + } + if err := osroot.MkdirAllNoSymlink(root, OPFSpanCacheDirName, 0o700); err != nil { + return nil, fmt.Errorf("create %s: %w", OPFSpanCacheDirName, err) + } + return &opfSpanCache{root: root}, nil +} + +// opfSpanRecord is the stored form of one redact.Span. It is a separate, +// tagged type so the on-disk format does not follow redact's field names. +type opfSpanRecord struct { + Start int `json:"s"` + End int `json:"e"` + Label string `json:"l"` +} + +func opfSpanEntryName(key string) (string, error) { + if len(key) != 64 || strings.Trim(key, "0123456789abcdef") != "" { + return "", fmt.Errorf("invalid OPF span cache key %q", key) + } + return OPFSpanCacheDirName + "/" + key + ".json", nil +} + +func (c *opfSpanCache) LoadOPFSpans(key string) (map[string][]redact.Span, bool) { + name, err := opfSpanEntryName(key) + if err != nil { + return nil, false + } + data, err := osroot.ReadFileNoFollow(c.root, name) + if err != nil { + return nil, false + } + var records map[string][]opfSpanRecord + if err := json.Unmarshal(data, &records); err != nil || records == nil { + return nil, false + } + spans := make(map[string][]redact.Span, len(records)) + for leaf, recs := range records { + out := make([]redact.Span, 0, len(recs)) + for _, r := range recs { + out = append(out, redact.Span{Start: r.Start, End: r.End, Label: r.Label}) + } + spans[leaf] = out + } + return spans, true +} + +func (c *opfSpanCache) StoreOPFSpans(key string, spans map[string][]redact.Span) error { + name, err := opfSpanEntryName(key) + if err != nil { + return err + } + records := make(map[string][]opfSpanRecord, len(spans)) + for leaf, ss := range spans { + recs := make([]opfSpanRecord, 0, len(ss)) + for _, sp := range ss { + recs = append(recs, opfSpanRecord{Start: sp.Start, End: sp.End, Label: sp.Label}) + } + records[leaf] = recs + } + data, err := json.Marshal(records) + if err != nil { + return fmt.Errorf("encode OPF span cache entry: %w", err) + } + if err := jsonutil.WriteFileAtomicIn(c.root, name, data, 0o600); err != nil { + return fmt.Errorf("write OPF span cache entry: %w", err) + } + return nil +} + +// PruneOPFSpanCache removes entries written before now-maxAge and returns how +// many it removed. Best-effort: entries it cannot inspect are left in place. +func PruneOPFSpanCache(gitCommonDir string, now time.Time, maxAge time.Duration) (int, error) { + root, err := gitdir.OpenAt(gitCommonDir) + if err != nil { + return 0, fmt.Errorf("open git common dir: %w", err) + } + entries, err := osroot.ReadDirNoSymlinks(root, OPFSpanCacheDirName) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return 0, nil + } + return 0, fmt.Errorf("list %s: %w", OPFSpanCacheDirName, err) + } + removed := 0 + for _, e := range entries { + if !e.Type().IsRegular() || !strings.HasSuffix(e.Name(), ".json") { + continue + } + info, infoErr := e.Info() + if infoErr != nil || now.Sub(info.ModTime()) < maxAge { + continue + } + if osroot.RemoveNoSymlinks(root, OPFSpanCacheDirName+"/"+e.Name()) == nil { + removed++ + } + } + return removed, nil +} diff --git a/cmd/entire/cli/checkpoint/opf_span_cache_test.go b/cmd/entire/cli/checkpoint/opf_span_cache_test.go new file mode 100644 index 0000000000..b16e58768a --- /dev/null +++ b/cmd/entire/cli/checkpoint/opf_span_cache_test.go @@ -0,0 +1,104 @@ +package checkpoint + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/gitrepo" + "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/entireio/cli/redact" +) + +var testOPFKey = strings.Repeat("ab", 32) + +func TestOPFSpanCache_RoundTrip(t *testing.T) { + t.Parallel() + dir := t.TempDir() + cache, err := OPFSpanCacheAt(dir) + require.NoError(t, err) + + _, ok := cache.LoadOPFSpans(testOPFKey) + assert.False(t, ok, "a missing entry must read as not cached") + + want := map[string][]redact.Span{ + strings.Repeat("cd", 32): {{Start: 0, End: 5, Label: "private_person"}}, + strings.Repeat("ef", 32): {}, + } + require.NoError(t, cache.StoreOPFSpans(testOPFKey, want)) + got, ok := cache.LoadOPFSpans(testOPFKey) + require.True(t, ok) + assert.Equal(t, want[strings.Repeat("cd", 32)], got[strings.Repeat("cd", 32)]) + _, present := got[strings.Repeat("ef", 32)] + assert.True(t, present, "a leaf with no spans must still be recorded as scanned") + + info, err := os.Stat(filepath.Join(dir, OPFSpanCacheDirName, testOPFKey+".json")) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +} + +func TestOPFSpanCache_RejectsKeysThatAreNotDigests(t *testing.T) { + t.Parallel() + cache, err := OPFSpanCacheAt(t.TempDir()) + require.NoError(t, err) + for _, key := range []string{"", "../escape", strings.Repeat("A", 64), strings.Repeat("a", 63)} { + require.Error(t, cache.StoreOPFSpans(key, nil), "key %q", key) + _, ok := cache.LoadOPFSpans(key) + assert.False(t, ok, "key %q", key) + } +} + +func TestOPFSpanCache_CorruptEntryReadsAsMissing(t *testing.T) { + t.Parallel() + dir := t.TempDir() + cache, err := OPFSpanCacheAt(dir) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, OPFSpanCacheDirName, testOPFKey+".json"), []byte("{not json"), 0o600)) + _, ok := cache.LoadOPFSpans(testOPFKey) + assert.False(t, ok) +} + +func TestPruneOPFSpanCache_RemovesOnlyOldEntries(t *testing.T) { + t.Parallel() + dir := t.TempDir() + cache, err := OPFSpanCacheAt(dir) + require.NoError(t, err) + oldKey, newKey := strings.Repeat("0a", 32), strings.Repeat("0b", 32) + require.NoError(t, cache.StoreOPFSpans(oldKey, nil)) + require.NoError(t, cache.StoreOPFSpans(newKey, nil)) + now := time.Now() + oldPath := filepath.Join(dir, OPFSpanCacheDirName, oldKey+".json") + require.NoError(t, os.Chtimes(oldPath, now.Add(-2*OPFSpanCacheMaxAge), now.Add(-2*OPFSpanCacheMaxAge))) + + removed, err := PruneOPFSpanCache(dir, now, OPFSpanCacheMaxAge) + require.NoError(t, err) + assert.Equal(t, 1, removed) + _, ok := cache.LoadOPFSpans(oldKey) + assert.False(t, ok, "the stale entry must be gone") + _, ok = cache.LoadOPFSpans(newKey) + assert.True(t, ok, "a recent entry must survive") +} + +// The cache must live in the repository it was opened for, whatever the +// process's working directory is. Resolving it from the working directory once +// wrote test results into the developer's own checkout. +func TestOPFSpanCacheForRepo_UsesTheRepositoryNotTheWorkingDirectory(t *testing.T) { + t.Parallel() + dir := t.TempDir() + testutil.InitRepo(t, dir) + repo, err := gitrepo.OpenPath(dir) + require.NoError(t, err) + t.Cleanup(func() { repo.Close() }) + + cache, err := OPFSpanCacheForRepo(repo) + require.NoError(t, err) + require.NoError(t, cache.StoreOPFSpans(testOPFKey, nil)) + + _, err = os.Stat(filepath.Join(dir, ".git", OPFSpanCacheDirName, testOPFKey+".json")) + require.NoError(t, err, "the entry must be written under the repository's own git dir") +} diff --git a/cmd/entire/cli/execx/spawn_detached.go b/cmd/entire/cli/execx/spawn_detached.go index 07a6cc9f0b..eebc6897fc 100644 --- a/cmd/entire/cli/execx/spawn_detached.go +++ b/cmd/entire/cli/execx/spawn_detached.go @@ -2,7 +2,6 @@ package execx import ( "context" - "io" "os" "os/exec" "testing" @@ -13,7 +12,7 @@ import ( // CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS on Windows, via detachFromTTY). // The child runs in dir (os.TempDir() when empty, so the child never holds the // parent's working directory), inherits the parent's environment, and has its -// stdout/stderr discarded. Best-effort: every error is swallowed — callers +// stdout/stderr sent to the null device. Best-effort: every error is swallowed — callers // treat the spawn as advisory background work. // // In-process `go test` runs are a no-op: the current executable is the test @@ -28,6 +27,25 @@ func SpawnDetached(dir string, args ...string) { return } + cmd := detachedCommand(executable, dir, args...) + if err := cmd.Start(); err != nil { + return + } + // Release the process so it can run independently of the parent. + //nolint:errcheck // best effort — the child continues regardless + _ = cmd.Process.Release() +} + +// detachedCommand builds the child SpawnDetached starts. Separate from the +// spawn so tests can check how the child is wired without forking it. +// +// Stdout and stderr stay nil, which os/exec opens as the null device. A +// non-*os.File writer such as io.Discard would instead hand the child a pipe +// drained by a goroutine in this process; once this process exits, the child's +// next write to that pipe raises SIGPIPE and kills it. A child that writes +// anything to stderr, such as a warning from the `__opf_scan` worker, would die +// on its first line. +func detachedCommand(executable, dir string, args ...string) *exec.Cmd { // context.Background(): the child must outlive the parent, so it is never // tied to a cancellable context. cmd := exec.CommandContext(context.Background(), executable, args...) @@ -37,13 +55,5 @@ func SpawnDetached(dir string, args ...string) { cmd.Dir = os.TempDir() } cmd.Env = os.Environ() - cmd.Stdout = io.Discard - cmd.Stderr = io.Discard - - if err := cmd.Start(); err != nil { - return - } - // Release the process so it can run independently of the parent. - //nolint:errcheck // best effort — the child continues regardless - _ = cmd.Process.Release() + return cmd } diff --git a/cmd/entire/cli/execx/spawn_detached_test.go b/cmd/entire/cli/execx/spawn_detached_test.go new file mode 100644 index 0000000000..dc28271288 --- /dev/null +++ b/cmd/entire/cli/execx/spawn_detached_test.go @@ -0,0 +1,25 @@ +package execx + +import ( + "testing" +) + +// A detached child must not write into a pipe this process drains: after the +// parent exits, the child's next stdout/stderr write raises SIGPIPE and kills +// it. Nil streams are opened as the null device instead. +func TestDetachedCommand_StdioIsNullDevice(t *testing.T) { + t.Parallel() + cmd := detachedCommand("/bin/entire", "", "__opf_scan") + if cmd.Stdout != nil { + t.Errorf("Stdout = %T; want nil so the child writes to the null device, not a pipe", cmd.Stdout) + } + if cmd.Stderr != nil { + t.Errorf("Stderr = %T; want nil so the child writes to the null device, not a pipe", cmd.Stderr) + } + if cmd.Stdin != nil { + t.Errorf("Stdin = %T; want nil", cmd.Stdin) + } + if cmd.Dir == "" { + t.Error("Dir is empty; want os.TempDir() fallback") + } +} diff --git a/cmd/entire/cli/hooks_git_cmd.go b/cmd/entire/cli/hooks_git_cmd.go index 41ba3bcece..a22c80ba4d 100644 --- a/cmd/entire/cli/hooks_git_cmd.go +++ b/cmd/entire/cli/hooks_git_cmd.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "os" "time" "github.com/entireio/cli/cmd/entire/cli/agent/external" @@ -228,7 +229,7 @@ func newHooksGitPostRewriteCmd() *cobra.Command { } func newHooksGitPrePushCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "pre-push ", Short: "Handle pre-push git hook", Args: cobra.ExactArgs(1), @@ -250,7 +251,16 @@ func newHooksGitPrePushCmd() *cobra.Command { defer g.span.End() g.logInvoked(slog.String("remote", remote)) - hookErr := g.strategy.PrePushFromGitHook(g.ctx, remote) + ctx := g.ctx + // Only scripts that replay stdin to whatever runs after them set + // this (see strategy.PrePushStdinRefsEnv). + if os.Getenv(strategy.PrePushStdinRefsEnv) == "1" { + // The ref list git passes on stdin is what lets OPF refuse a + // push that sends unverified checkpoint content itself. + ctx = strategy.WithPrePushRefsFrom(ctx, cmd.InOrStdin()) + } + + hookErr := g.strategy.PrePushFromGitHook(ctx, remote) g.logCompleted(hookErr) // Propagate the error so the hook script exits non-zero and @@ -270,4 +280,5 @@ func newHooksGitPrePushCmd() *cobra.Command { return fmt.Errorf("pre-push: %w", hookErr) }, } + return cmd } diff --git a/cmd/entire/cli/integration_test/opf_command_trust_test.go b/cmd/entire/cli/integration_test/opf_command_trust_test.go index 84406e860b..92231432da 100644 --- a/cmd/entire/cli/integration_test/opf_command_trust_test.go +++ b/cmd/entire/cli/integration_test/opf_command_trust_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/entireio/cli/cmd/entire/cli/testutil" ) @@ -32,6 +33,38 @@ func opfSettingsBlock(command string) map[string]any { } } +// waitForOPFScanWorker waits for the background OPF scan worker, which the +// push starts and which is where the opf binary actually runs, to log that it +// finished. Asserting on the marker before that would make the negative tests +// pass vacuously. +func waitForOPFScanWorker(t *testing.T, env *TestEnv) { + t.Helper() + // Wait for every worker a push spawned, not just the first to finish: a + // later one still writing to .git would break the test's cleanup. Every + // worker logs "finished" on every exit. + logPath := filepath.Join(env.RepoDir, ".entire", "logs", "entire.log") + deadline := time.Now().Add(60 * time.Second) + for time.Now().Before(deadline) { + if data, err := os.ReadFile(logPath); err == nil { + log := string(data) + finished := strings.Count(log, "opf scan worker finished") + if finished > 0 && finished >= strings.Count(log, "spawned background scan") { + return + } + } + time.Sleep(200 * time.Millisecond) + } + data, _ := os.ReadFile(logPath) //nolint:errcheck // diagnostics only + var opfLines []string + for _, line := range strings.Split(string(data), "\n") { + if strings.Contains(line, "opf") || strings.Contains(line, "OPF") { + opfLines = append(opfLines, line) + } + } + t.Fatalf("a background OPF scan worker never reported finishing; OPF log lines:\n%s", + strings.Join(opfLines, "\n")) +} + // setupOPFAttack stages the attacker's payload inside the repo and returns the // marker path (outside the repo, so committing the repo cannot include it). func setupOPFAttack(t *testing.T, env *TestEnv) (marker, command string) { @@ -56,10 +89,10 @@ func setupOPFAttack(t *testing.T, env *TestEnv) (marker, command string) { // holding a lock — but this test is what surfaced the hang, and a recurrence // still shows up here as a timeout. // -// The push is expected to FAIL: with the command correctly ignored, OPF falls -// back to resolving "opf" on $PATH, which is absent here, and the pre-push -// rewrite fails closed rather than pushing content OPF never scanned. What -// matters is which binary was reached — asserted via the marker. +// With the command correctly ignored, OPF falls back to resolving "opf" on +// $PATH: absent, the background scan fails closed; present, it scans with that +// binary instead. Either way what matters is which binary was reached — +// asserted via the marker once the worker has finished. func TestOPFCommandTrust_CommittedCommandIsNotExecuted(t *testing.T) { t.Parallel() @@ -76,6 +109,7 @@ func TestOPFCommandTrust_CommittedCommandIsNotExecuted(t *testing.T) { _ = createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") err := env.GitPushWithHooksAllowError("origin", "HEAD") + waitForOPFScanWorker(t, env) if _, statErr := os.Stat(marker); statErr == nil { t.Fatal("payload from a committed settings.json was EXECUTED during push") @@ -111,11 +145,13 @@ func TestOPFCommandTrust_UntrackedLocalCommandIsExecuted(t *testing.T) { _ = createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") - // The push is expected to fail: the payload exits non-zero, so OPF fails - // closed. Reaching the binary at all is the point here. - if pushErr := env.GitPushWithHooksAllowError("origin", "HEAD"); pushErr == nil { - t.Log("push succeeded; OPF still resolved the local command") + // The push itself succeeds: OPF runs in the background worker it starts. + // The payload exits non-zero there, so the scan fails closed; reaching the + // binary at all is the point here. + if pushErr := env.GitPushWithHooksAllowError("origin", "HEAD"); pushErr != nil { + t.Logf("push failed: %v", pushErr) } + waitForOPFScanWorker(t, env) if _, statErr := os.Stat(marker); statErr != nil { t.Fatal("a developer-owned local command must still be honored; " + @@ -143,6 +179,9 @@ func TestOPFCommandTrust_CommittedLocalFileIsNotExecuted(t *testing.T) { _ = createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") + // The whole committed layer is ignored, and it was the only place OPF was + // enabled, so OPF is off: no background worker starts and there is nothing + // to wait for before checking the marker. if pushErr := env.GitPushWithHooksAllowError("origin", "HEAD"); pushErr == nil { t.Log("push succeeded") } @@ -151,3 +190,43 @@ func TestOPFCommandTrust_CommittedLocalFileIsNotExecuted(t *testing.T) { t.Fatal("payload from a COMMITTED settings.local.json was EXECUTED during push") } } + +// A user push that sends entire/checkpoints/v1 itself (`git push --all`) while +// its checkpoints wait for the background scan must fail: holding back +// Entire's own v1 push does not stop git from sending the unscanned branch. +// A push of the user's branch alone still goes through. +func TestOPFPrePush_PushAllRefusesUnscannedV1(t *testing.T) { + t.Parallel() + + env := NewFeatureBranchEnv(t) + bareDir := env.SetupBareRemote() + // A local opf that always fails: the background worker this push starts + // cannot scan anything, so v1 stays unscanned for the whole test. + env.WriteFile(".entire/opf-broken", "#!/bin/sh\nexit 1\n") + if err := os.Chmod(filepath.Join(env.RepoDir, ".entire", "opf-broken"), 0o755); err != nil { + t.Fatal(err) + } + env.WriteFile(".entire/settings.local.json", + `{"redaction":{"openai_privacy_filter":{"enabled":true,"prompt_default":"always",`+ + `"categories":{"private_person":true},"command":"./.entire/opf-broken"}}}`) + env.GitAdd(".entire/opf-broken") + env.GitCommit("Add local opf shim") + + _ = createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") + + err := env.GitPushWithHooksAllowError("origin", "--all") + if err == nil { + t.Fatal("git push --all must fail while entire/checkpoints/v1 is unscanned") + } + if !strings.Contains(err.Error(), "exit status") { + t.Fatalf("unexpected push error: %v", err) + } + if out := testutil.RunGit(t, bareDir, "for-each-ref", "refs/heads/entire/checkpoints/v1"); strings.TrimSpace(out) != "" { + t.Fatalf("the unscanned v1 reached the remote: %s", out) + } + + if err := env.GitPushWithHooksAllowError("origin", "HEAD"); err != nil { + t.Fatalf("pushing only the user's branch must still succeed: %v", err) + } + waitForOPFScanWorker(t, env) +} diff --git a/cmd/entire/cli/integration_test/testenv.go b/cmd/entire/cli/integration_test/testenv.go index c271d6e3df..d6d85cff16 100644 --- a/cmd/entire/cli/integration_test/testenv.go +++ b/cmd/entire/cli/integration_test/testenv.go @@ -1909,7 +1909,7 @@ func (env *TestEnv) InstallRealPrePushHook() { env.T.Fatalf("failed to create hooks dir: %v", err) } // Quote the binary path so a temp path containing spaces still execs. - script := fmt.Sprintf("#!/bin/sh\nexec %q hooks git pre-push \"$1\"\n", getTestBinary()) + script := fmt.Sprintf("#!/bin/sh\n%s=1 exec %q hooks git pre-push \"$1\"\n", strategy.PrePushStdinRefsEnv, getTestBinary()) hookPath := filepath.Join(hooksDir, "pre-push") if err := os.WriteFile(hookPath, []byte(script), 0o755); err != nil { env.T.Fatalf("failed to write pre-push hook: %v", err) diff --git a/cmd/entire/cli/opf_scan_cmd.go b/cmd/entire/cli/opf_scan_cmd.go new file mode 100644 index 0000000000..098ec670b3 --- /dev/null +++ b/cmd/entire/cli/opf_scan_cmd.go @@ -0,0 +1,29 @@ +package cli + +import ( + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/strategy" +) + +// newOPFScanCmd creates the hidden command the pre-push hook spawns detached +// when it held checkpoints back because OPF had not scanned them yet (see +// strategy.RunOPFScan). It scans in the background and then delivers to the +// remote the spawning push named. Not for direct use: `entire push` is the +// interactive surface for delivery. +func newOPFScanCmd() *cobra.Command { + return &cobra.Command{ + Use: "__opf_scan ", + Hidden: true, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // Detached child with discarded stdout/stderr: attach the file + // logger so a scan or delivery that cannot finish is diagnosable + // in .entire/logs/entire.log. The root PersistentPostRun closes it. + ensureLogger(cmd) + // RunOPFScan is best-effort and never returns a non-nil error; + // nothing watches this child's exit code. + return strategy.RunOPFScan(cmd.Context(), args[0]) + }, + } +} diff --git a/cmd/entire/cli/opf_scan_cmd_test.go b/cmd/entire/cli/opf_scan_cmd_test.go new file mode 100644 index 0000000000..93c2d3e440 --- /dev/null +++ b/cmd/entire/cli/opf_scan_cmd_test.go @@ -0,0 +1,39 @@ +package cli + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/strategy" + "github.com/entireio/cli/redact" +) + +// TestOPFScanCmd_RunsThroughRootAndLogsToFile guards what only the real root +// command wires up: that __opf_scan is registered (pre-push spawns it by name, +// so a missing registration is a silent no-op in production) and that the +// detached child, whose stdout/stderr are discarded, leaves its reasoning in +// .entire/logs/entire.log. OPF off is the case under test because it must +// still exit 0: nothing watches the child's exit code. +func TestOPFScanCmd_RunsThroughRootAndLogsToFile(t *testing.T) { + setupStopTestRepo(t) + markRepoSetUpForLogging(t) + redact.ResetOPFConfigForTest() + t.Cleanup(redact.ResetOPFConfigForTest) + strategy.ResetRedactionConfiguredForTest() + t.Cleanup(strategy.ResetRedactionConfiguredForTest) + t.Setenv("ENTIRE_LOG_LEVEL", "debug") + + require.NoError(t, executeThroughRoot(t, "__opf_scan", "origin"), + "the detached scan child must exit 0; it is best-effort and unwatched") + + root, err := paths.WorktreeRoot(context.Background()) + require.NoError(t, err) + logData, err := os.ReadFile(filepath.Join(root, ".entire", "logs", "entire.log")) + require.NoError(t, err) + require.Contains(t, string(logData), "opf scan skipped: OPF is not enabled") +} diff --git a/cmd/entire/cli/root.go b/cmd/entire/cli/root.go index 978bcb9515..b83dd6e840 100644 --- a/cmd/entire/cli/root.go +++ b/cmd/entire/cli/root.go @@ -230,6 +230,7 @@ func NewRootCmd() *cobra.Command { cmd.AddCommand(newCurlBashPostInstallCmd()) cmd.AddCommand(newRefreshTrailEnablementCmd()) cmd.AddCommand(newSweepSessionsCmd()) + cmd.AddCommand(newOPFScanCmd()) // Experimental command (developer-only visibility; setup/tune runners). experimental.Register(cmd, newRunnerCmd()) // 'runner' (experimental) diff --git a/cmd/entire/cli/session_sweep.go b/cmd/entire/cli/session_sweep.go index fa42f129fc..44424bf3fc 100644 --- a/cmd/entire/cli/session_sweep.go +++ b/cmd/entire/cli/session_sweep.go @@ -10,6 +10,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/entireio/cli/cmd/entire/cli/spawnmarker" "github.com/entireio/cli/cmd/entire/cli/strategy" ) @@ -198,9 +199,10 @@ const sessionSweepSpawnThrottle = 15 * time.Minute // sweepRecentlySpawned reports whether a detached sweep was spawned for this // repo within sessionSweepSpawnThrottle and, when it wasn't, records now as // the most recent spawn. Same flock-serialized marker mechanism as the -// trail-enablement refresh (see recentlySpawnedMarker), with its own marker. +// trail-enablement refresh (see spawnmarker.RecentlySpawned), with its own +// marker. func sweepRecentlySpawned(commonDir string, now time.Time) bool { - return recentlySpawnedMarker(commonDir, "session-sweep-spawn", sessionSweepSpawnThrottle, now) + return spawnmarker.RecentlySpawned(commonDir, "session-sweep-spawn", sessionSweepSpawnThrottle, now) } // maybeSpawnSessionSweep fires one detached __sweep_sessions child when the diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 19621200e7..83b757d478 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -602,6 +602,21 @@ func (s *EntireSettings) GoredactEnabled() bool { return *s.Redaction.Goredact.Enabled } +// OPFEnabled reports whether the OpenAI Privacy Filter layer is switched on. +// Default: false. +// +// This is the SETTINGS answer, which is not the same question as +// redact.OPFEnabled: that one reports the process-global runtime, and it only +// becomes true once something has called EnsureRedactionConfigured. Read-only +// surfaces that never configure the runtime — `entire status` — must ask here, +// or they would report the filter as off in every repo that has it on. +func (s *EntireSettings) OPFEnabled() bool { + if s == nil || s.Redaction == nil || s.Redaction.OpenAIPrivacyFilter == nil { + return false + } + return s.Redaction.OpenAIPrivacyFilter.Enabled +} + // ErrScannerConfig marks scanner-configuration failures. Consumers use // errors.Is to distinguish these (fail-closed) from ordinary settings // problems (warn-and-default). diff --git a/cmd/entire/cli/spawnmarker/spawnmarker.go b/cmd/entire/cli/spawnmarker/spawnmarker.go new file mode 100644 index 0000000000..67b02d3cf9 --- /dev/null +++ b/cmd/entire/cli/spawnmarker/spawnmarker.go @@ -0,0 +1,65 @@ +// Package spawnmarker throttles detached one-shot child processes to one per +// repository per window. +// +// Every detached worker in this CLI (the trail-enablement refresh and the +// zombie-session sweep in package cli, the OPF flush in package strategy) is +// nominated by a hook that can fire in bursts and from several worktrees at +// once. Without a shared throttle each of those hooks forks its own child, +// every child re-opens the repository, and a condition that cannot self-heal +// is retried once per hook forever. +// +// The marker lives here rather than beside any one worker because package +// strategy cannot import package cli: the CLI commands are built on top of the +// strategy, so the dependency only runs one way. +package spawnmarker + +import ( + "strings" + "time" + + "github.com/entireio/cli/cmd/entire/cli/gitdir" + "github.com/entireio/cli/cmd/entire/cli/internal/flock" + "github.com/entireio/cli/cmd/entire/cli/jsonutil" + "github.com/entireio/cli/cmd/entire/cli/osroot" +) + +// dirName is the marker directory inside the git common dir. It is the same +// "entire" directory clone preferences live in. +const dirName = "entire" + +// RecentlySpawned reports whether the named spawn marker under the shared +// git-common-dir was refreshed within ttl and, when it wasn't, records now as +// the most recent spawn. The read-and-record is serialized with a flock keyed +// to the marker (so every worktree of the repo agrees), collapsing a burst of +// concurrent hooks to a single detached child rather than one per hook. +// Best-effort: any error resolving, locking, or writing the marker falls +// through to spawning — never worse than having no guard at all. Each caller +// passes its own marker name and ttl. +func RecentlySpawned(commonDir, marker string, ttl time.Duration, now time.Time) bool { + root, err := gitdir.OpenAt(commonDir) + if err != nil { + return false + } + // Create the directory before acquiring the lock: flock opens the lock file, + // which fails if its parent doesn't exist yet (mirrors + // ModifyClonePreferences, which creates before locking). + if err := osroot.MkdirAllNoSymlink(root, dirName, 0o750); err != nil { + return false + } + markerName := dirName + "/" + marker + release, err := flock.AcquireIn(root, markerName+".lock") + if err != nil { + return false + } + defer release() + + if data, readErr := osroot.ReadFileNoFollow(root, markerName); readErr == nil { + if last, parseErr := time.Parse(time.RFC3339Nano, strings.TrimSpace(string(data))); parseErr == nil && + now.After(last) && now.Sub(last) < ttl { + return true + } + } + //nolint:errcheck // best-effort marker; a failed write just means the next hook re-spawns + _ = jsonutil.WriteFileAtomicIn(root, markerName, []byte(now.UTC().Format(time.RFC3339Nano)), 0o600) + return false +} diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index 4fb40c58d4..6a485b1b1a 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -413,6 +413,17 @@ type checkpointSyncInfo struct { // where a user finds out why. IgnoredRemote string IgnoredReason string + // OPFPending counts checkpoints held until the OpenAI Privacy Filter's + // background scan covers them. It is 0 when the filter is off (the default), when + // there is no backlog, and when the local read failed — status never fails + // over this. A ref the worker can never rewrite simply stays counted here + // for as long as it exists; there is no separate escalation. + // + // Unlike Unpushed, it is not scoped to a sync destination: OPF is a local + // rewrite that runs before anything is pushed, so it holds identically + // whatever the elected remote turns out to be — which is why it is computed + // before the election rather than on any one of its branches. + OPFPending int // IgnoredDisproved marks a rejection where a remote named a different // owner — the fork case, where checkpoints landing on the elected remote is // the intended outcome — so it renders as information, not a warning. False @@ -471,6 +482,12 @@ func resolveDedicatedReadSource(ctx context.Context, s *EntireSettings, lead str func computeCheckpointSyncInfo(ctx context.Context, s *EntireSettings) checkpointSyncInfo { info := checkpointSyncInfo{PushDisabled: s.IsPushSessionsDisabled()} + // Before the election, and before every early return below: the OPF + // backlog is a property of the local refs alone, so it is just as true in + // a repo with no remotes or a fail-closed checkpoint_push_remote as in a + // healthy one. Computing it on one branch of the election would hide it + // from exactly the repos most likely to have accumulated one. + info.OPFPending = opfBacklogForStatus(ctx, s) elected, err := strategy.ResolveCheckpointSyncRemote(ctx) if err != nil { @@ -597,6 +614,38 @@ func countUnpushedCheckpointsForStatus(ctx context.Context, remoteName string) i return n } +// opfBacklogForStatus reads the local OpenAI Privacy Filter backlog: how many +// checkpoints are held until the background scan covers them. +// +// Best-effort and local-only, on the same contract as +// countUnpushedCheckpointsForStatus: status must never fail because a backlog +// read failed, so every error logs at debug and reads as "no backlog". +// +// Gated on the setting first so the default (filter off) opens no repository +// and touches no files: this runs on every `entire status` in an enabled repo. +// The settings answer is the right one here even though the push decision and +// worker consult redact.OPFEnabled — see settings.EntireSettings.OPFEnabled. +func opfBacklogForStatus(ctx context.Context, s *EntireSettings) int { + if !s.OPFEnabled() { + return 0 + } + repo, err := gitrepo.OpenCurrent(ctx) + if err != nil { + logging.Debug(ctx, "OPF backlog read failed to open the repository; omitting from status", + slog.String("error", err.Error())) + return 0 + } + defer repo.Close() + + awaiting, err := strategy.CheckpointsAwaitingOPF(ctx, repo) + if err != nil { + logging.Debug(ctx, "pending OPF checkpoint count failed; omitting from status", + slog.String("error", err.Error())) + return 0 + } + return awaiting +} + // writeCheckpointSyncLines reports the checkpoint sync destination (and the // unpushed counter, when non-zero) in the enabled status block, prefixed by the // disabled-pushing line when automatic pushing is off. No remotes configured @@ -679,6 +728,27 @@ func writeCheckpointSyncLines(ctx context.Context, b *strings.Builder, s *Entire b.WriteString("\n ") b.WriteString(sty.render(sty.dim, formatUnpushedCheckpointsLine(info))) } + // Dim, like the counter above: work in progress, nothing to do about it. + if info.OPFPending > 0 { + b.WriteString("\n ") + b.WriteString(sty.render(sty.dim, formatPendingOPFLine(info.OPFPending))) + } +} + +// formatPendingOPFLine phrases the pending-redaction counter: unpushed +// checkpoints that do not carry the OPF trailer yet. It does not claim a scan +// is running: none is until a push resolves OPFRun and holds them. +// +// It deliberately says nothing about whether these checkpoints can be pushed. +// That depends on the OPF decision resolved at push time, and a user who +// answers "No" pushes them regex-only; status has no way to know that answer in +// advance and must not promise either outcome. +func formatPendingOPFLine(pending int) string { + noun := nounCheckpoints + if pending == 1 { + noun = nounCheckpoint + } + return fmt.Sprintf("%d %s pending OpenAI Privacy Filter redaction", pending, noun) } // formatUnpushedCheckpointsLine phrases the unpushed counter. Dedicated URL @@ -1154,6 +1224,11 @@ type statusJSON struct { // fall back to the elected remote). Mirrors the text path's warning line. CheckpointRemoteIgnored string `json:"checkpoint_remote_ignored,omitempty"` CheckpointRemoteIgnoredReason string `json:"checkpoint_remote_ignored_reason,omitempty"` + // CheckpointOPFPending mirrors the text path's OPF line: queued checkpoint + // refs the privacy filter has not rewritten yet. Absent when the OpenAI + // Privacy Filter is off, which is the default, so its absence is not + // evidence that a backlog is clear. + CheckpointOPFPending int `json:"checkpoint_opf_pending,omitempty"` // CheckpointRemoteIgnoredRemedy is the command that claims the ignored // store for this clone. Emitted so an agent reading --json can act on the // rejection rather than only report it; absent when no single command @@ -1269,6 +1344,7 @@ func runStatusJSON(ctx context.Context, w io.Writer) error { result.UnpushedCheckpoints = syncInfo.Unpushed result.CheckpointRemoteIgnored = syncInfo.IgnoredRemote result.CheckpointRemoteIgnoredReason = syncInfo.IgnoredReason + result.CheckpointOPFPending = syncInfo.OPFPending result.CheckpointRemoteIgnoredRemedy = syncInfo.IgnoredRemedy result.CheckpointRemoteIgnoredVerdict = syncInfo.IgnoredVerdict() diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 12335543e8..293559fbd3 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -3337,3 +3337,183 @@ func TestRunStatusDetailed_ReportsRejectedExternalAgents(t *testing.T) { //nolin t.Errorf("status does not name where the setting must live:\n%s", got) } } + +// OPF backlog visibility: with the OpenAI Privacy Filter enabled on the +// git-refs backend, the model rewrite runs in a detached worker rather than on +// the push path, so `entire status` is the only place a user can see that +// redaction work is outstanding. + +// opfBacklogSettingsOn is the only configuration in which an OPF backlog can +// exist: Entire enabled, the git-refs checkpoint backend, and the filter on. +const opfBacklogSettingsOn = `{"enabled": true, "checkpoints": {"primary": {"type": "git-refs"}}, "redaction": {"openai_privacy_filter": {"enabled": true}}}` + +// opfBacklogSettingsOff is the same repo with the filter left off, which is the +// default. Identical in every other respect so a diff of the two outputs is a +// diff of the OPF lines alone. +const opfBacklogSettingsOff = `{"enabled": true, "checkpoints": {"primary": {"type": "git-refs"}}}` + +// queueCheckpointRefAwaitingOPF points refName at a commit carrying no OPF +// trailer and enqueues it — exactly the state a checkpoint sits in between +// being written and being rewritten by the privacy filter. +func queueCheckpointRefAwaitingOPF(t *testing.T, refName, commit string) { + t.Helper() + testutil.GitUpdateRef(t, ".", refName, commit) + queue := checkpoint.NewPushQueue(filepath.Join(testWorkingDir(t), ".git")) + if err := queue.Enqueue(plumbing.ReferenceName(refName)); err != nil { + t.Fatalf("Enqueue(%s): %v", refName, err) + } +} + +func testWorkingDir(t *testing.T) string { + t.Helper() + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + return cwd +} + +func TestRunStatus_OPFPendingCounter(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, opfBacklogSettingsOn) + testutil.AddRemote(t, ".", originRemoteName, "https://example.com/origin.git") + head := checkpointSyncTestCommit(t, "a.txt", "one") + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000001", head) + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000002", head) + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + + out := stdout.String() + if !strings.Contains(out, "2 checkpoints pending OpenAI Privacy Filter redaction") { + t.Errorf("expected the pending-redaction counter, got:\n%s", out) + } +} + +// A ref whose tip already carries the trailer is not awaiting anything, so a +// queue that is entirely rewritten must read as no backlog at all. +func TestRunStatus_OPFPendingOmittedWhenTrailered(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, opfBacklogSettingsOn) + testutil.AddRemote(t, ".", originRemoteName, "https://example.com/origin.git") + testutil.WriteFile(t, ".", "a.txt", "one") + testutil.GitAdd(t, ".", "a.txt") + testutil.GitCommit(t, ".", "commit a.txt\n\nEntire-OPF-Applied: true") + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000001", testutil.GetHeadHash(t, ".")) + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + + if out := stdout.String(); strings.Contains(out, "pending OpenAI Privacy Filter redaction") { + t.Errorf("a trailered tip awaits nothing; counter must be omitted, got:\n%s", out) + } +} + +// OPF is off by default, and the default must print exactly what it printed +// before this surface existed — not "similar output minus a line". +// +// Both halves build the identical repo, down to the queued refs, and differ +// only in whether the filter is enabled. Comparing the off output against the +// on output with the OPF line deleted is what makes the claim byte-for-byte: +// every remaining line comes from code this task did not touch, so if the +// disabled path perturbed anything at all — a counter the queue read moved, a +// stray blank line, a reordered section — the two would not match. A repo with +// no backlog at all would not prove that, because it also differs in the +// unpushed counter, which is not ours. +func TestRunStatus_OPFBacklogSilentWhenOPFDisabled(t *testing.T) { + statusWithOPFSetting := func(t *testing.T, opfSettings string) string { + t.Helper() + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, opfSettings) + testutil.AddRemote(t, ".", originRemoteName, "https://example.com/origin.git") + head := checkpointSyncTestCommit(t, "a.txt", "one") + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000001", head) + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000002", head) + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + return stdout.String() + } + + var disabled, enabled string + t.Run("opf disabled", func(t *testing.T) { disabled = statusWithOPFSetting(t, opfBacklogSettingsOff) }) + t.Run("opf enabled", func(t *testing.T) { enabled = statusWithOPFSetting(t, opfBacklogSettingsOn) }) + + const phrase = "OpenAI Privacy Filter" + if strings.Contains(disabled, phrase) { + t.Errorf("OPF off must not mention %q, got:\n%s", phrase, disabled) + } + if !strings.Contains(enabled, phrase) { + t.Fatalf("test is not exercising the OPF line: %q missing from:\n%s", phrase, enabled) + } + + var kept []string + for _, line := range strings.Split(enabled, "\n") { + if strings.Contains(line, phrase) { + continue + } + kept = append(kept, line) + } + if got := strings.Join(kept, "\n"); got != disabled { + t.Errorf("OPF off must be byte-identical to OPF on minus the OPF line:\ndisabled:\n%q\nenabled minus OPF line:\n%q", + disabled, got) + } +} + +func TestRunStatusJSON_OPFBacklog(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, opfBacklogSettingsOn) + testutil.AddRemote(t, ".", originRemoteName, "https://example.com/origin.git") + head := checkpointSyncTestCommit(t, "a.txt", "one") + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000001", head) + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000002", head) + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, true); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + + var result statusJSON + if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { + t.Fatalf("json.Unmarshal() error = %v", err) + } + if result.CheckpointOPFPending != 2 { + t.Errorf("checkpoint_opf_pending = %d, want 2", result.CheckpointOPFPending) + } +} + +func TestRunStatusJSON_OPFBacklogAbsentWhenOPFDisabled(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, opfBacklogSettingsOff) + testutil.AddRemote(t, ".", originRemoteName, "https://example.com/origin.git") + head := checkpointSyncTestCommit(t, "a.txt", "one") + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000001", head) + queueCheckpointRefAwaitingOPF(t, "refs/entire/checkpoints/aa/bb0000000002", head) + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, true); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + + var result statusJSON + if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { + t.Fatalf("json.Unmarshal() error = %v", err) + } + if result.CheckpointOPFPending != 0 { + t.Errorf("OPF off: field must be absent, got pending=%d", result.CheckpointOPFPending) + } + if strings.Contains(stdout.String(), "checkpoint_opf") { + t.Errorf("omitempty should drop the OPF field entirely, got: %s", stdout.String()) + } +} diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 3d72a07628..94cac2b50b 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "sort" "strings" @@ -33,9 +34,10 @@ const ( CleanupTypeShadowBranch CleanupType = "shadow-branch" CleanupTypeSessionState CleanupType = "session-state" CleanupTypeCheckpoint CleanupType = "checkpoint" - // CleanupTypeRedactCache is the redaction prefix cache in the git common dir. - // Purely derived data -- it is rebuilt on the next checkpoint -- so it is - // removed wholesale rather than per entry. + // CleanupTypeRedactCache is a derived cache in the git common dir: the + // redaction prefix cache or the OPF span cache (the item ID names which). + // Purely derived data -- rebuilt on the next checkpoint or scan -- so each + // is removed wholesale rather than per entry. CleanupTypeRedactCache CleanupType = "redact-cache" ) @@ -342,28 +344,40 @@ func ListAllItems(ctx context.Context) ([]CleanupItem, error) { } // The redaction prefix cache accumulates one small entry per session and is - // never superseded, so without this it would survive every `entire clean`. - if dir, err := redactCacheDir(ctx); err == nil && dir != "" { - if _, statErr := os.Stat(dir); statErr == nil { - cleanupItems = append(cleanupItems, CleanupItem{ - Type: CleanupTypeRedactCache, - ID: checkpoint.RedactCacheDirName, - Reason: cleanAllReason, - }) + // never superseded, and the OPF span cache is pruned only by a scan worker, + // which never runs once OPF is off. Without this both would survive every + // `entire clean`. + for _, name := range derivedCacheDirNames { + if dir, err := derivedCacheDir(ctx, name); err == nil && dir != "" { + if _, statErr := os.Stat(dir); statErr == nil { + cleanupItems = append(cleanupItems, CleanupItem{ + Type: CleanupTypeRedactCache, + ID: name, + Reason: cleanAllReason, + }) + } } } return cleanupItems, nil } +// derivedCacheDirNames are the git-common-dir caches `entire clean --all` +// reclaims; deletion accepts no other name. +var derivedCacheDirNames = []string{checkpoint.RedactCacheDirName, checkpoint.OPFSpanCacheDirName} + // redactCacheDir resolves the redaction prefix cache directory, or "" when the // git common dir cannot be resolved. func redactCacheDir(ctx context.Context) (string, error) { + return derivedCacheDir(ctx, checkpoint.RedactCacheDirName) +} + +func derivedCacheDir(ctx context.Context, name string) (string, error) { commonDir, err := session.GetGitCommonDir(ctx) if err != nil { return "", fmt.Errorf("resolve git common dir: %w", err) } - return filepath.Join(commonDir, checkpoint.RedactCacheDirName), nil + return filepath.Join(commonDir, name), nil } // DeleteAllCleanupItems deletes all specified cleanup items. @@ -393,18 +407,20 @@ func DeleteAllCleanupItems(ctx context.Context, items []CleanupItem) (*CleanupRe } } - // Remove the redaction prefix cache. Derived data, so a failure is recorded - // but never blocks the rest of the cleanup. - if len(redactCaches) > 0 { - if err := deleteRedactCache(ctx); err != nil { - result.FailedRedactCache = redactCaches - logging.Warn(logCtx, "failed to delete redaction cache", + // Remove the derived caches. Derived data, so a failure is recorded but + // never blocks the rest of the cleanup. + for _, name := range redactCaches { + if err := deleteDerivedCache(ctx, name); err != nil { + result.FailedRedactCache = append(result.FailedRedactCache, name) + logging.Warn(logCtx, "failed to delete derived cache", slog.String("type", string(CleanupTypeRedactCache)), + slog.String("cache", name), slog.String("error", err.Error())) } else { - result.RedactCaches = redactCaches - logging.Info(logCtx, "deleted redaction cache", - slog.String("type", string(CleanupTypeRedactCache))) + result.RedactCaches = append(result.RedactCaches, name) + logging.Info(logCtx, "deleted derived cache", + slog.String("type", string(CleanupTypeRedactCache)), + slog.String("cache", name)) } } @@ -507,7 +523,16 @@ func DeleteAllCleanupItems(ctx context.Context, items []CleanupItem) (*CleanupRe // derived data rebuilt on the next checkpoint, so removing the whole directory is // always safe; a missing directory is not an error. func deleteRedactCache(ctx context.Context) error { - dir, err := redactCacheDir(ctx) + return deleteDerivedCache(ctx, checkpoint.RedactCacheDirName) +} + +// deleteDerivedCache removes one of derivedCacheDirNames. A missing directory +// is not an error. +func deleteDerivedCache(ctx context.Context, name string) error { + if !slices.Contains(derivedCacheDirNames, name) { + return fmt.Errorf("not a derived cache: %q", name) + } + dir, err := derivedCacheDir(ctx, name) if err != nil { return err } @@ -515,8 +540,8 @@ func deleteRedactCache(ctx context.Context) error { if err != nil { return fmt.Errorf("open git common dir: %w", err) } - if err := root.RemoveAll(checkpoint.RedactCacheDirName); err != nil { - return fmt.Errorf("remove redaction cache %s: %w", dir, err) + if err := root.RemoveAll(name); err != nil { + return fmt.Errorf("remove derived cache %s: %w", dir, err) } return nil } diff --git a/cmd/entire/cli/strategy/cleanup_redact_cache_test.go b/cmd/entire/cli/strategy/cleanup_redact_cache_test.go index ab04432fde..cc0e3a5794 100644 --- a/cmd/entire/cli/strategy/cleanup_redact_cache_test.go +++ b/cmd/entire/cli/strategy/cleanup_redact_cache_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/entireio/cli/cmd/entire/cli/checkpoint" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/stretchr/testify/require" @@ -65,6 +66,33 @@ func TestDeleteRedactCache_AbsentDirIsNotAnError(t *testing.T) { require.NoError(t, deleteRedactCache(context.Background())) } +// The OPF span cache is pruned only by a scan worker, which never runs again +// once OPF is off, so clean all must reclaim it too. +// +// Not parallel: t.Chdir sets process-global state. +func TestOPFSpanCache_ListedAndDeletedByCleanAll(t *testing.T) { + tmpDir := t.TempDir() + testutil.InitRepo(t, tmpDir) + testutil.WriteFile(t, tmpDir, "f.txt", "init") + testutil.GitAdd(t, tmpDir, "f.txt") + testutil.GitCommit(t, tmpDir, "init") + t.Chdir(tmpDir) + ctx := context.Background() + + dir, err := derivedCacheDir(ctx, checkpoint.OPFSpanCacheDirName) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(dir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "deadbeef.json"), []byte(`{}`), 0o600)) + + items, err := ListAllItems(ctx) + require.NoError(t, err) + result, err := DeleteAllCleanupItems(ctx, items) + require.NoError(t, err) + require.Contains(t, result.RedactCaches, checkpoint.OPFSpanCacheDirName) + require.Empty(t, result.FailedRedactCache) + require.NoDirExists(t, dir, "clean all must reclaim the OPF span cache") +} + func cleanupItemTypes(items []CleanupItem) []CleanupType { types := make([]CleanupType, 0, len(items)) for _, item := range items { diff --git a/cmd/entire/cli/strategy/hooks.go b/cmd/entire/cli/strategy/hooks.go index fa00d95bcf..b339eef882 100644 --- a/cmd/entire/cli/strategy/hooks.go +++ b/cmd/entire/cli/strategy/hooks.go @@ -40,6 +40,10 @@ const missingEntireGitHookWarning = "[entire] Entire CLI is enabled but not inst // Entire branches on by name (see below). const postRewriteHook = "post-rewrite" +// prePushHook is named on its own because its state check looks for +// PrePushStdinRefsEnv. +const prePushHook = "pre-push" + // gitHookNames are the git hooks managed by Entire CLI var gitHookNames = []string{"prepare-commit-msg", "commit-msg", "post-commit", postRewriteHook, "pre-push"} @@ -525,6 +529,11 @@ func gitHookStateInHooksDir(hooksDir string) GitHookState { if entireHookLineRunsFromWorkingTree(content) { outdated = true } + if hook == prePushHook && !strings.Contains(content, PrePushStdinRefsEnv+"=1") { + // Predates reading git's ref list, so it cannot stop a push that + // sends unverified checkpoint content itself. + outdated = true + } } if outdated { return GitHooksOutdated @@ -580,7 +589,7 @@ func buildHookSpecs(cmdPrefix string) []hookSpec { // the user opted into by enabling OPF. Users hit by unrelated // bugs can `ENTIRE_OPF=no git push` for a one-off bypass while // the bug is fixed. - prePushCmd := gitHookCommand(cmdPrefix, `pre-push "$1"`, false) + prePushCmd := prePushHookLine(cmdPrefix) return []hookSpec{ { @@ -925,6 +934,41 @@ fi `, chainComment, backupSuffix, backupSuffix) } +// PrePushStdinRefsEnv, set to 1, tells `entire hooks git pre-push` to read the +// refs being pushed from stdin. It is an environment variable rather than a +// flag so that an older entire binary run by a newer script (a downgrade, or a +// second install earlier on PATH) ignores it instead of failing every push. +// prePushHookLine sets it; an installed script without it is outdated (see +// gitHookStateInHooksDir), and a hand-copied hook-manager line without it gets +// a warning when it matters (see warnPrePushRefsUnknown). +const PrePushStdinRefsEnv = "ENTIRE_PRE_PUSH_STDIN_REFS" + +// prePushHookLine is the pre-push invocation, as one line so a hook manager +// such as Husky can paste it into a script with other commands. Entire reads +// git's ref list from stdin (PrePushStdinRefsEnv), and so does whatever runs +// after it, typically `git lfs pre-push`. The line therefore saves the list, +// gives Entire the copy, and points the script's stdin back at the copy for +// the commands after it. Entire's failure ends the script there: a later +// command's exit status must not decide the push and lose a privacy abort. +// +// Saving the list must never be what blocks a push. When the temp file cannot +// be created or written, Entire runs without the list, as an old hook would: +// it still applies OPF to its own push and warns when an outer push cannot be +// checked. A partly written copy is discarded, not replayed: the commands +// after Entire then read whatever git's stdin still holds. +func prePushHookLine(cmdPrefix string) string { + withRefs := fmt.Sprintf(`%s=1 %s hooks git pre-push "$1" < "$_entire_refs"`, PrePushStdinRefsEnv, cmdPrefix) + withoutRefs := cmdPrefix + ` hooks git pre-push "$1" < /dev/null` + // Without Entire installed the line does nothing, stdin included. + return fmt.Sprintf(`if %s; then `, gitHookCommandAvailableTest(cmdPrefix)) + + `if _entire_refs="$(mktemp "${TMPDIR:-/tmp}/entire-pre-push.XXXXXX" 2>/dev/null)"; then ` + + `if cat > "$_entire_refs"; then ` + + withRefs + ` || { _entire_status=$?; rm -f "$_entire_refs"; exit "$_entire_status"; }; ` + + `exec < "$_entire_refs"; rm -f "$_entire_refs"; ` + + `else rm -f "$_entire_refs"; ` + withoutRefs + ` || exit $?; fi; ` + + `else ` + withoutRefs + ` || exit $?; fi; fi` +} + // hookCmdPrefix returns the command prefix for hook scripts and warning messages. // When absolutePath is true, resolves the full binary path via os.Executable() // and returns an error if resolution fails. This is needed for GUI git clients diff --git a/cmd/entire/cli/strategy/hooks_test.go b/cmd/entire/cli/strategy/hooks_test.go index f00f5f8cf5..8c3e574353 100644 --- a/cmd/entire/cli/strategy/hooks_test.go +++ b/cmd/entire/cli/strategy/hooks_test.go @@ -3,6 +3,7 @@ package strategy import ( "context" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -664,7 +665,7 @@ func TestCheckGitHookState_UserAdditionsAreNotDrift(t *testing.T) { dir := t.TempDir() for _, hook := range gitHookNames { content := "#!/bin/sh\n# " + entireHookMarker + "\n" + - "if command -v entire >/dev/null 2>&1; then entire hooks git " + hook + "; else :; fi\n" + + "if command -v entire >/dev/null 2>&1; then " + currentHookCommand(hook) + "; else :; fi\n" + userLine + "\n" if err := os.WriteFile(filepath.Join(dir, hook), []byte(content), 0o755); err != nil { t.Fatal(err) @@ -701,13 +702,21 @@ func TestCheckGitHookState_LegacyEntireLineIsStillDrift(t *testing.T) { func writeCurrentManagedHooks(t *testing.T, dir string) { t.Helper() for _, hook := range gitHookNames { - content := "#!/bin/sh\n# " + entireHookMarker + "\nentire hooks git " + hook + "\n" + content := "#!/bin/sh\n# " + entireHookMarker + "\n" + currentHookCommand(hook) + "\n" if err := os.WriteFile(filepath.Join(dir, hook), []byte(content), 0o755); err != nil { t.Fatal(err) } } } +// currentHookCommand is the hook's Entire invocation in the current script shape. +func currentHookCommand(hook string) string { + if hook == prePushHook { + return PrePushStdinRefsEnv + `=1 entire hooks git pre-push "$1"` + } + return "entire hooks git " + hook +} + func TestIsGitHookInstalled_LegacyLocalDevCountsAsNotInstalled(t *testing.T) { _, hooksDir := initHooksTestRepo(t) @@ -1677,8 +1686,8 @@ func TestRemoveGitHook_RestoresBackupWhenHookAlreadyGone(t *testing.T) { func TestGenerateChainedContent(t *testing.T) { t.Parallel() - base := "#!/bin/sh\n# Entire CLI hooks\nentire hooks git pre-push \"$1\" || true\n" - result := generateChainedContent(base, "pre-push") + base := "#!/bin/sh\n# Entire CLI hooks\nentire hooks git post-commit || true\n" + result := generateChainedContent(base, "post-commit") // Should start with the base content if !strings.HasPrefix(result, base) { @@ -1696,18 +1705,182 @@ func TestGenerateChainedContent(t *testing.T) { } // Should check executable permission on backup - expectedCheck := `[ -x "$_entire_hook_dir/pre-push` + backupSuffix + `" ]` + expectedCheck := `[ -x "$_entire_hook_dir/post-commit` + backupSuffix + `" ]` if !strings.Contains(result, expectedCheck) { t.Errorf("chained content should check -x on backup, got:\n%s", result) } // Should forward all arguments with "$@" - expectedExec := `"$_entire_hook_dir/pre-push` + backupSuffix + `" "$@"` + expectedExec := `"$_entire_hook_dir/post-commit` + backupSuffix + `" "$@"` if !strings.Contains(result, expectedExec) { t.Errorf("chained content should execute backup with $@, got:\n%s", result) } } +// Whatever runs after Entire's pre-push line (a hook Entire chains to, or the +// next line of a Husky script, typically `git lfs pre-push`) must still receive +// git's ref list on stdin after Entire has read it, and Entire's failure must +// end the script there: otherwise a later command's status decides the push +// and an OPF abort is lost. +// Saving the ref list must never be what blocks a push: when the temp file +// cannot be created or written, Entire runs without the list (as an old hook +// would) and the commands after it still get git's stdin, not a partial copy. +func TestPrePushHookLine_UnwritableTMPDIRFallsBack(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("runs the generated POSIX sh script") + } + const refs = "refs/heads/main 1111111111111111111111111111111111111111 refs/heads/main 2222222222222222222222222222222222222222\n" + for _, tc := range []struct { + name string + catFails bool + entireExit int + }{ + {name: "mktemp fails, entire succeeds", entireExit: 0}, + {name: "mktemp fails, entire fails", entireExit: 3}, + {name: "saving the list fails", catFails: true, entireExit: 0}, + } { + entireExit := tc.entireExit + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + binDir := filepath.Join(dir, "bin") + require.NoError(t, os.MkdirAll(binDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "entire"), + []byte(fmt.Sprintf("#!/bin/sh\nenv > %q\nexit %d\n", filepath.Join(dir, "entire.env"), entireExit)), 0o755)) + tmpDir := filepath.Join(dir, "missing") + if tc.catFails { + // A cat that fails before reading anything, as a full disk would. + require.NoError(t, os.WriteFile(filepath.Join(binDir, "cat"), []byte("#!/bin/sh\nexit 1\n"), 0o755)) + tmpDir = dir + } + script := "#!/bin/sh\n" + prePushHookLine(bareEntireHookCmd) + "\n/bin/cat > " + filepath.Join(dir, "after.stdin") + "\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, prePushHook), []byte(script), 0o755)) + + cmd := exec.CommandContext(t.Context(), "sh", filepath.Join(dir, prePushHook), "origin", "url") + cmd.Stdin = strings.NewReader(refs) + cmd.Env = append(os.Environ(), "PATH="+binDir+string(os.PathListSeparator)+os.Getenv("PATH"), + "TMPDIR="+tmpDir) + err := cmd.Run() + exit := 0 + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exit = exitErr.ExitCode() + } else { + require.NoError(t, err) + } + require.Equal(t, entireExit, exit, "the script's status must be Entire's") + env, readErr := os.ReadFile(filepath.Join(dir, "entire.env")) + require.NoError(t, readErr, "Entire must still run") + require.NotContains(t, string(env), PrePushStdinRefsEnv+"=1", "without a saved list Entire runs as an old hook") + after, afterErr := os.ReadFile(filepath.Join(dir, "after.stdin")) + if entireExit == 0 { + require.NoError(t, afterErr) + require.Equal(t, refs, string(after), "the command after Entire must still get git's stdin") + } else { + require.ErrorIs(t, afterErr, os.ErrNotExist, "nothing may run after Entire fails") + } + leftovers, globErr := filepath.Glob(filepath.Join(dir, "entire-pre-push.*")) + require.NoError(t, globErr) + require.Empty(t, leftovers, "a failed copy must be removed") + }) + } +} + +func TestPrePushHookLine_ReplaysStdinAndPropagatesFailure(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("runs the generated POSIX sh script") + } + + const refs = "refs/heads/main 1111111111111111111111111111111111111111 refs/heads/main 2222222222222222222222222222222222222222\n" + var installed string + for _, spec := range buildHookSpecs(bareEntireHookCmd) { + if spec.name == prePushHook { + installed = spec.content + } + } + require.NotEmpty(t, installed) + + for _, shape := range []struct { + name string + script func(dir string) string + after func(dir string) string // where the command after Entire records its stdin + }{ + { + name: "installed script chaining to a backed-up hook", + script: func(string) string { return generateChainedContent(installed, prePushHook) }, + after: func(dir string) string { return filepath.Join(dir, "chained.stdin") }, + }, + { + name: "Husky script with the printed line followed by another command", + script: func(dir string) string { + return "#!/bin/sh\n" + extractCommandLine(installed) + "\ncat > " + filepath.Join(dir, "chained.stdin") + "\n" + }, + after: func(dir string) string { return filepath.Join(dir, "chained.stdin") }, + }, + } { + for _, tc := range []struct { + name string + entireExit int + wantAfter bool + }{ + {name: "entire succeeds", entireExit: 0, wantAfter: true}, + {name: "entire fails", entireExit: 3, wantAfter: false}, + } { + t.Run(shape.name+"/"+tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + binDir := filepath.Join(dir, "bin") + require.NoError(t, os.MkdirAll(binDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "entire"), + []byte(fmt.Sprintf("#!/bin/sh\ncat > %q\nexit %d\n", filepath.Join(dir, "entire.stdin"), tc.entireExit)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, prePushHook), []byte(shape.script(dir)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, prePushHook+backupSuffix), + []byte(fmt.Sprintf("#!/bin/sh\ncat > %q\n", filepath.Join(dir, "chained.stdin"))), 0o755)) + + cmd := exec.CommandContext(t.Context(), "sh", filepath.Join(dir, prePushHook), "origin", "url") + cmd.Stdin = strings.NewReader(refs) + cmd.Env = append(os.Environ(), "PATH="+binDir+string(os.PathListSeparator)+os.Getenv("PATH"), "TMPDIR="+dir) + err := cmd.Run() + exit := 0 + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exit = exitErr.ExitCode() + } else { + require.NoError(t, err) + } + require.Equal(t, tc.entireExit, exit, "the script's status must be Entire's") + + got, readErr := os.ReadFile(filepath.Join(dir, "entire.stdin")) + require.NoError(t, readErr) + require.Equal(t, refs, string(got), "entire must read git's ref list") + after, afterErr := os.ReadFile(shape.after(dir)) + if tc.wantAfter { + require.NoError(t, afterErr) + require.Equal(t, refs, string(after), "the command after Entire must get the same ref list") + } else { + require.ErrorIs(t, afterErr, os.ErrNotExist, "nothing may run after Entire fails") + } + leftovers, globErr := filepath.Glob(filepath.Join(dir, "entire-pre-push.*")) + require.NoError(t, globErr) + require.Empty(t, leftovers, "the saved ref list must be removed") + }) + } + } +} + +// A pre-push script that does not read git's ref list cannot stop a push that +// sends unverified checkpoint content itself, so it is reinstalled. +func TestCheckGitHookState_PrePushWithoutStdinRefsIsOutdated(t *testing.T) { + t.Parallel() + dir := t.TempDir() + writeCurrentManagedHooks(t, dir) + content := "#!/bin/sh\n# " + entireHookMarker + "\nentire hooks git pre-push \"$1\"\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, prePushHook), []byte(content), 0o755)) + require.Equal(t, GitHooksOutdated, gitHookStateInHooksDir(dir)) +} + func TestGenerateChainedContent_PostRewritePreservesStdinForBackup(t *testing.T) { t.Parallel() diff --git a/cmd/entire/cli/strategy/manual_commit_opf_prompt.go b/cmd/entire/cli/strategy/manual_commit_opf_prompt.go index d1a10d22d1..7e34b8fdbc 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_prompt.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_prompt.go @@ -98,7 +98,7 @@ func resolveOPFDecisionForPrePush(ctx context.Context, opf *settings.OPFSettings return OPFAbort, err } if d == OPFRun && !hasTTY { - fmt.Fprintln(errOut, "→ OpenAI Privacy Filter: scanning checkpoints before push (may take ~30s)…") + fmt.Fprintln(errOut, "→ OpenAI Privacy Filter: enabled for checkpoint processing") } return d, nil } @@ -122,7 +122,7 @@ func askOPFPrompt(ctx context.Context) (OPFDecision, error) { huh.NewGroup( huh.NewSelect[string](). Title("Run OpenAI Privacy Filter on these checkpoints?"). - Description("Adds ~30s but redacts names/PII the regex layers can't catch. Ctrl-C to cancel the push."). + Description("Scans in the background after this push and sends checkpoints when done; redacts names/PII the regex layers can't catch. Ctrl-C to cancel the push."). Options( huh.NewOption("Yes — run OPF this push", choiceYes), huh.NewOption("No — skip OPF, push as-is", choiceNo), diff --git a/cmd/entire/cli/strategy/manual_commit_opf_prompt_test.go b/cmd/entire/cli/strategy/manual_commit_opf_prompt_test.go index 2fa069192c..64310833ec 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_prompt_test.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_prompt_test.go @@ -200,7 +200,7 @@ func TestPrePush_OPFProgressUsesConfiguredWriter(t *testing.T) { withOPFPrePushProgressWriterForTest(t, &out) require.NoError(t, (&ManualCommitStrategy{}).PrePush(t.Context(), "origin")) - require.Contains(t, out.String(), "OpenAI Privacy Filter: scanning checkpoints before push") + require.Contains(t, out.String(), "OpenAI Privacy Filter: enabled for checkpoint processing") } func withOPFPrePushProgressWriterForTest(t testing.TB, w io.Writer) { diff --git a/cmd/entire/cli/strategy/manual_commit_opf_refs.go b/cmd/entire/cli/strategy/manual_commit_opf_refs.go index 7cc7fbc715..64e5bff8f0 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_refs.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_refs.go @@ -49,6 +49,14 @@ import ( // the same error taxonomy as RewriteUnpushedV1WithOPF; delivery fails closed by // withholding any ref whose current generation still lacks the trailer. func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repository) error { + return rewriteQueuedCheckpointRefsWithOPF(ctx, repo, opfScanThenApply) +} + +// rewriteQueuedCheckpointRefsWithOPF is RewriteQueuedCheckpointRefsWithOPF with +// an explicit mode. Under opfApplyCachedOnly it never runs the model: a ref +// whose content the scan worker has not covered yet is left untouched and +// reported as *OPFScanPendingError, while its covered siblings are rewritten. +func rewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repository, mode opfRewriteMode) error { queue, err := checkpoint.PushQueueForRepo(ctx, repo) if err != nil { return fmt.Errorf("resolve push queue: %w", err) @@ -74,7 +82,7 @@ func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repositor if redact.OPFMisconfiguredNoCategories() { return &OPFNoCategoriesError{} } - if redact.OPFBreakerTripped() { + if mode == opfScanThenApply && redact.OPFBreakerTripped() { return &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand()} } @@ -85,6 +93,7 @@ func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repositor // the queue belongs to the flush. existing, _ := partitionLocalRefs(repo, queued) var firstErr error + var scanPending *OPFScanPendingError for _, refName := range existing { // Whole-flush stop, alongside ErrOPFNoEnabledCategories below: a // tripped process-wide breaker (this loop's own prior ref, or anything @@ -94,7 +103,7 @@ func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repositor // and stamp Entire-OPF-Applied on it. Per-ref isolation covers per-ref // conditions (the size cap); a broken runtime is broken for the whole // process. Refs already rewritten above were scanned before the trip. - if redact.OPFBreakerTripped() { + if mode == opfScanThenApply && redact.OPFBreakerTripped() { // Replaces any earlier per-ref error: a broken runtime is what // withholds every remaining ref, so it is the error to report. firstErr = &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand()} @@ -111,7 +120,7 @@ func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repositor } continue } - rewriteErr := rewriteCollectedCheckpointRefWithOPF(ctx, repo, queue, pending, batchLimit) + rewriteErr := rewriteCollectedCheckpointRefWithOPF(ctx, repo, queue, pending, batchLimit, mode) if rewriteErr == nil { continue } @@ -132,11 +141,26 @@ func RewriteQueuedCheckpointRefsWithOPF(ctx context.Context, repo *git.Repositor } break } + // Pending is kept apart from the other errors so neither can hide the + // other, in either queue order: the pending signal is what starts the + // scan worker, and a cap error is what tells the user a ref is stuck. + var pendingErr *OPFScanPendingError + if errors.As(rewriteErr, &pendingErr) { + scanPending = pendingErr + continue + } if firstErr == nil { firstErr = rewriteErr } } - return firstErr + switch { + case scanPending == nil: + return firstErr + case firstErr == nil: + return scanPending + default: + return errors.Join(firstErr, scanPending) + } } type pendingOPFCommit struct { @@ -201,6 +225,7 @@ func rewriteCollectedCheckpointRefWithOPF( queue *checkpoint.PushQueue, pending *pendingOPFRef, batchLimit int, + mode opfRewriteMode, ) error { var blobs []redact.NamedBlob for _, commit := range pending.commits { @@ -213,12 +238,12 @@ func rewriteCollectedCheckpointRefWithOPF( var redacted [][]byte if len(blobs) > 0 { var err error - redacted, err = redact.BatchBytesWithPrivacyFilter(ctx, blobs) + redacted, err = redactBlobsForOPFRewrite(ctx, repo, blobs, mode) if err != nil { if errors.Is(err, redact.ErrOPFNoEnabledCategories) { return fmt.Errorf("scan checkpoint ref %s: %w", pending.ref, err) } - return &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand(), Cause: err} + return err } } diff --git a/cmd/entire/cli/strategy/manual_commit_opf_rewrite.go b/cmd/entire/cli/strategy/manual_commit_opf_rewrite.go index a3555aab6a..67a21b60db 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_rewrite.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_rewrite.go @@ -157,15 +157,11 @@ func resolveBootstrapLimit() int { return bootstrapDefaultLimit } -// OPFBatchTooLargeError: one OPF pass — a single checkpoint ref on -// git-refs, the whole unpushed v1 chain on git-branch — has more -// prose-leaf content than ENTIRE_OPF_BATCH_LIMIT will allow OPF to chew -// through in one inference call. Pushing under the limit yields a single -// ~10-30s pause; without the cap, a 100MB-of-prose push could take an hour. -// -// The user-facing remediation is identical in shape to -// BootstrapTooLargeError: bump the limit, push without OPF, or break -// the push into smaller pieces. +// OPFBatchTooLargeError: one OPF unit of work — a single checkpoint ref on +// git-refs, the whole unpushed v1 chain on git-branch — has more prose-leaf +// content than ENTIRE_OPF_BATCH_LIMIT allows. See batchDefaultLimit: this is a +// backstop against broken input, so hitting it says something about the +// content, not about the size of the session. type OPFBatchTooLargeError struct { LeafBytes int Limit int @@ -173,17 +169,26 @@ type OPFBatchTooLargeError struct { func (e *OPFBatchTooLargeError) Error() string { return fmt.Sprintf("OPF would run inference on %d prose-leaf bytes "+ - "(limit %d). Set ENTIRE_OPF_BATCH_LIMIT= or =unlimited to override, "+ - "or push without OPF (ENTIRE_OPF=no git push) and let a smaller follow-up push run OPF", + "(limit %d). No real session should reach this limit, so check what this "+ + "content is — a corrupted transcript or an accidentally-embedded binary is "+ + "the likely explanation. If it really is content you want redacted, set "+ + "ENTIRE_OPF_BATCH_LIMIT= or =unlimited to override, or push without "+ + "OPF (ENTIRE_OPF=no git push)", e.LeafBytes, e.Limit) } const ( - // batchDefaultLimit caps the cumulative prose-leaf bytes one push - // will hand to OPF. 2 MB at ~5.4s/100KB ≈ ~110s of inference, on - // the high end of what's acceptable as a single push pause but - // generous enough that any realistic push fits. - batchDefaultLimit = 2 * 1024 * 1024 + // batchDefaultLimit caps the prose-leaf bytes of one OPF unit of work: + // one checkpoint ref on git-refs, the whole unpushed v1 chain on + // git-branch. It is a backstop against broken input (a corrupted + // transcript, an embedded binary, a multi-GiB paste), not a speed target: + // the model runs in the background scan worker in 1 MiB calls, so a large + // real session only takes longer. The largest real sessions measured hold + // a few MiB of prose (docs/development/opf-bug-investigation.md), so + // 128 MiB is far above anything real and well below redact's 256 MiB + // allocation wall, which keeps this check the one with an actionable + // message. + batchDefaultLimit = 128 * 1024 * 1024 batchEnvVar = "ENTIRE_OPF_BATCH_LIMIT" ) @@ -240,12 +245,87 @@ func (e *OPFRawBytesTooLargeError) Error() string { e.RawBytes, e.Limit, rawByteCapMultiplier) } -// rawByteCapMultiplier ties the raw-byte RAM ceiling to the leaf-byte -// inference cap. 100× means: leaf cap 2 MiB → raw ceiling 200 MiB. -// Picked to comfortably exceed any realistic JSON-scaffolding ratio -// (a 10 MiB JSONL with 300 KB of leaves still fits) while preventing -// pathological RAM blowups (a 5 GiB pasted dump aborts before loading). -const rawByteCapMultiplier = 100 +// rawByteCapMultiplier ties the raw-byte RAM ceiling to the leaf-byte cap +// so one env var moves both: default leaf cap 128 MiB → raw ceiling 256 MiB. +// The raw ceiling is the sized quantity (what one process may buffer at +// once); the multiplier only preserves the order the error messages rely on. +// Real transcripts run ~1.2× raw bytes per prose-leaf byte, so real content +// at the leaf cap trips the leaf cap first. +const rawByteCapMultiplier = 2 + +// opfRewriteMode says whether a rewrite may run the OPF model itself. +type opfRewriteMode int + +const ( + // opfScanThenApply scans whatever the span cache lacks, then applies it. + // Used where waiting for the model is the point: the background scan + // worker and the explicit migration push. + opfScanThenApply opfRewriteMode = iota + // opfApplyCachedOnly never runs the model. The pre-push hook uses it so a + // user's git push never waits on inference; content the worker has not + // scanned yet surfaces as *OPFScanPendingError. + opfApplyCachedOnly +) + +// OPFScanPendingError: the content to push has not been scanned by OPF yet. +// Nothing was rewritten. It is not a failure — the scan worker is running or +// about to, and delivers the content itself once the scan finishes. +type OPFScanPendingError struct{} + +func (e *OPFScanPendingError) Error() string { + return "the OpenAI Privacy Filter has not finished scanning these checkpoints" +} + +// OPFCacheUnavailableError: the OPF span cache in the git common dir cannot be +// opened. Pre-push can then neither use earlier scan results nor hand work to +// the scan worker, which needs the same cache, so the content is withheld with +// the real cause instead of being reported as a scan in progress. +type OPFCacheUnavailableError struct { + Cause error +} + +func (e *OPFCacheUnavailableError) Error() string { + return fmt.Sprintf("cannot use the OpenAI Privacy Filter scan cache (%v); checkpoints are held "+ + "until it is fixed. Check permissions and free space in the repository's .git directory, "+ + "or set ENTIRE_OPF=no on the push to skip OPF for this push only", e.Cause) +} + +func (e *OPFCacheUnavailableError) Unwrap() error { return e.Cause } + +// redactBlobsForOPFRewrite produces the OPF-redacted bytes for blobs through the +// span cache in the git common dir. Without a usable cache it falls back to a +// one-pass scan when mode allows the model, and returns *OPFCacheUnavailableError +// when it does not. +func redactBlobsForOPFRewrite(ctx context.Context, repo *git.Repository, blobs []redact.NamedBlob, mode opfRewriteMode) ([][]byte, error) { + cache, cacheErr := checkpoint.OPFSpanCacheForRepo(repo) + if cacheErr != nil { + if mode == opfApplyCachedOnly { + return nil, &OPFCacheUnavailableError{Cause: cacheErr} + } + out, err := redact.BatchBytesWithPrivacyFilter(ctx, blobs) + return out, opfRewriteRedactError(err) + } + if mode == opfScanThenApply { + if err := redact.ScanBlobsWithPrivacyFilter(ctx, blobs, cache); err != nil { + return nil, opfRewriteRedactError(err) + } + } + out, err := redact.ApplyCachedPrivacyFilter(blobs, cache) + if errors.Is(err, redact.ErrOPFScanPending) { + return nil, &OPFScanPendingError{} + } + return out, opfRewriteRedactError(err) +} + +// opfRewriteRedactError maps a redact-layer failure onto the rewrite's error +// taxonomy. ErrOPFNoEnabledCategories passes through for callers to convert to +// their config-remediation error; anything else is a runtime failure. +func opfRewriteRedactError(err error) error { + if err == nil || errors.Is(err, redact.ErrOPFNoEnabledCategories) { + return err + } + return &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand(), Cause: err} +} // RewriteUnpushedV1WithOPF re-redacts unpushed entire/checkpoints/v1 // commits with OPF, builds new commits carrying Entire-OPF-Applied: @@ -259,6 +339,14 @@ const rawByteCapMultiplier = 100 // privacy-critical failures — the pre-push hook propagates these so // git push aborts. func RewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target string) (plumbing.Hash, error) { + return rewriteUnpushedV1WithOPF(ctx, repo, target, opfScanThenApply) +} + +// rewriteUnpushedV1WithOPF is RewriteUnpushedV1WithOPF with an explicit mode. +// Under opfApplyCachedOnly it never runs the model and returns +// *OPFScanPendingError, with the local ref untouched, when any unpushed commit +// holds content the scan worker has not covered yet. +func rewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target string, mode opfRewriteMode) (plumbing.Hash, error) { localTip, err := readV1Tip(repo, plumbing.NewBranchReferenceName(paths.MetadataBranchName)) if err != nil { return plumbing.ZeroHash, fmt.Errorf("read local v1: %w", err) @@ -309,7 +397,7 @@ func RewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target // before tagging any commits as OPF-applied. Without this, the // per-blob fallback inside the no-OPF cases of BatchBytesWithPrivacyFilter // could let regex-only content slip out with the trailer attached. - if redact.OPFBreakerTripped() { + if mode == opfScanThenApply && redact.OPFBreakerTripped() { return plumbing.ZeroHash, &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand()} } @@ -330,11 +418,10 @@ func RewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target pendings := make([]pendingCommit, 0, len(unpushed)) // Bound raw-bytes-in-memory incrementally so a pathological push // (e.g. 5 GiB of pasted dumps) aborts before exhausting the user's - // shell RAM. The leaf-byte cap downstream is about inference cost; - // this one is about memory ceiling and fires earlier. + // shell RAM. The leaf-byte cap downstream backstops implausible + // content; this one is a memory ceiling and fires earlier. // rawByteCapForBatchLimit saturates at math.MaxInt so "unlimited" - // actually means unlimited — without saturation, "unlimited" × 100 - // overflows int and the cap trips on every push. + // actually means unlimited instead of overflowing. rawCap := rawByteCapForBatchLimit(resolveBatchLimit()) rawBudget := newOPFRawByteBudget(rawCap) for _, c := range unpushed { @@ -378,7 +465,7 @@ func RewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target if limit := resolveBatchLimit(); leafBytes > limit { return plumbing.ZeroHash, &OPFBatchTooLargeError{LeafBytes: leafBytes, Limit: limit} } - globalRedacted, err = redact.BatchBytesWithPrivacyFilter(ctx, globalBlobs) + globalRedacted, err = redactBlobsForOPFRewrite(ctx, repo, globalBlobs, mode) if err != nil { // Converge with the up-front gate: the misconfiguration // sentinel must surface config remediation, not "verify @@ -386,7 +473,7 @@ func RewriteUnpushedV1WithOPF(ctx context.Context, repo *git.Repository, target if errors.Is(err, redact.ErrOPFNoEnabledCategories) { return plumbing.ZeroHash, &OPFNoCategoriesError{} } - return plumbing.ZeroHash, &OPFRuntimeFailedError{OPFCommand: redact.OPFCommand(), Cause: err} + return plumbing.ZeroHash, err } } @@ -429,7 +516,11 @@ func readV1Tip(repo *git.Repository, refName plumbing.ReferenceName) (plumbing.H // opfRewriteFetchTmpRef is the temp ref used to stage the URL-fetched // remote v1 tip during OPF rewrite. Cleaned up at the end of each // resolveRemoteV1Tip call so the tracking is invisible to the user. -const opfRewriteFetchTmpRef = FetchTmpRefPrefix + "opf-rewrite-v1" +// +// It is per process: the scan worker's delivery and a user's push can resolve +// the remote tip at the same time, and a shared name let one delete the ref +// while the other read it, which reads as "remote has no v1" (a bootstrap). +var opfRewriteFetchTmpRef = FetchTmpRefPrefix + "opf-rewrite-v1-" + strconv.Itoa(os.Getpid()) //nolint:gochecknoglobals // fixed per process // resolveRemoteV1Tip returns the hash of the remote's // entire/checkpoints/v1 tip. @@ -685,7 +776,7 @@ func collectTreeBlobsWithinBudget( if pathPrefix != "" { fullPath = pathPrefix + "/" + e.Name } - *blobs = append(*blobs, redact.NamedBlob{Name: e.Name, Content: content}) + *blobs = append(*blobs, redact.NamedBlob{Name: e.Name, Content: content, ID: e.Hash.String()}) *paths = append(*paths, fullPath) case filemode.Empty, filemode.Deprecated, filemode.Symlink, filemode.Submodule: // Non-blob/non-tree entries are not redactable blobs. diff --git a/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go b/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go index 87e13e5e05..69a9f8178d 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "os" - "os/exec" "path/filepath" "strconv" "strings" @@ -312,7 +311,7 @@ func TestRewriteUnpushedV1WithOPF_NoEnabledCategoriesAbortsThenRepairs(t *testin require.True(t, trailers.HasOPFApplied(repaired.Message)) } -func TestPrePushFromGitHook_DeferralStillRunsOPF(t *testing.T) { +func TestPrePushFromGitHook_UnscannedV1IsHeldForTheWorker(t *testing.T) { fake := &fakeOPFForRewrite{} configureFakeOPF(t, fake) @@ -326,18 +325,74 @@ func TestPrePushFromGitHook_DeferralStillRunsOPF(t *testing.T) { t.Chdir(dir) paths.ClearWorktreeRootCache() t.Cleanup(paths.ClearWorktreeRootCache) + spawns := swapOPFScanSpawn(t) - // The empty remote defers Entire's automatic metadata push. The OPF rewrite - // still must run because the user's outer git push may include v1 directly. + // Pre-push never runs the model: v1 has not been scanned, so it is held + // back, the user's push succeeds, and the scan worker is started for the + // remote this push named. require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) - + require.Zero(t, fake.batchCallCount(), "pre-push must not call the model") + require.Equal(t, []string{"origin"}, *spawns, "the worker must be spawned for the push's remote") ref, err := repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) require.NoError(t, err) - require.NotEqual(t, originalTip, ref.Hash(), "OPF rewrite must advance the local v1 ref before deferral") - commit, err := repo.CommitObject(ref.Hash()) + require.Equal(t, originalTip, ref.Hash(), "a held v1 must not be rewritten by the hook") +} + +// A pre-push line that does not pass the refs being pushed (an old script, or +// an old Husky line) cannot refuse a `git push --all`, so a held v1 warns the +// user to update it. A line that passes them stays quiet. +func TestPrePushFromGitHook_HeldV1WarnsWhenRefsAreUnknown(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + dir, repo, _ := setupV1RepoInDir(t) + remoteDir := filepath.Join(t.TempDir(), "origin.git") + _, err := git.PlainInit(remoteDir, true) require.NoError(t, err) - require.True(t, trailers.HasOPFApplied(commit.Message)) - require.Equal(t, 1, fake.batchCallCount()) + _, err = repo.CreateRemote(&gitconfig.RemoteConfig{Name: "origin", URLs: []string{remoteDir}}) + require.NoError(t, err) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + swapOPFScanSpawn(t) + + var buf bytes.Buffer + oldWriter := stderrWriter + stderrWriter = &buf + t.Cleanup(func() { stderrWriter = oldWriter }) + + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) + require.Contains(t, buf.String(), "does not pass the refs being pushed") + + buf.Reset() + main := []PrePushRef{{LocalRef: "refs/heads/main", LocalSHA: strings.Repeat("1", 40), RemoteRef: "refs/heads/main"}} + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(WithPrePushRefs(t.Context(), main), "origin")) + require.NotContains(t, buf.String(), "does not pass the refs being pushed") +} + +// git-branch users whose checkpoints are held get one pointer to git-refs, not +// one on every push. +func TestPrePushFromGitHook_HintsGitRefsOnceForHeldV1(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + dir, repo, _ := setupV1RepoInDir(t) + remoteDir := filepath.Join(t.TempDir(), "origin.git") + _, err := git.PlainInit(remoteDir, true) + require.NoError(t, err) + _, err = repo.CreateRemote(&gitconfig.RemoteConfig{Name: "origin", URLs: []string{remoteDir}}) + require.NoError(t, err) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + swapOPFScanSpawn(t) + + var buf bytes.Buffer + oldWriter := stderrWriter + stderrWriter = &buf + t.Cleanup(func() { stderrWriter = oldWriter }) + + for range 2 { + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) + } + require.Equal(t, 1, strings.Count(buf.String(), "migrate-checkpoints"), "the hint must appear once, not per push") + require.Equal(t, 2, strings.Count(buf.String(), opfScanPendingNotice), "every held push still says why") } // Regression: the no-categories abort must reach the git hook boundary. @@ -1037,10 +1092,8 @@ func queuedRefEntries(t *testing.T, repo *git.Repository) []checkpoint.PushQueue // Queued checkpoint refs are OPF-rewritten and stamped applied; a second run is // a no-op because the trailer marks them done (no re-scan, no ref movement). // -// Scanning is scoped per ref — one OPF call per ref rather than one for the -// whole flush — so an oversized ref's cap failure cannot take its siblings down -// with it (see TestRewriteQueuedCheckpointRefsWithOPF_OversizedRefDoesNotBlockOthers). -// The cost of that isolation is N shell-outs instead of 1. +// Scanning is scoped per ref, so an oversized ref's cap failure cannot take its +// siblings down with it (see TestRewriteQueuedCheckpointRefsWithOPF_OversizedRefDoesNotBlockOthers). func TestRewriteQueuedCheckpointRefsWithOPF_RewritesThenIsIdempotent(t *testing.T) { fake := &fakeOPFForRewrite{} configureFakeOPF(t, fake) @@ -1048,7 +1101,9 @@ func TestRewriteQueuedCheckpointRefsWithOPF_RewritesThenIsIdempotent(t *testing. before := refHashes(t, repo, refs) require.NoError(t, RewriteQueuedCheckpointRefsWithOPF(t.Context(), repo)) - require.Equal(t, 2, fake.batchCallCount(), "one OPF call per ref") + // Blobs the refs share are scanned once and then served from the span cache. + require.GreaterOrEqual(t, fake.batchCallCount(), 1) + firstRunCalls := fake.batchCallCount() after := refHashes(t, repo, refs) entries := queuedRefEntries(t, repo) @@ -1064,7 +1119,7 @@ func TestRewriteQueuedCheckpointRefsWithOPF_RewritesThenIsIdempotent(t *testing. } require.NoError(t, RewriteQueuedCheckpointRefsWithOPF(t.Context(), repo)) - require.Equal(t, 2, fake.batchCallCount(), "already-applied refs must not be re-scanned") + require.Equal(t, firstRunCalls, fake.batchCallCount(), "already-applied refs must not be re-scanned") require.Equal(t, after, refHashes(t, repo, refs), "already-applied refs must keep their exact hash") } @@ -1184,7 +1239,7 @@ func TestRewriteQueuedCheckpointRefsWithOPF_RawByteCapIsPerRef(t *testing.T) { before := refHashes(t, repo, refs) require.NoError(t, RewriteQueuedCheckpointRefsWithOPF(t.Context(), repo)) - require.Equal(t, 2, fake.batchCallCount(), "each fitting ref must reach OPF") + require.GreaterOrEqual(t, fake.batchCallCount(), 1, "the fitting refs must reach OPF") after := refHashes(t, repo, refs) for i := range refs { require.NotEqual(t, before[i], after[i]) @@ -1314,12 +1369,12 @@ func TestRewriteQueuedCheckpointRefsWithOPF_OversizedRefDoesNotBlockOthers(t *te require.Equal(t, 1, fake.batchCallCount(), "only the ref that fits may reach OPF") } -// Backend divergence: the v1 path aborts the user's push on OPF failure; the -// refs path fails closed by withholding the flush instead — the user's push -// succeeds, nothing un-OPF'd ships, and the refs stay queued. -func TestPrePushCheckpointRefs_OPFFailureWithholdsFlush(t *testing.T) { +// Pre-push never runs the model: unscanned refs are withheld, the user's push +// succeeds, the user is told why, and nothing reaches the remote. +func TestPrePushCheckpointRefs_UnscannedRefsAreWithheldWithNotice(t *testing.T) { configureFakeOPF(t, &fakeRuntimeAlwaysFails{}) bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + swapOPFScanSpawn(t) var buf bytes.Buffer oldWriter := stderrWriter @@ -1327,15 +1382,24 @@ func TestPrePushCheckpointRefs_OPFFailureWithholdsFlush(t *testing.T) { t.Cleanup(func() { stderrWriter = oldWriter }) require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin"), - "an OPF failure must not block the user's git push") + "unscanned checkpoints must not block the user's git push") + + assert.Contains(t, buf.String(), opfScanPendingNotice, "the held push must be visible to the user") + assert.ElementsMatch(t, refs, queuedRefs(t, repo), "withheld refs stay queued") + assertRefsAbsentFromRemote(t, bareDir, refs, "no unscanned ref may reach the remote") +} + +// A worker whose model call fails delivers nothing: the refs stay queued and +// off the remote, so the next push (or worker) tries again. +func TestRunOPFScan_RuntimeFailureDeliversNothing(t *testing.T) { + configureFakeOPF(t, &fakeRuntimeAlwaysFails{}) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) - assert.Contains(t, buf.String(), "checkpoint ref", "the withheld push must be visible to the user") - assert.ElementsMatch(t, refs, queuedRefs(t, repo), "withheld refs stay queued for the next push") - lsCmd := exec.CommandContext(t.Context(), "git", "ls-remote", bareDir) - lsCmd.Env = testutil.GitIsolatedEnv() - out, err := lsCmd.CombinedOutput() - require.NoError(t, err, "ls-remote failed: %s", out) - assert.NotContains(t, string(out), refs[0].String(), "no ref may reach the remote when OPF failed") + require.NoError(t, RunOPFScan(t.Context(), "origin"), "the worker is best-effort and never fails the process") + assert.ElementsMatch(t, refs, queuedRefs(t, repo), "refs stay queued after a failed scan") + assertRefsAbsentFromRemote(t, bareDir, refs, "no ref may reach the remote when OPF failed") } // cancelOPFPrompt makes the git-refs decision an explicit Ctrl-C at the prompt, @@ -1455,3 +1519,65 @@ func TestRewriteQueuedCheckpointRefsWithOPF_AncestryOverBootstrapLimit(t *testin require.Equal(t, 1, tooLarge.Limit) require.Equal(t, before, refHashes(t, repo, refs), "an over-limit ancestry must not move any ref") } + +// Cache-only mode is what pre-push uses: it must never call the model, report +// unscanned refs as pending with the refs untouched, and rewrite them without a +// model call once a scan has filled the cache. +func TestRewriteQueuedCheckpointRefsWithOPF_ApplyCachedOnly(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + _, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6", "b2c3d4e5f6a1") + before := refHashes(t, repo, refs) + + err := rewriteQueuedCheckpointRefsWithOPF(t.Context(), repo, opfApplyCachedOnly) + var pending *OPFScanPendingError + require.ErrorAs(t, err, &pending) + require.Zero(t, fake.batchCallCount(), "cache-only mode must never call the model") + require.Equal(t, before, refHashes(t, repo, refs), "pending refs must be left untouched") + + // Scan without rewriting, the way the worker fills the cache first. + blobs := checkpointRefBlobsForTest(t, repo, refs) + cache, err := checkpoint.OPFSpanCacheForRepo(repo) + require.NoError(t, err) + require.NoError(t, redact.ScanBlobsWithPrivacyFilter(t.Context(), blobs, cache)) + scanCalls := fake.batchCallCount() + + require.NoError(t, rewriteQueuedCheckpointRefsWithOPF(t.Context(), repo, opfApplyCachedOnly)) + require.Equal(t, scanCalls, fake.batchCallCount(), "applying from the cache must not call the model") + after := refHashes(t, repo, refs) + for i := range refs { + commit, cErr := repo.CommitObject(after[i]) + require.NoError(t, cErr) + require.True(t, trailers.HasOPFApplied(commit.Message)) + require.NotContains(t, treeContents(t, repo, after[i]), "PERSONABC") + } +} + +func TestRewriteUnpushedV1WithOPF_ApplyCachedOnly(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + repo, originalTip := setupV1Repo(t) + + _, err := rewriteUnpushedV1WithOPF(context.Background(), repo, "origin", opfApplyCachedOnly) + var pending *OPFScanPendingError + require.ErrorAs(t, err, &pending) + require.Zero(t, fake.batchCallCount(), "cache-only mode must never call the model") + ref, err := repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) + require.NoError(t, err) + require.Equal(t, originalTip, ref.Hash(), "a pending v1 rewrite must not move the ref") +} + +// checkpointRefBlobsForTest collects the redactable blobs of every commit on +// refs, with their object IDs, the way the scan worker does. +func checkpointRefBlobsForTest(t *testing.T, repo *git.Repository, refs []plumbing.ReferenceName) []redact.NamedBlob { + t.Helper() + var blobs []redact.NamedBlob + for _, ref := range refs { + pending, err := collectCheckpointRefForOPF(repo, ref, rawByteCapForBatchLimit(resolveBatchLimit()), resolveBootstrapLimit()) + require.NoError(t, err) + for _, c := range pending.commits { + blobs = append(blobs, c.blobs...) + } + } + return blobs +} diff --git a/cmd/entire/cli/strategy/manual_commit_opf_scan.go b/cmd/entire/cli/strategy/manual_commit_opf_scan.go new file mode 100644 index 0000000000..dee8c5f203 --- /dev/null +++ b/cmd/entire/cli/strategy/manual_commit_opf_scan.go @@ -0,0 +1,601 @@ +// Detached OPF scan worker, shared by both checkpoint backends. +// +// OPF inference is slow (about 1.14 s per KB of prose on CPU), so a real +// session takes minutes to hours of model time. The pre-push hook therefore +// never runs the model: it rewrites checkpoints from the OPF span cache and +// holds back anything not scanned yet. This worker fills the cache in the +// background and then delivers what it scanned, so the user's `git push` +// returns immediately and their checkpoints follow as soon as OPF finishes. +// +// The worker only acts on the decision the user's push made. It is spawned +// only when that push resolved OPFRun, it delivers only to the remote that +// push named, and delivery goes through the same pre-push code, which ships a +// checkpoint only once its exact commit carries Entire-OPF-Applied. If the +// worker cannot reach the remote, the checkpoints stay queued and the next +// push delivers them. +package strategy + +import ( + "context" + "errors" + "fmt" + "log/slog" + "path" + "slices" + "strings" + "time" + + git "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/execx" + "github.com/entireio/cli/cmd/entire/cli/gitdir" + "github.com/entireio/cli/cmd/entire/cli/internal/flock" + "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/osroot" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/spawnmarker" + "github.com/entireio/cli/cmd/entire/cli/trailers" + "github.com/entireio/cli/redact" +) + +const opfScanComponent = "opf-scan" + +// opfScanMaxPasses bounds the worker's loop. A pass repeats only while new +// unscanned content keeps appearing (a session still writing checkpoints), so +// this exists to stop a detached process nobody watches from spinning. +const opfScanMaxPasses = 10 + +// opfScanDeliveryTimeout bounds one delivery attempt. Delivery is a push of +// already-rewritten refs; a push that takes longer than this is treated as +// failed and left for the next user push. +const opfScanDeliveryTimeout = 10 * time.Minute + +// opfScanWorkerLockName is held for a worker's whole run, in the shared git +// common dir so workers spawned from different worktrees exclude each other. +const opfScanWorkerLockName = "entire/opf-scan-worker.lock" + +// opfScanWorkerLockWait turns the blocking flock into a try-lock: a worker that +// cannot take the lock almost immediately leaves the work to the one holding +// it, which re-collects its work after every pass. +const opfScanWorkerLockWait = 100 * time.Millisecond + +// opfScanSpawn is the process-spawn seam, swapped in tests: execx.SpawnDetached +// is a no-op under `go test`, which would make the spawn decision unobservable. +var opfScanSpawn = spawnDetachedOPFScanProcess + +func spawnDetachedOPFScanProcess(worktreeRoot, remote string) { + execx.SpawnDetached(worktreeRoot, "__opf_scan", remote) +} + +type opfScanWorkerKey struct{} + +// withinOPFScanWorker marks ctx as running inside the scan worker. Pre-push +// code consults it to take the decision the spawning push already made, and to +// never spawn another worker from inside one. +func withinOPFScanWorker(ctx context.Context) context.Context { + return context.WithValue(ctx, opfScanWorkerKey{}, true) +} + +func inOPFScanWorker(ctx context.Context) bool { + v, _ := ctx.Value(opfScanWorkerKey{}).(bool) //nolint:errcheck // absent means false + return v +} + +// maybeSpawnOPFScan starts the scan worker for remote unless this already is +// the worker or a worker is running. A running worker picks up the new work: it +// re-collects after every pass and once more after releasing its lock (see +// RunOPFScan). Callers invoke it only when the push resolved OPFRun and +// something was held back as not yet scanned. +// +// Gating on the lock rather than on time since the last spawn means a push +// right after a worker finished always gets a new one. The cost is that a +// persistently failing OPF runtime is retried on every push. +func maybeSpawnOPFScan(ctx context.Context, remote string) { + if inOPFScanWorker(ctx) || !redact.OPFEnabled() { + return + } + logCtx := logging.WithComponent(ctx, opfScanComponent) + root, err := paths.WorktreeRoot(ctx) + if err != nil { + logging.Debug(logCtx, "skipping OPF scan spawn: could not resolve worktree root", + slog.String("error", err.Error())) + return + } + release, held := acquireOPFScanWorkerLock(ctx) + release() + if held { + return + } + opfScanSpawn(root, remote) + logging.Info(logCtx, "checkpoints held for OPF; spawned background scan", slog.String("remote", remote)) +} + +// RunOPFScan is the body of the detached `entire __opf_scan ` worker. +// Each pass collects the content waiting for OPF, scans what the span cache +// lacks, and runs the pre-push delivery for remote after each scan batch. It +// stops when nothing new is pending, or after opfScanMaxPasses. +// +// Best-effort by construction: every failure is logged, never returned as a +// process error, because nothing watches this child's exit code. +func RunOPFScan(ctx context.Context, remote string) error { + logCtx := logging.WithComponent(ctx, opfScanComponent) + // One line on every exit, including a worker that found another one + // running, so a failed or stalled background scan can be told apart from + // one that never started, and each spawn can be matched to its end. + defer logging.Info(logCtx, "opf scan worker finished") + // OPFEnabled reads process-global config that only EnsureRedactionConfigured + // sets; without it the worker would read "OPF off" and do nothing. Unlike + // the hook, a scanner-config error stops the worker: stamping commits with a + // scanner set the settings did not choose is not a call a background + // process gets to make. + if err := EnsureRedactionConfigured(ctx); err != nil { + logging.Warn(logCtx, "opf scan skipped: redaction could not be configured", + slog.String("error", err.Error())) + return nil + } + if !redact.OPFEnabled() { + logging.Debug(logCtx, "opf scan skipped: OPF is not enabled") + return nil + } + repo, err := OpenRepository(ctx) + if err != nil { + logging.Warn(logCtx, "opf scan skipped: could not open repo", slog.String("error", err.Error())) + return nil + } + defer repo.Close() + + release, held := acquireOPFScanWorkerLock(ctx) + if held { + logging.Debug(logCtx, "opf scan skipped: another worker is already running") + return nil + } + + cache, err := checkpoint.OPFSpanCacheForRepo(repo) + if err != nil { + release() + logging.Warn(logCtx, "opf scan skipped: span cache unavailable", slog.String("error", err.Error())) + return nil + } + if commonDir, cdErr := gitdir.CommonDir(ctx); cdErr == nil { + if _, pruneErr := checkpoint.PruneOPFSpanCache(commonDir, time.Now(), checkpoint.OPFSpanCacheMaxAge); pruneErr != nil { + logging.Debug(logCtx, "opf scan: cache prune failed", slog.String("error", pruneErr.Error())) + } + } + + ctx = withinOPFScanWorker(ctx) + w := &opfScanWorker{remote: remote, repo: repo, cache: cache, seen: make(map[string]struct{})} + for { + w.runPasses(ctx) + opfScanBeforeRelease() + release() + // A push that held new work while this worker held the lock saw it + // held and spawned nothing, so look once more now that it is free. Only + // work this process has not already tried brings it back, so units it + // could not scan or deliver do not make it spin. + if w.withdrawn || w.passes >= opfScanMaxPasses || !w.hasUnseenWork(ctx) { + return nil + } + if release, held = acquireOPFScanWorkerLock(ctx); held { + return nil // another worker took it and owns the new work + } + } +} + +// opfScanBeforeRelease runs after a worker's passes and before it releases the +// lock: the window in which a push holds new work but sees a worker running. +var opfScanBeforeRelease = func() {} //nolint:gochecknoglobals // exit-race test seam + +// opfScanWorker carries one worker process's state across its passes and +// across re-acquiring the lock. +type opfScanWorker struct { + remote string + repo *git.Repository + cache redact.OPFSpanCache + // seen holds the IDs of every commit a pass has already worked on. + seen map[string]struct{} + passes int + // withdrawn ends the worker: the settings no longer allow OPF to run. + withdrawn bool +} + +// runPasses collects, scans and delivers until nothing new is pending, a pass +// fails, or the pass budget runs out. Each pass delivers after every scan +// batch, so a checkpoint ships about one model call after it is scanned rather +// than after the whole backlog. +func (w *opfScanWorker) runPasses(ctx context.Context) { + logCtx := logging.WithComponent(ctx, opfScanComponent) + for ; w.passes < opfScanMaxPasses; w.passes++ { + if opfWorkerWithdrawn(ctx) { + w.withdrawn = true + logging.Info(logCtx, "opf scan stopped: OPF is now set to never run") + return + } + units, collectErr := collectOPFScanUnits(ctx, w.repo, w.remote) + if collectErr != nil { + logging.Warn(logCtx, "opf scan: could not collect pending checkpoints", + slog.String("error", collectErr.Error())) + return + } + if !w.markSeen(units) { + // Everything still pending was already scanned and delivered by + // this process, so delivery failed for a reason a retry here will + // not fix. The next user push tries again. + return + } + scanned, scanErr := scanOPFUnits(ctx, w.repo, units, w.cache, w.deliver) + if scanErr != nil { + logging.Warn(logCtx, "opf scan stopped; checkpoints stay held", slog.String("error", scanErr.Error())) + return + } + if scanned == 0 { + // Every pending unit is over a cap; the rewrite would refuse them + // too, so there is nothing to deliver. + return + } + } + logging.Warn(logCtx, "opf scan: stopping after the maximum number of passes", + slog.Int("passes", opfScanMaxPasses)) +} + +// deliver runs the pre-push delivery for the worker's remote, which rewrites +// from the cache and pushes whatever is now covered. +func (w *opfScanWorker) deliver(ctx context.Context) error { + deliverCtx, cancel := context.WithTimeout(ctx, opfScanDeliveryTimeout) + defer cancel() + if err := NewManualCommitStrategy().PrePushFromGitHook(deliverCtx, w.remote); err != nil { + return fmt.Errorf("delivery failed; checkpoints stay queued for the next push: %w", err) + } + return nil +} + +// markSeen records units' commits and reports whether any was new. +func (w *opfScanWorker) markSeen(units [][]*object.Commit) bool { + added := false + for _, id := range opfScanUnitIDs(units) { + if _, ok := w.seen[id]; !ok { + w.seen[id] = struct{}{} + added = true + } + } + return added +} + +// hasUnseenWork reports whether anything pending is new to this process. +func (w *opfScanWorker) hasUnseenWork(ctx context.Context) bool { + units, err := collectOPFScanUnits(ctx, w.repo, w.remote) + if err != nil { + return false + } + for _, id := range opfScanUnitIDs(units) { + if _, ok := w.seen[id]; !ok { + return true + } + } + return false +} + +// collectOPFScanUnits returns the checkpoint commits that still lack the OPF +// trailer on the primary backend, one unit per queued ref (git-refs) or the +// unpushed v1 chain (git-branch). It loads commits only, no blob content, so a +// long backlog costs little memory here; scanOPFUnits loads content in bounded +// batches. +func collectOPFScanUnits(ctx context.Context, repo *git.Repository, remote string) ([][]*object.Commit, error) { + var units [][]*object.Commit + if primaryIsGitRefs(ctx) { + queue, err := checkpoint.PushQueueForRepo(ctx, repo) + if err != nil { + return nil, fmt.Errorf("resolve push queue: %w", err) + } + queued, err := queue.Peek() + if err != nil { + return nil, fmt.Errorf("peek push queue: %w", err) + } + bootstrapLimit := resolveBootstrapLimit() + existing, _ := partitionLocalRefs(repo, queued) + for _, refName := range existing { + ref, refErr := repo.Reference(refName, true) + if refErr != nil { + continue + } + chain, _, chainErr := unappliedAncestry(repo, ref.Hash()) + if chainErr != nil { + return nil, fmt.Errorf("walk ancestry of %s: %w", refName, chainErr) + } + if len(chain) > bootstrapLimit { + // The rewrite refuses it (BootstrapTooLargeError), so scanning + // it would only burn model time. + logging.Warn(logging.WithComponent(ctx, opfScanComponent), "opf scan: skipping checkpoint over the bootstrap limit", + slog.String("ref", refName.String()), slog.Int("commits", len(chain)), slog.Int("limit", bootstrapLimit)) + continue + } + if len(chain) > 0 { + units = append(units, chain) + } + } + return units, nil + } + chain, err := v1CommitsAwaitingOPF(ctx, repo, remote) + if err != nil { + return nil, err + } + if len(chain) > 0 { + units = append(units, chain) + } + return units, nil +} + +// opfScanBlobs is the scan seam, swapped in tests to observe batch sizes. +var opfScanBlobs = redact.ScanBlobsWithPrivacyFilter //nolint:gochecknoglobals // batch-size test seam + +// scanOPFUnits scans units into the cache in batches whose raw blob bytes stay +// within one unit's raw cap, so the worker's resident input is bounded however +// long the backlog is, and whose prose stays within about one model call, so +// deliver runs after each batch and scanned checkpoints ship without waiting +// for the rest of the backlog. Each unit is capped the way the rewrite caps it; a unit +// over a cap is left out and logged, because the rewrite would refuse it anyway +// and scanning pathological content only burns model time. It returns how many +// units were scanned. +func scanOPFUnits(ctx context.Context, repo *git.Repository, units [][]*object.Commit, cache redact.OPFSpanCache, deliver func(context.Context) error) (int, error) { + logCtx := logging.WithComponent(ctx, opfScanComponent) + batchLimit := resolveBatchLimit() + rawCap := rawByteCapForBatchLimit(batchLimit) + + var batch []redact.NamedBlob + batchRaw, batchLeaf, scanned := 0, 0, 0 + flush := func() error { + if len(batch) == 0 { + return nil + } + err := opfScanBlobs(ctx, batch, cache) + batch, batchRaw, batchLeaf = nil, 0, 0 + if err != nil { + return err + } + return deliver(ctx) + } + for _, unit := range units { + unitBlobs, err := collectCommitBlobsForOPF(repo, unit, rawCap) + if err != nil { + var rawTooLarge *OPFRawBytesTooLargeError + if errors.As(err, &rawTooLarge) { + logging.Warn(logCtx, "opf scan: skipping checkpoint over the raw-byte cap", slog.String("error", err.Error())) + continue + } + return scanned, err + } + leafBytes := redact.SumProseLeafBytes(unitBlobs) + if leafBytes > batchLimit { + logging.Warn(logCtx, "opf scan: skipping checkpoint over the prose-leaf cap", + slog.Int("leaf_bytes", leafBytes), slog.Int("limit", batchLimit)) + continue + } + unitRaw := 0 + for _, b := range unitBlobs { + unitRaw += len(b.Content) + } + if batchRaw+unitRaw > rawCap || batchLeaf+leafBytes > redact.OPFBatchChunkBytes { + if err := flush(); err != nil { + return scanned, err + } + } + batch = append(batch, unitBlobs...) + batchRaw += unitRaw + batchLeaf += leafBytes + scanned++ + } + return scanned, flush() +} + +// v1CommitsAwaitingOPF returns the unpushed v1 commits that lack the OPF +// trailer, oldest first, measured against remote's live v1 tip the same way +// RewriteUnpushedV1WithOPF measures them. +func v1CommitsAwaitingOPF(ctx context.Context, repo *git.Repository, remote string) ([]*object.Commit, error) { + localTip, err := readV1Tip(repo, plumbing.NewBranchReferenceName(paths.MetadataBranchName)) + if err != nil { + return nil, fmt.Errorf("read local v1: %w", err) + } + if localTip.IsZero() { + return nil, nil + } + ps := resolvePushSettings(ctx, remote) + remoteTip, err := resolveRemoteV1Tip(ctx, repo, ps.pushTarget()) + if err != nil { + return nil, fmt.Errorf("read remote v1: %w", err) + } + unpushed, err := listUnpushedV1Commits(repo, localTip, remoteTip) + if err != nil { + return nil, fmt.Errorf("list unpushed v1 commits: %w", err) + } + if limit := resolveBootstrapLimit(); remoteTip.IsZero() && len(unpushed) > limit { + // The rewrite refuses this bootstrap (BootstrapTooLargeError), so + // scanning it would only burn model time. + logging.Warn(logging.WithComponent(ctx, opfScanComponent), "opf scan: skipping v1 over the bootstrap limit", + slog.Int("commits", len(unpushed)), slog.Int("limit", limit)) + return nil, nil + } + pending := unpushed[:0] + for _, c := range unpushed { + if !trailers.HasOPFApplied(c.Message) { + pending = append(pending, c) + } + } + return pending, nil +} + +func collectCommitBlobsForOPF(repo *git.Repository, commits []*object.Commit, rawCap int) ([]redact.NamedBlob, error) { + budget := newOPFRawByteBudget(rawCap) + var blobs []redact.NamedBlob + var treePaths []string + for _, c := range commits { + tree, err := repo.TreeObject(c.TreeHash) + if err != nil { + return nil, fmt.Errorf("load tree for %s: %w", c.Hash.String()[:7], err) + } + if err := collectTreeBlobsWithinBudget(repo, tree, "", &blobs, &treePaths, budget); err != nil { + return nil, fmt.Errorf("collect blobs %s: %w", c.Hash.String()[:7], err) + } + } + return blobs, nil +} + +// opfScanUnitIDs identifies a pass's pending work by its commits, so a pass +// that finds exactly the work the previous one delivered can stop. +func opfScanUnitIDs(units [][]*object.Commit) []string { + var ids []string + for _, unit := range units { + for _, c := range unit { + ids = append(ids, c.Hash.String()) + } + } + slices.Sort(ids) + return slices.Compact(ids) +} + +// acquireOPFScanWorkerLock takes the per-repo worker lock. held reports that +// another worker has it. Any other failure returns a no-op release with held +// false: running unguarded is never worse than two workers racing, which the +// ref CAS already makes safe. +func acquireOPFScanWorkerLock(ctx context.Context) (release func(), held bool) { + noop := func() {} + commonDir, err := gitdir.CommonDir(ctx) + if err != nil { + return noop, false + } + root, err := gitdir.OpenAt(commonDir) + if err != nil { + return noop, false + } + if err := osroot.MkdirAllNoSymlink(root, path.Dir(opfScanWorkerLockName), 0o750); err != nil { + return noop, false + } + lockCtx, cancel := context.WithTimeout(ctx, opfScanWorkerLockWait) + defer cancel() + release, err = flock.AcquireContextIn(lockCtx, root, opfScanWorkerLockName) + if err != nil { + return noop, errors.Is(err, context.DeadlineExceeded) + } + return release, false +} + +// opfPendingV1WalkLimit bounds the v1 history walk behind CheckpointsAwaitingOPF, +// which runs on every `entire status`. +const opfPendingV1WalkLimit = 1000 + +// CheckpointsAwaitingOPF counts checkpoint commits on the primary backend that +// still lack the OPF trailer and have not been pushed: queued refs on git-refs, +// unpushed v1 commits on git-branch. It reads local refs and the push queue +// only, so it is safe on a read-only status path. `entire status` reports it, +// because with scanning in a detached worker that is the one place held work +// is visible. +func CheckpointsAwaitingOPF(ctx context.Context, repo *git.Repository) (int, error) { + if primaryIsGitRefs(ctx) { + queue, err := checkpoint.PushQueueForRepo(ctx, repo) + if err != nil { + return 0, fmt.Errorf("resolve push queue: %w", err) + } + queued, err := queue.Peek() + if err != nil { + return 0, fmt.Errorf("peek push queue: %w", err) + } + existing, _ := partitionLocalRefs(repo, queued) + n := 0 + for _, refName := range existing { + ref, refErr := repo.Reference(refName, true) + if refErr != nil { + continue + } + commit, commitErr := repo.CommitObject(ref.Hash()) + if commitErr == nil && !trailers.HasOPFApplied(commit.Message) { + n++ + } + } + return n, nil + } + return v1CommitsAwaitingOPFLocally(repo) +} + +// v1CommitsAwaitingOPFLocally counts v1 commits from the local tip back to the +// first one that carries the OPF trailer or that some remote-tracking v1 ref +// already names, without contacting any remote. +func v1CommitsAwaitingOPFLocally(repo *git.Repository) (int, error) { + localTip, err := readV1Tip(repo, plumbing.NewBranchReferenceName(paths.MetadataBranchName)) + if err != nil { + return 0, fmt.Errorf("read local v1: %w", err) + } + if localTip.IsZero() { + return 0, nil + } + pushed := make(map[plumbing.Hash]struct{}) + refs, err := repo.References() + if err != nil { + return 0, fmt.Errorf("list references: %w", err) + } + suffix := "/" + paths.MetadataBranchName + _ = refs.ForEach(func(r *plumbing.Reference) error { //nolint:errcheck // callback never fails + if name := r.Name(); name.IsRemote() && strings.HasSuffix(name.String(), suffix) { + pushed[r.Hash()] = struct{}{} + } + return nil + }) + iter, err := repo.Log(&git.LogOptions{From: localTip}) + if err != nil { + return 0, fmt.Errorf("log local v1: %w", err) + } + defer iter.Close() + n := 0 + walkErr := iter.ForEach(func(c *object.Commit) error { + if _, ok := pushed[c.Hash]; ok || trailers.HasOPFApplied(c.Message) || n >= opfPendingV1WalkLimit { + return errStop + } + n++ + return nil + }) + if walkErr != nil && !errors.Is(walkErr, errStop) { + return 0, fmt.Errorf("walk local v1: %w", walkErr) + } + return n, nil +} + +// opfGitRefsHintInterval spaces out the git-refs suggestion below so it reads +// as a tip, not as a warning on every push. +const opfGitRefsHintInterval = 30 * 24 * time.Hour + +const opfGitRefsHint = "[entire] Tip: with the OpenAI Privacy Filter on, the git-refs checkpoint backend " + + "ships each checkpoint as soon as it is scanned instead of the whole branch at once. " + + "Switch with `entire doctor migrate-checkpoints`." + +// maybeHintGitRefsForOPF suggests the git-refs backend to a git-branch user +// whose checkpoints were just held for a scan, at most once per +// opfGitRefsHintInterval per repository. git-branch is fully supported; the +// hint only points at the backend where one slow checkpoint cannot hold back +// the rest. +func maybeHintGitRefsForOPF(ctx context.Context) { + if inOPFScanWorker(ctx) { + return + } + commonDir, err := gitdir.CommonDir(ctx) + if err != nil { + return + } + if spawnmarker.RecentlySpawned(commonDir, "opf-git-refs-hint", opfGitRefsHintInterval, time.Now()) { + return + } + fmt.Fprintln(stderrWriter, opfGitRefsHint) +} + +// opfWorkerWithdrawn reports that the settings now say OPF should never run. +// The push that spawned the worker resolved OPFRun, but a user who has since +// chosen "never" has withdrawn that decision, and a worker must not keep +// scanning and delivering on it. Read per pass, so a change applies from the +// next pass. +func opfWorkerWithdrawn(ctx context.Context) bool { + cfg, err := settings.Load(ctx) + if err != nil || cfg == nil || cfg.Redaction == nil || cfg.Redaction.OpenAIPrivacyFilter == nil { + return false + } + return strings.EqualFold(strings.TrimSpace(cfg.Redaction.OpenAIPrivacyFilter.PromptDefault), settings.OPFPromptNever) +} diff --git a/cmd/entire/cli/strategy/manual_commit_opf_scan_test.go b/cmd/entire/cli/strategy/manual_commit_opf_scan_test.go new file mode 100644 index 0000000000..0e17072eed --- /dev/null +++ b/cmd/entire/cli/strategy/manual_commit_opf_scan_test.go @@ -0,0 +1,387 @@ +package strategy + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/go-git/go-git/v6" + gitconfig "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/gitdir" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/entireio/cli/cmd/entire/cli/trailers" + "github.com/entireio/cli/redact" +) + +// swapOPFScanSpawn replaces the detached-spawn seam with a recorder and returns +// the remotes it was asked to scan for. execx.SpawnDetached is a no-op under +// `go test`, and the test binary does not understand `__opf_scan` anyway. +func swapOPFScanSpawn(t *testing.T) *[]string { + t.Helper() + remotes := make([]string, 0, 1) + old := opfScanSpawn + opfScanSpawn = func(_, remote string) { remotes = append(remotes, remote) } + t.Cleanup(func() { opfScanSpawn = old }) + return &remotes +} + +// The worker scans what pre-push held back and delivers it itself: after one +// run every queued ref is redacted, trailered, on the remote, and off the queue. +func TestRunOPFScan_ScansAndDeliversGitRefs(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6", "b2c3d4e5f6a1") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + spawns := swapOPFScanSpawn(t) + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + + require.Positive(t, fake.batchCallCount(), "the worker must run the model") + require.Empty(t, *spawns, "the worker must never spawn another worker") + require.Empty(t, queuedRefs(t, repo), "delivered refs leave the queue") + for _, ref := range refs { + tip := remoteRefHash(t, bareDir, ref) + require.NotEmpty(t, tip, "ref %s must reach the remote", ref) + commit, err := repo.CommitObject(plumbing.NewHash(tip)) + require.NoError(t, err) + require.True(t, trailers.HasOPFApplied(commit.Message)) + require.NotContains(t, treeContents(t, repo, commit.Hash), "PERSONABC") + } +} + +// A long backlog is scanned in batches bounded by one ref's raw cap rather than +// loaded whole, so the detached worker's memory does not grow with the queue. +// Every ref still gets scanned, and each batch is delivered as it finishes. +func TestRunOPFScan_BoundsEachScanBatchByTheRawCap(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6", "b2c3d4e5f6a1", "c3d4e5f6a1b2") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + swapOPFScanSpawn(t) + + maxRaw, maxLeaf, totalRaw := 0, 0, 0 + for _, ref := range refs { + raw, leaf := refRewriteSizes(t, repo, ref) + maxRaw, maxLeaf, totalRaw = max(maxRaw, raw), max(maxLeaf, leaf), totalRaw+raw + } + limit := max(maxLeaf, (maxRaw+rawByteCapMultiplier-1)/rawByteCapMultiplier) + rawCap := rawByteCapForBatchLimit(limit) + require.Greater(t, totalRaw, rawCap, "fixture: the whole backlog must exceed one raw cap") + t.Setenv(batchEnvVar, strconv.Itoa(limit)) + + var batchRaw, deliveredBefore []int + old := opfScanBlobs + opfScanBlobs = func(ctx context.Context, blobs []redact.NamedBlob, cache redact.OPFSpanCache) error { + n := 0 + for _, b := range blobs { + n += len(b.Content) + } + batchRaw = append(batchRaw, n) + deliveredBefore = append(deliveredBefore, len(refs)-len(queuedRefs(t, repo))) + return old(ctx, blobs, cache) + } + t.Cleanup(func() { opfScanBlobs = old }) + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + + require.Greater(t, len(batchRaw), 1, "the backlog must be split across scans") + require.Positive(t, deliveredBefore[len(deliveredBefore)-1], + "each batch is delivered before the next is scanned, not after the whole backlog") + for _, n := range batchRaw { + require.LessOrEqual(t, n, rawCap, "no scan batch may exceed the raw cap") + } + require.Empty(t, queuedRefs(t, repo), "every ref is still delivered") + for _, ref := range refs { + require.NotEmpty(t, remoteRefHash(t, bareDir, ref)) + } +} + +// A push spawns a worker whenever none is running, even right after another +// one was spawned and finished; while one runs, it leaves the work to it. +func TestMaybeSpawnOPFScan_GatesOnTheWorkerLockNotTime(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + spawns := swapOPFScanSpawn(t) + + maybeSpawnOPFScan(t.Context(), "origin") + maybeSpawnOPFScan(t.Context(), "origin") + require.Len(t, *spawns, 2, "no worker is running, so each push must start one") + + release, held := acquireOPFScanWorkerLock(t.Context()) + require.False(t, held) + defer release() + maybeSpawnOPFScan(t.Context(), "origin") + require.Len(t, *spawns, 2, "a running worker owns the new work") +} + +// Work held by a push while the worker still held its lock (so the push spawned +// nothing) is picked up by the worker once it releases the lock. +func TestRunOPFScan_PicksUpWorkHeldJustBeforeRelease(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + swapOPFScanSpawn(t) + + const lateID = "b2c3d4e5f6a1" + added := false + old := opfScanBeforeRelease + opfScanBeforeRelease = func() { + if !added { + added = true + addGitRefsSession(t, repo, lateID, "sess-late") + } + } + t.Cleanup(func() { opfScanBeforeRelease = old }) + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + + require.NotEmpty(t, remoteRefHash(t, bareDir, refs[0])) + require.NotEmpty(t, remoteRefHash(t, bareDir, mustRefName(t, id.MustCheckpointID(lateID))), + "work held just before the worker released its lock must still be delivered") + require.Empty(t, queuedRefs(t, repo)) +} + +// A ref whose unscanned history is over the bootstrap limit is refused by the +// rewrite, so the worker does not spend model time on it. +func TestRunOPFScan_SkipsRefsOverTheBootstrapLimit(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + _, repo, _ := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + addGitRefsSession(t, repo, "a1b2c3d4e5f6", "sess-2") + t.Setenv("ENTIRE_OPF_BOOTSTRAP_LIMIT", "1") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + swapOPFScanSpawn(t) + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + require.Zero(t, fake.batchCallCount(), "an over-limit ref must not reach the model") +} + +// On git-refs a user push that sends checkpoint refs itself (`--mirror`, an +// explicit refspec) is refused while their commits lack the OPF trailer, since +// withholding Entire's own push cannot stop it. A ref with the trailer goes +// through, and so does everything after an explicit opt-out for this push. +func TestPrePushCheckpointRefs_RefusesOuterPushOfUnverifiedRefs(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + _, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + swapOPFScanSpawn(t) + outer := func() context.Context { + tip := refHashes(t, repo, refs)[0].String() + return WithPrePushRefs(t.Context(), []PrePushRef{{ + LocalRef: refs[0].String(), LocalSHA: tip, RemoteRef: refs[0].String(), RemoteSHA: strings.Repeat("0", 40), + }}) + } + + err := NewManualCommitStrategy().PrePushFromGitHook(outer(), "origin") + require.ErrorIs(t, err, ErrOuterPushCarriesUnverifiedCheckpoints) + + t.Setenv("ENTIRE_OPF", "no") + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(outer(), "origin"), + "an explicit opt-out for this push lets it through") + t.Setenv("ENTIRE_OPF", "") + + _, repo, refs = setupGitRefsOPFRepo(t, "b2c3d4e5f6a1") + require.NoError(t, RewriteQueuedCheckpointRefsWithOPF(t.Context(), repo)) + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(outer(), "origin"), + "a ref that carries the trailer may be pushed") +} + +// On git-refs too, a held checkpoint ref tells a user whose hook does not pass +// the ref list that an outer push carrying it cannot be checked. +func TestPrePushCheckpointRefs_HeldRefWarnsWhenRefsAreUnknown(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + swapOPFScanSpawn(t) + var buf bytes.Buffer + oldWriter := stderrWriter + stderrWriter = &buf + t.Cleanup(func() { stderrWriter = oldWriter }) + + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) + require.Contains(t, buf.String(), "does not pass the refs being pushed") + + buf.Reset() + main := []PrePushRef{{LocalRef: "refs/heads/main", LocalSHA: strings.Repeat("1", 40), RemoteRef: "refs/heads/main"}} + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(WithPrePushRefs(t.Context(), main), "origin")) + require.NotContains(t, buf.String(), "does not pass the refs being pushed") +} + +// A user who sets OPF to "never" after a worker was spawned has withdrawn the +// decision it was spawned under: it must stop scanning and delivering. +func TestRunOPFScan_StopsWhenSettingsNowSayNever(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + // Configured as the spawning push left it, before the settings change. + require.NoError(t, EnsureRedactionConfigured(t.Context())) + wt, err := repo.Worktree() + require.NoError(t, err) + testutil.WriteFile(t, wt.Filesystem().Root(), ".entire/settings.local.json", + `{"redaction":{"openai_privacy_filter":{"enabled":true,"prompt_default":"never"}}}`) + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + require.Zero(t, fake.batchCallCount(), "a withdrawn worker must not call the model") + assertRefsAbsentFromRemote(t, bareDir, refs, "a withdrawn worker must not deliver") +} + +// A worker already running owns the work: a second one must leave it alone +// rather than repeat the same model calls. +func TestRunOPFScan_SkipsWhileAnotherWorkerHoldsTheLock(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + + release, held := acquireOPFScanWorkerLock(t.Context()) + require.False(t, held) + require.NoError(t, RunOPFScan(t.Context(), "origin")) + require.Zero(t, fake.batchCallCount(), "a second worker must not call the model") + assertRefsAbsentFromRemote(t, bareDir, refs, "a second worker must not deliver") + require.Equal(t, refs, queuedRefs(t, repo)) + + release() + resetRedactionConfiguredForTest() + require.NoError(t, RunOPFScan(t.Context(), "origin")) + require.NotEmpty(t, remoteRefHash(t, bareDir, refs[0]), "once the lock is free the worker delivers") +} + +// On git-branch the worker scans the unpushed v1 chain and pushes v1 itself. +func TestRunOPFScan_ScansAndDeliversV1(t *testing.T) { + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + dir, repo, originalTip := setupV1RepoInDir(t) + remoteDir := filepath.Join(t.TempDir(), "origin.git") + _, err := git.PlainInit(remoteDir, true) + require.NoError(t, err) + _, err = repo.CreateRemote(&gitconfig.RemoteConfig{Name: "origin", URLs: []string{remoteDir}}) + require.NoError(t, err) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + // The remote already has the user's branch, so v1 is not deferred as a + // would-be default branch on an empty remote. + for _, args := range [][]string{{"push", "-q", "origin", "HEAD:refs/heads/main"}, {"update-ref", "refs/remotes/origin/main", "HEAD"}} { + cmd := exec.CommandContext(t.Context(), "git", args...) + cmd.Dir = dir + cmd.Env = testutil.GitIsolatedEnv() + out, runErr := cmd.CombinedOutput() + require.NoError(t, runErr, "git %v: %s", args, out) + } + + require.NoError(t, RunOPFScan(t.Context(), "origin")) + + require.Positive(t, fake.batchCallCount()) + remote, err := git.PlainOpen(remoteDir) + require.NoError(t, err) + ref, err := remote.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) + require.NoError(t, err, "the worker must push v1") + require.NotEqual(t, originalTip, ref.Hash()) + commit, err := remote.CommitObject(ref.Hash()) + require.NoError(t, err) + require.True(t, trailers.HasOPFApplied(commit.Message)) + require.NotContains(t, treeContents(t, repo, ref.Hash()), "PERSONABC") +} + +func TestCheckpointsAwaitingOPF_GitRefs(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + _, repo, _ := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6", "b2c3d4e5f6a1") + + n, err := CheckpointsAwaitingOPF(t.Context(), repo) + require.NoError(t, err) + require.Equal(t, 2, n) + + require.NoError(t, RewriteQueuedCheckpointRefsWithOPF(t.Context(), repo)) + n, err = CheckpointsAwaitingOPF(t.Context(), repo) + require.NoError(t, err) + require.Zero(t, n, "rewritten refs no longer wait for OPF") +} + +func TestCheckpointsAwaitingOPF_V1(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + dir, repo, _ := setupV1RepoInDir(t) + t.Chdir(dir) + + n, err := CheckpointsAwaitingOPF(t.Context(), repo) + require.NoError(t, err) + require.Positive(t, n, "an unscanned v1 chain waits for OPF") + + _, err = RewriteUnpushedV1WithOPF(t.Context(), repo, "origin") + require.NoError(t, err) + n, err = CheckpointsAwaitingOPF(t.Context(), repo) + require.NoError(t, err) + require.Zero(t, n, "a trailered v1 tip no longer waits for OPF") +} + +// A cache that cannot be opened is a real failure, not a scan in progress: the +// refs are withheld with the cause, and no worker is started, because it would +// need the same cache. +func TestPrePushCheckpointRefs_UnusableCacheIsReportedNotPending(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + bareDir, repo, refs := setupGitRefsOPFRepo(t, "a1b2c3d4e5f6") + commonDir, err := gitdir.CommonDir(t.Context()) + require.NoError(t, err) + // A regular file where the cache directory belongs makes it unopenable. + require.NoError(t, os.WriteFile(filepath.Join(commonDir, checkpoint.OPFSpanCacheDirName), []byte("x"), 0o600)) + spawns := swapOPFScanSpawn(t) + + var buf bytes.Buffer + oldWriter := stderrWriter + stderrWriter = &buf + t.Cleanup(func() { stderrWriter = oldWriter }) + + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) + + require.Contains(t, buf.String(), "scan cache", "the real cause must reach the user") + require.NotContains(t, buf.String(), opfScanPendingNotice, "an unusable cache is not a scan in progress") + require.Empty(t, *spawns, "a worker cannot help without the cache") + require.Equal(t, refs, queuedRefs(t, repo)) + assertRefsAbsentFromRemote(t, bareDir, refs, "nothing may ship without a scan") +} + +// A ref that hits a cap and an unscanned sibling must not hide each other, in +// either queue order: the pending signal is what starts the worker, and the cap +// error is what tells the user a ref is stuck. +func TestPrePushCheckpointRefs_CapErrorAndPendingSiblingAreBothReported(t *testing.T) { + for _, capped := range []int{0, 1} { + t.Run(strconv.Itoa(capped), func(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + cpIDs := []string{"a1b2c3d4e5f6", "b2c3d4e5f6a1"} + _, repo, refs := setupGitRefsOPFRepo(t, cpIDs...) + addGitRefsSession(t, repo, cpIDs[capped], "sess-2") + require.Equal(t, refs[capped], queuedRefs(t, repo)[capped], "the capped ref keeps its queue position") + t.Setenv("ENTIRE_OPF_BOOTSTRAP_LIMIT", "1") + resetRedactionConfiguredForTest() + t.Cleanup(resetRedactionConfiguredForTest) + spawns := swapOPFScanSpawn(t) + + var buf bytes.Buffer + oldWriter := stderrWriter + stderrWriter = &buf + t.Cleanup(func() { stderrWriter = oldWriter }) + + require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin")) + + require.Equal(t, []string{"origin"}, *spawns, "the pending sibling needs the worker") + require.Contains(t, buf.String(), opfScanPendingNotice) + require.Contains(t, buf.String(), "bootstrap", "the cap must still be reported") + }) + } +} diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index d7c176b1c4..5bac7e1034 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -165,7 +165,26 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote openSpan.End() defer repo.Close() _, opfSpan := perf.Start(ctx, "opf_pre_push_rewrite") - if _, rewriteErr := RewriteUnpushedV1WithOPF(ctx, repo, ps.pushTarget()); rewriteErr != nil { + verified, rewriteErr := rewriteUnpushedV1WithOPF(ctx, repo, ps.pushTarget(), opfApplyCachedOnly) + var pending *OPFScanPendingError + if errors.As(rewriteErr, &pending) { + // Not scanned yet: hold v1 back rather than make the user's push + // wait on the model. The worker scans and then pushes it. + opfSpan.End() + maybeSpawnOPFScan(ctx, ps.remote) + // Holding back Entire's own push is not enough when the user's + // push sends v1 itself (`--all`, `--mirror`, an explicit refspec). + if err := checkOuterPushV1(ctx, plumbing.ZeroHash); err != nil { + return err + } + fmt.Fprintln(stderrWriter, opfScanPendingNotice) + if !inOPFScanWorker(ctx) { + warnPrePushRefsUnknown(ctx, stderrWriter) + } + maybeHintGitRefsForOPF(ctx) + return nil + } + if rewriteErr != nil { opfSpan.RecordError(rewriteErr) opfSpan.End() logging.Warn(ctx, "OPF pre-push rewrite failed; aborting push", @@ -174,6 +193,12 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote return rewriteErr } opfSpan.End() + // git chose what the user's push sends before this hook ran, so a + // push that carries v1 sends the pre-rewrite commit unless it + // already was the verified tip. + if err := checkOuterPushV1(ctx, verified); err != nil { + return err + } } } @@ -233,6 +258,11 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote // settings > prompt > non-TTY auto-run). Shared by both checkpoint backends so // the precedence cannot drift between them. func opfPrePushDecision(ctx context.Context) (OPFDecision, error) { + if inOPFScanWorker(ctx) { + // The worker exists only because the push that spawned it resolved + // OPFRun; it must not re-ask or fall back to a different default. + return OPFRun, nil + } cfg, _ := settings.Load(ctx) //nolint:errcheck // Load already failed at hook init; fall back to nil var opfCfg *settings.OPFSettings if cfg != nil && cfg.Redaction != nil { @@ -292,10 +322,10 @@ func opfDecisionForCheckpointRefs(ctx context.Context) (OPFDecision, error) { return decision, nil } -func rewriteCheckpointRefsForPush(ctx context.Context, repo *git.Repository) error { +func rewriteCheckpointRefsForPush(ctx context.Context, repo *git.Repository, mode opfRewriteMode) error { _, opfSpan := perf.Start(ctx, "opf_rewrite_refs") defer opfSpan.End() - if err := RewriteQueuedCheckpointRefsWithOPF(ctx, repo); err != nil { + if err := rewriteQueuedCheckpointRefsWithOPF(ctx, repo, mode); err != nil { opfSpan.RecordError(err) return err } @@ -351,10 +381,42 @@ func warnOPFCheckpointRefsWithheld(ctx context.Context, err error, withheld int) if withheld == 0 { return } + if pending, rest := splitOPFScanPending(err); pending { + fmt.Fprintln(stderrWriter, opfScanPendingNotice) + if rest == nil { + return + } + err = rest + } fmt.Fprintf(stderrWriter, "[entire] %d checkpoint ref(s) were not pushed and stay queued for the next push: %v\n", withheld, err) } +// splitOPFScanPending separates the scan-pending signal from any other error +// the rewrite reported alongside it: a capped ref and an unscanned sibling +// arrive joined, and the user needs to hear about both. +func splitOPFScanPending(err error) (bool, error) { + var pending *OPFScanPendingError + if !errors.As(err, &pending) { + return false, err + } + var joined interface{ Unwrap() []error } + if !errors.As(err, &joined) { + return true, nil + } + var rest []error + for _, e := range joined.Unwrap() { + if !errors.As(e, &pending) { + rest = append(rest, e) + } + } + return true, errors.Join(rest...) +} + +// opfScanPendingNotice is what the user sees when checkpoints are held for the +// background scan. It is not an error: nothing is lost and no action is needed. +const opfScanPendingNotice = "[entire] Checkpoints are being scanned by the OpenAI Privacy Filter in the background and will be pushed when it finishes." + // opfCancelledCheckpointRefs reports whether the user explicitly cancelled OPF // at the prompt. That withholds the whole flush, already-trailered refs // included: cancel means nothing ships, and the docs promise the refs stay @@ -491,16 +553,34 @@ func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pus // nothing un-OPF'd ships, those refs stay queued, and the user's push // proceeds. opfDecision, opfErr := opfDecisionForCheckpointRefs(ctx) + // Withholding Entire's own push is not enough when the user's push sends + // checkpoint refs itself (`--all`, `--mirror`, an explicit refspec). Unlike + // a checkpoint-ref failure, that must abort the user's push. Only an + // explicit OPF skip for this push lets it through. + var outerErr error + if redact.OPFEnabled() && opfDecision != OPFSkip { + outerErr = checkOuterPushCheckpointRefs(ctx, repo) + } if opfCancelledCheckpointRefs(opfErr) { warnOPFCheckpointRefsWithheld(ctx, opfErr, queuedCheckpointRefCount(ctx, repo)) - return nil + return outerErr } if opfDecision == OPFRun && opfErr == nil { - // Each queued ref is rewritten and capped on its own, so a ref OPF - // cannot finish stays untrailered while its siblings are rewritten. - // Delivery below reads each ref's own tip trailer, and exact queue-token - // cleanup preserves any generation that advanced during this push. - opfErr = rewriteCheckpointRefsForPush(ctx, repo) + // Each queued ref is rewritten from the OPF span cache on its own; a + // ref the scan worker has not covered yet stays untrailered while its + // covered siblings are rewritten. Delivery below reads each ref's own + // tip trailer, and exact queue-token cleanup preserves any generation + // that advanced during this push. + opfErr = rewriteCheckpointRefsForPush(ctx, repo, opfApplyCachedOnly) + var pending *OPFScanPendingError + if errors.As(opfErr, &pending) { + maybeSpawnOPFScan(ctx, ps.remote) + // checkOuterPushCheckpointRefs above cannot see an outer push the + // hook did not describe; say so, as the v1 path does. + if !inOPFScanWorker(ctx) { + warnPrePushRefsUnknown(ctx, stderrWriter) + } + } } flushed, withheld, flushErr := flushCheckpointRefsQueue(ctx, repo, ps, deliveryRequiresOPFTrailer(opfDecision)) if opfErr != nil { @@ -526,7 +606,7 @@ func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pus logging.Warn(ctx, "git-refs pre-push: checkpoint ref push failed; refs left queued", slog.String("error", flushErr.Error())) } - return nil + return outerErr } // PushQueuedCheckpointRefs pushes any queued checkpoint refs to the configured @@ -551,7 +631,9 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote return 0, false, opfErr } if opfDecision == OPFRun && opfErr == nil { - opfErr = rewriteCheckpointRefsForPush(ctx, repo) + // The migration push promises an immediate push, so it waits for the + // scan here instead of handing it to the background worker. + opfErr = rewriteCheckpointRefsForPush(ctx, repo, opfScanThenApply) } var withheld int pushed, withheld, err = flushCheckpointRefsQueue(ctx, repo, ps, deliveryRequiresOPFTrailer(opfDecision)) diff --git a/cmd/entire/cli/strategy/manual_commit_push_test.go b/cmd/entire/cli/strategy/manual_commit_push_test.go index 53af8b4990..99d9de5bd8 100644 --- a/cmd/entire/cli/strategy/manual_commit_push_test.go +++ b/cmd/entire/cli/strategy/manual_commit_push_test.go @@ -2,6 +2,7 @@ package strategy import ( "context" + "github.com/entireio/cli/redact" "os/exec" "testing" @@ -60,29 +61,32 @@ func TestDeferCheckpointPushOnEmptyRemote_UsesLocalTrackingRefs(t *testing.T) { "a dedicated checkpoint remote is exempt from the guard") } -// TestPrePushCheckpointRefs_RedactedRefShipsWhileFailedSiblingStaysQueued pins -// per-ref delivery on the pre-push path: a ref OPF finished ships in the same -// push even though a sibling could not be redacted, and that sibling stays -// queued without reaching the remote. -func TestPrePushCheckpointRefs_RedactedRefShipsWhileFailedSiblingStaysQueued(t *testing.T) { +// TestPrePushCheckpointRefs_ScannedRefShipsWhileUnscannedSiblingIsHeld pins +// per-ref delivery on the pre-push path: a ref the scan worker already covered +// is rewritten from the cache and ships in the same push, while an unscanned +// sibling stays queued, never reaches the remote, and starts the worker. +func TestPrePushCheckpointRefs_ScannedRefShipsWhileUnscannedSiblingIsHeld(t *testing.T) { // No t.Parallel: the fixture uses t.Chdir. - const okID, failID = "a1b2c3d4e5f6", "b2c3d4e5f6a1" - const failSentinel = "OPFBOOM" - configureFakeOPF(t, &fakeRuntimeFailsOnSentinel{sentinel: failSentinel}) - bareDir, repo, refs := setupGitRefsOPFRepo(t, okID) - addGitRefsSessionWithTranscript(t, repo, failID, "sess-fail", - "Hello, PERSONABC asked about "+failSentinel) - refs = append(refs, mustRefName(t, id.MustCheckpointID(failID))) + const scannedID, heldID = "a1b2c3d4e5f6", "b2c3d4e5f6a1" + fake := &fakeOPFForRewrite{} + configureFakeOPF(t, fake) + bareDir, repo, refs := setupGitRefsOPFRepo(t, scannedID, heldID) + cache, err := checkpoint.OPFSpanCacheForRepo(repo) + require.NoError(t, err) + require.NoError(t, redact.ScanBlobsWithPrivacyFilter(t.Context(), checkpointRefBlobsForTest(t, repo, refs[:1]), cache)) + scanCalls := fake.batchCallCount() + spawns := swapOPFScanSpawn(t) require.NoError(t, NewManualCommitStrategy().PrePushFromGitHook(t.Context(), "origin"), - "a ref OPF could not finish must not block the user's git push") + "an unscanned ref must not block the user's git push") - require.NotEmpty(t, remoteRefHash(t, bareDir, refs[0]), - "the redacted ref must reach the remote") + require.Equal(t, scanCalls, fake.batchCallCount(), "pre-push must not call the model") + require.NotEmpty(t, remoteRefHash(t, bareDir, refs[0]), "the scanned ref must reach the remote") assertRefsAbsentFromRemote(t, bareDir, []plumbing.ReferenceName{refs[1]}, - "the ref OPF could not finish must not reach the remote") + "the unscanned ref must not reach the remote") require.Equal(t, []plumbing.ReferenceName{refs[1]}, queuedRefs(t, repo), - "only the ref OPF could not finish stays queued") + "only the unscanned ref stays queued") + require.Equal(t, []string{"origin"}, *spawns) } // TestPushQueuedCheckpointRefs_ShipsReadyRefsAndReportsTheRest is the explicit diff --git a/cmd/entire/cli/strategy/prepush_refs.go b/cmd/entire/cli/strategy/prepush_refs.go new file mode 100644 index 0000000000..197caab439 --- /dev/null +++ b/cmd/entire/cli/strategy/prepush_refs.go @@ -0,0 +1,173 @@ +package strategy + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "log/slog" + "strings" + + git "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/trailers" +) + +// PrePushRef is one line of the ref list git writes to a pre-push hook's +// stdin: the local ref and commit being pushed, and the remote ref it updates. +type PrePushRef struct { + LocalRef string + LocalSHA string + RemoteRef string + RemoteSHA string +} + +// sends reports whether the line sends content: not a deletion, and not a ref +// the remote already has at that commit. +func (r PrePushRef) sends() bool { + local := plumbing.NewHash(r.LocalSHA) + return !local.IsZero() && local != plumbing.NewHash(r.RemoteSHA) +} + +// ParsePrePushRefs reads the " " +// lines git passes to a pre-push hook and keeps the ones that touch checkpoint +// refs, the only ones the checks read. It reads to the end with no size cap: +// a `--mirror` of a repository with many tags can be large, and a truncated +// list could hide the line those checks exist to catch. Memory stays bounded +// because other lines are dropped as they are read. Malformed lines are +// skipped; a line too long to be a ref line is an error. +func ParsePrePushRefs(r io.Reader) ([]PrePushRef, error) { + refs := []PrePushRef{} + scanner := bufio.NewScanner(r) + scanner.Buffer(make([]byte, 0, 4096), 64*1024) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) != 4 { + continue + } + ref := PrePushRef{LocalRef: fields[0], LocalSHA: fields[1], RemoteRef: fields[2], RemoteSHA: fields[3]} + if isCheckpointRefName(ref.LocalRef) || isCheckpointRefName(ref.RemoteRef) { + refs = append(refs, ref) + } + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("read pre-push refs: %w", err) + } + return refs, nil +} + +type prePushRefsKey struct{} + +// prePushRefsKnown reports whether the hook passed the ref list, which an old +// hook script or an old hook-manager line does not. +func prePushRefsKnown(ctx context.Context) bool { + _, ok := ctx.Value(prePushRefsKey{}).([]PrePushRef) + return ok +} + +// warnPrePushRefsUnknown tells a user whose pre-push line predates the ref list +// that a push sending checkpoint refs itself cannot be checked, at the one +// moment it matters: checkpoints OPF has not verified are being held back. +func warnPrePushRefsUnknown(ctx context.Context, w io.Writer) { + if prePushRefsKnown(ctx) { + return + } + fmt.Fprintln(w, "[entire] Warning: this pre-push hook does not pass the refs being pushed, so a push that "+ + "includes Entire's checkpoint refs itself (e.g. `git push --all`) would send them unscanned. If you use a hook "+ + "manager such as Husky, replace its Entire pre-push line with the one `entire enable` prints; otherwise run "+ + "`entire enable` to update the hook.") +} + +// WithPrePushRefsFrom parses git's pre-push ref list from r and records it. A +// list that cannot be read leaves the refs unknown, as with an old hook: OPF +// still applies to Entire's own push and warns that an outer push cannot be +// checked, and the user's push is not blocked over it. +func WithPrePushRefsFrom(ctx context.Context, r io.Reader) context.Context { + refs, err := ParsePrePushRefs(r) + if err != nil { + logging.Warn(ctx, "pre-push: could not read the ref list; treating it as unknown", + slog.String("error", err.Error())) + return ctx + } + return WithPrePushRefs(ctx, refs) +} + +// WithPrePushRefs records the refs the user's own push is about to update, so +// prePush can refuse to let it carry checkpoint content OPF has not verified. +// Without it (an old hook script, a hook manager line, or a caller that is not +// a hook) prePush cannot see the outer push and behaves as before. +func WithPrePushRefs(ctx context.Context, refs []PrePushRef) context.Context { + if refs == nil { + refs = []PrePushRef{} // known to be empty, unlike absent + } + return context.WithValue(ctx, prePushRefsKey{}, refs) +} + +// outerPushV1SHA returns the commit the user's own push is sending to +// entire/checkpoints/v1, if it sends one (see PrePushRef.sends). +func outerPushV1SHA(ctx context.Context) (plumbing.Hash, bool) { + refs, _ := ctx.Value(prePushRefsKey{}).([]PrePushRef) //nolint:errcheck // absent means "not known" + v1 := plumbing.NewBranchReferenceName(paths.MetadataBranchName).String() + for _, r := range refs { + if (r.LocalRef == v1 || r.RemoteRef == v1) && r.sends() { + return plumbing.NewHash(r.LocalSHA), true + } + } + return plumbing.ZeroHash, false +} + +// isCheckpointRefName reports whether name holds checkpoint content: the v1 +// branch or a per-checkpoint ref. +func isCheckpointRefName(name string) bool { + return name == plumbing.NewBranchReferenceName(paths.MetadataBranchName).String() || + strings.HasPrefix(name, checkpoint.CheckpointRefPrefix) +} + +// checkOuterPushCheckpointRefs is the git-refs counterpart of checkOuterPushV1: +// it refuses the user's push when it sends checkpoint content (a per-checkpoint +// ref, or a v1 branch left from before a migration) whose commit does not carry +// the OPF trailer. The trailer is the same proof Entire's own delivery requires. +func checkOuterPushCheckpointRefs(ctx context.Context, repo *git.Repository) error { + refs, _ := ctx.Value(prePushRefsKey{}).([]PrePushRef) //nolint:errcheck // absent means "not known" + for _, r := range refs { + if !r.sends() || (!isCheckpointRefName(r.LocalRef) && !isCheckpointRefName(r.RemoteRef)) { + continue + } + commit, err := repo.CommitObject(plumbing.NewHash(r.LocalSHA)) + if err == nil && trailers.HasOPFApplied(commit.Message) { + continue + } + return fmt.Errorf("%w: this push includes %s, which the OpenAI Privacy Filter has not verified. "+ + "Leave it out of this push (avoid --all/--mirror for checkpoint refs); Entire pushes checkpoints itself once they are scanned", + ErrOuterPushCarriesUnverifiedCheckpoints, r.LocalRef) + } + return nil +} + +// ErrOuterPushCarriesUnverifiedCheckpoints aborts a user push that sends +// checkpoint refs OPF has not verified, on the git-refs backend. +var ErrOuterPushCarriesUnverifiedCheckpoints = errors.New("refusing to push unverified checkpoint content") + +// ErrOuterPushCarriesUnverifiedV1 aborts a user push that includes +// entire/checkpoints/v1 at a commit OPF has not verified. Entire's own push of +// v1 cannot stop it: git already chose what to send before the hook ran. +var ErrOuterPushCarriesUnverifiedV1 = errors.New("this push includes entire/checkpoints/v1 with checkpoints the OpenAI Privacy Filter has not verified") + +// checkOuterPushV1 refuses the user's push when it sends entire/checkpoints/v1 +// at anything other than verified, the tip OPF has just verified (zero when +// the checkpoints are still waiting for the background scan). +func checkOuterPushV1(ctx context.Context, verified plumbing.Hash) error { + sha, ok := outerPushV1SHA(ctx) + if !ok || (!verified.IsZero() && sha == verified) { + return nil + } + if verified.IsZero() { + return fmt.Errorf("%w; they are being scanned in the background. Push again once `entire status` shows none pending, or leave entire/checkpoints/v1 out of this push", ErrOuterPushCarriesUnverifiedV1) + } + return fmt.Errorf("%w; it was just rewritten with the filter applied. Run the push again", ErrOuterPushCarriesUnverifiedV1) +} diff --git a/cmd/entire/cli/strategy/prepush_refs_test.go b/cmd/entire/cli/strategy/prepush_refs_test.go new file mode 100644 index 0000000000..da662bf521 --- /dev/null +++ b/cmd/entire/cli/strategy/prepush_refs_test.go @@ -0,0 +1,100 @@ +package strategy + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" +) + +const ( + testSHA1 = "1111111111111111111111111111111111111111" + testSHA2 = "2222222222222222222222222222222222222222" + zeroSHA = "0000000000000000000000000000000000000000" +) + +func TestParsePrePushRefs(t *testing.T) { + t.Parallel() + const v1 = "refs/heads/entire/checkpoints/v1" + refs, err := ParsePrePushRefs(strings.NewReader( + "refs/heads/main " + testSHA1 + " refs/heads/main " + testSHA2 + "\n" + + "malformed line\n" + + "\n" + + v1 + " " + testSHA1 + " " + v1 + " " + zeroSHA + "\n" + + "(delete) " + zeroSHA + " refs/entire/checkpoints/ab/cdef " + testSHA2 + "\n")) + require.NoError(t, err) + require.Equal(t, []PrePushRef{ + {LocalRef: v1, LocalSHA: testSHA1, RemoteRef: v1, RemoteSHA: zeroSHA}, + {LocalRef: "(delete)", LocalSHA: zeroSHA, RemoteRef: "refs/entire/checkpoints/ab/cdef", RemoteSHA: testSHA2}, + }, refs, "only lines that touch checkpoint refs are kept") +} + +// A `--mirror` of a repository with many tags sends a long ref list; the v1 +// line at its end must still be seen, not cut off by a size cap. +func TestParsePrePushRefs_LongListKeepsTheLastLine(t *testing.T) { + t.Parallel() + const v1 = "refs/heads/entire/checkpoints/v1" + var b strings.Builder + for i := 0; b.Len() < 20<<20; i++ { + fmt.Fprintf(&b, "refs/tags/t%d %s refs/tags/t%d %s\n", i, testSHA1, i, zeroSHA) + } + b.WriteString(v1 + " " + testSHA2 + " " + v1 + " " + zeroSHA + "\n") + + refs, err := ParsePrePushRefs(strings.NewReader(b.String())) + require.NoError(t, err) + require.Len(t, refs, 1) + require.Equal(t, testSHA2, refs[0].LocalSHA) +} + +func TestCheckOuterPushV1(t *testing.T) { + t.Parallel() + const v1 = "refs/heads/entire/checkpoints/v1" + verified := plumbing.NewHash(testSHA1) + for _, tc := range []struct { + name string + refs []PrePushRef + verified plumbing.Hash + wantErr bool + }{ + {name: "no ref list (old hook script)", refs: nil, verified: plumbing.ZeroHash}, + {name: "push without v1", refs: []PrePushRef{{"refs/heads/main", testSHA2, "refs/heads/main", zeroSHA}}, verified: plumbing.ZeroHash}, + {name: "v1 at the verified tip", refs: []PrePushRef{{v1, testSHA1, v1, zeroSHA}}, verified: verified}, + {name: "v1 at a pre-rewrite commit", refs: []PrePushRef{{v1, testSHA2, v1, zeroSHA}}, verified: verified, wantErr: true}, + {name: "v1 while held for the scan", refs: []PrePushRef{{v1, testSHA1, v1, zeroSHA}}, verified: plumbing.ZeroHash, wantErr: true}, + {name: "v1 pushed under another name", refs: []PrePushRef{{v1, testSHA2, "refs/heads/backup", zeroSHA}}, verified: verified, wantErr: true}, + {name: "another branch pushed onto v1", refs: []PrePushRef{{"refs/heads/main", testSHA2, v1, zeroSHA}}, verified: verified, wantErr: true}, + {name: "v1 the remote already has", refs: []PrePushRef{{v1, testSHA2, v1, testSHA2}}, verified: verified}, + {name: "deleting remote v1", refs: []PrePushRef{{"(delete)", zeroSHA, v1, zeroSHA}}, verified: plumbing.ZeroHash}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + if tc.refs != nil { + ctx = WithPrePushRefs(ctx, tc.refs) + } + err := checkOuterPushV1(ctx, tc.verified) + if tc.wantErr { + require.ErrorIs(t, err, ErrOuterPushCarriesUnverifiedV1) + } else { + require.NoError(t, err) + } + }) + } +} + +type failingReader struct{} + +func (failingReader) Read([]byte) (int, error) { return 0, errors.New("stdin went away") } + +// A ref list that cannot be read leaves the refs unknown, as an old hook does, +// rather than failing the user's push over it. +func TestWithPrePushRefsFrom_UnreadableListIsUnknown(t *testing.T) { + t.Parallel() + require.False(t, prePushRefsKnown(WithPrePushRefsFrom(context.Background(), failingReader{}))) + require.True(t, prePushRefsKnown(WithPrePushRefsFrom(context.Background(), + strings.NewReader("refs/heads/main "+testSHA1+" refs/heads/main "+testSHA2+"\n")))) +} diff --git a/cmd/entire/cli/trail_context_cache.go b/cmd/entire/cli/trail_context_cache.go index f0188b85d0..9acf513117 100644 --- a/cmd/entire/cli/trail_context_cache.go +++ b/cmd/entire/cli/trail_context_cache.go @@ -14,15 +14,13 @@ import ( "github.com/entireio/cli/cmd/entire/cli/api" "github.com/entireio/cli/cmd/entire/cli/auth" "github.com/entireio/cli/cmd/entire/cli/execx" - "github.com/entireio/cli/cmd/entire/cli/gitdir" "github.com/entireio/cli/cmd/entire/cli/gitremote" - "github.com/entireio/cli/cmd/entire/cli/internal/flock" "github.com/entireio/cli/cmd/entire/cli/jsonutil" "github.com/entireio/cli/cmd/entire/cli/logging" - "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/session" "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/spawnmarker" "github.com/entireio/cli/cmd/entire/cli/validation" "github.com/spf13/cobra" @@ -436,57 +434,11 @@ func spawnDetachedTrailEnablementRefresh(ctx context.Context) { // trailRefreshRecentlySpawned reports whether a detached refresh was spawned for // this repo within trailRefreshSpawnThrottle and, when it wasn't, records now as -// the most recent spawn. The read-and-record is serialized with a flock keyed to -// the shared git-common-dir (so every worktree of the repo agrees), collapsing a -// burst of concurrent SessionStart hooks to a single child rather than one per -// hook. Best-effort: any error resolving, locking, or writing the marker falls -// through to spawning — never worse than before this guard existed. +// the most recent spawn. See spawnmarker.RecentlySpawned for the mechanism. func trailRefreshRecentlySpawned(commonDir string, now time.Time) bool { - return recentlySpawnedMarker(commonDir, "trail-refresh-spawn", trailRefreshSpawnThrottle, now) + return spawnmarker.RecentlySpawned(commonDir, "trail-refresh-spawn", trailRefreshSpawnThrottle, now) } -// recentlySpawnedMarker reports whether the named spawn marker under the -// shared git-common-dir was refreshed within ttl and, when it wasn't, records -// now as the most recent spawn. The read-and-record is serialized with a flock -// keyed to the marker (so every worktree of the repo agrees), collapsing a -// burst of concurrent hooks to a single detached child rather than one per -// hook. Best-effort: any error resolving, locking, or writing the marker falls -// through to spawning — never worse than having no guard at all. Shared by the -// trail-enablement refresh and the zombie-session sweep, each with its own -// marker name and ttl. -func recentlySpawnedMarker(commonDir, marker string, ttl time.Duration, now time.Time) bool { - root, err := gitdir.OpenAt(commonDir) - if err != nil { - return false - } - // Create the directory before acquiring the lock: flock opens the lock file, - // which fails if its parent doesn't exist yet (mirrors - // ModifyClonePreferences, which creates before locking). - if err := osroot.MkdirAllNoSymlink(root, spawnMarkerDirName, 0o750); err != nil { - return false - } - markerName := spawnMarkerDirName + "/" + marker - release, err := flock.AcquireIn(root, markerName+".lock") - if err != nil { - return false - } - defer release() - - if data, readErr := osroot.ReadFileNoFollow(root, markerName); readErr == nil { - if last, parseErr := time.Parse(time.RFC3339Nano, strings.TrimSpace(string(data))); parseErr == nil && - now.After(last) && now.Sub(last) < ttl { - return true - } - } - //nolint:errcheck // best-effort marker; a failed write just means the next hook re-spawns - _ = jsonutil.WriteFileAtomicIn(root, markerName, []byte(now.UTC().Format(time.RFC3339Nano)), 0o600) - return false -} - -// spawnMarkerDirName is the marker directory inside the git common dir. It is -// the same "entire" directory clone preferences live in. -const spawnMarkerDirName = "entire" - // newRefreshTrailEnablementCmd creates the hidden command that performs the // (potentially slow) trails-enablement network refresh out of band. It is // invoked by spawnDetachedTrailEnablementRefresh from a detached subprocess diff --git a/docs/development/opf-bug-investigation.md b/docs/development/opf-bug-investigation.md new file mode 100644 index 0000000000..959e146482 --- /dev/null +++ b/docs/development/opf-bug-investigation.md @@ -0,0 +1,112 @@ +# The bug: OPF's batch cap is smaller than a single real checkpoint + +`OPF-INVESTIGATION.md` in this worktree asked whether OPF had ever run on a real +transcript. The answer came back **no** — zero commits carrying +`Entire-OPF-Applied` anywhere. That is the *corroboration*. This file is the +finding it corroborates. + +## The defect + +`batchDefaultLimit = 2 * 1024 * 1024` prose-leaf bytes +(`cmd/entire/cli/strategy/manual_commit_opf_rewrite.go:185`) is **below the size +of one real checkpoint's transcript**. Exceed it and +`RewriteQueuedCheckpointRefsWithOPF` / `RewriteUnpushedV1WithOPF` return +`OPFBatchTooLargeError` and rewrite **nothing**. + +Measured against 504 real checkpoint refs in a working checkout +(`.worktrees/linear-2`): + +| | | +| --- | --- | +| local checkpoint refs | 504 | +| median checkpoint tip-commit tree, raw | ~5 MB | +| tip trees exceeding 2 MiB **raw** (sample of 20) | **18** | +| a median tree's `0/full.jsonl` alone | 3.9 MB | +| prose-leaf ratio of a real `full.jsonl` | ~0.84 | +| whole un-trailered ancestry per ref, raw (n=80) | median ~9.5 MB | + +`isRedactableBlobName` (`manual_commit_opf_rewrite.go:613-615`) excludes exactly +one file, `content_hash.txt` — so `full.jsonl`, the session transcript, is in +the batch. The cap counts almost exactly the thing checkpoints are made of. + +So: **with OPF enabled, an ordinary push carrying one real session already trips +the cap.** No unusual configuration, no backlog, no destination change required. + +## Both backends, different failure modes + +Both share the cap — `resolveBatchLimit()` and `redact.SumProseLeafBytes` are +called identically at `manual_commit_opf_rewrite.go:372-373` (git-branch/v1) and +`manual_commit_opf_refs.go:132-135` (git-refs). + +- **git-branch** — the rewrite error propagates out of `prePush`, so the user's + `git push` **aborts**. Loud and immediate. +- **git-refs** — `opfGateForCheckpointRefs` warns and withholds the flush + (`manual_commit_push.go:441-444`), and the user's push succeeds. The queue is + untouched, so the next push builds the identical batch and fails identically. + **Quiet, and permanent.** The only documented way out is + `ENTIRE_OPF_BATCH_LIMIT`. + +## Why nobody has reported it + +`redaction.openai_privacy_filter.enabled` defaults to **off**, and it isn't +enabled in this repo. Combined with zero OPF-stamped commits anywhere, the +likeliest explanation is that the feature has never been exercised against +real-sized content — only against small synthetic test fixtures with a faked +runtime. + +"OPF aborts every push I make" (git-branch) would have been reported within a +day. It hasn't been. + +## What needs deciding + +1. **Is 2 MiB the right number?** Its comment sizes it by inference time — + "2 MB at ~5.4s/100KB ≈ ~110s… generous enough that any realistic push fits." + The first half may be right and the second half is measurably false. What is + the real cost of a 4 MB transcript at that rate, and is a multi-minute pause + on every push acceptable? If not, the feature needs a different shape, not a + bigger number. + +2. **Does the measure double-count?** `collectTreeBlobs` walks the whole tree of + **every** un-applied commit, and checkpoint ref chains carry near-identical + trees — on at least one ref, two consecutive commits have byte-identical + trees. `SumProseLeafBytes` explicitly does not dedup + (`redact/batch.go:196-201`) while the batch redactor dedups by leaf text. So + the cap may be enforcing a cost that isn't actually paid. Deduping the + collect pass by blob hash looks like a few lines. + +3. **Should the cap be cumulative at all?** Raw and prose-leaf caps are summed + across the whole flush; the bootstrap cap is per-ref + (`manual_commit_opf_refs.go:100-102`). Refs are otherwise independent — each + is pushed separately, each has its own ancestry. + +4. **`full.jsonl` vs `transcript.jsonl`.** Both are in the tree and both are the + same session (3.9 MB and 369 KB in the sampled tree). Does OPF need to scan + both? + +## Rejected approaches, with reasons + +Do not re-propose these. + +- **Chunking the rewrite** (process as many refs as fit, leave the rest queued). + Chunks must be ref-atomic — a commit chain can't be rewritten in parts, since + each rebuilt commit is the next one's parent. With most refs individually over + the cap, the "oversized ref processed alone" escape becomes the normal path, + which silently disables the cap rather than preserving it. Full write-up, with + an accurate diagnosis worth reading, in + `.worktrees/linear-2/docs/superpowers/plans/2026-09-16-opf-must-make-progress.md`. +- **Running the v1 rewrite on a push where checkpoint delivery is gated.** The + target carries no checkpoints, so `resolveRemoteV1Tip` returns zero, the whole + local history is treated as a bootstrap, and `rebuildCheckpointCommit` builds + the deepest commit with **no parents** (`:568-571`). Local + `entire/checkpoints/v1` gets CAS-updated to an unrelated root chain and every + later push diverges permanently. Repos with >100 unpushed v1 commits fail + harmlessly on the bootstrap cap; smaller ones are damaged — the risk is + inverted. + +## What this blocks + +`.worktrees/linear-2` holds a committed, green change +(`peyton/checkpoint-destination-redelivery`) that re-delivers checkpoints when +their destination changes. It doesn't break anything that works today, but it +adds another route into this hole, so it's on hold pending a direction here. +See `docs/superpowers/plans/2026-09-17-checkpoint-remote-handoff.md` there. diff --git a/docs/development/opf-throughput-findings.md b/docs/development/opf-throughput-findings.md new file mode 100644 index 0000000000..d3c34933c6 --- /dev/null +++ b/docs/development/opf-throughput-findings.md @@ -0,0 +1,73 @@ +# OPF real-runtime throughput findings + +Follows the branch that shipped in PR #2533 (dedup fix, per-ref cap scoping, +detached worker, status visibility). That PR fixed the *measurement* and +*blast-radius* bugs. This file records what real benchmarking of the actual +`opf` binary found afterward, which is why a fourth piece of work +(`docs/superpowers/plans/2026-09-18-opf-per-ref-delivery.md`) exists. + +## The finding + +`batchDefaultLimit` (2 MiB) was sized from a comment's estimate of +"~5.4s/100KB." Benchmarking the real `opf` binary — the exact invocation +`redact/opf.go`'s `shellOut.RedactBatch` uses +(`opf --device cpu --output-mode typed --format json --no-print-color-coded-text`, +fed via stdin) — against generated prose text of several sizes gave: + +| Size | Wall time | Rate | +| --- | --- | --- | +| 1 KB | 1.2s | — | +| 10 KB | 9.0s | 0.90 s/KB | +| 50 KB | 42.7s | 0.85 s/KB | +| 100 KB | 86.0s | 0.86 s/KB | +| 250 KB | 284.6s | 1.14 s/KB | +| 500 KB | 568.3s | 1.14 s/KB | + +Real rate is **~16x slower** than the code's own estimate, and it does not +stay at the small-input rate — it degrades to a sustained **~1.14 s/KB** +beyond roughly 250KB and holds there through 500KB. Treat 1.14 s/KB as the +number to design around, not the optimistic 0.85s/KB from small inputs. + +At this rate the *current* 2 MiB cap would cost roughly **29 minutes** of +inference if it were ever allowed to run to completion, and a real multi-MB +session transcript costs on the order of an hour. No cap value makes typical +real-session content redact quickly — the original assumption behind sizing +the cap for push-latency was never true at this throughput. + +## MPS acceleration: checked, not viable + +This machine has Apple Silicon with MPS available and built +(`torch.backends.mps.is_available()` → `True`). Two blockers: + +1. The model's MoE layers use fused kernels gated on the `triton` package + (`OPF_MOE_TRITON`), which is not installed and is not a supported + Apple-Silicon package (Triton targets NVIDIA/AMD). +2. With `OPF_MOE_TRITON=0` to fall back to the non-fused path, MPS is + **slower than CPU**, not faster — confirmed at two sizes: + - 1 KB: MPS 1.3s vs CPU 1.2s + - 100 KB: MPS 133.3s vs CPU 86.0s (55% slower) + +Conclusion: CPU is the fastest option available on this stack today. There +is no hardware-acceleration escape hatch to lean on when picking new +numbers — the fix has to be architectural (don't let a slow-but-working ref +block delivery of its siblings), not a faster runtime. + +## What this changes about the plan + +- `batchDefaultLimit` should stop trying to answer "will this be fast" (it + can't) and instead answer "is this plausibly a real session or a + runaway/corrupted one" — a much larger sanity ceiling, not a tuned number. +- The OPF shell-out's fixed 30s `timeout_seconds` default is already too + short for real content at this rate (100KB alone takes ~86s) — it needs to + scale with input size or it will misclassify legitimately-slow-but-working + redaction as a broken runtime, which trips the process-wide circuit + breaker and is worse than a plain cap rejection. +- The actual fix for "checkpoints don't ship" is not the cap or the timeout + — it's that today the whole flush is withheld if *any* queued ref isn't + redacted yet, and given real timing, "still redacting" will be the normal + state on any active repo. Delivery needs to ship per-ref: already-redacted + refs go out, still-cooking or perma-failing ones stay queued, without + holding the rest hostage. + +See `docs/superpowers/plans/2026-09-18-opf-per-ref-delivery.md` for the +concrete implementation plan. diff --git a/docs/security-and-privacy.md b/docs/security-and-privacy.md index e90cbb65bf..5e0973ceaa 100644 --- a/docs/security-and-privacy.md +++ b/docs/security-and-privacy.md @@ -192,7 +192,7 @@ Full settings reference: ``` - `command` — path or PATH-resolvable name of the `opf` binary. Defaults to `opf`. **Only read from `.entire/settings.local.json`**, and only when that file is untracked; see [Why `command` is local-only](#why-command-is-local-only). -- `timeout_seconds` — per-invocation timeout. Defaults to `30`. +- `timeout_seconds` — deadline for one `opf` invocation. Leave it unset: the deadline is then scaled to the size of each call (at most 1 MiB of text), from a 30s floor to a 3-hour ceiling. Setting it pins a fixed deadline, which is only useful for tests or deliberately constrained environments. - `prompt_default` — `"ask"` (default), `"never"`, or `"always"`. Controls whether the pre-push hook surfaces an interactive prompt before running OPF. `ENTIRE_OPF=yes` or `ENTIRE_OPF=no` on a single `git push` invocation overrides this for that push only. ### Why `command` is local-only @@ -218,8 +218,8 @@ The interactive prompt offers three options and reacts to **Ctrl-C** for cancell ``` Run OpenAI Privacy Filter on these checkpoints? -Adds ~30s but redacts names/PII the regex layers can't catch. -Ctrl-C to cancel the push. +Scans in the background after this push and sends checkpoints when done; +redacts names/PII the regex layers can't catch. Ctrl-C to cancel the push. ▸ Yes — run OPF this push No — skip OPF, push as-is @@ -231,28 +231,24 @@ Ctrl-C to cancel the push. - **Always** runs OPF this push AND writes `prompt_default: "always"` to `.entire/settings.local.json` so future pushes don't ask. - **Ctrl-C** cancels OPF. What that costs depends on the backend: on `git-branch` your `git push` aborts and exits non-zero; on `git-refs` your push completes and the checkpoint refs stay queued for a later push. Either way nothing under-redacted reaches the remote. -Non-interactive contexts (CI, scripted pipes with no TTY) skip the prompt and run OPF automatically when enabled, printing `→ OpenAI Privacy Filter: scanning checkpoints before push (may take ~30s)…` to stderr so the wait isn't silent. Progress and completion are reported as `→ OpenAI Privacy Filter: scanning checkpoints…` and `✓ OpenAI Privacy Filter: done (12.4s, 37 blobs)`. Set `ENTIRE_OPF=no` to skip OPF in those contexts without disabling the feature globally. +Non-interactive contexts (CI, scripted pipes with no TTY) skip the prompt and choose OPF automatically when enabled, printing `→ OpenAI Privacy Filter: enabled for checkpoint processing` to stderr. Set `ENTIRE_OPF=no` to skip OPF in those contexts without disabling the feature globally. -**CI consideration**: if you've enabled OPF locally and your CI runs `git push` (e.g. an agent-driven workflow), the CI push will attempt to run OPF too. If the `opf` binary isn't installed in CI, the failure is fail-closed rather than silently shipping under-redacted content — by design, since "I enabled OPF" should mean "no content leaves my machines without OPF." On `git-branch` that aborts the CI push; on `git-refs` the push succeeds but the checkpoints don't ship, which means they can accumulate unpushed until someone notices. The remedies are (a) install `opf` in CI, (b) set `ENTIRE_OPF=no` for CI pushes, or (c) set `prompt_default: "never"` if you only want OPF on interactive pushes. +**OPF never runs inside `git push`.** Inference is slow (about 1.14 s per KB of prose on CPU; see [OPF throughput findings](development/opf-throughput-findings.md)), so a real session is minutes to hours of model time. Instead, the push holds back any checkpoint OPF has not scanned yet, prints `[entire] Checkpoints are being scanned by the OpenAI Privacy Filter in the background and will be pushed when it finishes.`, and starts a background worker. Your own `git push` completes immediately on both backends. `entire status` shows how many unpushed checkpoints have not been through the filter yet. The worker pushes each batch as soon as it is scanned and saves its progress after every model call, so an interrupted scan resumes where it stopped. A push that sends checkpoint content itself is refused while that content is unverified: on `git-branch`, `entire/checkpoints/v1` (`git push --all`, `--mirror`, or naming the branch) until the scan finishes; on `git-refs`, any `refs/entire/checkpoints/*` ref (`--mirror`, an explicit refspec) or a `entire/checkpoints/v1` branch left from before a migration whose commit lacks the OPF trailer. Leave those refs out of the push; Entire pushes checkpoints itself once they are scanned. `ENTIRE_OPF=no` on the push lets it through, unscanned. Entire learns what a push sends from the ref list git gives the pre-push hook, so this needs the hook script Entire installs (it reinstalls older ones). With a hook manager such as Husky, use the pre-push line `entire enable` prints: it reads the list and hands it on unchanged to the commands after it (`git lfs pre-push`, for example). An older line that does not pass the list cannot be checked, and Entire warns when that matters. -OPF failures at push time are **fail-closed**: if OPF is not on PATH, fails to start, or times out during the pre-push rewrite, the per-process circuit breaker trips and no under-redacted content reaches the remote. The intent is that "the user enabled OPF" means "I do not want unredacted content leaving this machine" — falling back to 8-layer silently on the push path would violate that contract. Fix the install or set `ENTIRE_OPF=no` for a one-off push. +**CI consideration**: in CI or a throwaway container the background worker usually dies with the job, so checkpoints from those runs stay held locally rather than shipping unscanned. If you want them, install `opf` and push again before the job ends; otherwise set `ENTIRE_OPF=no` or `prompt_default: "never"` for CI pushes. -How that is enforced depends on the checkpoint backend, because they have different escape hatches: +OPF failures are **fail-closed**: if the `opf` binary is missing, fails to start, or times out, the worker's circuit breaker trips, nothing is stamped `Entire-OPF-Applied`, and the held checkpoints stay local. Nothing under-redacted reaches the remote, and your pushes keep succeeding. Each later push restarts the worker, so fixing the install is enough; `entire status` keeps counting the held checkpoints and `.entire/logs/entire.log` records why the worker could not finish. The explicit `entire doctor migrate-checkpoints` push is the exception: it scans synchronously and reports an OPF failure directly. -- **git-branch**: the rewrite aborts the push with `OPF runtime failed during pre-push rewrite (command=…); aborting push so regex-only content isn't tagged as OPF-applied`. Your `git push` exits non-zero. The checkpoint branch travels with that push, so refusing the push is the only way to withhold it. -- **git-refs**: checkpoint refs are pushed separately from your branch and stay queued when they are not flushed, so the failure withholds the checkpoint refs OPF did not finish and lets your own `git push` succeed. Refs already rewritten earlier in the same push still ship. Nothing under-redacted ships either way. This is not silent: a warning names the failure and states that checkpoint refs stayed queued for the next push. - -(The circuit breaker is per-process, so a broken install costs one warning instead of one timeout per blob — but the push still aborts.) - -Cost note: each shell-out loads the OPF model (~1.5B parameters on CPU). The pre-push rewrite batches **every redactable leaf across every unpushed commit** — v1 commits on git-branch, every unpushed commit on every queued ref on git-refs — into a single inference pass, so a typical real-world push pays the model-load cost once (~6s) plus inference (~5s per 100KB of leaf content) — not multiplied by the number of commits or blobs. A 3-commit push with ~250KB of total prose content runs in ~12–15s, not the ~50–100s a per-blob flow would take. Per-commit latency is unaffected because OPF doesn't run at commit time. +Cost note: each `opf` call loads the model (~1.5B parameters on CPU, about 6s). Redactable leaves are deduplicated across every held checkpoint and sent in calls of at most 1 MiB of text, and results are cached per blob in the git common directory, so nothing is scanned twice. Commit latency is unaffected because OPF never runs at commit time. #### When OPF actually runs -OPF execution lives in the pre-push hook. The flow: - 1. **Post-commit** writes the checkpoint with **8-layer-only** redaction to local git objects — per-checkpoint refs on `git-refs`, the `entire/checkpoints/v1` branch on `git-branch`. Fast, predictable, no OPF cost on the hot path. -2. **Pre-push** (`git push`): if OPF is enabled, the hook re-reads each not-yet-OPF'd commit, runs the OpenAI Privacy Filter over its blobs to add the categories the regex layers don't catch (person names, addresses, etc.), and builds **new commits** carrying an `Entire-OPF-Applied: true` trailer. Each backend then points its own local ref at the new tip with a compare-and-swap, and the (now 9-layer-redacted) commits are what get pushed. -3. The original 8-layer-only commits become **unreachable** in the local git object database and eventually get swept by `git gc`. +2. **Pre-push** (`git push`): if the push resolves to run OPF, the hook rewrites each not-yet-OPF'd commit from the **OPF span cache** — results the background scan stored earlier — and builds **new commits** carrying an `Entire-OPF-Applied: true` trailer. It never calls the model. Anything the cache does not cover yet is held back: on `git-refs` that checkpoint ref stays queued while covered siblings ship; on `git-branch` v1 is not pushed this time. The hook then starts the scan worker for the remote you pushed to. +3. **The scan worker** (`entire __opf_scan `, one per repository at a time) collects the held commits, runs OPF over whatever the cache lacks, and then runs the same pre-push delivery for that remote, so the checkpoints are pushed as soon as the scan finishes. It acts only on the decision your push made, pushes only to the remote your push named, uses non-interactive git, and delivers a checkpoint only once its exact commit carries the trailer. If it cannot reach the remote, the checkpoints stay held and your next push delivers them. +4. The original 8-layer-only commits become **unreachable** in the local git object database and eventually get swept by `git gc`. + +The span cache (`entire-opf-cache/` in the git common directory) stores, per blob, only SHA-256 hashes of leaf text and the offsets and labels OPF found, never the text itself. Entries are keyed by the blob's object hash, the configured OPF command and the enabled categories, so enabling a category or pointing Entire at a different OPF install rescans everything. A model upgraded in place behind the same command is not detected; Entire bumps the cache version when its own OPF handling changes. Entries older than 30 days are pruned by the worker, and `entire clean --all` removes the whole cache. A worker stops at its next pass once the settings say OPF should never run. The two backends differ in **how they find the commits to rewrite**: @@ -270,7 +266,7 @@ Note that the local ref move is **not** a fast-forward on either backend — the This means: - **The remote only ever sees 9-layer-redacted content** when OPF is enabled. -- **Local-only commits are 8-layer-redacted** until the moment you push. If you never push, OPF never runs. +- **Local-only commits are 8-layer-redacted** until they are scanned after a push. If you never push, OPF never runs. - **Re-running pre-push is idempotent** — commits already carrying the trailer are never re-redacted. #### Divergence, caps, and concurrent pushes @@ -291,7 +287,7 @@ set -x ENTIRE_OPF_BOOTSTRAP_LIMIT unlimited; git push The two backends trip this differently. On `git-branch` it applies **only on bootstrap** — the first push, when the remote has no v1 yet — counted across all unpushed commits. On `git-refs` it applies on **every** push, counted **per queued ref** over that ref's un-trailered ancestry. In practice the `git-refs` trigger is "OPF was enabled late" or "checkpoints were just migrated from the branch", not "first push". -**Batch cap: `2 MiB` of prose-leaf content by default** (≈110s of inference), counted per checkpoint ref on `git-refs` and across the whole unpushed chain on `git-branch`. Identical leaves are counted once, matching what OPF actually processes. An `OPF would run inference on …` error means you've hit it: +**Batch cap: `128 MiB` of prose-leaf content by default**, counted per checkpoint ref on `git-refs` and across the whole unpushed chain on `git-branch`. Identical leaves are counted once, matching what OPF actually processes. It is a backstop against broken input (a corrupted transcript, an embedded binary), not a size limit for real sessions: the largest real sessions measured hold a few MiB of prose ([investigation](development/opf-bug-investigation.md)), and a large session just takes longer to scan. An `OPF would run inference on …` error means you've hit it: ```fish set -x ENTIRE_OPF_BATCH_LIMIT 10485760; git push # 10 MiB @@ -299,11 +295,11 @@ set -x ENTIRE_OPF_BATCH_LIMIT 10485760; git push # 10 MiB set -x ENTIRE_OPF_BATCH_LIMIT unlimited; git push ``` -**Raw-byte cap: `200 MiB` of blob content buffered in memory**, counted per checkpoint ref on `git-refs` (one ref is collected, scanned, rebuilt, and released before the next is loaded) and across the whole unpushed chain on `git-branch`. It has no env var of its own — it is derived as 100× the batch cap, so raising `ENTIRE_OPF_BATCH_LIMIT` raises it too. Each blob's size is checked against the remaining budget before its content is read, so on a pathological push (one commit carrying a huge pasted transcript) this is the cap that fires first, before the oversized allocation. +**Raw-byte cap: `256 MiB` of blob content buffered in memory**, counted per checkpoint ref on `git-refs` (one ref is collected, scanned, rebuilt, and released before the next is loaded) and across the whole unpushed chain on `git-branch`. It has no env var of its own — it is derived as 2× the batch cap, so raising `ENTIRE_OPF_BATCH_LIMIT` raises it too. Each blob's size is checked against the remaining budget before its content is read, so on a pathological push (one commit carrying a huge pasted transcript) this is the cap that fires first, before the oversized allocation. The three caps protect different failure modes: the commit cap stops "100 throwaway commits", the batch cap stops "one commit with 50 MB of prose", and the raw-byte cap stops the loader exhausting memory before either of the others can be evaluated. On `git-refs` all three caps are per ref: a ref that trips one stays queued untouched while its siblings are still rewritten and ship. -**Concurrent push** from another worktree: both backends compare-and-swap the local ref. If another process moved it while OPF was running, `git-branch` exits with `entire/checkpoints/v1 moved during OPF rewrite …; re-run 'git push' (no fetch needed; the move was local)` and aborts. On `git-refs` the affected generation stays queued and the next push picks it up. Each ref is collected, scanned, rebuilt, and compare-and-swapped before the next one starts, so updates are not atomic *across* refs: earlier refs can already be rewritten when a later one conflicts. Delivery pushes the exact hash whose trailer it checked and removes only that generation's queue entry, so a concurrent advance of the same ref can neither substitute unchecked content nor be erased from the queue. +**Concurrent push** from another worktree: only one scan worker runs per repository; a worker started while another holds the lock exits immediately, and the running one re-collects its work after every pass. Both backends compare-and-swap the local ref. If another process moved it while OPF was running, `git-branch` exits with `entire/checkpoints/v1 moved during OPF rewrite …; re-run 'git push' (no fetch needed; the move was local)` and aborts. On `git-refs` the affected generation stays queued and the next push picks it up. Each ref is collected, scanned, rebuilt, and compare-and-swapped before the next one starts, so updates are not atomic *across* refs: earlier refs can already be rewritten when a later one conflicts. Delivery pushes the exact hash whose trailer it checked and removes only that generation's queue entry, so a concurrent advance of the same ref can neither substitute unchecked content nor be erased from the queue. #### Persistence of un-redacted-by-OPF content @@ -330,7 +326,7 @@ Two notes on the `git-refs` rows: Two `git-branch`-shaped leftovers are easy to miss once a repo has moved on: - **A `git-branch` mirror never gets OPF'd.** Mirrors receive best-effort write fan-out only and never ref-level mutations, so a mirror branch keeps 8-layer content indefinitely. It isn't pushed at pre-push, so it doesn't reach the remote — but it is local content, and it has a `refs/heads/` reflog. -- **Migration doesn't delete the old branch.** `entire doctor migrate-checkpoints` imports from `entire/checkpoints/v1` and leaves it in place, 8-layer, along with its reflog. Migrated refs also carry no OPF trailer, so the first push after a migration re-OPFs every migrated ref — one commit per ref keeps the commit cap happy, and the 2 MiB batch cap is counted per ref, so a large migrated session is withheld on its own rather than blocking the others. +- **Migration doesn't delete the old branch.** `entire doctor migrate-checkpoints` imports from `entire/checkpoints/v1` and leaves it in place, 8-layer, along with its reflog. Migrated refs also carry no OPF trailer, so the first push after a migration re-OPFs every migrated ref — one commit per ref keeps the commit cap happy, and the caps are counted per ref, so the realistic cost is the background scan time for every migrated session rather than a rejection. `.entire/metadata//full.jsonl` is Entire's own local working copy of the transcript, written mode `0600`. It is *sanitized* (agent state that cannot be replayed out of a checkpoint is stripped) but **not redacted** — redaction happens on the way into a git object, not on this file. It is the input condensation reads from when the live agent transcript is no longer available. diff --git a/redact/batch.go b/redact/batch.go index 0455370bd1..a4a47a407a 100644 --- a/redact/batch.go +++ b/redact/batch.go @@ -14,13 +14,19 @@ import ( // // Content is the raw blob bytes. The blob's redacted output appears at // the same index in the function's return slice. +// +// ID is the blob's git object hash. It is what the OPF span cache is keyed +// by (see ScanBlobsWithPrivacyFilter); a blob without an ID can be redacted +// in one pass by BatchBytesWithPrivacyFilter but never cached. type NamedBlob struct { Name string Content []byte + ID string } -// BatchBytesWithPrivacyFilter redacts N blobs with a single OPF -// inference call instead of N. Returns redacted bytes in input order +// BatchBytesWithPrivacyFilter redacts N blobs with one OPF inference call +// per opfBatchChunkBytes of unique leaf text instead of one per blob, so +// typical input is a single call. Returns redacted bytes in input order // (output[i] is the redaction of inputs[i]). // // Failure semantics — fail-closed: any error from the OPF runtime @@ -63,62 +69,121 @@ func BatchBytesWithPrivacyFilter(ctx context.Context, inputs []NamedBlob) ([][]b return applyRegexLayersToBlobs(inputs), nil } - // Pass 1: collect unique prose-shaped leaves across every blob. - // The has-space gate excludes structural strings (paths, IDs, - // snake_case keys) that would pay model-load cost for no benefit. - // Dedup keys by leaf text, mirroring JSONLContentWithPrivacyFilter: - // OPF is a pure function of input text, so identical leaves in - // different blobs share a single inference result. - seen := make(map[string]struct{}) - var batchInputs []string - addLeaf := func(v string) { - if !strings.ContainsRune(v, ' ') { - return - } - if _, ok := seen[v]; ok { - return - } - seen[v] = struct{}{} - batchInputs = append(batchInputs, v) + spansByInput, err := scanProseLeaves(ctx, cfg, cats, uniqueProseLeaves(inputs), len(inputs)) + if err != nil { + return nil, err } + + // Pass 3: apply per-leaf regex layers + cached OPF spans per blob. + out := make([][]byte, len(inputs)) + for i, in := range inputs { + out[i] = applyToBlob(in, spansByInput, cfg) + } + return out, nil +} + +// uniqueProseLeaves collects the prose-shaped leaves of inputs, in first-seen +// order and deduplicated by text. The has-space gate excludes structural strings +// (paths, IDs, snake_case keys) that would pay model cost for no benefit. OPF is +// a pure function of its input text, so identical leaves in different blobs +// share one inference result. +func uniqueProseLeaves(inputs []NamedBlob) []string { + seen := make(map[string]struct{}) + var leaves []string for _, in := range inputs { - collectLeaves(in, addLeaf) + collectLeaves(in, func(v string) { + if !isProseLeaf(v) { + return + } + if _, ok := seen[v]; ok { + return + } + seen[v] = struct{}{} + leaves = append(leaves, v) + }) } + return leaves +} + +// isProseLeaf is the has-space gate shared by collection, scanning and the +// cached apply path, so all three agree on which leaves OPF covers. +func isProseLeaf(v string) bool { + return strings.ContainsRune(v, ' ') +} - // Pass 2: single batched OPF call covering every unique leaf. - spansByInput := make(map[string][]Span, len(batchInputs)) - if len(batchInputs) > 0 { - fmt.Fprintln(opfStderr, "→ OpenAI Privacy Filter: scanning checkpoints…") - start := time.Now() - batched, err := cfg.runtime.RedactBatch(ctx, batchInputs, cats) +// scanProseLeaves runs the model over leaves in calls of at most +// opfBatchChunkBytes each (see chunkOPFBatchInputs), so each call's adaptive +// deadline tracks its own size. blobCount only feeds messages. +// +// Fail-closed: a runtime error or a short return trips the breaker and returns +// an error, because the caller is about to treat the result as a complete scan. +func scanProseLeaves(ctx context.Context, cfg *OPFConfig, cats []string, leaves []string, blobCount int) (map[string][]Span, error) { + spansByInput := make(map[string][]Span, len(leaves)) + if len(leaves) == 0 { + return spansByInput, nil + } + fmt.Fprintln(opfStderr, "→ OpenAI Privacy Filter: scanning checkpoints…") + start := time.Now() + for _, chunk := range chunkOPFBatchInputs(leaves, opfBatchChunkBytes) { + batched, err := cfg.runtime.RedactBatch(ctx, chunk, cats) if err != nil { handleOPFFailure(ctx, cfg, err) - return nil, fmt.Errorf("opf batch failed across %d blobs: %w", len(inputs), err) + return nil, fmt.Errorf("opf batch failed across %d blobs: %w", blobCount, err) } - fmt.Fprintf(opfStderr, "✓ OpenAI Privacy Filter: done (%.1fs, %d blobs)\n", - time.Since(start).Seconds(), len(inputs)) // A short return means the runtime gave us fewer span slices than // inputs — the tail leaves would receive zero OPF spans and the // rewrite would proceed as if OPF found nothing in them. With the // Entire-OPF-Applied trailer attached to the resulting commits, // that's silent under-redaction. Fail-closed: trip the breaker // and return an error so the orchestrator aborts before CAS. - if len(batched) != len(batchInputs) { - shortErr := fmt.Errorf("opf runtime returned %d span slices for %d inputs", len(batched), len(batchInputs)) + if len(batched) != len(chunk) { + shortErr := fmt.Errorf("opf runtime returned %d span slices for %d inputs", len(batched), len(chunk)) handleOPFFailure(ctx, cfg, shortErr) return nil, fmt.Errorf("opf batch short return: %w", shortErr) } - for i, leaf := range batchInputs { + for i, leaf := range chunk { spansByInput[leaf] = batched[i] } } + fmt.Fprintf(opfStderr, "✓ OpenAI Privacy Filter: done (%.1fs, %d blobs)\n", + time.Since(start).Seconds(), blobCount) + return spansByInput, nil +} - // Pass 3: apply per-leaf regex layers + cached OPF spans per blob. - out := make([][]byte, len(inputs)) - for i, in := range inputs { - out[i] = applyToBlob(in, spansByInput, cfg) +// opfBatchChunkBytes bounds the text one opf invocation carries. Without it +// every unique leaf of a unit of work goes to a single call, so one large +// checkpoint ref could need more time than opfTimeoutCeiling allows: the call +// is killed, the breaker trips, and the ref comes up first again on every +// later flush, starving every ref queued behind it. At the benchmarked rate a +// 1 MiB chunk finishes in about 20 minutes, well inside its own ~40-minute +// adaptive deadline, and the ~6s model load it adds is under 1% of that. +const opfBatchChunkBytes = 1024 * 1024 + +// OPFBatchChunkBytes is opfBatchChunkBytes for callers that pace work by model +// call, such as the background scan worker. +const OPFBatchChunkBytes = opfBatchChunkBytes + +// chunkOPFBatchInputs splits inputs, in order, into consecutive groups whose +// joined length (each input plus its separator) stays within limit. An input +// larger than limit on its own gets a group of its own rather than being +// split, since a span cannot be attributed across a cut in the text. +func chunkOPFBatchInputs(inputs []string, limit int) [][]string { + var chunks [][]string + var current []string + size := 0 + for _, in := range inputs { + n := len(in) + len(opfBatchSeparator) + if len(current) > 0 && size+n > limit { + chunks = append(chunks, current) + current, size = nil, 0 + } + current = append(current, in) + size += n } - return out, nil + if len(current) > 0 { + chunks = append(chunks, current) + } + return chunks } // collectLeaves invokes add for every prose-shaped leaf in the blob. @@ -198,20 +263,9 @@ func isJSONLikeName(name string) bool { // once here matches the single inference result the batch call reuses // for every repeat. func SumProseLeafBytes(inputs []NamedBlob) int { - seen := make(map[string]struct{}) - var total int - addLeaf := func(v string) { - if !strings.ContainsRune(v, ' ') { - return - } - if _, ok := seen[v]; ok { - return - } - seen[v] = struct{}{} - total += len(v) - } - for _, in := range inputs { - collectLeaves(in, addLeaf) + total := 0 + for _, leaf := range uniqueProseLeaves(inputs) { + total += len(leaf) } return total } diff --git a/redact/batch_test.go b/redact/batch_test.go index 8ad1cb1e5a..bc12e69d71 100644 --- a/redact/batch_test.go +++ b/redact/batch_test.go @@ -1,6 +1,7 @@ package redact import ( + "bytes" "context" "errors" "strings" @@ -576,3 +577,90 @@ func TestBatchBytesWithPrivacyFilter_NilRuntimeFailsClosed(t *testing.T) { t.Errorf("want nil output on fail-closed error, got %d blobs", len(out)) } } + +func TestChunkOPFBatchInputs(t *testing.T) { + t.Parallel() + sep := len(opfBatchSeparator) + tests := []struct { + name string + inputs []string + limit int + want [][]string + }{ + {name: "empty", inputs: nil, limit: 10, want: nil}, + {name: "all fit", inputs: []string{"ab", "cd"}, limit: 2 * (2 + sep), want: [][]string{{"ab", "cd"}}}, + {name: "splits at limit", inputs: []string{"ab", "cd", "ef"}, limit: 2 * (2 + sep), want: [][]string{{"ab", "cd"}, {"ef"}}}, + {name: "oversized input alone", inputs: []string{"a", "0123456789", "b"}, limit: 4, want: [][]string{{"a"}, {"0123456789"}, {"b"}}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := chunkOPFBatchInputs(tt.inputs, tt.limit) + if len(got) != len(tt.want) { + t.Fatalf("got %d chunks %q, want %d %q", len(got), got, len(tt.want), tt.want) + } + for i := range got { + if strings.Join(got[i], "|") != strings.Join(tt.want[i], "|") { + t.Errorf("chunk %d = %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + +// Leaf text past opfBatchChunkBytes is split across several calls, each within +// the bound, and every leaf still receives its spans. One oversized call would +// run into opfTimeoutCeiling on a large checkpoint ref. +func TestBatchBytesWithPrivacyFilter_SplitsLargeBatchesIntoBoundedCalls(t *testing.T) { + rt := &recordingSizesRuntime{spans: []Span{{Start: 0, End: 5, Label: "private_person"}}} + resetOPFConfig() + t.Cleanup(resetOPFConfig) + ConfigurePrivacyFilterWithRuntime(OPFConfig{ + Enabled: true, + Categories: map[string]bool{"private_person": true}, + }, rt) + + leaf := strings.Repeat("x", opfBatchChunkBytes/2) + inputs := []NamedBlob{ + {Name: "a.txt", Content: []byte("Alice " + leaf)}, + {Name: "b.txt", Content: []byte("Bobby " + leaf)}, + {Name: "c.txt", Content: []byte("Carol " + leaf)}, + } + out, err := BatchBytesWithPrivacyFilter(context.Background(), inputs) + if err != nil { + t.Fatalf("BatchBytesWithPrivacyFilter: %v", err) + } + if len(rt.callBytes) < 2 { + t.Fatalf("want the batch split across several calls, got %d", len(rt.callBytes)) + } + for i, n := range rt.callBytes { + if n > opfBatchChunkBytes { + t.Errorf("call %d carried %d bytes, want at most %d", i, n, opfBatchChunkBytes) + } + } + for i, o := range out { + if bytes.HasPrefix(o, inputs[i].Content[:5]) { + t.Errorf("blob %d: leading name not redacted, spans were lost across chunks", i) + } + } +} + +type recordingSizesRuntime struct { + spans []Span + callBytes []int +} + +func (r *recordingSizesRuntime) Redact(_ context.Context, _ string, _ []string) ([]Span, error) { + return r.spans, nil +} + +func (r *recordingSizesRuntime) RedactBatch(_ context.Context, inputs []string, _ []string) ([][]Span, error) { + n := 0 + out := make([][]Span, len(inputs)) + for i, in := range inputs { + n += len(in) + len(opfBatchSeparator) + out[i] = r.spans + } + r.callBytes = append(r.callBytes, n) + return out, nil +} diff --git a/redact/opf.go b/redact/opf.go index 6b55c4a6cd..709d37bd23 100644 --- a/redact/opf.go +++ b/redact/opf.go @@ -19,13 +19,22 @@ import ( ) // OPFConfig configures the optional OpenAI Privacy Filter detection layer. -// Defaults are applied by ConfigurePrivacyFilter; callers should pass values -// straight from settings without local normalization. +// Callers pass values straight from settings without local normalization: +// ConfigurePrivacyFilter fills in the default Command, and Timeout's default +// is resolved per call, at the point the batch size is known. type OPFConfig struct { Enabled bool Categories map[string]bool Command string // path or name of the opf binary; "" defaults to "opf" - Timeout int // seconds; 0 defaults to 30 + + // Timeout is the per-invocation deadline in seconds. It is deliberately + // NOT normalized to a default at configuration time: 0 has to survive + // all the way to the shell-out, where it means "no explicit override" + // and selects the size-adaptive deadline (adaptiveOPFTimeout). Folding + // it into a concrete number here would make an unset setting + // indistinguishable from someone typing that number, and the adaptive + // path would never run. + Timeout int // Logger receives OPF runtime-failure diagnostics. nil means // slog.Default(); the CLI injects its entry-point-initialized logger @@ -45,8 +54,8 @@ var ( // The first detectOPF failure trips it via handleOPFFailure; subsequent // detectOPF calls short-circuit before shelling out. Without this, a // broken OPF install (binary missing, persistently timing out, etc.) - // makes every redaction call pay the full timeout — turning one - // condensation into N × 30s waits instead of a single warning plus + // makes every redaction call pay its full timeout — turning one + // condensation into N such waits instead of a single warning plus // graceful fallback. Process-scoped so a fresh CLI invocation re-attempts. opfBreakerTripped atomic.Bool ) @@ -58,9 +67,6 @@ var ( // one). func ConfigurePrivacyFilter(cfg OPFConfig) { cfgCopy := cfg - if cfgCopy.Timeout <= 0 { - cfgCopy.Timeout = 30 - } if cfgCopy.Command == "" { cfgCopy.Command = defaultOPFCommand } @@ -75,9 +81,6 @@ func ConfigurePrivacyFilter(cfg OPFConfig) { // explicit runtime instead of constructing one. func ConfigurePrivacyFilterWithRuntime(cfg OPFConfig, rt opfRuntime) { cfgCopy := cfg - if cfgCopy.Timeout <= 0 { - cfgCopy.Timeout = 30 - } cfgCopy.runtime = rt opfConfigMu.Lock() opfConfig = &cfgCopy @@ -332,7 +335,11 @@ type opfRuntime interface { // invocation is intentional for v1 — daemon mode is a planned follow-up // that implements the same opfRuntime interface so callers don't change. type shellOut struct { - command string + command string + + // timeoutSeconds is the configured per-invocation deadline. 0 means the + // user set no timeout_seconds, which selects adaptiveOPFTimeout's + // size-scaled deadline instead of a fixed one. timeoutSeconds int // commandRunner builds the *exec.Cmd run for each Redact call. Tests @@ -361,12 +368,97 @@ const ( // span boundaries) opfBatchSeparator = "\x1e" - // Keep a pathological transcript or process from making the CLI allocate - // unbounded buffers while preparing or reading an OPF shell-out. - opfMaxBatchInputBytes = 16 * 1024 * 1024 + // opfMaxBatchInputBytes bounds the joined input this process buffers + // before handing it to the opf shell-out. It exists for exactly one + // reason: a buffer this size is safe to allocate on any machine + // capable of running this CLI, so no pathological or corrupted input + // can make the CLI allocate its way into an out-of-memory crash. It + // bounds process memory, not content legitimacy. + // + // It is therefore NOT tuned to how big a session is, and it is not + // user-configurable — not even via ENTIRE_OPF_BATCH_LIMIT=unlimited, + // which waives the friendlier strategy-layer cap (batchDefaultLimit in + // cmd/entire/cli/strategy) and never this one. "Unlimited" means "stop + // applying the sanity check", not "remove memory protection". Judging + // whether input is plausible belongs to that cap, which owns the + // explanatory error; this is the last wall and its error is blunt by + // design. Nothing real should ever reach it: the cap above sits far + // below, so hitting this means the layer above was explicitly waived. + opfMaxBatchInputBytes = 256 * 1024 * 1024 + + // opfMaxProcessOutputBytes bounds what we buffer back from the opf + // process's stdout/stderr, for the same allocate-no-further reason. opfMaxProcessOutputBytes = 1 * 1024 * 1024 ) +// The shell-out's deadline scales with how much text the call actually +// carries, because a fixed one cannot do this job. Benchmarked against the +// real binary (docs/development/opf-throughput-findings.md), OPF sustains +// roughly 1.14 s/KB of input on CPU — the only viable device on this stack, +// since MPS measured slower rather than faster — so the fixed 30s this +// replaces was already short of what 50 KB of prose needs. Reading +// slow-but-working redaction as a broken runtime is the worst failure +// available here: it trips the process-wide circuit breaker +// (handleOPFFailure), which abandons every other ref in the same pass, not +// just the call that ran long. +const ( + // opfTimeoutFloor is the minimum deadline, held at the previous fixed + // default. Below a few KB the per-call fixed cost (the ~6s model load) + // dominates throughput, so scaling further down would measure nothing + // real. + opfTimeoutFloor = 30 * time.Second + + // opfTimeoutRatePerKB is the measured sustained rate. It is the 1.14 s/KB + // figure the benchmark held at from 250 KB through 500 KB, not the + // friendlier 0.85 s/KB small inputs show before throughput degrades. + opfTimeoutRatePerKB = 1140 * time.Millisecond + + // opfTimeoutMargin covers hardware variance against the one machine the + // rate was measured on. It is slack, not tightness: this deadline should + // essentially never fire on content that is merely large. + opfTimeoutMargin = 2 + + // opfTimeoutCeiling clamps the scaled estimate. Its only job is to answer + // "at what point is 'the runtime is stuck' a better explanation than + // 'this is a big input still working'?" It is NOT derived from + // batchDefaultLimit in cmd/entire/cli/strategy: that cap is a byte-sized + // backstop with no time budget in it at all. + // + // BatchBytesWithPrivacyFilter sends at most opfBatchChunkBytes per call, + // whose scaled deadline (~40 minutes) sits far below this clamp, so in + // practice only a single leaf larger than a chunk can reach it. The clamp + // keeps one stuck call from holding the scan worker, and every checkpoint + // queued behind it, indefinitely. + opfTimeoutCeiling = 3 * time.Hour + + // opfTimeoutBytesPerKB converts batch bytes into the KB unit the rate is + // expressed in. + opfTimeoutBytesPerKB = 1024 +) + +// adaptiveOPFTimeout resolves the deadline for a single opf invocation from +// the size of the batch it carries. +// +// configuredSeconds is the raw timeout_seconds from settings, where 0 means +// "unset" — nothing normalizes it earlier, precisely so this distinction +// survives (see OPFConfig.Timeout). An explicit value is a deliberate +// override and is honored exactly, in both directions: the formula neither +// raises it for a large batch nor lowers it for a small one. +func adaptiveOPFTimeout(configuredSeconds, batchedLen int) time.Duration { + if configuredSeconds > 0 { + return time.Duration(configuredSeconds) * time.Second + } + estimated := time.Duration(batchedLen/opfTimeoutBytesPerKB) * opfTimeoutRatePerKB * opfTimeoutMargin + switch { + case estimated < opfTimeoutFloor: + return opfTimeoutFloor + case estimated > opfTimeoutCeiling: + return opfTimeoutCeiling + default: + return estimated + } +} + // Redact runs OPF on a single text input. func (s *shellOut) Redact(ctx context.Context, text string, categories []string) ([]Span, error) { if len(categories) == 0 { @@ -401,7 +493,7 @@ func (s *shellOut) RedactBatch(ctx context.Context, inputs []string, categories if tooLarge { return nil, fmt.Errorf("opf input too large (%d bytes, limit %d)", batchedLen, opfMaxBatchInputBytes) } - timeout := time.Duration(s.timeoutSeconds) * time.Second + timeout := adaptiveOPFTimeout(s.timeoutSeconds, batchedLen) callCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() diff --git a/redact/opf_cache.go b/redact/opf_cache.go new file mode 100644 index 0000000000..6c6e7a8bd4 --- /dev/null +++ b/redact/opf_cache.go @@ -0,0 +1,210 @@ +package redact + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "maps" + "strings" +) + +// OPFSpanCache persists OPF results per blob, so the model call can run in a +// background scan and a later rewrite can apply the result without it. +// +// An entry maps the hash of each prose leaf in one blob to the spans OPF found +// in it. It holds no text: a leaf is identified only by its SHA-256, and a span +// only by offsets and a label. Implementations must be safe across processes: +// the background scan worker stores and prunes entries while a pre-push reads +// them, so a store must replace an entry atomically, and a missing, partial, +// or unreadable entry reports ok=false rather than an error. +type OPFSpanCache interface { + LoadOPFSpans(key string) (spans map[string][]Span, ok bool) + StoreOPFSpans(key string, spans map[string][]Span) error +} + +// ErrOPFScanPending reports that at least one blob has not been scanned yet, +// so it cannot be redacted without a model call. +var ErrOPFScanPending = errors.New("OpenAI Privacy Filter has not finished scanning this content") + +// ErrOPFUnavailable reports that the OPF runtime failed earlier in this +// process, so no further scan will be attempted. +var ErrOPFUnavailable = errors.New("OpenAI Privacy Filter is unavailable for the rest of this process") + +// opfCacheVersion is part of every cache key. Bump it when the entry format or +// the meaning of a stored span changes, so older entries are never read. +// +// The key includes the configured OPF command, so pointing Entire at a +// different runtime rescans. It cannot see a model upgraded in place behind the +// same command: bump this whenever the opf model or its output changes in a way +// that should invalidate stored results, or old entries keep being applied for +// up to the cache's max age. +const opfCacheVersion = "1" + +// opfBlobCacheKey names one blob's entry. OPF's output depends only on the leaf +// text, the runtime and the categories it was asked for, so the key is the +// blob's object hash plus the OPF command and the sorted category set; enabling +// a category or changing the command rescans everything. +func opfBlobCacheKey(blobID string, cats []string) string { + sum := sha256.Sum256([]byte(opfCacheVersion + "\x00" + OPFCommand() + "\x00" + strings.Join(cats, ",") + "\x00" + blobID)) + return hex.EncodeToString(sum[:]) +} + +func opfLeafKey(leaf string) string { + sum := sha256.Sum256([]byte(leaf)) + return hex.EncodeToString(sum[:]) +} + +// ScanBlobsWithPrivacyFilter runs OPF over every blob of inputs that has no +// cache entry yet and stores one entry per such blob. Blobs already cached cost +// one cache read and no model time, so rescanning a backlog is cheap. +// +// Fail-closed like BatchBytesWithPrivacyFilter: zero enabled categories returns +// ErrOPFNoEnabledCategories and a runtime failure returns an error with nothing +// stored for the blobs of the failed pass. With OPF disabled there is nothing to +// scan and it returns nil. +func ScanBlobsWithPrivacyFilter(ctx context.Context, inputs []NamedBlob, cache OPFSpanCache) error { + cfg := getOPFConfig() + if cfg == nil || !cfg.Enabled { + return nil + } + cats := enabledCategories(cfg) + if len(cats) == 0 { + return ErrOPFNoEnabledCategories + } + if cfg.runtime == nil { + return errOPFNilRuntime + } + if opfBreakerTripped.Load() { + return ErrOPFUnavailable + } + + var pending []NamedBlob + seen := make(map[string]struct{}) + for _, in := range inputs { + if in.ID == "" { + continue + } + if _, dup := seen[in.ID]; dup { + continue + } + seen[in.ID] = struct{}{} + if _, ok := cache.LoadOPFSpans(opfBlobCacheKey(in.ID, cats)); !ok { + pending = append(pending, in) + } + } + if len(pending) == 0 { + return nil + } + + // Scan and store one model call's worth of blobs at a time, so a failure, + // a killed process or a sleeping laptop loses at most the group in flight + // rather than every result of a long scan. spansByLeaf spans the groups, so + // a leaf repeated across blobs is still scanned once. + spansByLeaf := make(map[string][]Span) + for _, group := range groupBlobsByLeafBytes(pending, OPFBatchChunkBytes) { + var leaves []string + for _, leaf := range uniqueProseLeaves(group) { + if _, done := spansByLeaf[leaf]; !done { + leaves = append(leaves, leaf) + } + } + scanned, err := scanProseLeaves(ctx, cfg, cats, leaves, len(group)) + if err != nil { + return err + } + maps.Copy(spansByLeaf, scanned) + for _, in := range group { + entry := make(map[string][]Span) + collectLeaves(in, func(v string) { + if isProseLeaf(v) { + entry[opfLeafKey(v)] = spansByLeaf[v] + } + }) + if err := cache.StoreOPFSpans(opfBlobCacheKey(in.ID, cats), entry); err != nil { + return fmt.Errorf("store OPF span cache entry: %w", err) + } + } + } + return nil +} + +// groupBlobsByLeafBytes splits blobs, in order, into groups whose prose leaves +// fit one model call by the measure chunkOPFBatchInputs uses. A blob larger +// than limit gets a group of its own; scanProseLeaves still chunks inside it. +func groupBlobsByLeafBytes(blobs []NamedBlob, limit int) [][]NamedBlob { + var groups [][]NamedBlob + var current []NamedBlob + size := 0 + for _, b := range blobs { + n := 0 + for _, leaf := range uniqueProseLeaves([]NamedBlob{b}) { + n += len(leaf) + len(opfBatchSeparator) + } + if len(current) > 0 && size+n > limit { + groups = append(groups, current) + current, size = nil, 0 + } + current = append(current, b) + size += n + } + if len(current) > 0 { + groups = append(groups, current) + } + return groups +} + +// ApplyCachedPrivacyFilter redacts inputs using cached OPF results only; it +// never calls the model. Every blob must have an ID and a complete cache entry, +// otherwise it returns ErrOPFScanPending and no output, because the caller is +// about to stamp Entire-OPF-Applied and partial coverage would make that false. +// +// With OPF disabled it returns regex-only output, and with zero enabled +// categories ErrOPFNoEnabledCategories, matching BatchBytesWithPrivacyFilter. A +// tripped breaker does not matter here: the results were produced earlier. +func ApplyCachedPrivacyFilter(inputs []NamedBlob, cache OPFSpanCache) ([][]byte, error) { + if len(inputs) == 0 { + return nil, nil + } + cfg := getOPFConfig() + if cfg == nil || !cfg.Enabled { + return applyRegexLayersToBlobs(inputs), nil + } + cats := enabledCategories(cfg) + if len(cats) == 0 { + return nil, ErrOPFNoEnabledCategories + } + + spansByLeaf := make(map[string][]Span) + for _, in := range inputs { + if in.ID == "" { + return nil, ErrOPFScanPending + } + entry, ok := cache.LoadOPFSpans(opfBlobCacheKey(in.ID, cats)) + if !ok { + return nil, ErrOPFScanPending + } + complete := true + collectLeaves(in, func(v string) { + if !isProseLeaf(v) { + return + } + spans, found := entry[opfLeafKey(v)] + if !found { + complete = false + return + } + spansByLeaf[v] = spans + }) + if !complete { + return nil, ErrOPFScanPending + } + } + + out := make([][]byte, len(inputs)) + for i, in := range inputs { + out[i] = applyToBlob(in, spansByLeaf, cfg) + } + return out, nil +} diff --git a/redact/opf_cache_test.go b/redact/opf_cache_test.go new file mode 100644 index 0000000000..f9eaead1ff --- /dev/null +++ b/redact/opf_cache_test.go @@ -0,0 +1,265 @@ +package redact + +import ( + "bytes" + "context" + "errors" + "slices" + "strings" + "testing" +) + +// memSpanCache is an in-memory OPFSpanCache for tests. +type memSpanCache struct { + entries map[string]map[string][]Span + stores int +} + +func newMemSpanCache() *memSpanCache { + return &memSpanCache{entries: make(map[string]map[string][]Span)} +} + +func (c *memSpanCache) LoadOPFSpans(key string) (map[string][]Span, bool) { + e, ok := c.entries[key] + return e, ok +} + +func (c *memSpanCache) StoreOPFSpans(key string, spans map[string][]Span) error { + c.stores++ + c.entries[key] = spans + return nil +} + +func cacheTestBlobs() []NamedBlob { + return []NamedBlob{ + {Name: "0/full.jsonl", ID: "blob-a", Content: []byte(`{"content":"Alice met Bob"}` + "\n" + `{"content":"Charlie sat down"}`)}, + {Name: "0/metadata.json", ID: "blob-b", Content: []byte(`{"summary":"Alice walked home","id":"x_1"}`)}, + {Name: "0/note.txt", ID: "blob-c", Content: []byte("Frank reviewed the diff")}, + } +} + +var cacheTestCats = map[string]bool{"private_person": true} + +// Scanning into the cache and applying from it must produce exactly what the +// one-pass batch produces: the split moves the model call, not the result. +func TestScanThenApplyMatchesOnePassBatch(t *testing.T) { + fake := &fakeRuntime{spans: []Span{{Start: 0, End: 5, Label: "private_person"}}} + configureFakeOPF(t, fake, cacheTestCats) + want, err := BatchBytesWithPrivacyFilter(context.Background(), cacheTestBlobs()) + if err != nil { + t.Fatalf("BatchBytesWithPrivacyFilter: %v", err) + } + + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); err != nil { + t.Fatalf("ScanBlobsWithPrivacyFilter: %v", err) + } + got, err := ApplyCachedPrivacyFilter(cacheTestBlobs(), cache) + if err != nil { + t.Fatalf("ApplyCachedPrivacyFilter: %v", err) + } + for i := range want { + if !bytes.Equal(got[i], want[i]) { + t.Errorf("blob %d: cached apply = %q, one-pass = %q", i, got[i], want[i]) + } + } + for key, entry := range cache.entries { + for leafKey := range entry { + if strings.Contains(key+leafKey, "Alice") { + t.Fatalf("cache keys must not contain leaf text") + } + } + } +} + +func TestApplyCachedPrivacyFilter_PendingWithoutScan(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{}, cacheTestCats) + if _, err := ApplyCachedPrivacyFilter(cacheTestBlobs(), newMemSpanCache()); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("err = %v, want ErrOPFScanPending", err) + } +} + +func TestApplyCachedPrivacyFilter_PendingWithoutBlobID(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{}, cacheTestCats) + cache := newMemSpanCache() + blobs := cacheTestBlobs() + if err := ScanBlobsWithPrivacyFilter(context.Background(), blobs, cache); err != nil { + t.Fatal(err) + } + blobs[1].ID = "" + if _, err := ApplyCachedPrivacyFilter(blobs, cache); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("err = %v, want ErrOPFScanPending for a blob that cannot be keyed", err) + } +} + +// A second scan of already-cached blobs must not call the model. +func TestScanBlobsWithPrivacyFilter_SkipsCachedBlobs(t *testing.T) { + fake := &fakeRuntime{} + configureFakeOPF(t, fake, cacheTestCats) + cache := newMemSpanCache() + for range 2 { + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); err != nil { + t.Fatal(err) + } + } + if fake.batchCalls != 1 { + t.Fatalf("batchCalls = %d, want 1", fake.batchCalls) + } + if cache.stores != 3 { + t.Fatalf("stores = %d, want one per blob", cache.stores) + } +} + +// Results are only valid for the categories they were computed with. +func TestApplyCachedPrivacyFilter_CategoryChangeRequiresRescan(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{}, cacheTestCats) + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); err != nil { + t.Fatal(err) + } + configureFakeOPF(t, &fakeRuntime{}, map[string]bool{"private_person": true, "private_email": true}) + if _, err := ApplyCachedPrivacyFilter(cacheTestBlobs(), cache); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("err = %v, want ErrOPFScanPending after enabling a category", err) + } +} + +// An entry that does not cover every prose leaf of its blob is not a complete +// scan and must not be applied. +func TestApplyCachedPrivacyFilter_IncompleteEntryIsPending(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{}, cacheTestCats) + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); err != nil { + t.Fatal(err) + } + for _, entry := range cache.entries { + for leafKey := range entry { + delete(entry, leafKey) + break + } + } + if _, err := ApplyCachedPrivacyFilter(cacheTestBlobs(), cache); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("err = %v, want ErrOPFScanPending for a partial entry", err) + } +} + +func TestOPFSpanCache_NoCategoriesFailsClosed(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{}, map[string]bool{}) + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); !errors.Is(err, ErrOPFNoEnabledCategories) { + t.Fatalf("scan err = %v, want ErrOPFNoEnabledCategories", err) + } + if _, err := ApplyCachedPrivacyFilter(cacheTestBlobs(), cache); !errors.Is(err, ErrOPFNoEnabledCategories) { + t.Fatalf("apply err = %v, want ErrOPFNoEnabledCategories", err) + } +} + +// A failed model call stores nothing, so a later apply cannot mistake the +// failure for a clean scan. +func TestScanBlobsWithPrivacyFilter_RuntimeFailureStoresNothing(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{err: errors.New("opf crashed")}, cacheTestCats) + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); err == nil { + t.Fatal("want an error from a failing runtime") + } + if cache.stores != 0 { + t.Fatalf("stores = %d, want 0 after a failed scan", cache.stores) + } + if err := ScanBlobsWithPrivacyFilter(context.Background(), cacheTestBlobs(), cache); !errors.Is(err, ErrOPFUnavailable) { + t.Fatalf("second scan err = %v, want ErrOPFUnavailable once the breaker tripped", err) + } +} + +// A long scan is stored one model call's worth of blobs at a time, so a failure +// in a later call keeps the results of the calls that finished. +func TestScanBlobsWithPrivacyFilter_FailureKeepsEarlierGroups(t *testing.T) { + configureFakeOPF(t, &fakeRuntime{failFromBatch: 2}, cacheTestCats) + prose := strings.Repeat("the team reviewed the plan ", OPFBatchChunkBytes/27) + blobs := []NamedBlob{ + {Name: "0/a.txt", ID: "blob-a", Content: []byte("first " + prose)}, + {Name: "0/b.txt", ID: "blob-b", Content: []byte("second " + prose)}, + } + cache := newMemSpanCache() + + if err := ScanBlobsWithPrivacyFilter(context.Background(), blobs, cache); err == nil { + t.Fatal("want the second call's failure") + } + if cache.stores != 1 { + t.Fatalf("stores = %d, want the first group kept", cache.stores) + } + if _, err := ApplyCachedPrivacyFilter(blobs[:1], cache); err != nil { + t.Fatalf("first blob must be fully cached: %v", err) + } + if _, err := ApplyCachedPrivacyFilter(blobs[1:], cache); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("second blob err = %v, want pending", err) + } +} + +// FuzzApplyCachedPrivacyFilter pins the two properties the trailer depends on, +// for arbitrary blob contents: applying cached results gives exactly what the +// one-pass batch gives, and an entry missing any single prose leaf makes the +// apply report pending instead of redacting that blob partially. +func FuzzApplyCachedPrivacyFilter(f *testing.F) { + f.Add(`{"content":"Alice met Bob"}`+"\n"+`{"content":"Charlie sat down"}`, "Frank reviewed the diff", 0) + f.Add(`{"a":["x y","x y"],"b":{"c":"no-space"}}`, "", 1) + f.Add("not json at all", "two words", 5) + + f.Fuzz(func(t *testing.T, jsonl, text string, drop int) { + fake := &fakeRuntime{spans: []Span{{Start: 0, End: 1, Label: "private_person"}}} + configureFakeOPF(t, fake, cacheTestCats) + blobs := []NamedBlob{ + {Name: "0/full.jsonl", ID: "blob-a", Content: []byte(jsonl)}, + {Name: "0/note.txt", ID: "blob-b", Content: []byte(text)}, + } + want, err := BatchBytesWithPrivacyFilter(context.Background(), blobs) + if err != nil { + t.Skip("inputs the one-pass batch rejects are out of scope") + } + cache := newMemSpanCache() + if err := ScanBlobsWithPrivacyFilter(context.Background(), blobs, cache); err != nil { + t.Fatalf("scan: %v", err) + } + got, err := ApplyCachedPrivacyFilter(blobs, cache) + if err != nil { + t.Fatalf("apply: %v", err) + } + for i := range want { + if !bytes.Equal(got[i], want[i]) { + t.Fatalf("blob %d: cached %q != one-pass %q", i, got[i], want[i]) + } + } + + // Drop one leaf from one entry; if the blob had any prose leaf, the + // apply must now refuse. + var keys []string + for key, entry := range cache.entries { + for leafKey := range entry { + keys = append(keys, key+"\x00"+leafKey) + } + } + if len(keys) == 0 { + return + } + slices.Sort(keys) + if drop < 0 { + drop = -drop + } + victim := strings.SplitN(keys[drop%len(keys)], "\x00", 2) + delete(cache.entries[victim[0]], victim[1]) + if _, err := ApplyCachedPrivacyFilter(blobs, cache); !errors.Is(err, ErrOPFScanPending) { + t.Fatalf("apply with a missing leaf: err = %v, want ErrOPFScanPending", err) + } + }) +} + +// Pointing Entire at a different OPF runtime must not reuse the old one's spans. +func TestOPFBlobCacheKey_FollowsTheOPFCommand(t *testing.T) { + resetOPFConfig() + t.Cleanup(resetOPFConfig) + cats := []string{"private_person"} + ConfigurePrivacyFilterWithRuntime(OPFConfig{Enabled: true, Command: "/opt/opf-a/bin/opf"}, &fakeRuntime{}) + first := opfBlobCacheKey("blob-a", cats) + ConfigurePrivacyFilterWithRuntime(OPFConfig{Enabled: true, Command: "/opt/opf-b/bin/opf"}, &fakeRuntime{}) + if second := opfBlobCacheKey("blob-a", cats); second == first { + t.Fatal("a different OPF command must produce a different cache key") + } +} diff --git a/redact/opf_test.go b/redact/opf_test.go index ae4aa26321..12c47735bf 100644 --- a/redact/opf_test.go +++ b/redact/opf_test.go @@ -10,6 +10,7 @@ import ( "strings" "sync" "testing" + "time" ) // shCmd returns a Cmd that runs the given shell snippet via `sh -c`. Used @@ -27,6 +28,8 @@ type fakeRuntime struct { err error calls int batchCalls int + // failFromBatch, when set, fails that RedactBatch call and every later one. + failFromBatch int } func (f *fakeRuntime) Redact(_ context.Context, _ string, _ []string) ([]Span, error) { @@ -43,6 +46,9 @@ func (f *fakeRuntime) RedactBatch(_ context.Context, inputs []string, _ []string if f.err != nil { return nil, f.err } + if f.failFromBatch > 0 && f.batchCalls >= f.failFromBatch { + return nil, errors.New("opf killed mid-scan") + } out := make([][]Span, len(inputs)) for i := range inputs { out[i] = f.spans @@ -82,8 +88,113 @@ func TestConfigurePrivacyFilter_AppliesDefaults(t *testing.T) { if got.Command != "opf" { t.Errorf("default Command: want \"opf\", got %q", got.Command) } - if got.Timeout != 30 { - t.Errorf("default Timeout: want 30, got %d", got.Timeout) + // Timeout is deliberately NOT defaulted here: normalizing it would make an + // unset setting indistinguishable from an explicit timeout_seconds, and + // the size-adaptive deadline would never run. A small batch still lands on + // the same ~30s the old fixed default gave. + if got.Timeout != 0 { + t.Errorf("unset Timeout must stay 0 so the adaptive deadline can apply, got %d", got.Timeout) + } + if d := adaptiveOPFTimeout(got.Timeout, 4*1024); d != opfTimeoutFloor { + t.Errorf("small batch with unset Timeout: want %s, got %s", opfTimeoutFloor, d) + } +} + +func TestAdaptiveOPFTimeout(t *testing.T) { + t.Parallel() + const mib = 1024 * 1024 + cases := []struct { + name string + configured int + batchedLen int + want time.Duration + }{ + {"unset tiny batch lands on the floor", 0, 1024, opfTimeoutFloor}, + {"unset empty batch lands on the floor", 0, 0, opfTimeoutFloor}, + // 1 MiB × 1.14 s/KB × 2 = 2334.72s, well past the floor and far + // short of the ceiling. + {"unset large batch scales with size", 0, mib, 2334720 * time.Millisecond}, + {"unset huge batch is clamped at the ceiling", 0, 64 * mib, opfTimeoutCeiling}, + // An explicit setting wins in both directions: it is not raised for a + // batch the formula would give hours, nor lowered below the floor. + {"explicit override wins over a scaled estimate", 45, 64 * mib, 45 * time.Second}, + {"explicit override wins below the floor", 5, 1024, 5 * time.Second}, + {"negative configured value is treated as unset", -1, 1024, opfTimeoutFloor}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := adaptiveOPFTimeout(tc.configured, tc.batchedLen); got != tc.want { + t.Errorf("adaptiveOPFTimeout(%d, %d) = %s, want %s", + tc.configured, tc.batchedLen, got, tc.want) + } + }) + } +} + +// TestShellOut_RedactBatchDeadlineFollowsBatchSize pins the wiring, not the +// formula: the deadline RedactBatch installs must come from the size of the +// batch it is about to send, so a legitimately slow large call is not killed +// on a small call's budget. +func TestShellOut_RedactBatchDeadlineFollowsBatchSize(t *testing.T) { + t.Parallel() + cases := []struct { + name string + timeoutSeconds int + input string + wantAtLeast time.Duration + wantAtMost time.Duration + }{ + { + name: "unset timeout, small input: the floor", + input: "hello world", + wantAtLeast: opfTimeoutFloor - 5*time.Second, + wantAtMost: opfTimeoutFloor, + }, + { + name: "unset timeout, 1 MiB input: scaled well past the floor", + input: strings.Repeat("a", 1024*1024), + wantAtLeast: 30 * time.Minute, + wantAtMost: opfTimeoutCeiling, + }, + { + name: "explicit timeout ignores batch size", + timeoutSeconds: 5, + input: strings.Repeat("a", 1024*1024), + wantAtLeast: time.Second, + wantAtMost: 5 * time.Second, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var mu sync.Mutex + var remaining time.Duration + rt := &shellOut{ + command: "opf", + timeoutSeconds: tc.timeoutSeconds, + commandRunner: func(ctx context.Context, _ string, _ ...string) *exec.Cmd { + mu.Lock() + if deadline, ok := ctx.Deadline(); ok { + remaining = time.Until(deadline) + } + mu.Unlock() + return shCmd(ctx, `cat >/dev/null; printf '{"detected_spans":[]}'`) + }, + } + if _, err := rt.RedactBatch(context.Background(), []string{tc.input}, []string{"private_person"}); err != nil { + t.Fatalf("RedactBatch: %v", err) + } + mu.Lock() + defer mu.Unlock() + if remaining == 0 { + t.Fatal("command context carried no deadline") + } + if remaining < tc.wantAtLeast || remaining > tc.wantAtMost { + t.Errorf("deadline remaining = %s, want within [%s, %s]", + remaining, tc.wantAtLeast, tc.wantAtMost) + } + }) } } @@ -344,10 +455,19 @@ func TestShellOut_RedactBatchRejectsOversizedInputBeforeCommand(t *testing.T) { }, } - _, err := rt.RedactBatch(context.Background(), - []string{strings.Repeat("x", 16*1024*1024+1)}, - []string{"private_person"}, - ) + // One chunk reused across the slice: Go strings share backing storage, + // so the whole input weighs 16 MiB in memory while presenting more than + // opfMaxBatchInputBytes of length to the size check. Materializing a + // single 256 MiB string instead would make this test allocate the very + // buffer the limit exists to prevent. + const chunkBytes = 16 * 1024 * 1024 + chunk := strings.Repeat("x", chunkBytes) + inputs := make([]string, opfMaxBatchInputBytes/chunkBytes+1) + for i := range inputs { + inputs[i] = chunk + } + + _, err := rt.RedactBatch(context.Background(), inputs, []string{"private_person"}) if err == nil { t.Fatal("RedactBatch: want oversized input error, got nil") }