anthropics / anthropics/claude-code-action

checkContainsTrigger: ternary with two identical branches, and the trigger regex rebuilt five times

Offen
#1,766 1 Kommentar 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
dev-experience good first issue p3
Vorherrschende Sprache
TypeScript
Sterne
8.9k
Forks
2.1k
PR-Merge-Kennzahlen
PR-Kennzahlen ausstehend

Beschreibung

**Type:** code quality / dead code
**Severity:** low
**Area:** `src/github/validation/trigger.ts`
**Effort:** trivial

## Summary

Two small defects in the trigger matcher, which is the function that decides
whether the action runs at all.

### 1. Ternary with identical branches

`src/github/validation/trigger.ts:128-130`

```ts
const commentBody = isIssueCommentEvent(context)
? context.payload.comment.body
: context.payload.comment.body;
```

Both arms are the same expression. The type narrowing is already established by
the enclosing `if (isIssueCommentEvent(context) || isPullRequestReviewCommentEvent(context))`,
so the ternary does nothing. It reads as though the two event shapes differ,
which is misleading for anyone touching this code - they do not, both payloads
expose `comment.body`.

### 2. The same regex is rebuilt five times

The identical `new RegExp(...)` literal appears at lines 54-57, 79-82, 106-109,
131-134 and inside the issue-title branch:

```ts
const regex = new RegExp(
`(^|\s)${escapeRegExp(triggerPhrase)}([\s.,!?;:]|$)`,
"i",
);
```

Five copies of a pattern that must stay in agreement. If the character class is
ever widened (say to accept a trailing `)` or `-`), four of the five sites will
keep the old behaviour and the trigger will fire for comments but not issue
bodies, or for PR titles but not review bodies. That class of divergence is
invisible until a user reports "@claude works in comments but not in my PR
description".

## Impact

No current misbehaviour - all five copies are presently identical, and the dead
ternary evaluates correctly. This is purely a maintenance hazard in a
high-traffic, security-relevant function.

## Suggested fix

```ts
/**
* Matches the trigger phrase as a whole token: preceded by start-of-string or
* whitespace, followed by whitespace, sentence punctuation, or end-of-string.
*/
function buildTriggerRegex(triggerPhrase: string): RegExp {
return new RegExp(`(^|\s)${escapeRegExp(triggerPhrase)}([\s.,!?;:]|$)`, "i");
}
```

Build it once at the top of `checkContainsTrigger` and reuse it in all five
branches. Replace the dead ternary with:

```ts
const commentBody = context.payload.comment.body;
```

`escapeRegExp` is already exported from this file, so the helper can live beside
it. Coverage exists in `test/trigger-validation.test.ts`.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.