diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 6658ebc9fd3..a790437b7e0 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,23 +1,43 @@ -### Motivation and Context +### Motivation & Context -### Description +### Description & Review Guide +- **What are the major changes?** +- **What is the impact of these changes?** +- **What do you want reviewers to focus on?** + + + +### Related Issue + + + +Fixes # + ### Contribution Checklist - [ ] The code builds clean without any errors or warnings -- [ ] The PR follows the [Contribution Guidelines](https://github.com/microsoft/agent-framework/blob/main/CONTRIBUTING.md) - [ ] All unit tests pass, and I have added new tests where possible -- [ ] **Is this a breaking change?** If yes, add "[BREAKING]" prefix to the title of the PR. \ No newline at end of file +- [ ] The PR follows the [Contribution Guidelines](https://github.com/microsoft/agent-framework/blob/main/CONTRIBUTING.md) +- [ ] This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above). +- [x] **This is not a breaking change.** If it _is_ a breaking change, add the `breaking change` label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically. diff --git a/.github/scripts/title_prefix.js b/.github/scripts/title_prefix.js new file mode 100644 index 00000000000..f9a7442847e --- /dev/null +++ b/.github/scripts/title_prefix.js @@ -0,0 +1,252 @@ +// Copyright (c) Microsoft. All rights reserved. + +const BREAKING_CHANGE_LABEL = 'breaking change'; +const BREAKING_PREFIX = '[BREAKING]'; + +const DEFAULT_PREFIX_LABELS = Object.freeze({ + python: 'Python', + '.NET': '.NET', +}); + +const DEFAULT_BRACKET_PREFIX_LABELS = Object.freeze({ + [BREAKING_CHANGE_LABEL]: BREAKING_PREFIX, +}); + +function escapeRegExp(value) { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +function getMatchingValueByKey(valuesByKey, keyToFind) { + const matchingKey = Object.keys(valuesByKey).find((key) => key.toLowerCase() === keyToFind.toLowerCase()); + return matchingKey === undefined ? null : valuesByKey[matchingKey]; +} + +function getPrefixPattern(prefixes) { + return prefixes.map(escapeRegExp).join('|'); +} + +function canonicalizePrefix(prefix, prefixes) { + return prefixes.find((knownPrefix) => knownPrefix.toLowerCase() === prefix.toLowerCase()) ?? prefix; +} + +function normalizeLeadingBracketPrefix(title, bracketPrefixes) { + const bracketPattern = getPrefixPattern(bracketPrefixes); + if (!bracketPattern) { + return title; + } + + const leadingBracketPrefix = new RegExp(`^(${bracketPattern})(?=\\s|$)`, 'i'); + return title.replace( + leadingBracketPrefix, + (bracketPrefix) => canonicalizePrefix(bracketPrefix, bracketPrefixes), + ); +} + +function parseLeadingTitlePrefix(title, titlePrefixes) { + const titlePrefixPattern = getPrefixPattern(titlePrefixes); + if (!titlePrefixPattern) { + return null; + } + + const match = title.match(new RegExp(`^(${titlePrefixPattern}):\\s*`, 'i')); + if (!match) { + return null; + } + + return { + prefix: canonicalizePrefix(match[1], titlePrefixes), + rest: title.slice(match[0].length).trimStart(), + }; +} + +function removeBracketPrefixToken(title, bracketPrefix) { + const bracketPrefixPattern = escapeRegExp(bracketPrefix); + return title + .replace(new RegExp(`(^|\\s+)${bracketPrefixPattern}(?=\\s|$)`, 'ig'), '$1') + .replace(/\s{2,}/g, ' ') + .trim(); +} + +function addTitlePrefix(title, prefix, bracketPrefixes = Object.values(DEFAULT_BRACKET_PREFIX_LABELS)) { + const bracketPattern = getPrefixPattern(bracketPrefixes); + const prefixPattern = escapeRegExp(prefix); + + if (bracketPattern) { + const bracketThenTitlePrefix = new RegExp(`^(${bracketPattern})(\\s+)(${prefixPattern})(?=:)`, 'i'); + if (bracketThenTitlePrefix.test(title)) { + return title.replace( + bracketThenTitlePrefix, + (match, bracketPrefix, spacing) => `${canonicalizePrefix(bracketPrefix, bracketPrefixes)}${spacing}${prefix}`, + ); + } + + title = normalizeLeadingBracketPrefix(title, bracketPrefixes); + } + + if (!title.startsWith(`${prefix}: `)) { + if (title.match(new RegExp(`^${prefixPattern}`, 'i'))) { + return title.replace(new RegExp(`^${prefixPattern}`, 'i'), prefix); + } + + return `${prefix}: ${title}`; + } + + return title; +} + +function hasBracketPrefix(title, bracketPrefix, titlePrefixes = Object.values(DEFAULT_PREFIX_LABELS)) { + const bracketPrefixPattern = escapeRegExp(bracketPrefix); + const leadingBracketPrefix = new RegExp(`^${bracketPrefixPattern}(?=\\s|$)`, 'i'); + if (leadingBracketPrefix.test(title)) { + return true; + } + + const leadingTitlePrefix = parseLeadingTitlePrefix(title, titlePrefixes); + if (!leadingTitlePrefix) { + return false; + } + + return leadingBracketPrefix.test(leadingTitlePrefix.rest); +} + +function addBracketPrefix(title, bracketPrefix, titlePrefixes = Object.values(DEFAULT_PREFIX_LABELS)) { + const bracketPrefixPattern = escapeRegExp(bracketPrefix); + const leadingBracketPrefix = new RegExp(`^${bracketPrefixPattern}(?=\\s|$)`, 'i'); + if (leadingBracketPrefix.test(title)) { + return title.replace(leadingBracketPrefix, bracketPrefix); + } + + const leadingTitlePrefix = parseLeadingTitlePrefix(title, titlePrefixes); + if (leadingTitlePrefix) { + if (leadingBracketPrefix.test(leadingTitlePrefix.rest)) { + const normalizedRest = leadingTitlePrefix.rest.replace(leadingBracketPrefix, bracketPrefix); + return `${leadingTitlePrefix.prefix}: ${normalizedRest}`; + } + + const titleWithoutBracketPrefix = removeBracketPrefixToken(leadingTitlePrefix.rest, bracketPrefix); + return `${leadingTitlePrefix.prefix}: ${bracketPrefix}` + + (titleWithoutBracketPrefix ? ` ${titleWithoutBracketPrefix}` : ''); + } + + const titleWithoutBracketPrefix = removeBracketPrefixToken(title, bracketPrefix); + return `${bracketPrefix}${titleWithoutBracketPrefix ? ` ${titleWithoutBracketPrefix}` : ''}`; +} + +function hasLabel(labels, labelName) { + return labels.some((label) => label.toLowerCase() === labelName.toLowerCase()); +} + +function getCurrentTitle(context) { + switch (context.eventName) { + case 'issues': + return context.payload.issue.title; + case 'pull_request_target': + return context.payload.pull_request.title; + default: + throw new Error(`Unrecognized eventName: ${context.eventName}`); + } +} + +async function updateTitleForAddedLabel({ + github, + context, + core, + prefixLabels = DEFAULT_PREFIX_LABELS, + bracketPrefixLabels = DEFAULT_BRACKET_PREFIX_LABELS, +}) { + const labelAdded = context.payload.label?.name; + if (!labelAdded) { + throw new Error('This script must be run from a labeled event.'); + } + + const currentTitle = getCurrentTitle(context); + let newTitle = null; + + const titlePrefix = getMatchingValueByKey(prefixLabels, labelAdded); + if (titlePrefix !== null) { + newTitle = addTitlePrefix(currentTitle, titlePrefix, Object.values(bracketPrefixLabels)); + } + + const bracketPrefix = getMatchingValueByKey(bracketPrefixLabels, labelAdded); + if (bracketPrefix !== null) { + newTitle = addBracketPrefix(currentTitle, bracketPrefix, Object.values(prefixLabels)); + } + + if (newTitle === null) { + core.info(`No title prefix configured for label "${labelAdded}".`); + return { updated: false, newTitle: currentTitle }; + } + + if (newTitle === currentTitle) { + core.info(`Title already includes the prefix for label "${labelAdded}".`); + return { updated: false, newTitle }; + } + + switch (context.eventName) { + case 'issues': + await github.rest.issues.update({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + title: newTitle, + }); + break; + + case 'pull_request_target': + await github.rest.pulls.update({ + pull_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + title: newTitle, + }); + break; + + default: + throw new Error(`Unrecognized eventName: ${context.eventName}`); + } + + return { updated: true, newTitle }; +} + +async function syncBreakingChangeLabelFromTitle({ + github, + context, + core, + labelName = BREAKING_CHANGE_LABEL, + bracketPrefix = BREAKING_PREFIX, + titlePrefixes = Object.values(DEFAULT_PREFIX_LABELS), +}) { + const pullRequest = context.payload.pull_request; + if (!pullRequest) { + throw new Error('This script must be run from a pull_request_target event.'); + } + + const title = pullRequest.title || ''; + if (!hasBracketPrefix(title, bracketPrefix, titlePrefixes)) { + core.info(`Title does not include ${bracketPrefix} in the title prefix.`); + return { added: false }; + } + + const labels = pullRequest.labels?.map((label) => label.name).filter(Boolean) ?? []; + if (hasLabel(labels, labelName)) { + core.info(`PR already has the "${labelName}" label.`); + return { added: false }; + } + + await github.rest.issues.addLabels({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + labels: [labelName], + }); + + return { added: true }; +} + +module.exports = { + addBracketPrefix, + addTitlePrefix, + hasBracketPrefix, + syncBreakingChangeLabelFromTitle, + updateTitleForAddedLabel, +}; diff --git a/.github/skills/pull-requests/SKILL.md b/.github/skills/pull-requests/SKILL.md new file mode 100644 index 00000000000..c8ec894e421 --- /dev/null +++ b/.github/skills/pull-requests/SKILL.md @@ -0,0 +1,116 @@ +--- +name: pull-requests +description: > + Guidance for creating pull requests and handling PR review comments in the + Agent Framework repository. Use this when writing a PR description (filling out + the PR template) or when responding to and resolving review comments on an + existing PR. +--- + +# Pull Request Workflow + +This skill covers two tasks: (1) writing a high-quality PR description, and +(2) handling review comments on an existing PR. + +## 1. Writing the PR description + +Always follow the repository PR template at +[`.github/pull_request_template.md`](../../pull_request_template.md). Keep its +exact structure and headings. Fill every section: + +### `### Motivation & Context` +Explain *why* the change is needed: the problem it solves and the scenario it +contributes to. Describe the net change relative to `main` — this is implied, so +do **not** spell out "vs main" explicitly. + +### `### Description & Review Guide` +Describe the changes, the overall approach, and the design. Answer the three +prompts: +- **What are the major changes?** +- **What is the impact of these changes?** +- **What do you want reviewers to focus on?** — This item is for **human + reviewers only**. Automated/AI reviewers must ignore it and review the entire + change rather than narrowing scope to it. + +### `### Related Issue` +Link the issue the PR fixes using a GitHub closing keyword (`Fixes #123` / +`Closes #123`) so it closes automatically on merge. A PR with no linked issue may +be closed regardless of how valid the change is. Before opening, confirm there is +no other open PR for the same issue; if there is, explain how this PR differs. + +### `### Contribution Checklist` +Check every item that applies. For the breaking-change item: +- Leave **"This is not a breaking change."** checked for the common case. +- If the change **is** breaking, add the `breaking change` label **or** put + `[BREAKING]` in the title prefix, before or after a language prefix such as + `Python:` or `.NET:` — workflows keep the label and the title prefix in sync + automatically (see `.github/workflows/label-title-prefix.yml` and + `.github/workflows/label-pr.yml`). + +### Do not +- Do **not** add ad-hoc sections such as "Validation" or "Tests run"; CI/CD and + the checklist already cover validation status. +- Do **not** remove or reorder the template's headings. + +### Creating the PR +Open new PRs as **drafts** until they are ready for review. Example: + +```bash +gh pr create --repo microsoft/agent-framework --base main \ + --head : --draft \ + --title "" --body "" +``` + +## 2. Handling review comments + +When a PR receives review comments, follow this sequence — **do not start editing +code before the user has reviewed the plan**: + +1. **Review the comments.** Read every review comment and thread on the PR, + including inline code comments and general review summaries. +2. **Make a plan.** Produce a concrete plan describing how each comment will be + addressed (or why it should not be, with reasoning). +3. **Let the user review the plan.** Present the plan and wait for the user's + approval or adjustments before implementing anything. +4. **Implement.** Make the agreed changes. +5. **Reply to every comment.** Add a reply to **all** comments explaining how it + was addressed (or the agreed outcome) — leave none unanswered. +6. **Resolve resolved threads.** Mark a review thread as resolved only when the + comment has actually been addressed. + +### Useful commands + +List review comments and threads: + +```bash +# Inline review comments +gh api repos/{owner}/{repo}/pulls/{pr}/comments + +# Review threads with resolution state (GraphQL) +gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$pr){ + reviewThreads(first:100){ + nodes{ id isResolved comments(first:50){ nodes{ id body author{login} } } } + } + } + } + }' -F owner={owner} -F repo={repo} -F pr={pr} +``` + +Reply to an inline review comment: + +```bash +gh api repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies \ + -f body="Addressed in : " +``` + +Resolve a review thread (needs the thread node id from the GraphQL query above): + +```bash +gh api graphql -f query=' + mutation($threadId:ID!){ + resolveReviewThread(input:{threadId:$threadId}){ thread{ isResolved } } + }' -F threadId={thread_id} +``` diff --git a/.github/workflows/dotnet-build-and-test.yml b/.github/workflows/dotnet-build-and-test.yml index 6582240a5cb..7585cb9e313 100644 --- a/.github/workflows/dotnet-build-and-test.yml +++ b/.github/workflows/dotnet-build-and-test.yml @@ -48,6 +48,10 @@ jobs: filters: | dotnet: - 'dotnet/**' + - '!dotnet/AGENTS.md' + - '!dotnet/**/AGENTS.md' + - '!dotnet/.github/skills/*' + - '!dotnet/.github/skills/**' cosmosdb: - 'dotnet/src/Microsoft.Agents.AI.CosmosNoSql/**' # The Foundry hosted-agent IT is costly (builds a container, pushes to ACR, diff --git a/.github/workflows/dotnet-format.yml b/.github/workflows/dotnet-format.yml index b9672967ef8..118ded3bfd4 100644 --- a/.github/workflows/dotnet-format.yml +++ b/.github/workflows/dotnet-format.yml @@ -10,6 +10,10 @@ on: branches: ["main", "feature*"] paths: - dotnet/** + - '!dotnet/AGENTS.md' + - '!dotnet/**/AGENTS.md' + - '!dotnet/.github/skills/*' + - '!dotnet/.github/skills/**' - '.github/workflows/dotnet-format.yml' concurrency: diff --git a/.github/workflows/label-pr.yml b/.github/workflows/label-pr.yml index 7d0282b9160..329a165cd2d 100644 --- a/.github/workflows/label-pr.yml +++ b/.github/workflows/label-pr.yml @@ -6,16 +6,34 @@ # https://github.com/actions/labeler name: Label pull request -on: [pull_request_target] +on: + pull_request_target: + types: [opened, synchronize, reopened, edited] jobs: add_label: runs-on: ubuntu-latest permissions: contents: read + issues: write pull-requests: write steps: - uses: actions/labeler@f27b608878404679385c85cfa523b85ccb86e213 # v6 with: repo-token: "${{ secrets.GH_ACTIONS_PR_WRITE }}" + + - name: Checkout scripts + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + with: + sparse-checkout: .github/scripts + fetch-depth: 1 + persist-credentials: false + + - name: "PR: add breaking change label from title" + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 + with: + github-token: ${{ secrets.GH_ACTIONS_PR_WRITE }} + script: | + const { syncBreakingChangeLabelFromTitle } = require('./.github/scripts/title_prefix.js'); + await syncBreakingChangeLabelFromTitle({ github, context, core }); diff --git a/.github/workflows/label-title-prefix.yml b/.github/workflows/label-title-prefix.yml index 8457e8e4289..4533ed0988e 100644 --- a/.github/workflows/label-title-prefix.yml +++ b/.github/workflows/label-title-prefix.yml @@ -15,58 +15,17 @@ jobs: pull-requests: write steps: + - name: Checkout scripts + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + with: + sparse-checkout: .github/scripts + fetch-depth: 1 + persist-credentials: false + - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 name: "Issue/PR: update title" with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - let prefixLabels = { - "python": "Python", - ".NET": ".NET" - }; - - function addTitlePrefix(title, prefix) - { - // Update the title based on the label and prefix - // Check if the title starts with the prefix (case-sensitive) - if (!title.startsWith(prefix + ": ")) { - // If not, check if the first word is the label (case-insensitive) - if (title.match(new RegExp(`^${prefix}`, 'i'))) { - // If yes, replace it with the prefix (case-sensitive) - title = title.replace(new RegExp(`^${prefix}`, 'i'), prefix); - } else { - // If not, prepend the prefix to the title - title = prefix + ": " + title; - } - } - - return title; - } - - labelAdded = context.payload.label.name - - // Check if the issue or PR has the label - if (labelAdded in prefixLabels) { - let prefix = prefixLabels[labelAdded]; - switch(context.eventName) { - case 'issues': - github.rest.issues.update({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - title: addTitlePrefix(context.payload.issue.title, prefix) - }); - break - - case 'pull_request_target': - github.rest.pulls.update({ - pull_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - title: addTitlePrefix(context.payload.pull_request.title, prefix) - }); - break - default: - core.setFailed('Unrecognited eventName: ' + context.eventName); - } - } + const { updateTitleForAddedLabel } = require('./.github/scripts/title_prefix.js'); + await updateTitleForAddedLabel({ github, context, core }); diff --git a/.github/workflows/python-code-quality.yml b/.github/workflows/python-code-quality.yml index 6527a89cd83..a548dc838ef 100644 --- a/.github/workflows/python-code-quality.yml +++ b/.github/workflows/python-code-quality.yml @@ -6,6 +6,10 @@ on: branches: ["main"] paths: - "python/**" + - "!python/AGENTS.md" + - "!python/**/AGENTS.md" + - "!python/.github/skills/*" + - "!python/.github/skills/**" env: # Configure a constant location for the uv cache diff --git a/.github/workflows/python-lab-tests.yml b/.github/workflows/python-lab-tests.yml index 3f959f85c25..8e5eeb68fcf 100644 --- a/.github/workflows/python-lab-tests.yml +++ b/.github/workflows/python-lab-tests.yml @@ -31,6 +31,10 @@ jobs: filters: | python: - 'python/**' + - '!python/AGENTS.md' + - '!python/**/AGENTS.md' + - '!python/.github/skills/*' + - '!python/.github/skills/**' # run only if 'python' files were changed - name: python tests if: steps.filter.outputs.python == 'true' diff --git a/.github/workflows/python-merge-tests.yml b/.github/workflows/python-merge-tests.yml index b1b1a8627b8..2ab6d39c4e3 100644 --- a/.github/workflows/python-merge-tests.yml +++ b/.github/workflows/python-merge-tests.yml @@ -49,6 +49,10 @@ jobs: filters: | python: - 'python/**' + - '!python/AGENTS.md' + - '!python/**/AGENTS.md' + - '!python/.github/skills/*' + - '!python/.github/skills/**' - '.github/actions/setup-local-mcp-server/**' - '.github/workflows/python-merge-tests.yml' - '.github/workflows/python-integration-tests.yml' diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 955fc9054d4..022abf3d04a 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -5,6 +5,10 @@ on: branches: ["main", "feature*"] paths: - "python/**" + - "!python/AGENTS.md" + - "!python/**/AGENTS.md" + - "!python/.github/skills/*" + - "!python/.github/skills/**" env: # Configure a constant location for the uv cache UV_CACHE_DIR: /tmp/.uv-cache diff --git a/dotnet/.github/skills/pull-requests b/dotnet/.github/skills/pull-requests new file mode 120000 index 00000000000..d70f65f7f26 --- /dev/null +++ b/dotnet/.github/skills/pull-requests @@ -0,0 +1 @@ +../../../.github/skills/pull-requests \ No newline at end of file diff --git a/dotnet/AGENTS.md b/dotnet/AGENTS.md index 965dd9f035d..7cf79f31201 100644 --- a/dotnet/AGENTS.md +++ b/dotnet/AGENTS.md @@ -10,6 +10,10 @@ See `./.github/skills/build-and-test/SKILL.md` for detailed instructions on buil See `./.github/skills/project-structure/SKILL.md` for an overview of the project structure. +## Pull Requests + +See `./.github/skills/pull-requests/SKILL.md` for guidance on writing PR descriptions and handling/resolving PR review comments. + ### Core types - `AIAgent`: The abstract base class that all agents derive from, providing common methods for interacting with an agent. diff --git a/python/.github/skills/pull-requests b/python/.github/skills/pull-requests new file mode 120000 index 00000000000..d70f65f7f26 --- /dev/null +++ b/python/.github/skills/pull-requests @@ -0,0 +1 @@ +../../../.github/skills/pull-requests \ No newline at end of file diff --git a/python/AGENTS.md b/python/AGENTS.md index 8dc259c42bb..90d84aaf542 100644 --- a/python/AGENTS.md +++ b/python/AGENTS.md @@ -14,6 +14,7 @@ Instructions for AI coding agents working in the Python codebase. - `python-feature-lifecycle` — package vs feature lifecycle stages, decorators, enums, and promotion guidance - `python-package-management` — monorepo structure, lazy loading, versioning, new packages - `python-samples` — sample file structure, PEP 723, documentation guidelines +- `pull-requests` — writing PR descriptions (template) and handling/resolving PR review comments ## Maintaining Documentation