fix(describe-affected): resolve PR base via merge-base with shallow-clone self-heal#2380
Merged
fix(describe-affected): resolve PR base via merge-base with shallow-clone self-heal#2380
Conversation
…lone self-heal; drop buggy origin-tip fallback When `ci.enabled: true` and the PR was not up to date with the target branch, `atmos describe affected` could report many more affected components than the PR actually modified. Root cause: when `MergeBase(HEAD, origin/<target>)` failed (shallow CI checkouts where `origin/<target>` is not fetched), the GitHub provider fell through to returning the *ref* `refs/remotes/origin/<target>`, which downstream resolved to the *current tip* of the target branch. The resulting tree-to-tree diff included every commit on `<target>` that the PR hadn't pulled in. Fix: - `pkg/git/merge_base.go` adds `MergeBaseWithAutoFetch` that transparently runs `git fetch origin <target>` (and optionally one `--deepen=200` fetch) when `MergeBase` cannot resolve a fork point. Bounded — at most one fetch and one deepen per call. - `pkg/ci/providers/github/base.go` uses `MergeBaseWithAutoFetch` and changes the post-fallback chain. The new last-resort is `event.pull_request.base.sha` (frozen at last PR sync, never the current tip). The legacy `refs/remotes/origin/<target>` ref path is kept only for hand-crafted payloads with no `base.sha`, with a `log.Warn` explaining it may include unrelated commits. - `ExecuteDescribeAffectedWithTargetRefCheckout` accepts a new `targetBranch` parameter; on worktree creation failure it runs `git fetch origin <targetBranch>` and retries once. Threaded through `DescribeAffectedCmdArgs` and all callers. - `BaseResolution` gains a `TargetBranch` field so providers can surface the target branch alongside the resolved base. - Adds `pkg/git/fetch.go` (`FetchRef`, `DeepenFetch`) lifted from the open PR #2285 (which this PR supersedes — see the docs/fixes entry). Tests: - `pkg/git/merge_base_test.go` — new `TestMergeBaseWithAutoFetch_RecoversFromMissingRef` builds an origin/clone pair, deletes `origin/main` to simulate the shallow case, and asserts the recovered SHA is the fork point. Also covers `ErrHeadOnTargetBranch` propagation and the no-recovery path. Existing `MergeBase` tests migrated to `t.Chdir`. - `pkg/ci/providers/github/base_test.go` — adds `TestResolveBase_PullRequest_OutOfDate_FallsBackToPayloadSHA` that reproduces the customer scenario at unit-test level and asserts we end up with a SHA, not the buggy origin-tip ref. - `internal/exec/describe_affected_test.go:TestResolveBaseFromCI` hardened to require `describe.SHA` is populated and `describe.Ref` empty — guards against any future regression. Docs: - `docs/fixes/2026-04-30-describe-affected-out-of-date-pr.md` documents the issue, root cause, fix, and the rejected `pull_request.merge_commit_sha` alternative the user suggested. - PRD `docs/prd/native-ci/framework/base-resolution.md`: removed "handles this gracefully" language; updated resolution matrix. - Blog post and command docs updated to reflect that merge-base is the actual primary strategy (the previous text predated PR #2241's merge-base implementation). Refs: #2241, supersedes #2285. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThreads PR targetBranch through describe-affected, adds git FetchRef/DeepenFetch and MergeBaseWithAutoFetch to recover shallow/out-of-date PR checkouts, updates GitHub base-resolution order to prefer merge-base-with-auto-fetch then event payload base SHA, and updates callsites, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Provider
participant MergeBase as MergeBaseWithAutoFetch
participant Fetch as FetchRef/DeepenFetch
participant Repo as Local Git Repo
participant Exec as Describe-Affected Executor
GH->>MergeBase: resolve merge-base(repoDir, targetBranch)
MergeBase->>Repo: git merge-base HEAD refs/remotes/origin/<target>
Repo-->>MergeBase: success / ErrReferenceNotFound / ErrNoCommonAncestor / ErrHeadOnTargetBranch
alt ErrReferenceNotFound
MergeBase->>Fetch: FetchRef(repoDir, targetBranch)
Fetch->>Repo: git fetch origin refs/heads/<target>:refs/remotes/origin/<target> --no-tags
Repo-->>Fetch: success / failure
MergeBase->>Repo: retry git merge-base
else ErrNoCommonAncestor
MergeBase->>Fetch: DeepenFetch(repoDir, targetBranch, deepenStep)
Fetch->>Repo: git fetch --deepen / --unshallow
Repo-->>Fetch: success / failure
MergeBase->>Repo: retry git merge-base
end
MergeBase-->>GH: return resolved SHA (or empty + ref)
GH-->>Exec: BaseResolution{SHA, TargetBranch}
Exec->>Exec: ExecuteDescribeAffectedWithTargetRefCheckout(..., targetBranch)
Exec->>Repo: CreateWorktree(targetCommit)
Repo-->>Exec: success / ErrGitRefNotFound
alt ErrGitRefNotFound and targetBranch present
Exec->>Fetch: FetchRef(repoDir, targetBranch) (one-shot)
Fetch->>Repo: git fetch origin refs/heads/<target>
Repo-->>Fetch: success / failure
Exec->>Repo: retry create worktree -> success or joined error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 13 minutes and 32 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-04-30-describe-affected-out-of-date-pr.md`:
- Around line 56-60: The fenced diagram block lacks a language hint; update the
opening fence from ``` to ```text so the block becomes ```text ... ``` (e.g.,
change the diagram block in the file to start with ```text and end with ```),
ensuring markdown tooling/linting recognizes it as plain text.
In `@docs/prd/native-ci/framework/base-resolution.md`:
- Around line 125-127: Update the two Validation bullets that still describe the
old fallback order so they match the matrix and new fallback chain: ensure they
list event.pull_request.base.sha (and MergeBaseWithAutoFetch use of base.sha) as
the primary source before any ref fallback, then the GITHUB_BASE_REF ref (and
any refs/remotes/origin/main fallback) afterward; adjust text to reference
MergeBaseWithAutoFetch(HEAD, origin/<target>), event.pull_request.base.sha, and
GITHUB_BASE_REF in that order and remove the previous
direct-GITHUB_BASE_REF-first wording.
In `@pkg/git/fetch.go`:
- Around line 49-50: Replace the ad-hoc formatted error returns in
pkg/git/fetch.go (the returns that currently use fmt.Errorf("fetching origin/%s:
%w\n%s", branch, err, string(output)) and the similar deepen-failure returns
around lines 84–85) so they wrap the original error with the project’s shared
static errors from errors/errors.go (for example the fetch-related and
deepen-related static errors) using fmt.Errorf and %w as the primary wrapped
error; keep the original err as additional context wrapped with %w and include
the command output as a string suffix in the message. Ensure you reference the
same static symbols from the errors package (e.g., the fetch/deepen error
constants) when wrapping and apply the same change to the other deepen/fetch
return paths in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b53bdf0e-ee8b-41d3-815c-159c7b3fd709
📒 Files selected for processing (19)
docs/fixes/2026-04-30-describe-affected-out-of-date-pr.mddocs/prd/native-ci/framework/base-resolution.mdinternal/exec/atlantis_generate_repo_config.gointernal/exec/describe_affected.gointernal/exec/describe_affected_helpers.gointernal/exec/describe_affected_test.gointernal/exec/terraform_affected.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_affected_test.gopkg/ci/internal/provider/types.gopkg/ci/providers/github/base.gopkg/ci/providers/github/base_test.gopkg/git/fetch.gopkg/git/fetch_test.gopkg/git/merge_base.gopkg/git/merge_base_test.gopkg/list/list_affected.gowebsite/blog/2026-03-21-describe-affected-auto-detection.mdxwebsite/docs/cli/commands/describe/describe-affected.mdx
Container jobs in GitHub Actions run as a different user than the checkout owner. Without `safe.directory` set, every git command (including the auto-fetch we just added in MergeBaseWithAutoFetch and the worktree-creation retry) fails with "fatal: detected dubious ownership in repository at '/github/workspace'". Call git.EnsureGitSafeDirectory() at the top of ResolveBase() so all git operations the GitHub provider triggers (merge-base, fetch, parent-commit lookup) succeed in container runners. The function is a no-op outside GitHub Actions so it's safe to call unconditionally. Also: hardened the two new fallback tests to use a non-existent target branch name. They previously assumed merge-base would fail in the test environment, which broke when running locally inside a real clone of the atmos repo (the host repo *has* a `main` branch and merge-base succeeded). Using `nonexistent-target-for-pr2380` makes the assertions deterministic regardless of host state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…led is true EnsureGitSafeDirectory mutates the runner's global git config (adds GITHUB_WORKSPACE to safe.directory). It should only run when the user has explicitly opted into CI auto-detect via ci.enabled: true. Move the call from pkg/ci/providers/github/base.go:ResolveBase() (unconditional) to internal/exec/describe_affected.go:resolveBaseFromCI() (only invoked when ci.enabled is true). The provider-layer doc-comment notes that callers are responsible for safe-directory setup. EnsureGitSafeDirectory itself remains a no-op outside GitHub Actions, so the practical effect is: with this commit, Atmos only writes to ~/.gitconfig when both ci.enabled is true AND we're running inside GitHub Actions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2380 +/- ##
==========================================
+ Coverage 77.95% 77.99% +0.04%
==========================================
Files 1090 1091 +1
Lines 103078 103206 +128
==========================================
+ Hits 80355 80499 +144
+ Misses 18316 18298 -18
- Partials 4407 4409 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…c fallback chain Address PR review feedback: - Add ErrFetchOrigin / ErrDeepenOrigin sentinels in errors/errors.go. - Wrap pkg/git/fetch.go FetchRef and DeepenFetch errors with the new sentinels (double %w preserves both sentinel and underlying err for errors.Is checks). - Update TestFetchRef_NonexistentBranch to assert errors.Is(err, ErrFetchOrigin) instead of matching the old message string. - Fix base-resolution PRD Validation bullets to match the documented fallback chain (MergeBaseWithAutoFetch -> base.sha -> ref-with-warn). - Add `text` language hint to fenced diagram block in fixes doc to satisfy markdownlint MD040. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/git/fetch_test.go`:
- Around line 119-123: Add a test that triggers the actual git fetch failure
path so the DeepenFetch error wrapping (ErrDeepenOrigin) is exercised: call
DeepenFetch with a syntactically valid branch name (so it doesn't return
ErrInvalidBranchName) against a repo setup that causes a real fetch to fail
(e.g., a temp directory that is not a git remote or a repo with no origin), then
assert require.Error(t, err) and assert.ErrorIs(t, err, ErrDeepenOrigin);
reference DeepenFetch and ErrDeepenOrigin in the new test to ensure the
deepen-path sentinel is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de229022-fcfb-4cfb-87f6-2a495234a4a3
📒 Files selected for processing (5)
docs/fixes/2026-04-30-describe-affected-out-of-date-pr.mddocs/prd/native-ci/framework/base-resolution.mderrors/errors.gopkg/git/fetch.gopkg/git/fetch_test.go
✅ Files skipped from review due to trivial changes (1)
- errors/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/git/fetch.go
Mirrors TestFetchRef_NonexistentBranch: clones a temp origin and asks DeepenFetch to fetch a branch that does not exist there, asserting the error chain wraps errUtils.ErrDeepenOrigin so a regression in the sentinel wrapping cannot slip through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bring pkg/git coverage from 75.x% to 80.3% by exercising two previously untested paths: - TestDeepenFetch_Unshallow: shallow-clones a multi-commit origin (depth=1), then calls DeepenFetch(repo, branch, 0) to take the depth<=0 --unshallow branch and asserts the .git/shallow marker is removed afterwards. Brings DeepenFetch from 80% -> 100% line coverage. - TestMergeBaseWithAutoFetch_DeepenPathExhausted: uses an orphan branch to deterministically produce ErrNoCommonAncestor (rather than the shallow-clone "object not found" path, which the auto-fetch logic does not classify as deepen-recoverable). Exercises the deepen branch end to end and asserts the function propagates the error when deepen cannot recover. Brings MergeBaseWithAutoFetch from 53.6% -> 71.4%. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/git/fetch_test.go`:
- Around line 160-162: Replace the string-concatenated file URI with a properly
built file:// URL using net/url and filepath to be Windows-safe: create a
url.URL with Scheme "file" and Path set to the OS-normalized absolute originDir
(use filepath.ToSlash or ensure a leading slash on Windows), call u.String() and
pass that to runGit instead of "file://"+originDir; update imports (net/url,
path/filepath and runtime/strings if needed) and modify the test in
fetch_test.go where clone is invoked (the cloneDir / runGit call).
In `@pkg/git/merge_base_test.go`:
- Around line 173-179: Update the test to assert the specific propagated error
instead of any error: when calling MergeBaseWithAutoFetch (the symbol to
modify), replace the generic require.Error check with an assertion that uses
errors.Is(err, ErrNoCommonAncestor) (or equivalent require.True with errors.Is)
so the test fails if a different error is returned; reference the
ErrNoCommonAncestor sentinel and ensure the errors package is imported in
pkg/git/merge_base_test.go.
- Around line 197-199: Update the test to assert the specific original
ref-missing error instead of only checking for any error: in the test that calls
MergeBaseWithAutoFetch(cloneDir, "release") replace the generic assert.Error(t,
err) with assert.ErrorIs(t, err, plumbing.ErrReferenceNotFound) (or use
errors.Is(err, plumbing.ErrReferenceNotFound) if not using testify) so the test
guarantees the fallback returns the original MergeBase failure when the branch
is absent; keep the call to MergeBaseWithAutoFetch and the setup unchanged.
- Around line 107-115: The test shows MergeBaseWithAutoFetch(cloneDir, "main")
still calls MergeBase("main") which resolves the repo from '.' and misses the
intended repoDir; update MergeBaseWithAutoFetch to call into a repo-aware
variant of MergeBase (e.g., pass repoDir through) or modify MergeBase to accept
and use an explicit repoDir parameter so both the initial call and the retry
after fetch operate on the same repo; ensure references to MergeBase and
MergeBaseWithAutoFetch in merge_base.go use the repoDir value rather than
relying on working directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe290f74-414f-4811-b220-b1cb9aa720bc
📒 Files selected for processing (2)
pkg/git/fetch_test.gopkg/git/merge_base_test.go
…s file URI Address PR review feedback: - MergeBase now takes (repoDir, targetBranch) and opens the repo at repoDir directly via go-git PlainOpenWithOptions, instead of relying on the cwd-scoped GetLocalRepo() helper. MergeBaseWithAutoFetch threads its repoDir argument into all three MergeBase calls so the initial computation, post-fetch retry, and post-deepen retry all operate on the same repository. Previously MergeBaseWithAutoFetch's repoDir parameter was used only for FetchRef/DeepenFetch, leaving the merge-base steps silently dependent on cwd. - Tests no longer t.Chdir into the test repo before exercising MergeBase / MergeBaseWithAutoFetch — the explicit repoDir argument is sufficient and removing chdir actually verifies the new contract. - Tighten error assertions: * TestMergeBase_ErrorWhenTargetRefMissing: errors.Is plumbing.ErrReferenceNotFound * TestMergeBaseWithAutoFetch_DeepenPathExhausted: errors.Is ErrNoCommonAncestor * TestMergeBaseWithAutoFetch_ReturnsErrorWhenFetchImpossible: errors.Is plumbing.ErrReferenceNotFound - Windows-safe file URI in TestDeepenFetch_Unshallow — build with net/url + filepath.ToSlash instead of "file://"+originDir, which is malformed on drive-letter paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…h tests
The shallow-clone self-heal logic in
ExecuteDescribeAffectedWithTargetRefCheckout (8 lines) was hard to
unit-test because it lived inside a 350-line orchestrator that the
existing tests stub via gomonkey. Hoist it into a small package-level
helper in pkg/git so it can be exercised end-to-end with real git
operations.
- New CreateWorktreeWithFetchRecovery(repoDir, targetCommit,
targetBranch) in pkg/git/worktree.go: tries CreateWorktree, and if
that fails AND a non-empty targetBranch is supplied, performs a
one-shot FetchRef(repoDir, targetBranch) and retries. On final
failure, joins the original CreateWorktree error with the fetch
error so callers retain hints about both failures.
- describe_affected_helpers.go now calls this helper directly,
shrinking the inline self-heal block from 8 lines to 1.
- Three tests cover the three branches at 100% line coverage:
* SuccessNoFetchNeeded — first CreateWorktree succeeds; fetch
must NOT run even when targetBranch is non-empty.
* SuccessAfterFetch — clone tracks an older origin/main; a new
commit lands on origin after the clone; missing SHA becomes
available after FetchRef and the retry succeeds. (This is the
customer-reported PR base.sha scenario.)
* FailsWhenFetchFails — bogus SHA + non-existent target branch;
asserts the joined error chain is reachable via errors.Is on
ErrFetchOrigin.
Net effect: pkg/git coverage 80.3% -> 81.1%, and the
describe_affected_helpers self-heal lines that were 0% covered are
gone (replaced with one delegation line).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/git/merge_base_test.go (1)
131-170: ⚡ Quick winExercise the real shallow-boundary deepen case too.
This only proves the
ErrNoCommonAncestorbranch on unrelated histories. The production path you’re fixing is a shallow clone with shared ancestry, where the fork point exists on origin but not in the local object graph yet. Please add a--depth=1scenario that forcesMergeBaseWithAutoFetchto deepen and then return the fork-point SHA; otherwise a shallow-history regression can still slip through here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/git/merge_base_test.go` around lines 131 - 170, The test only covers unrelated histories; add a shallow-depth scenario to exercise the real deepen path: in TestMergeBaseWithAutoFetch_DeepenPathExhausted (or a small new test), create an origin with a shared fork point (make a commit on main, create a branch, add further commits), then create a local repo that is a shallow clone of origin using "--depth=1" so the fork point is missing locally, call MergeBaseWithAutoFetch(dir, "main"), and assert it returns the fork-point SHA (not ErrNoCommonAncestor); keep existing unrelated-history assertions but add this shallow-case setup and the assertion that MergeBaseWithAutoFetch successfully deepens and returns the expected commit SHA.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/git/fetch_test.go`:
- Around line 115-118: The tests that call FetchRef (and the other failing path
at 141-144) currently only assert the high-level sentinel
errUtils.ErrFetchOrigin; add a check that the underlying error preserves the
shell-command exit-code contract by using errors.As or assert.ErrorAs to confirm
the wrapped error is an errUtils.ExitCodeError (i.e., assert that errors.As(err,
new(errUtils.ExitCodeError)) or assert.ErrorAs(t, err,
&errUtils.ExitCodeError{}) for the variable err returned by FetchRef and the
other failing call).
In `@pkg/git/worktree.go`:
- Around line 75-83: The code currently retries via FetchRef for any
CreateWorktree error when targetBranch is set; change this to only attempt the
fetch when CreateWorktree failed due to a missing/ref-not-found error. In the
CreateWorktree call handling in worktree.go, replace the broad check (err == nil
|| targetBranch == "") with a guard that (1) leaves early on success, (2)
returns immediately for empty targetBranch, and (3) inspects err to detect a
ref-missing condition (e.g., match the git error string or use/add a helper like
isRefMissing(err)) and only then call FetchRef(repoDir, targetBranch); on fetch
failure still return errors.Join(err, fetchErr). Ensure you reference
CreateWorktree, FetchRef, and the error join path so only missing-ref errors
trigger the auto-fetch retry.
---
Nitpick comments:
In `@pkg/git/merge_base_test.go`:
- Around line 131-170: The test only covers unrelated histories; add a
shallow-depth scenario to exercise the real deepen path: in
TestMergeBaseWithAutoFetch_DeepenPathExhausted (or a small new test), create an
origin with a shared fork point (make a commit on main, create a branch, add
further commits), then create a local repo that is a shallow clone of origin
using "--depth=1" so the fork point is missing locally, call
MergeBaseWithAutoFetch(dir, "main"), and assert it returns the fork-point SHA
(not ErrNoCommonAncestor); keep existing unrelated-history assertions but add
this shallow-case setup and the assertion that MergeBaseWithAutoFetch
successfully deepens and returns the expected commit SHA.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 073f4bb3-a906-417b-9dc0-cd1cccda5b9a
📒 Files selected for processing (6)
internal/exec/describe_affected_helpers.gopkg/git/fetch_test.gopkg/git/merge_base.gopkg/git/merge_base_test.gopkg/git/worktree.gopkg/git/worktree_test.go
…racts in tests
Address PR review feedback:
- CreateWorktreeWithFetchRecovery now gates the fetch/retry path on
errors.Is(err, errUtils.ErrGitRefNotFound). Previously *any*
CreateWorktree failure with a non-empty targetBranch would log
"Target commit not available locally, fetching base branch" and
attempt a fetch — including unrelated infrastructure failures
(temp-dir creation, permission denied, repo state corruption). Those
errors now propagate directly.
- Negative-path test TestCreateWorktreeWithFetchRecovery_GateSkipsNonRefNotFoundError
forces os.MkdirTemp to fail (TMPDIR/TEMP/TMP point at a nonexistent
path) and asserts the gate skips the recovery path: neither
ErrFetchOrigin nor ErrGitRefNotFound appear in the returned chain.
Side-benefit: this test exercises the MkdirTemp failure branch in
CreateWorktree itself, bringing it from 92.3% to 100%.
- TestFetchRef_NonexistentBranch and TestDeepenFetch_NonexistentBranch
now also assert errors.As(&exec.ExitError{}) and a non-zero exit
code, pinning the contract that the wrapped error chain preserves
the underlying shell exit-status info. (FetchRef/DeepenFetch use
raw exec.Command rather than the workflow ShellRunner that produces
errUtils.ExitCodeError, so *exec.ExitError is the equivalent
contract here.)
Coverage: pkg/git 81.1% -> 81.6%; CreateWorktree 92.3% -> 100%;
CreateWorktreeWithFetchRecovery stays at 100%.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/git/worktree_test.go`:
- Around line 385-388: The test currently asserts the joined error chain
contains errUtils.ErrFetchOrigin but omits the original create-worktree error;
update the test in worktree_test.go (the fetch-failure recovery case where
errors.Join(originalCreateWorktreeErr, fetchErr) is returned) to also assert the
original error is preserved by adding an assertion like assert.ErrorIs(t, err,
ErrGitRefNotFound) (use the same ErrGitRefNotFound symbol used where
originalCreateWorktreeErr is defined/imported) so both ErrFetchOrigin and
ErrGitRefNotFound are verified in the joined error chain.
In `@pkg/git/worktree.go`:
- Around line 82-83: CreateWorktree currently marks every git worktree add
failure as errUtils.ErrGitRefNotFound, causing non-missing-ref errors to trigger
recovery; change CreateWorktree so it parses the git stderr and only returns
errUtils.ErrGitRefNotFound when the error text matches a genuine missing-ref
pattern (e.g., "fatal: reference .* not found" or similar), and for other
failures return a distinct sentinel error (create e.g.
errUtils.ErrWorktreeAddFailed or ErrGitWorktreeAddFailed) or wrap the original
error with that sentinel; then update the caller check in
CreateWorktree/Worktree logic (the code that currently does errors.Is(err,
errUtils.ErrGitRefNotFound)) to only treat the missing-ref sentinel as retryable
and not the generic worktree-add sentinel, ensuring other failures skip
recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6fd5e21-9bad-43b9-adbd-e1d2acf86144
📒 Files selected for processing (3)
pkg/git/fetch_test.gopkg/git/worktree.gopkg/git/worktree_test.go
Previously CreateWorktree blanket-tagged every git worktree add failure as ErrGitRefNotFound, causing CreateWorktreeWithFetchRecovery's gate to fire its fetch retry on unrelated failures (path conflicts, repo state corruption, permissions) and to surface a misleading "make sure the ref is correct" hint for non-ref problems. Add ErrGitWorktreeAdd sentinel for generic worktree-add failures and parse git stderr to reserve ErrGitRefNotFound for true missing-ref cases. The recovery gate stays on ErrGitRefNotFound, which now fires only when fetch is actually a candidate fix. Test coverage includes the new non-ref classification and a parallel gate-skip case through the recovery helper. Also pins both joined sentinels (ErrFetchOrigin and ErrGitRefNotFound) in the fetch-failure recovery test so the preserved hint context can't regress. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aknysh
approved these changes
May 1, 2026
Contributor
|
These changes were released in v1.217.0-rc.3. |
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
2 participants
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.
what
Fix
atmos describe affectedreporting many more affected componentsthan the PR actually modified, specifically when the PR is out of
date with the target branch.
pkg/git/merge_base.goaddsMergeBaseWithAutoFetchthat runs atargeted
git fetch origin <target>(and optionally one--deepen=200) whenMergeBasecan't resolve. Bounded retries.pkg/ci/providers/github/base.go:resolvePRBasekeeps merge-base asthe primary strategy and drops the buggy last-resort path that
returned
refs/remotes/origin/<target>(which downstream resolvedto the current tip of the target branch, producing the
false-positives). New last-resort is
event.pull_request.base.sha,which is frozen at the last PR sync and never points to the
current tip.
ExecuteDescribeAffectedWithTargetRefCheckoutaccepts a newtargetBranchparameter and self-heals viagit fetchwhenworktree creation hits a missing target commit.
pkg/git/fetch.go(FetchRef,DeepenFetch) lifted fromPR fix: prefer GitHub PR payload base SHA #2285. New
TargetBranchfield onBaseResolutionandDescribeAffectedCmdArgs.why
A customer reported that
atmos describe affectedon an out-of-datePR listed components the PR did not touch. The root cause was a
fallback path documented as "handles this gracefully" in the PRD
that, in practice, silently produced wrong results when the local
repo was a shallow checkout (the
actions/checkout@v4default).Walkthrough and rationale are in
docs/fixes/2026-04-30-describe-affected-out-of-date-pr.md.The user's suggestion — using
pull_request.merge_commit_shaasthe base — would also work and is documented as a considered
alternative in the fixes doc. We chose merge-base + auto-fetch
because it preserves the existing PRD architecture, doesn't require
fetching
M's parent separately, and works naturally withactions/checkout@v4's default merge-ref checkout.supersedes #2285
PR #2285 proposed promoting
pull_request.base.shato the primarystrategy. This PR keeps merge-base as primary (gold standard) and
uses
base.shaonly as a fallback that replaces the buggyref-tip path. The fetch helpers and signature plumbing are lifted
from #2285; credit to the original work.
tests
pkg/git/merge_base_test.go: newTestMergeBaseWithAutoFetch_RecoversFromMissingRefbuilds anorigin/clone pair, deletes
origin/mainto simulate a shallowCI checkout, and asserts the recovered SHA is the fork point —
not the current main tip.
pkg/ci/providers/github/base_test.go:TestResolveBase_PullRequest_OutOfDate_FallsBackToPayloadSHAreproduces the customer scenario at unit-test level.
internal/exec/describe_affected_test.go:TestResolveBaseFromCIhardened to require
describe.SHAis populated anddescribe.Refempty — guards against any future regression thatre-introduces the ref-tip fallback.
references
🤖 Generated with Claude Code
Summary by CodeRabbit