refactor: rename 'upstream sync' to 'remote sync' / 'Sync from Remote'#33
Merged
damienriehl merged 3 commits intomainfrom Apr 6, 2026
Merged
refactor: rename 'upstream sync' to 'remote sync' / 'Sync from Remote'#33damienriehl merged 3 commits intomainfrom
damienriehl merged 3 commits intomainfrom
Conversation
|
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 38 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR systematically renames terminology throughout the codebase from "upstream sync" to "remote sync" across database schema, ORM models, API schemas, routes, services, and background workers, including database migration, model/schema class renames, API endpoint path updates, and background task name changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Merged
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
ontokit/worker.py (1)
689-697:⚠️ Potential issue | 🟠 MajorRoute terminal setup errors through the failure path.
Lines 689-697 and 722-733 return a
{"status": "failed"}payload instead of raising. That bypasses theexceptblock, so noremote_check_failedpubsub event is emitted, andontokit/services/remote_sync_service.pywill still surface the job as completed because it only marks raised jobs as failed.Also applies to: 722-733
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/worker.py` around lines 689 - 697, The current early-return paths that return {"status": "failed", ...} (e.g., when config is falsy or project is not found in the block using config, project_result and project) must instead raise an exception so the outer except can catch it and emit the remote_check_failed pubsub event; replace those returns with a specific exception (or RuntimeError) raised with the same error message, and apply the same change to the analogous checks in the later block (the 722-733 section) so ontokit/services/remote_sync_service.py will treat the job as failed and publish the failure event.ontokit/services/remote_sync_service.py (2)
157-171:⚠️ Potential issue | 🟠 MajorOnly persist
checkingafter the job is queued.Lines 158-163 commit the state flip before verifying the queue is available. If queue acquisition or enqueueing fails, this path exits before the
job is Nonefallback and the config stays stuck inchecking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/services/remote_sync_service.py` around lines 157 - 171, Currently the code sets config.status="checking" and commits before acquiring the queue and enqueueing the job, which can leave config stuck if get_arq_pool() or pool.enqueue_job(...) fails; change the flow in the method that enqueues run_remote_check_task so that you first attempt to acquire the pool (get_arq_pool()) and enqueue the job (pool.enqueue_job(str(project_id))), and only after a successful enqueue (job is not None) set config.status="checking" and call await self.db.commit(); additionally, catch exceptions from get_arq_pool() and enqueue_job and in those error paths set config.status="error", populate config.error_message, and commit via await self.db.commit() before raising the HTTPException so the DB never remains in a stale "checking" state.
93-105:⚠️ Potential issue | 🟠 MajorReject partial updates when no remote-sync config exists.
Lines 95-99 build a new
RemoteSyncConfigfromRemoteSyncConfigUpdate. If the request only includes partial fields, the write falls through to the non-nullrepo_owner,repo_name, andfile_pathcolumns and fails as a DB integrity error instead of a clean 4xx.Suggested guard
if config is None: # Create new config — require full payload if isinstance(data, RemoteSyncConfigUpdate): - # Treat partial update on non-existent config as create with defaults - config = RemoteSyncConfig(project_id=project_id) - for field, value in data.model_dump(exclude_unset=True).items(): - setattr(config, field, value) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Remote sync is not configured; send a full create payload first", + ) else: config = RemoteSyncConfig( project_id=project_id, **data.model_dump(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/services/remote_sync_service.py` around lines 93 - 105, When creating a new RemoteSyncConfig from an update payload in remote_sync_service.py (the branch that checks isinstance(data, RemoteSyncConfigUpdate)), validate that all required fields (e.g., repo_owner, repo_name, file_path) are present in data.model_dump(exclude_unset=True); if any are missing, raise a 4xx-style error (e.g., HTTPException/ValueError) instead of constructing a partial RemoteSyncConfig and writing to the DB. Update the logic around RemoteSyncConfig(project_id=project_id) to check the dumped keys first and only populate defaults when all required fields exist; otherwise return a clear client error indicating the missing required fields.ontokit/api/routes/pull_requests.py (1)
681-686:⚠️ Potential issue | 🟠 MajorDon't leave webhook-triggered syncs stuck in
checking.Lines 682-686 commit
sync_config.status = "checking"before the background job is safely queued. If queue acquisition or enqueueing fails, the webhook can error out after persisting that state, which leaves the config stuck incheckingand blocks later manual checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/api/routes/pull_requests.py` around lines 681 - 686, The code sets sync_config.status = "checking" and commits before acquiring the ARQ pool and enqueueing the background task, which can leave the config stuck if enqueueing fails; change the flow so you only persist the "checking" status after successfully obtaining get_arq_pool() and awaiting pool.enqueue_job("run_remote_check_task", str(project_id)), or alternatively wrap the pool acquisition/enqueue in a try/except and on failure reset sync_config.status (or rollback) and commit that change; reference sync_config.status, get_arq_pool(), pool.enqueue_job, and service.db.commit to locate and update the logic.
🧹 Nitpick comments (3)
ontokit/schemas/remote_sync.py (2)
66-67: Use Pydantic v2model_configinstead ofclass Config.The
class Configsyntax is Pydantic v1 style. For Pydantic v2 compliance, usemodel_config = ConfigDict(from_attributes=True).♻️ Proposed fix
from pydantic import BaseModel, Field +from pydantic import ConfigDict- class Config: - from_attributes = True + model_config = ConfigDict(from_attributes=True)Based on learnings: "Use Pydantic v2 for request/response validation with strict validation and computed fields".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/schemas/remote_sync.py` around lines 66 - 67, Replace the Pydantic v1 "class Config" usage with a Pydantic v2 model configuration: remove the inner class Config and add a module-level or model-level attribute model_config = ConfigDict(from_attributes=True); also import ConfigDict from pydantic at the top of the file so the model uses Pydantic v2 settings. Target the model that currently defines "class Config" (the Config inner class) and replace it with "model_config" and the ConfigDict import.
83-84: Same Pydantic v2 migration needed here.Apply the same
model_config = ConfigDict(from_attributes=True)pattern toSyncEventResponse.♻️ Proposed fix
- class Config: - from_attributes = True + model_config = ConfigDict(from_attributes=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/schemas/remote_sync.py` around lines 83 - 84, Replace the old Pydantic v1 style inner class Config in SyncEventResponse with a v2 config by adding model_config = ConfigDict(from_attributes=True) on the SyncEventResponse class and import ConfigDict from pydantic; remove or replace the nested class Config declaration so SyncEventResponse uses model_config = ConfigDict(from_attributes=True).ontokit/api/routes/remote_sync.py (1)
40-41: Consider usingDepends()for service injection.The current pattern calls
get_remote_sync_service(db)directly within each handler. Per coding guidelines, services should be injected via FastAPI'sDepends()for consistency and testability.♻️ Proposed approach
Create a dependency that composes the DB session:
from ontokit.services.remote_sync_service import RemoteSyncService, get_remote_sync_service # Dependency that yields the service def get_service(db: Annotated[AsyncSession, Depends(get_db)]) -> RemoteSyncService: return get_remote_sync_service(db)Then inject in handlers:
async def get_remote_sync_config( project_id: UUID, - db: Annotated[AsyncSession, Depends(get_db)], + service: Annotated[RemoteSyncService, Depends(get_service)], user: RequiredUser, ) -> RemoteSyncConfigResponse | None: - service = get_remote_sync_service(db) return await service.get_config(project_id, user)As per coding guidelines: "Use dependency injection via FastAPI's
Depends()for service access".Also applies to: 59-60, 76-77, 96-97, 111-112, 126-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ontokit/api/routes/remote_sync.py` around lines 40 - 41, Create a FastAPI dependency that composes the DB session and returns the service instead of calling get_remote_sync_service(db) directly: add a function (e.g., get_service) that takes db: Annotated[AsyncSession, Depends(get_db)] and returns get_remote_sync_service(db) (type RemoteSyncService). Then update each route handler to accept service: RemoteSyncService = Depends(get_service) and call methods on that injected service (e.g., service.get_config, service.get_state, service.sync, service.start_sync, etc.) in place of direct get_remote_sync_service(db) calls; apply the same change to the other handlers mentioned (lines ~59-60, 76-77, 96-97, 111-112, 126-127).
🤖 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 `@ontokit/api/routes/pull_requests.py`:
- Around line 681-686: The code sets sync_config.status = "checking" and commits
before acquiring the ARQ pool and enqueueing the background task, which can
leave the config stuck if enqueueing fails; change the flow so you only persist
the "checking" status after successfully obtaining get_arq_pool() and awaiting
pool.enqueue_job("run_remote_check_task", str(project_id)), or alternatively
wrap the pool acquisition/enqueue in a try/except and on failure reset
sync_config.status (or rollback) and commit that change; reference
sync_config.status, get_arq_pool(), pool.enqueue_job, and service.db.commit to
locate and update the logic.
In `@ontokit/services/remote_sync_service.py`:
- Around line 157-171: Currently the code sets config.status="checking" and
commits before acquiring the queue and enqueueing the job, which can leave
config stuck if get_arq_pool() or pool.enqueue_job(...) fails; change the flow
in the method that enqueues run_remote_check_task so that you first attempt to
acquire the pool (get_arq_pool()) and enqueue the job
(pool.enqueue_job(str(project_id))), and only after a successful enqueue (job is
not None) set config.status="checking" and call await self.db.commit();
additionally, catch exceptions from get_arq_pool() and enqueue_job and in those
error paths set config.status="error", populate config.error_message, and commit
via await self.db.commit() before raising the HTTPException so the DB never
remains in a stale "checking" state.
- Around line 93-105: When creating a new RemoteSyncConfig from an update
payload in remote_sync_service.py (the branch that checks isinstance(data,
RemoteSyncConfigUpdate)), validate that all required fields (e.g., repo_owner,
repo_name, file_path) are present in data.model_dump(exclude_unset=True); if any
are missing, raise a 4xx-style error (e.g., HTTPException/ValueError) instead of
constructing a partial RemoteSyncConfig and writing to the DB. Update the logic
around RemoteSyncConfig(project_id=project_id) to check the dumped keys first
and only populate defaults when all required fields exist; otherwise return a
clear client error indicating the missing required fields.
In `@ontokit/worker.py`:
- Around line 689-697: The current early-return paths that return {"status":
"failed", ...} (e.g., when config is falsy or project is not found in the block
using config, project_result and project) must instead raise an exception so the
outer except can catch it and emit the remote_check_failed pubsub event; replace
those returns with a specific exception (or RuntimeError) raised with the same
error message, and apply the same change to the analogous checks in the later
block (the 722-733 section) so ontokit/services/remote_sync_service.py will
treat the job as failed and publish the failure event.
---
Nitpick comments:
In `@ontokit/api/routes/remote_sync.py`:
- Around line 40-41: Create a FastAPI dependency that composes the DB session
and returns the service instead of calling get_remote_sync_service(db) directly:
add a function (e.g., get_service) that takes db: Annotated[AsyncSession,
Depends(get_db)] and returns get_remote_sync_service(db) (type
RemoteSyncService). Then update each route handler to accept service:
RemoteSyncService = Depends(get_service) and call methods on that injected
service (e.g., service.get_config, service.get_state, service.sync,
service.start_sync, etc.) in place of direct get_remote_sync_service(db) calls;
apply the same change to the other handlers mentioned (lines ~59-60, 76-77,
96-97, 111-112, 126-127).
In `@ontokit/schemas/remote_sync.py`:
- Around line 66-67: Replace the Pydantic v1 "class Config" usage with a
Pydantic v2 model configuration: remove the inner class Config and add a
module-level or model-level attribute model_config =
ConfigDict(from_attributes=True); also import ConfigDict from pydantic at the
top of the file so the model uses Pydantic v2 settings. Target the model that
currently defines "class Config" (the Config inner class) and replace it with
"model_config" and the ConfigDict import.
- Around line 83-84: Replace the old Pydantic v1 style inner class Config in
SyncEventResponse with a v2 config by adding model_config =
ConfigDict(from_attributes=True) on the SyncEventResponse class and import
ConfigDict from pydantic; remove or replace the nested class Config declaration
so SyncEventResponse uses model_config = ConfigDict(from_attributes=True).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2af34a3-5fee-4de1-a8e9-90563e7062fc
📒 Files selected for processing (10)
alembic/versions/r1s2t3u4v5w6_rename_upstream_to_remote_sync.pyontokit/api/routes/__init__.pyontokit/api/routes/pull_requests.pyontokit/api/routes/remote_sync.pyontokit/models/__init__.pyontokit/models/remote_sync.pyontokit/schemas/remote_sync.pyontokit/services/pull_request_service.pyontokit/services/remote_sync_service.pyontokit/worker.py
damienriehl
approved these changes
Apr 6, 2026
Contributor
damienriehl
left a comment
damienriehl
left a comment
There was a problem hiding this comment.
Clean mechanical rename with a well-structured Alembic migration. Table, column, and index renames in correct order with proper downgrade path.
Closes #31 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Renames upstream_sync_configs → remote_sync_configs, upstream_commit_sha → remote_commit_sha on both tables, and updates the associated index. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
damienriehl
force-pushed
the
fix/rename-upstream-to-remote
branch
from
8e45412 to
d04800a
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
upstream_sync.py→remote_sync.pyacross routes, models, schemas, and services/upstream-sync→/remote-syncUpstreamSync*→RemoteSync*upstream_commit_sha→remote_commit_shaCloses #31
Related
Test plan
alembic upgrade headsucceeds (table/column rename)alembic downgrade -1succeeds (revert)/remote-syncpaths🤖 Generated with Claude Code
Summary by CodeRabbit
/upstream-sync/*to/remote-sync/*upstream_commit_sha→remote_commit_sha)