Skip to content

Updates to github actions / workflows for claude#3157

Merged
jlowin merged 4 commits intomainfrom
update-claude-workflows
Feb 12, 2026
Merged

Updates to github actions / workflows for claude#3157
jlowin merged 4 commits intomainfrom
update-claude-workflows

Conversation

@strawgate
Copy link
Copy Markdown
Collaborator

@strawgate strawgate commented Feb 11, 2026

Adds dedicated workflows for triage, trigger from issue, and trigger from PR. let's PR trigger see and resolve open PR review comments.

@marvin-context-protocol marvin-context-protocol Bot added enhancement Improvement to existing functionality. For issues and smaller PR improvements. tests labels Feb 11, 2026
Resolve conflict: remove old martian-issue-triage.yml (replaced by martian-triage-issue.yml)

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3b630ee7b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +57 to +58
.isResolved == false and
.comments.nodes | any(.author.login == $author)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Parenthesize the jq author filter before piping to any

When gh-get-review-threads.sh is called with a filter argument (as documented in the PR review guidance), this jq expression fails because | binds after and, so it effectively pipes a boolean into any(...) and exits with Cannot iterate over boolean. Since the script runs with set -euo pipefail, reviewer-specific thread filtering currently aborts instead of returning matching unresolved threads.

Useful? React with 👍 / 👎.

Comment on lines +6 to +10
on:
pull_request_review_comment:
types: [created]
pull_request:
types: [opened, edited]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Trigger the PR workflow for regular PR conversation comments

This workflow says it handles “PR comments or PR body,” but it only subscribes to pull_request_review_comment and pull_request; regular top-level comments on PRs come through the issue_comment event, so /marvin mentions there never enter this PR-specific flow. In this commit, those mentions are instead picked up by the issue workflow, which does not set up the PR review helper environment and checks out the default branch context, so common PR-comment requests miss the intended review behavior.

Useful? React with 👍 / 👎.

@marvin-context-protocol
Copy link
Copy Markdown
Contributor

marvin-context-protocol Bot commented Feb 11, 2026

Test Failure Analysis

Summary: The test_uv_transport test is timing out on Python 3.13.12, but passes on Python 3.10. This is caused by a known asyncio subprocess race condition bug in Python 3.13.

Root Cause: Python 3.13.12 has an open asyncio subprocess race condition bug (cpython#127049) that affects subprocess cleanup. The test hangs during teardown when waiting for the uv subprocess to exit, specifically in os.waitpid() which is stuck in the asyncio-waitpid thread.

The bug affects:

  • Python 3.13.x (including 3.13.12 used in CI)
  • Python 3.14.x alpha
  • Does NOT affect Python 3.10 (test passes there)

Suggested Solution: Skip this test on Python 3.13 until the upstream bug is fixed.

# In tests/client/transports/test_uv_transport.py
@pytest.mark.timeout(10)
@pytest.mark.client_process
@pytest.mark.skipif(
    sys.platform == "win32",
    reason="Windows file locking issues with uv client process cleanup",
)
@pytest.mark.skipif(
    sys.version_info >= (3, 13) and sys.version_info < (3, 14),
    reason="Python 3.13 has asyncio subprocess race condition bug (cpython#127049)",
)
async def test_uv_transport():
    # ... test code

Apply the same skip to test_uv_transport_module on line 63.

Detailed Analysis

Test Behavior

  • Python 3.10: Test passes in 9.41s ✅
  • Python 3.13.12: Test times out after 10s ❌

What Happens

From the logs:

  1. UV subprocess starts successfully
  2. FastMCP server initializes
  3. Client connects and calls the add tool successfully
  4. Test enters teardown phase
  5. Hangs indefinitely in os.waitpid() waiting for the subprocess to be reaped

Stack Trace

File "/usr/lib/python3.13/asyncio/unix_events.py", line 1443, in _do_waitpid
    pid, status = os.waitpid(expected_pid, 0)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Failed: Timeout (>10.0s) from pytest-timeout.

The Bug

Python 3.13 introduced a fix for issue #87744 (racing waitpid calls), but this fix created a new race condition (issue #127049) where:

  • Process.send_signal() may call kill() on a PID that's already been freed
  • The PID lifecycle management is broken in asyncio
  • Subprocess cleanup can hang indefinitely

A draft fix exists (PR #127051) but hasn't been merged yet.

Why This Test Is Affected

The UvStdioTransport test:

  1. Spawns a UV subprocess that creates a virtual environment and runs FastMCP
  2. Uses keep_alive=False to trigger cleanup on context exit
  3. Explicitly calls await client.transport.close() which triggers subprocess termination
  4. The asyncio subprocess cleanup code hits the race condition
Related Files
  • tests/client/transports/test_uv_transport.py:26 - test_uv_transport function
  • tests/client/transports/test_uv_transport.py:63 - test_uv_transport_module function
  • src/fastmcp/client/transports/stdio.py:149 - _stdio_transport_connect_task (handles subprocess lifecycle)
  • src/fastmcp/client/transports/stdio.py:340 - UvStdioTransport class
References

🤖 Generated by Marvin - your friendly test failure analyzer

✏️ Updated: 2026-02-11 22:17:36 UTC

@strawgate
Copy link
Copy Markdown
Collaborator Author

/marvin are you running the latest workflow from this PR? can you test your tools etc and make sure everything looks good?

@marvin-context-protocol
Copy link
Copy Markdown
Contributor

marvin-context-protocol Bot commented Feb 11, 2026

Claude finished @strawgate's task in 1m 31s —— View job


Workflow Verification and Tool Testing Complete ✓

Key Findings

🔴 Critical Issue: Running OLD Workflow
I'm currently running from the OLD marvin.yml workflow (from main branch), not your new workflows. This is because:

  • GitHub Actions workflows execute from the base branch (main), not the PR branch
  • Your new workflows (marvin-comment-on-pr.yml, marvin-comment-on-issue.yml) are only on the update-claude-workflows branch
  • To test the new workflows, you need to merge this PR first

What I Tested

✓ Successfully Working:

  • GitHub MCP tools (CI status, workflow details, comment updates)
  • File system operations (Read, Write, Edit, Glob, Grep)
  • Bash commands and gh CLI (v2.86.0)
  • PR review helper scripts exist on branch (pr-diff.sh, pr-comment.sh, etc.)

✗ Not Available (Expected):

  • mcp__agents-md-generator__generate_agents_md - This MCP server tool is configured in the NEW workflow but not in the current OLD workflow
  • PR review environment variables (PR_REVIEW_REPO, PR_REVIEW_HELPERS_DIR, etc.) - These are set in the NEW workflow, not the OLD one

⚠️ Test Failure Detected:
Your PR has a test failure in "Tests: Python 3.13 on ubuntu-latest" - the "Run client process tests" step failed (job 63314926275).

Configuration Comparison

OLD Workflow (currently running):

  • No PR_REVIEW environment setup
  • No mention scripts environment
  • Different allowed-tools list
  • No MCP servers configured

NEW Workflow (in this PR):

  • Sets PR_REVIEW_* environment variables
  • Sets MENTION_SCRIPTS environment
  • Configures agents-md-generator and public-code-search MCP servers
  • Expanded allowed-tools list including PR review capabilities

Recommendation

To properly test the new workflows:

  1. Merge this PR (after fixing any issues)
  2. Create a test PR and comment /marvin there
  3. The new workflow will run from main branch and have all the new features

Alternatively, you could:

  • Temporarily push the new workflows to main to test them before merging
  • But this isn't recommended as it bypasses code review

--- | PR branch

Copy link
Copy Markdown
Member

@jlowin jlowin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid restructuring — the issue/PR workflow split is clean, the composite action centralizes things well, and the PR review script toolkit is nicely designed.

A few minor things for follow-up:

  • extra-allowed-tools is referenced in the composite action but never declared as an input (dead code)
  • Neither comment workflow has concurrency controls (triage does) — parallel /marvin invocations could collide
  • Both comment workflows declare contents: write but say they can't push — contents: read may suffice

Also noting the scope reduction from the old monolithic workflow: inline review comments (pull_request_review_comment), PR body mentions, discussion comments, and label-triggered triage no longer fire. Assuming that's intentional with the redesign.

@jlowin jlowin merged commit 88c8924 into main Feb 12, 2026
15 of 16 checks passed
@jlowin jlowin deleted the update-claude-workflows branch February 12, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality. For issues and smaller PR improvements. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants