Repository navigation
Conversation
Fork commits ship in vCluster products without a reviewed PR: kine v0.16.5.vcluster.1 carries 15 direct pushes over upstream v0.16.5, and tailscale v1.96.4-loft.3 carries 14 commits with no PR. A product bump PR approves a version string, not the fork commits behind it, so SOC 2 change management has no evidence for that code. This is build step 1 of the fork review check design (loft-sh/architecture engineering-enablement/designs/fork-review-check.md): a CLI that resolves each fork commit a build consumes and checks the range from the trusted upstream base to it. Every commit must belong to a merged fork PR approved at its final head, after its last force push, by a non-author with write access; the pinned commit itself must be that PR's final head or merge result. It runs read-only, so it can measure today's gap before anything blocks. The test workflow's live job runs it against the three pins the design names with the workflow token, which answers whether a required workflow's token can read approver permission on another repo. References DEVOPS-1740 Claude-Session: https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn
| for _, pr := range pulls { | ||
| approval, reasons := c.qualifyingApproval(ctx, repo, pr, effectiveAuthor(ci, pr)) | ||
| if approval == nil { | ||
| res.Failures = append(res.Failures, reasons...) | ||
| continue | ||
| } | ||
| res.Pull, res.Approval, res.Pass, res.Failures = pullRecordOf(pr), approval, true, nil | ||
| return res | ||
| } |
There was a problem hiding this comment.
question: Should we verify that the commit is part of the PR’s approved final head, or is a GitHub-generated merge/rebase result? commits/{sha}/pulls proves association with a PR, but not necessarily that the commit’s contents were reviewed
There was a problem hiding this comment.
Fixed in 7ae75e4. The consumed tip already had to match an approved final head or merge result. Each associated commit must now also be in the approved final head, or be the verified merge result, so a commit removed before approval cannot pass on association alone.
| func effectiveAuthor(ci commitInfo, pr pullInfo) string { | ||
| if ci.Merge && ci.Committer == githubMergeCommitter && ci.SHA == pr.MergeCommit { | ||
| return pr.Author | ||
| } | ||
| return ci.Author | ||
| } |
There was a problem hiding this comment.
question: Could a locally created commit appear to come from web-flow by using its noreply@github.com committer email? Should we require commit.verification.verified before treating a merge commit as GitHub-generated?
There was a problem hiding this comment.
Fixed in 7ae75e4. The merge-result path and author exception now require the exact PR merge SHA, the web-flow committer, and a verified commit signature. An unsigned commit with that committer no longer gets the exception.
| // With a vendor directory and no workspace, go list cannot compute "all" | ||
| // from vendor/modules.txt. Go already checks that file against go.mod, so | ||
| // reading the graph from go.mod yields the same versions. |
There was a problem hiding this comment.
question: For repositories that build with -mod=vendor, such as vcluster-pro, doesn’t -mod=mod verify the downloaded modules rather than the vendored source that is actually compiled. Should we verify the committed vendor/ contents as part of this check?
There was a problem hiding this comment.
Fixed in 7ae75e4. Go mode now requires a trusted vendor verifier for vendored modules and runs it against a temporary checkout of the committed tree. Without a verifier the check fails closed.
Builds using vendored Go code can compile sources that differ from the downloaded modules, and a PR association can survive changes to the reviewed head. Require a trusted vendor verifier and prove each commit is covered by the approved head or the verified merge result.\n\nA web-flow committer label alone can be spoofed, so the merge exception also requires GitHub signature verification.
Summary
Build step 1 of the fork review check design: a read-only CLI that checks every fork commit a product build consumes reached its loft-sh fork through a merged PR approved by someone other than its author.
Key Changes
.github/actions/fork-review-check/src: Go CLI with two modes.refchecks one fork tag or commit;goresolves every loft-sh fork in a module graph to the commit Go downloaded (Origin.Hash).-as-ofenforces rule 3. Unsuffixed tags, missing upstream tags, non-ancestor bases and ranges over 250 commits fail.fork-upstreamproperty only. A fork without it fails;-upstreamkeeps the per-commit evidence coming in read-only runs and does not clear the failure.fork-upstreamvalue and source, base, and per commit the PR, approver, approval time and permission read at check time.test-fork-review-check.yaml: unit tests, plus an advisory live job on kinev0.16.5.vcluster.1, tailscalev1.96.4-loft.3and the fuse commit vnode-internal pins, run with the workflow token. That run answers whether a required workflow's token can read approver permission cross-repo.Decisions to review
-upstream fork=upstream@branch.web-flow) names the merger as author. The CLI treats the PR author as that commit's author, so a reviewer who approves and merges still counts. A merge commit pushed by hand keeps its git author.head_ref_force_pushedevent. A new push changes the head, so a stale approval already fails the final-head test.Dependencies
None. Images by digest, release binaries, source clones, the unknown-reference net, the required workflow and the release receipt are step 4.
TODO
fork-review-check/v1after merge (no callers yet)References DEVOPS-1740
https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn