Skip to content

perf(ci): start no job for a comment with no slash command - #365

Merged
MattJColes merged 4 commits into
mainfrom
claude/lgtmaybe-perf-workflow-gate-3j8akh
Aug 3, 2026
Merged

perf(ci): start no job for a comment with no slash command#365
MattJColes merged 4 commits into
mainfrom
claude/lgtmaybe-perf-workflow-gate-3j8akh

Conversation

@MattJColes

Copy link
Copy Markdown
Owner

What

issue_comment fires on every comment on every pull request. Today each one
starts a job: a runner is claimed, the container pulled, Python booted, and
execute_comment calls parse_command, gets None, prints "No lgtmaybe slash
command found; ignoring." and exits. Correct behaviour and zero provider spend —
but a whole job. On a contended self-hosted pool that queueing is what delays the
reviews that do have work to do; on GitHub-hosted runners it is wasted minutes
and a confusing no-op check on every comment thread.

That decision is deterministic, so it belongs in the job if:, where it costs
nothing.

The guard

Added to the issue_comment arm only:

      (github.event.issue.pull_request &&
       contains(fromJson('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association) &&
       (contains(github.event.comment.body, '/review') ||
        contains(github.event.comment.body, '/improve') ||
        contains(github.event.comment.body, '/ask') ||
        contains(github.event.comment.body, '/describe') ||
        contains(github.event.comment.body, '/diagram'))) ||

Not gated:

  • the pull_request_review_comment arm — that is answer_replies, where a PR
    author's reply inside a finding thread is plain prose with no command in it.
    Gating it would silently switch the feature off.
  • the pull_request_target arm — the automatic review on open/sync.

Why it is looser than the parser, on purpose

src/lgtmaybe/cli/slash.py::parse_command strips the body, requires it to
start with /, splits on any whitespace, and lower-cases the first token
before matching SlashCommand. The guard is a superset of that:

Parser rule Guard
command must be at the start of the body not required — contains is substring-based
parts[0].lower() — case-insensitive contains() is documented "not case sensitive", so /REVIEW passes
/review full, /review\nfull, leading whitespace all parse all pass
exact token match (/reviewer is rejected) /reviewer passes the guard, then the CLI rejects it

A guard tighter than the parser would silently break a command that used to
work; being looser only costs the occasional harmless job.

Verification

  • New test (written failing-first): tests/test_workflow_examples.py::test_comment_arm_starts_no_job_without_a_slash_command
    — iterates the dogfood workflow and all ten starters, asserts one
    github.event.comment.body clause per SlashCommand member (so adding a
    command to the enum breaks the workflows until they admit it), asserts the
    count is exactly len(SlashCommand) and that the clause sits between the
    issue_comment arm and the pull_request_review_comment arm — i.e. the reply
    arm stays ungated.
  • Expression simulation (throwaway script, not committed): translated each of
    the 11 workflow if: strings into Python under Actions semantics
    (case-insensitive contains, null cast to "", property access on null yields
    null) and evaluated 16 scenarios each — PR opened by member/stranger, each of
    the five commands, /review full, /REVIEW, leading whitespace, /review\nfull,
    a plain comment, /deploy, /review from a stranger, a comment on a non-PR
    issue, and a finding-thread reply. 176/176 as expected, and parentheses balance
    in every file. In particular pull_request_target (where github.event.comment
    is null) is unaffected, and the reply arm still fires on plain prose.
  • Differential check against the real parser: 990 generated comment bodies run
    through both parse_command and the evaluated expression — zero bodies the
    parser accepts but the guard rejects.
  • contains()'s case-insensitivity confirmed against the GitHub docs source, not
    from memory: "This function is not case sensitive. Casts values to a string."
    The docs do not state &&/|| precedence, so every arm and the OR group are
    fully parenthesised — precedence is not load-bearing here.
  • Gate: uv run ruff check . ✅ · uv run ruff format --check . ✅ ·
    uv run mypy ✅ · uv run pytest -q ✅ (1828 passed, 3 skipped) ·
    uv run pytest tests/specs -q ✅ (17 passed).

Files

  • .github/workflows/lgtmaybe.yml — the dogfood workflow. The existing
    author-association reasoning and both concurrency guards are preserved verbatim;
    the new clause is appended to the comment arm with its own comment paragraph.
  • examples/workflows/*.yml — all ten starters, identically.
  • Docs: docs/how-to/use-as-github-action.md and the three cloud how-tos
    (review-with-{azure,bedrock-oidc,vertex-wif}.md) carry copy-paste snippets, so
    those got the guard too; docs/explanation/trust-and-cost.md gains a paragraph
    explaining the thrift and why the reply arm is exempt; README.md's minimal
    snippet too. The "open it up to everyone" advice used to say drop the if:
    which would now also drop this guard — so it says drop the author-association
    checks
    instead. docs/llms-full.txt regenerated via
    uv run python docs/generate_llms_txt.py.
  • tests/test_workflow_examples.py — the new test.

Spec anchors

No spec section went stale: parse_command and the slash router
(cli.slash) are untouched — this changes only which comments ever reach them.
uv run pytest tests/specs -q is green.

Note on coverage

This is a config/YAML change. The repo does have a workflow-file harness
(tests/test_workflow_examples.py), so it was extended failing-first rather than
left uncovered — but note that what is asserted is the shape of the if:
string
. GitHub's expression evaluator itself is not exercised by pytest; the
simulation and differential checks above were run by hand and are not committed.


🤖 Generated with Claude Code


Generated by Claude Code

`issue_comment` fires on every comment on every pull request, so every "lgtm,
merging" claimed a runner, pulled the container and booted Python only for
`execute_comment` to find no command and exit. Correct and free of provider
spend, but a whole job — and on a contended self-hosted pool that queueing is
what delays the reviews with real work to do. It is deterministic work, so it
belongs in the job `if:`, where it costs nothing.

The guard is added to the `issue_comment` arm only, keyed on the five names
`SlashCommand` accepts. It is deliberately looser than `parse_command`, which
additionally requires the command at the START of the body: `contains` is
case-insensitive (`/REVIEW` passes) and substring-based (`/review full`,
leading whitespace, `/review\nfull` all pass). A guard tighter than the parser
would silently disable a command that used to work.

The `pull_request_review_comment` arm is untouched: that is `answer_replies`,
where a PR author's reply inside a finding thread is plain prose with no
command in it. So is the `pull_request_target` arm.

Every arm stays fully parenthesised, so `&&`/`||` precedence is not
load-bearing, and `github.event.comment` being null on `pull_request_target` is
already handled by that arm's own condition.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ 2 of 4 review calls failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (35400 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.

⏱️ 1 batch was too big for one call (timed out, or ran past the max_tokens ceiling) and was reviewed in smaller pieces instead. Consider a lower max_input_tokens, a higher max_tokens, or a faster model.

2 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2

@lgtmaybe lgtmaybe Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ lgtmaybe review failed: review incomplete — every review call failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (34216 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less). Check the provider credentials/quota, model, and timeout (ollama: a larger model needs a longer --timeout), then retry.

lgtmaybe 1.12.2

Comment thread tests/test_workflow_examples.py Outdated
Comment thread tests/test_workflow_examples.py Outdated
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Gate issue_comment jobs on slash commands

Structure

flowchart LR
    n0["issue_comment event<br/>GitHub webhook<br/>fires on every PR comment"]
    n1["Workflow `if:` guard<br/>GitHub Actions<br/>requires slash command in body<br/>(changed)"]
    n2["Reply arm<br/>GitHub Actions<br/>pull_request_review_comment path, left ungated"]
    n3["Slash parser<br/>Python<br/>parse_command and SlashCommand enum"]
    n4["Workflow tests<br/>pytest<br/>assert one guard per command<br/>(changed)"]
    n5["Docs and examples<br/>Markdown/YAML<br/>README, how-tos, and starter workflows carry guard<br/>(changed)"]
    n0 -->|"triggers on comment"| n1
    n1 -->|"leaves ungated"| n2
    n3 -->|"defines command names"| n1
    n4 -->|"validates guard"| n1
    n4 -->|"imports enum"| n3
    n5 -->|"duplicates guard"| n1
Loading

⛶ Open full screen

Text version
[issue_comment event GitHub webhook] --triggers on comment--> [Workflow `if:` guard GitHub Actions (changed)]
[Workflow `if:` guard GitHub Actions (changed)] --leaves ungated--> [Reply arm GitHub Actions]
[Slash parser Python] --defines command names--> [Workflow `if:` guard GitHub Actions (changed)]
[Workflow tests pytest (changed)] --validates guard--> [Workflow `if:` guard GitHub Actions (changed)]
[Workflow tests pytest (changed)] --imports enum--> [Slash parser Python]
[Docs and examples Markdown/YAML (changed)] --duplicates guard--> [Workflow `if:` guard GitHub Actions (changed)]

Sequence

sequenceDiagram
    participant n0 as issue_comment event
    participant n1 as Workflow `if:` guard (changed)
    participant n3 as Slash parser
    n0->>n1: fires issue_comment event
    n1->>n1: evaluates author and command
    n1->>n3: starts job on match
    n3-->>n1: returns parsed command
Loading

⛶ Open full screen

Text version
1. [issue_comment event] -> [Workflow `if:` guard (changed)]: fires issue_comment event
2. [Workflow `if:` guard (changed)] -> [Workflow `if:` guard (changed)]: evaluates author and command
3. [Workflow `if:` guard (changed)] -> [Slash parser]: starts job on match
4. [Slash parser] --> [Workflow `if:` guard (changed)]: returns parsed command

The pull_request_target auto-review arm and the pull_request_review_comment reply arm are intentionally untouched; the new contains() guard is looser than parse_command, so a comment can pass the guard and still be rejected by the parser. Tests assert each SlashCommand appears once, between the issue_comment arm and the reply arm.

Review feedback on the workflow-guard test, both points valid.

The assertions checked that each SlashCommand value appeared in the `if:`
string and that the body clauses sat positionally between the issue_comment and
pull_request_review_comment arms — but nothing pinned that the command group is
positively ANDed with the author-association check *inside* the issue_comment
arm. Hoisting the group into its own `||` branch is still positionally between
the two arms, so every assertion kept passing. Verified: the old assertions all
pass on that mutation, and under it a stranger commenting /review starts a job —
defeating the gate whose stated purpose is to stop a drive-by /review spending
the provider budget.

Replaced with an assertion on the relationship rather than the formatting: a
regex over the whitespace-normalised condition requiring nothing but `&&`
between the author-association check and the command group. It is blind to the
order of the commands inside the group and survives a YAML reflow. The two
positional assertions and the occurrence count gave way to one direct statement
of the intent they were circling — the reply arm must inspect no comment body
at all — which is what protects answer_replies.

Proved non-vacuous by mutating the dogfood workflow and re-running: hoisted into
its own `||` branch, negated, reply arm gated on a command, and /diagram dropped
from the guard all fail, each on the assertion that owns them.

The docstring also overstated the guarantee — the pull_request_review_comment
arm deliberately does start a job for plain-prose replies, which the test's own
final assertion protects — so it is scoped to the issue_comment job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ 3 of 4 review calls failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (35231 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.

💬 2 earlier lgtmaybe conversations are still unresolved on this PR — this run's count covers what it reviewed now, not those.

1 finding · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2

Incremental review of the changes since e4b67e5 — earlier findings stay open until fixed.

Comment thread tests/test_workflow_examples.py
The previous commit traded coverage it did not mean to trade. Pinning the AND
relationship was right, but dropping the occurrence count and the positional
check left nothing verifying WHERE the guard lives. Confirmed by mutation
rather than by reading: three regressions passed the test as it stood —
deleting `github.event.issue.pull_request`, duplicating the command group into
the `pull_request_target` arm, and moving the guard into its own arm ahead of
the reply arm.

The first is the one that bites. Without `github.event.issue.pull_request` a
`/review` on a plain (non-PR) issue starts a job, and the arm also begins
matching `pull_request_review_comment` events, so a reply quoting "/review"
starts a second one. Both verified against an evaluator for the expression.

Substring and index checks over the whole condition cannot express "in which
arm", which is the root of all three: a clause hoisted into its own `||` branch
reads identically to one ANDed inside the arm it belongs to, and sits at the
same offset. So the condition is now split into its top-level `||` arms first
(`_top_level_arms`), and the assertions name the arm they mean:

- exactly one top-level arm is keyed on `github.event.issue.pull_request`;
- the command group is ANDed onto the trusted-author check *inside that arm*
  (the regex from the previous commit, now scoped to the arm rather than the
  whole string — strictly stronger, and it subsumes the positional check it
  replaces, which the "moved to its own arm" mutation walked straight through);
- the reply arm inspects no comment body at all;
- the body is referenced exactly `len(SlashCommand)` times, which with the
  above pins that no other arm gates on it.

The mutation table grows to seven and each entry now declares which assertion
should catch it, so a mutation caught by the wrong one is reported instead of
counted as covered. All seven fail on their intended assertion, and every
assertion is exercised by at least one.

Not added: a positional "the PR check precedes the command group" assertion.
Within an `&&` chain that order is semantically inert, and pinning it would
re-introduce the reflow brittleness flagged in the previous round. Requiring
both clauses in the same top-level arm is the stronger statement and is what
the mutations verify.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ lgtmaybe review failed: review incomplete — every review call failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (34216 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less). Check the provider credentials/quota, model, and timeout (ollama: a larger model needs a longer --timeout), then retry.

lgtmaybe 1.12.2

@MattJColes
MattJColes merged commit e72903d into main Aug 3, 2026
6 of 7 checks passed
@MattJColes
MattJColes deleted the claude/lgtmaybe-perf-workflow-gate-3j8akh branch August 3, 2026 02:45
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