Skip to content

ci: tolerate book preview comment permission errors#3635

Draft
thepastaclaw wants to merge 1 commit into
dashpay:v3.1-devfrom
thepastaclaw:fix-book-preview-comment-permission
Draft

ci: tolerate book preview comment permission errors#3635
thepastaclaw wants to merge 1 commit into
dashpay:v3.1-devfrom
thepastaclaw:fix-book-preview-comment-permission

Conversation

@thepastaclaw
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw commented May 12, 2026

Book preview comment permission handling

Issue being fixed or feature implemented

The Book Preview workflow can build mdBook successfully and still fail on
fork-style PR runs when the final actions/github-script step cannot create or
update a PR comment with the preview artifact link.

This is currently surfacing as red CI on #3092 even though the
preview build and artifact upload completed.

What was done?

  • Added issues: write to the workflow permissions because PR comments use the
    Issues comments API.
  • Wrapped the preview-comment create/update logic in a targeted
    403 Resource not accessible by integration handler.
  • Kept unexpected GitHub API errors failing the workflow so real regressions are
    still visible.

How Has This Been Tested?

  • python3 YAML parse of .github/workflows/book-preview.yml passed.
  • git diff --check passed.
  • Pre-PR code review gate returned ship for
    upstream/v3.1-dev..fix-book-preview-comment-permission.

Breaking Changes

None.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the
    corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling in the automated preview workflow to gracefully handle permission failures when posting comments, preventing build disruptions while logging diagnostic information.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1abf6b5e-a660-4147-bcd7-0a163561094d

📥 Commits

Reviewing files that changed from the base of the PR and between fa1e492 and 181f512.

📒 Files selected for processing (1)
  • .github/workflows/book-preview.yml

📝 Walkthrough

Walkthrough

The book preview workflow is updated to grant issues: write permissions and wrap the PR comment-posting logic in error handling that gracefully skips on permission-denied errors while preserving other error behavior.

Changes

Book Preview PR Comment Posting

Layer / File(s) Summary
Permissions and error-tolerant comment posting
.github/workflows/book-preview.yml
Job permissions add issues: write to enable PR comment creation. The "Post preview link" step wraps comment-listing and create/update calls in try/catch, logging a warning and skipping comments on 403 "Resource not accessible by integration" errors while re-throwing other failures.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A workflow once spoke but had no voice,
Permission denied left little choice,
Now try and catch save the day,
Graceful failures pave the way,
Preview links post without dismay! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ci: tolerate book preview comment permission errors' directly and accurately describes the main change—adding error tolerance for permission-related failures in the book preview workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added this to the v3.1.0 milestone May 12, 2026
@thepastaclaw
Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw
Copy link
Copy Markdown
Collaborator Author

thepastaclaw commented May 12, 2026

✅ Review complete (commit 181f512)

Copy link
Copy Markdown
Collaborator Author

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Small CI fix that wraps the book-preview comment logic in try/catch swallowing 403 'Resource not accessible by integration' and grants issues: write/pull-requests: write. The change is correct and well-scoped. Three nitpicks flagged by Claude are all verified against the file; Codex had no findings.

Reviewed commit: 181f512

💬 3 nitpick(s)

});

const marker = '<!-- book-preview -->';
const existing = comments.find(c => c.body.includes(marker));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: c.body.includes(marker) will throw if a comment body is null

listComments can return entries whose body is null (e.g., deleted/redacted comments). comments.find(c => c.body.includes(marker)) would then throw a TypeError. The new catch block only swallows 403s with the specific 'Resource not accessible by integration' message — a TypeError would be rethrown and fail the workflow with a confusing error. Defensive null-coalescing avoids it.

💡 Suggested change
Suggested change
const existing = comments.find(c => c.body.includes(marker));
const existing = comments.find(c => (c.body ?? '').includes(marker));

source: ['claude']

Comment on lines +55 to 59
const { data: comments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body,
});
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: listComments is not paginated; long-lived PRs can produce duplicate preview comments

github.rest.issues.listComments returns a single page (default 30, max 100). On a PR with many comments, the existing <!-- book-preview --> marker may live on a later page, so the workflow creates a new comment on each push instead of updating in place. Use github.paginate(github.rest.issues.listComments, {...}) to scan all comments. Not a functional bug — just a UX paper-cut.

source: ['claude']

Comment on lines 10 to 13
permissions:
contents: read
issues: write
pull-requests: write
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: issues: write has no effect for fork PRs under the pull_request trigger

With the pull_request trigger (line 4, not pull_request_target), GITHUB_TOKEN is forced read-only for PRs from forks regardless of declared permissions. So issues: write and pull-requests: write only take effect for branch PRs in the same repository — the new try/catch is what actually keeps fork-PR runs green. A short inline comment noting that fork PRs intentionally skip commenting would help the next maintainer reading this workflow.

source: ['claude']

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

Labels

None yet

1 participant