Add provider-independent external comic identities - #1038
Conversation
JoshCLWren
added
factory
ChatGPT Codex Connector
Factory resume packetHead: |
JoshCLWren
added
factory:building
ChatGPT Codex Connector
|
Warning Review limit reached
Next review available in: 36 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds provider-independent external identity persistence for issues and thread series. The change includes database migrations, SQLAlchemy models, public exports, normalized identity upserts, validated mappings, ownership checks, idempotent updates, documentation, and contract tests. ChangesExternal identity mapping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExternalIdentityService
participant Database
Caller->>ExternalIdentityService: Upsert or link external identity
ExternalIdentityService->>Database: Validate ownership and entity type
ExternalIdentityService->>Database: Create or update identity mapping
Database-->>Caller: Return persisted mapping
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
JoshCLWren
added
factory:2
ChatGPT Codex Connector
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/models/external_identity.py (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the SQLAlchemy import.
Line 7 exceeds the 100-character line limit. Split this import into a parenthesized multi-line import.
As per coding guidelines, format Python code with a 100-character line length.
🤖 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 `@app/models/external_identity.py` at line 7, Reformat the SQLAlchemy import statement by wrapping its imported names in a parenthesized multi-line import, keeping the same imports and complying with the 100-character line limit.Source: Coding guidelines
app/external_identities.py (1)
183-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the public-function docstrings.
These public functions omit required Google-style documentation sections.
app/external_identities.py#L183-L187: addArgsandReturnssections forlink_thread_external_series.alembic/versions/c84500000001_add_external_identities.py#L36-L37: document the no-argument andNonereturn contract forupgrade.alembic/versions/c84500000001_add_external_identities.py#L113-L114: document the no-argument andNonereturn contract fordowngrade.As per coding guidelines, public functions must use Google-style docstrings, including
ArgsandReturnssections.🤖 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 `@app/external_identities.py` around lines 183 - 187, Complete the Google-style docstrings for public functions: in app/external_identities.py lines 183-187, update link_thread_external_series with accurate Args and Returns sections; in alembic/versions/c84500000001_add_external_identities.py lines 36-37, document upgrade as taking no arguments and returning None; and at lines 113-114, document downgrade with the same no-argument and None-return contract.Source: Coding guidelines
🤖 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 `@app/external_identities.py`:
- Around line 119-149: Update the ownership lookup in the confirmation flow
around the owned_issue query to lock the owned Issue row within the current
transaction before evaluating conflicts. Ensure confirmations for the same issue
serialize while preserving the existing provider conflict check and validation
behavior.
- Around line 58-86: Make the select-then-insert flows atomic for
ExternalIdentity at app/external_identities.py:58-86,
IssueExternalIdentityMapping at app/external_identities.py:151-169, and
ThreadExternalSeriesMapping at app/external_identities.py:199-216. Replace each
race-prone creation path with database-level conflict handling, such as INSERT
... ON CONFLICT, or use a savepoint to recover from a unique-constraint conflict
and reload the existing row; preserve the current update behavior for rows that
already exist.
---
Nitpick comments:
In `@app/external_identities.py`:
- Around line 183-187: Complete the Google-style docstrings for public
functions: in app/external_identities.py lines 183-187, update
link_thread_external_series with accurate Args and Returns sections; in
alembic/versions/c84500000001_add_external_identities.py lines 36-37, document
upgrade as taking no arguments and returning None; and at lines 113-114,
document downgrade with the same no-argument and None-return contract.
In `@app/models/external_identity.py`:
- Line 7: Reformat the SQLAlchemy import statement by wrapping its imported
names in a parenthesized multi-line import, keeping the same imports and
complying with the 100-character line limit.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22301c9d-fbfd-462a-a9ba-22c70a836179
📒 Files selected for processing (4)
alembic/versions/c84500000001_add_external_identities.pyapp/external_identities.pyapp/models/__init__.pyapp/models/external_identity.py
|
Based on my thorough review of the PR changes, here's my analysis: SummaryThis PR implements the core provider-independent external comic identity layer (models, migration, services) for issue #1018. The implementation is well-structured and follows the codebase conventions. Code Quality Assessment✅ Strengths
|
github-actions
Bot
added
factory:changes-requested
github-actions
Bot
added
factory:review
JoshCLWren
added
factory:unowned
ChatGPT Codex Connector
JoshCLWren
added
the
factory:ci
ChatGPT Codex Connector
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…tity normalization, lint errors. All checks pass. Co-authored-by: JoshCLWren <JoshCLWren@users.noreply.github.com>
|
The PR review is complete. Here's a summary of the issues I fixed: Changes Made1. Fixed CodeRabbit nitpicks
2. Fixed CodeRabbit major issues
3. Fixed lint errors
Verification
|
github-actions
Bot
added
factory:review
|
User opencode-agent[bot] does not have write permissions |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_external_identities.py (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the helper signature to satisfy the 100-character limit.
Line 23 is about 128 characters. The formatting guideline sets a 100-character line length with trailing commas in multi-line structures.
♻️ Proposed reformat
-async def _owned_issue(db: AsyncSession, *, username: str, title: str, issue_number: str = "1") -> tuple[User, Thread, Issue]: +async def _owned_issue( + db: AsyncSession, + *, + username: str, + title: str, + issue_number: str = "1", +) -> tuple[User, Thread, Issue]:As per coding guidelines: "Format Python code with a 100-character line length, four-space indentation, and trailing commas in multi-line structures."
🤖 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_external_identities.py` at line 23, Reformat the _owned_issue helper signature to stay within the 100-character line limit by placing its parameters on multiple indented lines and adding a trailing comma, while preserving the existing types, defaults, and return annotation.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_external_identities.py`:
- Line 23: Reformat the _owned_issue helper signature to stay within the
100-character line limit by placing its parameters on multiple indented lines
and adding a trailing comma, while preserving the existing types, defaults, and
return annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86d6ede9-ca18-47a3-a1e5-50fad1b36bff
📒 Files selected for processing (5)
alembic/versions/c84500000001_add_external_identities.pyapp/external_identities.pyapp/models/external_identity.pydocs/changelog.d/2026-08-09-1038.mdtests/test_external_identities.py
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/external_identity.py
- app/external_identities.py
- alembic/versions/c84500000001_add_external_identities.py
JoshCLWren
added
factory:2
ChatGPT Codex Connector
|
Author identity unknown *** Please tell me who you are. Run git config --global user.email "you@example.com" to set your account's default identity. fatal: empty ident name (for runner@runnervmvrwv9.ikxrxnj0kw1ung3twvp3vsgvoh.gx.internal.cloudapp.net) not allowed |
JoshCLWren
added
factory:1
ChatGPT Codex Connector
JoshCLWren
left a comment
JoshCLWren
left a comment
There was a problem hiding this comment.
Factory 1 exact-current-head review: no actionable correctness, security, regression, data-integrity, or missing-test findings remain. I verified provider/entity normalization, stale-provider protection, transactional duplicate recovery, per-issue confirmation serialization, user ownership checks, non-exclusive thread mappings, external-evidence deletion safety, and the focused regression coverage. This COMMENT records review evidence only; it is not self-approval.
JoshCLWren
added
factory:ready
ChatGPT Codex Connector
JoshCLWren
removed
factory:ready
ChatGPT Codex Connector
1 participant
Implements the core external identity layer for #1018: provider-independent series/issue identities and user-owned ComicPile mapping services, without adding provider-specific columns to core issue/thread models.
This is the current Factory 1 implementation branch. Further acceptance coverage and changelog work may still be required before merge.
Refs #1018
Summary by CodeRabbit