fix: accept GitHub App bot trigger actors#54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates trigger actor normalization so GitHub App bot logins ending in [bot] can be stored instead of being treated as unknown authors.
Changes:
- Extends the GitHub login validation regex to accept a trailing
[bot]suffix. - Adds queue coverage for persisting
copilot-pull-request-reviewer[bot]actor details from context lookup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/github_agent_bridge/actors.py |
Updates login validation to allow GitHub App bot actor names. |
tests/test_queue.py |
Adds an enqueue test for a GitHub App bot trigger actor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pilipilisbot
left a comment
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved. This is the right scope for the trigger-actor issue: the normalization now accepts GitHub App bot logins such as copilot-pull-request-reviewer[bot] while still rejecting the reserved/generic senders that would produce misleading dashboard actors.
What I checked:
- Reviewed the PR diff and surrounding actor lookup/enqueue flow.
- Confirmed the new queue coverage exercises the GitHub App bot actor path from context lookup, not just the fallback sender path.
- Sanity-checked normalization for the Copilot app login, reserved senders, invalid names, and ordinary logins with the PR source on
PYTHONPATH. - Ran
PYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest tests/test_actors.py tests/test_queue.py -q->20 passed. - Ran
PYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest -q->164 passed. - Ran
git diff --check origin/main...HEAD-> clean. - Checked GitHub Actions for this head:
dashboard,pytest (3.11), andpytest (3.12)are passing.
pilipilisbot
left a comment
pilipilisbot
left a comment
There was a problem hiding this comment.
Requesting changes for the test coverage gap Copilot flagged.
The code change itself is small and the current focused suite still passes (tests/test_actors.py tests/test_queue.py -> 20 passed), but the new regression test does not actually exercise the changed normalization path: it monkeypatches github_actor_details_for_context() to return a TriggerActor(login="copilot-pull-request-reviewer[bot]") directly. That means the test would still pass if LOGIN_RE rejected [bot] logins again.
Before merge, please add direct coverage for the behavior this PR is changing, for example normalize_github_login("copilot-pull-request-reviewer[bot]") and/or actor_details_from_github_payload({"user": {"login": "copilot-pull-request-reviewer[bot]", ...}}), while keeping the reserved/generic sender rejection covered. Once that is in place, this should be ready.
pilipilisbot
left a comment
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved after the follow-up in 9a45ea2.
The requested coverage gap is resolved: tests/test_actors.py now directly covers both normalize_github_login("copilot-pull-request-reviewer[bot]") and actor_details_from_github_payload() with the bracketed GitHub App bot login, so the regression test would fail if the [bot] suffix were rejected again. The existing queue coverage still verifies the enqueue persistence path.
Verification on the updated head:
PYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest tests/test_actors.py tests/test_queue.py -q->22 passedPYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest -q->166 passedgit diff --check origin/main...HEAD-> clean- GitHub Actions for
9a45ea2:dashboard,pytest (3.11), andpytest (3.12)are passing
|
Resolved the requested coverage gap in That commit adds direct regression coverage for Verification after the follow-up:
Current PR head is |
pilipilisbot
left a comment
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved on the updated head ba393ca.
The follow-up is a reasonable extension of the same fix: actor lookups now respect GITHUB_AGENT_BRIDGE_GH_BIN, so service deployments that configure a non-default gh path use the same binary for trigger-actor backfill/enqueue, and unauthenticated dashboard HTML routes now redirect to /auth/login when OAuth is configured instead of returning a bare 401.
Verification on this head:
PYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest tests/test_actors.py tests/test_backend.py -q->33 passedPYTHONPATH=src ../github-agent-bridge/.venv/bin/python -m pytest -q->167 passedgit diff --check origin/main...HEAD-> clean- GitHub Actions for
ba393ca:dashboard,pytest (3.11), andpytest (3.12)are passing
|
Post-merge cleanup complete. I fetched |
|
Additional active-workspace cleanup is complete. For this OpenClaw workspace, I removed the clean dedicated PR #54 worktree at Post-merge status checked: PR #54 merged as |
|
Additional post-merge cleanup complete for the canonical checkout under I fetched remotes, confirmed PR head |
Summary
[bot]as valid trigger actorsnotificationsfiltered outcopilot-pull-request-reviewer[bot]Verification
PYTHONPATH=src /home/openclaw/.local/share/github-agent-bridge/venv/bin/python -m pytest tests/test_actors.py tests/test_queue.py -q->22 passedPYTHONPATH=src /home/openclaw/.local/share/github-agent-bridge/venv/bin/python -m pytest -q->166 passedgit diff --check-> cleanContext
GitHub review payloads can return app actors such as
copilot-pull-request-reviewer[bot]. The actor lookup was reaching the GitHub API correctly, butnormalize_github_login()rejected the bracketed bot suffix, so dashboard rows appeared asunknown author.