Refactor git commands and increase test coverage#995
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors git network operations (push/fetch/ls-remote) into a dedicated checkpoint/remote helper layer (including checkpoint token injection) and adds new integration coverage for HTTPS remotes and non-standard ports.
Changes:
- Replace ad-hoc
exec.Command("git", ...)usage in push/reconcile paths withcheckpoint/remotewrappers (Fetch,Push,LsRemote,ResolveFetchTarget). - Extend URL parsing/derivation to preserve non-standard ports and adjust SSH URL derivation accordingly.
- Add new integration tests for HTTPS remotes (TLS + auth header requirements) and enhance integration test environment configurability.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Promotes go-billy/v6 to a direct dependency (used by new HTTPS integration tests). |
| cmd/entire/cli/strategy/push_v2.go | Switches v2 ref push/fetch/ls-remote flows to checkpoint/remote wrappers. |
| cmd/entire/cli/strategy/push_common.go | Switches branch push/fetch logic to checkpoint/remote and removes local URL detection helper. |
| cmd/entire/cli/strategy/metadata_reconcile.go | Uses checkpoint/remote wrappers for ls-remote/fetch in reconcile flows. |
| cmd/entire/cli/strategy/checkpoint_token.go | Deletes old strategy-scoped git+token helper (logic moved/centralized). |
| cmd/entire/cli/strategy/checkpoint_remote_test.go | Adds checkpoint URL derivation tests for non-standard ports; removes old isURL test. |
| cmd/entire/cli/strategy/checkpoint_remote.go | Uses checkpoint/remote.Fetch for URL-based fetches. |
| cmd/entire/cli/integration_test/testenv.go | Adds ExtraEnv to pass per-test env vars to CLI subprocesses. |
| cmd/entire/cli/integration_test/http_remote_test.go | New HTTPS remote integration tests (token-required push + routing behavior). |
| cmd/entire/cli/gitremote/gitremote.go | Preserves port in parsed host (u.Host) for scheme URLs. |
| cmd/entire/cli/gitremote/gitremote_test.go | Adds coverage for parsing hosts with ports and standard-port behavior. |
| cmd/entire/cli/git_operations.go | Migrates fetch/push helpers to checkpoint/remote wrappers. |
| cmd/entire/cli/checkpoint/remote/util.go | Updates checkpoint URL derivation for SSH when host includes a port. |
| cmd/entire/cli/checkpoint/remote/git_test.go | Moves/expands tests to the remote package; adds IsURL coverage. |
| cmd/entire/cli/checkpoint/remote/git.go | New centralized git command wrapper layer (token injection + filtered fetches). |
Existing integration tests use local bare repo paths as remotes, which bypasses token injection, checkpoint_remote URL derivation, and HTTP transport code paths. Add four integration tests that exercise the full push/fetch flow over HTTPS using go-git's in-process smart HTTP backend: - TestHTTPS_PushCheckpointBranchToRemote: token-authenticated push - TestHTTPS_CheckpointRemoteRoutesToSeparateRepo: checkpoint_remote routes both push and fetch to a separate repo (two-clone concurrent push proves fetch routing via non-fast-forward recovery) - TestHTTPS_OutOfSyncCheckpointBranchRebases: fetch+rebase+retry over HTTPS when two clones race - TestHTTPS_PushFailsWithoutToken: 401 without token, success with Fix ParseURL to preserve non-standard ports (u.Hostname() strips them), and handle SSH URL-style ports in deriveCheckpointURLFromInfo by emitting ssh:// format instead of malformed SCP-style. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 31dd11a05e0c
The target parameter was always identical to the remote URL or name already present in the args slice. Replace it with extractRemoteFromArgs, which finds the first positional argument after the git subcommand by skipping flags. This eliminates the duplication at every call site. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 479d1780b8bf
Move the low-level git command builder from strategy/checkpoint_token.go into checkpoint/remote as three typed operations: Fetch, Push, and LsRemote. Each encapsulates token injection, GIT_TERMINAL_PROMPT=0 suppression, and (for Fetch) filtered-fetch args behind a structured API. This eliminates repetitive arg-slice construction and manual env-var appending at all 13 call sites. Callers now express intent through FetchOptions/Push/LsRemote rather than assembling raw git arguments. Key moves: - newCommand (renamed from CheckpointGitCommand) + token helpers → remote/git.go - ResolveFetchTarget, IsURL → remote/git.go (exported) - checkpoint_token.go, checkpoint_token_test.go → deleted (contents in remote/) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 34f0241a0ac7
Add NoFilter option to FetchOptions so callers can suppress --filter=blob:none even when filtered_fetches is globally enabled. FetchAndCheckoutRemoteBranch (used by resume to fetch a remote-only branch) now sets NoFilter: true because it needs full blob content (source files), not just tree structure. FetchMetadataBranch and FetchV2MainRef also set NoFilter since resume/explain need metadata blobs (transcripts, JSON). Push-time sync paths (fetchAndRebaseSessionsCommon, fetchAndMergeRef) still use filtered fetches for bandwidth efficiency. Also make --force on `entire resume` skip the "fetch from remote?" interactive prompt, enabling non-interactive use in CI and tests. Add TestResume_FetchesPrimaryBranchFullyWithFilteredFetches which enables filtered_fetches in a clone, deletes the feature branch locally, then confirms `entire resume --force` fetches it from origin with full blob content for both the primary branch and the metadata branch. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 45886ab6dd31
FetchBlobsByHash was routed through remote.Fetch, which adds --filter=blob:none when filtered_fetches is enabled. This is contradictory — the function downloads specific blobs by hash, but the filter tells git to exclude blobs from the packfile. This caused unnecessary fallbacks to a full metadata branch fetch. Add remote.FetchBlobs as a dedicated path that never applies --filter=blob:none and always uses --no-write-fetch-head. Its doc recommends passing a URL (not a remote name) to avoid persisting promisor settings on the named remote. Also rename ensureEnv to disableTerminalPrompt to satisfy the unparam linter (all callers passed the same constant). Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 77a7e37f01f2
Entire-Checkpoint: d2c30fad5a3c
Member
Author
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b3ac65b. Configure here.
util.go had an unexported duplicate of the exported constant from git.go. Use the exported one everywhere. Entire-Checkpoint: 5dd8f9232fab
Add Info.Port and HostPort() helper so Host stays hostname-only. Keeps ResolveRemoteRepo callers (trail_cmd apiHostAlias lookup) unaffected while letting checkpoint URL derivation handle non-default ports cleanly. Entire-Checkpoint: 666a7fa375fe
The checkpoint/remote package is imported as 'remote' in this file now; rename the parameter to remoteName to avoid shadowing. Entire-Checkpoint: 29da11f7278a
- git.go: drop cross-package resolveCheckpointFetchTarget mention - util.go: distinguish empty vs missing push remote in fallback error - integration tests: update CheckpointGitCommand / parseGitRemoteURL to their current names (remote.newCommand / remote.ParseURL) Entire-Checkpoint: 8b42be6f9e5d
appendCheckpointTokenEnv was stripping every existing GIT_CONFIG_* entry and writing count=1 with the auth header. Any caller- or CI-provided git config (safe.directory, custom CA, credential behavior, etc.) was silently dropped whenever ENTIRE_CHECKPOINT_TOKEN was set. Read the existing GIT_CONFIG_COUNT, leave KEY_*/VALUE_* entries in place, and append the Authorization header at the next free index. Entire-Checkpoint: 562e610aced8
deriveTokenOriginURL and PushURL's token path were producing URLs like https://host:2222/... when the source was ssh://git@host:2222/... — the SSH port doesn't map to an HTTPS port on the same server, so token-based fetch/push broke for self-hosted remotes with non-default SSH ports. Keep the port only when the source protocol was already HTTPS; drop it when coercing from SSH. Entire-Checkpoint: bbee48012b74
Push already coerced SSH→HTTPS via resolvePushCommandTarget/PushURL, so token-based auth worked for pushes from SSH origins. Fetch had no equivalent: remote.Fetch/LsRemote/FetchBlobs received 'origin' (or an SSH URL already resolved by filtered_fetches) and newCommand detected SSH, printed a warning, and skipped token injection. As a result v1 and v2 metadata fetches, doctor reconciliation, and resume branch fetches silently failed for users relying on ENTIRE_CHECKPOINT_TOKEN instead of SSH keys. Move the SSH→HTTPS coercion down into newCommand itself: when the token is set, resolve the target (remote name or URL), and rewrite SSH to the equivalent HTTPS URL in the args slice before exec. Every caller of newCommand — fetch, push, ls-remote, fetch blobs — now benefits uniformly, without callers needing to know about the rewrite. Unit tests cover SCP-style, ssh:// with port, already-HTTPS passthrough, local paths, unknown remotes, and the fallback path when rewrite can't produce a usable HTTPS URL. resolveTargetProtocol was only used by newCommand and its own tests; remove it in favor of resolveTargetForTokenAuth, which returns both the rewritten target and its resolved protocol in one pass. Entire-Checkpoint: da4fbdaeb02d
resolvePushCommandTarget was discarding PushURL's enabled flag and always returning a URL. Pushing via URL means git never updates refs/remotes/<name>/<branch>, so hasUnpushedSessionsCommon's tracking-ref compare in pushBranchIfNeeded stayed stale and every subsequent call hit the network with a redundant up-to-date push. Respect the enabled flag: when PushURL reports that no dedicated checkpoint_remote routing is in play, return the remote name so git uses its own config and updates the tracking ref. The checkpoint remote case (enabled=true) still returns the derived URL so pushes route to the separate repo as intended. Token-based SSH→HTTPS coercion remains handled by newCommand. Entire-Checkpoint: b43b0adcead2
gtrrz-victor
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

https://entire.io/gh/entireio/cli/trails/2c852be685c4
Note
Medium Risk
Touches git fetch/push/ls-remote execution and auth header injection paths (including protocol/URL resolution), which can break checkpoint sync and resume flows if incorrect. Risk is mitigated by substantial new unit + integration coverage, but behavior changes around filtered fetches and HTTPS/SSH handling need careful review.
Overview
Refactors checkpoint git CLI execution into
checkpoint/remote/git.go, replacing the old strategy-level token wrapper with higher-levelremote.Fetch/remote.Push/remote.LsRemotehelpers that always disable terminal prompts and optionally injectENTIRE_CHECKPOINT_TOKENviahttp.extraHeaderfor HTTPS remotes (with validation and one-time SSH warning).Adjusts fetch behavior to better handle partial clones: metadata and resume-related fetches now explicitly suppress
--filter=blob:nonewhen blob content is required, andresume --forcenow skips the interactive “fetch from remote?” prompt. URL parsing/derivation is updated to preserve non-standard ports and generate correct checkpoint URLs for SSH-with-port cases.Adds significant new coverage: unit tests for command/target extraction and push target resolution, integration tests with an in-process HTTPS git server validating authenticated push/fetch routing (including separate checkpoint remotes), plus a new integration test ensuring
resumefetches full blobs even whenfiltered_fetchesis enabled.Reviewed by Cursor Bugbot for commit b3ac65b. Configure here.