Skip to content

ci: tell the Claude reviewer to wait for its commands in the foreground - #1256

Merged
jooyoungseo merged 1 commit into
mainfrom
ci/claude-review-forks-and-foreground
Sep 15, 2026
Merged

jooyoungseo merged 1 commit into
mainfrom
ci/claude-review-forks-and-foreground

Conversation

@jooyoungseo

Copy link
Copy Markdown
Member

What

One paragraph added to the review prompt:

Run every command in the foreground and wait for it to finish. Nothing resumes this session after your final message, so a command left running in the background, or a wakeup you schedule, is lost, and so is the review. Give a long run a longer Bash timeout (up to 30 minutes) instead.

Why

The first real review after #301/#302 landed, on xability/r-maidr#305, ran for 44 minutes and 68 turns, started testthat and rcmdcheck in the background, and ended its turn with "I'll hold off here and resume once the background test run and rcmdcheck finish (the wakeup I scheduled will bring me back)". In an action run nothing brings it back. The fallback step did its job, posting that sentence with a link to the run rather than losing it, but the PR got no review.

The Bash timeouts the job sets (10 minutes by default, 30 at most) already cover a full rcmdcheck; the prompt just did not say to use them.

Verifying

  • actionlint 1.7.12 with shellcheck 0.11.0: clean.
  • Prompt-only change; the next review on a real PR shows whether the tests are now waited for.

🤖 Generated with Claude Code

On xability/r-maidr#305 the review started the tests in the background,
ended its turn "until the wakeup", and the fallback step posted that
sentence instead of a review: nothing resumes the session once the
action's turn is over. Say so in the prompt, and point long runs at the
Bash timeout instead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jooyoungseo
jooyoungseo merged commit 16706fb into main Sep 15, 2026
@jooyoungseo
jooyoungseo deleted the ci/claude-review-forks-and-foreground branch September 15, 2026 20:26
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review

What changed: a single paragraph added to the review prompt in .github/workflows/claude-code-review.yml (lines 139–143), telling the Claude reviewer to run commands in the foreground and use a longer Bash timeout instead of backgrounding work or scheduling a wakeup.

Code quality

  • Clean, minimal diff (6 additions, 0 deletions), scoped to exactly the described change.
  • Indentation and blank-line separation match the surrounding prompt: | block scalar exactly — no trailing whitespace or tabs (checked with cat -A).
  • The instruction is internally consistent with the job config: it tells the model it can extend a Bash timeout "up to 30 minutes," which matches BASH_MAX_TIMEOUT_MS: "1800000" set in env: (line 58). It's not a made-up number.
  • No conflicting or duplicate guidance elsewhere in the file — this is the only mention of "background"/"wakeup" in the workflow (grep -n -i "background\|wakeup" .github/workflows/claude-code-review.yml).

Bugs / correctness

  • None found. This is a prompt-only change to an Action; it doesn't touch application code, so it can't introduce a functional regression in MAIDR itself.
  • YAML still parses correctly (python3 -c "import yaml; yaml.safe_load(open(...))" succeeds) and the workflow structure (7 steps in the claude-review job) is unchanged apart from the new paragraph.

Performance

  • Not applicable — no runtime code path is affected.

Security

  • No permission, secret, or trigger changes. permissions:, additional_permissions:, and claude_args allowlist are untouched. The added text only shapes model behavior around timeouts; it doesn't expand what the action can do.

Test coverage / verification

  • Ran the repo's actual CI check for this file: downloaded the pinned actionlint v1.7.7 (same version/checksum as .github/workflows/ci.yml, verified via sha256sum --check) and ran it against .github/workflows/claude-code-review.yml with shellcheck 0.9.0 (the version installed on this runner) present — clean, exit 0. This matches the PR description's claim of a clean actionlint/shellcheck run (the description cites newer versions, 1.7.12/0.11.0, than what's pinned/available here, but the result is the same: no findings).
  • npm test, npm run type-check, npm run lint aren't meaningful checks for this diff since no source under src/ or test/ changed; I skipped the full suite for that reason. I did run npm run lint as a sanity check — it reported pre-existing formatting errors, but only inside an untracked .claude-pr/ directory in the runner's working tree (not part of this PR's diff, not tracked by git per git ls-files), so they're an artifact of the review environment, not this change.
  • This is inherently a "prompt engineering" change whose real test is behavioral (does the next review actually wait for its background commands), which the PR description itself acknowledges can only be verified by observing the next real review run — reasonable given there's no way to unit-test model behavior here.

Overall

Small, well-scoped, well-justified fix with a clear failure story in the description (the r-maidr#305 run that ended by scheduling a wakeup nothing would fire). Wording is consistent with the rest of the prompt and with the job's actual timeout configuration. No issues found — looks good to merge.


🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown

🔎 MAIDR example preview is ready

Open the preview: https://pr-1256.maidr-preview.pages.dev

Built from 9a66311. This preview URL is stable and updates automatically on every new commit to the PR.

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.

1 participant