Fix run watch with short SHA - #1518
Open
schurchleycci wants to merge 6 commits into
Open
Conversation
schurchleycci
marked this pull request as draft
June 26, 2026 19:41
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- ExpandSHA now returns (string, error) with distinct sentinel errors (ErrSHANotHex, ErrSHARepoInaccessible, ErrSHANotFound) so callers can give accurate messages for each failure mode - Hex validation guards against branch names / tags resolving silently - watch.go emits a specific error per case instead of one catch-all - --project suggestion no longer mentions --branch when --sha is set - Acceptance tests updated to use full 40-char SHAs, restoring the original intent of testing the API-level paths - Unit tests added for all five ExpandSHA branches Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
schurchleycci
force-pushed
the
fix/run-watch-sha
branch
from
July 28, 2026 14:42
36b8ec7 to
6f14504
Compare
- Clarify the ErrSHANotFound secondary suggestion to make the shallow-clone use case explicit rather than presenting it as general advice - Add acceptance test for short SHA passed outside a git repo, covering the ErrSHARepoInaccessible error message and exit code end-to-end Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
schurchleycci
marked this pull request as ready for review
July 28, 2026 14:52
CLIError.Format only writes Message to stderr; Title is exposed via --json output. The assertion looked for the title text, which can never appear on stderr, so the test failed on all three platforms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gcirclest
approved these changes
Jul 28, 2026
danmux
reviewed
Jul 28, 2026
| if needsGit { | ||
| info, err := gitremote.Detect() | ||
| if err != nil { | ||
| return cmdutil.GitDetectErr(err, "Or specify --project and --branch explicitly") |
Contributor
There was a problem hiding this comment.
Could the short-SHA problem be fixed more simply by doing a client-side prefix match on the SHA returned by the API, rather than expanding locally via go-git? Something like strings.HasPrefix(pipeline.GitRevision, sha) instead of a server-side exact filter. That would remove the local git repo dependency entirely — no new error sentinels, no os.Chdir in tests, works in shallow clones and CI environments without access to the repo.
Contributor
Author
There was a problem hiding this comment.
Possibly! If I'm understanding correctly, the drawback of that approach would be if the run you're looking for isn't in the first page of runs returned, the client side check would return with nothing even though the run does exist (unless we keep paging, which doesn't seem great). I can keep poking at this if you feel strongly about the approach!
Regardless, I did a bit of cleanup on the error sentinels (removing ErrSHANotHex) and the tests (removing the need for os.Chdir).
…chdir Move the hex check out of gitremote and into `run watch`. A non-hex --sha is a bad-argument error rather than a git failure, so ErrSHANotHex is gone and the check runs before the project lookup — a malformed --sha now costs no API call. Add ExpandSHAIn, which takes an explicit starting directory, so the tests point at temporary checkouts instead of mutating the process working directory with os.Chdir. ExpandSHA delegates to it via the working directory. Every subtest can now run in parallel. Close the repository handle in ExpandSHAIn. openRepo's contract requires it and both other callers already did; without it the tests leave handles open on temp repos they then delete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
3 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.
Summary
run watch --shafailed when passed a short SHA because the API filter uses strict equality onpipeline.git.revision, which stores the full 40-character SHAgitremote.ExpandSHAwhich resolves an abbreviated SHA to its full form via go-git before constructing the filter; already-full 40-character SHAs pass through unchangedbranchwas being populated from git even when--shawas provided, which was unnecessaryTest plan
git push && circleci run watch --sha $(git rev-parse --short HEAD)resolves and watches correctlycircleci run watch --sha $(git rev-parse HEAD)(full SHA) still works — skips expansioncircleci run watch --sha <bogus>fails immediately with "commit not found" rather than waiting 2 minutes🤖 Generated with Claude Code