Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Refactor daily project management slack digest#13157

Draft
jimchamp wants to merge 1 commit into
internetarchive:masterinternetarchive/openlibrary:masterfrom
jimchamp:pm-workflow-updatesjimchamp/openlibrary:pm-workflow-updatesCopy head branch name to clipboard
Draft

Refactor daily project management slack digest#13157
jimchamp wants to merge 1 commit into
internetarchive:masterinternetarchive/openlibrary:masterfrom
jimchamp:pm-workflow-updatesjimchamp/openlibrary:pm-workflow-updatesCopy head branch name to clipboard

Conversation

@jimchamp

Copy link
Copy Markdown
Collaborator

Refactors existing daily project management digest workflow, removing the need for issue_comment_bot.py.

Technical

Testing

Screenshot

Stakeholders

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Took a look at this while scoping the "Issue Follower" sub-initiative under #13160 — this PR already covers most of that ground (event-driven Needs: Response labeling instead of the daily batch job), so rather than open a competing PR, a few concrete proposals here:

Blocking: missing config file

Both needs_response_labeler.yml and pm_new_comment_digest.yml read .github/workflows/config/new_comment_digest.json via fs.readFileSync(...), but it isn't part of this diff — as-is, both workflows will crash at runtime (file not found).

From how the fields are consumed in the diff, the shape needed is roughly:

{
  "leads": [
    { "githubUsername": "mekarpeles", "leadLabel": "Lead: @mekarpeles", "slackId": "U0XXXXXXX" },
    { "githubUsername": "cdrini", "leadLabel": "Lead: @cdrini", "slackId": "U0XXXXXXX" }
  ],
  "bots": [
    { "githubUsername": "openlibrary-bot", "triggersNeedsResponse": false },
    { "githubUsername": "dependabot[bot]", "triggersNeedsResponse": false },
    { "githubUsername": "renovate[bot]", "triggersNeedsResponse": false },
    { "githubUsername": "copilot-pull-request-reviewer[bot]", "triggersNeedsResponse": false }
  ]
}

slackId values need filling in by whoever has workspace access — not something derivable from the repo. leads presumably needs an entry per Lead: label currently in use, not just the two shown here.

Old digest workflow isn't retired

.github/workflows/new_comment_digest.yml (daily 08:30 cron, invokes scripts/gh_scripts/issue_comment_bot.py) still exists on master and isn't touched by this diff. Since the PR description says the goal is removing the need for issue_comment_bot.py, landing this as-is would leave both the old daily digest and the new pm_new_comment_digest.yml (08:45 cron) posting to Slack in parallel, and both the old script and the new real-time labeler attempting to apply Needs: Response. Worth having this PR also remove/deprecate new_comment_digest.yml (and the script, if nothing else calls it) so there's one system of record.

Good design choice worth keeping

Using an explicit bots config list (rather than checking GitHub's user.type/is_bot fields) to exclude bot accounts from triggering Needs: Response is the right call — we independently hit the same class of bug elsewhere this week: openlibrary-bot's account type is "User", not "Bot", so anything relying on user.type != 'Bot' silently fails to exclude it (see #13161, #13165). This design avoids that trap as long as openlibrary-bot (and the other bot accounts above) are actually listed once the config file lands.

Possible follow-on, not a blocker for this PR

needs_response_labeler.yml currently applies a blanket Needs: Response regardless of what the comment actually says. A natural v2 (separate PR, once this lands) could have an LLM-based pass read the comment and apply a more specific label where one clearly fits — e.g. an assignment request vs. a blocked/waiting question vs. a stalled-assignee check-in — rather than always defaulting to the same label. Flagging for later, not asking for it here.

Linking this to #13160 (Epic: AI Workflows) as the "Issue Follower" sub-initiative — no separate PAM PR planned for this as long as this one lands.

@jimchamp

jimchamp commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@openlibrary-bot's "Blocking: missing config file" section in the above message is not correct. The config file does indeed exist, and contains the appropriate configurations.

cc @mekarpeles

Edit: Also noting that the old daily digest workflow hasn't been removed b/c I'd like to compare the Slack messages that each workflow publishes. Once this workflow is producing the desired output, I'll delete the old digest.

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Correction: The "Blocking: missing config file" point in our 2026-07-15 comment was wrong — .github/workflows/config/new_comment_digest.json does exist on master with the appropriate configuration; it simply predates this diff so it doesn't show up in the PR's changed files. Thanks to @jimchamp for catching and correcting this. Apologies for the noise.

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Verification (Ada, on behalf of Mek): Read the current diff's three workflow files in full (needs_response_labeler.yml, pm_new_comment_digest.yml, publish_slack_thread_digest.yml), checked them out in an isolated worktree, and cross-referenced against the live .github/workflows/config/new_comment_digest.json and recent bot activity in the repo (GitHub Actions/local YAML execution wasn't runnable here, noted below).

Result: One real, currently-reachable issue found; the two non-blocking points from the earlier review still hold as accurate.

Details:

  1. Bot-exclusion list is incomplete for the labeler's actual trigger surface. needs_response_labeler.yml skips "Needs: Response" only for commenters listed in config.bots with triggersNeedsResponse: false. The live config currently lists only openlibrary-botrenovate[bot] isn't in it. Renovate commented on open PRs #12808 and #12761 as recently as 2026-07-21, and both are still open. Once this workflow lands, routine Renovate comments on those (and any future) PRs will get mislabeled "Needs: Response." Worth adding renovate[bot] (and any other bots that post plain issue comments — dependabot[bot] doesn't appear to have commented on this repo recently, but is worth including defensively) to new_comment_digest.json before/at merge, since the config file itself isn't part of this diff and won't otherwise get updated alongside the workflow that depends on it.

  2. Re-checked the two non-blocking points from the 2026-07-15 comment against the current file contents — both still hold: the explicit bots config-list design (vs. relying on GitHub's user.type) is still the right call given Issue Enrichment: allowed_non_write_users bypass not taking effect for non-write actors #13161/Issue Enrichment does not exclude openlibrary-bot (account type is User, not Bot) #13165, and the "old digest workflow left in place intentionally to compare Slack output" is confirmed by jimchamp's note and matches the current diff (the old new_comment_digest.yml/issue_comment_bot.py path isn't touched here).

  3. Couldn't verify locally: GitHub Actions execution itself (cron trigger, workflow_dispatch override step, the github-script steps, Slack posting). Read-through suggests the workflow_dispatch override (job-level env: default overridden via $GITHUB_ENV when dispatched manually) and the channel-empty-skips-publish logic in pm_new_comment_digest.yml are both correct, but this wasn't exercised end-to-end.

  4. Minor, non-blocking: in publish_slack_thread_digest.yml, if a caller ever passes message: '' together with a non-empty replies_json (no current caller in this PR does), publish_replies would silently no-op since no parent thread gets created to reply into. Not reachable by either current caller — just worth knowing if a future caller is added.

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.