Conversation
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
WalkthroughAdded a nil-check and explicit error when a configured persisted-operations storage provider ID has no matching storage client in router core; updated GitHub Actions Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2727 +/- ##
===========================================
+ Coverage 45.69% 63.34% +17.64%
===========================================
Files 1032 249 -783
Lines 138960 26661 -112299
Branches 8629 0 -8629
===========================================
- Hits 63504 16889 -46615
+ Misses 73731 8409 -65322
+ Partials 1725 1363 -362 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/router.go (1)
1278-1280: Add a regression test for this new error branch.Please add a PQL manifest test case for: explicit
storage.provider_idset, no matching provider instorage_providers, and assert this exact startup error path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 1278 - 1280, Add a regression test that constructs a PQL manifest which sets storage.provider_id to a specific value while leaving storage_providers without any entry for that ID, then invokes the startup/manifest-validation entrypoint that exercises the code path that resolves pClient (e.g., the router startup or manifest load function) and asserts the returned error matches the exact message produced when pClient == nil: fmt.Errorf("storage provider %q is configured for PQL manifest but no matching provider was found in storage_providers", storageProviderID). Use the testing framework (testing.T with require/assert) to assert the error string equality and reference the storage.provider_id value you used so the test reliably triggers the pClient == nil branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/core/router.go`:
- Around line 1278-1280: Add a regression test that constructs a PQL manifest
which sets storage.provider_id to a specific value while leaving
storage_providers without any entry for that ID, then invokes the
startup/manifest-validation entrypoint that exercises the code path that
resolves pClient (e.g., the router startup or manifest load function) and
asserts the returned error matches the exact message produced when pClient ==
nil: fmt.Errorf("storage provider %q is configured for PQL manifest but no
matching provider was found in storage_providers", storageProviderID). Use the
testing framework (testing.T with require/assert) to assert the error string
equality and reference the storage.provider_id value you used so the test
reliably triggers the pClient == nil branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 735c90f0-7b29-4547-b2d9-03e0ab3833fb
📒 Files selected for processing (1)
router/core/router.go
It should no longer be necessary to run the router tests on big machines, since they have been sharded. This reduces cost.
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.