feat: Week 10 /pr command, GitHub App auth, and PR link comments - #38
Conversation
Ship contributor PR history in Discord, prefer GitcordApp installation tokens, and nudge unlinked authors on GitHub when they open a PR. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
github-actions
Bot
added
no-issue-linked
backend
configuration
docker
documentation
WalkthroughThe change adds GitHub App authentication with cached installation tokens, a ChangesGitHub authentication and client wiring
Contributor pull-request listing
PR-opened account-link notifications
Snapshot repository settings and write intervals
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Discord
participant Bot
participant GitHubRestAdapter
participant pr_list
Discord->>Bot: invoke /pr with count and skip
Bot->>GitHubRestAdapter: list_pull_requests_for_author
GitHubRestAdapter->>pr_list: return classified PRs
Bot->>pr_list: format recent results
pr_list->>Discord: send ephemeral messages
sequenceDiagram
participant Orchestrator
participant Notifications
participant GitHubPlanWriter
Orchestrator->>Notifications: process pr_opened event
Notifications->>GitHubPlanWriter: create_issue_comment
GitHubPlanWriter-->>Notifications: return success or failure
Notifications-->>Orchestrator: retain Discord notification flow
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ghdcbot/engine/snapshots.py (1)
152-164: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdvance the snapshots cursor only after a complete snapshot.
The surrounding guard enters this block when at least one file succeeds. If another file fails,
set_cursor("snapshots", now)records a successful timestamp for an incomplete snapshot. The next run can then skip the configured interval and delay recovery. Requirefiles_written == len(snapshot_data)before updating the cursor and treating the snapshot as successful. Add a test with one successful and one failed file write.Proposed fix
- if files_written > 0: + if files_written == len(snapshot_data) and files_written > 0:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghdcbot/engine/snapshots.py` around lines 152 - 164, Update the cursor advancement in the snapshots-writing flow so set_cursor("snapshots", now) and the corresponding success log run only when files_written equals len(snapshot_data), not merely when files_written is positive. Preserve partial-write handling without advancing the cursor, and add a test covering one successful and one failed file write.src/ghdcbot/adapters/github/rest.py (1)
204-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the log message constant and move
log_labelintoextra.The f-string makes the log message vary per call site, which prevents grouping in log aggregation. Ruff flags this as G004.
♻️ Proposed fix
self._logger.warning( - f"GitHub search for {log_label} failed", + "GitHub search failed", extra={ + "search_label": log_label, "status_code": response.status_code, "github_user": author, "org": self._org, }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghdcbot/adapters/github/rest.py` around lines 204 - 212, Update the warning call in the response-handling branch to use a constant message instead of interpolating log_label. Preserve the existing status_code, github_user, and org fields in extra, and add log_label there so the context remains available without varying the primary log message.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ghdcbot/adapters/github/app_auth.py`:
- Line 14: Replace typing.Callable with collections.abc.Callable in
src/ghdcbot/adapters/github/app_auth.py (line 14); in
src/ghdcbot/adapters/github/identity.py (line 5), move Callable and Iterable to
collections.abc; in src/ghdcbot/adapters/github/rest.py (line 9), move Callable,
Iterable, Iterator, and Sequence there; and in
src/ghdcbot/adapters/github/writer.py (line 4), move Callable and Iterable
there, preserving all other typing imports.
- Around line 25-35: Update the GitHubAppTokenProvider dataclass fields so
private_key_pem and _token use repr=False, preventing secrets from appearing in
generated representations; also set _token and _expires_at to init=False so
callers cannot provide cache values during construction, while preserving valid
dataclass field ordering.
- Around line 106-111: Update github_app_provider_from_env to detect when
exactly one of app_id or installation_id is set, log a warning describing the
incomplete GitHub App configuration, then return None as before. Do not warn
when both variables are absent or both are present, and preserve the existing
PAT fallback behavior.
- Around line 144-152: Update DynamicBearerAuth.auth_flow to catch failures from
self._get_token() and convert them into an httpx-compatible error, preserving
the original exception as the cause. Keep the existing Authorization header
assignment and request yield unchanged for successful token retrieval so
existing HTTP error handlers can handle token-mint failures.
- Around line 37-41: Protect the cache check and token minting in get_token with
a threading.Lock so concurrent worker threads serialize the full operation.
Acquire the lock before reading _token and _expires_at, keep the valid-token
return and _mint_installation_token call inside the critical section, and
release it safely on every path.
In `@src/ghdcbot/adapters/github/identity.py`:
- Around line 25-29: Update _raw_contains_code to fetch absolute raw gist URLs
with a separate unauthenticated HTTP client instead of self._client, while
continuing to use self._client for GitHub API requests. Ensure the client
created in __init__ with build_github_httpx_client and its installation-token
authentication are never used for raw gist content.
In `@src/ghdcbot/adapters/github/rest.py`:
- Around line 432-457: Update create_issue_comment to use the existing _request
path or equivalent rate-limit handling instead of calling self._client.post
directly, preserving its success and failure return behavior. Parse and log
GitHub rate-limit headers consistently with neighboring write methods such as
assign_issue, and classify a 403 response carrying Retry-After as retryable so
secondary-rate-limit backoff is applied.
- Around line 190-206: Update the GitHub search helper around the /search/issues
request to add allowlisted repositories and denylisted repositories as repo
qualifiers in the query before requesting results, while retaining the existing
local PR filter as a safety net. Ensure generated queries respect GitHub’s
256-character limit, and update the exact query assertion in
tests/test_open_prs_search.py to match the new scoped query.
In `@src/ghdcbot/bot.py`:
- Around line 633-641: Apply a per-user cooldown to the `/pr` command handler
containing the `list_pull_requests_for_author` flow, using the framework’s
`app_commands.checks.cooldown` mechanism. Configure an appropriate rate and
cooldown window for each user, and ensure cooldown violations are handled with
the command’s existing user-facing error response path without affecting other
commands.
- Around line 642-651: Replace the user-facing exception interpolation in both
the /pr handler and open_prs_cmd with a generic PR-fetch failure message, while
leaving logger.exception to capture the detailed exception internally. Ensure no
raw exc text is sent through interaction.followup.send, including ephemeral
responses.
In `@src/ghdcbot/cli.py`:
- Around line 64-71: Update the CLI initialization around build_orchestrator and
GitHubIdentityReader to reuse the orchestrator’s already resolved GitHub token
or shared token provider instead of calling resolve_github_token again. Ensure
app_auth.resolve_github_token uses a cached provider when needed, so only one
GitHubAppTokenProvider manages token minting and refresh for the installation.
In `@src/ghdcbot/engine/notifications.py`:
- Around line 341-357: Sanitize the author-controlled pr_title before
constructing header: escape Discord Markdown link delimiters and neutralize
mentions such as `@everyone`, then use the sanitized title in the linked PR
announcement. Add coverage for a title containing injected link syntax and
`@everyone`, while preserving normal title rendering and the existing author/link
behavior.
- Around line 268-293: The PR comment flow around _was_notification_sent and
_mark_notification_sent is not atomic, allowing concurrent syncs to create
duplicate comments. Replace the separate read-before-post sequence with a
storage-level atomic claim for dedupe_key, using an appropriate lease or
in-progress status; only the claimant may call create_issue_comment, release or
make the claim retryable when posting fails, and finalize the claim after
success. Add a concurrent test covering two syncs attempting the same
notification.
In `@tests/test_github_app_auth.py`:
- Around line 15-40: Add tests for GitHubAppTokenProvider.get_token covering
cached-token reuse and refresh at the skew boundary by setting _token and
_expires_at directly. Patch httpx.Client.post to test _mint_installation_token
parsing expires_at and its 3500-second fallback when parsing fails; also cover
RuntimeError when app credentials exist without a private key and verify
_load_private_key converts escaped newline sequences from the environment.
In `@tests/test_open_prs_search.py`:
- Around line 139-151: Extend the test coverage around
_search_pull_requests_for_author and list_open_pull_requests_for_author to
assert that returned rows include updated_at, while rows from
list_open_pull_requests_for_author do not include status. Keep the existing
request and status assertions unchanged.
- Line 145: Reverse the operands in the assertion within the open-PR search test
so params["q"] is compared against the expected query string, satisfying Ruff's
SIM300 rule while preserving the same assertion behavior.
---
Outside diff comments:
In `@src/ghdcbot/adapters/github/rest.py`:
- Around line 204-212: Update the warning call in the response-handling branch
to use a constant message instead of interpolating log_label. Preserve the
existing status_code, github_user, and org fields in extra, and add log_label
there so the context remains available without varying the primary log message.
In `@src/ghdcbot/engine/snapshots.py`:
- Around line 152-164: Update the cursor advancement in the snapshots-writing
flow so set_cursor("snapshots", now) and the corresponding success log run only
when files_written equals len(snapshot_data), not merely when files_written is
positive. Preserve partial-write handling without advancing the cursor, and add
a test covering one successful and one failed file write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa6b43ed-cc5f-4075-a607-42c33b40788e
⛔ Files ignored due to path filters (3)
public/gitcord-github-app-logo-dark.pngis excluded by!**/*.pngpublic/gitcord-github-app-logo-preview.pngis excluded by!**/*.pngpublic/gitcord-github-app-logo.pngis excluded by!**/*.png
📒 Files selected for processing (23)
.env.exampleINSTALLATION.mdREADME.mdTECHNICAL_DOCUMENTATION.mdconfig/aussie.yamldocker-compose.ymldocs/TESTING_DISCORD.mdpyproject.tomlsrc/ghdcbot/adapters/github/app_auth.pysrc/ghdcbot/adapters/github/identity.pysrc/ghdcbot/adapters/github/rest.pysrc/ghdcbot/adapters/github/writer.pysrc/ghdcbot/bot.pysrc/ghdcbot/cli.pysrc/ghdcbot/config/models.pysrc/ghdcbot/engine/notifications.pysrc/ghdcbot/engine/orchestrator.pysrc/ghdcbot/engine/pr_list.pysrc/ghdcbot/engine/snapshots.pytests/test_github_app_auth.pytests/test_notifications.pytests/test_open_prs_search.pytests/test_pr_list.py
…safety. Serialize token minting, reuse the shared provider, scope PR search queries, and make unverified-PR comment dedupe atomic with safer Discord/error handling. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Done coderabbit fixes . |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ghdcbot/engine/notifications.py (2)
190-224: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExtend the atomic claim to
send_pr_opened_channel_notification.
_claim_notification_sent/_release_notification_claim(428-456) close the duplicate-post race that a prior review flagged for GitHub comments.send_pr_opened_channel_notification(190-224) still uses the older pattern:_was_notification_sent(line 193) as a separate read, thensend_msg(line 210), then_mark_notification_sent(line 221) as a separate write.Two concurrent syncs can both pass the
_was_notification_sentcheck before either marks the key, so both can post duplicate PR-opened Discord channel messages. This is the same race class the atomic claim was built to close, just for the Discord-channel path instead of the GitHub-comment path.Migrate
send_pr_opened_channel_notificationto call_claim_notification_sentbeforesend_msg, and_release_notification_claimwhensend_msgreturns falsy or raises, mirroringsend_pr_opened_github_link_comment.Also applies to: 428-456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghdcbot/engine/notifications.py` around lines 190 - 224, Update send_pr_opened_channel_notification to replace the separate _was_notification_sent check with _claim_notification_sent before send_msg. When sending returns falsy or raises, call _release_notification_claim for the same dedupe key; preserve the existing successful notification handling and avoid separately calling _mark_notification_sent if the claim helper already records success, mirroring send_pr_opened_github_link_comment.
264-297: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winError-handle the atomic claim before posting the comment.
claim_notification_sent()performs anINSERT OR IGNOREinsidesqlite3.connect(...)fromsrc/ghdcbot/adapters/storage/sqlite.py, and_send_notifications_for_new_events()insrc/ghdcbot/engine/orchestrator.pydoes not catch failures fromsend_pr_opened_github_link_comment(). If the claim write raises, the exception propagates instead of returningFalse, so wrap_claim_notification_sent()in a try/except that logs and returnsFalse, then make the subsequentif not claimedpath returnFalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ghdcbot/engine/notifications.py` around lines 264 - 297, Wrap the _claim_notification_sent call in the PR GitHub link notification flow with exception handling that logs the failure and returns False. Store its result in a claimed variable, then make the subsequent not claimed check return False while preserving the existing dedupe key and posting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ghdcbot/adapters/github/rest.py`:
- Around line 489-491: Update create_issue_comment to avoid
_execute_request_with_retries for POST requests, using a single non-retrying
request while preserving the SQLite claim for uncertain outcomes. Add a
regression test covering a timeout after GitHub accepts the comment and verify
no duplicate /link comment is created.
In `@src/ghdcbot/engine/snapshots.py`:
- Around line 179-189: Update the snapshot-writing flow around write_file and
the files_written partial branch to avoid leaving incomplete per-run snapshot
directories when any file fails. Stage the full snapshot commit payload and
publish it atomically, or on partial failure remove the run’s written
files/directory and apply equivalent cleanup before leaving the cursor
unchanged; ensure retries do not accumulate incomplete directories.
In `@tests/test_notifications.py`:
- Around line 1225-1258: The concurrent claim test covers successful races but
not storage claim failures. Add a companion test near
test_pr_opened_github_link_comment_concurrent_claim that makes the storage claim
operation raise a transient or lock-related exception, then verify
send_pr_opened_github_link_comment handles it gracefully without propagating and
does not post the GitHub comment.
- Line 1196: Update the datetime construction in the affected test and the
adjacent occurrence to use datetime.now(datetime.UTC) instead of
datetime.now(timezone.utc), consistent with the project’s Python 3.11 minimum
target.
In `@tests/test_snapshots.py`:
- Around line 331-383: Strengthen
test_write_snapshots_partial_write_does_not_advance_cursor by having
PartialWriter record every write attempt separately from successful
files_written entries. Assert that at least two attempts occurred and exactly
one write succeeded, while retaining the assertion that the snapshots cursor was
not stored.
---
Outside diff comments:
In `@src/ghdcbot/engine/notifications.py`:
- Around line 190-224: Update send_pr_opened_channel_notification to replace the
separate _was_notification_sent check with _claim_notification_sent before
send_msg. When sending returns falsy or raises, call _release_notification_claim
for the same dedupe key; preserve the existing successful notification handling
and avoid separately calling _mark_notification_sent if the claim helper already
records success, mirroring send_pr_opened_github_link_comment.
- Around line 264-297: Wrap the _claim_notification_sent call in the PR GitHub
link notification flow with exception handling that logs the failure and returns
False. Store its result in a claimed variable, then make the subsequent not
claimed check return False while preserving the existing dedupe key and posting
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 535883e0-56d7-4857-b180-8d5b71a7f18f
📒 Files selected for processing (13)
src/ghdcbot/adapters/github/app_auth.pysrc/ghdcbot/adapters/github/identity.pysrc/ghdcbot/adapters/github/rest.pysrc/ghdcbot/adapters/github/writer.pysrc/ghdcbot/adapters/storage/sqlite.pysrc/ghdcbot/bot.pysrc/ghdcbot/cli.pysrc/ghdcbot/engine/notifications.pysrc/ghdcbot/engine/snapshots.pytests/test_github_app_auth.pytests/test_notifications.pytests/test_open_prs_search.pytests/test_snapshots.py
| response = self._execute_request_with_retries( | ||
| "POST", path, params={}, json_body={"body": body} | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not automatically retry issue-comment POST requests.
_execute_request_with_retries retries timeouts and transient responses. GitHub can persist the comment before an ambiguous failure reaches this client. A retry can then create duplicate /link comments.
The SQLite claim prevents concurrent senders. It does not identify a completed external write. Disable ambiguous retries for create_issue_comment. Retain the claim for an uncertain outcome, or reconcile an existing bot comment before a later retry. Add a regression test for a timeout after server acceptance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ghdcbot/adapters/github/rest.py` around lines 489 - 491, Update
create_issue_comment to avoid _execute_request_with_retries for POST requests,
using a single non-retrying request while preserving the SQLite claim for
uncertain outcomes. Add a regression test covering a timeout after GitHub
accepts the comment and verify no duplicate /link comment is created.
| elif files_written > 0: | ||
| logger.warning( | ||
| "GitHub snapshots partially written; cursor not advanced", | ||
| extra={ | ||
| "org": config.github.org, | ||
| "repo": f"{owner}/{repo}", | ||
| "snapshot_dir": snapshot_dir, | ||
| "files_written": files_written, | ||
| "files_expected": len(snapshot_data), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 -P 'def write_file\s*\(|write_file\(|snapshot_dir|snapshots/' \
src/ghdcbot/adapters/github src/ghdcbot/engine testsRepository: AOSSIE-Org/Gitcord-GithubDiscordBot
Length of output: 12299
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== snapshots relevant implementation =="
sed -n '110,210p;360,395p' src/ghdcbot/engine/snapshots.py
echo
echo "== github rest write_file implementation =="
sed -n '560,640p;680,730p' src/ghdcbot/adapters/github/rest.py
echo
echo "== snapshot read/list references =="
rg -n -C 4 'read_snapshot|load_snapshot|snapshot_data|snapshot_dir|directories?|glob|listdir|README|snapshots' src/ghdcbot/tests src/ghdcbot/docs README.md docs 2>/dev/null || true
echo
echo "== test partial behavior =="
sed -n '332,370p' tests/test_snapshots.pyRepository: AOSSIE-Org/Gitcord-GithubDiscordBot
Length of output: 12097
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== complete GitHub write_file implementation with response handling =="
sed -n '591,680p' src/ghdcbot/adapters/github/rest.py
echo
echo "== snapshot config and full test partial expected assertions =="
rg -n -C 3 'SnapshotConfig|cursor|files_written|expected|read_.*snapshot|SnapshotReader|metadata' src tests 2>/dev/null || true
sed -n '332,380p' tests/test_snapshots.pyRepository: AOSSIE-Org/Gitcord-GithubDiscordBot
Length of output: 32571
🌐 Web query:
GitHub Contents API update file commit message default branch behavior current SHA
💡 Result:
The GitHub REST API endpoint for creating or updating file contents is PUT /repos/{owner}/{repo}/contents/{path} [1][2]. Key requirements and behaviors for updating a file are as follows: - Commit Message: The message parameter is required for all file creation and update requests [1][2]. - Branch Behavior: The branch parameter allows you to specify the target branch for the update [1][2]. If omitted, the request defaults to the repository’s default branch [1][2]. - SHA Requirement: When updating an existing file, the sha parameter is required [1][2]. This must be the blob SHA of the file being replaced [1][2]. If this SHA is omitted during an update request, the API will return a 422 Unprocessable Entity error because it cannot verify the state of the file being modified [3]. You can obtain this SHA by first performing a GET request on the file path [4].
Citations:
- 1: https://docs.github.com/en/rest/repos/contents?apiVersion=2026-03-10
- 2: https://docs.github.com/en/rest/repos/contents
- 3: github_repository_file cant update/overwrite existing files. integrations/terraform-provider-github#438
- 4: https://stackoverflow.com/questions/71054940/update-github-files-through-the-rest-api
🌐 Web query:
"PUT /repos/{owner}/{repo}/contents/{path}" "if you supply a sha, commit will be applied to the given branch" GitHub
💡 Result:
When using the GitHub REST API endpoint PUT /repos/{owner}/{repo}/contents/{path} to update an existing file, the sha parameter is required [1][2]. This sha must correspond to the blob SHA of the specific file version you are replacing [1][2]. The purpose of this parameter is to ensure that the update is applied correctly to the desired file state and to prevent conflicts (e.g., if the file was modified by another user in the interim) [1][2]. When you provide the correct blob sha, the new content is committed to the specified branch (or the repository's default branch if no branch is specified) [1][2]. If you are creating a new file, the sha parameter should be omitted [1][2]. For updating existing files, you can typically retrieve the required blob sha by first performing a GET request on the file path or by using the Git Trees API to inspect the repository's file structure [3].
Citations:
- 1: https://docs.github.com/en/rest/repos/contents
- 2: https://docs.github.com/en/rest/repos/contents?apiVersion=2026-03-10
- 3: https://stackoverflow.com/questions/26203603/how-do-i-get-the-sha-parameter-from-github-api-without-downloading-the-whole-f
🌐 Web query:
"PUT /repos/{owner}/{repo}/contents/{path}" "creating a file, the branch parameter is used to find the tree" GitHub
💡 Result:
When creating or updating a file using the GitHub REST API endpoint PUT /repos/{owner}/{repo}/contents/{path}, the branch parameter is indeed used to specify the branch where the commit should be made [1][2]. By including the branch parameter in your request body, you instruct GitHub to apply the changes to that specific branch [1][3]. If the branch parameter is omitted, the API defaults to the repository's default branch [1][2]. This parameter functions as a high-level instruction for the repository contents API. Behind the scenes, GitHub handles the necessary Git operations—such as identifying the current tree of that branch, creating the new file blob, generating a new tree, and creating a commit—to ensure the file is correctly updated on the requested branch [3][4]. This differs from the lower-level Git Database API (found under /repos/{owner}/{repo}/git/...), where you would manually need to fetch the reference, retrieve the tree, construct new objects, and update the branch reference yourself [4].
Citations:
- 1: https://docs.github.com/en/rest/repos/contents
- 2: https://docs.github.com/en/rest/repos/contents?apiVersion=2026-03-10
- 3: https://stackoverflow.com/questions/57809917/publish-in-a-repo-to-a-specific-branch-with-github-api
- 4: https://docs.github.com/en/rest/guides/using-the-rest-api-to-interact-with-your-git-database?apiVersion=2026-03-10
Bundle partial snapshot writes into one atomic commit.
write_file writes each snapshot file with a single PUT and returns on failure. A later file in snapshot_data can fail while earlier files remain in a per-run snapshot_dir, but the partial case only logs counts and leaves the cursor unchanged. Repeated runs use a new run_id, so retries leave additional incomplete directories. Collect the commit payload and write it atomically, or add the same atomicity/cleanup logic as the partial branch leaves in GitHub.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ghdcbot/engine/snapshots.py` around lines 179 - 189, Update the
snapshot-writing flow around write_file and the files_written partial branch to
avoid leaving incomplete per-run snapshot directories when any file fails. Stage
the full snapshot commit payload and publish it atomically, or on partial
failure remove the run’s written files/directory and apply equivalent cleanup
before leaving the cursor unchanged; ensure retries do not accumulate incomplete
directories.
| github_user="alice", | ||
| event_type="pr_opened", | ||
| repo="repo", | ||
| created_at=datetime.now(timezone.utc), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'target-version|requires-python' pyproject.tomlRepository: AOSSIE-Org/Gitcord-GithubDiscordBot
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1188,1218p' tests/test_notifications.pyRepository: AOSSIE-Org/Gitcord-GithubDiscordBot
Length of output: 1127
Use datetime.UTC at the supported Python target.
pyproject.toml requires Python >=3.11, so replace datetime.now(timezone.utc) with datetime.now(datetime.UTC) here and the adjacent occurrence.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 1196-1196: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_notifications.py` at line 1196, Update the datetime construction
in the affected test and the adjacent occurrence to use
datetime.now(datetime.UTC) instead of datetime.now(timezone.utc), consistent
with the project’s Python 3.11 minimum target.
Source: Linters/SAST tools
| def test_pr_opened_github_link_comment_concurrent_claim(tmp_path) -> None: | ||
| storage = SqliteStorage(str(tmp_path)) | ||
| storage.init_schema() | ||
| github_writer = MockGithubWriter() | ||
| config = NotificationConfig(enabled=True, pr_opened_github_comment=True) | ||
| policy = MutationPolicy(mode=RunMode.ACTIVE, github_write_allowed=True, discord_write_allowed=True) | ||
| event = _pr_opened_event(github_user="stranger") | ||
| barrier = Barrier(2) | ||
| results: list[bool] = [] | ||
|
|
||
| def _run() -> None: | ||
| barrier.wait() | ||
| results.append( | ||
| send_pr_opened_github_link_comment( | ||
| event, | ||
| storage, | ||
| github_writer, | ||
| policy, | ||
| config, | ||
| "AOSSIE-Org", | ||
| "https://discord.gg/hjUhu33uAn", | ||
| ) | ||
| ) | ||
|
|
||
| threads = [Thread(target=_run) for _ in range(2)] | ||
| for thread in threads: | ||
| thread.start() | ||
| for thread in threads: | ||
| thread.join() | ||
|
|
||
| assert sorted(results) == [False, True] | ||
| assert len(github_writer.comments) == 1 | ||
| assert storage.was_notification_sent("pr_opened_github_link:Gitcord-GithubDiscordBot:42") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Good coverage for the atomic claim race.
This test confirms only one of two concurrent callers posts the GitHub comment and only one dedupe row persists. This directly addresses the prior review request for concurrent-claim coverage.
Consider adding a companion test that forces the storage claim call to raise (simulating lock contention or a transient DB error) to confirm the caller degrades gracefully instead of propagating an unhandled exception, tied to the error-handling gap noted in src/ghdcbot/engine/notifications.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_notifications.py` around lines 1225 - 1258, The concurrent claim
test covers successful races but not storage claim failures. Add a companion
test near test_pr_opened_github_link_comment_concurrent_claim that makes the
storage claim operation raise a transient or lock-related exception, then verify
send_pr_opened_github_link_comment handles it gracefully without propagating and
does not post the GitHub comment.
|
|
||
|
|
||
| def test_write_snapshots_partial_write_does_not_advance_cursor() -> None: | ||
| """Cursor advances only when every snapshot file write succeeds.""" | ||
|
|
||
| class PartialWriter(MockGitHubWriter): | ||
| def write_file(self, owner: str, repo: str, file_path: str, content: str, commit_message: str, branch: str | None = None) -> bool: | ||
| # Fail the second write attempt. | ||
| if len(self.files_written) >= 1: | ||
| return False | ||
| return super().write_file(owner, repo, file_path, content, commit_message, branch) | ||
|
|
||
| class CursorStorage(MockStorage): | ||
| def __init__(self) -> None: | ||
| super().__init__() | ||
| self.cursors: dict[str, datetime] = {} | ||
|
|
||
| def set_cursor(self, source: str, cursor: datetime) -> None: | ||
| self.cursors[source] = cursor | ||
|
|
||
| storage = CursorStorage() | ||
| config = BotConfig( | ||
| runtime=RuntimeConfig( | ||
| mode=RunMode.DRY_RUN, | ||
| log_level="INFO", | ||
| data_dir="/tmp/test", | ||
| github_adapter="test", | ||
| discord_adapter="test", | ||
| storage_adapter="test", | ||
| activity_period_days=30, | ||
| ), | ||
| github=GitHubConfig(org="test-org", token="test", api_base="https://api.github.com", permissions=PermissionConfig()), | ||
| discord=DiscordConfig(guild_id="123", token="test", permissions=PermissionConfig()), | ||
| assignments=AssignmentConfig(), | ||
| snapshots=SnapshotConfig(enabled=True, repo_path="org/repo"), | ||
| ) | ||
| github_writer = PartialWriter() | ||
|
|
||
| write_snapshots_to_github( | ||
| storage=storage, | ||
| config=config, | ||
| github_writer=github_writer, | ||
| identity_mappings=[ | ||
| IdentityMapping(discord_user_id="123", github_user="alice"), | ||
| ], | ||
| scores=[], | ||
| member_roles={}, | ||
| period_start=datetime(2024, 1, 1, tzinfo=timezone.utc), | ||
| period_end=datetime(2024, 1, 31, tzinfo=timezone.utc), | ||
| ) | ||
|
|
||
| assert len(github_writer.files_written) >= 1 | ||
| assert "snapshots" not in storage.cursors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record the failed write attempt in this regression test.
PartialWriter uses len(self.files_written) to fail after the first successful write, but it does not record failed attempts. assert len(github_writer.files_written) >= 1 proves only that one file succeeded. It does not prove that the second file was attempted.
Track attempts and assert at least two attempts and exactly one successful write.
Proposed test strengthening
class PartialWriter(MockGitHubWriter):
+ def __init__(self) -> None:
+ super().__init__()
+ self.attempts = 0
+
def write_file(self, owner: str, repo: str, file_path: str, content: str, commit_message: str, branch: str | None = None) -> bool:
+ self.attempts += 1
# Fail the second write attempt.
if len(self.files_written) >= 1:
return False
return super().write_file(owner, repo, file_path, content, commit_message, branch)
...
- assert len(github_writer.files_written) >= 1
+ assert github_writer.attempts >= 2
+ assert len(github_writer.files_written) == 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_write_snapshots_partial_write_does_not_advance_cursor() -> None: | |
| """Cursor advances only when every snapshot file write succeeds.""" | |
| class PartialWriter(MockGitHubWriter): | |
| def write_file(self, owner: str, repo: str, file_path: str, content: str, commit_message: str, branch: str | None = None) -> bool: | |
| # Fail the second write attempt. | |
| if len(self.files_written) >= 1: | |
| return False | |
| return super().write_file(owner, repo, file_path, content, commit_message, branch) | |
| class CursorStorage(MockStorage): | |
| def __init__(self) -> None: | |
| super().__init__() | |
| self.cursors: dict[str, datetime] = {} | |
| def set_cursor(self, source: str, cursor: datetime) -> None: | |
| self.cursors[source] = cursor | |
| storage = CursorStorage() | |
| config = BotConfig( | |
| runtime=RuntimeConfig( | |
| mode=RunMode.DRY_RUN, | |
| log_level="INFO", | |
| data_dir="/tmp/test", | |
| github_adapter="test", | |
| discord_adapter="test", | |
| storage_adapter="test", | |
| activity_period_days=30, | |
| ), | |
| github=GitHubConfig(org="test-org", token="test", api_base="https://api.github.com", permissions=PermissionConfig()), | |
| discord=DiscordConfig(guild_id="123", token="test", permissions=PermissionConfig()), | |
| assignments=AssignmentConfig(), | |
| snapshots=SnapshotConfig(enabled=True, repo_path="org/repo"), | |
| ) | |
| github_writer = PartialWriter() | |
| write_snapshots_to_github( | |
| storage=storage, | |
| config=config, | |
| github_writer=github_writer, | |
| identity_mappings=[ | |
| IdentityMapping(discord_user_id="123", github_user="alice"), | |
| ], | |
| scores=[], | |
| member_roles={}, | |
| period_start=datetime(2024, 1, 1, tzinfo=timezone.utc), | |
| period_end=datetime(2024, 1, 31, tzinfo=timezone.utc), | |
| ) | |
| assert len(github_writer.files_written) >= 1 | |
| assert "snapshots" not in storage.cursors | |
| def test_write_snapshots_partial_write_does_not_advance_cursor() -> None: | |
| """Cursor advances only when every snapshot file write succeeds.""" | |
| class PartialWriter(MockGitHubWriter): | |
| def __init__(self) -> None: | |
| super().__init__() | |
| self.attempts = 0 | |
| def write_file(self, owner: str, repo: str, file_path: str, content: str, commit_message: str, branch: str | None = None) -> bool: | |
| self.attempts += 1 | |
| # Fail the second write attempt. | |
| if len(self.files_written) >= 1: | |
| return False | |
| return super().write_file(owner, repo, file_path, content, commit_message, branch) | |
| class CursorStorage(MockStorage): | |
| def __init__(self) -> None: | |
| super().__init__() | |
| self.cursors: dict[str, datetime] = {} | |
| def set_cursor(self, source: str, cursor: datetime) -> None: | |
| self.cursors[source] = cursor | |
| storage = CursorStorage() | |
| config = BotConfig( | |
| runtime=RuntimeConfig( | |
| mode=RunMode.DRY_RUN, | |
| log_level="INFO", | |
| data_dir="/tmp/test", | |
| github_adapter="test", | |
| discord_adapter="test", | |
| storage_adapter="test", | |
| activity_period_days=30, | |
| ), | |
| github=GitHubConfig(org="test-org", token="test", api_base="https://api.github.com", permissions=PermissionConfig()), | |
| discord=DiscordConfig(guild_id="123", token="test", permissions=PermissionConfig()), | |
| assignments=AssignmentConfig(), | |
| snapshots=SnapshotConfig(enabled=True, repo_path="org/repo"), | |
| ) | |
| github_writer = PartialWriter() | |
| write_snapshots_to_github( | |
| storage=storage, | |
| config=config, | |
| github_writer=github_writer, | |
| identity_mappings=[ | |
| IdentityMapping(discord_user_id="123", github_user="alice"), | |
| ], | |
| scores=[], | |
| member_roles={}, | |
| period_start=datetime(2024, 1, 1, tzinfo=timezone.utc), | |
| period_end=datetime(2024, 1, 31, tzinfo=timezone.utc), | |
| ) | |
| assert github_writer.attempts >= 2 | |
| assert len(github_writer.files_written) == 1 | |
| assert "snapshots" not in storage.cursors |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 355-355: Do not hardcode temporary file or directory names
Context: "/tmp/test"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.16.0)
[error] 356-356: Probable insecure usage of temporary file or directory: "/tmp/test"
(S108)
[error] 362-362: Possible hardcoded password assigned to argument: "token"
(S106)
[error] 363-363: Possible hardcoded password assigned to argument: "token"
(S106)
[warning] 378-378: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 379-379: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_snapshots.py` around lines 331 - 383, Strengthen
test_write_snapshots_partial_write_does_not_advance_cursor by having
PartialWriter record every write attempt separately from successful
files_written entries. Assert that at least two attempts occurred and exactly
one write succeeded, while retaining the assertion that the snapshots cursor was
not stored.
fix: unbreak Python Tests after PR #38 json= client.request
fix: CodeRabbit follow-ups for PR #38 notification/snapshot safety
PR AOSSIE-Org#38 started passing json= to client.request(); outdated mocks rejected it and broke Python Tests on main (23 failures). Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
/prDiscord command to list a verified contributor’s PRs (closed/merged/open) with count/pagination and multi-message formattingGitcordApp) for API writes/snapshots so actions appear asgitcordapp[bot]/linksteps)Test plan
pytest tests/test_pr_list.py tests/test_github_app_auth.py tests/test_notifications.py tests/test_open_prs_search.py/prfor a verified member in DiscordMade with Cursor
Summary by CodeRabbit
New Features
/prDiscord command to list recent contributor pull requests by status, with optional count and skip filters.Documentation
Bug Fixes