feat: add lint-strategy input for squash-merge workflows#35
Merged
Conversation
Adds a new `lint-strategy` action input that decides which items the action should hand to commitlint: - `commits` (default) — current behaviour, lints every commit in the event. Backwards compatible: workflows that don't set the input see zero change. - `pr-title` — lints only the pull request title, read directly from the webhook payload (no extra GitHub API calls). Useful for squash-merge workflows where individual commits are discarded at merge time and the PR title becomes the squash commit message. - `both` — lints the PR title and every commit. Degrades gracefully to `commits` on events without a `pull_request` payload. Implemented as a Strategy pattern in `src/strategies/`, orthogonal to the existing event-based fetchers. `pr-title` skips the `fetchCommits` callback entirely, so no `pulls.listCommits` API round-trip happens when the strategy has no use for the commits. To make `pr-title` safe to combine with `push: branches: [main]` triggers (so the post-merge `push` run does not fail), `run()` detects the `pr-title` + non-PR-payload case and emits a workflow notice and returns success rather than failing with "No commits found to lint". `commit-depth` continues to apply only to commits; the PR title is never trimmed. All strategy code is fully immutable: `readonly` fields, `ReadonlyArray` returns, frozen arrays and items. 100% line/branch/function coverage on `src/strategies/*` plus new integration scenarios in `test/index.test.ts` covering every strategy on every relevant event type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le docs) Follow-up to the initial `lint-strategy` PR. Addresses four real gotchas surfaced during a deeper audit. 1. **README squash-merge recipe was missing `edited`.** GitHub's default `pull_request` activity types are `[opened, synchronize, reopened]` — `edited` is **not** included. Without `edited`, a contributor who opens a PR with a bad title and then fixes the title in the GitHub UI would not re-trigger the workflow, leaving them stuck unless they pushed a new commit. The recipe and the input description in the README now opt in to `edited` and explain why. 2. **`action.yml` description was stale.** It still claimed `lint-strategy: pr-title` would fail with "No commits found to lint" on push/merge_group, even though that path was already replaced with a graceful skip-with-notice. Updated to match reality and to mirror the README's `edited` advice for marketplace consumers. 3. **Defensive `null` check on `pull_request`.** The early return in `run()` and the check inside `PrTitleStrategy.resolve()` only handled `pull_request === undefined`. A literal `"pull_request": null` in the payload would have slipped past both and crashed on `pr.title`. Both spots now also handle `null`, with new unit and integration tests covering the scenario. 4. **Notice message wording.** The previous wording named only `push` and `merge_group`, but the skip can also fire on `workflow_dispatch`, `schedule`, and other events without a PR context. Tightened to "events without a pull request context, such as a 'push' event fired after a squash-merge". 79 tests pass (up from 77), `src/strategies/*` still 100% coverage, and `dist/main.cjs` rebuilt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Six follow-ups surfaced during a thorough audit of the lint-strategy work, focused on documentation accuracy, formatter wording, factory hardening, and a real test for the graceful skip notice. - README: extend the `edited` warning to cover `lint-strategy: both`, which has the same UX trap as `pr-title` (PR title fixes won't re-trigger the workflow without `edited` in the trigger types). - Formatter: replace the misleading hardcoded "commits were analyzed as part of this push" wording with neutral "messages were analyzed", rename the `SHA` table column to `ID` so PR-title rows don't read oddly, and pluralize the headline correctly. - index.ts: pluralize "Found N commit message(s) with errors/warnings" so a single failing commit no longer reads as "1 commit messages". - strategies/index.ts: add a defensive `default` branch with an `assertNever` cast so a future addition to `LintStrategyName` that forgets to update the factory throws at runtime instead of silently returning undefined. - tests: add a stdout-spy test that asserts a workflow notice is actually emitted on the `lint-strategy=pr-title` graceful skip path, closing a real test gap. Also add a factory test for the new defensive default branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
A pedantic third-pass audit of the workflows, README, and top-level config files surfaced both pre-existing accuracy bugs and PR-related staleness in the user-facing documentation. - README: remove the dead `config-file` reference from the main usage example. The input was removed in commit 4160881 ("More changes") but never cleaned up — it's not declared in `action.yml`, not read anywhere in `src/`, and silently no-ops for users who set it. Cosmiconfig handles configuration auto-detection on its own. - README: update the `lint-strategy` inline comment in the main example so users who switch from `commits` to `pr-title`/`both` see the `edited` requirement at the point of use, not just buried in the squash-merge subsection. - action.yml: declare the previously-undocumented `working-directory` input. It was already read by `src/index.ts` and worked at runtime via env-var translation, but was invisible to the Marketplace card, IDE validators, and anyone reading `action.yml` to discover the available inputs. - action.yml + package.json: update the top-level descriptions to mention that the action can lint pull request titles in addition to commits, since both descriptions were stale after the lint-strategy work landed. - src/index.ts: generalize the graceful-skip notice message so it names both `push` (post squash-merge) and `merge_group` (merge queue) as expected events instead of only the push case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore proper noun/verb agreement in DefaultFormatter summary lines (regression from a previous round that removed "commit/commits" without updating dependent verbs) - Pluralize the "All N commit messages are okay" success log - Drive-by typo fix: "warning" -> "warnings" in fail-on-warnings notice
The "Found N commit message(s) with errors" / "...with warnings" messages emitted by setFailed() were using `Results.errorCount` and `Results.warningCount`, which sum the number of individual findings across all commits. A single commit with three errors was reported as "Found 3 commit messages with errors", which is misleading. Add `errorCommitsCount` and `warningCommitsCount` to `Results` (count of distinct commits with at least one error/warning) and use them in the failure messages. The new counts are computed in the same reduce pass as the existing totals, so there is no extra iteration. Refactor DefaultFormatter.formatSummary to consume `errorCommitsCount` from Results instead of recomputing it inline. Tests added to test/linter/result.test.ts cover the bug case (one commit with multiple findings) for both errors and warnings, and all existing assertions are extended to cover the two new properties. No breaking change: the existing `errorCount` / `warningCount` properties retain their meaning and value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a new
lint-strategyaction input with three values:commits(default) — lints every commit in the event. Identical to today's behaviour.pr-title— lints only the pull request title, read straight from the webhook payload (no extra GitHub API call).both— lints the PR title and every commit.Implemented as a Strategy pattern in
src/strategies/(CommitsStrategy,PrTitleStrategy,BothStrategy+ a tinygetLintStrategy()factory). Strategies sit between the existing event-specific commit fetchers and the linter, so the fetcher layer is unchanged.When
lint-strategy: pr-titleis used on an event without a pull request in the payload (push,merge_group, etc.), the action emits acore.notice()and exits successfully instead of failing with "No commits found to lint". This makes the strategy safe to combine withpush: branches: [main]triggers — the post-mergepushrun no longer fails.commit-depthcontinues to limit commits only; the PR title is never trimmed.This PR also includes several drive-by quality fixes discovered during multi-round review:
DefaultFormatterJob Summary linesfail-on-warningsnoticeRelated Issue
N/A — direct contribution.
Motivation and Context
For repositories that use squash-merge, individual commits on a feature branch are discarded at merge time. The single squash commit that lands in
maindefaults its message to the PR title, so linting the per-commit messages is busywork — the thing that actually ends up in git history is the PR title, and that is what should be enforced.Before this change, users had no way to point this action at the PR title. After this change,
lint-strategy: pr-titledoes exactly that, with zero extra API calls (the title is read from the existing webhook payload).The graceful skip on non-PR events was added after a footgun was identified: a workflow with both
pull_requestandpush: branches: [main]triggers would fail every merge underpr-titlebecause the post-mergepushevent has no PR title in its payload. The skip-with-notice behaviour makes the mixed-trigger pattern safe.How Has This Been Tested?
test/strategies/{commits,pr-title,both,index}.test.ts, including:PrTitleStrategyasserting the fetcher is never invoked (mock throws on call)BothStrategyconstructor injection + depth interaction (PR title not counted towardcommit-depth)test/index.test.tscovering:pr-titleonpull_requestwith valid/invalid titlepr-titleonpushandmerge_group(asserts graceful skip, fetcher not called)bothonpull_requestwith valid title + commits, and invalid title + valid commitslint-strategyvalue rejected with a clear errorprettier --check,npm run lint,tsc --noEmit,knip,npm test,npm run buildsrc/strategies/*Documentation:
README updated:
lint-strategyentry in the Inputs section documenting all three valuesJSDoc on every new class, interface, method, and the input parser.
Checklist:
🤖 Generated with Claude Code