diff --git a/.claude/skills/review-issue/SKILL.md b/.claude/skills/review-issue/SKILL.md new file mode 100644 index 000000000..384aa137c --- /dev/null +++ b/.claude/skills/review-issue/SKILL.md @@ -0,0 +1,168 @@ +--- +name: review-issue +description: Review an incoming external issue (and any gated-closed PR behind it) and decide whether to assign the contributor or decline. Use when the maintainer says "look at this issue", "review issue #N", "should we take this", or asks whether to assign someone. Assigning the author auto-reopens their PR for normal review. This is the entry point for incoming-issue triage — distinct from review-pr, which responds to bot reviews on your own open PR. +--- + +# Triaging contributions under the issue-link gate + +FastMCP auto-closes external PRs unless the author is **assigned to a referenced issue** +(see [require-issue-link.yml](../../../.github/workflows/require-issue-link.yml)). The practical +effect: contributors open an issue, open a PR, get auto-closed, and ask to be assigned. The +maintainer almost never sees the PR directly — **the issue is the decision point**, and +**assigning the author is the single action that reopens their PR** and sends it into review. + +This skill turns "look at this issue" into one of two outcomes: +- **Assign** — the issue is valid, we want it fixed, an external PR is appropriate, and a sound + PR already exists → assign the author (auto-reopens the PR) and queue it for code review. +- **Decline** — leave the issue/PR closed and explain why on the issue. + +Be opinionated about declining. The gate moved spam from junk PRs to junk issues; this skill is +worthless if it just rubber-stamps assignment. Assignment is a commitment to review and likely +merge, not a courtesy. + +## How the gate works (the part that matters here) + +- External PR is closed unless its body has `Fixes/Closes/Resolves #N` **and** the author is + assigned to issue `#N`. +- **Assigning the author to the issue auto-reopens their closed PR** and re-runs the check — + this is the lever you pull. `gh issue edit N --add-assignee `. The assignment fires a + `require-issue-link` run; expect it to pass. If it fails, the gate itself misbehaved (not the + PR) — investigate the run, don't re-assign. +- Maintainer-authored PRs are exempt. A `trusted-contributor` label exempts a contributor up + front. Reopening the PR or removing the `missing-issue-link` label applies a sticky + `bypass-issue-check`. +- Sibling bots have usually already run on the issue: `marvin-triage-issue` (investigates + + recommends), `marvin-dedupe-issues` / `auto-close-duplicates` (dupes), `auto-close-needs-mre` + (missing MRE). Read their comments before re-deriving anything. + +## Step 1 — Orient + +Read the issue, its bot triage, and any PR behind it. Run these together: + +```bash +gh issue view N --repo PrefectHQ/fastmcp \ + --json number,title,state,author,body,labels,assignees,comments +# Find PRs the author opened that reference this issue (they're likely CLOSED): +gh pr list --repo PrefectHQ/fastmcp --state all --search "author: #N in:body" \ + --json number,title,state,url,labels +``` + +If a PR exists, pull its metadata and any review-bot comments (CodeRabbit, Codex). Treat the bot +comments as leads, not conclusions — they often don't run on closed PRs at all, and even when +they do you still owe the PR your own read: + +```bash +gh pr view --repo PrefectHQ/fastmcp --json number,title,body,labels,files,additions,deletions +gh pr view --repo PrefectHQ/fastmcp --comments +``` + +## Step 2 — Classify the issue (is it valid AND a real bug?) + +- Is there a real, reproducible problem? For bugs, demand an MRE that shows FastMCP misbehaving + — not user config error, not a question, not an upstream-SDK issue. +- Is it a duplicate or already fixed on `main`? Check the dedupe bot's comment and recent commits. +- If the issue itself is weak, **stop here and decline** — don't evaluate the PR. A good PR + attached to a bad issue is still declined. + +**A reproducible MRE is not the same as a bug.** This is the trap that produces wrong verdicts: +an MRE can demonstrate real, observable behavior that is nonetheless *not a bug*, because it +violates no contract the framework intends to hold. The decisive question is not "does this +reproduce?" but "does the demonstrated behavior violate the intended contract for this API?" A +shared-mutable-state MRE only matters if callers are *supposed* to mutate that state; an +ordering/timing MRE only matters if the framework promises an order; a "wrong" value only matters +relative to what the API guarantees. An MRE that has to reach past the supported surface to +trigger the behavior (mutating a field meant to be set only at construction, depending on an +internal that isn't part of the public contract) is showing you a property, not a defect. + +You usually cannot read the intended contract off the code — the code shows what it *does*, not +what it *promises*. **The maintainer is often the only authoritative source for the contract, so +stopping to ask is legitimate and expected here.** Ask "is X a supported pattern / does this API +promise Y?" before sinking time into investigating a fix. If the behavior is in-contract correct, +decline — no matter how cleanly the PR fixes it, and no matter how real the MRE looks. + +## Step 3 — Investigate the PR (mandatory; do NOT skip if a PR exists) + +The most common failure of this skill is judging a PR from the diff hunk and the PR description +alone. That is a cursory review and it produces wrong verdicts — a redundant-looking conditional +can be a real bug fix; a tidy-looking diff can patch the wrong layer. **You cannot assess a PR +without reading the code it changes in context.** Reading `gh pr diff` is necessary but never +sufficient. + +Do all of this before forming any opinion on quality: + +1. **Read the diff in full**, then **open every file it touches in the repo** (`Read`, not just + the patch). The hunk shows *what changed*; the file shows *what it changed into*. +2. **Trace the functions and values the change depends on.** Grep for the called functions, + the fields being set, and the defaults. If the PR overrides or replaces a value, find what + produced the original value and what consumes it downstream. +3. **Establish the actual root cause from the issue's MRE**, then check whether the change fixes + *that* — at the layer where the bug originates, not a compensating patch elsewhere. +4. **Check consistency with adjacent code.** Does the new value/behavior match how nearby code + already handles the same case? An inconsistency is a real finding; a match is evidence the fix + is correct. +5. **Run or read the tests** the PR adds/changes — do they actually exercise the bug, and would + they fail without the fix? + +Write down, for yourself, a one-line answer to: *what was broken, where, and does this change fix +it there?* If you can't answer from evidence you've actually read, you haven't investigated yet. + +Then separate findings by severity: a **cosmetic** nit (style, a redundant-but-harmless line) is a +review comment, not a blocker. A **substantive** defect (wrong layer, breaks an adjacent path, +doesn't actually fix the MRE) changes the verdict. Don't let a cosmetic nit read as a reason to +decline, and don't let a clean style read as evidence of correctness. + +## Step 4 — Decide if an external PR is appropriate (CONTRIBUTING.md) + +This is the gate CONTRIBUTING.md actually enforces. Map the change to a category: + +- **Simple, well-scoped bug fix** → external PR welcome. Assignable. +- **Docs / typo / example fix** → welcome. Assignable. +- **Auth provider** → assignable (auth is the one integration exception). +- **Enhancement / feature** → needs a maintainer-approved design proposal *in the issue first*. + Do **not** assign just because code exists. If the proposal is sound, the path is "approve the + approach in the issue, then assign" — not "assign because they were fast." +- **Third-party integration** (middleware, provider adapters, non-auth) → decline; belongs in a + separate package. +- **Sweeping / multi-subsystem change with no prior discussion** → decline. + +Combine the category with the Step 3 investigation: does it fix the cause or paper over a symptom? +Does it read like unedited LLM output (verbose body, speculative/shotgun changes)? CONTRIBUTING.md +says we close those — a closed PR that reads that way is staying closed. + +## Step 5 — Recommend, then act + +Present a short verdict to the maintainer before mutating anything: **assign** or **decline**, +one or two sentences of reasoning, and the exact command you'll run. Wait for confirmation on +borderline calls; for clear-cut ones you may proceed and report. + +**Assign** (valid issue + appropriate external contribution + sound PR exists): + +```bash +gh issue edit N --repo PrefectHQ/fastmcp --add-assignee +``` + +That reopens the PR automatically. Then hand off to code review — invoke the `code-review` / +`review-pr` skills on the reopened PR. Assignment is not approval; the code still gets the normal +pass. + +If a PR's head branch was deleted, assignment can't reopen it — the workflow comments asking the +author to open a fresh PR. Don't try to force it. + +**Decline** (invalid issue, wrong contribution type, or low-quality PR): leave it closed and +comment on the **issue** explaining the decision, pointing to the relevant CONTRIBUTING.md +section. Per repo rules, use `--body-file`, never inline `--body`, for any comment that could +contain `$`, backticks, or code: + +```bash +gh issue comment N --repo PrefectHQ/fastmcp --body-file /tmp/triage-reply.md +``` + +Keep the reply short and point to the relevant CONTRIBUTING.md section. (If a `github-reply` +skill is available for maintainer voice/tone, use it — but it isn't required.) + +## What this skill does NOT do + +- It doesn't bypass the gate via `trusted-contributor` / `bypass-issue-check` — that's a + deliberate maintainer escalation, not a triage outcome. +- It doesn't merge. Assignment → reopen → review → (maybe) merge are distinct steps. +- It doesn't re-run the first-pass triage the bots already did; read their output instead. diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index da5f1ff3d..ae37ee33a 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -94,6 +94,12 @@ After evaluating comments: Codex sometimes re-posts old comments that reference code you've already fixed (they appear on the old commit's diff). These are stale — verify the fix is in the latest commit and reply noting the fix is already in place. +## Labels — never apply or invent them + +**Do not apply labels to PRs or issues programmatically, and never create new ones.** Issues and PRs in this repo are auto-labeled by a bot based on title, body, and code changes — there's no fixed canonical list to match against, and GitHub's "add labels" API auto-creates any label name that doesn't already exist, so a typo or guessed name silently pollutes the repo's label list with a stray, uncolored duplicate. There is no MCP tool to delete a label, so a mistaken creation can only be cleaned up by hand in repo settings. + +Don't call out a "suggested" or "appropriate" label in the PR body either — the bot doesn't read it, and it just adds noise. + ## When a PR is ready A PR is ready for human review when: diff --git a/.github/actions/run-claude/action.yml b/.github/actions/run-claude/action.yml index fff6788a6..b79131462 100644 --- a/.github/actions/run-claude/action.yml +++ b/.github/actions/run-claude/action.yml @@ -37,10 +37,15 @@ inputs: required: false default: "" + extra-allowed-tools: + description: "Additional comma-separated tools to append to allowed-tools" + required: false + default: "" + model: description: "Model to use for Claude" required: false - default: "claude-opus-4-6" + default: "claude-opus-4-8" allowed-bots: description: "Allowed bot usernames, or '*' for all bots" @@ -88,7 +93,7 @@ runs: track_progress: ${{ inputs.track-progress }} prompt: ${{ inputs.prompt }} claude_args: | - ${{ (inputs.allowed-tools != '' || inputs.extra-allowed-tools != '') && format('--allowedTools {0}{1}', inputs.allowed-tools, inputs.extra-allowed-tools != '' && format(',{0}', inputs.extra-allowed-tools) || '') || '' }} + ${{ (inputs.allowed-tools != '' || inputs.extra-allowed-tools != '') && format('--allowedTools ''{0}{1}''', inputs.allowed-tools, inputs.extra-allowed-tools != '' && format(',{0}', inputs.extra-allowed-tools) || '') || '' }} ${{ inputs.mcp-servers != '' && format('--mcp-config ''{0}''', inputs.mcp-servers) || '' }} --model ${{ inputs.model }} settings: | diff --git a/.github/actions/run-pytest/action.yml b/.github/actions/run-pytest/action.yml index b7e5509e5..c82e9c0bd 100644 --- a/.github/actions/run-pytest/action.yml +++ b/.github/actions/run-pytest/action.yml @@ -19,7 +19,7 @@ runs: MAX_PROCS="2" EXTRA_FLAGS="" elif [ "${{ inputs.test-type }}" == "client_process" ]; then - MARKER="client_process" + MARKER="client_process or subprocess_heavy" TIMEOUT="5" MAX_PROCS="0" EXTRA_FLAGS="-x" @@ -29,17 +29,33 @@ runs: MAX_PROCS="0" EXTRA_FLAGS="-x" else - MARKER="not integration and not client_process and not conformance" + MARKER="not integration and not client_process and not subprocess_heavy and not conformance" TIMEOUT="5" MAX_PROCS="4" EXTRA_FLAGS="" fi + # Windows previously ran serially: parallel workers crashed intermittently + # when many tests spawned stdio subprocesses (#2715, reverted in #2726). + # Most of those tests now run in-memory, but tests that spawn a fresh + # interpreter importing all of FastMCP still crash xdist workers on the + # 2-core Windows runners. They carry the subprocess_heavy marker and run + # in the serial client_process step instead. PARALLEL_FLAGS="" - if [ "$MAX_PROCS" != "0" ] && [ "${{ runner.os }}" != "Windows" ]; then + if [ "$MAX_PROCS" != "0" ]; then PARALLEL_FLAGS="--numprocesses auto --maxprocesses $MAX_PROCS --dist worksteal" fi + # pytest-timeout has no signal-based method on Windows, so it falls back + # to the thread method, which dumps stacks and os._exit()s the process. + # Under a contended runner that turns a single slow test into a dead + # xdist worker, failing whichever unrelated test that worker happened to + # be running. Give parallel Windows runs more headroom so ordinary + # scheduling jitter does not take a worker down. + if [ "$RUNNER_OS" == "Windows" ] && [ "$MAX_PROCS" != "0" ]; then + TIMEOUT=$((TIMEOUT * 4)) + fi + uv run --no-sync pytest \ --inline-snapshot=disable \ --timeout=$TIMEOUT \ diff --git a/.github/dependabot.yml b/.github/dependabot.yml deleted file mode 100644 index 20d3ccecf..000000000 --- a/.github/dependabot.yml +++ /dev/null @@ -1,14 +0,0 @@ -version: 2 -updates: - - package-ecosystem: "pip" - directory: "/" - schedule: - interval: "daily" - labels: - - "dependencies" - - package-ecosystem: "github-actions" - directory: "/" - schedule: - interval: "weekly" - labels: - - "dependencies" diff --git a/.github/scripts/triage-label.sh b/.github/scripts/triage-label.sh new file mode 100755 index 000000000..c6bdda85c --- /dev/null +++ b/.github/scripts/triage-label.sh @@ -0,0 +1,80 @@ +#!/usr/bin/env bash +# Locked-down label helper for the Marvin triage workflow. +# +# Marvin runs on untrusted issue/PR bodies from non-write users, so it must +# NOT be handed raw `gh api` (that would expose every endpoint the app token +# can reach). This helper is the ONLY GitHub write it is allowed to perform: +# it adds or removes repository labels on the one issue/PR being triaged. +# +# The target repo and number come from the environment set by the workflow — +# never from the model — and the operation is fixed to the additive labels +# endpoint (POST/DELETE /repos/{repo}/issues/{n}/labels), which works for both +# issues and PRs and cannot clobber labels applied by other workflows. +set -euo pipefail + +repo="${TRIAGE_REPO:?TRIAGE_REPO not set}" +number="${TRIAGE_NUMBER:?TRIAGE_NUMBER not set}" + +if [[ ! "$number" =~ ^[0-9]+$ ]]; then + echo "TRIAGE_NUMBER must be numeric, got: $number" >&2 + exit 1 +fi + +op="${1:-}" +shift || true +case "$op" in + add) method=POST ;; + remove) method=DELETE ;; + *) + echo "usage: triage-label.sh