ci: verify submodule pins are reachable from tracked branch#11063
ci: verify submodule pins are reachable from tracked branch#11063jkiviluoto-nv wants to merge 13 commits into
Conversation
Add a CI check that fails the build if any submodule pointer in .gitmodules points at a commit that is not reachable from the upstream submodule's tracked branch (the `branch =` override if present, otherwise the remote's default branch). Motivated by the slang-rhi haaggarwal/SER incident: PRs only show the submodule pointer diff, so reviewers cannot tell when a pin moves to a developer branch instead of mainline. This check enforces the invariant automatically. extras/check-submodule-commits.sh resolves each pin via `git ls-tree HEAD` so it works without checking out submodules. It fetches each submodule into a temporary bare repo with progressive depth (50 -> 500 -> full) and uses `git merge-base --is-ancestor` to verify reachability. A `--diff-base <ref>` flag limits work to submodules that actually moved between the base and HEAD; without it, every submodule is checked (useful for ad-hoc local invocation). The workflow runs on pull_request and merge_group, and intentionally does not use a `paths:` filter — GitHub treats path-skipped jobs as "not run", which breaks required-check gating. The `--diff-base` flag short-circuits when no submodule pointer changed. Two adjustments to keep the initial rollout clean against master: - Opt-out: a new `submodule.<name>.slang-skip-pin-check = true` key in .gitmodules disables branch-reachability for that submodule. The script still verifies the pinned SHA is fetchable from the URL, so typos and rewritten history are still caught. Applied to external/imgui (vendored fork of v1.68 carrying a slang-local patch whose commit isn't on upstream master). - Branch override for external/lua: pinned at tag v5.4.8, which lives on the v5.4 maintenance branch, not master. Set `branch = v5.4` in .gitmodules so the check tracks the right branch. Fixes shader-slang#9336
|
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:
📝 WalkthroughWalkthroughAdds CI checks and a verification script to ensure git submodule pointers reference SHAs reachable from their tracked remote branches, with opt-out support. Updates ChangesSubmodule Pointer Validation
Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant check-submodule-commits.sh
participant .gitmodules
participant RemoteGitRepo
PullRequest->>GitHubActions: trigger on pull_request or merge_group
GitHubActions->>check-submodule-commits.sh: run with BASE_SHA and --diff-base
check-submodule-commits.sh->> .gitmodules: read submodule path/url/branch/skip flags
check-submodule-commits.sh->>RemoteGitRepo: git ls-remote / fetch pin history
RemoteGitRepo-->>check-submodule-commits.sh: branch and commit reachability data
check-submodule-commits.sh-->>GitHubActions: pass or fail summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3de66caf-9068-42cb-9964-890736e4ac4e
📒 Files selected for processing (3)
.github/workflows/check-submodules.yml.gitmodulesextras/check-submodule-commits.sh
|
I think this is against to the recommended workflow. |
Add an explicit `permissions: contents: read` block on the check-submodules job so the GITHUB_TOKEN granted to the run carries only the scope the script actually needs (a checkout). Without this, PR-triggered jobs inherit the repository's default token scopes, which is broader than necessary for a workflow that performs no writes. Addresses CodeRabbit review feedback on PR shader-slang#11063.
…s.sh Pass `--default ''` to the `git config` reads of `submodule.<name>.path` and `submodule.<name>.url` so a missing key returns an empty string rather than triggering `set -e` and aborting the script before the existing `if [[ -z "$path" || -z "$url" ]]` guard can emit a warning and skip the malformed entry. Matches the `--default ''` pattern already used for the optional `branch` and `slang-skip-pin-check` keys two lines below. Addresses CodeRabbit review feedback on PR shader-slang#11063.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef89f7d5-b492-4242-acf8-e4e80133f0df
📒 Files selected for processing (2)
.github/workflows/check-submodules.ymlextras/check-submodule-commits.sh
@jhelferty-nv as original issue #9336 author could comment - do we need this or not? If above quote would mean we have more expections from the rule than cases where this check actually guards CI, let's close both the issue and this PR. |
@jkwak-work I'm fine with allowing that sort of exception. What I want to avoid is having commits pointing to arbitrary developers' commits, so we can avoid the exact following scenario:
At this point, The other way we could try to handle this is by making it an agent responsibility to check the git commit hashes, and report on which repository and branch they come from. (It seemed cheaper/easier to just enforce it with automation, and maybe a few allow lists, though) |
I see. |
Right. And even on the main repo, shader-slang team members can push developer branches. Hence the suggestion to always enforce being a commit from main/master branch. That said, if this is overly contentious, I'd be ok with an initial half-way measure of just disallowing arbitrary developer repositories as you suggest. Some protection is better than none. |
|
I discussed with @jhelferty-nv on the meeting. |
We routinely pin SPIRV-Tools to a fix that has been upstreamed as a PR but not yet merged to KhronosGroup main, to avoid a chicken-and-egg wait. Such a commit is deliberately not yet on the tracked branch, so the branch-reachability rule would reject it. Opt SPIRV-Tools out via the existing slang-skip-pin-check mechanism: the branch check is skipped, but the pinned SHA is still verified to be fetchable from the official KhronosGroup URL, which still catches pins that accidentally reference a developer fork. Agreed with jhelferty-nv and jkwak-work in PR shader-slang#11063 review discussion.
|
Added the SPIRV-Tools exception we agreed on (@jhelferty-nv / @jkwak-work), and merged in the latest SPIRV-Tools exception ( Verified locally — the check reports for spirv-tools: Also merged @jhelferty-nv this should address the change request — could you take another look when you have a moment? |
The lua pin (3fe7be9, v5.4.7-7-g3fe7be9) is reachable from lua's v5.4 maintenance branch but not from its default branch 'master'. The 'branch = v5.4' override is therefore required for the pin check to resolve the correct branch; without it the check would fall back to 'master' and fail. Expand the comment to state this, and correct the stale 'tag v5.4.8' note (the pin is 7 commits past v5.4.7, not v5.4.8).
verify_reachable in check-submodule-commits.sh can escalate to a full `git fetch --unshallow` per submodule against its upstream remote. Without a job timeout, a slow or unresponsive remote could hold a runner until GitHub's 360-minute default deadline. Bound the job at 20 minutes, which still leaves ample headroom for full-history fetches of every tracked submodule. Addresses CodeRabbit review feedback on PR shader-slang#11063.
The submodule pin check resolved a submodule's tracked ref (the `branch =` value in .gitmodules, or the remote default) only as a branch (refs/heads/<name>). But a submodule may legitimately track a release tag instead: external/fast_float tracks `v8.2.7`, which exists upstream only as refs/tags/v8.2.7 with no same-named branch. The branch-only fetch could never resolve such a ref, so the pin was reported unreachable even though it is in fact pinned to an immutable tag — a stronger guarantee than a branch pin. Factor the depth-escalating fetch into verify_reachable_from_ref, which takes a fully-qualified source ref, and have verify_reachable try the branch form first and fall back to the tag form. A commit that is on neither the branch nor the tag still fails, so real developer-branch pins are still caught. Updates the script header, INFO/PASS/ERROR wording, and the fix hint to say "branch or tag" instead of "branch".
|
@jhelferty-nv friendly nudge to revisit — your
Could you dismiss/update the stale review when you have a moment? Thanks! |
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)
extras/check-submodule-commits.sh (1)
210-213: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not trust PR-modified submodule metadata when deciding what to validate.
The check reads
url,branch, andslang-skip-pin-checkfrom HEAD, then--diff-baseskips whenever the gitlink SHA is unchanged. A PR can therefore change only.gitmodulesmetadata and be skipped, or change the URL to a fork and validate the new pin against that fork’s branch. Load the base.gitmodulestoo, include metadata changes in the skip decision, and reject or explicitly require review for URL changes before using the HEAD URL as the trust anchor.Also applies to: 237-246
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dfa8021-ef3d-4872-972c-fd0a361ef873
📒 Files selected for processing (3)
.github/workflows/check-submodules.yml.gitmodulesextras/check-submodule-commits.sh
imgui was updated to v1.92.8 (8936b58f) on master by shader-slang#11713, and that commit is reachable from imgui's upstream master. The vendored-v1.68 skip is stale, so drop the slang-skip-pin-check opt-out and let imgui go through the normal branch reachability check like every other tracked submodule.
An opt-out submodule (slang-skip-pin-check = true) only reaches the failure list when its pinned SHA is not fetchable from the URL at all; its branch/tag reachability is never checked. The generic 'not reachable from tracked ref' remediation is misleading for that case, so print URL/SHA/rewritten-history guidance when the ref is '<opted out>'.
check-submodules is not a required status check, so a path-skipped job on an unrelated PR is harmless. Add a pull_request paths: filter (external/**, .gitmodules, the workflow, the script) so the job only spins up a runner and fetches submodule history when a submodule pointer could have changed. merge_group keeps running over all submodules as a final gate (GitHub ignores paths: on merge_group). This makes the per-PR --diff-base short-circuit redundant, so drop it from the workflow invocation.
4 participants
Summary
Adds a CI check that fails when any submodule pointer in
.gitmodulespoints at a commit that is not reachable from the upstream submodule's tracked branch (thebranch =override if present, otherwise the remote's default branch).Fixes #9336.
Why
PRs only show the submodule pointer diff, so reviewers cannot tell when a pin moves to a developer branch instead of mainline. The slang-rhi
haaggarwal/SERincident (#9335) is the motivating example. This check enforces the invariant automatically so the same class of mistake cannot land again.How it works
extras/check-submodule-commits.shresolves each pin viagit ls-tree HEAD(no submodule checkout needed). For each submodule, it fetches into a temporary bare repo with progressive depth (50 → 500 → full) and usesgit merge-base --is-ancestorto verify reachability..github/workflows/check-submodules.ymlruns onpull_requestandmerge_group. The job always runs (no top-levelpaths:filter — those interact badly with required-check gating) but the script's--diff-base $BASE_SHAflag short-circuits when no submodule pointer changed, so the typical-PR overhead is ~1 second.Opt-out and branch fixes
While building this I found two pre-existing issues on master that the check surfaces:
external/imguiis pinned at4c902849("correct include case"), which is a slang-local patch on top of vendored imgui v1.68 and is not on upstreammaster. This is a legitimate vendored fork, not a mistake. Added aslang-skip-pin-check = trueflag in.gitmodulesto opt this submodule out of branch reachability. The script still verifies the pinned SHA is fetchable from the URL, so a typo or upstream history rewrite would still be caught.external/luais pinned at tagv5.4.8, which lives on the upstream'sv5.4maintenance branch (notmaster). Addedbranch = v5.4to track the right branch, similar to howexternal/cmarkalready usesbranch = gfm.The opt-out flag should be used sparingly. The only legitimate use case is a vendored fork like imgui where the pinned commit intentionally isn't on any upstream branch.
Follow-ups
Check Submodule Pointersto required status checks onmaster. I'd appreciate help with this since it needs admin access to repo settings.slang-rhi, etc.) — happy to file follow-up issues / PRs once this approach is settled here.