Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .agent/skills/contributing/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
21 changes: 20 additions & 1 deletion .github/autolabeler.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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/**/*"]
Expand Down Expand Up @@ -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/**/*"]
Expand Down
4 changes: 3 additions & 1 deletion scripts/ci/pr-bot/findPrsNeedingAttention.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion scripts/ci/pr-bot/processNewPrs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
47 changes: 43 additions & 4 deletions scripts/ci/pr-bot/shared/commentStrings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string> = {
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(" ")}`;
}
Expand All @@ -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";

Expand All @@ -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\`.
Expand Down Expand Up @@ -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";

Expand All @@ -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
Expand Down
12 changes: 8 additions & 4 deletions scripts/ci/pr-bot/shared/userCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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);
Expand Down
45 changes: 45 additions & 0 deletions scripts/ci/pr-bot/test/commentStringsTest.ts
Original file line number Diff line number Diff line change
@@ -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"));
});
});
});
Loading