diff --git a/.agent/skills/contributing/SKILL.md b/.agent/skills/contributing/SKILL.md index 2a60c5a3cd98..bf4d00f86a02 100644 --- a/.agent/skills/contributing/SKILL.md +++ b/.agent/skills/contributing/SKILL.md @@ -17,7 +17,7 @@ # under the License. name: contributing -description: Guides the contribution workflow for Apache Beam, including creating PRs, issue management, code review process, and release cycles. Use when contributing code, creating PRs, or understanding the contribution process. +description: Guides the contribution workflow for Apache Beam, including creating PRs, issue management, code review process, release cycles, and rigorous evaluation rules for high-risk core component changes. Use when contributing code, creating PRs, or modifying core Beam components. --- # Contributing to Apache Beam @@ -71,6 +71,10 @@ description: Guides the contribution workflow for Apache Beam, including creatin - Implementation details belong in inline code comments. - Use descriptive commit messages +#### Working on Core Components (High Risk — Heightened Scrutiny) + +> **AGENT DIRECTIVE**: modification to **Core Components** carries an inherently elevated risk of silent regression across distributed runners. Refer to `.github/autolabeler.yml` under the `"core"` label for the authoritative list of core component paths (including `sdks/java/core`, `runners/core-*`, `model`, `sdks/python/apache_beam/transforms|coders`, `sdks/go/pkg/beam/core`, `FileIO`, and `GCS`). If you are an AI agent modifying files matched by these paths, **evaluate your changes with scrutiny.** + ### 5. Create Pull Request - Link to the issue in PR description - Pre-commit tests run automatically @@ -91,6 +95,7 @@ description: Guides the contribution workflow for Apache Beam, including creatin ### For Reviewers - PRs can only be merged by [Beam committers](https://home.apache.org/phonebook.html?pmc=beam) +- PRs with the `core` label touch foundational Beam semantics or common code paths; review with extra scrutiny for non-obvious failure modes, edge case, and performance regressions ## Testing Workflows diff --git a/.github/autolabeler.yml b/.github/autolabeler.yml index 0b2cdf7dc282..24ab2ca9a606 100644 --- a/.github/autolabeler.yml +++ b/.github/autolabeler.yml @@ -17,6 +17,26 @@ # Please keep the entries sorted lexicographically in each category. +# All core components +"core": # This label instructs review with scrutiny + - "model/**/*" + - "runners/core-java/**/*" + - "runners/java-fn-execution/**/*" + - "sdks/go/pkg/beam/*.go" + - "sdks/go/pkg/beam/core/**/*" + - "sdks/go/pkg/beam/io/fileio/**/*" + - "sdks/go/pkg/beam/io/filesystem/**/*" + - "sdks/go/pkg/beam/transforms/**/*" + - "sdks/java/core/**/*" + - "sdks/java/extensions/google-cloud-platform-core/**/*" + - "sdks/java/harness/**/*" + - "sdks/python/apache_beam/*.py" + - "sdks/python/apache_beam/coders/**/*" + - "sdks/python/apache_beam/io/file*" + - "sdks/python/apache_beam/io/gcp/gcs*" + - "sdks/python/apache_beam/transforms/**/*" + - "sdks/python/apache_beam/typehints/**/*" + # General build: ["assembly.xml", "buildSrc/**/*", ".gitattributes", ".github/workflows/*", ".gitignore", "gradle/**/*", ".mailmap", "release/**/*", "sdks/java/build-tools/**/*"] docker: ["runners/flink/job-server-container/**/*", "runners/spark/job-server/container/**/*", "sdks/go/container/**/*", "sdks/java/container/**/*", "sdks/python/container/**/*"] @@ -80,7 +100,6 @@ io: ["sdks/go/pkg/beam/io/**/*", "sdks/java/io/**/*", "sdks/python/apache_beam/ # Runners "runners": ["runners/**/*", "sdks/go/pkg/beam/runners/**/*", "sdks/python/apache_beam/runners/**/*", "sdks/typescript/src/apache_beam/runners/**/*"] -"core": ["runners/core-java/**/*"] "dataflow": ["runners/google-cloud-dataflow-java/**/*", "sdks/go/pkg/beam/runners/dataflow/**/*", "sdks/python/runners/dataflow/**/*"] "direct": ["runners/direct-java/**/*", "sdks/go/pkg/beam/runners/direct/**/*", "sdks/python/runners/direct/**/*"] "flink": ["runners/flink/**/*", "sdks/go/pkg/beam/runners/flink/**/*"] diff --git a/scripts/ci/pr-bot/findPrsNeedingAttention.ts b/scripts/ci/pr-bot/findPrsNeedingAttention.ts index ca868ad783fc..ebca0deba83e 100644 --- a/scripts/ci/pr-bot/findPrsNeedingAttention.ts +++ b/scripts/ci/pr-bot/findPrsNeedingAttention.ts @@ -128,7 +128,9 @@ async function assignToNewReviewers( console.log(`Assigning new reviewers for pr ${pull.number}`); await github.addPrComment( pull.number, - commentStrings.assignNewReviewer(prState.reviewersAssignedForLabels) + commentStrings.assignNewReviewer(prState.reviewersAssignedForLabels, { + labels: pull.labels, + }) ); await stateClient.writePrState(pull.number, prState); diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts index dd20d4a98c2b..9f0ed52a116a 100644 --- a/scripts/ci/pr-bot/processNewPrs.ts +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -341,7 +341,9 @@ async function processPull( console.log(`Assigning reviewers for PR ${pull.number}`); await github.addPrComment( pull.number, - commentStrings.assignReviewer(prState.reviewersAssignedForLabels) + commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { + labels: pull.labels, + }) ); github.nextActionReviewers(pull.number, pull.labels); diff --git a/scripts/ci/pr-bot/shared/commentStrings.ts b/scripts/ci/pr-bot/shared/commentStrings.ts index b556deea4f75..272f185d4ec3 100644 --- a/scripts/ci/pr-bot/shared/commentStrings.ts +++ b/scripts/ci/pr-bot/shared/commentStrings.ts @@ -16,8 +16,37 @@ * limitations under the License. */ +import { Label } from "./githubUtils"; const { NO_MATCHING_LABEL } = require("./constants"); +export interface AssignReviewerOptions { + labels?: (string | Label)[]; + notices?: string[]; +} + +// Custom notices for specific labels +const LABEL_NOTICES: Record = { + core: "This pull request likely touches a core component (\"core\" label). Please review with scrutiny.", +}; + +function formatNotices( + labelToReviewerMapping: any, + options?: AssignReviewerOptions +): string { + const notices = [...(options?.notices || [])]; + const labels = [ + ...(options?.labels || []), + ...Object.keys(labelToReviewerMapping), + ]; + for (const label of labels) { + const name = (typeof label === "string" ? label : label.name).toLowerCase(); + if (LABEL_NOTICES[name] && !notices.includes(LABEL_NOTICES[name])) { + notices.push(LABEL_NOTICES[name]); + } + } + return notices.length ? `\n${notices.join("\n\n")}\n` : ""; +} + export function allChecksPassed(reviewersToNotify: string[]): string { return `All checks have passed: @${reviewersToNotify.join(" ")}`; } @@ -26,7 +55,10 @@ export function assignCommitter(committer: string): string { return `R: @${committer} for final approval`; } -export function assignReviewer(labelToReviewerMapping: any): string { +export function assignReviewer( + labelToReviewerMapping: any, + options?: AssignReviewerOptions +): string { let commentString = "Assigning reviewers:\n\n"; @@ -39,6 +71,8 @@ export function assignReviewer(labelToReviewerMapping: any): string { } } + commentString += formatNotices(labelToReviewerMapping, options); + commentString += ` Note: If you would like to opt out of this review, comment \`assign to next reviewer\`. @@ -114,9 +148,12 @@ Users are removed if they haven't reviewed or completed a PR in the last 3 month return commentString; } -export function assignNewReviewer(labelToReviewerMapping: { - [label: string]: string; -}): string { +export function assignNewReviewer( + labelToReviewerMapping: { + [label: string]: string; + }, + options?: AssignReviewerOptions +): string { let commentString = "Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:\n\n"; @@ -129,6 +166,8 @@ export function assignNewReviewer(labelToReviewerMapping: { } } + commentString += formatNotices(labelToReviewerMapping, options); + commentString += ` Available commands: - \`stop reviewer notifications\` - opt out of the automated review tooling diff --git a/scripts/ci/pr-bot/shared/userCommand.ts b/scripts/ci/pr-bot/shared/userCommand.ts index 6980468c3b19..1c545f36dfde 100644 --- a/scripts/ci/pr-bot/shared/userCommand.ts +++ b/scripts/ci/pr-bot/shared/userCommand.ts @@ -101,14 +101,16 @@ async function assignToNextReviewer( // Comment assigning reviewer console.log(`Assigning ${chosenReviewer}`); + const existingLabels = + payload.issue?.labels || payload.pull_request?.labels; await github.addPrComment( pullNumber, - commentStrings.assignReviewer(prState.reviewersAssignedForLabels) + commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { + labels: existingLabels, + }) ); // Set next action to reviewer - const existingLabels = - payload.issue?.labels || payload.pull_request?.labels; await github.nextActionReviewers(pullNumber, existingLabels); prState.nextAction = "Reviewers"; @@ -227,7 +229,9 @@ async function assignReviewerSet( console.log(`Assigning reviewers for pr ${pullNumber}`); await github.addPrComment( pullNumber, - commentStrings.assignReviewer(prState.reviewersAssignedForLabels) + commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { + labels: existingLabels, + }) ); github.nextActionReviewers(pullNumber, existingLabels); diff --git a/scripts/ci/pr-bot/test/commentStringsTest.ts b/scripts/ci/pr-bot/test/commentStringsTest.ts new file mode 100644 index 000000000000..f2a16ca1c634 --- /dev/null +++ b/scripts/ci/pr-bot/test/commentStringsTest.ts @@ -0,0 +1,45 @@ +/* + * 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. + */ + +var assert = require("assert"); +const commentStrings = require("../shared/commentStrings"); + +describe("commentStrings", function () { + describe("assignReviewer()", function () { + it("should not include scrutiny statement when core label is not present", function () { + const comment = commentStrings.assignReviewer( + { Java: "reviewer1" }, + { labels: [{ name: "Java" }] } + ); + assert(!comment.includes("review with scrutiny")); + }); + + it("should include scrutiny statement when core label is present in options.labels", function () { + const comment = commentStrings.assignReviewer( + { Java: "reviewer1" }, + { labels: [{ name: "core" }] } + ); + assert(comment.includes("review with scrutiny")); + }); + + it("should include scrutiny statement when core label is in mapping keys", function () { + const comment = commentStrings.assignReviewer({ core: "reviewer1" }); + assert(comment.includes("review with scrutiny")); + }); + }); +});