feat(api): expose sync status#1635
Conversation
|
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:
📝 WalkthroughWalkthroughAdds paginated, family-scoped Syncs API endpoints (index, latest, show); moves UUID validation and pagination sanitizers to Api::V1::BaseController; introduces Sync.for_family and sync predicates/error payloads; adds Jbuilder views, OpenAPI schemas, routes, request and controller tests, and a model tie-breaker for ordered scope. ChangesSyncs API Endpoint Implementation
Sequence DiagramsequenceDiagram
participant Client
participant API as "API Server\n(Api::V1::SyncsController)"
participant Base as "Api::V1::BaseController\n(valid_uuid?/pagination helpers)"
participant DB as "Database\n(Syncs + syncable records)"
participant Views as "Jbuilder\n(index/show/_sync)"
Client->>API: GET /api/v1/syncs (X-Api-Key)
API->>API: authorize_scope!(:read)
API->>Base: safe_page_param / safe_per_page_param
API->>DB: Sync.for_family(...).ordered.preload(...).paginate
DB-->>API: sync records
API->>Views: render index.json.jbuilder (`@syncs`, `@pagy`)
Views-->>Client: JSON collection + meta
Client->>API: GET /api/v1/syncs/latest
API->>API: authorize_scope!(:read)
API->>DB: Sync.for_family(...).ordered.preload(...).first
DB-->>API: single record or nil
API->>Views: render show.json.jbuilder (data or null)
Views-->>Client: JSON resource
Client->>API: GET /api/v1/syncs/:id
API->>Base: valid_uuid?(params[:id])
API->>DB: family_scoped_query.preload(...).find(id)
DB-->>API: record or not_found
API->>Views: render show.json.jbuilder
Views-->>Client: JSON resource
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
JSONbored
force-pushed
the
codex/feat-api-sync-status
branch
from
ccb7ca5 to
9698e47
Compare
brin-security-scanner
Bot
added
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9698e4720a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/controllers/api/v1/syncs_controller_test.rb (1)
110-113: ⚡ Quick winAdd unauthenticated coverage for
showandlatesttoo.This file only checks
401onindex, so auth regressions on the new member/collection sync endpoints could slip through. Since these are new API actions, the controller test should exercise all three read paths.♻️ Suggested additions
test "requires authentication" do get api_v1_sync_records_url assert_response :unauthorized end + + test "requires authentication for show" do + get api_v1_sync_record_url(SecureRandom.uuid) + assert_response :unauthorized + end + + test "requires authentication for latest" do + get latest_api_v1_sync_records_url + assert_response :unauthorized + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v1/syncs_controller_test.rb` around lines 110 - 113, Extend the existing "requires authentication" test to also exercise the member and collection read endpoints by issuing unauthenticated GET requests to the show and latest endpoints and asserting :unauthorized; specifically, after the existing get api_v1_sync_records_url assertion, perform a GET to the member route (api_v1_sync_record_url) using a valid record id (e.g., a fixture like sync_records(:one) or a created SyncRecord) and assert_response :unauthorized, and then perform a GET to the collection "latest" route (latest_api_v1_sync_records_url) and assert_response :unauthorized so all three read paths (index, show, latest) are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/v1/syncs_controller.rb`:
- Around line 37-40: The latest action sets `@sync` via
family_syncs_query.preload(:syncable, :children).ordered.first which can be nil;
before calling render :show add a nil check and return your API's empty-state
response (e.g., render nothing with status 404 or head :not_found) when `@sync` is
nil so the view isn't rendered with a missing `@sync`; update the latest method to
short-circuit on `@sync.nil`? and return the appropriate response.
In `@spec/requests/api/v1/syncs_spec.rb`:
- Around line 34-35: The test creates a shared Sync via let! which eagerly
persists a family sync and skews index/latest expectations; change the eager
helper to a lazy one by replacing the let! declaration for sync (the
Sync.create!(...) instance) with let(:sync) and ensure only the show example
(the spec that references id or inspects that single record) references sync so
other examples (index/latest) do not get the extra persisted record.
In `@spec/swagger_helper.rb`:
- Around line 656-677: The OpenAPI schemas using `{ type: :null }` inside the
oneOf unions are invalid; update the oneOf entries for the error property (in
the resource schema containing SyncErrorSummary) and the data property inside
SyncResponse to replace `{ type: :null }` with `{ type: :object, nullable: true
}`, ensuring the subschema explicitly allows nullability so the union permits
null values; keep the existing `{ '$ref' =>
'#/components/schemas/SyncErrorSummary' }` and `{ '$ref' =>
'#/components/schemas/SyncResource' }` entries unchanged.
---
Nitpick comments:
In `@test/controllers/api/v1/syncs_controller_test.rb`:
- Around line 110-113: Extend the existing "requires authentication" test to
also exercise the member and collection read endpoints by issuing
unauthenticated GET requests to the show and latest endpoints and asserting
:unauthorized; specifically, after the existing get api_v1_sync_records_url
assertion, perform a GET to the member route (api_v1_sync_record_url) using a
valid record id (e.g., a fixture like sync_records(:one) or a created
SyncRecord) and assert_response :unauthorized, and then perform a GET to the
collection "latest" route (latest_api_v1_sync_records_url) and assert_response
:unauthorized so all three read paths (index, show, latest) are covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8430a321-718c-4f9b-90d4-cf385e647af4
📥 Commits
Reviewing files that changed from the base of the PR and between 268f5ae and 9698e4720ad8e41da32e7427271ce9e9101ed311.
📒 Files selected for processing (9)
app/controllers/api/v1/syncs_controller.rbapp/views/api/v1/syncs/_sync.json.jbuilderapp/views/api/v1/syncs/index.json.jbuilderapp/views/api/v1/syncs/show.json.jbuilderconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/syncs_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/syncs_controller_test.rb
brin-security-scanner
Bot
added
the
contributor:flagged
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Review: feat(api): expose sync status
Good foundation — scoping is sound, error sanitization is correct, the Minitest coverage is thorough, and the rswag/OpenAPI split follows the project conventions. A few things need attention before this is merge-ready.
Must fix
SYNCABLE_ASSOCIATIONS in the controller (see inline)
This hardcoded map violates the "fat model, skinny controller" convention and will need a manual update every time a new provider is added. The family-scoping logic belongs on the Sync model as a named scope (Sync.for_family(family)). It also has a latent NoMethodError risk if family.public_send(association_name) is called for an association that doesn't exist on Family in a given deployment mode (self-hosted vs managed).
Missing 403 in OpenAPI docs (see inline)
ensure_read_scope returns 403 when a valid key lacks the required scope. All three endpoints are missing this response code; other endpoints in the same API document it.
syncable&.try(:name) nil guard (see inline)
After preloading, a deleted syncable makes sync.syncable return nil, causing a NoMethodError in the partial.
Worth discussing
latestreturning 200 +{"data": null}vs a 404 — either is defensible but the OpenAPI description should explicitly state the null-data behavior so consumers know to handle it.in_progress/terminalas derived Jbuilder fields — these will silently mis-classify syncs if the state machine gains a new state. Aterminal?predicate on theSyncmodel would be the authoritative source.helper_method :sync_error_payload— works because Jbuilder evaluates in an ActionView context, but the implicit coupling is non-obvious. Worth a comment or restructuring as a plain private method called from a local variable in the partial.
Minor / nits
Sync.destroy_allin test setup — safe under transactional tests but brittle; scope to the family instead.- Three API keys + Redis cleanup created for every test; most tests only need one or two.
- Route alias
as: :sync_recordscreates confusing helper names (api_v1_sync_records_url) — at minimum add an inline comment explaining the collision.
Generated by Claude Code
JSONbored
force-pushed
the
codex/feat-api-sync-status
branch
from
aadfecd to
dfafaf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/v1/syncs_controller.rb`:
- Around line 44-46: The method family_syncs_query currently uses
current_resource_owner.family which can raise when no resource owner is present
and bypasses the standardized auth context; update family_syncs_query to build
the scope using Current.family (and Current.user if needed) and call
Sync.for_family with Current.family while passing the authenticated user via
resource_owner: Current.user so the method uses the app's standardized auth
context (replace current_resource_owner.family and resource_owner:
current_resource_owner with Current.family and resource_owner: Current.user in
family_syncs_query and the Sync.for_family call).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f9a7d6a-4649-41e1-8098-af8923bf6f8c
📥 Commits
Reviewing files that changed from the base of the PR and between aadfecd7d491a8709c412da3ed42bfbdfd591e10 and dfafaf9398660c09a851656ffe154de194db93a6.
📒 Files selected for processing (10)
app/controllers/api/v1/syncs_controller.rbapp/models/sync.rbapp/views/api/v1/syncs/_sync.json.jbuilderapp/views/api/v1/syncs/index.json.jbuilderapp/views/api/v1/syncs/show.json.jbuilderconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/syncs_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/syncs_controller_test.rb
✅ Files skipped from review due to trivial changes (3)
- app/views/api/v1/syncs/index.json.jbuilder
- spec/swagger_helper.rb
- app/views/api/v1/syncs/show.json.jbuilder
🚧 Files skipped from review as they are similar to previous changes (5)
- config/routes.rb
- app/views/api/v1/syncs/_sync.json.jbuilder
- docs/api/openapi.yaml
- test/controllers/api/v1/syncs_controller_test.rb
- spec/requests/api/v1/syncs_spec.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/v1/syncs_controller.rb`:
- Around line 11-15: The current ordering is non-deterministic because
Sync.ordered only sorts by created_at DESC; update the ordering to include a
stable tiebreaker (id DESC) so pagination and .latest are deterministic. Fix by
changing the scope Sync.ordered in the Sync model to order(created_at: :desc,
id: :desc) (or by appending .order(id: :desc) where
family_syncs_query.preload(:syncable, :children).ordered is called) so both the
pagy call using family_syncs_query and any usages of Sync.ordered use created_at
DESC, id DESC as the ordering.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eeb0a6bc-b6a0-4b2a-ab7f-717b1b4a1c53
📥 Commits
Reviewing files that changed from the base of the PR and between dfafaf9398660c09a851656ffe154de194db93a6 and 066507cf1db7ee02acee65b824207f6eb8802ee1.
📒 Files selected for processing (3)
app/controllers/api/v1/base_controller.rbapp/controllers/api/v1/syncs_controller.rbtest/controllers/api/v1/syncs_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/controllers/api/v1/syncs_controller_test.rb
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/controllers/api/v1/base_controller.rb (1)
224-233: ⚡ Quick win
safe_per_page_paramsilently resets to 25 for values > 100 instead of clamping.A caller passing
per_page=200gets 25 items back — the default — rather than the enforced maximum of 100. This is surprising API behavior that can be hard to debug. Clamping out-of-range positive values to the maximum is the more conventional and transparent contract.♻️ Proposed fix: clamp instead of reset for values > 100
def safe_per_page_param per_page = params[:per_page].to_i case per_page when 1..100 per_page + when 101..Float::INFINITY + 100 else 25 end endOr more concisely:
def safe_per_page_param per_page = params[:per_page].to_i - - case per_page - when 1..100 - per_page - else - 25 - end + return 25 unless per_page >= 1 + [per_page, 100].min end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/base_controller.rb` around lines 224 - 233, The method safe_per_page_param currently returns the default 25 for any value outside 1..100, so a caller passing per_page=200 gets 25 instead of the intended maximum; update safe_per_page_param to clamp positive out-of-range values by returning 100 when params[:per_page].to_i > 100, return params[:per_page].to_i when in 1..100, and keep returning 25 for non-positive or missing values—change the conditional logic in safe_per_page_param accordingly.app/models/sync.rb (1)
112-120: ⚡ Quick winUse
I18n.t()for the API-facingmessagestrings.Both
"Sync became stale before completion"and"Sync failed"are hard-coded English strings that flow through Jbuilder into API responses visible to clients. Per the coding guidelines, all user-facing strings in**/*.{erb,rb}must use thet()helper.♻️ Proposed refactor
Add to
config/locales/en.yml:en: sync: errors: failed: "Sync failed" stale: "Sync became stale before completion"Then update
api_error_payload:def api_error_payload return unless failed? || stale? return if stale? && error.blank? { present: error.present?, - message: stale? ? "Sync became stale before completion" : "Sync failed" + message: stale? ? I18n.t("sync.errors.stale") : I18n.t("sync.errors.failed") } endAs per coding guidelines: "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sync.rb` around lines 112 - 120, The api_error_payload method currently returns hard-coded English messages; change it to use the Rails I18n helper (t or I18n.t) for API-facing strings: replace the literal "Sync became stale before completion" and "Sync failed" with translations (e.g. t('sync.errors.stale') and t('sync.errors.failed')) and add corresponding keys to config/locales/en.yml; ensure the method still guards with failed?, stale?, and error presence as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/api/v1/base_controller.rb`:
- Around line 224-233: The method safe_per_page_param currently returns the
default 25 for any value outside 1..100, so a caller passing per_page=200 gets
25 instead of the intended maximum; update safe_per_page_param to clamp positive
out-of-range values by returning 100 when params[:per_page].to_i > 100, return
params[:per_page].to_i when in 1..100, and keep returning 25 for non-positive or
missing values—change the conditional logic in safe_per_page_param accordingly.
In `@app/models/sync.rb`:
- Around line 112-120: The api_error_payload method currently returns hard-coded
English messages; change it to use the Rails I18n helper (t or I18n.t) for
API-facing strings: replace the literal "Sync became stale before completion"
and "Sync failed" with translations (e.g. t('sync.errors.stale') and
t('sync.errors.failed')) and add corresponding keys to config/locales/en.yml;
ensure the method still guards with failed?, stale?, and error presence as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c910c27-0743-4790-88aa-5e93fe73a675
📥 Commits
Reviewing files that changed from the base of the PR and between 066507cf1db7ee02acee65b824207f6eb8802ee1 and 65e4235883b9764323acf54fd4b586732b5a2359.
📒 Files selected for processing (5)
app/controllers/api/v1/base_controller.rbapp/controllers/api/v1/syncs_controller.rbapp/models/sync.rbtest/controllers/api/v1/syncs_controller_test.rbtest/models/sync_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/controllers/api/v1/syncs_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/v1/syncs_controller.rb
JSONbored
force-pushed
the
codex/feat-api-sync-status
branch
from
4faee07 to
d0f85e0
Compare
JSONbored
force-pushed
the
codex/feat-api-sync-status
branch
from
d0f85e0 to
82aca49
Compare
|
Thanks for the detailed review. I pushed a follow-up in
I also tightened the sync error payload so emitted error objects are always actionable ( Validation:
GitHub CI is green now as well. |
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Can you fix merge conflicts before next review, @JSONbored?
brin-security-scanner
Bot
added
the
contributor:flagged
JSONbored
force-pushed
the
codex/feat-api-sync-status
branch
from
eb1532d to
1188b2d
Compare
Fixed! |
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Re-review — independent code read
Verified the diff end-to-end against the live base controller and model. Summary below.
✅ Properly addressed since the previous review
| Issue | How it landed |
|---|---|
| P1 scope leak | Sync.for_family now lives in the model and threads resource_owner through account_syncable_ids; controller passes Current.user. |
| P2 UUID validation | valid_uuid? is called before find in set_sync; the helper is defined in BaseController with a strict regex — confirmed. |
| Deterministic ordering | scope :ordered, -> { order(created_at: :desc, id: :desc) } — tiebreaker present. |
latest nil case |
Early-returns render json: { data: nil } before hitting the view. |
Current.* usage |
family_syncs_query correctly uses Current.family / Current.user. |
| Fat-model convention | for_family, in_progress?, terminal?, api_error_payload all live in Sync, not the controller. |
| Orphaned syncable guard | syncable&.try(:name) in the partial handles nil syncable. |
| Test teardown scope | Sync.for_family(@family).destroy_all — scoped, not global. |
| 403 documented | All three paths now carry the 403 response in the YAML. |
| Route alias | as: :sync_job on the existing POST prevents the name collision. |
🔴 Still broken — OAS 3.0 $ref + nullable: true sibling issue
The CodeRabbit thread about type: :null was marked resolved after replacing the oneOf approach, but the replacement is also non-conformant with OpenAPI 3.0.x.
In OAS 3.0, $ref replaces the entire schema object at that level — any sibling keys (including nullable: true) are silently dropped by compliant validators. This affects two properties right now:
SyncResource.error (docs/api/openapi.yaml and spec/swagger_helper.rb):
# current — nullable is ignored by OAS 3.0 parsers
error:
"$ref": "#/components/schemas/SyncErrorSummary"
nullable: true# correct OAS 3.0
error:
nullable: true
allOf:
- "$ref": "#/components/schemas/SyncErrorSummary"SyncResponse.data — same pattern, same fix needed.
In spec/swagger_helper.rb the Ruby equivalent would be:
error: { nullable: true, allOf: [ { '$ref' => '#/components/schemas/SyncErrorSummary' } ] }
data: { nullable: true, allOf: [ { '$ref' => '#/components/schemas/SyncResource' } ] }Swagger UI and rswag tend to be lenient here so it may render fine, but swagger-parser --validate will flag it and any strict client generator will mishandle the nullability.
🟡 Threads that are functionally resolved but not marked so
- P2 UUID thread (
r3176234818) —is_resolved: false, is_outdated: false, but the code already callsvalid_uuid?beforefind. Please resolve. helper_methodthread (r3177232155) —is_resolved: false, is_outdated: false, but there is nohelper_methodanywhere in the current controller diff. This comment appears to reference a version of the code that was reworked before the diff settled. Please resolve.family.public_sendNoMethodError guard (r3177232276) — outdated;family_syncable_associationsnow lives in theSyncmodel and iterates overFamily.reflect_on_all_associations, so every association name it yields is guaranteed to exist onFamily. Therescue NameErrorinside the block handles any badklassreferences. Safe as written.- Routes naming (
r3177232939) — outdated;as: :sync_jobresolves the collision. - 403 docs (
r3177233088) — outdated; 403 is present on all three paths.
🟡 Minor — rswag spec key-creation pattern differs from Minitest
Minitest creates keys with display_key::
ApiKey.create!(display_key: "test_rw_#{SecureRandom.hex(8)}", source: "web")The rswag spec uses key: and ApiKey.generate_secure_key:
key = ApiKey.generate_secure_key
ApiKey.create!(key: key, source: "web")If ApiKey.find_by_value hashes the incoming header value and compares against the stored key column, the rswag pattern may be correct — but it differs from every other controller test in this project. If it's wrong, rswag:specs:swaggerize will silently produce 401 responses in the generated YAML instead of 200s for the success cases. Worth a quick sanity-check against one of the existing rswag specs (e.g., accounts_spec.rb) to make sure the pattern matches.
ℹ️ Design notes (not blocking, no action required unless you disagree)
api_error_payloadfor stale+error — returns{ present: true, message: "Sync became stale before completion" }even though the raw error is suppressed. Returningnilfor stale-no-error but an object for stale-with-error means clients need to branch on the object's existence rather thanpresent. Consistent, but the field namepresent: trueinside an object that is already non-null is redundant. The field could be dropped; presence of the object is the signal. This is a design call, not a correctness issue.show.json.jbuilder200+null forlatest— the OpenAPI description already documentsdata: null, so this is fine. Noted only because jjmata's thread is still open.
Bottom line
One real defect remains: the OAS 3.0 $ref + nullable: true sibling pattern in both the YAML and the swagger helper. Everything else is addressed or is a design preference. Fixing that schema issue and resolving the stale threads would clear the PR for merge consideration.
Generated by Claude Code
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Re-review — full independent code read (commit 9bc4c7f)
All substantive issues from prior rounds are resolved. Below is an itemised verification, then the two things still worth a look before merge.
✅ Everything from the previous review is addressed
| Issue | Verification |
|---|---|
OAS 3.0 $ref + nullable: true sibling bug |
Fixed: both SyncResource.error and SyncResponse.data now use nullable: true + allOf: [{ $ref }], which is the correct OAS 3.0 pattern. |
present: semantic confusion in error payload |
Removed. api_error_payload now returns { message: "..." } or nil. SyncErrorSummary schema updated to match — only message in required and properties. |
| rswag ApiKey creation inconsistency | Fixed: spec now sets both key: key and display_key: key, matching how ApiKey.find_by_value / plain_key work. |
403 rswag test used update_column mutation |
Fixed: api_key_without_read_scope is a proper let with its own key, not a mid-test column mutation. |
| All other prior-round issues | Confirmed addressed (UUID guard, Current.*, for_family in model, syncable&.try(:name), scoped destroy, 403 docs, route alias, deterministic ordering). |
The base controller confirms valid_uuid?, safe_page_param, safe_per_page_param, and handle_not_found are all present — no hidden dependency issues.
🟡 Four open threads are addressed but not yet marked resolved
r3177231986 (skinny-controllers), r3177232701 (nil syncable guard), r3177232804 (terminal? coverage), r3177233290 (three keys / Redis) are all is_outdated: true — the code they pointed at was reworked. Please mark them resolved to keep the thread list clean.
ℹ️ Nit — nil branch in show.json.jbuilder is unreachable
# show.json.jbuilder
if @sync
json.partial! "api/v1/syncs/sync", sync: @sync
else
json.nil! # ← dead code
endThe latest action returns early with render json: { data: nil } before this view is ever called with a nil @sync, and the show action always raises RecordNotFound rather than setting @sync = nil. The else branch is harmless but it's dead code. The early return in latest could instead flow through the view (drop the guard and let show.json.jbuilder handle the nil), or the view's nil branch can just be removed since it's never hit. Either way, not blocking.
Bottom line
No correctness issues remain. The OAS schema is now valid, the error payload semantics are clean, and test setup is consistent. Ready to merge once the four stale threads are closed.
Generated by Claude Code
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
4 participants
Summary
What changed
GET /api/v1/syncs,GET /api/v1/syncs/latest, andGET /api/v1/syncs/{id}.Why
API clients and self-hosted operators need a stable way to poll sync status without scraping web UI state or exposing provider internals.
Validation
Notes
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests