This is an automated email from the ASF dual-hosted git repository.
shuke987 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 6cfe3656fbb [fix](ci) Supply missing review skills for 4.x release
branches (#68556)
6cfe3656fbb is described below
commit 6cfe3656fbb0b1f273554b82e7a93ea84ef35c1b
Author: shuke <[email protected]>
AuthorDate: Mon Sep 28 16:43:11 2026 +0800
[fix](ci) Supply missing review skills for 4.x release branches (#68556)
Reviews check out the PR head, but the shared workflow requires a
repository `code-review` skill before any code inspection. Older release
heads do not contain that skill. The review of #66227 (targeting
`branch-4.1`) therefore exhausted its goal turns without inspecting the
diff or submitting a review, even with the working Codex runtime.
For PRs targeting `branch-4.2`, `branch-4.1`, or `branch-4.0` whose
checkout lacks the skill, prepare the skill and its referenced module
guides from the immutable workflow commit. Store them under the per-run
review context and rewrite the skill's guide paths to resolve to
existing branch guides or the prepared copies. Keep existing branch
guides and skills intact and give every review agent the explicit skill
path. Preserve the current behavior for other target branches.
Record the source SHA/path mapping and fail context preparation if
trusted instructions are incomplete. Branch code and branch-specific
guides take precedence over supplemental guidance from the workflow
version.
This is a shared-workflow fix on master: after merge, newly triggered
reviews for all three target branches receive it without updating their
PR heads or cherry-picking documentation.
---
.github/scripts/prepare_review_agents.py | 103 ++++++++++++-
.github/scripts/test_prepare_review_agents.py | 212 ++++++++++++++++++++++++++
.github/workflows/code-review-runner.yml | 16 +-
3 files changed, 325 insertions(+), 6 deletions(-)
diff --git a/.github/scripts/prepare_review_agents.py
b/.github/scripts/prepare_review_agents.py
index a39862734e3..12f12f764f0 100644
--- a/.github/scripts/prepare_review_agents.py
+++ b/.github/scripts/prepare_review_agents.py
@@ -1,12 +1,20 @@
#!/usr/bin/env python3
-"""Prepare the AGENTS.md guide list for automated PR review prompts."""
+"""Prepare review guides and a release-branch skill fallback for PR reviews."""
from __future__ import annotations
import argparse
+import json
+import re
+import subprocess
from pathlib import Path, PurePosixPath
+SKILL_PATH = ".claude/skills/code-review/SKILL.md"
+SKILL_FALLBACK_BRANCHES = {"branch-4.2", "branch-4.1", "branch-4.0"}
+GUIDE_REFERENCE = re.compile(r"`([^`\n]+/AGENTS\.md)`")
+
+
def parse_args() -> argparse.Namespace:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument(
@@ -33,6 +41,12 @@ def parse_args() -> argparse.Namespace:
type=Path,
help="Repository root. Defaults to the current working directory.",
)
+ parser.add_argument("--base-ref", default="", help="Live PR target
branch.")
+ parser.add_argument("--trusted-ref", default="", help="Immutable workflow
commit SHA.")
+ parser.add_argument(
+ "--skill-prompt-block", type=Path,
+ help="Output skill instructions; fallback assets are stored beside
this file.",
+ )
return parser.parse_args()
@@ -69,6 +83,89 @@ def required_agents(repo_root: Path, changed_files:
list[PurePosixPath]) -> list
return agents
+def prepare_review_skill(repo_root: Path, base_ref: str, trusted_ref: str,
context_dir: Path) -> str:
+ if base_ref not in SKILL_FALLBACK_BRANCHES:
+ return (
+ "Before reviewing any code, you MUST read and follow the code
review skill in this repository. "
+ "During review, you must strictly follow those instructions.\n"
+ )
+ skill_path = SKILL_PATH
+ instructions = ""
+ # Older release PR heads predate the repository skill. Limit this rollout
to
+ # the three supported branches, and retain a skill supplied by the
checkout.
+ if not (repo_root / SKILL_PATH).is_file():
+ if not re.fullmatch(r"[0-9a-f]{40}", trusted_ref):
+ raise ValueError("Skill fallback requires an immutable workflow
commit SHA")
+ context_dir = context_dir.resolve()
+ context_dir.relative_to(repo_root)
+ # Full-history checkout normally already contains the workflow commit.
+ # Fetch that exact commit if the PR checkout did not bring it in.
+ available = subprocess.run(
+ ["git", "cat-file", "-e", f"{trusted_ref}^{{commit}}"],
+ cwd=repo_root, capture_output=True,
+ )
+ if available.returncode:
+ subprocess.run(
+ ["git", "fetch", "--no-tags", "origin", trusted_ref],
cwd=repo_root, check=True,
+ )
+
+ def read_trusted(path: str) -> str:
+ return subprocess.run(
+ ["git", "show", f"{trusted_ref}:{path}"], cwd=repo_root,
+ check=True, stdout=subprocess.PIPE, text=True,
+ ).stdout
+
+ skill = read_trusted(SKILL_PATH)
+ bundle = context_dir / "review-guidance"
+ bundle.mkdir()
+ sources = {}
+
+ def resolve_guide(match: re.Match) -> str:
+ path = match.group(1)
+ # The checklist also uses abbreviated paths, e.g. fe/.../persist.
+ # Their full paths appear in the module guide table in the skill.
+ if "..." in PurePosixPath(path).parts:
+ return match.group(0)
+ valid_changed_path(path)
+ if path not in sources:
+ checkout_path = repo_root / path
+ if checkout_path.is_file():
+ checkout_path.resolve().relative_to(repo_root)
+ sources[path] = path
+ else:
+ target = bundle / path
+ target.parent.mkdir(parents=True, exist_ok=True)
+ target.write_text(read_trusted(path))
+ sources[path] = target.relative_to(repo_root).as_posix()
+ return f"`{sources[path]}`"
+
+ # Point the skill directly at existing checkout guides or prepared
copies;
+ # otherwise the skill would still require paths absent on old branches.
+ skill = GUIDE_REFERENCE.sub(resolve_guide, skill)
+ target = bundle / SKILL_PATH
+ target.parent.mkdir(parents=True, exist_ok=True)
+ target.write_text(skill)
+ skill_path = target.relative_to(repo_root).as_posix()
+ manifest = {
+ "base_ref": base_ref, "trusted_ref": trusted_ref,
+ "skill": skill_path, "guides": sources,
+ }
+ (context_dir /
"review_guidance_sources.json").write_text(json.dumps(manifest, indent=2) +
"\n")
+ print(f"Prepared code-review skill for {base_ref} from workflow commit
{trusted_ref}: {skill_path}")
+ instructions = (
+ f"The workflow prepared this missing skill and its module guides
from commit {trusted_ref}. "
+ "The AGENTS.md paths in this skill resolve to the existing
checkout guides where available, "
+ "or to prepared fallback copies. Existing branch guides and actual
branch code take precedence. "
+ "Use fallback guides as review checklists; verify each rule
against the checked-out branch, "
+ "and do not assume newer source layouts or features exist on this
release branch. "
+ "Source-code paths mentioned inside module guides are relative to
the repository root.\n"
+ )
+ return (
+ f"Before reviewing any code, you MUST read and follow the code-review
skill at `{skill_path}`. "
+ "Pass this exact skill path to every review subagent.\n" + instructions
+ )
+
+
def main() -> None:
args = parse_args()
repo_root = args.repo_root.resolve()
@@ -77,6 +174,10 @@ def main() -> None:
for path in (valid_changed_path(line) for line in
args.changed_files.read_text().splitlines())
if path is not None
]
+ if args.skill_prompt_block is not None:
+ args.skill_prompt_block.write_text(prepare_review_skill(
+ repo_root, args.base_ref, args.trusted_ref,
args.skill_prompt_block.parent,
+ ))
agents = required_agents(repo_root, changed_files)
args.required_agents.write_text("".join(f"{path}\n" for path in agents))
diff --git a/.github/scripts/test_prepare_review_agents.py
b/.github/scripts/test_prepare_review_agents.py
new file mode 100644
index 00000000000..3c1be37a183
--- /dev/null
+++ b/.github/scripts/test_prepare_review_agents.py
@@ -0,0 +1,212 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Test release review context preparation without credentials or model
calls."""
+
+import json
+import os
+import re
+import shutil
+import subprocess
+import sys
+import tempfile
+import textwrap
+import unittest
+from pathlib import Path
+
+from prepare_review_agents import SKILL_PATH, prepare_review_skill
+
+SCRIPTS = Path(__file__).resolve().parent
+WORKFLOW = SCRIPTS.parent / "workflows/code-review-runner.yml"
+EXISTING_GUIDE = "be/src/runtime/AGENTS.md"
+MISSING_GUIDE = "fe/fe-core/AGENTS.md"
+BRANCH_ONLY_GUIDE = "be/src/format_v2/AGENTS.md"
+
+
+def step_script(name):
+ step = re.search(
+ rf"^ - name: {re.escape(name)}\n(.*?)(?=^ - name:|\Z)",
+ WORKFLOW.read_text(), re.MULTILINE | re.DOTALL,
+ ).group(1)
+ return textwrap.dedent(re.search(
+ r"^ run: \|\n((?: .*\n|\n)+)", step, re.MULTILINE,
+ ).group(1))
+
+
+class ReviewGuidanceTest(unittest.TestCase):
+ def setUp(self):
+ self.temp = tempfile.TemporaryDirectory()
+ self.addCleanup(self.temp.cleanup)
+ self.root = Path(self.temp.name).resolve()
+ self.repo = self.root / "repo"
+ self.repo.mkdir()
+ self.git("init", "-q")
+ self.git("config", "user.name", "Review Test")
+ self.git("config", "user.email", "[email protected]")
+ self.write(SKILL_PATH, f"Read `{EXISTING_GUIDE}` and
`{MISSING_GUIDE}`.\n"
+ f"Read `{MISSING_GUIDE}` again. Shorthand:
`fe/.../persist/AGENTS.md`.\n")
+ self.write(EXISTING_GUIDE, "Trusted runtime guide\n")
+ self.write(MISSING_GUIDE, "Trusted FE guide\n")
+ self.trusted = self.commit("Trusted workflow instructions")
+ self.git("rm", "-q", SKILL_PATH, MISSING_GUIDE)
+ self.write(EXISTING_GUIDE, "Release-specific runtime guide\n")
+ self.write(BRANCH_ONLY_GUIDE, "Release-specific format guide\n")
+ self.base = self.commit("Old release without skill")
+ self.write("be/src/format_v2/reader.cpp", "PR change\n")
+ self.head = self.commit("PR change")
+ self.context = self.repo / ".code-review.test"
+ self.context.mkdir()
+
+ def git(self, *args, cwd=None):
+ return subprocess.run(["git", *args], cwd=cwd or self.repo, check=True,
+ capture_output=True, text=True).stdout.strip()
+
+ def write(self, path, content):
+ target = self.repo / path
+ target.parent.mkdir(parents=True, exist_ok=True)
+ target.write_text(content)
+
+ def commit(self, message):
+ self.git("add", ".")
+ self.git("commit", "-qm", message)
+ return self.git("rev-parse", "HEAD")
+
+ def prepare(self, branch="branch-4.1", trusted=None):
+ return prepare_review_skill(self.repo, branch, trusted or
self.trusted, self.context)
+
+ def check_bundle(self):
+ manifest = json.loads((self.context /
"review_guidance_sources.json").read_text())
+ self.assertEqual(manifest["trusted_ref"], self.trusted)
+ self.assertEqual(manifest["guides"][EXISTING_GUIDE], EXISTING_GUIDE)
+ skill = (self.repo / manifest["skill"]).read_text()
+ for guide in manifest["guides"].values():
+ self.assertTrue((self.repo / guide).is_file())
+ self.assertIn(f"`{guide}`", skill)
+ fallback = self.repo / manifest["guides"][MISSING_GUIDE]
+ self.assertEqual(fallback.read_text(), "Trusted FE guide\n")
+ self.assertEqual((self.repo / EXISTING_GUIDE).read_text(),
"Release-specific runtime guide\n")
+ self.assertFalse((self.repo / MISSING_GUIDE).exists())
+ self.assertFalse((self.repo / SKILL_PATH).exists())
+ self.assertEqual(self.git("diff", "HEAD"), "")
+ return manifest
+
+ def test_all_three_release_branches_get_resolvable_guides(self):
+ for branch in ("branch-4.2", "branch-4.1", "branch-4.0"):
+ with self.subTest(branch=branch):
+ self.context = self.repo / f".code-review.{branch}"
+ self.context.mkdir()
+ prompt = self.prepare(branch)
+ manifest = self.check_bundle()
+ self.assertIn(manifest["skill"], prompt)
+ self.assertIn("take precedence", prompt)
+ self.assertEqual(manifest["base_ref"], branch)
+
+ def test_other_branches_do_not_load_fallback(self):
+ for branch in ("master", "branch-4.1.4", "branch-3.1", "branch-3.0"):
+ with self.subTest(branch=branch):
+ prompt = self.prepare(branch, trusted="not-a-ref")
+ self.assertIn("code review skill in this repository", prompt)
+ self.assertNotIn(SKILL_PATH, prompt)
+ self.assertEqual(list(self.context.iterdir()), [])
+
+ def test_checkout_skill_is_preserved(self):
+ self.write(SKILL_PATH, "Release-specific skill\n")
+ prompt = self.prepare(trusted="not-a-ref")
+ self.assertIn(f"`{SKILL_PATH}`", prompt)
+ self.assertEqual((self.repo / SKILL_PATH).read_text(),
"Release-specific skill\n")
+ self.assertEqual(list(self.context.iterdir()), [])
+
+ def test_missing_trusted_guide_fails_before_model_start(self):
+ self.write(SKILL_PATH, "Read `missing/AGENTS.md`\n")
+ bad_ref = self.commit("Incomplete trusted instructions")
+ (self.repo / SKILL_PATH).unlink()
+ with self.assertRaises(subprocess.CalledProcessError):
+ self.prepare(trusted=bad_ref)
+ self.assertFalse((self.context /
"review_guidance_sources.json").exists())
+
+ def test_mutable_ref_is_rejected(self):
+ with self.assertRaisesRegex(ValueError, "immutable workflow commit
SHA"):
+ self.prepare(trusted="master")
+
+ def test_context_outside_checkout_is_rejected(self):
+ self.context = self.root
+ with self.assertRaises(ValueError):
+ self.prepare()
+
+ def test_missing_workflow_commit_is_fetched_from_origin(self):
+ checkout = self.root / "checkout"
+ checkout.mkdir()
+ self.git("init", "-q", cwd=checkout)
+ self.git("remote", "add", "origin", str(self.repo), cwd=checkout)
+ context = checkout / ".code-review.test"
+ context.mkdir()
+ prepare_review_skill(checkout, "branch-4.0", self.trusted, context)
+ self.assertEqual(self.git("rev-parse", f"{self.trusted}^{{commit}}",
cwd=checkout), self.trusted)
+ manifest = json.loads((context /
"review_guidance_sources.json").read_text())
+ self.assertTrue((checkout / manifest["skill"]).is_file())
+
+ @unittest.skipUnless(shutil.which("jq"), "workflow shell requires jq")
+ def test_actual_workflow_prepares_context_and_final_prompt(self):
+ # Execute the real shell blocks. Only GitHub HTTP is replaced by a
fixture.
+ bin_dir = self.root / "bin"
+ bin_dir.mkdir()
+ gh = bin_dir / "gh"
+ gh.write_text(f"#!{sys.executable}\n" + textwrap.dedent('''
+ import json
+ import os
+ import sys
+ from pathlib import Path
+ endpoint = sys.argv[-1]
+ if endpoint == "repos/apache/doris/pulls/66227":
+ print(json.dumps({"head": {"sha": os.environ["HEAD_SHA"]},
+ "base": {"sha": os.environ["BASE_SHA"],
"ref": "branch-4.1"}}))
+ else:
+ assert endpoint ==
("repos/apache/doris/contents/.github/scripts/"
+ "prepare_review_agents.py?ref=" +
os.environ["HELPER_REF"])
+ print(Path(os.environ["TEST_HELPER"]).read_text())
+ '''))
+ gh.chmod(0o755)
+ if sys.platform == "darwin":
+ # GNU sed -i and BSD sed -i have different argv conventions.
+ sed = bin_dir / "sed"
+ sed.write_text('#!/bin/bash\nif [ "$1" = -i ]; then shift; exec
/usr/bin/sed -i "" "$@"; fi\nexec /usr/bin/sed "$@"\n')
+ sed.chmod(0o755)
+ env = {**os.environ, "PATH": f"{bin_dir}:{os.environ['PATH']}",
+ "REPO": "apache/doris", "PR_NUMBER": "66227", "HEAD_SHA":
self.head,
+ "BASE_SHA": self.base, "HELPER_REF": self.trusted,
+ "REVIEW_CONTEXT_DIR": str(self.context), "REVIEW_CONTEXT_REL":
self.context.name,
+ "RUNNER_TEMP": str(self.root), "TEST_HELPER": str(SCRIPTS /
"prepare_review_agents.py")}
+ for step in ("Prepare authoritative PR context and required AGENTS
guides", "Prepare review prompt"):
+ result = subprocess.run(["bash", "-e", "-c", step_script(step)],
+ cwd=self.repo, env=env,
capture_output=True, text=True)
+ self.assertEqual(result.returncode, 0, result.stdout +
result.stderr)
+ manifest = self.check_bundle()
+ prompt = (self.context / "review_prompt.txt").read_text()
+ self.assertIn(manifest["skill"], prompt)
+ self.assertIn(self.trusted, prompt)
+ self.assertIn(BRANCH_ONLY_GUIDE, prompt)
+ self.assertNotIn("PLACEHOLDER_", prompt)
+ self.assertEqual((self.context / "required_agents.txt").read_text(),
BRANCH_ONLY_GUIDE + "\n")
+ self.assertEqual((self.context / "pr_changed_files.txt").read_text(),
"be/src/format_v2/reader.cpp\n")
+ self.assertIn("+PR change", (self.context / "pr.diff").read_text())
+ self.assertIn(f"{self.context.name}/review_prompt.txt",
+ (self.context / "codex_goal_prompt.txt").read_text())
+
+
+if __name__ == "__main__":
+ unittest.main()
diff --git a/.github/workflows/code-review-runner.yml
b/.github/workflows/code-review-runner.yml
index bfa518584fc..55c4f69e0ed 100644
--- a/.github/workflows/code-review-runner.yml
+++ b/.github/workflows/code-review-runner.yml
@@ -627,6 +627,7 @@ jobs:
live_pr="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}")"
live_head_sha="$(jq -r '.head.sha' <<<"$live_pr")"
live_base_sha="$(jq -r '.base.sha' <<<"$live_pr")"
+ live_base_ref="$(jq -er '.base.ref' <<<"$live_pr")"
if [ "$live_head_sha" != "$HEAD_SHA" ] || [ "$live_base_sha" !=
"$BASE_SHA" ]; then
echo "PR changed while review context was being prepared; restart
the review"
echo "Expected base/head: $BASE_SHA $HEAD_SHA"
@@ -663,7 +664,10 @@ jobs:
python3 "$helper" \
--changed-files "$REVIEW_CONTEXT_DIR/pr_changed_files.txt" \
--required-agents "$REVIEW_CONTEXT_DIR/required_agents.txt" \
- --prompt-block "$REVIEW_CONTEXT_DIR/required_agents_prompt.txt"
+ --prompt-block "$REVIEW_CONTEXT_DIR/required_agents_prompt.txt" \
+ --base-ref "$live_base_ref" \
+ --trusted-ref "$HELPER_REF" \
+ --skill-prompt-block "$REVIEW_CONTEXT_DIR/review_skill_prompt.txt"
echo "Required AGENTS.md files for this review:"
sed 's/^/ /' "$REVIEW_CONTEXT_DIR/required_agents.txt"
@@ -696,9 +700,9 @@ jobs:
PR diff and path-reading (for all subagents and the main agent):
- PLACEHOLDER_CONTEXT_DIR/pr.diff and
PLACEHOLDER_CONTEXT_DIR/pr_changed_files.txt were both generated locally with
`git diff PLACEHOLDER_BASE_SHA...PLACEHOLDER_HEAD_SHA` after the live PR
base/head were verified. They are the authoritative PR diff and changed-path
list: they contain changes from the computed merge base to the PR head. The
listed PR Base SHA identifies the target-branch snapshot and is not necessarily
the diff's left endpoint. Do not obtain the PR diff or chang [...]
- - Before reading any file whose exact path is not already confirmed
by PLACEHOLDER_CONTEXT_DIR/pr_changed_files.txt,
PLACEHOLDER_CONTEXT_DIR/pr.diff, or a previous successful command output, you
MUST first run `rg --files` to confirm the actual path of the target file.
+ - Before reading any file whose exact path is not already confirmed
by this prompt, the selected skill's AGENTS.md references,
PLACEHOLDER_CONTEXT_DIR/pr_changed_files.txt, PLACEHOLDER_CONTEXT_DIR/pr.diff,
or a previous successful command output, you MUST first run `rg --files` to
confirm the actual path of the target file.
- Before reviewing any code, you MUST read and follow the code review
skill in this repository. During review, you must strictly follow those
instructions.
+ PLACEHOLDER_REVIEW_SKILL_BLOCK
The active review goal's progress tracking MUST include, and must
stay current throughout the review:
1. Read the review prompt, code-review skill, required AGENTS.md
files, existing review threads, user focus, changed-file list, and the shared
subagent review ledger.
2. Perform a main-agent initial risk scan before spawning review
subagents. You must thoroughly read all the changes in this PR and understand
all the involved mechanisms, pointing out: if there is a problem with this PR,
where is the problem most likely to be? Or are there any points you suspect to
be risky? write the result under `## Main Initial Risk Scan` in the shared
ledger.
@@ -752,7 +756,7 @@ jobs:
sed -i "s|PLACEHOLDER_BASE_SHA|${BASE_SHA}|g"
"$REVIEW_CONTEXT_DIR/review_prompt.txt"
sed -i "s|PLACEHOLDER_CONTEXT_DIR|${REVIEW_CONTEXT_REL}|g"
"$REVIEW_CONTEXT_DIR/review_prompt.txt"
- python3 - "$REVIEW_CONTEXT_DIR/review_prompt.txt"
"$REVIEW_CONTEXT_DIR/required_agents_prompt.txt" <<'PY'
+ python3 - "$REVIEW_CONTEXT_DIR/review_prompt.txt"
"$REVIEW_CONTEXT_DIR/required_agents_prompt.txt"
"$REVIEW_CONTEXT_DIR/review_skill_prompt.txt" <<'PY'
import sys
from pathlib import Path
@@ -760,7 +764,9 @@ jobs:
required_agents_path = Path(sys.argv[2])
prompt = prompt_path.read_text()
required_agents = required_agents_path.read_text().rstrip()
-
prompt_path.write_text(prompt.replace("PLACEHOLDER_REQUIRED_AGENTS_BLOCK",
required_agents))
+ skill_instructions = Path(sys.argv[3]).read_text().rstrip()
+ prompt = prompt.replace("PLACEHOLDER_REQUIRED_AGENTS_BLOCK",
required_agents)
+
prompt_path.write_text(prompt.replace("PLACEHOLDER_REVIEW_SKILL_BLOCK",
skill_instructions))
PY
cat > "$REVIEW_CONTEXT_DIR/subagent_review_findings.md" <<'LEDGER'
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
