address OPF review feedback · Entire

address OPF review feedback

f324879→main·

peyton-alt·1mo ago·15 files·+333 added/-89 removed

Sessions

3f7a2c0b3fc3View transcript

?\ Address OPF Review Feedback and TestingCodex·GPT-5.5·1 step

Changes

15

4216 unmodified lines

4217
4218
4219
4220
4220
4221
4222
4223
25 unmodified lines

4249
4250
4251
4252
4252
4253
4254
4255
4256
4257
4258
4259
4259
4260
4261
4262
46 unmodified lines

4309
4310
4311
4312
4312
4313
4314
4315
49 unmodified lines

4365
4366
4367
4368
4368
4369
4370
4371

4216 unmodified lines

}

entries := make(map[string]object.TreeEntry)
err = addDirectoryToEntriesWithAbsPath(repo, metadataDir, ".entire/metadata/session", entries)
err = addDirectoryToEntriesWithAbsPath(context.Background(), repo, metadataDir, ".entire/metadata/session", entries)
if err != nil {
    t.Fatalf("addDirectoryToEntriesWithAbsPath failed: %v", err)
}
25 unmodified lines

expectedPath := filepath.ToSlash(filepath.Join("checkpoint", "..generated", "schema.json"))

entries := make(map[string]object.TreeEntry)
if err := addDirectoryToEntriesWithAbsPath(repo, metadataDirAbs, "checkpoint", entries); err != nil {
if err := addDirectoryToEntriesWithAbsPath(context.Background(), repo, metadataDirAbs, "checkpoint", entries); err != nil {
    t.Fatalf("addDirectoryToEntriesWithAbsPath failed: %v", err)
}
if _, ok := entries[expectedPath]; !ok {
    t.Fatalf("expected entry at %q, got entries: %v", expectedPath, entries)
}

changes, err := addDirectoryToChanges(repo, metadataDir, "checkpoint")
changes, err := addDirectoryToChanges(context.Background(), repo, metadataDir, "checkpoint")
if err != nil {
    t.Fatalf("addDirectoryToChanges failed: %v", err)
}
46 unmodified lines

}

entries := make(map[string]object.TreeEntry)
err = addDirectoryToEntriesWithAbsPath(repo, metadataDir, "checkpoint/", entries)
err = addDirectoryToEntriesWithAbsPath(context.Background(), repo, metadataDir, "checkpoint/", entries)
if err != nil {
    t.Fatalf("addDirectoryToEntriesWithAbsPath failed: %v", err)
}
49 unmodified lines

}

entries := make(map[string]object.TreeEntry)
err = addDirectoryToEntriesWithAbsPath(repo, metadataDir, "checkpoint/", entries)
err = addDirectoryToEntriesWithAbsPath(context.Background(), repo, metadataDir, "checkpoint/", entries)
if err != nil {
    t.Fatalf("addDirectoryToEntriesWithAbsPath failed: %v", err)
}

Mcmd/entire/cli/checkpoint/checkpoint_test.go+5/-5

1857 unmodified lines

1858
1859
1860
1861
1862
1861
1862
1863
1864
21 unmodified lines

1886
1887
1888
1890
1889
1890
1891
1892
20 unmodified lines

1913
1914
1915
1917
1916
1917
1918
1919

1857 unmodified lines

// tree, re-redacts these blobs with OPF when enabled, and
// rewrites entire/checkpoints/v1 into 8-layer commits before
// they leave the local machine.
_ = ctx // ctx not needed by the 7-layer path; kept on caller signature for future use
blobHash, mode, err := createRedactedBlobFromFile(s.repo, path, relPath)
blobHash, mode, err := createRedactedBlobFromFile(ctx, s.repo, path, relPath)
if err != nil {
    return fmt.Errorf("failed to create blob for %s: %w", path, err)
}
21 unmodified lines

// (strategy/manual_commit_opf_rewrite.go), which re-redacts the 7-layer
// blobs into 8-layer commits before they leave the local machine.
// JSONL files get JSONL-aware redaction; all other files get plain byte redaction.
func createRedactedBlobFromFile(repo *git.Repository, filePath, treePath string) (plumbing.Hash, filemode.FileMode, error) {
func createRedactedBlobFromFile(ctx context.Context, repo *git.Repository, filePath, treePath string) (plumbing.Hash, filemode.FileMode, error) {
info, err := os.Stat(filePath)
if err != nil {
    return plumbing.ZeroHash, 0, fmt.Errorf("failed to stat file: %w", err)
}
20 unmodified lines

return hash, mode, nil
}

content = RedactBlobBytes(context.Background(), content, treePath, false)
content = RedactBlobBytes(ctx, content, treePath, false)

hash, err := CreateBlobFromContent(repo, content)
if err != nil {

Mcmd/entire/cli/checkpoint/committed.go+3/-4

833 unmodified lines

834
835
836
837
837
838
839
840
104 unmodified lines

945
946
947
948
948
949
950
951
36 unmodified lines

988
989
990
991
991
992
993
994
19 unmodified lines

1014
1015
1016
1017
1017
1018
1019
1020
26 unmodified lines

1047
1048
1049
1050
1050
1051
1052
1053

833 unmodified lines

if relErr != nil {
        logInvalidGitTreePath(ctx, "add metadata directory", metadataDir, relErr)
    } else {
        metaChanges, metaErr := addDirectoryToChanges(s.repo, metadataDirAbs, metadataRel)
        metaChanges, metaErr := addDirectoryToChanges(ctx, s.repo, metadataDirAbs, metadataRel)
        if metaErr != nil {
            return plumbing.ZeroHash, fmt.Errorf("failed to add metadata directory: %w", metaErr)
        }
104 unmodified lines

// addDirectoryToEntriesWithAbsPath recursively adds all files in a directory to the entries map.
func addDirectoryToEntriesWithAbsPath(repo *git.Repository, dirPathAbs, dirPathRel string, entries map[string]object.TreeEntry) error {
func addDirectoryToEntriesWithAbsPath(ctx context.Context, repo *git.Repository, dirPathAbs, dirPathRel string, entries map[string]object.TreeEntry) error {
err := filepath.Walk(dirPathAbs, func(path string, info os.FileInfo, err error) error {
    if err != nil {
        return err
    }

// Use redacted blob creation for metadata files (transcripts, prompts, etc.)
    // to ensure PII and secrets are redacted before writing to git.
    blobHash, mode, err := createRedactedBlobFromFile(repo, path, treePath)
    blobHash, mode, err := createRedactedBlobFromFile(ctx, repo, path, treePath)
    if err != nil {
        return fmt.Errorf("failed to create blob for %s: %w", path, err)
    }
19 unmodified lines

// addDirectoryToChanges walks a filesystem directory and returns TreeChange entries
// for each file, suitable for use with ApplyTreeChanges.
// dirPathAbs is the absolute filesystem path; dirPathRel is the git tree-relative path.
func addDirectoryToChanges(repo *git.Repository, dirPathAbs, dirPathRel string) ([]TreeChange, error) {
func addDirectoryToChanges(ctx context.Context, repo *git.Repository, dirPathAbs, dirPathRel string) ([]TreeChange, error) {
var changes []TreeChange
err := filepath.Walk(dirPathAbs, func(path string, info os.FileInfo, err error) error {
    if err != nil {
26 unmodified lines

treePath := filepath.ToSlash(filepath.Join(dirPathRel, relWithinDir))

blobHash, mode, blobErr := createRedactedBlobFromFile(repo, path, treePath)
blobHash, mode, blobErr := createRedactedBlobFromFile(ctx, repo, path, treePath)
if blobErr != nil {
        return fmt.Errorf("failed to create blob for %s: %w", path, blobErr)
    }

Mcmd/entire/cli/checkpoint/temporary.go+5/-5

268 unmodified lines

269
270
271
272
272
273
274
275

268 unmodified lines

}

if metadataDir != "" && metadataDirAbs != "" {
    if err := addDirectoryToEntriesWithAbsPath(repo, metadataDirAbs, metadataDir, entries); err != nil {
    if err := addDirectoryToEntriesWithAbsPath(context.Background(), repo, metadataDirAbs, metadataDir, entries); err != nil {
        t.Fatalf("add metadata: %v", err)
    }
}

Mcmd/entire/cli/checkpoint/tree_surgery_equiv_test.go+1/-1

988 unmodified lines

989
990
991
992
993
994
995
996
997

988 unmodified lines

return fmt.Errorf("openai_privacy_filter.categories has unknown key %q (see docs/security-and-privacy.md for the supported set)", name)
    }
}
if opf.TimeoutSeconds < 0 {
    return fmt.Errorf("openai_privacy_filter.timeout_seconds must be greater than or equal to 0 (got %d)", opf.TimeoutSeconds)
}
switch opf.PromptDefault {
case "", OPFPromptAsk, OPFPromptAlways, OPFPromptNever:
    // ok
}

Mcmd/entire/cli/settings/settings.go+3

4 unmodified lines

5
6
7
8
9
10
11
982 unmodified lines

994
995
996
997
998
999
1000
1001
1002
1003
1004
1005
1006
1007
1008
1009
1010
1011
1012
1013
1014
1015
1016
1017
1018
1019
1020
1021
1022
1023
1024
1025

4 unmodified lines

"encoding/json"
    "os"
    "path/filepath"
    "strconv"
    "strings"
    "testing"
    "time"
982 unmodified lines

}
}

func TestLoadFromBytes_OPFSettings_TimeoutValidation(t *testing.T) {
t.Parallel()
cases := []struct {
name string
value int
wantErr bool
}{
{name: "positive_allowed", value: 45},
{name: "zero_allowed_as_default", value: 0},
{name: "negative_rejected", value: -1, wantErr: true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
body := []byte(`{"redaction":{"openai_privacy_filter":{"timeout_seconds":` + strconv.Itoa(tc.value) + `}}}`)
_, err := LoadFromBytes(body)
if tc.wantErr && err == nil {
t.Fatalf("expected error for timeout_seconds=%d, got nil", tc.value)
}
if !tc.wantErr && err != nil {
t.Fatalf("unexpected error for timeout_seconds=%d: %v", tc.value, err)
}
})
}
}

// TestLoadFromBytes_OPFSettings_Merge verifies override semantics for the
// merge path (settings.local.json on top of settings.json): present fields
// override, omitted fields preserve, categories merge per-key.

Mcmd/entire/cli/settings/settings_test.go+27/-5

3 unmodified lines

4
5
6
7
8
9
10
11
12
65 unmodified lines

78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
5 unmodified lines

102
103
104
90
105
106
107
108
8 unmodified lines

117
118
119
105
120
121
122
123
1 unmodified line

125
126
127
113
114
115
116
117
128
129
130
131
122
123
124
125
132
133
134
135
136
137
138
139
140
4 unmodified lines

145
146
147
148
149
150
151
152
153
154
141
155
156
157
158
145
159
160
161
162
6 unmodified lines

169
170
171
158
172
173
174
175
176
177
178
165
166
179
180
181
168
182
183
184
185
186
187
188
175
176
177
178
179
180
181
182
189
190
191
192
3 unmodified lines

196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230

3 unmodified lines

"context"
    "fmt"
    "log/slog"
    "os/exec"
    "regexp"
    "sort"
    "strings"
    "time"
65 unmodified lines

// The "entire/checkpoints/v1" branch is excluded as it stores permanent metadata.
// Returns an empty slice (not nil) if no shadow branches exist.
func ListShadowBranches(ctx context.Context) ([]string, error) {
    heads, err := listShadowBranchHeads(ctx)
    if err != nil {
        return nil, err
    }
    branches := make([]string, 0, len(heads))
    for branch := range heads {
        branches = append(branches, branch)
    }
    sort.Strings(branches)
    return branches, nil
}

func listShadowBranchHeads(ctx context.Context) (map[string]plumbing.Hash, error) {
    repo, err := OpenRepository(ctx)
    if err != nil {
        return nil, fmt.Errorf("failed to open git repository: %w", err)
    }
5 unmodified lines

return nil, fmt.Errorf("failed to get references: %w", err)
    }

var shadowBranches []string
    shadowBranches := map[string]plumbing.Hash{}

err = refs.ForEach(func(ref *plumbing.Reference) error {
        if err := ctx.Err(); err != nil {
            return err
        }

branchName := strings.TrimPrefix(ref.Name().String(), "refs/heads/")

if IsShadowBranch(branchName) {
            shadowBranches = append(shadowBranches, branchName)
            shadowBranches[branchName] = ref.Hash()
        }
        return nil
    })
1 unmodified line

return nil, fmt.Errorf("failed to iterate references: %w", err)
    }

// Ensure we return empty slice, not nil
    if shadowBranches == nil {
        shadowBranches = []string{}
    }

return shadowBranches, nil
}

// CleanupPushedShadowBranches deletes shadow branches whose sessions
// have all ended cleanly (no active session referencing them, no
// pending turn-checkpoints awaiting finalization). Intended to be
// called only after a successful push so the caller knows any
// condensed checkpoint data already reached the remote.
// have fully condensed into committed checkpoint metadata (no active
// session referencing them, no pending turn-checkpoints awaiting
// finalization, and no ended-but-uncondensed session still relying on
// shadow-only data). Intended to be called only after a successful push
// so the caller knows any condensed checkpoint data already reached
// the remote.

// Returns the count of branches deleted. Failures (e.g., one branch
// fails to delete due to a stale lock) are logged but don't abort
4 unmodified lines

//     == nil (still active).
//   - Skips any shadow branch whose session has TurnCheckpointIDs
//     pending (mid-finalize race window).
//   - Skips ended sessions until PhaseEnded and FullyCondensed prove the
//     shadow branch contents have been copied to committed metadata.
//   - Multiple sessions can share the same shadow branch (same base
//     commit + worktree); ALL must satisfy the criteria above.
//   - Shadow branches with no associated session state are deleted
//     (no session to lose data from).
func CleanupPushedShadowBranches(ctx context.Context) (int, error) {
    branches, err := ListShadowBranches(ctx)
    branchHeads, err := listShadowBranchHeads(ctx)
    if err != nil {
        return 0, fmt.Errorf("list shadow branches: %w", err)
    }
    if len(branches) == 0 {
    if len(branchHeads) == 0 {
        return 0, nil
    }

6 unmodified lines

// because at least one session still depends on them.
    protected := map[string]bool{}
    for _, s := range states {
        if s.EndedAt != nil && len(s.TurnCheckpointIDs) == 0 {
            if s.Phase == session.PhaseEnded && s.FullyCondensed && len(s.TurnCheckpointIDs) == 0 {
                continue // safe — session ended cleanly and finalized
            }
            shadow := getShadowBranchNameForCommit(s.BaseCommit, s.WorktreeID)
            protected[shadow] = true
        }
    }

var toDelete []string
    for _, b := range branches {
toDelete := map[string]plumbing.Hash{}
    for b, hash := range branchHeads {
        if !protected[b] {
            toDelete = append(toDelete, b)
            toDelete[b] = hash
        }
    }
    if len(toDelete) == 0 {
        return 0, nil
    }

deleted, failed, delErr := DeleteShadowBranches(ctx, toDelete)
    if delErr != nil {
        // DeleteShadowBranches signature returns an error for future
        // extensibility but currently always returns nil; log defensively.
        logging.Warn(ctx, "shadow branch deletion reported error",
            slog.String("error", delErr.Error()),
        )
    }
    deleted, failed := DeleteShadowBranchesIfUnchanged(ctx, toDelete)
    if len(failed) > 0 {
        logging.Warn(ctx, "some shadow branches failed to delete during post-push cleanup",
            slog.Int("failed_count", len(failed))
        )
}
return len(deleted), nil
}

// DeleteShadowBranchesIfUnchanged deletes shadow branches only if each branch
// still points at the hash observed by the caller. This avoids deleting a
// branch that another session advanced after cleanup's initial scan.
func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]plumbing.Hash) (deleted []string, failed []string) {
    if len(branches) == 0 {
        return []string{}, []string{}
    }
    for branch, expected := range branches {
        if !IsShadowBranch(branch) || expected.IsZero() {
            failed = append(failed, branch)
            continue
        }
        ref := "refs/heads/" + branch
        cmd := exec.CommandContext(ctx, "git", "update-ref", "-d", ref, expected.String())
        if output, runErr := cmd.CombinedOutput(); runErr != nil {
            logging.Debug(ctx, "shadow branch unchanged-delete skipped",
                slog.String("branch", branch),
                slog.String("expected", expected.String()),
                slog.String("output", strings.TrimSpace(string(output))),
                slog.String("error", runErr.Error()),
            )
            failed = append(failed, branch)
            continue
        }
        deleted = append(deleted, branch)
    }
return deleted, failed
}

// DeleteShadowBranches deletes the specified branches from the repository.
// Returns two slices: successfully deleted branches and branches that failed to delete.
// Individual branch deletion failures do not stop the operation - all branches are attempted.