diff --git a/.claude/hooks/session-init.sh b/.claude/hooks/session-init.sh new file mode 100755 index 000000000..3c767fc54 --- /dev/null +++ b/.claude/hooks/session-init.sh @@ -0,0 +1,26 @@ +#!/bin/bash +set -e + +# Only run in remote/cloud environments +if [ "$CLAUDE_CODE_REMOTE" != "true" ]; then + exit 0 +fi + +command -v gh &> /dev/null && exit 0 + +LOCAL_BIN="$HOME/.local/bin" +mkdir -p "$LOCAL_BIN" + +ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') +VERSION=$(curl -fsSL https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d'"' -f4) +TARBALL="gh_${VERSION#v}_linux_${ARCH}.tar.gz" + +echo "Installing gh ${VERSION}..." +TEMP=$(mktemp -d) +trap 'rm -rf "$TEMP"' EXIT +curl -fsSL "https://github.com/cli/cli/releases/download/${VERSION}/${TARBALL}" | tar -xz -C "$TEMP" +cp "$TEMP"/gh_*/bin/gh "$LOCAL_BIN/gh" +chmod 755 "$LOCAL_BIN/gh" + +[ -n "$CLAUDE_ENV_FILE" ] && echo "export PATH=\"$LOCAL_BIN:\$PATH\"" >> "$CLAUDE_ENV_FILE" +echo "gh installed: $("$LOCAL_BIN/gh" --version | head -1)" diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..afc82c2ea --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "SessionStart": [ + { + "hooks": [ + { + "type": "command", + "command": "bash \"$CLAUDE_PROJECT_DIR\"/.claude/hooks/session-init.sh", + "timeout": 120 + } + ] + } + ] + } +} diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md new file mode 100644 index 000000000..bcc2698dd --- /dev/null +++ b/.claude/skills/code-review/SKILL.md @@ -0,0 +1,101 @@ +--- +name: reviewing-code +description: Review code for quality, maintainability, and correctness. Use when reviewing pull requests, evaluating code changes, or providing feedback on implementations. Focuses on API design, patterns, and actionable feedback. +--- + +# Code Review + +## Philosophy + +Code review maintains a healthy codebase while helping contributors succeed. The burden of proof is on the PR to demonstrate it adds value. Your job is to help it get there through actionable feedback. + +**Critical**: A perfectly written PR that adds unwanted functionality must still be rejected. The code must advance the codebase in the intended direction. When rejecting, provide clear guidance on how to align with project goals. + +Be friendly and welcoming while maintaining high standards. Call out what works well. When code needs improvement, be specific about why and how to fix it. + +## What to Focus On + +### Does this advance the codebase correctly? + +Even perfect code for unwanted features should be rejected. + +### Dependency version compatibility + +When a PR adapts code to a new version of a dependency (e.g., removing a parameter that was dropped upstream, using a new API): +- **The version pin in `pyproject.toml` must match.** If the change breaks compatibility with the previously-pinned minimum version, the minimum version must be bumped. Otherwise users on the old version get a regression. +- **If backwards compatibility with the old version is desired**, the code must handle both versions (e.g., try/except, version check). Simply deleting the old API usage without bumping the pin is always wrong — it silently breaks users on the old version. +- **Lock file (`uv.lock`) changes should be scoped to the PR's purpose.** A PR fixing a ty compatibility issue should not also include unrelated dependency version bumps (anthropic, google-auth, etc.) from running `uv sync --upgrade`. These create noise and make the diff harder to review. + +### API design and naming + +Identify confusing patterns or non-idiomatic code: +- Parameter values that contradict defaults +- Mutable default arguments +- Unclear naming that will confuse future readers +- Inconsistent patterns with the rest of the codebase + +### Specific improvements + +Provide actionable feedback, not generic observations. + +### User ergonomics + +Think about the API from a user's perspective. Is it intuitive? What's the learning curve? + +## For Agent Reviewers + +1. **Read the full context**: Examine related files, tests, and documentation before reviewing +2. **Check against established patterns**: Look for consistency with codebase conventions +3. **Verify functionality claims**: Understand what the code actually does, not just what it claims +4. **Consider edge cases**: Think through error conditions and boundary scenarios + +## What to Avoid + +- Generic feedback without specifics +- Hypothetical problems unlikely to occur +- Nitpicking organizational choices without strong reason +- Summarizing what the PR already describes +- Star ratings or excessive emojis +- Bikeshedding style preferences when functionality is correct +- Requesting changes without suggesting solutions +- Focusing on personal coding style over project conventions + +## Tone + +- Acknowledge good decisions: "This API design is clean" +- Be direct but respectful +- Explain impact: "This will confuse users because..." +- Remember: Someone else maintains this code forever + +## Decision Framework + +Before approving, ask: + +1. Does this PR achieve its stated purpose? +2. Is that purpose aligned with where the codebase should go? +3. Would I be comfortable maintaining this code? +4. Have I actually understood what it does, not just what it claims? +5. Does this change introduce technical debt? + +If something needs work, your review should help it get there through specific, actionable feedback. If it's solving the wrong problem, say so clearly. + +## Comment Examples + +**Good comments:** + +| Instead of | Write | +|------------|-------| +| "Add more tests" | "The `handle_timeout` method needs tests for the edge case where timeout=0" | +| "This API is confusing" | "The parameter name `data` is ambiguous - consider `message_content` to match the MCP specification" | +| "This could be better" | "This approach works but creates a circular dependency. Consider moving the validation to `utils/validators.py`" | + +## Checklist + +Before approving, verify: + +- [ ] All required development workflow steps completed (uv sync, prek, pytest) +- [ ] Changes align with repository patterns and conventions +- [ ] API changes are documented and backwards-compatible where possible +- [ ] Error handling follows project patterns (specific exception types) +- [ ] Tests cover new functionality and edge cases +- [ ] The change advances the codebase in the intended direction diff --git a/.claude/skills/python-tests/SKILL.md b/.claude/skills/python-tests/SKILL.md new file mode 100644 index 000000000..4193f6144 --- /dev/null +++ b/.claude/skills/python-tests/SKILL.md @@ -0,0 +1,220 @@ +--- +name: testing-python +description: Write and evaluate effective Python tests using pytest. Use when writing tests, reviewing test code, debugging test failures, or improving test coverage. Covers test design, fixtures, parameterization, mocking, and async testing. +--- + +# Writing Effective Python Tests + +## Core Principles + +Every test should be **atomic**, **self-contained**, and test **single functionality**. A test that tests multiple things is harder to debug and maintain. + +## Test Structure + +### Atomic unit tests + +Each test should verify a single behavior. The test name should tell you what's broken when it fails. Multiple assertions are fine when they all verify the same behavior. + +```python +# Good: Name tells you what's broken +def test_user_creation_sets_defaults(): + user = User(name="Alice") + assert user.role == "member" + assert user.id is not None + assert user.created_at is not None + +# Bad: If this fails, what behavior is broken? +def test_user(): + user = User(name="Alice") + assert user.role == "member" + user.promote() + assert user.role == "admin" + assert user.can_delete_others() +``` + +### Use parameterization for variations of the same concept + +```python +import pytest + +@pytest.mark.parametrize("input,expected", [ + ("hello", "HELLO"), + ("World", "WORLD"), + ("", ""), + ("123", "123"), +]) +def test_uppercase_conversion(input, expected): + assert input.upper() == expected +``` + +### Use separate tests for different functionality + +Don't parameterize unrelated behaviors. If the test logic differs, write separate tests. + +## Project-Specific Rules + +### No async markers needed + +This project uses `asyncio_mode = "auto"` globally. Write async tests without decorators: + +```python +# Correct +async def test_async_operation(): + result = await some_async_function() + assert result == expected + +# Wrong - don't add this +@pytest.mark.asyncio +async def test_async_operation(): + ... +``` + +### Imports at module level + +Put ALL imports at the top of the file: + +```python +# Correct +import pytest +from fastmcp import FastMCP +from fastmcp.client import Client + +async def test_something(): + mcp = FastMCP("test") + ... + +# Wrong - no local imports +async def test_something(): + from fastmcp import FastMCP # Don't do this + ... +``` + +### Use in-memory transport for testing + +Pass FastMCP servers directly to clients: + +```python +from fastmcp import FastMCP +from fastmcp.client import Client + +mcp = FastMCP("TestServer") + +@mcp.tool +def greet(name: str) -> str: + return f"Hello, {name}!" + +async def test_greet_tool(): + async with Client(mcp) as client: + result = await client.call_tool("greet", {"name": "World"}) + assert result[0].text == "Hello, World!" +``` + +Only use HTTP transport when explicitly testing network features. + +### Inline snapshots for complex data + +Use `inline-snapshot` for testing JSON schemas and complex structures: + +```python +from inline_snapshot import snapshot + +def test_schema_generation(): + schema = generate_schema(MyModel) + assert schema == snapshot() # Will auto-populate on first run +``` + +Commands: +- `pytest --inline-snapshot=create` - populate empty snapshots +- `pytest --inline-snapshot=fix` - update after intentional changes + +## Fixtures + +### Prefer function-scoped fixtures + +```python +@pytest.fixture +def client(): + return Client() + +async def test_with_client(client): + result = await client.ping() + assert result is not None +``` + +### Use `tmp_path` for file operations + +```python +def test_file_writing(tmp_path): + file = tmp_path / "test.txt" + file.write_text("content") + assert file.read_text() == "content" +``` + +## Mocking + +### Mock at the boundary + +```python +from unittest.mock import patch, AsyncMock + +async def test_external_api_call(): + with patch("mymodule.external_client.fetch", new_callable=AsyncMock) as mock: + mock.return_value = {"data": "test"} + result = await my_function() + assert result == {"data": "test"} +``` + +### Don't mock what you own + +Test your code with real implementations when possible. Mock external services, not internal classes. + +## Test Naming + +Use descriptive names that explain the scenario: + +```python +# Good +def test_login_fails_with_invalid_password(): +def test_user_can_update_own_profile(): +def test_admin_can_delete_any_user(): + +# Bad +def test_login(): +def test_update(): +def test_delete(): +``` + +## Error Testing + +```python +import pytest + +def test_raises_on_invalid_input(): + with pytest.raises(ValueError, match="must be positive"): + calculate(-1) + +async def test_async_raises(): + with pytest.raises(ConnectionError): + await connect_to_invalid_host() +``` + +## Running Tests + +```bash +uv run pytest -n auto # Run all tests in parallel +uv run pytest -n auto -x # Stop on first failure +uv run pytest path/to/test.py # Run specific file +uv run pytest -k "test_name" # Run tests matching pattern +uv run pytest -m "not integration" # Exclude integration tests +``` + +## Checklist + +Before submitting tests: +- [ ] Each test tests one thing +- [ ] No `@pytest.mark.asyncio` decorators +- [ ] Imports at module level +- [ ] Descriptive test names +- [ ] Using in-memory transport (not HTTP) unless testing networking +- [ ] Parameterization for variations of same behavior +- [ ] Separate tests for different behaviors 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 new file mode 100644 index 000000000..ae37ee33a --- /dev/null +++ b/.claude/skills/review-pr/SKILL.md @@ -0,0 +1,108 @@ +--- +name: review-pr +description: Monitor and respond to automated PR reviews (Codex bot). Use when pushing a PR, checking review status, or responding to bot feedback. Handles the full cycle of push -> wait for review -> evaluate comments -> fix -> re-push. +--- + +# PR Review Workflow + +This repo has `chatgpt-codex-connector[bot]` configured as an automated reviewer. After every push to a PR branch, Codex reviews the diff and either: +- Reacts with a thumbs-up on its review body (no suggestions — PR is clean) +- Posts inline comments with suggestions (each tagged with a priority badge) + +## Checking review status + +After pushing, check whether Codex has reviewed the latest commit: + +```bash +# Get the latest commit SHA on the branch +LATEST=$(git rev-parse HEAD) + +# Check if Codex has reviewed that specific commit +gh api repos/PrefectHQ/fastmcp/pulls/{PR_NUMBER}/reviews \ + | jq "[.[] | select(.user.login == \"chatgpt-codex-connector[bot]\" and .commit_id == \"$LATEST\")] | length" +``` + +If the count is 0, Codex hasn't reviewed the latest push yet. Wait and check again. + +If the count is > 0, check for inline comments on the latest review: + +```bash +# Get the review body to check for thumbs-up +gh api repos/PrefectHQ/fastmcp/pulls/{PR_NUMBER}/reviews \ + | jq '[.[] | select(.user.login == "chatgpt-codex-connector[bot]") | {state, body: .body[:300], commit_id: .commit_id}] | last' +``` + +A clean review from Codex looks like a review body that contains a thumbs-up reaction or says "no suggestions." If the body contains "Here are some automated review suggestions," there are inline comments to evaluate. + +## Evaluating Codex comments + +Fetch all inline comments from Codex: + +```bash +gh api repos/PrefectHQ/fastmcp/pulls/{PR_NUMBER}/comments \ + | jq '[.[] | select(.user.login == "chatgpt-codex-connector[bot]") | {body, path, line, created_at}]' +``` + +Codex comments include priority badges: +- `P0` (red) — Critical issue, likely a real bug +- `P1` (orange) — Important, worth fixing +- `P2` (yellow) — Moderate, evaluate on merit + +**How to evaluate Codex comments:** + +1. **Treat Codex as a competent but sometimes overzealous reviewer.** It catches real bugs (cache eviction ordering, silent data loss, missing validation) but also suggests scope expansions and hypothetical improvements. + +2. **Fix real bugs** — issues in code you actually changed where behavior is incorrect or data is silently lost. + +3. **Dismiss scope expansion** — if a comment points out a pre-existing limitation unrelated to your diff, note it as a potential follow-up but don't block the PR. + +4. **Dismiss speculative concerns** — if a comment describes a scenario that requires very specific conditions and the existing behavior is acceptable, dismiss it. + +5. **When fixing, be proactive** — if Codex found one instance of a pattern bug (e.g., missing role validation in one handler), check all similar code paths before pushing. Codex will find the next instance on the next review cycle, so get ahead of it. + +## Responding to every comment + +**Every Codex comment must get a visible response** — either a fix or a reply explaining why it was dismissed. The maintainer can't see your reasoning otherwise. + +- **If fixing**: The fix itself is the response. No reply needed unless the fix is non-obvious. +- **If dismissing**: Reply to the comment thread with a brief explanation of why. Keep it to 1-2 sentences. Examples: + - "This is pre-existing behavior unrelated to this diff — the scope lookup fallback existed before caching was added. Worth a follow-up issue but not blocking this PR." + - "The AsyncExitStack handles cleanup when the session exits, so the subprocess isn't leaked — just kept alive slightly longer than necessary in this edge case." + - "Gemini supports a much wider range of media types than OpenAI/Anthropic, so a restrictive allowlist would be inaccurate here." + +Use `gh api` to reply (note: use `in_reply_to`, not a `/replies` sub-path): + +```bash +# Reply to a specific review comment +gh api repos/PrefectHQ/fastmcp/pulls/{PR_NUMBER}/comments \ + -f body="Your reply here" \ + -F in_reply_to={COMMENT_ID} +``` + +## The fix-push-review cycle + +After evaluating comments: + +1. Fix all real issues in one batch +2. Reply to all dismissed comments with reasoning +3. Think about what patterns Codex might flag next — check similar code paths proactively +4. Commit and push +5. Check that Codex reviews the new commit +6. Repeat until Codex gives a clean review (thumbs-up) or only has dismissible comments + +## Responding to stale 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: +- All Codex comments are either fixed or replied to with dismissal reasoning +- CI checks pass +- The diff is clean and focused on the stated purpose diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 000000000..b7937d760 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,3 @@ +reviews: + path_filters: + - "!docs/python-sdk/**" diff --git a/.cursor/rules/core-mcp-objects.mdc b/.cursor/rules/core-mcp-objects.mdc index c8cc92818..ccbc829ec 100644 --- a/.cursor/rules/core-mcp-objects.mdc +++ b/.cursor/rules/core-mcp-objects.mdc @@ -10,4 +10,4 @@ There are four major MCP object types: - Resource Templates (src/resources/) - Prompts (src/prompts) -While these have slightly different semantics and implementations, in general changes that affect interactions with any one (like adding tags, importing, etc.) will need to be adopted, applied, and tested on all others. Be sure to look at not only the object definition but also the related `Manager` (e.g. `ToolManager`, `ResourceManager`, and `PromptManager`). Also note that while resources and resource templates are different objects, they both are handled by the `ResourceManager`. \ No newline at end of file +While these have slightly different semantics and implementations, in general changes that affect interactions with any one (like adding tags, importing, etc.) will need to be adopted, applied, and tested on all others. Note that while resources and resource templates are different objects, they are both in `src/resources/`. \ No newline at end of file diff --git a/.cursor/worktrees.json b/.cursor/worktrees.json deleted file mode 100644 index 3321da678..000000000 --- a/.cursor/worktrees.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "setup-worktree": [ - "uv sync", - "uv run pre-commit install" - ] -} diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index 3d9a53394..1e6139cfb 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -3,31 +3,30 @@ description: Report a bug or unexpected behavior in FastMCP labels: [bug, pending] body: - - type: markdown - attributes: - value: Thanks for contributing to FastMCP! 🙏 - - type: markdown attributes: value: | + Thanks for reporting a bug! + + A good bug report is one of the most valuable contributions you can make — see [CONTRIBUTING.md](../../CONTRIBUTING.md). If the fix is straightforward, a PR is also welcome. + ### Before you submit - To help us help you, please: - - - 🔄 **Make sure you're testing on the latest version of FastMCP** - many issues are already fixed in newer versions - - 🔍 **Check if someone else has already reported this issue** or if it's been fixed on the main branch - - 📋 **You MUST include a copy/pasteable and properly formatted MRE** (minimal reproducible example) below or your issue may be closed without response - - Thanks for helping to make FastMCP better! 🚀 + - Make sure you're testing on the **latest version** of FastMCP — many issues are already fixed in newer releases + - Check if someone else has **already reported this** or if it's been fixed on the main branch + - You **must** include a copy/pasteable, properly formatted MRE (minimal reproducible example) or your issue may be closed without response + - **The ideal issue is a clear problem description and an MRE — that's it.** If you've done genuine investigation and have a non-obvious insight into the root cause, include it. But please don't speculate or ask an LLM to generate a diagnosis. We have LLMs too, and an incorrect analysis is harder to work with than none at all. + - **Keep it short.** A clear description plus a concise MRE is ideal — aim to fit in a single screen. Issues that include unsolicited root cause analysis, proposed fixes, or multi-section diagnostic writeups will be labeled `too-long` and not triaged until condensed. + - **Using an LLM?** Great — but it must follow these guidelines. Generic LLM output that ignores our contributing conventions will be closed. See [CONTRIBUTING.md](../../CONTRIBUTING.md). - type: textarea id: description attributes: - label: Description + label: What happened? description: | - Please explain what you're experiencing and what you would expect to happen instead. + Describe the bug in a few sentences. What did you do, what happened, and what did you expect instead? - Provide as much detail as possible to help us understand and solve your problem quickly. + Do NOT include root cause analysis, proposed fixes, or diagnostic writeups — just describe the problem. validations: required: true diff --git a/.github/ISSUE_TEMPLATE/enhancement.yml b/.github/ISSUE_TEMPLATE/enhancement.yml index 43b8b4de9..39c66647d 100644 --- a/.github/ISSUE_TEMPLATE/enhancement.yml +++ b/.github/ISSUE_TEMPLATE/enhancement.yml @@ -3,32 +3,27 @@ description: Suggest an idea or improvement for FastMCP labels: [enhancement, pending] body: - - type: markdown - attributes: - value: Thanks for contributing to FastMCP! 🙏 - - type: markdown attributes: value: | + Thanks for suggesting an improvement to FastMCP! + + Enhancement issues are the **primary way** features and improvements get into FastMCP. Maintainers use well-written issues to implement changes that fit the codebase's patterns and ship quickly. A clear issue here is more impactful than a PR — see [CONTRIBUTING.md](../../CONTRIBUTING.md) for why. + ### Before you submit - To help us evaluate your enhancement request: - - - 🔍 **Check if this has already been requested** - search existing issues first - - 💭 **Think about the broader impact** - how would this affect other users? - - 📋 **Consider implementation complexity** - is this a small change or a major feature? - - Thanks for helping to make FastMCP better! 🚀 + - 🔍 **Check if this has already been requested** — search existing issues first + - 🎯 **Describe the problem you're trying to solve**, not the solution you want — we'll figure out the best implementation + - ✂️ **Keep it short.** A motivating description and a concrete use case is the ideal request — aim to fit in a single screen. Skip proposed implementations, API designs, or multi-option analyses — maintainers will figure out the approach. Requests that are difficult to parse will be labeled `too-long` and not triaged until condensed. + - 🤖 **Using an LLM?** Great — but it must follow these guidelines. Generic LLM output that ignores our contributing conventions will be closed. See [CONTRIBUTING.md](../../CONTRIBUTING.md). - type: textarea id: description attributes: label: Enhancement description: | - Please describe the enhancement: + What problem or use case does this solve? How does current behavior fall short? - - What problem or use case would it solve? - - How would it improve your workflow or experience with FastMCP? - - Are there any alternative solutions you've considered? + Focus on the *what* and *why* — the motivating scenario. You don't need to propose an API or implementation. validations: required: true diff --git a/.github/actions/run-claude/action.yml b/.github/actions/run-claude/action.yml new file mode 100644 index 000000000..b79131462 --- /dev/null +++ b/.github/actions/run-claude/action.yml @@ -0,0 +1,100 @@ +# Composite Action for running Claude Code Action +# +# Wraps anthropics/claude-code-action with MCP server configuration. +# Template based on elastic/ai-github-actions base action. +# +# Usage: +# - uses: ./.github/actions/run-claude +# with: +# prompt: "Your prompt here" +# claude-oauth-token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} +# github-token: ${{ steps.marvin-token.outputs.token }} +# allowed-tools: "Edit,Read,Write,Bash(*),mcp__github__add_issue_comment" +# +name: "Run Claude" +description: "Run Claude Code with MCP servers" +author: "FastMCP" + +branding: + icon: "cpu" + color: "orange" + +inputs: + prompt: + description: "Prompt to pass to Claude" + required: true + + claude-oauth-token: + description: "Claude Code OAuth token for authentication" + required: true + + github-token: + description: "GitHub token for Claude to operate with" + required: true + + allowed-tools: + description: "Comma-separated list of allowed tools (e.g. Edit,Write,Bash(npm test))" + 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-8" + + allowed-bots: + description: "Allowed bot usernames, or '*' for all bots" + required: false + default: "" + + track-progress: + description: "Whether Claude should track progress" + required: false + default: "true" + + mcp-servers: + description: "MCP server configuration JSON" + required: false + default: '{"mcpServers":{"agents-md-generator":{"type":"http","url":"https://agents-md-generator.fastmcp.app/mcp"},"public-code-search":{"type":"http","url":"https://public-code-search.fastmcp.app/mcp"}}}' + + trigger-phrase: + description: "Trigger phrase (for mention workflows)" + required: false + default: "/marvin" + +outputs: + conclusion: + description: "The conclusion of the Claude Code run" + value: ${{ steps.claude.outputs.conclusion }} + +runs: + using: "composite" + steps: + - name: Clean up stale Claude locks + shell: bash + run: rm -rf ~/.claude/.locks ~/.local/state/claude/locks || true + + - name: Run Claude Code + id: claude + env: + GITHUB_TOKEN: ${{ inputs.github-token }} + uses: anthropics/claude-code-action@v1 + with: + github_token: ${{ inputs.github-token }} + claude_code_oauth_token: ${{ inputs.claude-oauth-token }} + bot_name: "Marvin Context Protocol" + trigger_phrase: ${{ inputs.trigger-phrase }} + allowed_bots: ${{ inputs.allowed-bots }} + 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.mcp-servers != '' && format('--mcp-config ''{0}''', inputs.mcp-servers) || '' }} + --model ${{ inputs.model }} + settings: | + {"model": "${{ inputs.model }}"} diff --git a/.github/actions/run-pytest/action.yml b/.github/actions/run-pytest/action.yml new file mode 100644 index 000000000..c82e9c0bd --- /dev/null +++ b/.github/actions/run-pytest/action.yml @@ -0,0 +1,66 @@ +name: "Run Pytest" +description: "Run pytest with appropriate flags for the test type and platform" + +inputs: + test-type: + description: "Type of tests to run: unit, integration, client_process, or conformance" + required: false + default: "unit" + +runs: + using: "composite" + steps: + - name: Run pytest + shell: bash + run: | + if [ "${{ inputs.test-type }}" == "integration" ]; then + MARKER="integration" + TIMEOUT="30" + MAX_PROCS="2" + EXTRA_FLAGS="" + elif [ "${{ inputs.test-type }}" == "client_process" ]; then + MARKER="client_process or subprocess_heavy" + TIMEOUT="5" + MAX_PROCS="0" + EXTRA_FLAGS="-x" + elif [ "${{ inputs.test-type }}" == "conformance" ]; then + MARKER="conformance" + TIMEOUT="120" + MAX_PROCS="0" + EXTRA_FLAGS="-x" + else + 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" ]; 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 \ + --durations=50 \ + -m "$MARKER" \ + $PARALLEL_FLAGS \ + $EXTRA_FLAGS \ + tests diff --git a/.github/actions/setup-uv/action.yml b/.github/actions/setup-uv/action.yml new file mode 100644 index 000000000..0becaffad --- /dev/null +++ b/.github/actions/setup-uv/action.yml @@ -0,0 +1,33 @@ +name: "Setup UV Environment" +description: "Install uv and dependencies (requires checkout first)" + +inputs: + python-version: + description: "Python version to use" + required: false + default: "3.10" + resolution: + description: "Dependency resolution: locked, upgrade, or lowest-direct" + required: false + default: "locked" + +runs: + using: "composite" + steps: + - name: Install uv + uses: astral-sh/setup-uv@v7 + with: + enable-cache: true + cache-dependency-glob: "uv.lock" + python-version: ${{ inputs.python-version }} + + - name: Install dependencies + shell: bash + run: | + if [ "${{ inputs.resolution }}" == "locked" ]; then + uv sync --locked + elif [ "${{ inputs.resolution }}" == "upgrade" ]; then + uv sync --upgrade + else + uv sync --resolution ${{ inputs.resolution }} + fi diff --git a/.github/dependabot.yml b/.github/dependabot.yml deleted file mode 100644 index d7be3b7fc..000000000 --- a/.github/dependabot.yml +++ /dev/null @@ -1,20 +0,0 @@ -version: 2 -updates: - - package-ecosystem: "uv" - directory: "/" - schedule: - interval: "daily" - labels: - - "dependencies" - - package-ecosystem: "pip" - directory: "/" - schedule: - interval: "daily" - labels: - - "dependencies" - - package-ecosystem: "github-actions" - directory: "/" - schedule: - interval: "weekly" - labels: - - "dependencies" diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 68212c72f..1b3333782 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,28 +1,22 @@ ## Description - + -**Contributors Checklist** - +## Contribution type -- [ ] My change closes #(issue number) -- [ ] I have followed the repository's development workflow -- [ ] I have tested my changes manually and by adding relevant tests -- [ ] I have performed all required documentation updates + -**Review Checklist** - +- [ ] Bug fix (simple, well-scoped fix for a clearly broken behavior) +- [ ] Documentation improvement +- [ ] Enhancement (maintainers typically implement enhancements — see [CONTRIBUTING.md](../CONTRIBUTING.md)) +## Checklist + +- [ ] This PR addresses an existing issue (or fixes a self-evident bug) +- [ ] I have read [CONTRIBUTING.md](../CONTRIBUTING.md) +- [ ] I have added tests that cover my changes +- [ ] I have run `uv run prek run --all-files` and all checks pass - [ ] I have self-reviewed my changes -- [ ] My Pull Request is ready for review - ---- +- [ ] If I used an LLM, it followed the repo's contributing conventions (not generic output) diff --git a/.github/release.yml b/.github/release.yml index 5ff95aace..5397d75e4 100644 --- a/.github/release.yml +++ b/.github/release.yml @@ -8,12 +8,25 @@ changelog: labels: - feature - - title: Enhancements 🔧 + - title: Breaking Changes ⚠️ + labels: + - breaking change + exclude: + labels: + - contrib + - security + + - title: Enhancements ✨ labels: - enhancement exclude: labels: - breaking change + - security + + - title: Security 🔒 + labels: + - security - title: Fixes 🐞 labels: @@ -21,13 +34,7 @@ changelog: exclude: labels: - contrib - - - title: Breaking Changes 🛫 - labels: - - breaking change - exclude: - labels: - - contrib + - security - title: Docs 📚 labels: @@ -41,6 +48,9 @@ changelog: - title: Dependencies 📦 labels: - dependencies + exclude: + labels: + - security - title: Other Changes 🦾 labels: diff --git a/.github/scripts/mention/gh-get-review-threads.sh b/.github/scripts/mention/gh-get-review-threads.sh new file mode 100755 index 000000000..2e1f4b35d --- /dev/null +++ b/.github/scripts/mention/gh-get-review-threads.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Get PR review threads with comments via GitHub GraphQL API +# +# Usage: +# gh-get-review-threads.sh [FILTER] +# +# Arguments: +# FILTER - Optional: filter for unresolved threads from specific author +# +# Environment (set by composite action): +# MENTION_REPO - Repository (owner/repo format) +# MENTION_PR_NUMBER - Pull request number +# GITHUB_TOKEN - GitHub API token +# +# Output: +# JSON array of review threads with nested comments + +# Parse OWNER and REPO from MENTION_REPO +REPO_FULL="${MENTION_REPO:?MENTION_REPO environment variable is required}" +OWNER="${REPO_FULL%/*}" +REPO="${REPO_FULL#*/}" +PR_NUMBER="${MENTION_PR_NUMBER:?MENTION_PR_NUMBER environment variable is required}" +FILTER="${1:-}" + +gh api graphql -f query=' + query($owner: String!, $repo: String!, $prNumber: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $prNumber) { + reviewThreads(first: 100) { + nodes { + id + isResolved + isOutdated + path + line + comments(first: 50) { + nodes { + id + body + author { login } + createdAt + } + } + } + } + } + } + }' -F owner="$OWNER" \ + -F repo="$REPO" \ + -F prNumber="$PR_NUMBER" \ + --jq '.data.repository.pullRequest.reviewThreads.nodes' | \ +if [ -n "$FILTER" ]; then + jq --arg author "$FILTER" ' + map(select( + .isResolved == false and + .comments.nodes | any(.author.login == $author) + ))' +else + cat +fi diff --git a/.github/scripts/mention/gh-resolve-review-thread.sh b/.github/scripts/mention/gh-resolve-review-thread.sh new file mode 100755 index 000000000..5dc08c239 --- /dev/null +++ b/.github/scripts/mention/gh-resolve-review-thread.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Resolve a GitHub PR review thread, optionally posting a comment first +# +# Usage: +# gh-resolve-review-thread.sh THREAD_ID [COMMENT] +# +# Arguments: +# THREAD_ID - The GraphQL node ID of the review thread to resolve +# COMMENT - Optional: Comment body to post before resolving +# +# Environment (set by composite action): +# MENTION_REPO - Repository (owner/repo format) +# MENTION_PR_NUMBER - Pull request number +# GITHUB_TOKEN - GitHub API token +# +# Behavior: +# 1. If COMMENT is provided, posts it as a reply to the thread +# 2. Resolves the thread + +# Validate required environment variables +: "${MENTION_REPO:?MENTION_REPO environment variable is required}" +: "${MENTION_PR_NUMBER:?MENTION_PR_NUMBER environment variable is required}" +THREAD_ID="${1:?Thread ID required}" +COMMENT="${2:-}" + +# Step 1: Post comment if provided +if [ -n "$COMMENT" ]; then + echo "Posting comment to thread..." >&2 + COMMENT_RESULT=$(gh api graphql -f query=' + mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { + pullRequestReviewThreadId: $threadId, + body: $body + }) { + comment { + id + } + } + }' -f threadId="$THREAD_ID" -f body="$COMMENT") + if echo "$COMMENT_RESULT" | jq -e '.errors' > /dev/null 2>&1; then + echo "Error posting comment: $COMMENT_RESULT" >&2 + exit 1 + fi +fi + +# Step 2: Resolve the thread +echo "Resolving thread..." >&2 +RESOLVE_RESULT=$(gh api graphql -f query=' + mutation($threadId: ID!) { + resolveReviewThread(input: {threadId: $threadId}) { + thread { + id + isResolved + } + } + }' -f threadId="$THREAD_ID" --jq '.data.resolveReviewThread.thread') + +echo "$RESOLVE_RESULT" +echo "✓ Thread resolved" >&2 diff --git a/.github/scripts/pr-review/pr-comment.sh b/.github/scripts/pr-review/pr-comment.sh new file mode 100755 index 000000000..d571f6757 --- /dev/null +++ b/.github/scripts/pr-review/pr-comment.sh @@ -0,0 +1,251 @@ +#!/bin/bash +# pr-comment.sh - Queue a structured inline review comment for the PR review +# +# Usage: +# pr-comment.sh --severity --title --why [suggestion via stdin] +# pr-comment.sh --severity --title --why --no-suggestion +# +# Arguments: +# file File path (required) +# line Line number (required) +# --severity Severity level: critical, high, medium, low, nitpick (required) +# --title Brief description for comment heading (required) +# --why One sentence explaining the risk/impact (required) +# --no-suggestion Explicitly skip suggestion (use for architectural issues) +# +# The suggestion code is read from stdin (use heredoc). If no stdin and no --no-suggestion, errors. +# +# Examples: +# # With suggestion (preferred) +# pr-comment.sh src/main.go 42 --severity high --title "Missing error check" --why "Errors are silently ignored" <<'EOF' +# if err != nil { +# return fmt.Errorf("operation failed: %w", err) +# } +# EOF +# +# # Without suggestion (for issues requiring broader changes) +# pr-comment.sh src/main.go 42 --severity medium --title "Consider extracting to function" \ +# --why "This logic is duplicated in 3 places" --no-suggestion +# +# Environment variables (set by the composite action): +# PR_REVIEW_REPO - Repository (owner/repo) +# PR_REVIEW_PR_NUMBER - Pull request number +# PR_REVIEW_COMMENTS_DIR - Directory to cache comments (default: /tmp/pr-review-comments) + +set -e + +# Configuration from environment +REPO="${PR_REVIEW_REPO:?PR_REVIEW_REPO environment variable is required}" +PR_NUMBER="${PR_REVIEW_PR_NUMBER:?PR_REVIEW_PR_NUMBER environment variable is required}" +COMMENTS_DIR="${PR_REVIEW_COMMENTS_DIR:-/tmp/pr-review-comments}" + +# Severity emoji mapping +declare -A SEVERITY_EMOJI=( + [critical]="🔴 CRITICAL" + [high]="🟠 HIGH" + [medium]="🟡 MEDIUM" + [low]="⚪ LOW" + [nitpick]="💬 NITPICK" +) + +# Parse arguments +FILE="" +LINE="" +SEVERITY="" +TITLE="" +WHY="" +NO_SUGGESTION=false + +# First two positional args are file and line +if [ $# -lt 2 ]; then + echo "Error: file and line are required" + echo "Usage: pr-comment.sh --severity --title --why [<<'EOF' ... EOF]" + exit 1 +fi + +FILE="$1" +LINE="$2" +shift 2 + +# Parse named arguments +while [ $# -gt 0 ]; do + case "$1" in + --severity) + SEVERITY="$2" + shift 2 + ;; + --title) + TITLE="$2" + shift 2 + ;; + --why) + WHY="$2" + shift 2 + ;; + --no-suggestion) + NO_SUGGESTION=true + shift + ;; + *) + echo "Error: Unknown argument: $1" + exit 1 + ;; + esac +done + +# Read suggestion from stdin if available +SUGGESTION="" +if [ ! -t 0 ]; then + SUGGESTION=$(cat) +fi + +# Validate required arguments +if [ -z "$SEVERITY" ]; then + echo "Error: --severity is required (critical, high, medium, low, nitpick)" + exit 1 +fi + +if [ -z "$TITLE" ]; then + echo "Error: --title is required" + exit 1 +fi + +if [ -z "$WHY" ]; then + echo "Error: --why is required" + exit 1 +fi + +# Validate severity level +if [ -z "${SEVERITY_EMOJI[$SEVERITY]}" ]; then + echo "Error: Invalid severity '$SEVERITY'. Must be one of: critical, high, medium, low, nitpick" + exit 1 +fi + +# Require either suggestion or explicit --no-suggestion +if [ -z "$SUGGESTION" ] && [ "$NO_SUGGESTION" = false ]; then + echo "Error: Suggestion required. Provide code via stdin (heredoc) or use --no-suggestion" + echo "" + echo "Example with suggestion:" + echo " pr-comment.sh file.go 42 --severity high --title \"desc\" --why \"reason\" <<'EOF'" + echo " fixed code here" + echo " EOF" + echo "" + echo "Example without suggestion:" + echo " pr-comment.sh file.go 42 --severity medium --title \"desc\" --why \"reason\" --no-suggestion" + exit 1 +fi + +# Validate line is a positive integer (>= 1) +if ! [[ "$LINE" =~ ^[1-9][0-9]*$ ]]; then + echo "Error: Line number must be a positive integer (>= 1), got: $LINE" + exit 1 +fi + +# Get the diff for this file to validate the comment location +DIFF_DATA=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate | jq --arg f "$FILE" '.[] | select(.filename==$f)') + +if [ -z "$DIFF_DATA" ]; then + echo "Error: File '${FILE}' not found in PR diff" + echo "" + echo "Files changed in this PR:" + gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' + exit 1 +fi + +PATCH=$(echo "$DIFF_DATA" | jq -r '.patch // empty') + +if [ -z "$PATCH" ]; then + echo "Error: No patch data for file '${FILE}' (file may be binary or too large)" + exit 1 +fi + +# Verify the line exists in the diff +LINE_IN_DIFF=$(echo "$PATCH" | awk -v target_line="$LINE" ' +BEGIN { current_line = 0; found = 0 } +/^@@/ { + line = $0 + gsub(/.*\+/, "", line) + gsub(/[^0-9].*/, "", line) + current_line = line - 1 + next +} +{ + if (substr($0, 1, 1) != "-") { + current_line++ + if (current_line == target_line) { + found = 1 + exit + } + } +} +END { if (found) print "1"; else print "0" } +') + +if [ "$LINE_IN_DIFF" != "1" ]; then + echo "Error: Line ${LINE} not found in the diff for '${FILE}'" + echo "" + echo "Note: You can only comment on lines that appear in the diff (added, modified, or context lines)" + echo "" + echo "First 50 lines of diff for this file:" + echo "$PATCH" | head -50 + exit 1 +fi + +# Create comments directory if it doesn't exist +mkdir -p "${COMMENTS_DIR}" + +# Assemble the comment body +SEVERITY_LABEL="${SEVERITY_EMOJI[$SEVERITY]}" + +BODY="**${SEVERITY_LABEL}** ${TITLE} + +Why: ${WHY}" + +# Add suggestion block if provided +if [ -n "$SUGGESTION" ]; then + BODY="${BODY} + +\`\`\`suggestion +${SUGGESTION} +\`\`\`" +fi + +# Append standard footer +FOOTER=' + +--- +Marvin Context Protocol | Type `/marvin` to interact further + +Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.' + +BODY_WITH_FOOTER="${BODY}${FOOTER}" + +# Generate unique comment ID +COMMENT_ID="comment-$(date +%s)-$(od -An -N4 -tu4 /dev/urandom | tr -d ' ')" +COMMENT_FILE="${COMMENTS_DIR}/${COMMENT_ID}.json" + +# Create the comment JSON object +jq -n \ + --arg path "$FILE" \ + --argjson line "$LINE" \ + --arg side "RIGHT" \ + --arg body "$BODY_WITH_FOOTER" \ + --arg id "$COMMENT_ID" \ + '{ + path: $path, + line: $line, + side: $side, + body: $body, + _meta: { + id: $id, + file: $path, + line: $line + } + }' > "${COMMENT_FILE}" + +echo "✓ Queued review comment for ${FILE}:${LINE}" +echo " Severity: ${SEVERITY_LABEL}" +echo " Title: ${TITLE}" +echo " Comment ID: ${COMMENT_ID}" +echo " Comment will be submitted with pr-review.sh" +echo " Remove with: pr-remove-comment.sh ${FILE} ${LINE}" diff --git a/.github/scripts/pr-review/pr-diff.sh b/.github/scripts/pr-review/pr-diff.sh new file mode 100755 index 000000000..4448e0012 --- /dev/null +++ b/.github/scripts/pr-review/pr-diff.sh @@ -0,0 +1,128 @@ +#!/bin/bash +# pr-diff.sh - Show changed files or diff for a specific file +# +# Usage: +# pr-diff.sh - List all changed files (shows full diff if small enough) +# pr-diff.sh - Show diff for a specific file with line numbers +# +# Environment variables (set by the composite action): +# PR_REVIEW_REPO - Repository (owner/repo) +# PR_REVIEW_PR_NUMBER - Pull request number + +set -e + +# Configuration from environment +REPO="${PR_REVIEW_REPO:?PR_REVIEW_REPO environment variable is required}" +PR_NUMBER="${PR_REVIEW_PR_NUMBER:?PR_REVIEW_PR_NUMBER environment variable is required}" +EXPECTED_HEAD="${PR_REVIEW_HEAD_SHA:-}" + +# Check if HEAD has changed since review started (race condition detection) +if [ -n "$EXPECTED_HEAD" ]; then + CURRENT_HEAD=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha') + if [ "$CURRENT_HEAD" != "$EXPECTED_HEAD" ]; then + echo "⚠️ WARNING: PR head has changed since review started!" + echo " Review started at: ${EXPECTED_HEAD:0:7}" + echo " Current head: ${CURRENT_HEAD:0:7}" + echo " Line numbers below may not match the commit being reviewed." + echo "" + fi +fi + +# Thresholds for "too big" - show file list only if exceeded +MAX_FILES=25 +MAX_TOTAL_LINES=1500 + +FILE="$1" + +# Function to add line numbers to a patch +# Format: [LINE] +added | [LINE] context | [----] -deleted +add_line_numbers() { + awk ' + BEGIN { new_line = 0 } + /^@@/ { + # Parse hunk header: @@ -old_start,old_count +new_start,new_count @@ + match($0, /\+([0-9]+)/) + new_line = substr($0, RSTART+1, RLENGTH-1) - 1 + print "" + print $0 + next + } + /^-/ { + # Deleted line - cannot comment on these + printf "[----] %s\n", $0 + next + } + /^\+/ { + # Added line - can comment, show line number + new_line++ + printf "[%4d] %s\n", new_line, $0 + next + } + { + # Context line (space prefix) - can comment, show line number + new_line++ + printf "[%4d] %s\n", new_line, $0 + } + ' +} + +if [ -z "$FILE" ]; then + # Get file list with stats + FILES_DATA=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate) + + FILE_COUNT=$(echo "$FILES_DATA" | jq 'length') + TOTAL_ADDITIONS=$(echo "$FILES_DATA" | jq '[.[].additions] | add // 0') + TOTAL_DELETIONS=$(echo "$FILES_DATA" | jq '[.[].deletions] | add // 0') + TOTAL_LINES=$((TOTAL_ADDITIONS + TOTAL_DELETIONS)) + + echo "PR #${PR_NUMBER} Summary: ${FILE_COUNT} files changed (+${TOTAL_ADDITIONS}/-${TOTAL_DELETIONS})" + echo "" + + # Check if diff is too large + if [ "$FILE_COUNT" -gt "$MAX_FILES" ] || [ "$TOTAL_LINES" -gt "$MAX_TOTAL_LINES" ]; then + echo "⚠️ Large diff detected (>${MAX_FILES} files or >${MAX_TOTAL_LINES} lines changed)" + echo " Review files individually using: pr-diff.sh " + echo "" + echo "Files changed:" + echo "$FILES_DATA" | jq -r '.[] | " \(.filename) (+\(.additions)/-\(.deletions))"' + else + # Small enough - show all diffs with line numbers + echo "Files changed:" + echo "$FILES_DATA" | jq -r '.[] | " \(.filename) (+\(.additions)/-\(.deletions))"' + echo "" + echo "─────────────────────────────────────────────────────────────────────" + echo "" + + # Show each file's diff by iterating over indices + for i in $(seq 0 $((FILE_COUNT - 1))); do + FNAME=$(echo "$FILES_DATA" | jq -r ".[$i].filename") + PATCH=$(echo "$FILES_DATA" | jq -r ".[$i].patch // empty") + + if [ -n "$PATCH" ]; then + echo "## ${FNAME}" + echo "Use: pr-comment.sh ${FNAME} --severity --title \"desc\" --why \"reason\" <<'EOF' ... EOF" + echo "Format: [LINE] +added | [LINE] context | [----] -deleted (can't comment)" + echo "$PATCH" | add_line_numbers + echo "" + echo "─────────────────────────────────────────────────────────────────────" + echo "" + fi + done + fi +else + # Show specific file diff + PATCH=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq --arg file "$FILE" '.[] | select(.filename==$file) | .patch') + + if [ -z "$PATCH" ]; then + echo "Error: File '${FILE}' not found in PR diff" + echo "" + echo "Files changed in this PR:" + gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' + exit 1 + fi + + echo "## ${FILE}" + echo "Use: pr-comment.sh ${FILE} --severity --title \"desc\" --why \"reason\" <<'EOF' ... EOF" + echo "Format: [LINE] +added | [LINE] context | [----] -deleted (can't comment)" + echo "$PATCH" | add_line_numbers +fi diff --git a/.github/scripts/pr-review/pr-existing-comments.sh b/.github/scripts/pr-review/pr-existing-comments.sh new file mode 100755 index 000000000..10fa05f16 --- /dev/null +++ b/.github/scripts/pr-review/pr-existing-comments.sh @@ -0,0 +1,190 @@ +#!/bin/bash +# pr-existing-comments.sh - Fetch existing review threads on a PR +# +# Usage: +# pr-existing-comments.sh - Show all review threads with full details +# pr-existing-comments.sh --summary - Show per-file summary only (for large PRs) +# pr-existing-comments.sh --unresolved - Show only unresolved threads +# pr-existing-comments.sh --file - Show threads for a specific file +# pr-existing-comments.sh --full - Show full comment text (no truncation) +# +# Output: Formatted summary of existing review threads grouped by file, +# showing thread status, comments, and whether issues were addressed. +# +# For large PRs, use --summary first to see the overview, then --file +# to get full thread details when reviewing each file. +# +# Environment variables (set by the composite action): +# PR_REVIEW_REPO - Repository (owner/repo) +# PR_REVIEW_PR_NUMBER - Pull request number + +set -e + +# Configuration from environment +REPO="${PR_REVIEW_REPO:?PR_REVIEW_REPO environment variable is required}" +PR_NUMBER="${PR_REVIEW_PR_NUMBER:?PR_REVIEW_PR_NUMBER environment variable is required}" + +OWNER="${REPO%/*}" +REPO_NAME="${REPO#*/}" + +# Parse arguments +FILTER_UNRESOLVED=false +FILTER_FILE="" +SUMMARY_ONLY=false +FULL_TEXT=false + +while [ $# -gt 0 ]; do + case "$1" in + --unresolved) + FILTER_UNRESOLVED=true + shift + ;; + --file) + FILTER_FILE="$2" + shift 2 + ;; + --summary) + SUMMARY_ONLY=true + shift + ;; + --full) + FULL_TEXT=true + shift + ;; + *) + echo "Usage: pr-existing-comments.sh [--summary] [--unresolved] [--file ] [--full]" + exit 1 + ;; + esac +done + +# Fetch review threads via GraphQL +THREADS=$(gh api graphql -f query=' + query($owner: String!, $repo: String!, $prNumber: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $prNumber) { + reviewThreads(first: 100) { + nodes { + id + isResolved + isOutdated + path + line + originalLine + startLine + originalStartLine + diffSide + comments(first: 50) { + nodes { + id + body + author { login } + createdAt + originalCommit { abbreviatedOid } + } + } + } + } + } + } + }' -F owner="$OWNER" \ + -F repo="$REPO_NAME" \ + -F prNumber="$PR_NUMBER" \ + --jq '.data.repository.pullRequest.reviewThreads.nodes') + +if [ -z "$THREADS" ] || [ "$THREADS" = "null" ]; then + echo "No existing review threads found." + exit 0 +fi + +# Apply filters +FILTERED="$THREADS" + +if [ "$FILTER_UNRESOLVED" = true ]; then + FILTERED=$(echo "$FILTERED" | jq '[.[] | select(.isResolved == false)]') +fi + +if [ -n "$FILTER_FILE" ]; then + FILTERED=$(echo "$FILTERED" | jq --arg file "$FILTER_FILE" '[.[] | select(.path == $file)]') +fi + +THREAD_COUNT=$(echo "$FILTERED" | jq 'length') + +if [ "$THREAD_COUNT" -eq 0 ]; then + if [ "$FILTER_UNRESOLVED" = true ]; then + echo "No unresolved review threads found." + elif [ -n "$FILTER_FILE" ]; then + echo "No review threads found for ${FILTER_FILE}." + else + echo "No existing review threads found." + fi + exit 0 +fi + +# Count resolved vs unresolved +RESOLVED_COUNT=$(echo "$FILTERED" | jq '[.[] | select(.isResolved == true)] | length') +UNRESOLVED_COUNT=$(echo "$FILTERED" | jq '[.[] | select(.isResolved == false)] | length') +OUTDATED_COUNT=$(echo "$FILTERED" | jq '[.[] | select(.isOutdated == true)] | length') + +echo "Existing review threads: ${THREAD_COUNT} total (${UNRESOLVED_COUNT} unresolved, ${RESOLVED_COUNT} resolved, ${OUTDATED_COUNT} outdated)" +echo "" + +# Summary mode: show per-file counts only +if [ "$SUMMARY_ONLY" = true ]; then + echo "Threads by file:" + echo "$FILTERED" | jq -r ' + group_by(.path) | .[] | + . as $threads | + ($threads | length) as $total | + ([$threads[] | select(.isResolved == false)] | length) as $unresolved | + ([$threads[] | select(.isResolved == true)] | length) as $resolved | + ([$threads[] | select(.isOutdated == true)] | length) as $outdated | + ([$threads[] | select(.comments.nodes | length > 1)] | length) as $has_replies | + " " + $threads[0].path + + " — " + ($total | tostring) + " threads" + + " (" + ($unresolved | tostring) + " unresolved, " + ($resolved | tostring) + " resolved" + + (if $outdated > 0 then ", " + ($outdated | tostring) + " outdated" else "" end) + + ")" + + (if $has_replies > 0 then " ⚠️ " + ($has_replies | tostring) + " with replies" else "" end) + ' + echo "" + echo "Use: pr-existing-comments.sh --file to see full thread details for a file" + exit 0 +fi + +# Full detail mode: output threads grouped by file +# Show full conversation for threads with replies +FIRST_LIMIT=200 +REPLY_LIMIT=300 +if [ "$FULL_TEXT" = true ]; then + FIRST_LIMIT=999999 + REPLY_LIMIT=999999 +fi + +echo "$FILTERED" | jq -r --argjson first_limit "$FIRST_LIMIT" --argjson reply_limit "$REPLY_LIMIT" ' + group_by(.path) | .[] | + "## " + .[0].path + " (" + (length | tostring) + " threads)\n" + + ([.[] | + " " + + (if .isResolved then "✅ RESOLVED" elif .isOutdated then "⚠️ OUTDATED" else "🔴 UNRESOLVED" end) + + " (line " + (if .line then (.line | tostring) elif .startLine then (.startLine | tostring) elif .originalLine then ("~" + (.originalLine | tostring)) elif .originalStartLine then ("~" + (.originalStartLine | tostring)) else "?" end) + ")" + + # Show the commit the comment was originally made on + (if .comments.nodes[0].originalCommit.abbreviatedOid then " [" + .comments.nodes[0].originalCommit.abbreviatedOid + "]" else "" end) + + # Flag threads with replies — indicates a conversation happened + (if (.comments.nodes | length) > 1 then " ← has replies" else "" end) + + "\n" + + ([.comments.nodes | to_entries[] | + .value as $comment | + .key as $idx | + ($comment.body | gsub("\n"; " ")) as $flat | + if $idx == 0 then + " @" + ($comment.author.login // "unknown") + ": " + $flat[0:$first_limit] + + (if ($flat | length) > $first_limit then " [truncated]" else "" end) + else + " ↳ @" + ($comment.author.login // "unknown") + ": " + $flat[0:$reply_limit] + + (if ($flat | length) > $reply_limit then " [truncated]" else "" end) + end + ] | join("\n")) + + "\n" + ] | join("\n")) +' diff --git a/.github/scripts/pr-review/pr-remove-comment.sh b/.github/scripts/pr-review/pr-remove-comment.sh new file mode 100755 index 000000000..04b73fbf6 --- /dev/null +++ b/.github/scripts/pr-review/pr-remove-comment.sh @@ -0,0 +1,84 @@ +#!/bin/bash +# pr-remove-comment.sh - Remove a queued review comment +# +# Usage: +# pr-remove-comment.sh +# pr-remove-comment.sh +# +# Examples: +# pr-remove-comment.sh src/main.go 42 +# pr-remove-comment.sh comment-1234567890-1234567890 +# +# This script removes a previously queued comment before it's submitted. +# Useful if the agent realizes it made a mistake or wants to update a comment. +# +# Environment variables (set by the composite action): +# PR_REVIEW_COMMENTS_DIR - Directory containing comment files (default: /tmp/pr-review-comments) + +set -e + +COMMENTS_DIR="${PR_REVIEW_COMMENTS_DIR:-/tmp/pr-review-comments}" + +if [ ! -d "${COMMENTS_DIR}" ]; then + echo "No comments directory found: ${COMMENTS_DIR}" + exit 0 +fi + +# Check if first argument looks like a comment ID +if [[ "$1" =~ ^comment- ]]; then + COMMENT_ID="$1" + COMMENT_FILE="${COMMENTS_DIR}/${COMMENT_ID}.json" + + if [ -f "${COMMENT_FILE}" ]; then + FILE=$(jq -r '._meta.file // .path' "${COMMENT_FILE}") + LINE=$(jq -r '._meta.line // .line' "${COMMENT_FILE}") + rm -f "${COMMENT_FILE}" + echo "✓ Removed comment ${COMMENT_ID} for ${FILE}:${LINE}" + else + echo "Comment not found: ${COMMENT_ID}" + exit 1 + fi +else + # Treat as file and line number + FILE="$1" + LINE="$2" + + if [ -z "$FILE" ] || [ -z "$LINE" ]; then + echo "Usage:" + echo " pr-remove-comment.sh " + echo " pr-remove-comment.sh " + echo "" + echo "Examples:" + echo " pr-remove-comment.sh src/main.go 42" + echo " pr-remove-comment.sh comment-1234567890-1234567890" + exit 1 + fi + + # Validate line is a positive integer (>= 1) + if ! [[ "$LINE" =~ ^[1-9][0-9]*$ ]]; then + echo "Error: Line number must be a positive integer (>= 1), got: $LINE" + exit 1 + fi + + # Find and remove matching comment files + # Use nullglob to handle case where no files match + shopt -s nullglob + REMOVED=0 + for COMMENT_FILE in "${COMMENTS_DIR}"/comment-*.json; do + + COMMENT_FILE_PATH=$(jq -r '._meta.file // .path' "${COMMENT_FILE}") + COMMENT_LINE=$(jq -r '._meta.line // .line' "${COMMENT_FILE}") + + if [ "$COMMENT_FILE_PATH" = "$FILE" ] && [ "$COMMENT_LINE" = "$LINE" ]; then + COMMENT_ID=$(basename "${COMMENT_FILE}" .json) + rm -f "${COMMENT_FILE}" + echo "✓ Removed comment ${COMMENT_ID} for ${FILE}:${LINE}" + REMOVED=$((REMOVED + 1)) + fi + done + + if [ "$REMOVED" -eq 0 ]; then + echo "No comment found for ${FILE}:${LINE}" + exit 1 + fi +fi diff --git a/.github/scripts/pr-review/pr-review.sh b/.github/scripts/pr-review/pr-review.sh new file mode 100755 index 000000000..48c0b6888 --- /dev/null +++ b/.github/scripts/pr-review/pr-review.sh @@ -0,0 +1,143 @@ +#!/bin/bash +# pr-review.sh - Submit a PR review (approve, request changes, or comment) +# +# Usage: pr-review.sh [review-body] +# Example: pr-review.sh REQUEST_CHANGES "Please fix the issues noted above" +# +# This script creates and submits a review with any queued inline comments. +# Comments are read from individual files in PR_REVIEW_COMMENTS_DIR (created by pr-comment.sh). +# +# The review body can contain special characters (backticks, dollar signs, etc.) +# and will be safely passed to the GitHub API without shell interpretation. +# +# Environment variables (set by the composite action): +# PR_REVIEW_REPO - Repository (owner/repo) +# PR_REVIEW_PR_NUMBER - Pull request number +# PR_REVIEW_HEAD_SHA - HEAD commit SHA +# PR_REVIEW_COMMENTS_DIR - Directory containing queued comment files (default: /tmp/pr-review-comments) + +set -e + +# Configuration from environment +REPO="${PR_REVIEW_REPO:?PR_REVIEW_REPO environment variable is required}" +PR_NUMBER="${PR_REVIEW_PR_NUMBER:?PR_REVIEW_PR_NUMBER environment variable is required}" +HEAD_SHA="${PR_REVIEW_HEAD_SHA:?PR_REVIEW_HEAD_SHA environment variable is required}" +COMMENTS_DIR="${PR_REVIEW_COMMENTS_DIR:-/tmp/pr-review-comments}" + +# Arguments +EVENT="$1" +shift 2>/dev/null || true + +# Read body from remaining arguments +# Join all remaining arguments with spaces, preserving the string as-is +BODY="$*" + +if [ -z "$EVENT" ]; then + echo "Usage: pr-review.sh [review-body]" + echo "Example: pr-review.sh REQUEST_CHANGES 'Please fix the issues noted in the inline comments'" + exit 1 +fi + +# Validate event type +case "$EVENT" in + APPROVE|REQUEST_CHANGES|COMMENT) + ;; + *) + echo "Error: Invalid event type '${EVENT}'" + echo "Must be one of: APPROVE, REQUEST_CHANGES, COMMENT" + exit 1 + ;; +esac + +# Read queued comments from individual files +COMMENTS="[]" +COMMENT_COUNT=0 + +if [ -d "${COMMENTS_DIR}" ]; then + # Collect all comment files and merge into a single JSON array + # Remove _meta fields before submitting (they're only for internal use) + COMMENT_FILES=("${COMMENTS_DIR}"/comment-*.json) + + if [ -f "${COMMENT_FILES[0]}" ]; then + # Use jq to read all comment files, extract the comment data (without _meta), and combine + COMMENTS=$(jq -s '[.[] | del(._meta)]' "${COMMENTS_DIR}"/comment-*.json) + COMMENT_COUNT=$(echo "$COMMENTS" | jq 'length') + if [ "$COMMENT_COUNT" -gt 0 ]; then + echo "Found ${COMMENT_COUNT} queued inline comment(s)" + fi + fi +fi + +# Append standard footer to the review body (if body is provided) +FOOTER=' + +--- +Marvin Context Protocol | Type `/marvin` to interact further + +Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.' + +if [ -n "$BODY" ]; then + BODY_WITH_FOOTER="${BODY}${FOOTER}" +else + BODY_WITH_FOOTER="" +fi + +# Build the review request JSON +# Use jq to safely construct the JSON with all special characters handled +REVIEW_JSON=$(jq -n \ + --arg commit_id "$HEAD_SHA" \ + --arg event "$EVENT" \ + --arg body "$BODY_WITH_FOOTER" \ + --argjson comments "$COMMENTS" \ + '{ + commit_id: $commit_id, + event: $event, + comments: $comments + } + (if $body != "" then {body: $body} else {} end)') + +# Check if HEAD has changed since review started (race condition detection) +CURRENT_HEAD=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha') +if [ "$CURRENT_HEAD" != "$HEAD_SHA" ]; then + echo "⚠️ WARNING: PR head has changed since review started!" + echo " Review started at: ${HEAD_SHA:0:7}" + echo " Current head: ${CURRENT_HEAD:0:7}" + echo "" + echo " New commits may have shifted line numbers. Review will be submitted" + echo " against the original commit (${HEAD_SHA:0:7}) but comments may be outdated." + echo "" +fi + +echo "Submitting ${EVENT} review for commit ${HEAD_SHA:0:7}..." + +# Create and submit the review in one API call +# Use a temp file to safely pass the JSON body +TEMP_JSON=$(mktemp) +trap "rm -f ${TEMP_JSON}" EXIT +echo "$REVIEW_JSON" > "${TEMP_JSON}" + +RESPONSE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" \ + -X POST \ + --input "${TEMP_JSON}" 2>&1) || { + echo "Error submitting review:" + echo "$RESPONSE" + exit 1 +} + +# Clean up the comments directory after successful submission +if [ -d "${COMMENTS_DIR}" ] && [ "$COMMENT_COUNT" -gt 0 ]; then + rm -f "${COMMENTS_DIR}"/comment-*.json + # Remove directory if empty + rmdir "${COMMENTS_DIR}" 2>/dev/null || true +fi + +REVIEW_URL=$(echo "$RESPONSE" | jq -r '.html_url // empty') +REVIEW_STATE=$(echo "$RESPONSE" | jq -r '.state // empty') + +if [ -n "$REVIEW_URL" ]; then + echo "✓ Review submitted (${REVIEW_STATE}): ${REVIEW_URL}" + if [ "$COMMENT_COUNT" -gt 0 ]; then + echo " Included ${COMMENT_COUNT} inline comment(s)" + fi +else + echo "✓ Review submitted successfully" +fi 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