Repository navigation
fix: prefer GitHub PR payload base SHA - #2285
Closed
Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Closed
Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Conversation
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
April 4, 2026 21:27
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPrefer PR payload Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions (event)
participant Resolver as ResolveBase
participant Git as Local Git
participant Remote as origin
GH->>Resolver: deliver event payload (pull_request / pull_request_target)
Resolver->>Resolver: extract pull_request.base.sha
alt payload.base.sha present
Resolver-->>GH: return BaseResolution{SHA: payload.base.sha, TargetBranch: base}
else payload.base.sha missing
Resolver->>Git: merge-base(HEAD, origin/<base>)
alt merge-base succeeds
Resolver-->>GH: return BaseResolution{SHA: merge-base, TargetBranch: base}
else merge-base fails
Resolver->>Resolver: record merge-base error
Resolver->>Git: EnsureSafeDirectory()
Resolver->>Remote: FetchRef(repoDir, <base>)
Remote-->>Resolver: fetch result
alt fetch succeeded
Resolver->>Git: retry merge-base(HEAD, origin/<base>)
alt retry succeeds
Resolver-->>GH: return BaseResolution{SHA: merge-base, TargetBranch: base}
else
alt action == "closed"
Resolver->>Git: parentCommitResolver (HEAD~1)
Resolver-->>GH: return BaseResolution{SHA: HEAD~1, TargetBranch: base}
else
Resolver-->>GH: return BaseResolution{Ref: refs/remotes/origin/<base>, TargetBranch: base}
end
end
else fetch failed
Resolver-->>GH: return BaseResolution{Ref: refs/remotes/origin/<base>, TargetBranch: base}
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ci/providers/github/base.go (1)
102-106: KeepSourcestable by not embedding raw error text.Appending
mergeBaseErr.Error()toSourcecan make output noisy and environment-specific. Prefer a stable label and keep detailed error text in logs.Suggested tweak.
if baseSHA != "" { source := sourcePRBaseSHA if mergeBaseErr != nil { - source += " (merge-base unavailable: " + mergeBaseErr.Error() + ")" + source += " (merge-base unavailable)" } return &provider.BaseResolution{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ci/providers/github/base.go` around lines 102 - 106, The code appends mergeBaseErr.Error() into the computed Source (when baseSHA != "" using sourcePRBaseSHA), making Source unstable; instead leave Source stable by appending a fixed label like " (merge-base unavailable)" or similar, and send the full mergeBaseErr to the component logger (e.g., log the error via the existing logger used in this package) so detailed error text is preserved in logs but not embedded in Source. Update the block building Source (referencing baseSHA, sourcePRBaseSHA, mergeBaseErr, and Source) to use a stable label and add a separate logging call that records mergeBaseErr.Error().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ci/providers/github/base.go`:
- Around line 102-106: The code appends mergeBaseErr.Error() into the computed
Source (when baseSHA != "" using sourcePRBaseSHA), making Source unstable;
instead leave Source stable by appending a fixed label like " (merge-base
unavailable)" or similar, and send the full mergeBaseErr to the component logger
(e.g., log the error via the existing logger used in this package) so detailed
error text is preserved in logs but not embedded in Source. Update the block
building Source (referencing baseSHA, sourcePRBaseSHA, mergeBaseErr, and Source)
to use a stable label and add a separate logging call that records
mergeBaseErr.Error().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d261a7b-2330-40fc-af10-ac50e778b87a
📒 Files selected for processing (4)
internal/exec/describe_affected_test.gopkg/ci/providers/github/base.gopkg/ci/providers/github/base_test.gowebsite/docs/cli/commands/describe/describe-affected.mdx
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
When Atmos Pro returns HTTP 403 during `describe affected --upload`, wrap the error with the error builder pattern and surface hints about OIDC permissions and repository configuration. Also includes CI provider improvements: auto-fetch target branch when merge-base fails, safe.directory handling, and TargetBranch propagation through ExecuteDescribeAffectedWithTargetRefCheckout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Branch Add tests for FetchRef validation, EnsureSafeDirectory, and CI provider auto-fetch retry logic. Fix terraform affected test mocks to match updated ExecuteDescribeAffectedWithTargetRefCheckout signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@internal/exec/describe_affected_helpers.go`:
- Around line 234-241: The fetch failure is being swallowed: when
g.FetchRef(localRepoInfo.LocalWorktreePath, targetBranch) returns an error
(fetchErr) you only log it at Debug level and return the original CreateWorktree
error (err), losing the real cause; update the branch where fetchErr != nil to
log at Error (including fetchErr) and return a wrapped/combined error that
includes fetchErr (or replace err with fmt.Errorf("fetch failed: %w", fetchErr))
so callers see the actual FetchRef failure; touch the FetchRef/ CreateWorktree
error path around variables fetchErr, err, worktreePath and use error wrapping
to preserve both contexts.
In `@pkg/git/fetch.go`:
- Around line 16-27: The current branch validation using the validBranchName
regexp in FetchRef is too strict/incorrect for Git rules; remove or bypass the
regexp check and instead validate the branch by invoking git check-ref-format
--branch inside FetchRef (e.g., run exec.CommandContext with repoDir as working
dir or use absolute git call) and treat a non-zero exit as ErrInvalidBranchName;
ensure you preserve the existing error wrapping (return fmt.Errorf("%w: %q",
ErrInvalidBranchName, branch)) and keep the perf.Track defer, and handle command
errors and context/timeout appropriately so only branches accepted by Git pass
validation.
In `@pkg/git/safe_directory.go`:
- Around line 35-36: The current return uses a raw dynamic error
(fmt.Errorf("setting safe.directory for %s: %w\n%s", workspace, err,
string(output))) — replace this with a wrapped, static error from
errors/errors.go (e.g., the git/config related static error) so failures are
classifiable; keep workspace and output as string context by building a
contextual fmt.Errorf for the message and then combine or wrap the underlying
err with the static error using errors.Join or %w as appropriate (update the
code in the safe_directory helper where that fmt.Errorf is used to return
errors.Join(staticGitConfigError, fmt.Errorf("setting safe.directory for %s: %w
-- output: %s", workspace, err, string(output)))).
🪄 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: dad45f93-975b-446e-8b33-bb93bfd2fa66
📒 Files selected for processing (11)
internal/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.gopkg/ci/internal/provider/types.gopkg/ci/providers/github/base.gopkg/git/fetch.gopkg/git/safe_directory.gopkg/list/list_affected.go
✅ Files skipped from review due to trivial changes (1)
- pkg/ci/internal/provider/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ci/providers/github/base.go
- internal/exec/describe_affected_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/terraform_affected_test.go (1)
829-841:⚠️ Potential issue | 🔴 CriticalFix benchmark mock signature to match production function.
The benchmark mock at lines 829–841 uses the old 8-parameter signature, but the production function at
internal/exec/terraform_utils.go:350expects 4 parameters:(info *schema.ConfigAndStacksInfo, affectedList []schema.Affected, params *affectedDepOrderParams, args *DescribeAffectedCmdArgs). When gomonkey attempts to patch with a mismatched signature, it panics immediately—the benchmark will fail during execution. Update the mock to accept the struct-basedparamsinstead of individual parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_affected_test.go` around lines 829 - 841, The benchmark mock for executeTerraformAffectedComponentInDepOrder uses the old 8-parameter signature; update the gomonkey.ApplyFunc replacement to match the real function signature: func(info *schema.ConfigAndStacksInfo, affectedList []schema.Affected, params *affectedDepOrderParams, args *DescribeAffectedCmdArgs) error, and use params to access the previously separate values (parentComponent, parentStack, dependents) if needed, returning nil as before so the patched function matches production and avoids the gomonkey panic.
🧹 Nitpick comments (2)
internal/exec/terraform_affected_test.go (2)
196-204: Same typos in the error test case mock.Consistent with the earlier mock but same readability issue.
✏️ Suggested fix
- tttttttargetBranch string, + targetBranch string, includeSpaceliftAdminStacks bool, includeSettings bool, stack string, processTemplates bool, processYamlFunctions bool, skip []string, excludeLocked bool, - ttttttauthManager auth.AuthManager, + authManager auth.AuthManager,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_affected_test.go` around lines 196 - 204, The test mock function signature contains leftover typos 'tttttttargetBranch' and 'ttttttauthManager' which reduce readability; rename these parameters to 'targetBranch string' and 'authManager auth.AuthManager' in the mock declaration and update all usages inside the test (the mock function passed into the test harness and any callers) to use the corrected parameter names so the signature matches the earlier mock and improves consistency.
107-115: Parameter name typos in mock signature.The mock correctly matches the production signature by type and order, but the parameter names have extra
tcharacters:tttttttargetBranchandttttttauthManager. While gomonkey ignores parameter names, this hurts readability.✏️ Suggested fix
- tttttttargetBranch string, + targetBranch string, includeSpaceliftAdminStacks bool, includeSettings bool, stack string, processTemplates bool, processYamlFunctions bool, skip []string, excludeLocked bool, - ttttttauthManager auth.AuthManager, + authManager auth.AuthManager,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_affected_test.go` around lines 107 - 115, The mock signature in internal/exec/terraform_affected_test.go has parameter name typos—rename the parameters `tttttttargetBranch` to `targetBranch` and `ttttttauthManager` to `authManager` in the mock function declaration so the names match the production signature (types/order already correct); update any references inside the mock body to use the corrected `targetBranch` and `authManager` identifiers to improve readability (gomonkey will still work since it ignores names).
🤖 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 54-66: The test hardcodes "master" when switching back from the
new branch which can fail if git's default branch is different; modify the test
to read the repo's default branch and use that instead of the literal "master":
call runGit(t, originDir, "symbolic-ref", "refs/remotes/origin/HEAD") or parse
the output of runGit invoked with "rev-parse", or runGit(t, originDir, "branch",
"--show-current") after initial commit to capture the default branch name into a
variable (e.g., defaultBranch) and then use runGit(t, originDir, "checkout",
defaultBranch) instead of "master"; also add "strings" to the imports as
suggested.
In `@pkg/git/safe_directory_test.go`:
- Around line 17-24: The test TestEnsureSafeDirectory_ConfiguresSafeDirectory
currently only asserts EnsureSafeDirectory() returns no error but doesn't verify
git global config was mutated; update the test to sandbox git's global config by
setting HOME and XDG_CONFIG_HOME (and USERPROFILE on Windows) to a fresh
t.TempDir(), call EnsureSafeDirectory(), then run git config --global --get-all
safe.directory (via exec.Command) and assert the expected tmpDir value is
present; reference EnsureSafeDirectory and the test
TestEnsureSafeDirectory_ConfiguresSafeDirectory when locating where to add the
environment setup and the git config read/assertion.
---
Outside diff comments:
In `@internal/exec/terraform_affected_test.go`:
- Around line 829-841: The benchmark mock for
executeTerraformAffectedComponentInDepOrder uses the old 8-parameter signature;
update the gomonkey.ApplyFunc replacement to match the real function signature:
func(info *schema.ConfigAndStacksInfo, affectedList []schema.Affected, params
*affectedDepOrderParams, args *DescribeAffectedCmdArgs) error, and use params to
access the previously separate values (parentComponent, parentStack, dependents)
if needed, returning nil as before so the patched function matches production
and avoids the gomonkey panic.
---
Nitpick comments:
In `@internal/exec/terraform_affected_test.go`:
- Around line 196-204: The test mock function signature contains leftover typos
'tttttttargetBranch' and 'ttttttauthManager' which reduce readability; rename
these parameters to 'targetBranch string' and 'authManager auth.AuthManager' in
the mock declaration and update all usages inside the test (the mock function
passed into the test harness and any callers) to use the corrected parameter
names so the signature matches the earlier mock and improves consistency.
- Around line 107-115: The mock signature in
internal/exec/terraform_affected_test.go has parameter name typos—rename the
parameters `tttttttargetBranch` to `targetBranch` and `ttttttauthManager` to
`authManager` in the mock function declaration so the names match the production
signature (types/order already correct); update any references inside the mock
body to use the corrected `targetBranch` and `authManager` identifiers to
improve readability (gomonkey will still work since it ignores names).
🪄 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: 304b6528-f048-409f-bea6-d284828296e3
📒 Files selected for processing (5)
internal/exec/terraform_affected_test.gopkg/ci/providers/github/base_test.gopkg/git/fetch.gopkg/git/fetch_test.gopkg/git/safe_directory_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/git/fetch.go
- pkg/ci/providers/github/base_test.go
Merge origin/main into osterman/gha-pr-base-sha, combining the PR payload base SHA with head SHA and CIEventType assertions from main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/describe_affected.go (1)
331-335:⚠️ Potential issue | 🟠 MajorGate the retry on missing-commit/ref errors only, not all CreateWorktree failures.
Currently the retry at lines 234-242 in
describe_affected_helpers.gotriggers on any CreateWorktree error. Since CreateWorktree wraps all errors (temp-dir creation, git command failures, missing refs) asErrGitRefNotFound, the retry logic can't distinguish between a genuinely missing ref and unrelated failures like permission or temp-path issues. This causes unnecessary fetch and retry attempts for problems that won't be fixed by fetching.Check the CreateWorktree error message or output to confirm it's a missing-commit/ref case before attempting the fetch and retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/describe_affected.go`:
- Around line 452-465: The non-403 upload error path should also return a
wrapped static error instead of bubbling raw; update the UploadAffectedStacks
error handling so that after detecting and handling the 403 case, any other
uploadErr is returned via
errUtils.Build(errUtils.ErrFailedToUploadStacks).WithCause(uploadErr).Err()
(keeping any relevant hints if desired) instead of returning uploadErr directly;
locate the apiClient.UploadAffectedStacks call and replace the final "return
uploadErr" with the wrapped errUtils.Build(...) return to ensure callers receive
the stable ErrFailedToUploadStacks sentinel.
🪄 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: 7fd292d7-ade0-4ccf-938e-ec31f54ec044
📒 Files selected for processing (2)
internal/exec/describe_affected.gointernal/exec/describe_affected_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/describe_affected_test.go
…, and tests - Join fetch error with worktree error so failures propagate to callers - Wrap non-403 upload errors with ErrFailedToUploadStacks sentinel - Replace regex branch validation with git check-ref-format for correctness - Wrap safe.directory error in static ErrGitCommandFailed error - Detect default branch dynamically in tests instead of hardcoding "master" - Sandbox git global config in safe_directory test and assert configured value Co-Authored-By: Claude Opus 4.6 <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.go`:
- Around line 15-22: Change isValidBranchName to return (bool, error) and update
its callers (e.g., FetchRef) to handle and propagate real git execution errors:
run cmd.Run(), if err==nil return (true, nil); if err is an *exec.ExitError
return (false, nil) to indicate an invalid branch name; otherwise return (false,
err) so callers can distinguish git failures (missing binary, permissions, etc.)
from validation rejection and surface the proper error up the stack.
🪄 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: e4b069a7-2070-449e-b16d-1aac164c9f2d
📒 Files selected for processing (6)
internal/exec/describe_affected.gointernal/exec/describe_affected_helpers.gopkg/git/fetch.gopkg/git/fetch_test.gopkg/git/safe_directory.gopkg/git/safe_directory_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/git/fetch_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/git/safe_directory_test.go
- pkg/git/safe_directory.go
- internal/exec/describe_affected_helpers.go
…validateBranchName Rename isValidBranchName to validateBranchName, returning error instead of bool. Now distinguishes *exec.ExitError (invalid branch name) from other errors (missing git binary, permissions) so callers see the actual failure cause. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2285 +/- ##
==========================================
+ Coverage 76.99% 77.03% +0.04%
==========================================
Files 1059 1061 +2
Lines 100601 100711 +110
==========================================
+ Hits 77453 77579 +126
+ Misses 18852 18839 -13
+ Partials 4296 4293 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Member
Author
|
Closing in favor of #2380, which keeps Why supersede rather than land this PR as-is:
The customer-reported regression is fixed in #2380 with the same shallow-checkout coverage you had here, plus a stronger test bar (the new The improved upload-error hints from this PR are unrelated to base resolution — happy to land them in a small standalone PR if you want. Thanks for the original work — it pointed straight at the right code paths. |
Andriy Knysh (aknysh)
added a commit
that referenced
this pull request
May 1, 2026
…lone self-heal (#2380) * fix(describe-affected): resolve PR base via merge-base with shallow-clone 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> * fix(describe-affected): trust GitHub Actions workspace before git ops 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> * fix(describe-affected): only call EnsureGitSafeDirectory when ci.enabled 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> * refactor(git): wrap fetch/deepen errors with static sentinels; fix doc 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> * test(git): cover DeepenFetch failure path with ErrDeepenOrigin sentinel 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> * test(git): cover unshallow and deepen branches in fetch/merge_base 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> * fix(git): MergeBase honors repoDir; tighten test sentinels and Windows 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> * refactor(git): extract worktree-with-fetch-recovery helper, cover with 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> * fix(git): gate worktree self-heal to ErrGitRefNotFound; pin exec contracts 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> * [autocommit] formatting fixes * fix(git): narrow CreateWorktree ref-not-found classification 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> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: atmos-pro[bot] <atmos-pro[bot]@users.noreply.github.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
This branch was successfully deployed
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
1 participant
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
pull_request.base.shafrom the GitHub Actions event payload when resolving the base foratmos describe affectedmerge-base,HEAD~1, and ref-based resolution as fallbacks, and surface clearer source details when local refs are unavailabledescribe affecteddocs to describe the new GitHub Actions precedencewhy
origin/<base>refs even withactions/checkoutandfetch-depth: 0--clone-target-ref=trueor explicitgit fetchjust to compute affected stacksreferences
atmos describe affectedfor GitHub Actions PR job-container workflowsSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests