From 6d26eaefe7db892b5c7619f3c629415b8234a3b8 Mon Sep 17 00:00:00 2001 From: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:49:29 +0000 Subject: [PATCH 1/7] [None][fix] Validate vendor source forks against live dev tips Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> --- 3rdparty/vendor-sources.md | 200 ++++++++++++++++++ scripts/vendor/README.md | 16 +- scripts/vendor/bot.py | 5 + scripts/vendor/promote.py | 43 +++- .../unittest/others/test_maintain_prims_ts.py | 143 ++++++++++++- tests/unittest/others/test_vendor_bot.py | 36 +++- 6 files changed, 432 insertions(+), 11 deletions(-) diff --git a/3rdparty/vendor-sources.md b/3rdparty/vendor-sources.md index 886fd05482fc..28c4bc3a236a 100644 --- a/3rdparty/vendor-sources.md +++ b/3rdparty/vendor-sources.md @@ -19,6 +19,13 @@ edit the YAML, generated patches, or digests by hand. All examples below use the default lock. For an isolated test or another consumer repository, place `--lock PATH` before the subcommand. +For a code-changing PR, start with the +[PrimTS contributor walkthrough](#contributor-workflow-primts-example). +It covers editing the vendored code, exporting to a temporary branch in your +fork, and pinning that branch in TensorRT-LLM. Do not point a normal PrimTS +source-update PR at FlashInfer upstream `main`, even if your fix has merged +there; canonical-branch promotion and upstream refresh are maintainer workflows. + ## Lock contract A locked vendor has one of two durable states: @@ -230,6 +237,199 @@ The same rule applies when adopting a newer commit that was developed upstream first: prepare the destination to exactly match the proposed commit plus the existing patch, then run `pin`. Do not use `sync` to look for that commit. +## Contributor workflow: PrimTS example + +Suppose you need to fix a validation check in the vendored PrimTS `context.py`. +The source change belongs in FlashInfer; TensorRT-LLM's PR must contain both the +updated vendored files and a lock pointing to your committed temporary branch. +The same export/pin mechanics work for other vendors using their own lock +entry, paths, upstream fork, and maintainer policy. + +### 1. Identify the canonical baseline + +Start with a clean TensorRT-LLM checkout based on the latest **target base +branch** (`main` or the intended release branch), and your own FlashInfer fork. +Read `vendors.flashinfer-prims-ts` in that base branch's +`3rdparty/vendor_sources.lock.yaml`: its `url`, `branch`, and `commit` identify +the canonical source baseline. If it still points to another contributor's +temporary branch, wait for the preceding promotion or coordinate with the +maintainer; do not treat that temporary branch as the canonical branch. + +Replace every placeholder below. `PRIMTS_REPO` must be a new local directory +for this example. The dev-branch name is illustrative: use the branch for your +target base, not a dated branch copied from this document or another PR. +Run all commands from the TensorRT-LLM repository root; `git -C` commands act +on the separate FlashInfer checkout. + +```bash +PRIMTS_REPO=/absolute/path/to/new-flashinfer-checkout +PRIMTS_FORK_URL=https://github.com/YOUR_ACCOUNT/flashinfer.git +PRIMTS_DEV_URL=https://github.com/MAINTAINER/flashinfer.git +PRIMTS_DEV_BRANCH=trtllm-prims-ts-dev-YYYYMMDD +PRIMTS_LOCKED_SHA=FULL_40_CHARACTER_LOCKED_COMMIT +PRIMTS_TMP_BRANCH=trtllm-prims-ts-validation-fix + +git switch -c feat/prims-ts-validation-fix +python scripts/vendor/manage.py check flashinfer-prims-ts +git clone "$PRIMTS_FORK_URL" "$PRIMTS_REPO" +git -C "$PRIMTS_REPO" fetch "$PRIMTS_DEV_URL" "$PRIMTS_DEV_BRANCH" +PRIMTS_DEV_TIP=$(git -C "$PRIMTS_REPO" rev-parse FETCH_HEAD) +``` + +Compare `PRIMTS_LOCKED_SHA` with `PRIMTS_DEV_TIP`. The straightforward path below +assumes they are equal. If they differ, the lock is not the live dev-branch tip: +first rebase onto the consumer base containing the completed promotion, or +follow [the advanced-tip instructions](#if-the-dev-branch-advances). Never +substitute the live tip for the locked SHA when calling `export`. + +### 2. Edit and test the vendored code + +For this example, edit +`tensorrt_llm/_torch/attention/backends/prims_ts/context.py`, add the appropriate +tests, and run the affected TensorRT-LLM tests. Inspect your pending source delta: + +```bash +git diff -- tensorrt_llm/_torch/attention/backends/prims_ts +``` + +An offline vendor check now fails until you pin the exported change; this is +expected. Do not run `sync`, which would discard the pending edits, or record +an upstream-worthy fix in the TensorRT-LLM-only compatibility patch. Do not +hand-edit the lock or its digests to bypass the check. + +### 3. Create the temporary source branch and export + +Create the branch at the **locked SHA** in your clean FlashInfer checkout: + +```bash +git -C "$PRIMTS_REPO" switch -c "$PRIMTS_TMP_BRANCH" "$PRIMTS_LOCKED_SHA" +python scripts/vendor/manage.py export flashinfer-prims-ts --repo "$PRIMTS_REPO" +git -C "$PRIMTS_REPO" diff -- flashinfer/attention/prims_ts +``` + +`export` requires that exact starting SHA. It transfers only the pending delta +to `flashinfer/attention/prims_ts`; it does not copy TensorRT-LLM's existing +compatibility patch upstream, change the lock, or commit anything. If the edit +overlaps the compatibility patch and export refuses it, resolve the upstream +change and downstream adaptation deliberately rather than copying the whole +vendored directory over FlashInfer. + +### 4. Commit, check the live dev tip, and push to your fork + +Run the relevant FlashInfer tests and review the exported changes before committing. +Stage any additional upstream test files explicitly as well. + +```bash +git -C "$PRIMTS_REPO" add flashinfer/attention/prims_ts +git -C "$PRIMTS_REPO" commit -s -m 'Fix PrimTS context validation' +git -C "$PRIMTS_REPO" fetch "$PRIMTS_DEV_URL" "$PRIMTS_DEV_BRANCH" +PRIMTS_DEV_TIP=$(git -C "$PRIMTS_REPO" rev-parse FETCH_HEAD) +git -C "$PRIMTS_REPO" merge-base --is-ancestor "$PRIMTS_DEV_TIP" HEAD +``` + +The ancestry check must succeed (exit status 0) before continuing. If it fails, +follow the advanced-tip instructions below. The pinned commit must contain the +**latest** dev tip, not just the older commit recorded in the consumer lock, +because the maintainer must be able to fast-forward the dev branch after merge. + +```bash +git -C "$PRIMTS_REPO" push "$PRIMTS_FORK_URL" "HEAD:refs/heads/$PRIMTS_TMP_BRANCH" +PRIMTS_NEW_SHA=$(git -C "$PRIMTS_REPO" rev-parse HEAD) +``` + +Do not push directly to the canonical dev branch or use upstream `main` as the +source of this consumer update. Open the related FlashInfer PR separately; +if it needs a different base/history for upstream review, use a separate branch +without changing the consumer pin to upstream `main`. + +### 5. Pin the pushed commit and verify + +```bash +python scripts/vendor/manage.py pin flashinfer-prims-ts \ + --url "$PRIMTS_FORK_URL" \ + --branch "$PRIMTS_TMP_BRANCH" \ + --commit "$PRIMTS_NEW_SHA" \ + --repo "$PRIMTS_REPO" +python scripts/vendor/manage.py check flashinfer-prims-ts +python scripts/vendor/manage.py check flashinfer-prims-ts --repo "$PRIMTS_REPO" +git diff -- 3rdparty/vendor_sources.lock.yaml \ + 3rdparty/vendor_patches/flashinfer-prims-ts.patch \ + tensorrt_llm/_torch/attention/backends/prims_ts +``` + +`pin` verifies that the committed source plus the existing compatibility patch +reproduces your destination. It updates the lock, not the destination files. +Keep the patch unchanged unless it needs a reviewed downstream adaptation or +`pin` removes it because upstream has absorbed it. + +### 6. Submit the TensorRT-LLM PR + +Stage the lock and vendored files together, plus the relevant tests and any +intentional compatibility-patch changes. Commit with DCO sign-off and push to +your TensorRT-LLM fork (replace the remote name below): + +```bash +git add 3rdparty/vendor_sources.lock.yaml tensorrt_llm/_torch/attention/backends/prims_ts +git commit -s -m '[None][fix] update vendored PrimTS context validation' +git push -u YOUR_TRTLLM_FORK_REMOTE HEAD +``` + +Open the PR against the same target base used in step 1 and run normal CI. +Put this block in the **PR description**, replacing `123` with the real paired +FlashInfer PR number; list all upstream PRs needed to attribute the source delta: + +````markdown + +```yaml +schema_version: 1 +vendors: + flashinfer-prims-ts: + upstream_prs: + - https://github.com/flashinfer-ai/flashinfer/pull/123 +``` + +```` + +If there genuinely is no paired upstream PR, replace `upstream_prs` with a +nonempty `unpaired_reason` explaining why; do not use it to bypass a failed +match. See [contributor metadata](../scripts/vendor/README.md#contributor-metadata) +for multiple PRs and ambiguity resolution. + +Leave the lock on your temporary fork branch through review and merge. Do not +delete or rewrite that branch before promotion completes. The maintainer or +bot then fast-forwards the canonical dev branch and creates a separate +**lock-only** PR restoring its URL/branch without changing the reviewed SHA or +vendored code. Do not pin back to upstream after the paired FlashInfer PR merges. + +### If the dev branch advances + +If your changes have not yet been exported, first export and commit at the +locked SHA as in steps 3–4. If you already exported and pinned the change, keep +working on that temporary source branch; do not export the same change again. +Fetch the latest canonical branch and rebase your temporary source branch onto +it, resolving conflicts and rerunning tests: + +```bash +git -C "$PRIMTS_REPO" fetch "$PRIMTS_DEV_URL" "$PRIMTS_DEV_BRANCH" +PRIMTS_DEV_TIP=$(git -C "$PRIMTS_REPO" rev-parse FETCH_HEAD) +git -C "$PRIMTS_REPO" rebase "$PRIMTS_DEV_TIP" +``` + +Update the TensorRT-LLM destination to match the rebased selected source files +plus the existing compatibility patch, retaining downstream adaptations. This +may require incorporating other promoted changes or resolving patch conflicts; +neither `sync` nor `pin` imports a new revision for you. Then repeat the ancestry +check, push, pin, source verification, and consumer tests with the **new SHA**. +For an already-published branch, coordinate before rewriting it and use +`--force-with-lease`, or push a new temporary branch and pin that instead. +Never force-push the canonical branch. + +Updating only your source branch or PR description does not update the consumer +pin. The bot rechecks live ancestry before promotion; repeat this process if +another promotion advances the dev branch again. After a maintainer refresh +changes the canonical branch name, rebase the consumer PR onto its latest target +base and use that new canonical branch rather than continuing the retired one. + ## Remove a vendor Remove a lock entry and its generated compatibility patch while preserving the diff --git a/scripts/vendor/README.md b/scripts/vendor/README.md index fcff541083af..ec5691211b38 100644 --- a/scripts/vendor/README.md +++ b/scripts/vendor/README.md @@ -80,6 +80,15 @@ selecting an arbitrary newer pin. ## Contributor metadata +Source-update PRs must pin a temporary branch in a fork of the configured +upstream, not the upstream repository itself (including upstream `main`). +The pinned commit must contain the **latest canonical dev-branch tip**, even +when the consumer's lock still names an older commit. Fetch and rebase onto +the dev branch, push the temporary branch, then regenerate the consumer lock +and vendor snapshot at the new SHA. Updating only the temporary branch or PR +description is insufficient. A broader upstream refresh uses the separate, +explicitly configured maintainer refresh workflow. + Authors add exactly one block to the source-update PR description. Multiple vendors can share it; each monitor reads its configured vendor entry: @@ -163,7 +172,12 @@ are retained during future refresh until equivalence is established separately. Before a new operation is committed, the source head and normalized metadata are rechecked before feedback and publication. Metadata corrections rerun -validation. Durable promotion records remain authoritative; later description +validation. The live canonical tip is checked before and after attribution and +again before publication; it is not inferred from the consumer lock or cached +successful feedback. A canonical tip already contained in the pinned source +is allowed, including one advanced by another consumer branch. Divergent or +newer tips require a rebase, and a canonical history rewrite requires maintainer +inspection. Durable promotion records remain authoritative; later description edits or upstream PR changes do not rewrite a recorded operation's provenance. ## Launch the local monitor diff --git a/scripts/vendor/bot.py b/scripts/vendor/bot.py index 6df61a6705fc..8db4d5043b34 100644 --- a/scripts/vendor/bot.py +++ b/scripts/vendor/bot.py @@ -816,11 +816,16 @@ def inspect(self, number: int) -> None: promote._check_current(self.gh, plan) try: self.progress.operation("validating_metadata", number) + promote._check_source_repo(self.gh, promote._repo_name(reviewed.url)) + promote._check_canonical_tip(self.gh, previous, reviewed) entry = metadata.parse(pr.get("body") or "", self.gh.vendor_name, self.gh.upstream_repo) cache = self.args.workdir / "sources.git" # Until provenance is committed, decisions follow current remote # inputs, not whichever upstream heads a previous process cached. _, evidence = metadata.resolve(self.gh, previous, reviewed, entry, cache) + # Attribution may take long enough for another consumer's promotion + # to advance the shared canonical branch. Do not report stale inputs as valid. + promote._check_canonical_tip(self.gh, previous, reviewed) except Diagnostic as error: self._feedback(pr, error) return diff --git a/scripts/vendor/promote.py b/scripts/vendor/promote.py index d06ed02200c4..c20083042483 100644 --- a/scripts/vendor/promote.py +++ b/scripts/vendor/promote.py @@ -229,8 +229,42 @@ def _check_fork(gh: GitHub, repo: str, upstream: str) -> None: def _check_source_repo(gh: GitHub, repo: str) -> None: - if repo.lower() != gh.upstream_repo.lower(): + try: _check_fork(gh, repo, gh.upstream_repo) + except Diagnostic as error: + raise Diagnostic( + error.problem, + "Create a temporary branch in an upstream fork from the latest canonical dev " + "branch and apply the intended source changes there. Push it, then use " + "`scripts/vendor/manage.py` to update the lock URL, branch, SHA, and vendored " + "snapshot. A broader upstream refresh requires the separate, explicitly " + "configured maintainer refresh workflow.", + ) from error + + +def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> str: + """Require the immutable source pin to contain the live canonical branch tip.""" + repo = _repo_name(previous.url) + tip = _sha(gh.get(f"repos/{repo}/git/ref/heads/{previous.branch}")["object"]["sha"]) + if tip in (previous.commit, reviewed.commit): + return tip + if gh.compare(repo, previous.commit, tip)["merge_base_commit"]["sha"] != previous.commit: + raise RuntimeError( + "Canonical branch no longer contains the consumer lock's revision; " + "a maintainer must inspect the branch history before promotion." + ) + if gh.compare(repo, tip, reviewed.commit)["merge_base_commit"]["sha"] != tip: + raise Diagnostic( + f"The pinned temporary source commit {field(reviewed.commit)} does not contain " + f"the latest canonical branch tip {field(tip)}.", + f"Fetch {field(previous.url)} / {field(previous.branch)} and rebase the temporary " + f"source branch {field(reviewed.branch)} onto `{tip}`. Push it, then use " + "`scripts/vendor/manage.py` to update the consumer lock and vendored snapshot " + "to the rebased SHA. The consumer lock's old pin alone is not a sufficient " + "base; updating only the PR metadata or temporary branch without its lock pin " + "will not fix this.", + ) + return tip def _record_from_message(message: str) -> dict: @@ -326,11 +360,7 @@ def _check_current(gh: GitHub, plan: Promotion) -> None: raise ValueError( "Consumer branch's vendor lock has been superseded; do not promote this PR." ) - ref = gh.get(f"repos/{plan.canonical_repo}/git/ref/heads/{plan.previous.branch}") - if ref["object"]["sha"] not in (plan.previous.commit, plan.reviewed.commit): - raise ValueError( - "Canonical branch moved unexpectedly; coordinate with the other maintainer." - ) + _check_canonical_tip(gh, plan.previous, plan.reviewed) def _plan_identity(args: argparse.Namespace, gh: GitHub) -> Promotion: @@ -431,6 +461,7 @@ def _make_plan( raise ValueError( "Promote requires a nonempty, linear source commit range; rebase merges first." ) + _check_canonical_tip(gh, previous, reviewed) prs = {} for reference in args.upstream_pr: upstream_number = _pr_number(reference, gh.upstream_repo) diff --git a/tests/unittest/others/test_maintain_prims_ts.py b/tests/unittest/others/test_maintain_prims_ts.py index 3751df9bced9..ee4441759b2f 100644 --- a/tests/unittest/others/test_maintain_prims_ts.py +++ b/tests/unittest/others/test_maintain_prims_ts.py @@ -708,14 +708,151 @@ def test_unmerged_or_wrong_base_rejected(world: World, field: str, value: object m._make_plan(world.args(), world) +def _advance_canonical(world: World) -> str: + _write(world.source, "prims_ts/kernel.py", "VALUE = 4\n") + tip = _commit(world.source, "new canonical update") + _git(world.canonical, "fetch", "-q", str(world.source), tip) + _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", tip) + return tip + + +@pytest.mark.parametrize("tip_kind", ["previous", "intermediate", "reviewed"]) +def test_promotion_accepts_live_tip_contained_in_reviewed_pin(world: World, tip_kind: str) -> None: + tip = {"previous": world.previous, "intermediate": world.first, "reviewed": world.reviewed}[ + tip_kind + ] + _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", tip) + args = world.args(publish=True) + plan = m._make_plan(args, world) + m._publish(args, world, plan) + assert _git(world.canonical, "rev-parse", "trtllm-prims-ts-dev") == world.reviewed + assert world.prs[99]["base"]["ref"] == "main" + + +def test_plan_rejects_tip_newer_than_pinned_source(world: World) -> None: + tip = _advance_canonical(world) + with pytest.raises(m.Diagnostic, match="latest canonical branch tip") as failure: + m._make_plan(world.args(), world) + assert tip in failure.value.problem + assert world.reviewed in failure.value.problem + assert "consumer lock" in failure.value.action + assert not any(method != "GET" for method, _, _ in world.calls) + + +def test_canonical_rewind_is_operator_error(world: World) -> None: + _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", world.upstream) + with pytest.raises(RuntimeError, match="Canonical branch no longer contains"): + m._make_plan(world.args(), world) + assert not any(method != "GET" for method, _, _ in world.calls) + + +def test_tip_advance_during_preparation_blocks_all_remote_writes( + world: World, monkeypatch: pytest.MonkeyPatch +) -> None: + args = world.args(publish=True) + plan = m._make_plan(args, world) + prepare = m._prepare_commit + + def advance(*arguments): + result = prepare(*arguments) + _advance_canonical(world) + return result + + monkeypatch.setattr(m, "_prepare_commit", advance) + with pytest.raises(m.Diagnostic, match="latest canonical branch tip"): + m._publish(args, world, plan) + assert not any(method != "GET" for method, _, _ in world.calls) + assert not any(call[0] == "push" for call in world.git_calls) + + +def test_manual_promotion_rejects_upstream_source( + world: World, monkeypatch: pytest.MonkeyPatch +) -> None: + plan = m._plan_identity(world.args(), world) + plan.reviewed = dataclasses.replace( + plan.reviewed, url=f"https://github.com/{_UPSTREAM}.git", branch="main" + ) + monkeypatch.setattr(m, "_plan_identity", lambda *args: plan) + with pytest.raises(m.Diagnostic, match="not upstream itself"): + m._make_plan(world.args(), world) + assert not any(method != "GET" for method, _, _ in world.calls) + + +@pytest.mark.parametrize("consumer_base", ["main", "release/1.3"]) +@pytest.mark.parametrize( + "case", ["stale", "diverged", "advance-during-attribution", "contained", "upstream"] +) +def test_open_pr_checks_live_dev_tip_before_valid_feedback( + world: World, monkeypatch: pytest.MonkeyPatch, consumer_base: str, case: str +) -> None: + world.base_branch = consumer_base + args = world.args(publish=True) + args.workdir = world.root + args.since = None + args.refresh_branch = [] + args.migrate_canonical_branch_from = None + previous = m._lock_at(world, _CONSUMER, world.base) + reviewed = m._lock_at(world, _CONSUMER, world.merged) + if case == "upstream": + reviewed = dataclasses.replace( + reviewed, url=f"https://github.com/{_UPSTREAM}.git", branch="main" + ) + endpoint = f"repos/{_CONSUMER}/pulls/17" + pr = world.get(endpoint) + pr.update( + merged=False, + state="open", + base={"ref": consumer_base}, + head={"sha": world.merged}, + user={"login": "maintainer"}, + ) + world.overrides[endpoint] = pr + world.main = world.base + if case == "diverged": + _git(world.source, "checkout", "-q", "--detach", world.previous) + if case in ("stale", "diverged"): + tip = _advance_canonical(world) + elif case == "contained": + _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", world.first) + matches = [] + + def resolve(*arguments): + matches.append(True) + if case == "advance-during-attribution": + _advance_canonical(world) + return {}, {} + + monkeypatch.setattr(b.metadata, "resolve", resolve) + with contextlib.closing(b.Store(":memory:")) as store: + monitor = b.Monitor(args, world, store) + monkeypatch.setattr(monitor, "_inputs", lambda source_pr: (previous, reviewed)) + monitor.inspect(17) + message = world.source_comments[0]["body"] + if case == "contained": + assert store.get("observation:17")["state"] == "metadata_valid" + assert "metadata and source attribution are valid" in message + else: + assert store.get("observation:17")["state"] == "blocked" + assert "metadata and source attribution are valid" not in message + if case == "upstream": + assert "not upstream itself" in message + else: + assert "latest canonical branch tip" in message + assert "rebase" in message and "scripts/vendor/manage.py" in message + if case in ("stale", "diverged"): + assert tip in message + assert bool(matches) == (case in ("contained", "advance-during-attribution")) + assert not any(call[0] == "push" for call in world.git_calls) + + @pytest.mark.parametrize("target", ["main", "canonical"]) def test_concurrent_updates_block_before_publication(world: World, target: str) -> None: plan = m._make_plan(world.args(), world) if target == "main": world.main = world.base else: - _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", world.first) - with pytest.raises(ValueError, match="superseded|moved unexpectedly"): + _advance_canonical(world) + with pytest.raises(ValueError, match="superseded|latest canonical branch tip"): m._publish(world.args(publish=True), world, plan) assert not any(method != "GET" for method, _, _ in world.calls) assert not any(args[0] == "push" for args in world.git_calls) @@ -1478,7 +1615,7 @@ def test_recovery_rejects_changed_artifacts_without_writes(world: World, tamper: elif tamper == "dco": commit["commit"]["message"] = m._record_text(original.record) elif tamper == "canonical": - _git(world.canonical, "update-ref", "refs/heads/trtllm-prims-ts-dev", world.first) + _advance_canonical(world) else: pr["state"] = "closed" world.overrides[endpoint] = commit diff --git a/tests/unittest/others/test_vendor_bot.py b/tests/unittest/others/test_vendor_bot.py index 4574e337248f..30541eea41c1 100644 --- a/tests/unittest/others/test_vendor_bot.py +++ b/tests/unittest/others/test_vendor_bot.py @@ -734,6 +734,7 @@ def __init__(self) -> None: self.reviews: list[dict] = [] self.writes: list[tuple[str, str, dict]] = [] self.merge_base_sha = "c" * 40 + self.canonical_tip = "b" * 40 def api( self, endpoint: str, *, method: str = "GET", payload: dict | None = None @@ -742,6 +743,10 @@ def api( self.writes.append((endpoint, method, payload)) if endpoint == "user": return {"login": "operator"} + if endpoint in ("repos/author/library", "repos/operator/library", "repos/org/library"): + return {"source": {"full_name": self.upstream_repo}} + if endpoint == "repos/operator/library/git/ref/heads/dev": + return {"object": {"sha": self.canonical_tip}} if endpoint.startswith("repos/org/consumer/compare/"): return {"merge_base_commit": {"sha": self.merge_base_sha}} if endpoint.endswith("/pulls/7"): @@ -1191,7 +1196,7 @@ def test_posted_marker_feedback_is_specific_and_recovers_without_cache( assert not any(payload.get("event") == "APPROVE" for _, _, payload in monitor.gh.writes) -@pytest.mark.parametrize("stage", ["inputs", "resolve"]) +@pytest.mark.parametrize("stage", ["inputs", "resolve", "canonical-tip"]) @pytest.mark.parametrize("error_type", [ValueError, RuntimeError, b.manage.VendorError]) def test_unrecognized_failures_are_operator_errors_not_author_feedback( monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch, stage: str, error_type: type[Exception] @@ -1206,6 +1211,8 @@ def fail(*args): if stage == "inputs": monkeypatch.setattr(monitor, "_inputs", fail) + elif stage == "canonical-tip": + monkeypatch.setattr(p, "_check_canonical_tip", fail) else: monkeypatch.setattr(m, "resolve", fail) monitor.poll() @@ -1215,6 +1222,31 @@ def fail(*args): assert "Operator failure" in health["last_error"]["message"] +def test_valid_feedback_is_rechecked_when_only_dev_tip_changes( + monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch +) -> None: + previous = _vendor("a" * 40, "https://github.com/operator/library.git", "dev") + reviewed = _vendor("b" * 40, "https://github.com/author/library.git", "temporary") + monkeypatch.setattr(monitor, "_inputs", lambda pr: (previous, reviewed)) + monkeypatch.setattr(m, "resolve", lambda *args: ({}, {})) + monitor.gh.pr["body"] = m.template("toy", "org/library") + original_pr = copy.deepcopy(monitor.gh.pr) + monitor.inspect(7) + assert monitor.store.get("observation:7")["state"] == "metadata_valid" + + monitor.gh.canonical_tip = "c" * 40 + monkeypatch.setattr( + monitor.gh, "compare", lambda *args: {"merge_base_commit": {"sha": previous.commit}} + ) + monitor.inspect(7) + assert monitor.gh.pr == original_pr + assert monitor.store.get("observation:7")["state"] == "blocked" + assert len(monitor.gh.comments) == 1 + assert "latest canonical branch tip" in monitor.gh.comments[0]["body"] + assert monitor.gh.canonical_tip in monitor.gh.comments[0]["body"] + assert monitor.gh.reviews[0]["state"] == "CHANGES_REQUESTED" + + @pytest.mark.parametrize("change", ["other-vendor", "patch-only", "vendor-created"]) def test_refresh_ignores_prs_without_selected_source_pin_change( monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch, change: str @@ -1261,6 +1293,7 @@ def test_refresh_source_update_uses_current_canonical_as_promotion_baseline( def refresh_candidate(monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch): previous = _vendor("1" * 40, "https://github.com/operator/library.git", "dev") reviewed = _vendor("2" * 40, "https://github.com/operator/library.git", "dev-new") + monitor.gh.canonical_tip = previous.commit locks = {"c" * 40: previous, "a" * 40: reviewed} monkeypatch.setattr(p, "_main_sha", lambda gh: "c" * 40) monkeypatch.setattr( @@ -1790,6 +1823,7 @@ def test_monitor_posts_similarity_and_deduplicates_feedback( ) previous = _vendor(base, "https://github.com/operator/library.git", "dev") reviewed = _vendor(downstream, "https://github.com/author/library.git", "temporary") + monitor.gh.canonical_tip = previous.commit monkeypatch.setattr(monitor, "_inputs", lambda pr: (previous, reviewed)) monitor.gh.pr["body"] = _body(" upstream_prs: [https://github.com/org/library/pull/42]") monkeypatch.setattr( From 65747a3118668c8c69e88afbc973d6ed54c4612d Mon Sep 17 00:00:00 2001 From: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Date: Thu, 22 Oct 2026 04:39:36 +0000 Subject: [PATCH 2/7] [None][fix] Allow ancestry-proven upstream vendor promotion pins Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> --- 3rdparty/vendor-sources.md | 61 ++- scripts/vendor/README.md | 64 +++- scripts/vendor/bot.py | 48 ++- scripts/vendor/metadata.py | 12 +- scripts/vendor/promote.py | 208 ++++++++-- .../unittest/others/test_maintain_prims_ts.py | 359 +++++++++++++++++- tests/unittest/others/test_vendor_bot.py | 58 +++ 7 files changed, 733 insertions(+), 77 deletions(-) diff --git a/3rdparty/vendor-sources.md b/3rdparty/vendor-sources.md index 28c4bc3a236a..19ead6aa02ae 100644 --- a/3rdparty/vendor-sources.md +++ b/3rdparty/vendor-sources.md @@ -22,9 +22,11 @@ default lock. For an isolated test or another consumer repository, place For a code-changing PR, start with the [PrimTS contributor walkthrough](#contributor-workflow-primts-example). It covers editing the vendored code, exporting to a temporary branch in your -fork, and pinning that branch in TensorRT-LLM. Do not point a normal PrimTS -source-update PR at FlashInfer upstream `main`, even if your fix has merged -there; canonical-branch promotion and upstream refresh are maintainer workflows. +fork, and pinning that branch in TensorRT-LLM. An already-upstream update may +instead use the [upstream-main fast-forward exception](#using-an-upstream-main-pin). +An upstream PR merging alone is insufficient: the chosen pin must contain the +latest canonical dev tip by Git ancestry. Canonical-branch promotion and divergent +upstream refresh remain maintainer workflows. ## Lock contract @@ -337,10 +339,10 @@ git -C "$PRIMTS_REPO" push "$PRIMTS_FORK_URL" "HEAD:refs/heads/$PRIMTS_TMP_BRANC PRIMTS_NEW_SHA=$(git -C "$PRIMTS_REPO" rev-parse HEAD) ``` -Do not push directly to the canonical dev branch or use upstream `main` as the -source of this consumer update. Open the related FlashInfer PR separately; -if it needs a different base/history for upstream review, use a separate branch -without changing the consumer pin to upstream `main`. +Do not push directly to the canonical dev branch. Open the related FlashInfer PR +separately; if it needs different history for upstream review, use a separate +branch. Keep the consumer pin on your temporary branch unless it independently +satisfies the [upstream-main exception](#using-an-upstream-main-pin). ### 5. Pin the pushed commit and verify @@ -399,7 +401,50 @@ Leave the lock on your temporary fork branch through review and merge. Do not delete or rewrite that branch before promotion completes. The maintainer or bot then fast-forwards the canonical dev branch and creates a separate **lock-only** PR restoring its URL/branch without changing the reviewed SHA or -vendored code. Do not pin back to upstream after the paired FlashInfer PR merges. +vendored code. A paired FlashInfer PR merging does not by itself permit switching +the consumer pin to upstream; the exception below requires exact ancestry. + +### Using an upstream-main pin + +When all intended changes are already on the configured upstream branch, the +consumer lock may use that upstream repository/branch directly if: + +```text +previous canonical pin <= latest canonical dev tip <= chosen upstream pin <= upstream main tip +``` + +`<=` means Git ancestor-or-equal, not equivalent patches under different SHAs. +Check both live tips against the **exact SHA** you intend to pin, for example +using the source repository and dev variables from the walkthrough: + +```bash +PRIMTS_UPSTREAM_URL=https://github.com/flashinfer-ai/flashinfer.git +git -C "$PRIMTS_REPO" fetch "$PRIMTS_DEV_URL" "$PRIMTS_DEV_BRANCH" +PRIMTS_DEV_TIP=$(git -C "$PRIMTS_REPO" rev-parse FETCH_HEAD) +git -C "$PRIMTS_REPO" fetch "$PRIMTS_UPSTREAM_URL" main +PRIMTS_UPSTREAM_TIP=$(git -C "$PRIMTS_REPO" rev-parse FETCH_HEAD) +PRIMTS_NEW_SHA=$PRIMTS_UPSTREAM_TIP # Or an older reviewed commit on main. +git -C "$PRIMTS_REPO" merge-base --is-ancestor "$PRIMTS_DEV_TIP" "$PRIMTS_NEW_SHA" +git -C "$PRIMTS_REPO" merge-base --is-ancestor "$PRIMTS_NEW_SHA" "$PRIMTS_UPSTREAM_TIP" +``` + +Both checks must succeed. Bring the selected destination files and compatibility +patch into alignment with that exact revision, retaining necessary downstream +adaptations, then use `manage.py pin` as in step 5 with `--url +"$PRIMTS_UPSTREAM_URL" --branch main --commit "$PRIMTS_NEW_SHA"`. `pin` verifies +materialization; it does not import a newer source tree. Commit the source/lock +update together and run normal review and CI. + +No upstream PR list, `unpaired_reason`, or vendor-promotion entry is needed for +this qualifying vendor. Other changed vendors still need their own metadata. +Upstream merge commits are allowed. The bot checks live ancestry again before +feedback and promotion; a later dev advance may require a newer pin. If dev and +upstream diverge, use a temporary fork based on dev or ask for a maintainer +refresh rather than forcing the canonical branch or bypassing metadata checks. + +Promotion still restores the canonical fork/branch and fast-forwards it only to +the reviewed SHA. It saves versioned ancestry proof instead of individual PR +mappings. See the [toolkit exception and provenance contract](../scripts/vendor/README.md#upstream-main-fast-forward-exception). ### If the dev branch advances diff --git a/scripts/vendor/README.md b/scripts/vendor/README.md index ec5691211b38..08ab87f2ca4b 100644 --- a/scripts/vendor/README.md +++ b/scripts/vendor/README.md @@ -28,6 +28,9 @@ lock and is not migrated by this change. flowchart TD candidate[Lock-changing PR for the configured consumer and base branch] --> classify{Selected vendor change} classify -->|new pin on temporary branch| metadata[Author lists upstream PRs in description] + classify -->|pin on configured upstream branch| ancestry{Live dev tip <= pin <= upstream tip?} + ancestry -->|yes, no PR list needed| verify + ancestry -->|no| feedback classify -->|temporary reference at current canonical pin| restore[Author restores canonical URL and branch only] restore --> cleanup[Reconcile obsolete bot feedback without approving code] classify -->|no promotion needed| cleanup @@ -55,6 +58,45 @@ Promotion preserves the source SHA, selected files, compatibility patch, and digests. It does not re-vendor code. A periodic upstream refresh is a separate code-changing operation and must not use the promotion CI-skip path. +### Upstream-main fast-forward exception + +A source update can pin the configured `--upstream-repo` and `--upstream-branch` +(normally `main`) directly, provided immutable Git ancestry proves: + +```text +previous canonical lock pin <= live canonical dev tip <= reviewed source pin <= observed upstream tip +``` + +Here `<=` means ancestor-or-equal, not patch similarity. The source pin may be +older than the upstream tip. Other upstream branches/tags do not qualify, and a +fork branch named `main` still uses normal contributor metadata. Divergent dev +and upstream histories require a temporary fork based on dev or an explicitly +audited maintainer refresh; do not rebase upstream or force-push dev. + +For a qualifying vendor, no `vendor-promotion` block entry, upstream PR list, or +`unpaired_reason` is required. Existing metadata for that vendor is not parsed +or matched. Other vendors in the same consumer PR still need their own normal +metadata unless they independently qualify. The exception does not bypass +source/include selection checks, materialization verification, normal source-PR +review/CI, or canonical publishing-fork checks. Upstream merge commits are allowed; +temporary-fork updates still require linear history. + +After merge, promotion advances dev to the **reviewed pin**, never to the moving +upstream tip, then restores the canonical URL/branch in a lock-only PR. Live +ancestry is rechecked before valid feedback and publication steps; ref movement +that invalidates it stops promotion without force-pushing. This is not a permanent +switch to tracking upstream and does not select a new canonical branch. + +New upstream-main promotions use `VENDOR PROMOTION V2` commit-message provenance: +`source_mode: upstream-main`, the prior pin, `checked_canonical_tip`, `promoted`, +`upstream_observed_main`, repository/branch identities, and attribution scope. +They contain no synthetic unpaired commits or PR mappings. Retries verify and +reuse the original immutable proof; mutable refs cannot rewrite published +provenance. V1 temporary-fork records remain supported. A future refresh must +verify that its **actual chosen upstream target contains the promoted pin** +before omitting these commits; an old observation of upstream is not permission +to drop them after an upstream history rewrite or an older target selection. + ## Operator setup Use your own approved GitHub credentials (`gh auth login`, `GH_CONFIG_DIR`, or @@ -80,8 +122,9 @@ selecting an arbitrary newer pin. ## Contributor metadata -Source-update PRs must pin a temporary branch in a fork of the configured -upstream, not the upstream repository itself (including upstream `main`). +Source-update PRs normally pin a temporary branch in a fork of the configured +upstream. The upstream-main exception below also permits the configured upstream +repository/branch when Git ancestry proves that it contains the dev history. The pinned commit must contain the **latest canonical dev-branch tip**, even when the consumer's lock still names an older commit. Fetch and rebase onto the dev branch, push the temporary branch, then regenerate the consumer lock @@ -89,8 +132,9 @@ and vendor snapshot at the new SHA. Updating only the temporary branch or PR description is insufficient. A broader upstream refresh uses the separate, explicitly configured maintainer refresh workflow. -Authors add exactly one block to the source-update PR description. Multiple -vendors can share it; each monitor reads its configured vendor entry: +For temporary-fork updates, authors add exactly one block to the source-update +PR description. Multiple vendors can share it; each monitor reads its configured +vendor entry: ````markdown @@ -147,7 +191,7 @@ Commits with no lock-selected changes need no upstream attribution or author resolution. Empty selected patches never count as an upstream match; omit PRs that cover only excluded changes. The promotion record saves `attribution_scope` and marks excluded-only commits `outside-vendor-scope`, in an unpaired group with -`refresh_policy: retain`. All source commits still undergo the linear-history +`refresh_policy: retain`. Temporary-fork source commits still undergo the linear-history and ancestry checks, and the promoted SHA and vendored-tree verification remain unchanged. Scoped attribution does not prove whole-commit equivalence or authorize dropping changes to excluded files during a future source-branch refresh. @@ -491,11 +535,19 @@ assertions but cannot accompany automatic matching. `--source-repo` selects a local immutable source checkout for materialization; `--worktree` overrides the follow-up checkout location. `--wait` optionally waits for and verifies merge. +For the verified upstream-main exception, omit all three attribution options +(`--from-pr-description`, `--upstream-pr`, `--unpaired-reason`). The engine infers +the mode from the reviewed lock and verifies live ancestry; no additional opt-in +flag or PR list is needed. If `--from-pr-description` is supplied, it is ignored +only for this qualifying mode. Omitting attribution for an ordinary fork update +still fails with instructions to provide it. + Publication checks the merged source PR, expected previous canonical lock, -linear fast-forward ancestry, current consumer pin, complete materialization, +fast-forward ancestry (linear for temporary forks), current consumer pin, complete materialization, and full lock-only diff. It uses non-force pushes and DCO-signed commits. The `VENDOR PROMOTION V1` JSON block in the commit/squash message records vendor, source/consumer identities, source SHAs, upstream snapshots, and attribution. +Upstream-main promotions use the V2 ancestry record described above instead. Historical `PRIMTS PROMOTION V1` messages remain readable. No provenance file is added to the consumer repository. diff --git a/scripts/vendor/bot.py b/scripts/vendor/bot.py index 8db4d5043b34..33cac8c6738f 100644 --- a/scripts/vendor/bot.py +++ b/scripts/vendor/bot.py @@ -450,6 +450,7 @@ def _feedback( problem: Diagnostic | None, *, attribution: dict[str, dict] | None = None, + upstream_proof: dict | None = None, no_promotion: bool = False, ) -> None: number = pr["number"] @@ -461,6 +462,18 @@ def _feedback( else "Vendor promotion metadata and source attribution are valid. " "This is **not** a code-review approval. Promotion still requires the source PR to merge." ) + if upstream_proof is not None and problem is None: + message = ( + "Upstream-main source ancestry is valid: pinned commit " + f"`{upstream_proof['promoted']}` contains canonical " + f"dev tip `{upstream_proof['checked_canonical_tip']}` and is reachable from " + f"the configured upstream branch at `{upstream_proof['upstream_observed_main']}`. " + f"No vendor-promotion metadata or upstream PR list is required for " + f"{field(self.gh.vendor_name)} in this mode. Promotion will fast-forward dev to " + "the exact reviewed pin, not the moving upstream tip, and restore the canonical " + "lock URL/branch. This is **not** a code-review approval. Normal review and CI " + "still apply; promotion requires the source PR to merge and ancestry to remain valid." + ) if no_promotion: message = ( f"This PR does not require vendor promotion for {field(self.gh.vendor_name)} " @@ -813,24 +826,28 @@ def inspect(self, number: int) -> None: if current.to_mapping() != reviewed.to_mapping(): self._transition(number, "ignored", "historical pin is no longer pending promotion") return - promote._check_current(self.gh, plan) try: self.progress.operation("validating_metadata", number) - promote._check_source_repo(self.gh, promote._repo_name(reviewed.url)) - promote._check_canonical_tip(self.gh, previous, reviewed) - entry = metadata.parse(pr.get("body") or "", self.gh.vendor_name, self.gh.upstream_repo) + if pr.get("merged"): + promote._check_current(self.gh, plan) + upstream_proof = promote._check_source(self.gh, previous, reviewed) + entry, evidence = None, None cache = self.args.workdir / "sources.git" - # Until provenance is committed, decisions follow current remote - # inputs, not whichever upstream heads a previous process cached. - _, evidence = metadata.resolve(self.gh, previous, reviewed, entry, cache) + if upstream_proof is None: + entry = metadata.parse( + pr.get("body") or "", self.gh.vendor_name, self.gh.upstream_repo + ) + # Until provenance is committed, decisions follow current remote + # inputs, not whichever upstream heads a previous process cached. + _, evidence = metadata.resolve(self.gh, previous, reviewed, entry, cache) # Attribution may take long enough for another consumer's promotion # to advance the shared canonical branch. Do not report stale inputs as valid. - promote._check_canonical_tip(self.gh, previous, reviewed) + upstream_proof = promote._check_source(self.gh, previous, reviewed) except Diagnostic as error: self._feedback(pr, error) return self.progress.operation("reconciling_feedback", number) - self._feedback(pr, None, attribution=evidence) + self._feedback(pr, None, attribution=evidence, upstream_proof=upstream_proof) if not pr.get("merged"): return # Re-read mutable description/head before any promotion. Immutable lock @@ -841,19 +858,20 @@ def inspect(self, number: int) -> None: number, "deferred", "source head or description changed; retry next poll" ) return - arguments.upstream_pr = [str(item) for item in entry["upstream_prs"]] - arguments.unpaired_reason = entry["unpaired_reason"] - arguments.source_repo = cache if cache.exists() else None + if entry is not None: + arguments.upstream_pr = [str(item) for item in entry["upstream_prs"]] + arguments.unpaired_reason = entry["unpaired_reason"] + arguments.source_repo = cache if cache.exists() else None self.progress.operation("planning_promotion", number) plan = promote._make_plan( arguments, self.gh, - auto_match=bool(entry["upstream_prs"]), - resolutions=entry["resolutions"], + auto_match=entry is not None and bool(entry["upstream_prs"]), + resolutions=entry["resolutions"] if entry is not None else None, ) # Attribution evidence is versioned with the durable promotion record. # Reuse the original published record on retries rather than replacing it. - if plan.completed_pr is None: + if plan.completed_pr is None and entry is not None: plan.record["contributor_metadata"] = { "source_head": pr["head"]["sha"], "metadata_digest": metadata.fingerprint(entry), diff --git a/scripts/vendor/metadata.py b/scripts/vendor/metadata.py index 544cc3dc3ee6..97a11f5bea7e 100644 --- a/scripts/vendor/metadata.py +++ b/scripts/vendor/metadata.py @@ -666,17 +666,7 @@ def resolve( cache: Path, ) -> tuple[dict[str, int | None], dict[str, dict]]: """Fetch only Git data into a private bare cache and resolve declared PRs.""" - if previous.source != reviewed.source or set(previous.include) != set(reviewed.include): - changed = (["source"] if previous.source != reviewed.source else []) + ( - ["include"] if set(previous.include) != set(reviewed.include) else [] - ) - raise Diagnostic( - "Automatic attribution requires unchanged lock source/include selection. " - "Changed fields: " + ", ".join(f"`{name}`" for name in changed) + ".", - "Separate the selection change into a different PR. In this source-update PR, " - "restore the canonical `source`/`include` selection in " - "`3rdparty/vendor_sources.lock.yaml` and regenerate the vendor snapshot.", - ) + promote._check_selection(previous, reviewed) cache.mkdir(parents=True, exist_ok=True) if not (cache / "HEAD").exists(): promote._git(cache, "init", "--bare", "--quiet") diff --git a/scripts/vendor/promote.py b/scripts/vendor/promote.py index c20083042483..7e112c4ee75c 100644 --- a/scripts/vendor/promote.py +++ b/scripts/vendor/promote.py @@ -242,6 +242,70 @@ def _check_source_repo(gh: GitHub, repo: str) -> None: ) from error +def _is_upstream_main(gh: GitHub, reviewed: vendor.Vendor) -> bool: + return ( + _repo_name(reviewed.url).lower() == gh.upstream_repo.lower() + and reviewed.branch == gh.upstream_branch + ) + + +def _check_selection(previous: vendor.Vendor, reviewed: vendor.Vendor) -> None: + if previous.source != reviewed.source or set(previous.include) != set(reviewed.include): + changed = (["source"] if previous.source != reviewed.source else []) + ( + ["include"] if set(previous.include) != set(reviewed.include) else [] + ) + raise Diagnostic( + "Automatic attribution requires unchanged lock source/include selection. " + "Changed fields: " + ", ".join(f"`{name}`" for name in changed) + ".", + "Separate the selection change into a different PR. In this source-update PR, " + "restore the canonical `source`/`include` selection in " + "`3rdparty/vendor_sources.lock.yaml` and regenerate the vendor snapshot.", + ) + + +def _check_source(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> dict | None: + """Validate live source ancestry; return proof only for the configured upstream branch.""" + repo = _repo_name(reviewed.url) + if repo.lower() != gh.upstream_repo.lower(): + _check_source_repo(gh, repo) + _check_canonical_tip(gh, previous, reviewed) + return None + if not _is_upstream_main(gh, reviewed): + raise Diagnostic( + f"An upstream source must use the configured branch {field(gh.upstream_branch)}.", + "Pin a commit reachable from that branch which contains the latest canonical " + "dev tip, or use a temporary branch in a personal fork based on that dev tip. " + "Update the lock and regenerate the vendored snapshot.", + ) + _check_selection(previous, reviewed) + tip = _check_canonical_tip(gh, previous, reviewed) + if ( + gh.compare(repo, previous.commit, reviewed.commit)["merge_base_commit"]["sha"] + != previous.commit + ): + raise Diagnostic( + "The upstream pin does not contain the previous canonical revision.", + "Choose an upstream pin containing the canonical history, or use the explicit " + "maintainer refresh workflow to reconcile divergent histories.", + ) + upstream = _sha(gh.get(f"repos/{repo}/git/ref/heads/{gh.upstream_branch}")["object"]["sha"]) + if gh.compare(repo, reviewed.commit, upstream)["merge_base_commit"]["sha"] != reviewed.commit: + raise Diagnostic( + f"The pinned source commit {field(reviewed.commit)} is not reachable from " + f"{field(repo)} / {field(gh.upstream_branch)} at `{upstream}`.", + "Pin a commit on the configured upstream branch that also contains the latest " + "canonical dev tip. If the changes are not upstream yet, publish them on a " + "temporary fork branch and provide vendor-promotion metadata instead. " + "Regenerate the lock and vendored snapshot for the chosen SHA.", + ) + return { + "promoted": reviewed.commit, + "upstream_base": previous.commit, + "checked_canonical_tip": tip, + "upstream_observed_main": upstream, + } + + def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> str: """Require the immutable source pin to contain the live canonical branch tip.""" repo = _repo_name(previous.url) @@ -254,6 +318,16 @@ def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.V "a maintainer must inspect the branch history before promotion." ) if gh.compare(repo, tip, reviewed.commit)["merge_base_commit"]["sha"] != tip: + if _is_upstream_main(gh, reviewed): + raise Diagnostic( + f"The pinned upstream commit {field(reviewed.commit)} does not contain " + f"the latest canonical branch tip {field(tip)}.", + "Select a newer upstream commit containing that exact dev tip, or use a " + "temporary fork branch based on the latest dev tip with vendor-promotion " + "metadata. If upstream and dev have diverged, a maintainer must use the " + "explicit refresh workflow. Update the lock and vendored snapshot; " + "equivalent patches under different SHAs do not establish Git ancestry.", + ) raise Diagnostic( f"The pinned temporary source commit {field(reviewed.commit)} does not contain " f"the latest canonical branch tip {field(tip)}.", @@ -273,10 +347,22 @@ def _record_from_message(message: str) -> dict: message = message.replace(legacy, _BEGIN).replace( "----- END PRIMTS PROMOTION V1 -----", _END ) - if message.count(_BEGIN) != 1 or message.count(_END) != 1: + begins = re.findall(r"----- BEGIN VENDOR PROMOTION V(\d+) -----", message) + ends = re.findall(r"----- END VENDOR PROMOTION V(\d+) -----", message) + if len(begins) != 1 or ends != begins: raise ValueError("Missing or ambiguous Vendor promotion metadata in commit message.") - record = json.loads(message.split(_BEGIN, 1)[1].split(_END, 1)[0]) - if not isinstance(record, dict) or record.get("version") != 1: + if begins[0] not in ("1", "2"): + raise ValueError("Unsupported Vendor promotion metadata version.") + version = int(begins[0]) + begin, end = _BEGIN.replace("V1", f"V{version}"), _END.replace("V1", f"V{version}") + if message.index(end) < message.index(begin): + raise ValueError("Promotion metadata end marker precedes its begin marker.") + record = json.loads(message.split(begin, 1)[1].split(end, 1)[0]) + if ( + not isinstance(record, dict) + or type(record.get("version")) is not int + or record["version"] != version + ): raise ValueError("Unsupported Vendor promotion metadata version.") if "trtllm_pr" in record: record["consumer_pr"] = record.pop("trtllm_pr") @@ -288,7 +374,9 @@ def _record_from_message(message: str) -> dict: def _record_text(record: dict) -> str: - return f"{_BEGIN}\n{json.dumps(record, indent=2, sort_keys=True)}\n{_END}" + version = record["version"] + begin, end = _BEGIN.replace("V1", f"V{version}"), _END.replace("V1", f"V{version}") + return f"{begin}\n{json.dumps(record, indent=2, sort_keys=True)}\n{end}" def _map_commits( @@ -360,7 +448,9 @@ def _check_current(gh: GitHub, plan: Promotion) -> None: raise ValueError( "Consumer branch's vendor lock has been superseded; do not promote this PR." ) - _check_canonical_tip(gh, plan.previous, plan.reviewed) + _check_source(gh, plan.previous, plan.reviewed) + if "checked_canonical_tip" in plan.record: + _verify_upstream_snapshot(gh, plan.record) def _plan_identity(args: argparse.Namespace, gh: GitHub) -> Promotion: @@ -407,6 +497,8 @@ def _plan_identity(args: argparse.Namespace, gh: GitHub) -> Promotion: "source_url": reviewed.url, "source_branch": reviewed.branch, } + if _is_upstream_main(gh, reviewed): + record.update(version=2, source_mode="upstream-main") return Promotion( number, _main_sha(gh), @@ -427,18 +519,9 @@ def _make_plan( auto_match: bool = False, resolutions: dict[str, dict] | None = None, ) -> Promotion: - if args.unpaired_reason is not None: - if not args.unpaired_reason.strip() or args.upstream_pr or args.map_upstream: - raise ValueError( - "--unpaired-reason requires a nonempty reason and cannot be combined " - "with --upstream-pr or --map-upstream." - ) - elif not args.upstream_pr: - raise ValueError("Provide --upstream-pr or an explicit --unpaired-reason.") plan = _plan_identity(args, gh) previous, reviewed, record = plan.previous, plan.reviewed, plan.record canonical_repo, fork = plan.canonical_repo, plan.fork - source_repo = _repo_name(reviewed.url) # A completed operation is historical: later promotions, deleted temporary # forks, or changed upstream PR state must not make it run again or fail. existing = _find_followup(gh, plan) @@ -449,8 +532,35 @@ def _make_plan( plan.completed_pr = existing return plan _check_source_repo(gh, canonical_repo) - _check_source_repo(gh, source_repo) + upstream_proof = _check_source(gh, previous, reviewed) _check_fork(gh, fork, gh.consumer_repo) + if upstream_proof is not None: + record.update(upstream_proof) + record["attribution_scope"] = {"source": reviewed.source, "include": list(reviewed.include)} + _check_current(gh, plan) + return plan + if getattr(args, "from_pr_description", False): + if __package__: + from . import metadata + else: + import metadata + pr = gh.get(f"repos/{gh.consumer_repo}/pulls/{plan.number}") + entry = metadata.parse(pr.get("body") or "", gh.vendor_name, gh.upstream_repo) + args.upstream_pr = [str(number) for number in entry["upstream_prs"]] + args.unpaired_reason = entry["unpaired_reason"] + resolutions = entry["resolutions"] + auto_match = True + if args.unpaired_reason is not None: + if not args.unpaired_reason.strip() or args.upstream_pr or args.map_upstream: + raise ValueError( + "--unpaired-reason requires a nonempty reason and cannot be combined " + "with --upstream-pr or --map-upstream." + ) + elif not args.upstream_pr: + raise ValueError( + "Temporary-fork updates require --from-pr-description, --upstream-pr, " + "or an explicit --unpaired-reason." + ) comparison = gh.compare(canonical_repo, previous.commit, reviewed.commit, all_commits=True) if comparison["merge_base_commit"]["sha"] != previous.commit: raise ValueError( @@ -624,10 +734,10 @@ def _recover_plan(args: argparse.Namespace, gh: GitHub, plan: Promotion) -> bool ) message = _git(args.repo, "show", "-s", "--format=%B", sha) _adopt_record(plan, message) + _verify_upstream_snapshot(gh, plan.record) if not re.search(r"^Signed-off-by: .+ <.+>$", message, re.MULTILINE): raise ValueError("Local promotion commit has no DCO sign-off.") _check_source_repo(gh, plan.canonical_repo) - _check_source_repo(gh, _repo_name(plan.reviewed.url)) _check_fork(gh, plan.fork, gh.consumer_repo) _check_current(gh, plan) return True @@ -636,7 +746,12 @@ def _recover_plan(args: argparse.Namespace, gh: GitHub, plan: Promotion) -> bool def _adopt_record(plan: Promotion, message: str) -> None: """Keep the original upstream snapshot when resuming an already-published run.""" existing = _record_from_message(message) - stable_keys = set(plan.record) - {"upstream_base", "upstream_observed_main", "changes"} + stable_keys = set(plan.record) - { + "upstream_base", + "upstream_observed_main", + "checked_canonical_tip", + "changes", + } if any(existing.get(key) != plan.record[key] for key in stable_keys): raise ValueError("Existing promotion metadata does not describe this source update.") _validate_snapshot(existing, plan.record["upstream_repository"]) @@ -679,6 +794,20 @@ def _validate_snapshot(record: dict, upstream_repo: str) -> None: raise ValueError("Invalid promotion attribution scope.") vendor._validate_relative_path(scope["source"], "Attribution source") vendor._validate_include(scope["include"], record["vendor"]) + if record.get("version") == 2: + if ( + record.get("source_mode") != "upstream-main" + or _repo_name(record["source_url"]).lower() != upstream_repo.lower() + or record["source_branch"] != record["upstream_branch"] + or record["upstream_base"] != record["previous"] + or "attribution_scope" not in record + or "changes" in record + ): + raise ValueError("Invalid upstream-main promotion provenance.") + _sha(record["checked_canonical_tip"]) + _sha(record["previous"]) + _sha(record["promoted"]) + return commits = [] if not isinstance(record["changes"], list) or not record["changes"]: raise ValueError("Promotion metadata must contain source changes.") @@ -704,6 +833,19 @@ def _validate_snapshot(record: dict, upstream_repo: str) -> None: raise ValueError("Promotion metadata has duplicate or missing source commits.") +def _verify_upstream_snapshot(gh: GitHub, record: dict) -> None: + """Verify recorded immutable ancestry, without consulting mutable branch tips.""" + if record.get("version") != 2: + return + chain = [ + record[key] + for key in ("previous", "checked_canonical_tip", "promoted", "upstream_observed_main") + ] + for base, head in zip(chain, chain[1:]): + if gh.compare(gh.upstream_repo, base, head)["merge_base_commit"]["sha"] != base: + raise ValueError("Recorded upstream-main promotion ancestry is invalid.") + + def _verify_remote_commit(gh: GitHub, plan: Promotion, repo: str, sha: str) -> str: commit = gh.get(f"repos/{repo}/commits/{_sha(sha)}") if len(commit["parents"]) != 1 or [item["filename"] for item in commit["files"]] != [_LOCK]: @@ -719,6 +861,7 @@ def _verify_remote_commit(gh: GitHub, plan: Promotion, repo: str, sha: str) -> s _check_document_change(before, after, plan) message = commit["commit"]["message"] _adopt_record(plan, message) + _verify_upstream_snapshot(gh, plan.record) if not re.search(r"^Signed-off-by: .+ <.+>$", message, re.MULTILINE): raise ValueError("Promotion commit has no DCO sign-off.") return message @@ -991,7 +1134,14 @@ def _arm_auto_merge(gh: GitHub, plan: Promotion, pr: dict, message: str) -> None def _pr_body(plan: Promotion, message: str) -> str: unpaired = "" - if any(group["upstream_pr"] is None for group in plan.record["changes"]): + if plan.record.get("source_mode") == "upstream-main": + unpaired = ( + "This source pin is reachable from the configured upstream branch and contains " + "the checked canonical dev tip. Version 2 provenance records that immutable " + "ancestry instead of upstream PR mappings. A future refresh must verify that " + "its chosen upstream target contains this pin before omitting its commits.\n\n" + ) + elif any(group["upstream_pr"] is None for group in plan.record["changes"]): unpaired = ( "Some source commits have no paired upstream PR. Their provenance " "records an explicit reason and `refresh_policy: retain`, including commits " @@ -1145,7 +1295,12 @@ def main(argv: list[str] | None = None) -> int: """Run the maintainer CLI; dry-run unless publication is explicitly requested.""" parser = argparse.ArgumentParser(description=__doc__) commands = parser.add_subparsers(dest="command", required=True) - promote = commands.add_parser("promote", help="Promote a merged consumer source update.") + promote = commands.add_parser( + "promote", + help="Promote a merged consumer source update.", + epilog="Attribution options are required for temporary-fork updates, but not for a " + "pin on the configured upstream branch that contains the live canonical dev tip.", + ) promote.add_argument("--source-pr", required=True) promote.add_argument("--vendor", required=True, help="Vendor key in the lock file.") promote.add_argument("--consumer-repo", default="NVIDIA/TensorRT-LLM") @@ -1162,7 +1317,7 @@ def main(argv: list[str] | None = None) -> int: required=True, help="Expected canonical branch; specify a dated branch when applicable.", ) - provenance = promote.add_mutually_exclusive_group(required=True) + provenance = promote.add_mutually_exclusive_group() provenance.add_argument("--upstream-pr", action="append", default=[]) provenance.add_argument( "--from-pr-description", @@ -1218,25 +1373,12 @@ def main(argv: list[str] | None = None) -> int: args.base_branch, args.upstream_branch, ) - resolutions = None - if args.from_pr_description: - if __package__: - from . import metadata - else: - import metadata - number = _pr_number(args.source_pr, gh.consumer_repo) - pr = gh.get(f"repos/{gh.consumer_repo}/pulls/{number}") - entry = metadata.parse(pr.get("body") or "", gh.vendor_name, gh.upstream_repo) - args.upstream_pr = [str(number) for number in entry["upstream_prs"]] - args.unpaired_reason = entry["unpaired_reason"] - resolutions = entry["resolutions"] if args.map_upstream and (args.auto_match or args.from_pr_description): raise ValueError("Explicit --map-upstream cannot accompany automatic matching.") plan = _make_plan( args, gh, auto_match=args.auto_match or args.from_pr_description, - resolutions=resolutions, ) mode = ( "ALREADY COMPLETED" if plan.completed_pr else "PUBLISH" if args.publish else "DRY RUN" diff --git a/tests/unittest/others/test_maintain_prims_ts.py b/tests/unittest/others/test_maintain_prims_ts.py index ee4441759b2f..9ab55b953a7e 100644 --- a/tests/unittest/others/test_maintain_prims_ts.py +++ b/tests/unittest/others/test_maintain_prims_ts.py @@ -765,7 +765,7 @@ def advance(*arguments): assert not any(call[0] == "push" for call in world.git_calls) -def test_manual_promotion_rejects_upstream_source( +def test_manual_promotion_rejects_pin_not_on_upstream_branch( world: World, monkeypatch: pytest.MonkeyPatch ) -> None: plan = m._plan_identity(world.args(), world) @@ -773,11 +773,361 @@ def test_manual_promotion_rejects_upstream_source( plan.reviewed, url=f"https://github.com/{_UPSTREAM}.git", branch="main" ) monkeypatch.setattr(m, "_plan_identity", lambda *args: plan) - with pytest.raises(m.Diagnostic, match="not upstream itself"): + with pytest.raises(m.Diagnostic, match="not reachable"): m._make_plan(world.args(), world) assert not any(method != "GET" for method, _, _ in world.calls) +def _pin_upstream(world: World, *, branch: str = "main") -> None: + """Make the source update pin upstream directly, preserving the canonical parent.""" + _write( + world.consumer, + "vendored/prims_ts/kernel.py", + _git(world.source, "show", f"{world.reviewed}:prims_ts/kernel.py") + "\n", + ) + assert ( + m.vendor.main( + [ + "--lock", + str(world.consumer / m._LOCK), + "pin", + _VENDOR, + "--url", + f"https://github.com/{_UPSTREAM}.git", + "--branch", + branch, + "--commit", + world.reviewed, + "--repo", + str(world.source), + ] + ) + == 0 + ) + _git(world.consumer, "add", "-A") + _git(world.consumer, "commit", "--amend", "-q", "-s", "--no-edit") + world.merged = world.main = _git(world.consumer, "rev-parse", "HEAD") + world.upstream = world.reviewed + world.source_body = "No contributor metadata is necessary for this upstream pin." + + +def _bot_args(world: World, **kwargs: object) -> argparse.Namespace: + return world.args( + workdir=world.root, + since=None, + refresh_branch=[], + migrate_canonical_branch_from=None, + **kwargs, + ) + + +@pytest.mark.parametrize("merge_commit", [False, True]) +def test_upstream_main_promotes_exact_pin_without_pr_list(world: World, merge_commit: bool) -> None: + if merge_commit: + _git(world.source, "switch", "-q", "-c", "side", world.first) + _write(world.source, "side.txt", "upstream side branch\n") + _commit(world.source, "side change") + _git(world.source, "switch", "-q", "main") + _git(world.source, "merge", "--no-ff", "-q", "-m", "merge upstream side", "side") + world.reviewed = _git(world.source, "rev-parse", "HEAD") + _pin_upstream(world) + _write(world.source, "later.txt", "not in the reviewed pin\n") + world.upstream = _commit(world.source, "later upstream update") + args = world.args(upstream_pr=[], publish=True) + plan = m._make_plan(args, world) + assert plan.record["version"] == 2 + assert plan.record["source_mode"] == "upstream-main" + assert plan.record["checked_canonical_tip"] == world.previous + assert plan.record["upstream_observed_main"] == world.upstream + assert "changes" not in plan.record + assert "unpaired_reason" not in plan.record + m._validate_snapshot(plan.record, _UPSTREAM) + m._verify_upstream_snapshot(world, plan.record) + pr = m._publish(args, world, plan) + assert _git(world.canonical, "rev-parse", "trtllm-prims-ts-dev") == world.reviewed + assert ( + _git(world.consumer, "diff", "--name-only", f"{pr['head']['sha']}^", pr["head"]["sha"]) + == m._LOCK + ) + assert m._lock_at(world, _CONSUMER, pr["head"]["sha"]).to_mapping() == plan.promoted + assert ( + m._record_from_message(_git(world.consumer, "show", "-s", "--format=%B", pr["head"]["sha"])) + == plan.record + ) + assert "Version 2 provenance" in pr["body"] + assert not any(route.startswith(f"repos/{_UPSTREAM}/pulls/") for _, route, _ in world.calls) + + +def test_upstream_exception_uses_configured_branch(world: World) -> None: + world.upstream_branch = "stable" + _pin_upstream(world, branch="stable") + plan = m._make_plan(world.args(upstream_pr=[]), world) + assert plan.record["source_mode"] == "upstream-main" + assert plan.record["source_branch"] == plan.record["upstream_branch"] == "stable" + + +def test_temporary_fork_still_requires_linear_history( + world: World, monkeypatch: pytest.MonkeyPatch +) -> None: + compare = world.compare + + def merged_range(repo, base, head, **kwargs): + result = compare(repo, base, head, **kwargs) + if kwargs.get("all_commits"): + result["commits"][-1]["parents"].append({"sha": world.previous}) + return result + + monkeypatch.setattr(world, "compare", merged_range) + with pytest.raises(ValueError, match="linear source commit range"): + m._make_plan(world.args(), world) + assert all(method == "GET" for method, _, _ in world.calls) + + +@pytest.mark.parametrize("description_flag", [False, True]) +def test_upstream_main_cli_does_not_require_metadata( + world: World, capfd: pytest.CaptureFixture, description_flag: bool +) -> None: + _pin_upstream(world) + world.source_body = " malformed" + options = ["--from-pr-description"] if description_flag else [] + assert ( + m.main( + [ + "promote", + "--vendor", + _VENDOR, + "--upstream-repo", + _UPSTREAM, + "--source-pr", + "17", + "--canonical-repo", + "maintainer/flashinfer", + "--canonical-branch", + "trtllm-prims-ts-dev", + "--repo", + str(world.consumer), + *options, + ] + ) + == 0 + ) + assert '"source_mode": "upstream-main"' in capfd.readouterr().out + assert all(method == "GET" for method, _, _ in world.calls) + + +@pytest.mark.parametrize("failure", ["wrong-branch", "not-reachable", "diverged", "selection"]) +def test_upstream_main_exception_is_narrow(world: World, failure: str) -> None: + _pin_upstream(world, branch="release" if failure == "wrong-branch" else "main") + if failure == "not-reachable": + world.upstream = world.previous + elif failure == "diverged": + _git(world.source, "switch", "-q", "--detach", world.previous) + _advance_canonical(world) + elif failure == "selection": + data = m._local_lock_document(world.consumer) + data["vendors"][_VENDOR]["include"] = ["kernel.py"] + _write(world.consumer, m._LOCK, m.yaml.safe_dump(data)) + _git(world.consumer, "add", "-A") + _git(world.consumer, "commit", "--amend", "-q", "-s", "--no-edit") + world.merged = world.main = _git(world.consumer, "rev-parse", "HEAD") + messages = { + "wrong-branch": "configured branch", + "not-reachable": "not reachable", + "diverged": "latest canonical branch tip", + "selection": "unchanged lock source/include", + } + with pytest.raises(m.Diagnostic, match=messages[failure]): + m._make_plan(world.args(upstream_pr=[]), world) + assert all(method == "GET" for method, _, _ in world.calls) + assert not any(call[0] == "push" for call in world.git_calls) + + +@pytest.mark.parametrize("movement", ["dev", "upstream"]) +def test_upstream_main_rechecks_refs_before_publication( + world: World, monkeypatch: pytest.MonkeyPatch, movement: str +) -> None: + _pin_upstream(world) + args = world.args(upstream_pr=[], publish=True) + plan = m._make_plan(args, world) + prepare = m._prepare_commit + + def move_ref(*arguments): + result = prepare(*arguments) + if movement == "dev": + _advance_canonical(world) + else: + world.upstream = world.previous + return result + + monkeypatch.setattr(m, "_prepare_commit", move_ref) + with pytest.raises(m.Diagnostic): + m._publish(args, world, plan) + assert all(method == "GET" for method, _, _ in world.calls) + assert not any(call[0] == "push" for call in world.git_calls) + + +@pytest.mark.parametrize("body", ["", " malformed"]) +@pytest.mark.parametrize("consumer_base", ["main", "release/1.3"]) +def test_open_upstream_main_needs_no_block( + world: World, monkeypatch: pytest.MonkeyPatch, body: str, consumer_base: str +) -> None: + _pin_upstream(world) + world.base_branch = consumer_base + previous = m._lock_at(world, _CONSUMER, world.base) + reviewed = m._lock_at(world, _CONSUMER, world.merged) + endpoint = f"repos/{_CONSUMER}/pulls/17" + pr = world.get(endpoint) + pr.update( + merged=False, + state="open", + body=body, + base={"ref": consumer_base}, + user={"login": "maintainer"}, + ) + world.overrides[endpoint] = pr + + def no_matching(*unused): + raise AssertionError("Upstream ancestry must not invoke patch/PR matching") + + monkeypatch.setattr(b.metadata, "resolve", no_matching) + with contextlib.closing(b.Store(":memory:")) as store: + monitor = b.Monitor(_bot_args(world, publish=True), world, store) + monkeypatch.setattr(monitor, "_inputs", lambda source_pr: (previous, reviewed)) + monitor.inspect(17) + assert store.get("observation:17")["state"] == "metadata_valid" + assert "Upstream-main source ancestry is valid" in world.source_comments[0]["body"] + assert "No vendor-promotion metadata" in world.source_comments[0]["body"] + assert not world.prs + + +def test_upstream_main_rechecks_dev_before_valid_feedback( + world: World, monkeypatch: pytest.MonkeyPatch +) -> None: + _pin_upstream(world) + previous = m._lock_at(world, _CONSUMER, world.base) + reviewed = m._lock_at(world, _CONSUMER, world.merged) + endpoint = f"repos/{_CONSUMER}/pulls/17" + pr = world.get(endpoint) + pr.update(merged=False, state="open", user={"login": "maintainer"}) + world.overrides[endpoint] = pr + api = world.api + + def advance(endpoint, **kwargs): + result = api(endpoint, **kwargs) + if endpoint == f"repos/{_UPSTREAM}/git/ref/heads/main": + _advance_canonical(world) + return result + + monkeypatch.setattr(world, "api", advance) + with contextlib.closing(b.Store(":memory:")) as store: + monitor = b.Monitor(_bot_args(world, publish=True), world, store) + monkeypatch.setattr(monitor, "_inputs", lambda source_pr: (previous, reviewed)) + monitor.inspect(17) + assert store.get("observation:17")["state"] == "blocked" + assert "latest canonical branch tip" in world.source_comments[0]["body"] + assert "ancestry is valid" not in world.source_comments[0]["body"] + assert not world.prs + + +@pytest.mark.parametrize("completed", [False, True]) +def test_upstream_main_recovers_without_sqlite_and_preserves_proof( + world: World, completed: bool +) -> None: + _pin_upstream(world) + args = world.args(upstream_pr=[], publish=True, auto_merge=True) + if completed: + world.state = "CLEAN" + plan = m._make_plan(args, world) + m._publish(args, world, plan) + original_record = copy.deepcopy(plan.record) + _write(world.source, "later.txt", "upstream advances after promotion\n") + world.upstream = _commit(world.source, "new upstream tip") + world.source_body = " invalid edited metadata" + world.calls.clear() + with contextlib.closing(b.Store(":memory:")) as store: + b.Monitor(_bot_args(world, publish=True), world, store).inspect(17) + if completed: + assert store.get("complete:17") + resumed = m._make_plan(args, world) + m._publish(args, world, resumed) + assert resumed.record == original_record + assert len(world.prs) == len(world.comments) == 1 + assert not world.source_comments + assert not any(route.startswith(f"repos/{_UPSTREAM}/pulls/") for _, route, _ in world.calls) + + +def test_bot_promotes_merged_upstream_pin_without_metadata(world: World) -> None: + _pin_upstream(world) + with contextlib.closing(b.Store(":memory:")) as store: + b.Monitor(_bot_args(world, publish=True), world, store).inspect(17) + pr = world.prs[99] + record = m._record_from_message(pr["auto_merge"]["commit_message"]) + assert record["version"] == 2 + assert record["promoted"] == world.reviewed + assert "contributor_metadata" not in record + assert "changes" not in record + assert _git(world.canonical, "rev-parse", "trtllm-prims-ts-dev") == world.reviewed + assert [item["body"] for item in world.comments] == [m._CI_SKIP_COMMAND] + assert "No vendor-promotion metadata" in world.source_comments[0]["body"] + + +@pytest.mark.parametrize("tamper", ["mode", "source", "tip", "upstream", "mixed-version"]) +def test_upstream_main_recovery_rejects_tampered_proof(world: World, tamper: str) -> None: + _pin_upstream(world) + args = world.args(upstream_pr=[], publish=True) + plan = m._make_plan(args, world) + pr = m._publish(args, world, plan) + record = copy.deepcopy(plan.record) + if tamper == "mode": + record["source_mode"] = "temporary-fork" + elif tamper == "source": + record["source_url"] = "https://github.com/dev/flashinfer.git" + elif tamper == "tip": + record["checked_canonical_tip"] = _advance_canonical(world) + elif tamper == "upstream": + record["upstream_observed_main"] = world.previous + message = m._record_text(record) + if tamper == "mixed-version": + message += m._record_text({"version": 1}) + endpoint = f"repos/{_CONSUMER}/commits/{pr['head']['sha']}" + commit = world.get(endpoint) + commit["commit"]["message"] = ( + message + "\nSigned-off-by: Promotion Test \n" + ) + world.overrides[endpoint] = commit + world.calls.clear() + world.git_calls.clear() + with pytest.raises(ValueError): + m._recover_plan(args, world, m._plan_identity(args, world)) + assert all(method == "GET" for method, _, _ in world.calls) + assert not any(call[0] == "push" for call in world.git_calls) + + +def test_upstream_main_still_checks_materialization(world: World) -> None: + _pin_upstream(world) + _write(world.consumer, "vendored/prims_ts/kernel.py", "VALUE = 'not in upstream'\n") + _git(world.consumer, "add", "-A") + _git(world.consumer, "commit", "--amend", "-q", "-s", "--no-edit") + world.merged = world.main = _git(world.consumer, "rev-parse", "HEAD") + args = world.args(upstream_pr=[], publish=True) + with pytest.raises(m.vendor.VendorError): + m._publish(args, world, m._make_plan(args, world)) + assert all(method == "GET" for method, _, _ in world.calls) + assert not any(call[0] == "push" for call in world.git_calls) + + +@pytest.mark.parametrize("version", ["0", "02", "3"]) +def test_promotion_record_rejects_unsupported_marker_version(version: str) -> None: + message = m._record_text({"version": 2}).replace("V2", f"V{version}") + with pytest.raises(ValueError, match="Unsupported"): + m._record_from_message(message) + + +def test_promotion_record_rejects_reversed_markers() -> None: + with pytest.raises(ValueError, match="precedes"): + m._record_from_message(m._END + "\n" + m._BEGIN + '\n{"version": 1}') + + @pytest.mark.parametrize("consumer_base", ["main", "release/1.3"]) @pytest.mark.parametrize( "case", ["stale", "diverged", "advance-during-attribution", "contained", "upstream"] @@ -835,7 +1185,7 @@ def resolve(*arguments): assert store.get("observation:17")["state"] == "blocked" assert "metadata and source attribution are valid" not in message if case == "upstream": - assert "not upstream itself" in message + assert "not reachable" in message else: assert "latest canonical branch tip" in message assert "rebase" in message and "scripts/vendor/manage.py" in message @@ -1148,7 +1498,8 @@ def test_unpaired_promotion_publishes_and_resumes(world: World) -> None: def test_unpaired_options_require_explicit_unambiguous_reason(world: World, options: dict) -> None: with pytest.raises(ValueError, match="unpaired-reason"): m._make_plan(world.args(**options), world) - assert not world.calls + # Repository identity must be read to determine whether attribution is required. + assert all(method == "GET" for method, _, _ in world.calls) @pytest.mark.parametrize("same_fork", [False, True]) diff --git a/tests/unittest/others/test_vendor_bot.py b/tests/unittest/others/test_vendor_bot.py index 30541eea41c1..1e34b4ae0b7c 100644 --- a/tests/unittest/others/test_vendor_bot.py +++ b/tests/unittest/others/test_vendor_bot.py @@ -1247,6 +1247,64 @@ def test_valid_feedback_is_rechecked_when_only_dev_tip_changes( assert monitor.gh.reviews[0]["state"] == "CHANGES_REQUESTED" +def test_upstream_main_reconciles_obsolete_review_without_sqlite( + monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch +) -> None: + monitor._feedback(monitor.gh.pr, _issue("Upstream is not a personal fork")) + previous = _vendor("a" * 40, "https://github.com/operator/library.git", "dev") + reviewed = _vendor("b" * 40, "https://github.com/org/library.git", "main") + api = monitor.gh.api + upstream_tip = "c" * 40 + + def get(endpoint, **kwargs): + if endpoint == "repos/org/library/git/ref/heads/main": + return {"object": {"sha": upstream_tip}} + return api(endpoint, **kwargs) + + monkeypatch.setattr(monitor.gh, "api", get) + monkeypatch.setattr( + monitor.gh, "compare", lambda repo, base, head: {"merge_base_commit": {"sha": base}} + ) + with contextlib.closing(b.Store(":memory:")) as store: + restarted = b.Monitor(monitor.args, monitor.gh, store) + monkeypatch.setattr(restarted, "_inputs", lambda pr: (previous, reviewed)) + restarted.inspect(7) + assert store.get("observation:7")["state"] == "metadata_valid" + assert len(monitor.gh.comments) == len(monitor.gh.reviews) == 1 + assert monitor.gh.reviews[0]["state"] == "DISMISSED" + assert "Upstream-main source ancestry is valid" in monitor.gh.comments[0]["body"] + assert reviewed.commit in monitor.gh.comments[0]["body"] + writes = copy.deepcopy(monitor.gh.writes) + restarted.inspect(7) + assert monitor.gh.writes == writes + + # Same PR and pin, but upstream no longer contains it: invalidate feedback. + monkeypatch.setattr( + monitor.gh, + "compare", + lambda repo, base, head: {"merge_base_commit": {"sha": previous.commit}}, + ) + restarted.inspect(7) + assert store.get("observation:7")["state"] == "blocked" + assert len(monitor.gh.comments) == 1 + assert "not reachable" in monitor.gh.comments[0]["body"] + assert monitor.gh.reviews[-1]["state"] == "CHANGES_REQUESTED" + + +def test_upstream_exception_does_not_waive_other_vendors_metadata( + monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch +) -> None: + # The description mentions an upstream-main update for another vendor only. + monitor.gh.pr["body"] = m.template("other-vendor", "org/library") + previous = _vendor("a" * 40, "https://github.com/operator/library.git", "dev") + reviewed = _vendor("b" * 40, "https://github.com/author/library.git", "main") + monkeypatch.setattr(monitor, "_inputs", lambda pr: (previous, reviewed)) + monitor.inspect(7) + assert monitor.store.get("observation:7")["state"] == "blocked" + assert "vendors.toy" in monitor.gh.comments[0]["body"] + assert monitor.gh.reviews[0]["state"] == "CHANGES_REQUESTED" + + @pytest.mark.parametrize("change", ["other-vendor", "patch-only", "vendor-created"]) def test_refresh_ignores_prs_without_selected_source_pin_change( monitor: b.Monitor, monkeypatch: pytest.MonkeyPatch, change: str From b1152623bbaec96b9fb5f3fd2a9437873999ae43 Mon Sep 17 00:00:00 2001 From: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Date: Thu, 22 Oct 2026 10:55:39 +0000 Subject: [PATCH 3/7] [None][fix] Publish actionable vendor validation updates as new comments Post meaningful validation changes as new author-mentioned comments with source snapshots and links to superseded results. Keep GitHub-backed deduplication and recovery idempotent, and explain branch ancestry failures with actionable next steps. Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> --- scripts/vendor/README.md | 28 +- scripts/vendor/bot.py | 207 ++++++---- scripts/vendor/diagnostics.py | 3 + scripts/vendor/metadata.py | 22 +- scripts/vendor/promote.py | 139 ++++--- .../unittest/others/test_maintain_prims_ts.py | 35 +- tests/unittest/others/test_vendor_bot.py | 368 +++++++++++++++++- 7 files changed, 638 insertions(+), 164 deletions(-) diff --git a/scripts/vendor/README.md b/scripts/vendor/README.md index 08ab87f2ca4b..4c41793bb5cb 100644 --- a/scripts/vendor/README.md +++ b/scripts/vendor/README.md @@ -464,7 +464,7 @@ local consumer checkout; no code or hooks from source forks are executed. ## Feedback and review safety -With `--publish`, recognized author-actionable problems produce an updatable +With `--publish`, recognized author-actionable problems produce a new comment containing the specific problem, its location, and a **Fix** section. Examples are included only when relevant, using the configured vendor and upstream repository. Missing metadata gets a complete copyable marked block; @@ -473,6 +473,23 @@ nested resolution example. Existing entries for other vendors must be preserved. Edit the **PR description**, not a comment, when correcting metadata. The bot rechecks automatically on later polls; no manual recheck comment is required. +Each comment is headed **Vendor validation**, mentions the PR author, identifies +the checked PR revision and source pin when available, and links to the previous +result it supersedes. The bot posts a new comment when the source URL/branch/pin, +problem, fix instructions, or validation status changes. Earlier comments remain +unchanged snapshots; the bot's marked reviews link to the newest result with +**Latest vendor validation and step-by-step fix**. + +Unchanged polls do not post again. Moving observed dev/upstream tips alone or +unrelated consumer commits do not notify unless they change the validation +result. The bot still revalidates against live tips on each poll. Deduplication +compares a versioned fingerprint in the newest bot-owned GitHub comment, not +SQLite or every historical result: A → B → A posts three results. This also +recovers after a lost POST response or a restart without SQLite. Legacy comments +without a fingerprint are preserved and superseded once by the new format. +Only comments/reviews from the configured account with the vendor marker count; +manual feedback and other accounts/vendors are not modified. + | Problem | What the bot tells the author to fix | |---------|-------------------------------------| | Missing markers | Names exactly which of `` and `` is missing, and where to insert it. | @@ -480,7 +497,8 @@ rechecks automatically on later polls; no manual recheck comment is required. | Malformed YAML or schema | Reports YAML line/column or the affected field path and the expected structure, without copying raw parser excerpts. YAML line 1 is the first line inside the fence. | | Invalid upstream PR or resolution | Identifies the list entry, duplicated PR, source SHA, missing reason, or unlisted PR, and gives the correction. | | Unresolved attribution | Shows candidate PRs and similarity scores where applicable; explains how to correct the PR list/source pin or supply a targeted author assertion. | -| Source-history or lock-selection problem | Explains the rebase or lock-file change and vendor regeneration needed. Rebase guidance identifies the canonical pin. Metadata edits alone cannot fix these failures. | +| Source-history or lock-selection problem | Explains the rebase or lock-file change and vendor regeneration needed. Rebase guidance targets the latest canonical dev tip, not just the older consumer lock pin. Merge diagnostics explain the linear-history requirement for temporary-fork attribution. Metadata edits alone cannot fix these failures. | +| Upstream source fails the exception | Reports the configured-branch, latest-dev ancestry, and upstream-reachability checks together, with observed SHAs and comparison links. Gives both the temporary-fork recovery path and the requirements for using upstream directly; renaming the lock branch alone cannot repair ancestry. | | Same-commit temporary reference | If the PR redirects the source URL/branch but pins the current canonical commit, asks the author to restore the canonical URL/branch from its target base. Keep the commit, other lock fields, vendored files, and patch unchanged; no upstream-PR metadata is needed. | `unpaired_reason` is appropriate only when there genuinely is no paired upstream @@ -500,10 +518,10 @@ configured refresh targets are not treated as unnecessary temporary redirects. After correction it dismisses only its own marked metadata reviews, when authorized. If an open PR no longer needs promotion, it also reconciles existing -comments/reviews from GitHub, without requiring SQLite history. It updates the -comment to explain that the previous request no longer applies, rather than +comments/reviews from GitHub, without requiring SQLite history. It posts a new +result to explain that the previous request no longer applies, rather than claiming metadata validation succeeded. It posts no feedback on an ignored PR -unless it already has a bot comment or an unresolved bot review to reconcile. +unless it already has a bot comment or a marked bot review to reconcile. If dismissal is forbidden, an authorized human must dismiss the review; the bot reports the failure rather than approving the code. If the authenticated account already has a manual approval/objection, the bot uses diff --git a/scripts/vendor/bot.py b/scripts/vendor/bot.py index 33cac8c6738f..c3eb05be2a82 100644 --- a/scripts/vendor/bot.py +++ b/scripts/vendor/bot.py @@ -35,6 +35,7 @@ _STATE_FILE = "state.sqlite" _STOP = threading.Event() _HEARTBEAT_SECONDS = 30 +_FEEDBACK_KEY = re.compile(r"") def _now() -> str: @@ -53,6 +54,20 @@ def _summary(error: object) -> str: return text[:500] +def _upstream_feedback(proof: dict, vendor_name: str) -> str: + return ( + "Upstream-main source ancestry is valid: pinned commit " + f"`{proof['promoted']}` contains canonical " + f"dev tip `{proof['checked_canonical_tip']}` and is reachable from " + f"the configured upstream branch at `{proof['upstream_observed_main']}`. " + f"No vendor-promotion metadata or upstream PR list is required for " + f"{field(vendor_name)} in this mode. Promotion will fast-forward dev to " + "the exact reviewed pin, not the moving upstream tip, and restore the canonical " + "lock URL/branch. This is **not** a code-review approval. Normal review and CI " + "still apply; promotion requires the source PR to merge and ancestry to remain valid." + ) + + class Store: """Disposable reconciliation cache; Git/GitHub own promotion provenance and state.""" @@ -452,27 +467,32 @@ def _feedback( attribution: dict[str, dict] | None = None, upstream_proof: dict | None = None, no_promotion: bool = False, + source: manage.Vendor | None = None, ) -> None: number = pr["number"] state = "blocked" if problem else "ignored" if no_promotion else "metadata_valid" message = ( - f"@{pr['user']['login']} — vendor promotion needs {problem.owner} action:\n\n{problem}" + f"Vendor promotion needs {problem.owner} action:\n\n{problem}" "\n\nThe bot will recheck automatically on a later poll." if problem else "Vendor promotion metadata and source attribution are valid. " "This is **not** a code-review approval. Promotion still requires the source PR to merge." ) + notification_message = message + if problem is not None and problem.notification_text is not None: + notification_message = ( + f"Vendor promotion needs {problem.owner} action:\n\n{problem.notification_text}" + "\n\nThe bot will recheck automatically on a later poll." + ) if upstream_proof is not None and problem is None: - message = ( - "Upstream-main source ancestry is valid: pinned commit " - f"`{upstream_proof['promoted']}` contains canonical " - f"dev tip `{upstream_proof['checked_canonical_tip']}` and is reachable from " - f"the configured upstream branch at `{upstream_proof['upstream_observed_main']}`. " - f"No vendor-promotion metadata or upstream PR list is required for " - f"{field(self.gh.vendor_name)} in this mode. Promotion will fast-forward dev to " - "the exact reviewed pin, not the moving upstream tip, and restore the canonical " - "lock URL/branch. This is **not** a code-review approval. Normal review and CI " - "still apply; promotion requires the source PR to merge and ancestry to remain valid." + message = _upstream_feedback(upstream_proof, self.gh.vendor_name) + notification_message = _upstream_feedback( + upstream_proof + | { + "checked_canonical_tip": "LATEST_DEV_TIP", + "upstream_observed_main": "UPSTREAM_TIP", + }, + self.gh.vendor_name, ) if no_promotion: message = ( @@ -481,11 +501,13 @@ def _feedback( "no longer applies; no upstream-PR metadata is required by this monitor. " "This is **not** a code-review approval. Normal review and CI still apply." ) + notification_message = message if attribution: details = metadata.similarity_feedback(attribution, self.gh.upstream_repo) if details: message += "\n\n" + details - body = f"{self.marker}\n{message}" + notification_message += "\n\n" + details + head = promote._sha(pr["head"]["sha"]) dismissal_notice = ( "\n\nAn authorized reviewer must dismiss the automated metadata " "review; this account lacks dismissal permission." @@ -517,8 +539,7 @@ def _feedback( for item in comments if item["user"]["login"] == self.actor and item.get("body", "").startswith(self.marker) ] - if len(existing) > 1: - raise ValueError("Multiple bot feedback comments; inspect before continuing.") + latest = max(existing, key=lambda item: int(item["id"]), default=None) reviews = ( [ item @@ -528,69 +549,24 @@ def _feedback( if current["state"] == "open" and current["user"]["login"] != self.actor else [] ) - automated = [ + marked_reviews = [ item for item in reviews - if item.get("body", "").startswith(self.marker) and item["state"] == "CHANGES_REQUESTED" + if item.get("body", "").startswith(self.marker) + and item["state"] in ("CHANGES_REQUESTED", "DISMISSED") ] + automated = [item for item in marked_reviews if item["state"] == "CHANGES_REQUESTED"] manual = [ item for item in reviews if not item.get("body", "").startswith(self.marker) and item["state"] in ("APPROVED", "CHANGES_REQUESTED") ] - if no_promotion and not existing and not automated: + if no_promotion and latest is None and not marked_reviews: self._transition(number, "ignored", "not a promotable update for this canonical vendor") return - if existing: - if existing[0]["body"] != body and not ( - not problem and existing[0]["body"] == body + dismissal_notice - ): - self.gh.api( - f"{self.prefix}/issues/comments/{existing[0]['id']}", - method="PATCH", - payload={"body": body}, - ) - _LOG.info("PR #%s: updated metadata feedback comment", number) - else: - existing = [ - self.gh.api( - f"{self.prefix}/issues/{number}/comments", method="POST", payload={"body": body} - ) - ] - _LOG.info("PR #%s: posted metadata feedback comment", number) - if current["state"] != "open" or current["user"]["login"] == self.actor: - self._transition( - number, - state, - str(problem) if problem else "feedback reconciled; not a code-review approval", - ) - return - if problem and problem.owner == "author": - review_body = ( - f"{self.marker}\n{problem.problem}\n\nSee the vendor-bot comment for " - "the specific fix and the latest validation result." - ) - for review in automated: - if review["body"] != review_body: - self.gh.api( - f"{self.prefix}/pulls/{number}/reviews/{review['id']}", - method="PUT", - payload={"body": review_body}, - ) - _LOG.info("PR #%s: updated metadata review %s", number, review["id"]) - if not automated and not manual: - self.gh.api( - f"{self.prefix}/pulls/{number}/reviews", - method="POST", - payload={ - "event": "REQUEST_CHANGES", - "commit_id": pr["head"]["sha"], - "body": review_body, - }, - ) - _LOG.info("PR #%s: requested metadata changes", number) - elif not problem: + dismissal_blocked = False + if not problem: for review in automated: # Never replace a metadata objection with APPROVE or dismiss a # human review. GitHub may require an authorized human dismissal. @@ -613,26 +589,87 @@ def _feedback( number, review["id"], ) - self._transition( - number, "blocked", "Authorized human must dismiss metadata review" - ) - if existing and existing[0]["body"] != body + dismissal_notice: - self.gh.api( - f"{self.prefix}/issues/comments/{existing[0]['id']}", - method="PATCH", - payload={"body": body + dismissal_notice}, - ) - return - if existing and existing[0]["body"] == body + dismissal_notice: + dismissal_blocked = True + if dismissal_blocked: + message += dismissal_notice + notification_message += dismissal_notice + fingerprint = metadata.fingerprint( + { + "vendor": self.gh.vendor_name, + "base_branch": self.gh.base_branch, + "source": {"url": source.url, "branch": source.branch, "commit": source.commit} + if source is not None + else None, + "state": state, + "message": notification_message, + } + ) + # Only the newest bot-owned result counts: A -> B -> A must notify again. + # The fingerprint is durable on GitHub even if the POST response or SQLite is lost. + lines = latest["body"].splitlines() if latest else [] + recorded = _FEEDBACK_KEY.fullmatch(lines[1]) if len(lines) > 1 else None + if recorded is None or recorded[1] != fingerprint: + body = ( + f"{self.marker}\n\n" + f"### Vendor validation: {field(self.gh.vendor_name)}\n\n" + f"@{pr['user']['login']} — {message}\n\n" + f"Checked PR revision: [`{head[:12]}`]" + f"(https://github.com/{self.gh.consumer_repo}/pull/{number}/commits/{head}). " + "This is a snapshot; newer results are posted as new comments." + ) + if source is not None: + body += ( + f"\n\nChecked source: {field(source.url)} / {field(source.branch)}, " + f"pin `{source.commit}`." + ) + if latest is not None: + body += ( + f"\n\nSupersedes [previous vendor validation]" + f"(https://github.com/{self.gh.consumer_repo}/pull/{number}" + f"#issuecomment-{latest['id']})." + ) + latest = self.gh.api( + f"{self.prefix}/issues/{number}/comments", method="POST", payload={"body": body} + ) + _LOG.info("PR #%s: posted metadata feedback comment %s", number, latest["id"]) + feedback_url = ( + f"https://github.com/{self.gh.consumer_repo}/pull/{number}#issuecomment-{latest['id']}" + ) + review_body = ( + f"{self.marker}\nVendor validation result: " + f"{'needs ' + problem.owner + ' action' if problem else 'no source-validation objection'}." + f"\n\n[Latest vendor validation and step-by-step fix]({feedback_url})." + ) + for review in marked_reviews: + if review["body"] != review_body: self.gh.api( - f"{self.prefix}/issues/comments/{existing[0]['id']}", - method="PATCH", - payload={"body": body}, + f"{self.prefix}/pulls/{number}/reviews/{review['id']}", + method="PUT", + payload={"body": review_body}, ) + _LOG.info("PR #%s: updated metadata review %s", number, review["id"]) + if ( + problem + and problem.owner == "author" + and not automated + and not manual + and current["state"] == "open" + and current["user"]["login"] != self.actor + ): + self.gh.api( + f"{self.prefix}/pulls/{number}/reviews", + method="POST", + payload={"event": "REQUEST_CHANGES", "commit_id": head, "body": review_body}, + ) + _LOG.info("PR #%s: requested metadata changes", number) self._transition( number, - state, - str(problem) if problem else "feedback reconciled; not a code-review approval", + "blocked" if dismissal_blocked else state, + "Authorized human must dismiss metadata review" + if dismissal_blocked + else str(problem) + if problem + else "feedback reconciled; not a code-review approval", ) def _inputs(self, pr: dict) -> tuple[manage.Vendor, manage.Vendor] | None: @@ -844,10 +881,12 @@ def inspect(self, number: int) -> None: # to advance the shared canonical branch. Do not report stale inputs as valid. upstream_proof = promote._check_source(self.gh, previous, reviewed) except Diagnostic as error: - self._feedback(pr, error) + self._feedback(pr, error, source=reviewed) return self.progress.operation("reconciling_feedback", number) - self._feedback(pr, None, attribution=evidence, upstream_proof=upstream_proof) + self._feedback( + pr, None, attribution=evidence, upstream_proof=upstream_proof, source=reviewed + ) if not pr.get("merged"): return # Re-read mutable description/head before any promotion. Immutable lock diff --git a/scripts/vendor/diagnostics.py b/scripts/vendor/diagnostics.py index 19dd7b1698bc..a9412e8af111 100644 --- a/scripts/vendor/diagnostics.py +++ b/scripts/vendor/diagnostics.py @@ -20,8 +20,11 @@ def __init__( *, example: str = "", owner: Literal["author", "maintainer"] = "author", + notification_text: str | None = None, ) -> None: self.problem, self.action, self.example, self.owner = problem, action, example, owner + # Observation-only changes must not notify authors about the same actionable result. + self.notification_text = notification_text super().__init__(problem) def __str__(self) -> str: diff --git a/scripts/vendor/metadata.py b/scripts/vendor/metadata.py index 97a11f5bea7e..85d392cf3b88 100644 --- a/scripts/vendor/metadata.py +++ b/scripts/vendor/metadata.py @@ -479,10 +479,17 @@ def match( parents = promote._git(repo, "show", "-s", "--format=%P", sha).split() if len(parents) != 1: raise Diagnostic( - f"Source history must be linear; commit `{sha}` has {len(parents)} parents.", - f"Rebase the temporary source branch onto canonical pin `{previous}` without " - "merge commits. Then use `scripts/vendor/manage.py` to update the lock and " - "vendored snapshot to the new source SHA, and push the consumer PR.", + f"Source history must be linear; commit `{sha}` has {len(parents)} parents. " + "Temporary-fork updates require a linear range for commit-by-commit upstream " + "attribution; containing the dev tip alone is not sufficient.", + "Fetch the latest canonical dev branch and rebase your temporary branch's " + "changes onto its tip without merge commits, or cherry-pick the intended " + "changes onto a fresh branch from that tip. Do not rewrite canonical history " + f"or assume the consumer lock pin `{previous}` is still the latest dev tip. " + "Test and push the source branch, then use `scripts/vendor/manage.py` to " + "update and verify the lock and vendored snapshot at the new source SHA. " + "Push the consumer PR. Editing the upstream PR list or `resolutions` alone " + "cannot remove a merge commit.", ) source_parents[sha] = parents[0] resolutions = resolutions or {} @@ -690,9 +697,10 @@ def resolve( cache, "merge-base", previous.commit, reviewed.commit, allowed_returncodes=(0, 1) ) rebase_action = ( - f"Rebase the temporary source branch {field(reviewed.branch)} onto canonical " - f"repository {field(previous.url)}, branch {field(previous.branch)}, SHA " - f"`{previous.commit}`. Then use `scripts/vendor/manage.py` to update " + f"Fetch canonical repository {field(previous.url)}, branch {field(previous.branch)}, " + f"and rebase the temporary source branch {field(reviewed.branch)} onto its latest " + f"tip without merge commits, not just the consumer lock's pin `{previous.commit}`. " + "Then use `scripts/vendor/manage.py` to update " "`3rdparty/vendor_sources.lock.yaml` and the vendored snapshot to the rebased SHA, " "and push the source branch and consumer PR. Editing metadata alone will not fix this." ) diff --git a/scripts/vendor/promote.py b/scripts/vendor/promote.py index 7e112c4ee75c..dc0a945a8ec8 100644 --- a/scripts/vendor/promote.py +++ b/scripts/vendor/promote.py @@ -270,34 +270,65 @@ def _check_source(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) _check_source_repo(gh, repo) _check_canonical_tip(gh, previous, reviewed) return None - if not _is_upstream_main(gh, reviewed): - raise Diagnostic( - f"An upstream source must use the configured branch {field(gh.upstream_branch)}.", - "Pin a commit reachable from that branch which contains the latest canonical " - "dev tip, or use a temporary branch in a personal fork based on that dev tip. " - "Update the lock and regenerate the vendored snapshot.", - ) _check_selection(previous, reviewed) - tip = _check_canonical_tip(gh, previous, reviewed) - if ( + tip = _canonical_tip(gh, previous, reviewed) + contains_tip = ( + gh.compare(_repo_name(previous.url), tip, reviewed.commit)["merge_base_commit"]["sha"] + == tip + ) + contains_previous = ( gh.compare(repo, previous.commit, reviewed.commit)["merge_base_commit"]["sha"] - != previous.commit - ): - raise Diagnostic( - "The upstream pin does not contain the previous canonical revision.", - "Choose an upstream pin containing the canonical history, or use the explicit " - "maintainer refresh workflow to reconcile divergent histories.", - ) + == previous.commit + ) upstream = _sha(gh.get(f"repos/{repo}/git/ref/heads/{gh.upstream_branch}")["object"]["sha"]) - if gh.compare(repo, reviewed.commit, upstream)["merge_base_commit"]["sha"] != reviewed.commit: - raise Diagnostic( - f"The pinned source commit {field(reviewed.commit)} is not reachable from " - f"{field(repo)} / {field(gh.upstream_branch)} at `{upstream}`.", - "Pin a commit on the configured upstream branch that also contains the latest " - "canonical dev tip. If the changes are not upstream yet, publish them on a " - "temporary fork branch and provide vendor-promotion metadata instead. " - "Regenerate the lock and vendored snapshot for the chosen SHA.", - ) + reachable = ( + gh.compare(repo, reviewed.commit, upstream)["merge_base_commit"]["sha"] == reviewed.commit + ) + branch_matches = _is_upstream_main(gh, reviewed) + if not (branch_matches and contains_tip and contains_previous and reachable): + + def describe(dev_tip: str, upstream_tip: str) -> Diagnostic: + checks = [ + f"- {'PASS' if branch_matches else 'FAIL'} — configured branch: " + f"expected {field(gh.upstream_branch)}, selected {field(reviewed.branch)}.", + f"- {'PASS' if contains_tip and contains_previous else 'FAIL'} — " + f"the pin {'contains' if contains_tip and contains_previous else 'does not contain'} " + f"the latest canonical branch tip and its history: {field(previous.branch)} " + f"at `{dev_tip}`. [Compare pin → dev](https://github.com/{_repo_name(previous.url)}" + f"/compare/{reviewed.commit}...{dev_tip}).", + f"- {'PASS' if reachable else 'FAIL'} — the pin " + f"{'is reachable' if reachable else 'is not reachable'} from " + f"{field(repo)} / {field(gh.upstream_branch)} at `{upstream_tip}`. " + f"[Compare pin → upstream](https://github.com/{repo}" + f"/compare/{reviewed.commit}...{upstream_tip}).", + ] + return Diagnostic( + "The upstream source cannot be promoted with the current lock.\n\n" + f"Selected source: {field(repo)} / {field(reviewed.branch)}, " + f"pin `{reviewed.commit}`.\n\n" + "\n".join(checks), + "Choose one of these paths:\n\n" + f"1. **Temporary fork branch:** Fetch {field(previous.url)} / " + f"{field(previous.branch)}, create or rebase your temporary branch onto its " + f"latest tip (observed `{dev_tip}`), and apply the intended changes. Test and push " + "that branch to your fork. Update the consumer lock URL, branch, and commit; " + "use `scripts/vendor/manage.py` to regenerate and verify the vendored snapshot, " + "then commit and push the consumer changes. Add the paired upstream PRs in the " + "PR description's vendor-promotion block (use `unpaired_reason` only when " + "there genuinely is no upstream PR).\n" + f"2. **Upstream branch:** Use {field(repo)} / {field(gh.upstream_branch)} " + "only with a pin that passes all three checks above, then regenerate and " + "verify the snapshot. This mode does not require an upstream PR list.\n\n" + "Changing only the branch field or PR metadata cannot fix failed ancestry " + "checks. Equivalent patches under different SHAs do not establish Git ancestry. " + "If preserving the dev history is not possible, ask a maintainer for an explicit " + "refresh rather than replacing the dev branch history. See the " + f"[vendor update guide](https://github.com/{gh.consumer_repo}/blob/" + f"{gh.base_branch}/3rdparty/vendor-sources.md).", + ) + + problem = describe(tip, upstream) + problem.notification_text = str(describe("LATEST_DEV_TIP", "UPSTREAM_TIP")) + raise problem return { "promoted": reviewed.commit, "upstream_base": previous.commit, @@ -306,8 +337,8 @@ def _check_source(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) } -def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> str: - """Require the immutable source pin to contain the live canonical branch tip.""" +def _canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> str: + """Read the live dev tip without accepting a rewritten canonical history.""" repo = _repo_name(previous.url) tip = _sha(gh.get(f"repos/{repo}/git/ref/heads/{previous.branch}")["object"]["sha"]) if tip in (previous.commit, reviewed.commit): @@ -317,27 +348,43 @@ def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.V "Canonical branch no longer contains the consumer lock's revision; " "a maintainer must inspect the branch history before promotion." ) + return tip + + +def _check_canonical_tip(gh: GitHub, previous: vendor.Vendor, reviewed: vendor.Vendor) -> str: + """Require the immutable source pin to contain the live canonical branch tip.""" + repo = _repo_name(previous.url) + tip = _canonical_tip(gh, previous, reviewed) + if tip in (previous.commit, reviewed.commit): + return tip if gh.compare(repo, tip, reviewed.commit)["merge_base_commit"]["sha"] != tip: - if _is_upstream_main(gh, reviewed): - raise Diagnostic( - f"The pinned upstream commit {field(reviewed.commit)} does not contain " - f"the latest canonical branch tip {field(tip)}.", - "Select a newer upstream commit containing that exact dev tip, or use a " - "temporary fork branch based on the latest dev tip with vendor-promotion " - "metadata. If upstream and dev have diverged, a maintainer must use the " - "explicit refresh workflow. Update the lock and vendored snapshot; " - "equivalent patches under different SHAs do not establish Git ancestry.", + + def describe(dev_tip: str) -> Diagnostic: + if _is_upstream_main(gh, reviewed): + return Diagnostic( + f"The pinned upstream commit {field(reviewed.commit)} does not contain " + f"the latest canonical branch tip {field(dev_tip)}.", + "Select a newer upstream commit containing that exact dev tip, or use a " + "temporary fork branch based on the latest dev tip with vendor-promotion " + "metadata. If upstream and dev have diverged, a maintainer must use the " + "explicit refresh workflow. Update the lock and vendored snapshot; " + "equivalent patches under different SHAs do not establish Git ancestry.", + ) + return Diagnostic( + f"The pinned temporary source commit {field(reviewed.commit)} does not contain " + f"the latest canonical branch tip {field(dev_tip)}.", + f"Fetch {field(previous.url)} / {field(previous.branch)} and rebase the temporary " + f"source branch {field(reviewed.branch)} onto its latest tip (observed " + f"`{dev_tip}`). Push it, then use " + "`scripts/vendor/manage.py` to update the consumer lock and vendored snapshot " + "to the rebased SHA. The consumer lock's old pin alone is not a sufficient " + "base; updating only the PR metadata or temporary branch without its lock pin " + "will not fix this.", ) - raise Diagnostic( - f"The pinned temporary source commit {field(reviewed.commit)} does not contain " - f"the latest canonical branch tip {field(tip)}.", - f"Fetch {field(previous.url)} / {field(previous.branch)} and rebase the temporary " - f"source branch {field(reviewed.branch)} onto `{tip}`. Push it, then use " - "`scripts/vendor/manage.py` to update the consumer lock and vendored snapshot " - "to the rebased SHA. The consumer lock's old pin alone is not a sufficient " - "base; updating only the PR metadata or temporary branch without its lock pin " - "will not fix this.", - ) + + problem = describe(tip) + problem.notification_text = str(describe("LATEST_DEV_TIP")) + raise problem return tip diff --git a/tests/unittest/others/test_maintain_prims_ts.py b/tests/unittest/others/test_maintain_prims_ts.py index 9ab55b953a7e..f65ea064541c 100644 --- a/tests/unittest/others/test_maintain_prims_ts.py +++ b/tests/unittest/others/test_maintain_prims_ts.py @@ -942,6 +942,36 @@ def test_upstream_main_exception_is_narrow(world: World, failure: str) -> None: assert not any(call[0] == "push" for call in world.git_calls) +def test_upstream_source_reports_all_failed_checks(world: World) -> None: + _pin_upstream(world, branch="pull/5434/head") + world.upstream = world.previous + _git(world.source, "switch", "-q", "--detach", world.previous) + tip = _advance_canonical(world) + with pytest.raises(m.Diagnostic) as failure: + m._make_plan(world.args(upstream_pr=[]), world) + text = str(failure.value) + assert text.count("- FAIL") == 3 + assert "- PASS" not in text + assert "pull/5434/head" in text and "configured branch" in text + assert "latest canonical branch tip" in text and tip in text + assert "not reachable" in text and world.upstream in text + assert f"/compare/{world.reviewed}...{tip}" in text + assert f"/compare/{world.reviewed}...{world.upstream}" in text + assert "Temporary fork branch" in text and "rebase" in text + assert "Changing only the branch field" in text + assert "vendor-promotion block" in text and "vendor-sources.md" in text + assert all(method == "GET" for method, _, _ in world.calls) + + +def test_upstream_source_wrong_branch_does_not_invent_ancestry_failures(world: World) -> None: + _pin_upstream(world, branch="pull/