Add assigned policy list to catalog response#548
Conversation
|
Warning Rate limit exceeded
⌛ 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis change introduces support for Policies in LLM provider catalog entries by adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
agent-manager-service/repositories/llm_provider_repository.go (1)
189-195: Misleading log message: UUID is validated but not "resolved".The log message at line 190 says "resolved UUID", but the parsed UUID is now discarded (
_). The parse only serves as validation. Consider updating the log message to accurately reflect the operation.Suggested fix
return r.db.Transaction(func(tx *gorm.DB) error { - slog.Info("LLMProviderRepo.Update: resolved UUID", "providerID", providerID) + slog.Info("LLMProviderRepo.Update: validating provider ID format", "providerID", providerID) _, err := uuid.Parse(providerID) if err != nil { return fmt.Errorf("error parsing provider id: %s, error: %w", providerID, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/repositories/llm_provider_repository.go` around lines 189 - 195, The log message "LLMProviderRepo.Update: resolved UUID" is misleading because the code only validates the UUID via uuid.Parse and discards the result; update the slog.Info call in the Transaction block to reflect validation (e.g., "validated UUID") or, alternatively, capture the parsed value (assign the result of uuid.Parse to a variable) and log the actual parsed UUID; refer to the Transaction lambda, providerID, the uuid.Parse call and the slog.Info invocation to locate and fix the message.agent-manager-service/services/agent_configuration_service.go (2)
1679-1680: Minor: Go naming convention for UUID acronym.Per Go conventions, acronyms should be fully uppercase.
contextUuidshould becontextUUID.✏️ Suggested fix
- contextUuid := uuid.New() - contextPath := fmt.Sprintf("/%s", contextUuid) + contextUUID := uuid.New() + contextPath := fmt.Sprintf("/%s", contextUUID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 1679 - 1680, The variable name contextUuid violates Go conventions for acronyms; rename contextUuid to contextUUID and update any dependent uses such as the subsequent contextPath assignment (contextPath := fmt.Sprintf("/%s", contextUUID)) and all other references in the same function/file to use contextUUID so the identifier follows the uppercase-acronym style.
902-902: ReturningproviderUUIDalone has no effect on rollback.The returned
rollbackResource{providerUUID: providerUUID}is never added torollbackResourcesbecause line 1307 checksrbRes.providerAPIKeyID != "", which will be false. TheproviderUUIDfield alone is unused during rollback (line 1992 also checksproviderAPIKeyID != "").Consider returning an empty struct or documenting that Scenario B requires no rollback tracking:
♻️ Suggested simplification
- return rollbackResource{providerUUID: providerUUID}, nil + // Scenario B reuses existing proxy/keys; no new external resources to roll back. + return rollbackResource{}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` at line 902, The returned rollbackResource{providerUUID: providerUUID} is ineffective because rollback logic only uses entries with a non-empty providerAPIKeyID (checks like rbRes.providerAPIKeyID != "" and later checks providerAPIKeyID != ""); either stop returning a partial rollbackResource for this code path or populate providerAPIKeyID when rollback tracking is required. Update the function to return an empty rollbackResource{} for Scenario B (or add a comment documenting that Scenario B requires no rollback tracking), or set providerAPIKeyID on the returned rollbackResource when you expect it to be consumed by the rollbackResources aggregation and execution paths (rollbackResource, rbRes.providerAPIKeyID, providerAPIKeyID checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent-manager-service/repositories/llm_provider_repository.go`:
- Around line 189-195: The log message "LLMProviderRepo.Update: resolved UUID"
is misleading because the code only validates the UUID via uuid.Parse and
discards the result; update the slog.Info call in the Transaction block to
reflect validation (e.g., "validated UUID") or, alternatively, capture the
parsed value (assign the result of uuid.Parse to a variable) and log the actual
parsed UUID; refer to the Transaction lambda, providerID, the uuid.Parse call
and the slog.Info invocation to locate and fix the message.
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 1679-1680: The variable name contextUuid violates Go conventions
for acronyms; rename contextUuid to contextUUID and update any dependent uses
such as the subsequent contextPath assignment (contextPath := fmt.Sprintf("/%s",
contextUUID)) and all other references in the same function/file to use
contextUUID so the identifier follows the uppercase-acronym style.
- Line 902: The returned rollbackResource{providerUUID: providerUUID} is
ineffective because rollback logic only uses entries with a non-empty
providerAPIKeyID (checks like rbRes.providerAPIKeyID != "" and later checks
providerAPIKeyID != ""); either stop returning a partial rollbackResource for
this code path or populate providerAPIKeyID when rollback tracking is required.
Update the function to return an empty rollbackResource{} for Scenario B (or add
a comment documenting that Scenario B requires no rollback tracking), or set
providerAPIKeyID on the returned rollbackResource when you expect it to be
consumed by the rollbackResources aggregation and execution paths
(rollbackResource, rbRes.providerAPIKeyID, providerAPIKeyID checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 355bd4b1-196b-4ff8-bdb7-04e8486c8508
📒 Files selected for processing (8)
agent-manager-service/controllers/catalog_controller.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/catalog.goagent-manager-service/repositories/catalog_repository.goagent-manager-service/repositories/llm_provider_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/spec/model_catalog_llm_provider_entry.go
💤 Files with no reviewable changes (1)
- agent-manager-service/repositories/llm_proxy_repository.go
menakaj
force-pushed
the
internal-agent-config
branch
from
96679df to
f562b85
Compare
menakaj
force-pushed
the
internal-agent-config
branch
from
f562b85 to
ddb80a9
Compare
2 participants
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Bug Fixes