Skip to content

ci: tolerate book preview comment permission errors #3635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
thepastaclaw wants to merge 1 commit into
base: v3.1-dev
Choose a base branch
from
Draft

ci: tolerate book preview comment permission errors #3635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 37 additions & 28 deletions .github/workflows/book-preview.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ on:

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

@thepastaclaw thepastaclaw May 12, 2026

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']


concurrency:
Expand Down Expand Up @@ -50,36 +51,44 @@ jobs:
const repoUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}`;
const artifactUrl = `${repoUrl}/actions/runs/${runId}`;

const { data: comments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
});

const marker = '<!-- book-preview -->';
const existing = comments.find(c => c.body.includes(marker));
const body = [
marker,
`📖 **Book Preview** built successfully.`,
``,
`Download the preview from the [workflow artifacts](${artifactUrl}).`,
`To view locally: download the artifact, unzip, and open \`index.html\`.`,
``,
`_Updated at ${new Date().toISOString()}_`,
].join('\n');

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

@thepastaclaw thepastaclaw May 12, 2026

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']


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

@thepastaclaw thepastaclaw May 12, 2026

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']

const body = [
marker,
`📖 **Book Preview** built successfully.`,
``,
`Download the preview from the [workflow artifacts](${artifactUrl}).`,
`To view locally: download the artifact, unzip, and open \`index.html\`.`,
``,
`_Updated at ${new Date().toISOString()}_`,
].join('\n');

if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body,
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body,
});
}
} catch (error) {
if (error?.status === 403 && String(error?.message ?? '').includes('Resource not accessible by integration')) {
core.warning('Skipping book preview PR comment because this workflow token cannot write comments for the event.');
} else {
throw error;
}
}
Loading