Skip to content

fix(workspace): fetch PR refs from upstream and pin applied commit (audit #3) - #257

Closed
sbalabanov wants to merge 2 commits into
mainfrom
audit/fix-03-pr-fetch-and-pinning
Closed

fix(workspace): fetch PR refs from upstream and pin applied commit (audit #3)#257
sbalabanov wants to merge 2 commits into
mainfrom
audit/fix-03-pr-fetch-and-pinning

Conversation

@sbalabanov

Copy link
Copy Markdown
Contributor

Summary

Workers cloned with --local had their origin pointing at the pool's local directory (no pull/* refs), and Apply diffed against the floating PR head instead of the pinned commit -- allowing cache key collisions when the PR advanced.

  • Thread the upstream remote URL from the orchestrator through to gitRequest.Apply so PR refs are fetched from the real remote, not the worker's local origin
  • Pin content by diffing baseRef...commit instead of baseRef...pull/<id>/head; retain the ancestor check as a sanity guard
  • Make commit required at validation (mapper.ProtoToBuildDescription) since it forms cache identity
  • Add real-git integration tests: stable tree across PR advancement, upstream fetch from local clone, stale commit rejection

Test plan

  • All existing unit tests pass with updated signatures
  • New mock-based tests verify upstream fetch and commit pinning
  • New real-git tests with a local bare repo verify stable treehash across PR advancement
  • go build ./... and go test ./... pass (excluding env-gated integration tests)
  • make gazelle regenerated BUILD files

🤖 Generated with Claude Code

…udit #3)

Two coordinated defects in the native PR-application path:

(a) Workers cloned with --local have their "origin" pointing at the pool's
local directory, which lacks pull/* refs. gitRequest.Apply now fetches PR
refs from the real upstream remote URL, threaded through from the
orchestrator via NewRequest/NewGitRequest.

(b) Apply previously diffed against the floating PR head, so the same
(URL, commit) cache key could materialize different trees as the PR
advanced. Apply now diffs against the pinned commit instead. The ancestor
check is retained as a sanity guard. Commit is now required at validation
(mapper.ProtoToBuildDescription) since it forms cache identity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

…f unconditional fetch

Replace the unconditional bare-SHA fetch with a RevParse-based local
object-presence check. The pinned commit is almost always already in the
local odb because it must be an ancestor of the PR head we just fetched.
A bare-SHA fetch is only attempted as a best-effort fallback when the
object is missing locally, avoiding hard failures against git servers
that refuse bare-SHA fetches (no uploadpack.allowAnySHA1InWant).

Add real-git test proving Apply succeeds without bare-SHA fetch when the
commit is reachable from the fetched PR head.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@sbalabanov sbalabanov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Github pull request which comes from upstream is supposed to be immutable - i.e. not only it contains a traditional Github URL in a form of #257, but also a commit hash of the head of the request, so advancement in a PR causes a request to be revalidated. Analyze the fix in the light of that, specifically analyze how the request URL is parsed. Is the fix needed?
Reply to this comment, keep the PR in draft mode.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

Thanks for calling this out. I traced the request construction and application path against the immutable (PR URL, head commit) contract.

workspace.NewRequest parses the URL with net/url, accepts the normalized github://... scheme, and passes only u.Path onward. NewGitRequest then uses filepath.Base(requestPath), so the URL contributes only the PR number to Apply; it does not pin the applied Git object, and its repository/remote components are discarded.

The commit therefore has to provide the immutable content identity. Before this PR, Apply fetched the floating pull/<id>/head, checked only that the supplied commit was an ancestor, and then diffed against the floating head. If the PR advanced, an already-issued request for the older (URL, commit) still passed the ancestor check but materialized the newer head. Upstream revalidation creates a new request/cache key, but it does not change the semantics of that older or in-flight request. Diffing baseRef...commit is consequently still needed.

The upstream-fetch part is also needed independently: worker clones have origin set to the pool's intermediate local clone, which does not expose GitHub pull/* refs. Because URL parsing currently discards the repository location, this change threads the already-authoritative build remote to the fetch. The commit-presence/ancestor check remains only a provenance guard; the requested commit is the content actually applied.

No additional code change was needed from this analysis; the reviewed implementation remains at ac28100c69e58e524f0e5a70c7f3a16659c16286. I have kept the PR in draft as requested.

[addressed by agent]

@sbalabanov

Copy link
Copy Markdown
Contributor Author

closing in favor of #264 and subsequent implementations

@sbalabanov sbalabanov closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants