Skip to content

ci: skip check-skills review job on forks - #1989

Open
Archibald1948 wants to merge 1 commit into
toss:mainfrom
Archibald1948:ci/guard-check-skills-review-job
Open

ci: skip check-skills review job on forks#1989
Archibald1948 wants to merge 1 commit into
toss:mainfrom
Archibald1948:ci/guard-check-skills-review-job

Conversation

@Archibald1948

@Archibald1948 Archibald1948 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Overview

The review job in check-skills.yml is gated only on the event name:

review:
  name: Check intent skill coverage
  if: github.event_name != 'pull_request'

Its triggers are push to main, release: published, and workflow_dispatch — all of which fire in forks too. The job holds contents: write and pull-requests: write, and its last step pushes a skills/review-* branch and opens a PR against github.event.repository.default_branch. In a fork, that default branch is the fork owner's own main.

So every contributor who forks this repository eventually gets an unsolicited "Review intent skills" PR sitting in their fork, addressed to a maintainer audience that does not exist there.

What happens in a fork

fork-pr-evidence
Value in toss/suspensive Value in a fork
github.repository toss/suspensive <user>/suspensive
PR base toss/suspensive:main <user>/suspensive:main
Audience maintainers nobody

The PR body asks the reader to resolve skill version drift and lists ~19 stale-skill signals plus a long agent prompt — all of it meaningless in a fork, since the fork owner is not the one who publishes @suspensive/* skills.

To be clear, this is not a permissions problem: the fork owner is merging into their own repository, which is exactly what owning a fork means, and it has no effect on toss/suspensive. It is repository noise, not a security issue.

Change

Add the repository guard this repo already uses everywhere else that writes:

-    if: github.event_name != 'pull_request'
+    if: github.event_name != 'pull_request' && github.repository == 'toss/suspensive'

The same guard is already present in release.yml:12, graph.yml:19, graph.yml:36, and broken-link-checker.yml:10. check-skills.yml is the only write-capable workflow missing it.

The sibling validate job is intentionally left alone: it is pull_request-only, read-only, and running it in forks is useful, since it validates a contributor's own PR before they send it here.

Note for maintainers

This file carries an intent-workflow-version: 4 header and says it is generated by intent setup. If that template is regenerated, this guard will be dropped again, so the same change may be worth making upstream in @tanstack/intent's workflow template rather than only here.

PR Checklist

  • I did below actions if need
  1. I read the Contributing Guide
  2. I added documents and tests.

No tests or docs apply: this is a one-line CI trigger guard.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WeNzSeeWC9CBPNfhpi88cE

The review job pushes a branch and opens a pull request using
`contents: write` and `pull-requests: write`. It is gated only on the
event name, so it also runs in forks, where it opens a "Review intent
skills" PR against the fork owner's own default branch.

Every other workflow that writes to the repository already guards on
`github.repository == 'toss/suspensive'` (release.yml, graph.yml,
broken-link-checker.yml). Apply the same guard here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WeNzSeeWC9CBPNfhpi88cE
@coauthors

coauthors Bot commented Sep 8, 2026

Copy link
Copy Markdown

People can be co-author:

Candidate Reasons Count Add this as commit message
@Archibald1948 #1989 #1989 (comment) 2 Co-authored-by: Archibald1948 <176382683+Archibald1948@users.noreply.github.com>
@codecov-commenter #1989 (comment) 1 Co-authored-by: codecov-commenter <65553080+codecov-commenter@users.noreply.github.com>

@vercel

vercel Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@Archibald1948 is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot

changeset-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: bb4fd92

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.22%. Comparing base (d86e08a) to head (bb4fd92).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1989   +/-   ##
=======================================
  Coverage   93.22%   93.22%           
=======================================
  Files          42       42           
  Lines         664      664           
  Branches      163      163           
=======================================
  Hits          619      619           
  Misses         42       42           
  Partials        3        3           
Components Coverage Δ
@suspensive/react 100.00% <ø> (ø)
@suspensive/react-query 95.83% <ø> (ø)
@suspensive/react-query-4 100.00% <ø> (ø)
@suspensive/react-query-5 100.00% <ø> (ø)
@suspensive/jotai 100.00% <ø> (ø)
@suspensive/codemods 81.60% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Archibald1948

Copy link
Copy Markdown
Contributor Author

Please review it.
Let me say it again, it doesn’t seem to be a security issue.
Please make the judgment, Maintainer! I may be wrong.

Please close it if it overkills.
:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants