Skip to content

Acknowledge a /review comment immediately, so it doesn't look like nothing is happening #20

Closed

Description

@polewskm

Problem

After someone comments /review, nothing visible happens until the review finishes, which takes minutes. In the meantime the pull request shows only the comment and, at most, a failed or in-progress deployment line for the code-review environment. People conclude the command didn't work and comment again, or give up. This happened on #19 and on NCodeGroup/project-template#124.

The automatic reviews on open and ready have the same silence, but a /review comment is a request, so the missing reply is more noticeable.

What happens today

  • The workflow starts in a few seconds, then runs: checkout of the default branch, the pull request head (until v0.3.0), setup-node, npm ci, the review, and only then the publish step.
  • The app token is minted after the review step on purpose: the app's private key is not in the review step's environment, so nothing is posted until the review is done. The "Code review" check run is created in the publish step too.
  • A comment that isn't a /review command starts the workflow and ends it as skipped, so a visible acknowledgement must not appear for those.

Wanted

An immediate, visible sign that the command was seen, within about ten seconds, and that says what to expect. Either or both:

  1. A reaction (👀) on the /review comment, added by the first step that can.
  2. A "Code review" check run in_progress, created early, with a short summary such as "Review started at ; it usually takes a few minutes", and completed by the publish step (the existing check run, updated, not a second one).
  3. Optionally, a placeholder summary comment carrying the ncode-review:summary marker, with the same text, edited in place when the review finishes. The engine already finds its own summary comment, so it should replace the placeholder and not post a second one.

It must also cover an automatic review (open, ready for review), and show a failure in the same place: when the review step fails, as it did with "No provider available", the check run should end failure or neutral with that reason, not stay in_progress, and the placeholder should say it failed.

Design constraints

  • The app's private key must not reach the review step. An early step could mint the token and pass only the token to a small acknowledgement step, before the review step runs, as long as the token isn't in the review step's environment or the agents'. Or the acknowledgement can use the job's own github.token, which needs pull-requests: write (reactions on a comment) or checks: write in the workflow's permissions, and the workflows grant read-only today. Pick one and say what the workflow files must change; every adopted repository pins the action, so a new input or a new permission means each repository's workflow changes.
  • It should come before setup-node and npm ci, which is most of the wait, so it can't be a step the engine runs. Likely an early step in action.yml.
  • A comment that isn't /review from a writer on a pull request must get nothing. The check that decides this today is in the workflow's if: and again in the engine, and the early step has to use the same one.
  • Pull requests from forks are not reviewed and should get no acknowledgement.

Acceptance

  • /review from a writer gets a visible sign within about ten seconds, in a run where the model call later fails.
  • A run that fails ends with the failure visible on the pull request, not an in-progress check.
  • No second summary comment or second check run appears when the review finishes.
  • A non-/review comment, a fork and a comment from someone without write access get nothing.
  • A test or a documented manual check for each of those.

Activity

  1. polewskm commented on Oct 31, 2026

    @polewskm
    ContributorAuthor

    A second example, from NCodeGroup/project-template#124: a /review comment at 00:28:10 started a run at 00:28:13 that failed at 00:28:33 with "No provider available…" (the repository wasn't yet on the Bedrock secret's list), and nothing appeared on the pull request for the next three minutes except a red deployment line in the timeline. So the case to cover is a run that fails quickly and leaves no sign, as well as a slow success: the failure must end up on the pull request, with its reason.

  2. polewskm commented on Oct 31, 2026

    @polewskm
    ContributorAuthor

    One more failure mode to cover with the same in-progress then completed check: no provider available. That is either the API credits used up with the Bedrock key missing, expired or refused, or Anthropic login failing with no key. Today chooseProvider throws and the job fails red. Wanted: the "Code review" check completes neutral with the title "Review budget used up" and the reason (credits exhausted / key refused / key missing), and the summary says the autopilot can carry on without it. Nothing is charged anywhere. Because of #34, this has already happened in production.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions