extend Storage Protocol to cover all SqliteStorage methods#13
extend Storage Protocol to cover all SqliteStorage methods#13sujalgoel wants to merge 9 commits intoAOSSIE-Org:mainfrom
Conversation
github-actions
bot
added
no-issue-linked
backend
python
size/M
first-time-contributor
labels
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughStorage protocol extended with identity-linking, issue-request persistence, audit logging, and notification deduplication APIs; Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as SnapshotWriter
participant Storage as SnapshotStorage
participant GitHub as GitHubRepo
participant FS as Filesystem
Engine->>Storage: list_contribution_summaries(...)
Engine->>Storage: list_pending_issue_requests()
Engine->>Storage: list_recent_notifications(limit=1000)
Engine->>GitHub: write snapshot files
Engine->>FS: write local snapshot files
Engine->>Storage: append_audit_event({event_type: "snapshot_written", ...})
GitHub-->>Engine: write result/ack
Storage-->>Engine: lists returned / audit recorded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/core/interfaces.py`:
- Around line 148-156: The parameter event in the interface method
mark_notification_sent currently uses Any which removes type safety; update the
signature to a more specific type (e.g., a Union of known event types like
ContributionEvent | OtherEvent, or a TypedDict/dataclass/Protocol that matches
the event shape) and update any related imports and type annotations so callers
and implementations use that concrete type instead of Any; ensure the new type
name (e.g., ContributionEvent or EventProtocol) is referenced in
mark_notification_sent and exported from this module so static analysis and
downstream implementations pick up the stronger typing.
- Around line 4-6: Update the import statement to use collections.abc for
Iterable and Sequence: replace "from typing import Any, Iterable, Protocol,
Sequence" with imports that keep Any and Protocol from typing but import
Iterable and Sequence from collections.abc so the module uses the modern PEP 585
sources; adjust the import line(s) around the symbols Iterable, Sequence, Any,
and Protocol in interfaces.py accordingly.
- Around line 132-133: The update_issue_request_status method's status parameter
should be annotated with a Literal of the allowed values to enable static
checking: change the signature of update_issue_request_status(self, request_id:
str, status: Literal["pending","approved","rejected","cancelled"]) -> None and
add the appropriate import for Literal (from typing or typing_extensions as
needed); also update any implementing classes or overrides to use the same
Literal annotation so type checkers remain consistent with the documented valid
statuses.
- Around line 137-141: Update the docstrings in
src/ghdcbot/adapters/storage/sqlite.py for the methods that are now part of the
Storage protocol by removing the outdated phrase "Optional method; not part of
the Storage protocol" and replacing it with a concise description matching the
Storage interface: update get_identity_links_for_discord_user(),
append_audit_event(), list_audit_events(), and list_recent_notifications()
docstrings to simply describe what each method returns or does (e.g., parameters
and return type semantics) so they reflect that these methods are part of the
Storage protocol.
In `@src/ghdcbot/engine/snapshots.py`:
- Around line 255-267: Replace the explicit loop that builds issue_requests_data
from pending_requests with a single list comprehension: iterate over
storage.list_pending_issue_requests() (or the existing pending_requests
variable) and construct the same dict for each req using req.get("...") for the
keys
"request_id","discord_user_id","github_user","owner","repo","issue_number","issue_url","created_at","status";
update the reference to issue_requests_data to be assigned the comprehension
result instead of repeatedly calling append in the for-loop.
- Around line 279-290: Replace the explicit for-loop that builds
notifications_data from recent_notifications with a single list comprehension to
make the code more idiomatic: use notifications_data = [ { "dedupe_key":
notif.get("dedupe_key"), "event_type": notif.get("event_type"), "github_user":
notif.get("github_user"), "discord_user_id": notif.get("discord_user_id"),
"repo": notif.get("repo"), "target": notif.get("target"), "channel_id":
notif.get("channel_id"), "sent_at": notif.get("sent_at") } for notif in
storage.list_recent_notifications(limit=1000) ] so the mapping logic in
snapshots.py (the recent_notifications variable and notifications_data list)
remains identical but implemented as a comprehension.
🪄 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
Run ID: 4f99d523-7fbe-44b0-8999-baeda92e5850
📒 Files selected for processing (2)
src/ghdcbot/core/interfaces.pysrc/ghdcbot/engine/snapshots.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ghdcbot/adapters/storage/sqlite.py (1)
666-666:⚠️ Potential issue | 🟡 MinorFile handle not closed; potential resource leak.
The file is opened without a context manager, so the handle isn't guaranteed to close promptly. Under high-frequency audit logging, this may exhaust file descriptors.
🛡️ Proposed fix using context manager
- path.open("a", encoding="utf-8").write(line) + with path.open("a", encoding="utf-8") as f: + f.write(line)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ghdcbot/adapters/storage/sqlite.py` at line 666, Replace the unguarded call path.open("a", encoding="utf-8").write(line) with a context-managed open to ensure the file handle is always closed; locate the code that calls path.open(...) and change it to use a with statement (e.g., with path.open("a", encoding="utf-8") as f: f.write(line)) so that writes still append but the file descriptor is released promptly.tests/test_snapshots.py (1)
343-344: 🧹 Nitpick | 🔵 TrivialConsider asserting no audit events on error path.
The test verifies no files are written on error, but doesn't verify that no audit event was recorded. Adding this assertion would strengthen coverage of the error handling path.
♻️ Proposed additional assertion
# Should not have written files due to error assert len(github_writer.files_written) == 0 + + # Should not have recorded audit event on error + assert len(storage.audit_events) == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_snapshots.py` around lines 343 - 344, The test currently asserts github_writer.files_written == 0 on the error path but doesn't assert audit activity; add an assertion immediately after the existing check to verify that the in-test audit collector is empty (for example assert audit_logger.events == [] or assert audit_writer.events == [] or assert audit_events == [] depending on the test fixture used) so the error path records no audit events; locate the audit fixture used in this test file (paired with github_writer) and use its canonical name in the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ghdcbot/adapters/storage/sqlite.py`:
- Line 666: Replace the unguarded call path.open("a",
encoding="utf-8").write(line) with a context-managed open to ensure the file
handle is always closed; locate the code that calls path.open(...) and change it
to use a with statement (e.g., with path.open("a", encoding="utf-8") as f:
f.write(line)) so that writes still append but the file descriptor is released
promptly.
In `@tests/test_snapshots.py`:
- Around line 343-344: The test currently asserts github_writer.files_written ==
0 on the error path but doesn't assert audit activity; add an assertion
immediately after the existing check to verify that the in-test audit collector
is empty (for example assert audit_logger.events == [] or assert
audit_writer.events == [] or assert audit_events == [] depending on the test
fixture used) so the error path records no audit events; locate the audit
fixture used in this test file (paired with github_writer) and use its canonical
name in the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 424e703c-f1bc-4f60-a75e-fa1a887ed1f6
📒 Files selected for processing (3)
src/ghdcbot/adapters/storage/sqlite.pysrc/ghdcbot/engine/snapshots.pytests/test_snapshots.py
|
Addressed all CodeRabbit feedback: fixed |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@shubham5080 would love a review on this when you get a chance! |
|
@sujalgoel thanx for the PR as i get time i will check it . |
|
please resolve the coderabbit issues first |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/core/interfaces.py`:
- Around line 93-163: The protocol methods use bare dict/list[dict] which
prevents static checking of required keys; define TypedDicts (e.g.,
IdentityLinkDict, IdentityStatusDict, IssueRequestDict, AuditEventDict,
NotificationRecordDict) and replace usages in signatures such as
get_identity_link, unlink_identity, get_identity_links_for_discord_user,
get_identity_status, list_pending_issue_requests, get_issue_request,
list_audit_events, append_audit_event,
was_notification_sent/mark_notification_sent, and list_recent_notifications with
the appropriate TypedDict or list[TypedDict]; add imports from typing for
TypedDict/NotRequired if needed and update adapters/mocks to construct and
return those TypedDict types so callers can be statically validated.
In `@tests/test_snapshots.py`:
- Around line 30-44: Introduce a focused SnapshotStorage Protocol that declares
the three methods append_audit_event(event: dict) -> None,
list_pending_issue_requests() -> list[dict], and
list_recent_notifications(limit: int = 1000) -> list[dict]; import Protocol (and
typing primitives) and define this protocol, then change the type annotations on
the snapshot-related functions in snapshots.py to accept SnapshotStorage instead
of the broader Storage type so the MockStorage class (which implements
append_audit_event, list_pending_issue_requests, list_recent_notifications)
satisfies the contract and static type checkers can validate the test double.
🪄 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
Run ID: 77c8f217-f74a-44ee-bb05-9d362539009a
📒 Files selected for processing (3)
src/ghdcbot/adapters/storage/sqlite.pysrc/ghdcbot/core/interfaces.pytests/test_snapshots.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/core/interfaces.py`:
- Around line 54-57: The AuditEventDict currently uses a permissive dict for the
context field; define a more specific TypedDict (e.g., AuditEventContext with
the known keys like org, repo, timestamp or a Union of specific context
TypedDicts if shapes vary) and replace context: dict with context:
AuditEventContext (or the appropriate Union) in AuditEventDict to improve type
safety; if event contexts are intentionally heterogeneous, document that
decision and keep dict.
- Around line 28-32: UnlinkResultDict is missing two fields that
SqliteStorage.unlink_identity() returns; update the TypedDict to include
cooldown_until (use Optional[str] since it may be null) and cooldown_hours (int)
so callers get correct type information for those returned values and to keep
UnlinkResultDict aligned with SqliteStorage.unlink_identity().
- Around line 17-25: The TypedDict IdentityLinkDict is missing the
github_user_normalized key that SqliteStorage.get_identity_link() returns, so
add github_user_normalized: str to the IdentityLinkDict definition (it can be
optional since the TypedDict is total=False) to restore type safety for callers
that access the normalized GitHub username; update the IdentityLinkDict class
declaration to include the github_user_normalized key with type str.
🪄 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
Run ID: 525c042c-f01f-4077-a69f-939c8dd074ce
📒 Files selected for processing (2)
src/ghdcbot/core/interfaces.pysrc/ghdcbot/engine/snapshots.py
…entContext TypedDict
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/core/interfaces.py`:
- Line 7: You are importing IdentityMapping from ghdcbot.config.models into the
core protocol, which couples core to the config layer; instead either move the
IdentityMapping definition into ghdcbot.core.models (and update all imports to
import from the new core module) or replace the config model here with a
lightweight core-local type (e.g., a dataclass or TypedDict) declared in
src/ghdcbot/core/models or within core/interfaces.py, then change all references
and type annotations in functions/classes that use IdentityMapping (including
the usages noted around lines 175-176) to point at the new core-local type so
the core package no longer depends on ghdcbot.config.
- Around line 48-58: The IssueRequestDict TypedDict currently types "status" as
plain str, losing the same constrained status type enforced by
update_issue_request_status(); change IssueRequestDict["status"] to reuse the
existing constrained status alias/type (e.g., IssueRequestStatus or the Literal
union used elsewhere) so callers of get_issue_request() and
list_pending_issue_requests() get the tightened type; apply the same change to
the other TypedDicts referenced around the 208-213 range and then rerun the type
checker to surface remaining mismatches.
🪄 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
Run ID: 765f602b-f8f8-4dc0-911d-458d72bb9bdf
📒 Files selected for processing (1)
src/ghdcbot/core/interfaces.py
|
@coderabbit review |
|
✅ Actions performedReview triggered.
|
|
please leave a review on this when you get a chance! @shubham5080 |
|
Could you check that why the workflow is causing error ? |
Tracked it down! The |
|
@sujalgoel Small request before merge: IdentityMapping exists twice (Pydantic in config/models.py and the new dataclass in core/models.py). Can you use one type everywhere—e.g. drop the dataclass and import IdentityMapping from config.models in the protocol/SQLite? Thanks. |
|
@shubham5080 Worth flagging before I change it: the dataclass in |
2 participants
Storageincore/interfaces.pyonly declared 8 methods.SqliteStorageimplements 30+. The gap forced callers insnapshots.pyto usegetattr()checks for identity, issue request, audit, and notification methods.What changed
core/interfaces.pyStorage:create_identity_claim,get_identity_link,mark_identity_verified,unlink_identity,list_verified_identity_mappings,get_identity_links_for_discord_user,get_identity_statusinsert_issue_request,list_pending_issue_requests,get_issue_request,update_issue_request_statusappend_audit_event,list_audit_eventswas_notification_sent,mark_notification_sent,list_recent_notificationsdifficulty_weightsparam tolist_contribution_summaries(matches actualSqliteStoragesignature)engine/snapshots.pygetattr(storage, ...)workarounds with direct callsstorage: Any->storage: Storagein all three snapshot functionsWhy
This gives
GitHubStorageAdapter(Phase 3) a complete interface to implement against, and means type checkers will catch any incomplete implementation.How it was tested
pytest— 201 passed, 0 failed.Summary by CodeRabbit
New Features
Tests