Skip to content

Fix checkpoint_remote silently falling back to origin on owner mismatch #805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
blackgirlbytes wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix checkpoint_remote silently falling back to origin on owner mismatch #805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions cmd/entire/cli/strategy/checkpoint_remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,39 +76,46 @@ func resolvePushSettings(ctx context.Context, pushRemoteName string) pushSetting
return ps
}

// Get the push remote URL for protocol detection and fork detection
// Get the push remote URL for protocol detection
pushRemoteURL, err := getRemoteURL(ctx, pushRemoteName)
if err != nil {
fmt.Fprintf(os.Stderr, "[entire] Warning: checkpoint_remote is configured (%s) but could not get push remote URL: %v\n", config.Repo, err)
fmt.Fprintln(os.Stderr, "[entire] Checkpoints will be pushed to origin instead. Check your git remote configuration.")
Comment on lines +82 to +83
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback message hard-codes "origin", but resolvePushSettings is called with the remote name passed by the git pre-push hook (not always "origin"). This warning can mislead users when pushing to a different remote; consider referencing pushRemoteName (or wording like "your push remote") instead.

Copilot uses AI. Check for mistakes.
logging.Debug(ctx, "checkpoint-remote: could not get push remote URL, skipping",
slog.String("remote", pushRemoteName),
slog.String("error", err.Error()),
)
return ps
}

// Parse the push remote URL once for both fork detection and URL derivation
// Parse the push remote URL for protocol detection and URL derivation
pushInfo, err := parseGitRemoteURL(pushRemoteURL)
if err != nil {
fmt.Fprintf(os.Stderr, "[entire] Warning: checkpoint_remote is configured (%s) but could not parse push remote URL: %v\n", config.Repo, err)
fmt.Fprintln(os.Stderr, "[entire] Checkpoints will be pushed to origin instead. Check your git remote configuration.")
Comment on lines +94 to +95
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback message hard-codes "origin", but resolvePushSettings may be invoked for other remotes (e.g., when pushing to an alternate remote). Recommend referencing pushRemoteName (or "the push remote") so the stderr guidance is accurate.

Copilot uses AI. Check for mistakes.
logging.Warn(ctx, "checkpoint-remote: could not parse push remote URL",
slog.String("remote", pushRemoteName),
slog.String("error", err.Error()),
)
return ps
}

// Fork detection: compare owners
// Log when push remote owner differs from checkpoint remote owner.
// Previously this skipped checkpoint_remote entirely (fork detection),
// but the user explicitly configured a checkpoint remote — respect that choice.
Comment on lines +104 to +105
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior here no longer skips checkpoint_remote on owner mismatch, but the function-level comment above resolvePushSettings still states it "Skips if the push remote owner differs". Please update the doc comment to match the new behavior so future readers don't reintroduce the fork-detection skip.

Suggested change
// Previously this skipped checkpoint_remote entirely (fork detection),
// but the user explicitly configured a checkpoint remote — respect that choice.
// We no longer skip checkpoint_remote in this case (previously used for fork detection);
// the user explicitly configured a checkpoint remote, so we always proceed with it.

Copilot uses AI. Check for mistakes.
checkpointOwner := config.Owner()
if pushInfo.owner != "" && checkpointOwner != "" && !strings.EqualFold(pushInfo.owner, checkpointOwner) {
logging.Debug(ctx, "checkpoint-remote: push remote owner differs from checkpoint remote owner, skipping (fork detected)",
logging.Debug(ctx, "checkpoint-remote: push remote owner differs from checkpoint remote owner, proceeding with configured checkpoint remote",
slog.String("push_owner", pushInfo.owner),
slog.String("checkpoint_owner", checkpointOwner),
)
return ps
}

// Derive checkpoint URL using same protocol as push remote
checkpointURL, err := deriveCheckpointURLFromInfo(pushInfo, config)
if err != nil {
fmt.Fprintf(os.Stderr, "[entire] Warning: checkpoint_remote is configured (%s) but could not derive URL from push remote: %v\n", config.Repo, err)
fmt.Fprintln(os.Stderr, "[entire] Checkpoints will be pushed to origin instead. Check your checkpoint_remote settings.")
Comment on lines +117 to +118
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback warning says checkpoints will be pushed to "origin", but the code actually falls back to pushing to ps.remote (the remote passed into resolvePushSettings). Update the message to avoid implying a specific remote name.

Copilot uses AI. Check for mistakes.
logging.Warn(ctx, "checkpoint-remote: could not derive URL from push remote",
slog.String("remote", pushRemoteName),
slog.String("repo", config.Repo),
Expand Down
7 changes: 4 additions & 3 deletions cmd/entire/cli/strategy/checkpoint_remote_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,10 @@ func TestResolvePushSettings_ForkDetection(t *testing.T) {
t.Chdir(localDir)

ps := resolvePushSettings(ctx, "origin")
// Should fall back to origin since fork detected (alice != org)
assert.False(t, ps.hasCheckpointURL())
assert.Equal(t, "origin", ps.pushTarget())
// Should proceed with checkpoint remote even when owners differ —
// the user explicitly configured it, respect that choice.
assert.True(t, ps.hasCheckpointURL())
assert.Equal(t, "git@github.com:org/checkpoints.git", ps.pushTarget())
Comment on lines +493 to +496
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name TestResolvePushSettings_ForkDetection is now misleading because the expected behavior is to proceed even when owners differ (i.e., fork-detection no longer disables checkpoint_remote). Consider renaming the test to reflect the new requirement (owner mismatch should not cause fallback).

Copilot uses AI. Check for mistakes.
assert.False(t, ps.pushDisabled)
}

Expand Down
Loading