refactor(strategy): address review — redact URLs, harden push-target guard, tidy marker store · Entire
refactor(strategy): address review — redact URLs, harden push-target guard, tidy marker store
e907fa2→main·
karthik-rameshkumar·3d ago·6 files·+163 added/-34 removed
Applies the multi-agent review findings on the empty-remote guard:
- Privacy: redact push-target URLs before logging them (they can embed
credentials). Use checkpoint/remote.RedactURL in both deferral log lines,
matching the package's existing promisor-logging pattern.
- Correctness: PushTargetsInDir no longer relies on IsURL's bare-'@' heuristic
to skip pushurl resolution, which misclassified a remote NAME containing '@'
(e.g. "build@ci") as a URL and permanently fail-closed-deferred it. New
isConcretePushTarget follows git's own scp detection (colon before any
slash). Also returns an explicit error for an empty target instead of a
silent []string{""} on the new exported API.
- Consistency: move the bootstrap marker from the ad-hoc .git/entire/ file to
the sibling-convention .git/entire-push-bootstrap/ directory, route reads and
writes through os.Root like session state, and read content+mtime from a
single open file handle (was ReadFile + Stat). Register the marker with
entire clean discovery/deletion so a reset clears it.
Tests: isConcretePushTarget classification (incl. '@' remote name), empty-target error; existing marker/defer/OPF/cleanup suites and the full canary still pass.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Sessions
01KXG1Q21WTF76Q8E851XF226AView transcript
Changes
6
cmd/entire/cli
checkpoint/remote
Mgit.go+26/-3
Mgit_test.go+33
Mclean.go+8/-3
strategy
Mcleanup.go+50/-11
Mmanual_commit_push.go+43/-16
Mmanual_commit_push_test.go+3/-1
2 unmodified lines
3
4
5
6
7
8
9
237 unmodified lines
247
248
249
249
250
251
252
253
254
255
256
9 unmodified lines
266
267
268
265
266
269
270
271
272
273
2 unmodified lines
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
2 unmodified lines
import (
"context"
"encoding/base64"
"errors"
"fmt"
"log/slog"
"os"
237 unmodified lines
// can contain more than one destination. URLs and local paths are already
// concrete push targets and are returned unchanged.
func PushTargetsInDir(ctx context.Context, dir, target string) ([]string, error) {
if target == "" || IsURL(target) || isLocalPath(target) {
if target == "" {
return nil, errors.New("push target must not be empty")
}
if isConcretePushTarget(target) {
return []string{target}, nil
}
9 unmodified lines
var targets []string
for _, line := range strings.Split(string(out), "\n") {
if target := strings.TrimSpace(line); target != "" {
targets = append(targets, target)
if pushURL := strings.TrimSpace(line); pushURL != "" {
targets = append(targets, pushURL)
}
}
}
if len(targets) == 0 {
2 unmodified lines
return targets, nil
}
// isConcretePushTarget reports whether target is already a concrete push
// endpoint (a URL or local path) rather than a git remote NAME whose pushurl
// must be resolved. It deliberately does not reuse IsURL's '@' heuristic, which
// misclassifies a remote name that merely contains '@' (e.g. "build@ci") as a
// URL and would skip pushurl resolution for it. Following git's own transport
// detection, an scp-like SSH target has a colon before any slash
// ("[user@]host:path"); a bare remote name has neither a scheme nor such a
// colon.
func isConcretePushTarget(target string) bool {
if strings.Contains(target, "://") || isLocalPath(target) {
return true
}
// scp-like SSH URL: a colon appears before any slash.
if i := strings.IndexAny(target, ":/"); i >= 0 && target[i] == ':' {
return true
}
return false
}
func lsRemote(ctx context.Context, dir, remote string, patterns ...string) ([]byte, error) {
args := append([]string{"ls-remote", remote}, patterns...)
cmd := newCommand(ctx, args...)
Mcmd/entire/cli/checkpoint/remote/git.go+26/-3
755 unmodified lines
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
755 unmodified lines
}
}
func TestIsConcretePushTarget(t *testing.T) {
t.Parallel()
tests := []struct {
name string
val string
want bool
}{
{"remote name", "origin", false},
{"remote name with @ (no colon)", "build@ci", false}, // must resolve pushurl, not be treated as a URL
{"HTTPS URL", "https://github.com/org/repo.git", true},
{"SSH protocol URL", "ssh://git@github.com:22/org/repo.git", true},
{"scp SSH with user", "git@github.com:org/repo.git", true},
{"scp SSH without user", "github.com:org/repo.git", true},
{"absolute path", "/tmp/repo.git", true},
{"relative path", "./repo.git", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
assert.Equal(t, tt.want, isConcretePushTarget(tt.val))
})
}
}
func TestPushTargetsInDir_EmptyTargetErrors(t *testing.T) {
t.Parallel()
_, err := PushTargetsInDir(context.Background(), "", "")
require.Error(t, err)
}
func TestIsLocalPath(t *testing.T) {
t.Parallel()
Mcmd/entire/cli/checkpoint/remote/git_test.go+33
316 unmodified lines
317
318
319
320
320
321
322
323
2 unmodified lines
326
327
328
329
330
331
332
333
5 unmodified lines
339
340
341
342
343
344
345
31 unmodified lines
377
378
379
377
378
380
381
382
383
384
1 unmodified line
386
387
388
389
390
391
392
4 unmodified lines
397
398
399
400
401
402
403
316 unmodified lines
}
// Group items by type for display
var branches, states, checkpoints []strategy.CleanupItem
var branches, states, checkpoints, pushBootstrap []strategy.CleanupItem
for _, item := range items {
switch item.Type {
case strategy.CleanupTypeShadowBranch:
2 unmodified lines
states = append(states, item)
case strategy.CleanupTypeCheckpoint:
checkpoints = append(checkpoints, item)
case strategy.CleanupTypePushBootstrap:
pushBootstrap = append(pushBootstrap, item)
}
}
5 unmodified lines
printSection(w, "Shadow branches", cleanupItemIDs(branches))
printSection(w, "Session states", cleanupItemIDs(states))
printSection(w, "Checkpoint metadata", cleanupItemIDs(checkpoints))
printSection(w, "Push-bootstrap marker", cleanupItemIDs(pushBootstrap))
printSection(w, "Temp files", tempFiles)
if dryRun {
31 unmodified lines
deletedTempFiles, failedTempFiles := deleteTempFiles(ctx, tempFiles)
// Report results
totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints) + len(deletedTempFiles)
totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + len(failedTempFiles)
totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints) + len(result.PushBootstrap) + len(deletedTempFiles)
totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + len(result.FailedPushBootstrap) + len(failedTempFiles)
if totalDeleted > 0 {
fmt.Fprintf(w, "✓ Deleted %d %s:\n", totalDeleted, itemWord(totalDeleted))
printResultSection(w, "Shadow branches", result.ShadowBranches)
printResultSection(w, "Session states", result.SessionStates)
printResultSection(w, "Checkpoints", result.Checkpoints)
printResultSection(w, "Push-bootstrap marker", result.PushBootstrap)
printResultSection(w, "Temp files", deletedTempFiles)
}
4 unmodified lines
printResultSection(errW, "Shadow branches", result.FailedBranches)
printResultSection(errW, "Session states", result.FailedStates)
printResultSection(errW, "Checkpoints", result.FailedCheckpoints)
printResultSection(errW, "Push-bootstrap marker", result.FailedPushBootstrap)
if len(failedTempFiles) > 0 {
fmt.Fprintf(errW, "\nTemp files:\n")
Mcmd/entire/cli/clean.go+8/-3
3 unmodified lines
4
5
6
7
8
9
10
21 unmodified lines
32
33
34
34
35
36
35
36
37
38
39
40
41
5 unmodified lines
47
48
49
48
49
50
51
52
53
50
51
52
53
54
55
56
57
58
59
60
475 unmodified lines
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
11 unmodified lines
564
565
566
567
568
569
570
2 unmodified lines
573
574
575
576
577
578
579
580
78 unmodified lines
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
645
646
684
685
686
687
688
3 unmodified lines
"context"
"fmt"
"log/slog"
"os"
"os/exec"
"regexp"
"sort"
21 unmodified lines
// CleanupType represents the type of a cleanup activity.
type CleanupType string
const (
CleanupTypeShadowBranch CleanupType = "shadow-branch"
CleanupTypeSessionState CleanupType = "session-state"
CleanupTypeCheckpoint CleanupType = "checkpoint"
CleanupTypeShadowBranch CleanupType = "shadow-branch"
CleanupTypeSessionState CleanupType = "session-state"
CleanupTypeCheckpoint CleanupType = "checkpoint"
CleanupTypePushBootstrap CleanupType = "push-bootstrap"
)
// CleanupItem represents an item that can be cleaned up.
// CleanupResult contains the results of a cleanup operation.
type CleanupResult struct {
ShadowBranches []string // Deleted shadow branches
SessionStates []string // Deleted session state files
Checkpoints []string // Deleted checkpoint metadata
FailedBranches []string // Shadow branches that failed to delete
FailedStates []string // Session states that failed to delete
FailedCheckpoints []string // Checkpoints that failed to delete
ShadowBranches []string // Deleted shadow branches
SessionStates []string // Deleted session state files
Checkpoints []string // Deleted checkpoint metadata
PushBootstrap []string // Deleted push-bootstrap markers
FailedBranches []string // Shadow branches that failed to delete
FailedStates []string // Session states that failed to delete
FailedCheckpoints []string // Checkpoints that failed to delete
FailedPushBootstrap []string // Push-bootstrap markers that failed to delete
}
// shadowBranchPattern matches shadow branch names in both old and new formats:
275 unmodified lines
})
}
// Push-bootstrap marker (empty-remote guard cache), when present.
if dir, err := pushBootstrapDir(ctx); err == nil {
if _, statErr := os.Stat(dir); statErr == nil {
cleanupItems = append(cleanupItems, CleanupItem{
Type: CleanupTypePushBootstrap,
ID: pushBootstrapDirName,
Reason: "clean all",
})
}
}
return cleanupItems, nil
}
11 unmodified lines
// Group items by type
var branches, states, checkpoints []string
pushBootstrap := false
for _, item := range items {
switch item.Type {
case CleanupTypeShadowBranch:
2 unmodified lines
states = append(states, item.ID)
case CleanupTypeCheckpoint:
checkpoints = append(checkpoints, item.ID)
case CleanupTypePushBootstrap:
pushBootstrap = true
}
}
78 unmodified lines
}
// Delete the push-bootstrap marker directory.
if pushBootstrap {
if dir, err := pushBootstrapDir(ctx); err == nil {
if rmErr := os.RemoveAll(dir); rmErr != nil {
result.FailedPushBootstrap = append(result.FailedPushBootstrap, pushBootstrapDirName)
logging.Warn(logCtx, "failed to delete push-bootstrap marker",
slog.String("type", string(CleanupTypePushBootstrap)),
slog.String("id", pushBootstrapDirName),
slog.String("error", rmErr.Error()),
)
} else {
result.PushBootstrap = append(result.PushBootstrap, pushBootstrapDirName)
logging.Info(logCtx, "deleted push-bootstrap marker",
slog.String("type", string(CleanupTypePushBootstrap)),
slog.String("id", pushBootstrapDirName),
slog.String("reason", reasonMap[pushBootstrapDirName]),
)
}
}
// Log summary
totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints)
totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints)
totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints) + len(result.PushBootstrap)
totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + len(result.FailedPushBootstrap)
if totalDeleted > 0 || totalFailed > 0 {
logging.Info(logCtx, "cleanup completed",
slog.Int("deleted_branches", len(result.ShadowBranches)),