Skip to content

fix(ci): gate extract-claude-lessons on head branch, not PR author#212

Merged
shuheng-liu merged 1 commit into
mainfrom
claude/fix-ci-closed-prs-LnVFT
Apr 29, 2026
Merged

fix(ci): gate extract-claude-lessons on head branch, not PR author#212
shuheng-liu merged 1 commit into
mainfrom
claude/fix-ci-closed-prs-LnVFT

Conversation

@shuheng-liu
Copy link
Copy Markdown
Member

@shuheng-liu shuheng-liu commented Apr 29, 2026

What this does

.github/workflows/extract-claude-lessons.yml was added in #199 but
its if: gate has never let a job run. The previous condition was:

if: >-
  github.event.pull_request.merged == true
  && contains(github.event.pull_request.user.login, 'claude')
  && !startsWith(github.event.pull_request.title, 'chore(claude): learn from')

Two notes on why this was failing — both from looking at the recent
merged PRs (#171, #178, #182, #183, #189, #190, #198, #199, #204, #207,
#208, #209, #206):

  1. [closed] is the right trigger — there is no [merged] action
    type for pull_request in GitHub Actions. closed fires for both
    close-without-merge and merge, and the merged == true check
    correctly narrows it. So the trigger isn't the bug.
  2. user.login is the wrong field. In this repo, Claude Code on
    the web pushes to a claude/<slug> branch, then a human (e.g.
    shuheng-liu) opens the PR. So pull_request.user.login is the
    human, never claude*, and contains(..., 'claude') is always
    false. The reliable signal is the head branch prefix, which is
    under Claude Code's control.

This PR swaps the filter to startsWith(head.ref, 'claude/'). Backfill
across the 14 most-recent merged PRs:

filter match count
contains(user.login, 'claude') (old) 0
startsWith(head.ref, 'claude/') (new) 13

Tag: 🐛 Bug

How it was tested

  • Confirmed via the GitHub API (gh api .../pulls?state=closed) that
    every recent PR's user.login is the human reviewer's login and
    every Claude-touched PR's head.ref starts with claude/.
  • Verified the YAML still parses (only the boolean expression in the
    if: changed).
  • Did NOT run pre-commit run --all-files — the sandbox this PR was
    authored in has no pre-commit binary. Please run it locally if the
    CI hook complains.
  • No runtime code changed, so no pytest was run.
    tests: skipped — workflow YAML condition only, no Python touched.

How to checkout & try? (for the reviewer)

The workflow only fires on pull_request: closed, so the real test is
to merge this PR and watch the next merged claude/* PR — the
extract-lessons job should now appear in the Actions tab and either
open a chore(claude): learn from #N PR or exit cleanly.

git fetch origin claude/fix-ci-closed-prs-LnVFT
git diff main...origin/claude/fix-ci-closed-prs-LnVFT -- .github/workflows/extract-claude-lessons.yml

Checklist

  • I have added Google-style docstrings to important functions and ensured function parameters are typed.
  • My PR includes policy-related changes.
    • If the above is checked: I have run the GPU pytests (pytest -m "gpu") and regression tests.

(This PR only touches a single GitHub Actions YAML file — no Python
functions are added or changed, so the docstring item is vacuously
satisfied. The policy-changes box is correctly unchecked.)

Note: Before submitting this PR, please read the contributor guideline.

The previous filter required the PR's user.login to contain 'claude',
but Claude Code on the web pushes to a claude/* branch and a human
opens the PR — so user.login is the human and the workflow has never
fired. Switch to startsWith(head.ref, 'claude/'), which matches the
actual signal of Claude involvement.

https://claude.ai/code/session_01M9a6hQdzjmLh7rL6ak1PLk
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Confirmed via gh api /repos/TensorAuto/OpenTau/pulls?state=closed: every recently merged PR has user.login = shuheng-liu and head.ref starting with claude/, matching the PR description's backfill table. The new gate (startsWith(github.event.pull_request.head.ref, 'claude/')) is the right signal.

A few notes:

  • pull_request.head.ref is the bare branch name (no refs/heads/ prefix), so startsWith(..., 'claude/') is the correct comparison.
  • Recursion guard is unchanged (the chore(claude): learn from title check), so lessons-extraction PRs — which would also be on claude/* branches — still won't loop.
  • The change is workflow-YAML only; the PR explicitly defers pre-commit and pytest. That's appropriate here — the only way to truly validate is to merge and watch the next merged claude/* PR.

No blocking issues found.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 29, 2026

[claude-review] summary for commit cd6a496

No blocking issues found.

  • nit.github/workflows/extract-claude-lessons.yml:34head.ref is the right field and startsWith(..., 'claude/') is the right matcher; backfill in the PR description (13/14 recent merged PRs) is verified against the live API.

@shuheng-liu shuheng-liu self-assigned this Apr 29, 2026
@shuheng-liu shuheng-liu marked this pull request as ready for review April 29, 2026 23:01
@shuheng-liu shuheng-liu merged commit 35629df into main Apr 29, 2026
7 of 8 checks passed
@shuheng-liu shuheng-liu deleted the claude/fix-ci-closed-prs-LnVFT branch April 29, 2026 23:08
@claude claude Bot mentioned this pull request Apr 29, 2026
3 tasks
shuheng-liu added a commit that referenced this pull request May 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants