Scope org data by Thunder OU ID instead of org name#1266
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR migrates tenant scoping across the agent-manager-service from organization-name-based identifiers ( ChangesOU-ID Scoping Migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant Middleware
participant Controller
participant Service
participant openChoreoClient
participant Repository
Request->>Middleware: HTTP request with JWT token
Middleware->>Middleware: extract OuId claim, set ResolvedOrg
Middleware->>Controller: forward request
Controller->>Controller: ouID := OUIDFromRequest(r)
Controller->>Service: call(ctx, ouID, ...)
Service->>Repository: query filtered by ou_id
Repository-->>Service: scoped results
Service->>openChoreoClient: call(ctx, ouID, ...)
openChoreoClient->>openChoreoClient: NamespaceFor(ouID)
openChoreoClient-->>Service: result
Service-->>Controller: response
Controller-->>Request: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
agent-manager-service/services/infra_resource_manager.go (1)
72-425: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUnexpected errors from
ocClientcalls are mostly returned unwrapped.Across
GetOrganization,CreateProject,ListProjects,DeleteProject,GetProject,GetProjectDeploymentPipeline,CreateOrgDeploymentPipeline,UpdateOrgDeploymentPipeline, andGetDataplanes, client errors are propagated with plainreturn nil, err/return err(e.g. lines 78, 92-93, 105, 133, 152, 200, 211, 227, 240, 246, 313, 320, 339, 370, 412). This is inconsistent withUpdateProject's own patch-failure path (line 145:fmt.Errorf("failed to update project: %w", err)) and withDeleteOrgDeploymentPipeline(lines 384, 399). As per coding guidelines,agent-manager-service/**/*.gorequires: "wrap all other unexpected errors with context usingfmt.Errorf(\"...: %w\", err)".🐛 Example fix pattern (apply consistently)
org, err := s.ocClient.GetOrganization(ctx, ouID) if err != nil { s.logger.Error("Failed to get organization from OpenChoreo", "ouID", ouID, "error", err) - return nil, err + return nil, fmt.Errorf("failed to get organization %s: %w", ouID, err) }🤖 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 `@agent-manager-service/services/infra_resource_manager.go` around lines 72 - 425, The ocClient error paths in infraResourceManager are returning raw errors instead of adding context, which is inconsistent with the rest of the file. Update the affected methods GetOrganization, CreateProject, ListProjects, DeleteProject, GetProject, GetProjectDeploymentPipeline, CreateOrgDeploymentPipeline, UpdateOrgDeploymentPipeline, and GetDataplanes to wrap unexpected client failures with fmt.Errorf using a descriptive message and %w, similar to UpdateProject and DeleteOrgDeploymentPipeline. Keep the existing logger calls, but ensure each returned error provides operation-specific context and preserves the original error for callers.Source: Coding guidelines
agent-manager-service/repositories/agent_config_repository.go (1)
35-44: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftI/O methods missing
context.Contextparameter.
Get,DeleteAllByAgent, andUpsertperform DB I/O but take nocontext.Context, so cancellation/timeout/tracing context cannot propagate. This contrasts withagent_configuration_repository.goin the same PR, where every method correctly takesctx context.Contextfirst. As per coding guidelines, "Every I/O method must takecontext.Contextas the first parameter and propagate it."🔧 Suggested fix
- Get(ouID, projectName, agentName, environmentName string) (*models.AgentConfig, error) + Get(ctx context.Context, ouID, projectName, agentName, environmentName string) (*models.AgentConfig, error) - DeleteAllByAgent(ouID, projectName, agentName string) error + DeleteAllByAgent(ctx context.Context, ouID, projectName, agentName string) error - Upsert(config *models.AgentConfig) error + Upsert(ctx context.Context, config *models.AgentConfig) errorand thread
.WithContext(ctx)into ther.dbcalls, updating callers accordingly.Also applies to: 57-57, 91-91, 105-105
🤖 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 `@agent-manager-service/repositories/agent_config_repository.go` around lines 35 - 44, The AgentConfigRepository methods are missing context propagation for DB I/O. Update the interface methods Upsert, Get, and DeleteAllByAgent to accept context.Context as the first parameter, then thread ctx through the AgentConfigRepository implementation by using it in the r.db calls via WithContext(ctx). Also update any callers to pass context through consistently, matching the pattern used in agent_configuration_repository.go.Source: Coding guidelines
agent-manager-service/repositories/ai_application_repository.go (1)
77-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winErrors aren't mapped to a sentinel or wrapped with context.
GetByAgentEnvreturns the rawFirst(...)error (includinggorm.ErrRecordNotFound) verbatim, andListByOrg/ListByAgent/DeleteByAgent/DeleteByAgentEnvlikewise pass througherrunwrapped. Compare withagent_kind_repository.go, which explicitly checkserrors.Is(result.Error, gorm.ErrRecordNotFound)before returning. Callers depending on a mapped not-found sentinel (rather than the raw gorm error) won't detect it correctly, and unexpected DB errors lose diagnostic context.♻️ Suggested fix for GetByAgentEnv
func (r *AIApplicationRepo) GetByAgentEnv(ctx context.Context, ouID, projectName, agentID, envName string) (*models.AIApplication, error) { var app models.AIApplication err := r.db.WithContext(ctx). Where("ou_id = ? AND project_name = ? AND agent_id = ? AND environment_name = ?", ouID, projectName, agentID, envName). First(&app).Error if err != nil { - return nil, err + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, gorm.ErrRecordNotFound + } + return nil, fmt.Errorf("failed to get AI application: %w", err) } return &app, nil }As per coding guidelines: "Map
gorm.ErrRecordNotFoundto the appropriate sentinel not-found error, wrap all other unexpected errors with context usingfmt.Errorf("...: %w", err)."🤖 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 `@agent-manager-service/repositories/ai_application_repository.go` around lines 77 - 124, The repository methods in AIApplicationRepo are returning raw GORM errors instead of the project’s not-found sentinel and context-wrapped failures. Update GetByAgentEnv, ListByOrg, ListByAgent, DeleteByAgent, and DeleteByAgentEnv to follow the same pattern as agent_kind_repository.go: detect gorm.ErrRecordNotFound and map it to the appropriate sentinel, and wrap all other errors with fmt.Errorf including the method context. Use the existing AIApplicationRepo method names to apply the fix consistently across all query/delete paths.Source: Coding guidelines
♻️ Duplicate comments (5)
agent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.go (1)
84-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame hand-edit pattern:
callsstruct field left asOrgNameforouID-renamed methods.
GetByIdentifier,GetByIdentifiers, andListrename their first parameter toouID, but the tracked-call struct field remainsOrgName, with only the comment updated to referenceouID.As per path instructions: "Do not hand-edit generated repository mocks under
repositories/repomocks/; regenerate them withmake codegen."Also applies to: 167-182, 203-218, 239-254
🤖 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 `@agent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.go` around lines 84 - 99, The mock call-tracking fields in CustomEvaluatorRepositoryMock are still using the old OrgName name even though GetByIdentifier, GetByIdentifiers, and List now take ouID, so don’t hand-edit this generated file; regenerate the repository mocks instead. Update the underlying repository interface/method signatures as needed, then run make codegen so the generated calls struct and related tracking fields are refreshed consistently across GetByIdentifier, GetByIdentifiers, List, and the other affected sections.Source: Path instructions
agent-manager-service/repositories/repomocks/apikey_repository_mock.go (1)
101-121: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame hand-edit pattern:
callsstruct field left asOrgNameforouID-renamed methods.
ListByArtifactKind,ListByArtifactKindAndEnvs, andListPermanentByArtifactKindrename their first parameter toouID, but the tracked-call struct field is stillOrgName, only the comment was updated.As per path instructions: "Do not hand-edit generated repository mocks under
repositories/repomocks/; regenerate them withmake codegen."Also applies to: 275-289, 311-328, 351-366
🤖 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 `@agent-manager-service/repositories/repomocks/apikey_repository_mock.go` around lines 101 - 121, The mock call-tracking structs for ListByArtifactKind, ListByArtifactKindAndEnvs, and ListPermanentByArtifactKind are still using OrgName even though the methods now take ouID, so the generated mock is partially hand-edited. Regenerate the repository mock with make codegen instead of editing apikey_repository_mock.go by hand, and ensure the resulting calls fields and comments consistently match the renamed ouID parameter in the affected methods.Source: Path instructions
agent-manager-service/repositories/repomocks/ai_application_repository_mock.go (1)
78-79: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame hand-edit pattern:
callsstruct field left asOrgNameforouID-renamed methods.
DeleteByAgent,DeleteByAgentEnv,GetByAgentEnv,ListByAgent, andListByOrgall rename their parameter toouID, but the corresponding tracked-call struct field staysOrgName(only the comment text changed). A truemake codegenrun would rename this field toOuID, matching the new parameter name.As per path instructions: "Do not hand-edit generated repository mocks under
repositories/repomocks/; regenerate them withmake codegen."Also applies to: 91-92, 104-105, 117-118, 128-129, 181-199, 225-247, 277-297, 325-343, 369-384
🤖 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 `@agent-manager-service/repositories/repomocks/ai_application_repository_mock.go` around lines 78 - 79, The tracked-call fields in ai_application_repository_mock are still left as OrgName after the parameter rename to ouID in methods like DeleteByAgent, DeleteByAgentEnv, GetByAgentEnv, ListByAgent, and ListByOrg. Regenerate the mock with make codegen so the generated calls struct and related method call tracking use OuID consistently instead of hand-editing the generated file.Source: Path instructions
agent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.go (1)
39-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame hand-edit pattern:
OrgNamefield not renamed despiteouIDparameter change.Consistent with the other repomocks in this PR — field/comment mismatch suggests these were hand-edited instead of regenerated via
make codegen.Based on path instructions: "Do not hand-edit generated repository mocks under
repositories/repomocks/; regenerate them withmake codegen."Also applies to: 71-71, 132-142, 406-447
🤖 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 `@agent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.go` at line 39, The mock in agent_env_config_variable_repository_mock still has a hand-edited parameter/comment mismatch where OrgName references were not updated consistently with the ouID rename; do not patch this mock manually. Regenerate the repository mocks with make codegen so ListSecretReferencesByAgentAndEnvFunc and the related mock blocks are updated consistently across the affected sections.Source: Coding guidelines
agent-manager-service/repositories/repomocks/agent_configuration_repository_mock.go (1)
21-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame hand-edit pattern as the other repomocks: field names not actually regenerated.
Across
Count,CountByAgent,CountByAgentAndType,Delete,Exists,GetByAgentID,GetByUUID,List,ListByAgent, andListByAgentAndType, the tracked-call struct fields are still namedOrgNamedespite the parameter rename toouID; only doc comments were patched. This is consistent with hand-editing rather than amake codegenregeneration.Based on path instructions: "Do not hand-edit generated repository mocks under
repositories/repomocks/; regenerate them withmake codegen."Also applies to: 65-95, 106-245, 248-701
🤖 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 `@agent-manager-service/repositories/repomocks/agent_configuration_repository_mock.go` around lines 21 - 51, The repository mock still contains hand-edited tracked-call field names and signatures instead of a full regeneration, so update the AgentConfigurationRepository mock by regenerating the file rather than patching the comments or individual methods manually. Use the mock type and its constructor/helpers in agent_configuration_repository_mock.go, including the Count, CountByAgent, CountByAgentAndType, Delete, Exists, GetByAgentID, GetByUUID, List, ListByAgent, and ListByAgentAndType entries, and run the code generation workflow so the field names and generated method stubs are consistent with the renamed ouID parameter throughout.Source: Coding guidelines
🧹 Nitpick comments (6)
agent-manager-service/mcp/tools/projects.go (1)
59-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
org_nameinput schema now that org identity comes from the token.
list_projectsandcreate_projectstill declareorg_name(Lines 60, 71), but handlers now derive the org viaresolveOUID(ctx)and ignoreinput.OrgName. Droporg_namefrom these schemas (and thelistProjectsInput/createProjectInputstructs) or mark it as ignored to avoid misleading MCP clients.🤖 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 `@agent-manager-service/mcp/tools/projects.go` around lines 59 - 76, The list_projects and create_project tools still advertise org_name even though the handlers now use resolveOUID(ctx) and ignore input.OrgName. Update the tool schemas in projects.go and the corresponding listProjectsInput/createProjectInput structs to remove org_name, or explicitly mark it as ignored, so MCP clients are not given a stale field.agent-manager-service/mcp/tools/deployments.go (1)
86-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
org_nameinput schema now that org identity comes from the token.
list_deployments,deploy_agent, andupdate_deployment_statestill declare anorg_nameproperty (Lines 87, 99, 118), but the handlers now derive the org exclusively viaresolveOUID(ctx)and ignoreinput.OrgName. Advertising an accepted-but-ignored parameter is misleading to MCP clients/LLMs. Consider droppingorg_namefrom these schemas (and the corresponding input structs) or documenting it as ignored.🤖 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 `@agent-manager-service/mcp/tools/deployments.go` around lines 86 - 124, The tool schemas for list_deployments, deploy_agent, and update_deployment_state still advertise org_name even though the handlers resolve org identity only through resolveOUID(ctx) and do not use input.OrgName. Remove org_name from the InputSchema definitions and the corresponding input structs for listDeployments, deployAgent, and updateDeploymentState, or explicitly mark it as ignored so MCP clients are not misled.agent-manager-service/mcp/tools/observability.go (1)
329-357: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winResponse field
org_namenow holds an OU ID, not an org name.
listTraces,getTraces, andgetTraceDetailspopulatereducedTraces["org_name"]/reducedTrace["org_name"]withouID(Lines 351, 413, 456). Callers that previously read this field expecting a human-readable org name will now silently receive an OU ID, which can break downstream consumers or displays relying on this key's semantics.♻️ Suggested rename to avoid confusion
- reducedTraces["org_name"] = ouID + reducedTraces["ou_id"] = ouID(apply analogously to the other two occurrences)
Also applies to: 378-419, 439-459
🤖 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 `@agent-manager-service/mcp/tools/observability.go` around lines 329 - 357, The response payload is using the key org_name to store an OU ID, which changes the meaning of that field and can break callers expecting a human-readable org name. Update listTraces and the related getTraces/getTraceDetails paths to use a more accurate key for ouID (and keep org_name only if it truly contains an org name), so the reduced trace maps built in observability.go remain semantically consistent. Make the rename consistently across the helper that builds reduced traces and any place that reads or documents this field.agent-manager-service/services/infra_resource_manager.go (1)
72-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated organization-existence validation across ~8 methods.
Every method (
GetOrganization,CreateProject,UpdateProject,ListProjects,DeleteProject,GetProject,ListOrgDeploymentPipelines,ListOrgEnvironments,GetProjectDeploymentPipeline,GetDataplanes) repeats the sames.ocClient.GetOrganization(ctx, ouID)check-and-log block. Consider extracting a shared helper (e.g.validateOrganization(ctx, ouID)) to reduce duplication and keep error/logging behavior consistent.🤖 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 `@agent-manager-service/services/infra_resource_manager.go` around lines 72 - 412, A repeated organization-existence check is duplicated across multiple infraResourceManager methods, making the logging and error handling inconsistent and hard to maintain. Extract the shared `s.ocClient.GetOrganization(ctx, ouID)` check-and-log block into a helper like `validateOrganization(ctx, ouID)` on `infraResourceManager`, and have `GetOrganization`, `CreateProject`, `UpdateProject`, `ListProjects`, `DeleteProject`, `GetProject`, `ListOrgDeploymentPipelines`, `ListOrgEnvironments`, `GetProjectDeploymentPipeline`, and `GetDataplanes` call it before their main logic. Keep the helper responsible for the existing error logging and return the fetched org so the callers can reuse it where needed.agent-manager-service/db_migrations/026_add_ou_id.go (1)
30-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap 026 in a transaction for atomicity.
027 runs inside
db.Transaction, but 026 executes DDL statements table-by-table with no transaction. A failure partway through leaves some tables withou_idand altered defaults while others are untouched, making reruns/rollbacks harder. PostgreSQL DDL is transactional, so wrapping this loop indb.Transactionmatches 027 and keeps the schema change all-or-nothing.🤖 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 `@agent-manager-service/db_migrations/026_add_ou_id.go` around lines 30 - 59, Wrap the migration logic in 026_add_ou_id.go inside db.Transaction so the ou_id column additions and orgColumn default updates are applied atomically across all tables. Use the existing Migrate function and keep the loop over orgColumnByTable and its db.Exec statements inside the transaction callback, returning any error so a mid-migration failure rolls back all schema changes instead of leaving tables partially updated.agent-manager-service/repositories/gateway_repository.go (1)
53-96: 🩺 Stability & Availability | 🔵 TrivialNone of the touched repository methods take
context.Context.
GetByOrganizationID,GetByNameAndOrgID,Delete,HasGatewayDeployments,HasGatewayAssociations,HasGatewayAssociationsOrDeployments, andListIdentityProvidersByOrgall perform DB I/O but don't accept actxparam (same pattern seen inevaluation_score_repository.goandllm_provider_repository.goin this batch). This predates the OU-ID rename, but since the guideline explicitly requires context propagation for all I/O methods, it's worth tracking as follow-up debt rather than reintroducing in every future touch of these files.Based on coding guidelines: "Every I/O method must take
context.Contextas the first parameter and propagate it; HTTP clients must useNewRequestWithContext."🤖 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 `@agent-manager-service/repositories/gateway_repository.go` around lines 53 - 96, Several GatewayRepository I/O methods are missing context propagation. Update the GatewayRepository interface so GetByOrganizationID, GetByNameAndOrgID, Delete, HasGatewayDeployments, HasGatewayAssociations, HasGatewayAssociationsOrDeployments, and ListIdentityProvidersByOrg accept context.Context as the first parameter, matching the pattern used in evaluation_score_repository.go and llm_provider_repository.go. Then update every implementation and call site to pass the ctx through to the underlying DB operations so repository I/O follows the project guideline.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 `@agent-manager-service/repositories/custom_evaluator_repository.go`:
- Around line 82-89: GetByIdentifier in CustomEvaluatorRepo is leaking
gorm.ErrRecordNotFound instead of converting it to a domain not-found sentinel.
Update the error handling in this method to use errors.Is for
gorm.ErrRecordNotFound, return the appropriate repository/domain sentinel for
that case, and wrap all other errors with contextual fmt.Errorf using the method
name. Make sure any callers can compare the sentinel with errors.Is, consistent
with the other repository methods like DeploymentRepo.GetByArtifactAndGateway.
In `@agent-manager-service/repositories/repomocks/agent_kind_repository_mock.go`:
- Line 119: Regenerate the mock instead of hand-editing it: the mocked
repository methods on AgentKindRepositoryMock have been renamed to use ouID, but
the tracked calls fields still expose OrgName, which will drift from moq output.
Update agent_kind_repository_mock.go by rerunning codegen so the generated calls
struct and all affected method stubs (DeleteKindFunc, ExistsBySourceAgentFunc,
FindVersionByImageIDInOrgFunc, GetKindFunc, and ListKindsFunc) consistently use
the regenerated OuID field name.
In
`@agent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.go`:
- Around line 82-83: The mock call-tracking struct in
env_agent_mcp_mapping_repository_mock.go is inconsistent with the renamed
Create/ouID parameter because the generated calls.Create field still uses
OrgName. Regenerate the repository mock with make codegen so the derived
call-recording type and its related references (including the CreateFunc/Create
path and any uses of calls.Create) are updated consistently, rather than
hand-editing the generated code.
In
`@agent-manager-service/repositories/repomocks/evaluation_score_repository_mock.go`:
- Around line 143-144: The call-tracking structs in
evaluation_score_repository_mock still use OrgName for the renamed ouID argument
across GetAgentTraceScores, GetMonitorID, and GetScoresByTraceID; regenerate the
mock instead of hand-editing it. Update the generated repository mock so the
calls.* fields and related assertions reflect the new ouID naming consistently,
and apply the same regeneration to the other touched repomocks rather than
manually changing comments only.
In `@agent-manager-service/repositories/repomocks/gateway_repository_mock.go`:
- Around line 243-244: The repo mock call-tracking structs still use OrgName for
methods whose org argument was renamed to ouID, so update the generated
repository mock instead of hand-editing it. Regenerate the mocks with make
codegen so the calls.* fields for Delete, GetByNameAndOrgID,
GetByOrganizationID, HasGatewayAssociations,
HasGatewayAssociationsOrDeployments, HasGatewayDeployments, and
ListIdentityProvidersByOrg are updated consistently to match the renamed
parameter and keep gateway_repository_mock in sync.
In `@agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go`:
- Around line 95-97: The repository mock still has call-tracking fields named
OrgName even though the renamed methods now use ouID, so update the generated
mock to keep the call structs consistent. Regenerate
llm_proxy_repository_mock.go with make codegen instead of hand-editing, and
ensure the calls.* structs for Count, CountByProject, CountByProvider, Create,
Delete, Exists, GetByID, List, ListByProject, ListByProvider, and Update all use
the renamed ouID field alongside the corresponding method signatures.
---
Outside diff comments:
In `@agent-manager-service/repositories/agent_config_repository.go`:
- Around line 35-44: The AgentConfigRepository methods are missing context
propagation for DB I/O. Update the interface methods Upsert, Get, and
DeleteAllByAgent to accept context.Context as the first parameter, then thread
ctx through the AgentConfigRepository implementation by using it in the r.db
calls via WithContext(ctx). Also update any callers to pass context through
consistently, matching the pattern used in agent_configuration_repository.go.
In `@agent-manager-service/repositories/ai_application_repository.go`:
- Around line 77-124: The repository methods in AIApplicationRepo are returning
raw GORM errors instead of the project’s not-found sentinel and context-wrapped
failures. Update GetByAgentEnv, ListByOrg, ListByAgent, DeleteByAgent, and
DeleteByAgentEnv to follow the same pattern as agent_kind_repository.go: detect
gorm.ErrRecordNotFound and map it to the appropriate sentinel, and wrap all
other errors with fmt.Errorf including the method context. Use the existing
AIApplicationRepo method names to apply the fix consistently across all
query/delete paths.
In `@agent-manager-service/services/infra_resource_manager.go`:
- Around line 72-425: The ocClient error paths in infraResourceManager are
returning raw errors instead of adding context, which is inconsistent with the
rest of the file. Update the affected methods GetOrganization, CreateProject,
ListProjects, DeleteProject, GetProject, GetProjectDeploymentPipeline,
CreateOrgDeploymentPipeline, UpdateOrgDeploymentPipeline, and GetDataplanes to
wrap unexpected client failures with fmt.Errorf using a descriptive message and
%w, similar to UpdateProject and DeleteOrgDeploymentPipeline. Keep the existing
logger calls, but ensure each returned error provides operation-specific context
and preserves the original error for callers.
---
Duplicate comments:
In
`@agent-manager-service/repositories/repomocks/agent_configuration_repository_mock.go`:
- Around line 21-51: The repository mock still contains hand-edited tracked-call
field names and signatures instead of a full regeneration, so update the
AgentConfigurationRepository mock by regenerating the file rather than patching
the comments or individual methods manually. Use the mock type and its
constructor/helpers in agent_configuration_repository_mock.go, including the
Count, CountByAgent, CountByAgentAndType, Delete, Exists, GetByAgentID,
GetByUUID, List, ListByAgent, and ListByAgentAndType entries, and run the code
generation workflow so the field names and generated method stubs are consistent
with the renamed ouID parameter throughout.
In
`@agent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.go`:
- Line 39: The mock in agent_env_config_variable_repository_mock still has a
hand-edited parameter/comment mismatch where OrgName references were not updated
consistently with the ouID rename; do not patch this mock manually. Regenerate
the repository mocks with make codegen so ListSecretReferencesByAgentAndEnvFunc
and the related mock blocks are updated consistently across the affected
sections.
In
`@agent-manager-service/repositories/repomocks/ai_application_repository_mock.go`:
- Around line 78-79: The tracked-call fields in ai_application_repository_mock
are still left as OrgName after the parameter rename to ouID in methods like
DeleteByAgent, DeleteByAgentEnv, GetByAgentEnv, ListByAgent, and ListByOrg.
Regenerate the mock with make codegen so the generated calls struct and related
method call tracking use OuID consistently instead of hand-editing the generated
file.
In `@agent-manager-service/repositories/repomocks/apikey_repository_mock.go`:
- Around line 101-121: The mock call-tracking structs for ListByArtifactKind,
ListByArtifactKindAndEnvs, and ListPermanentByArtifactKind are still using
OrgName even though the methods now take ouID, so the generated mock is
partially hand-edited. Regenerate the repository mock with make codegen instead
of editing apikey_repository_mock.go by hand, and ensure the resulting calls
fields and comments consistently match the renamed ouID parameter in the
affected methods.
In
`@agent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.go`:
- Around line 84-99: The mock call-tracking fields in
CustomEvaluatorRepositoryMock are still using the old OrgName name even though
GetByIdentifier, GetByIdentifiers, and List now take ouID, so don’t hand-edit
this generated file; regenerate the repository mocks instead. Update the
underlying repository interface/method signatures as needed, then run make
codegen so the generated calls struct and related tracking fields are refreshed
consistently across GetByIdentifier, GetByIdentifiers, List, and the other
affected sections.
---
Nitpick comments:
In `@agent-manager-service/db_migrations/026_add_ou_id.go`:
- Around line 30-59: Wrap the migration logic in 026_add_ou_id.go inside
db.Transaction so the ou_id column additions and orgColumn default updates are
applied atomically across all tables. Use the existing Migrate function and keep
the loop over orgColumnByTable and its db.Exec statements inside the transaction
callback, returning any error so a mid-migration failure rolls back all schema
changes instead of leaving tables partially updated.
In `@agent-manager-service/mcp/tools/deployments.go`:
- Around line 86-124: The tool schemas for list_deployments, deploy_agent, and
update_deployment_state still advertise org_name even though the handlers
resolve org identity only through resolveOUID(ctx) and do not use input.OrgName.
Remove org_name from the InputSchema definitions and the corresponding input
structs for listDeployments, deployAgent, and updateDeploymentState, or
explicitly mark it as ignored so MCP clients are not misled.
In `@agent-manager-service/mcp/tools/observability.go`:
- Around line 329-357: The response payload is using the key org_name to store
an OU ID, which changes the meaning of that field and can break callers
expecting a human-readable org name. Update listTraces and the related
getTraces/getTraceDetails paths to use a more accurate key for ouID (and keep
org_name only if it truly contains an org name), so the reduced trace maps built
in observability.go remain semantically consistent. Make the rename consistently
across the helper that builds reduced traces and any place that reads or
documents this field.
In `@agent-manager-service/mcp/tools/projects.go`:
- Around line 59-76: The list_projects and create_project tools still advertise
org_name even though the handlers now use resolveOUID(ctx) and ignore
input.OrgName. Update the tool schemas in projects.go and the corresponding
listProjectsInput/createProjectInput structs to remove org_name, or explicitly
mark it as ignored, so MCP clients are not given a stale field.
In `@agent-manager-service/repositories/gateway_repository.go`:
- Around line 53-96: Several GatewayRepository I/O methods are missing context
propagation. Update the GatewayRepository interface so GetByOrganizationID,
GetByNameAndOrgID, Delete, HasGatewayDeployments, HasGatewayAssociations,
HasGatewayAssociationsOrDeployments, and ListIdentityProvidersByOrg accept
context.Context as the first parameter, matching the pattern used in
evaluation_score_repository.go and llm_provider_repository.go. Then update every
implementation and call site to pass the ctx through to the underlying DB
operations so repository I/O follows the project guideline.
In `@agent-manager-service/services/infra_resource_manager.go`:
- Around line 72-412: A repeated organization-existence check is duplicated
across multiple infraResourceManager methods, making the logging and error
handling inconsistent and hard to maintain. Extract the shared
`s.ocClient.GetOrganization(ctx, ouID)` check-and-log block into a helper like
`validateOrganization(ctx, ouID)` on `infraResourceManager`, and have
`GetOrganization`, `CreateProject`, `UpdateProject`, `ListProjects`,
`DeleteProject`, `GetProject`, `ListOrgDeploymentPipelines`,
`ListOrgEnvironments`, `GetProjectDeploymentPipeline`, and `GetDataplanes` call
it before their main logic. Keep the helper responsible for the existing error
logging and return the fetched org so the callers can reuse it where needed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c612ae0-27c0-48cf-9d68-864056bab88a
📒 Files selected for processing (144)
agent-manager-service/clients/openchoreosvc/client/builds.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/generic-workflows.goagent-manager-service/clients/openchoreosvc/client/git_secrets.goagent-manager-service/clients/openchoreosvc/client/infrastructure.goagent-manager-service/clients/openchoreosvc/client/projects.goagent-manager-service/clients/openchoreosvc/client/secret_references.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/agent_apikey_controller.goagent-manager-service/controllers/agent_build_options_controller.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/agent_controller.goagent-manager-service/controllers/agent_kind_controller.goagent-manager-service/controllers/agent_token_controller.goagent-manager-service/controllers/catalog_controller.goagent-manager-service/controllers/environment_controller.goagent-manager-service/controllers/evaluator_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/gateway_identity_provider_controller.goagent-manager-service/controllers/gateway_internal_controller.goagent-manager-service/controllers/git_secret_controller.goagent-manager-service/controllers/infra_resource_controller.goagent-manager-service/controllers/llm_controller.goagent-manager-service/controllers/llm_deployment_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/llm_proxy_deployment_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/controllers/monitor_controller.goagent-manager-service/controllers/monitor_scores_controller.goagent-manager-service/controllers/websocket_controller.goagent-manager-service/db_migrations/026_add_ou_id.goagent-manager-service/db_migrations/027_swap_org_constraints_to_ou_id.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/mcp/handlers/agent_handler.goagent-manager-service/mcp/handlers/build_handler.goagent-manager-service/mcp/handlers/deployment_handler.goagent-manager-service/mcp/handlers/observability_handler.goagent-manager-service/mcp/handlers/project_handler.goagent-manager-service/mcp/tools/agents.goagent-manager-service/mcp/tools/agents_specs_test.goagent-manager-service/mcp/tools/builds.goagent-manager-service/mcp/tools/builds_specs_test.goagent-manager-service/mcp/tools/deployments.goagent-manager-service/mcp/tools/deployments_specs_test.goagent-manager-service/mcp/tools/helpers.goagent-manager-service/mcp/tools/mock_test.goagent-manager-service/mcp/tools/observability.goagent-manager-service/mcp/tools/observability_specs_test.goagent-manager-service/mcp/tools/projects.goagent-manager-service/mcp/tools/projects_specs_test.goagent-manager-service/mcp/tools/setup_test.goagent-manager-service/mcp/tools/types.goagent-manager-service/middleware/authorization.goagent-manager-service/middleware/jwtassertion/auth.goagent-manager-service/middleware/orgresolver.goagent-manager-service/models/agent_config.goagent-manager-service/models/agent_configuration.goagent-manager-service/models/agent_kind.goagent-manager-service/models/ai_application.goagent-manager-service/models/apikey.goagent-manager-service/models/artifact.goagent-manager-service/models/association_mapping.goagent-manager-service/models/catalog.goagent-manager-service/models/custom_evaluator.goagent-manager-service/models/deployment.goagent-manager-service/models/gateway.goagent-manager-service/models/llm_provider_template.goagent-manager-service/models/monitor.goagent-manager-service/repositories/agent_config_repository.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/agent_env_config_variable_repository.goagent-manager-service/repositories/agent_kind_repository.goagent-manager-service/repositories/ai_application_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/artifact_repository.goagent-manager-service/repositories/catalog_repository.goagent-manager-service/repositories/custom_evaluator_repository.goagent-manager-service/repositories/deployment_repository.goagent-manager-service/repositories/env_agent_mcp_mapping_repository.goagent-manager-service/repositories/evaluation_score_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_provider_repository.goagent-manager-service/repositories/llm_provider_template_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/mcp_proxy_repository.goagent-manager-service/repositories/monitor_repository.goagent-manager-service/repositories/org_publisher_credential_repository.goagent-manager-service/repositories/repomocks/agent_config_repository_mock.goagent-manager-service/repositories/repomocks/agent_configuration_repository_mock.goagent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.goagent-manager-service/repositories/repomocks/agent_kind_repository_mock.goagent-manager-service/repositories/repomocks/ai_application_repository_mock.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.goagent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.goagent-manager-service/repositories/repomocks/evaluation_score_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/repositories/repomocks/monitor_repository_mock.goagent-manager-service/repositories/repomocks/org_publisher_credential_repository_mock.goagent-manager-service/services/agent_api_artifact.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_kind_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/agent_token_manager.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/catalog_service.goagent-manager-service/services/catalog_service_unit_test.goagent-manager-service/services/environment_service.goagent-manager-service/services/environment_service_test.goagent-manager-service/services/evaluator_manager.goagent-manager-service/services/gateway_config_applier.goagent-manager-service/services/gateway_internal_service.goagent-manager-service/services/git_credentials_service.goagent-manager-service/services/git_secret_service.goagent-manager-service/services/infra_resource_manager.goagent-manager-service/services/infra_resource_manager_test.goagent-manager-service/services/infra_resource_manager_unit_test.goagent-manager-service/services/llm_deployment_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_provider_template_service.goagent-manager-service/services/llm_proxy_deployment_service.goagent-manager-service/services/llm_proxy_deployment_service_test.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/llm_proxy_service.goagent-manager-service/services/llm_template_seeder.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/monitor_executor.goagent-manager-service/services/monitor_manager.goagent-manager-service/services/monitor_scheduler.goagent-manager-service/services/monitor_scheduler_unit_test.goagent-manager-service/services/monitor_scores_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/services/publisher_credential_provisioner.goagent-manager-service/services/repository_service.goagent-manager-service/services/repository_service_unit_test.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
Comment on lines
+82
to
89
| func (r *CustomEvaluatorRepo) GetByIdentifier(ouID, identifier string) (*models.CustomEvaluator, error) { | ||
| var evaluator models.CustomEvaluator | ||
| if err := r.db.Where("org_name = ? AND identifier = ? AND deleted_at IS NULL", orgName, identifier). | ||
| if err := r.db.Where("ou_id = ? AND identifier = ? AND deleted_at IS NULL", ouID, identifier). | ||
| First(&evaluator).Error; err != nil { | ||
| return nil, err | ||
| } | ||
| return &evaluator, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Map gorm.ErrRecordNotFound to a domain sentinel instead of leaking it.
GetByIdentifier returns the raw gorm error directly, leaking gorm.ErrRecordNotFound past the repository boundary. Other repositories in this PR (e.g., DeploymentRepo.GetByArtifactAndGateway) correctly translate it to a domain sentinel and wrap other errors with fmt.Errorf("...: %w", err).
♻️ Proposed fix
func (r *CustomEvaluatorRepo) GetByIdentifier(ouID, identifier string) (*models.CustomEvaluator, error) {
var evaluator models.CustomEvaluator
if err := r.db.Where("ou_id = ? AND identifier = ? AND deleted_at IS NULL", ouID, identifier).
First(&evaluator).Error; err != nil {
- return nil, err
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return nil, utils.ErrCustomEvaluatorNotFound
+ }
+ return nil, fmt.Errorf("failed to get custom evaluator by identifier: %w", err)
}
return &evaluator, nil
}(adjust the sentinel name to whatever is already defined for this domain, or add one if missing.)
As per coding guidelines: "Map gorm.ErrRecordNotFound to the appropriate sentinel not-found error, wrap all other unexpected errors with context using fmt.Errorf(\"...: %w\", err), and compare sentinels with errors.Is rather than == or string matching."
🤖 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 `@agent-manager-service/repositories/custom_evaluator_repository.go` around
lines 82 - 89, GetByIdentifier in CustomEvaluatorRepo is leaking
gorm.ErrRecordNotFound instead of converting it to a domain not-found sentinel.
Update the error handling in this method to use errors.Is for
gorm.ErrRecordNotFound, return the appropriate repository/domain sentinel for
that case, and wrap all other errors with contextual fmt.Errorf using the method
name. Make sure any callers can compare the sentinel with errors.Is, consistent
with the other repository methods like DeploymentRepo.GetByArtifactAndGateway.
Source: Coding guidelines
hanzjk
force-pushed
the
workflow-fix
branch
2 times, most recently
from
bcc70ee to
27dc057
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/repositories/monitor_repository.go (1)
90-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winErrors aren't wrapped with context and
gorm.ErrRecordNotFoundisn't mapped to a sentinel.
GetMonitorByNamereturns the rawFirst()error directly (nofmt.Errorf("...: %w", err)wrap, no mapping ofgorm.ErrRecordNotFoundto a domain sentinel), andListMonitorsByAgent,ListMonitorsByAgentEnvironment, andFindActiveMonitorsByEvaluatorIdentifiersimilarly propagate raw GORM errors. This leaks GORM-specific error types up the stack instead of a stable, comparable sentinel, and loses debugging context.As per coding guidelines: "Map
gorm.ErrRecordNotFoundto the appropriate sentinel not-found error, wrap all other unexpected errors with context usingfmt.Errorf(\"...: %w\", err), and compare sentinels witherrors.Is."🐛 Example fix for GetMonitorByName
func (r *MonitorRepo) GetMonitorByName(ouID, projectName, agentName, monitorName string) (*models.Monitor, error) { var monitor models.Monitor if err := r.db.Where("name = ? AND ou_id = ? AND project_name = ? AND agent_name = ?", monitorName, ouID, projectName, agentName).First(&monitor).Error; err != nil { - return nil, err + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, ErrMonitorNotFound + } + return nil, fmt.Errorf("get monitor by name: %w", err) } return &monitor, nil }Also applies to: 109-124, 150-157
Source: Coding guidelines
agent-manager-service/clients/openchoreosvc/client/components.go (1)
1216-1220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the trait index after appending new traits.
If
traitRequestscontains the same new trait type twice, the second entry is appended again instead of replacing the first appended trait.Proposed fix
if idx, exists := existingTraitIdx[string(req.TraitType)]; exists { traits[idx] = newTrait } else { + existingTraitIdx[string(req.TraitType)] = len(traits) traits = append(traits, newTrait) }🤖 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 `@agent-manager-service/clients/openchoreosvc/client/components.go` around lines 1216 - 1220, The trait upsert logic in the existingTraitIdx/traits append block does not refresh the index after adding a new trait, so duplicate new trait types in traitRequests get appended multiple times. After appending newTrait in the else branch, update existingTraitIdx for that trait type to the new position so later duplicates replace the first appended entry instead of adding another. Keep the replacement behavior in the if branch unchanged.
🧹 Nitpick comments (2)
agent-manager-service/models/agent_thunder_client.go (1)
53-53: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRename the model field to reflect OU-ID semantics.
OrgNamenow persists toou_id, which makes future repository/service code easy to misuse during this tenancy migration. PreferOUID/OuIDconsistently, or add a compatibility comment if the rename is intentionally deferred.🤖 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 `@agent-manager-service/models/agent_thunder_client.go` at line 53, The model field currently named OrgName is mapped to the ou_id column, which is misleading and can cause misuse in repository/service code during the tenancy migration. Rename the field in AgentThunderClient to OUID or OuID so its name matches the persisted semantics, and update any related references or add an explicit compatibility comment if the rename must be deferred.agent-manager-service/repositories/agent_thunder_client_repository.go (1)
169-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ClaimForAttemptusestime.Now()directly instead of accepting an explicitnow.
FindDueacceptsnow time.Timefor determinism/testability, butClaimForAttemptcomputesstaleBeforefromtime.Now()internally (line 186), breaking that consistency and making the stale-claim boundary harder to unit test deterministically.♻️ Suggested consistency fix
- ClaimForAttempt(ctx context.Context, id uuid.UUID) (claimed bool, err error) + ClaimForAttempt(ctx context.Context, id uuid.UUID, now time.Time) (claimed bool, err error)-func (r *AgentThunderClientRepo) ClaimForAttempt(ctx context.Context, id uuid.UUID) (bool, error) { - staleBefore := time.Now().Add(-inProgressStaleThreshold) +func (r *AgentThunderClientRepo) ClaimForAttempt(ctx context.Context, id uuid.UUID, now time.Time) (bool, error) { + staleBefore := now.Add(-inProgressStaleThreshold)🤖 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 `@agent-manager-service/repositories/agent_thunder_client_repository.go` around lines 169 - 199, `ClaimForAttempt` should follow the same deterministic time handling as `FindDue` by not calling `time.Now()` internally. Update `AgentThunderClientRepo.ClaimForAttempt` to accept an explicit `now time.Time` parameter and compute `staleBefore` from that value, then adjust its call sites accordingly so the stale-claim boundary can be tested consistently alongside `FindDue`.
🤖 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 `@agent-manager-service/clients/openchoreosvc/client/components.go`:
- Around line 1135-1137: The ComponentExists method is deriving the namespace
twice by calling c.namespaceFor(ouID) and then passing that value into
GetComponent, which already performs namespaceFor(ouID) internally. Update
ComponentExists in openChoreoClient to pass the original ouID to GetComponent
instead of namespaceName, and keep the existing existence check logic unchanged
so the lookup uses the correct namespace path.
In `@agent-manager-service/clients/openchoreosvc/client/deployments.go`:
- Around line 700-709: `GetDeployments` is deriving the namespace twice before
calling migrated client methods. Update the `openChoreoClient.GetDeployments`
flow so `GetProjectDeploymentPipeline` and `ListEnvironments` are called with
`ouID` instead of `namespaceName`, since those methods now derive their own
namespace internally. Keep the existing `namespaceFor` usage only where a raw
namespace is actually required, and verify any other nested `openChoreoClient`
calls follow the same `ouID`-based contract.
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 948-950: The API-key create and rotate handlers are decoding
request bodies directly with json.NewDecoder without the same 1MB MaxBytesReader
protection used elsewhere in agent_configuration_controller.go. Update the
create path and both rotate decode paths to wrap r.Body with http.MaxBytesReader
before decoding into spec.CreateLLMAPIKeyRequest, using the same pattern as the
other write handlers in this controller.
- Around line 866-878: The per-config API key handlers still use orgName from
the request path as the tenant key, but they need to be switched to the resolved
ouID flow used by the other multi-tenant handlers. Update configEnvAPIKeyParams
and each List/Create/Rotate/Revoke*ConfigAPIKey call-site to resolve and
validate the caller organization against the target resource, perform the
missing-tenant check, and pass ouID instead of orgName into the service methods.
Rename the local variable at each affected handler to ouID so the intent matches
the Thunder OU ID scoping.
- Line 76: The agent_configuration_controller handlers are using
middleware.OUIDFromRequest without verifying whether it returned an empty tenant
key, so missing org context can flow into service/persistence calls. Add a local
checked helper or change OUIDFromRequest to return a (string, bool) so each
affected handler rejects the request early when the OU ID is missing. Update the
call sites in the controller methods that currently assign ouID from
middleware.OUIDFromRequest to fail before any service access when tenant
identity is absent.
In `@agent-manager-service/db_migrations/029_add_ou_id.go`:
- Around line 54-55: The migration loop in the statement execution path is
returning the raw GORM error, which loses useful context. Update the
`db.Exec(stmt).Error` failure handling in the migration function to wrap the
error with migration/table/statement context using `fmt.Errorf("...: %w", err)`,
so the returned error clearly identifies which ALTER statement failed while
preserving the original error.
In `@agent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.go`:
- Around line 29-35: Migration030 currently assumes ou_id is already populated,
but it is registered unconditionally and can swap constraints against rows still
carrying an empty tenant value. Update migration030.Migrate to either backfill
ou_id from the authoritative org-to-OU source before calling the constraint swap
logic, or explicitly detect empty ou_id rows and abort with an error before any
schema changes. Use the migration030 and Migrate flow to locate the fix, and
make sure the swap only proceeds when every row has a valid ou_id.
In `@agent-manager-service/mcp/tools/agents_specs_test.go`:
- Line 43: The specs in the agent-manager MCP tools tests still drive OU ID from
the old org_name tool input, so the new context/JWT-claims path is never
exercised. Update the test setup around the relevant cases in
agents_specs_test.go by removing stale org_name expectations and populating the
OU ID via the tool context/JWT claims instead. Keep the assertions aligned with
the new behavior by validating the resolved OU ID from the tool execution path
in the affected test cases.
---
Outside diff comments:
In `@agent-manager-service/clients/openchoreosvc/client/components.go`:
- Around line 1216-1220: The trait upsert logic in the existingTraitIdx/traits
append block does not refresh the index after adding a new trait, so duplicate
new trait types in traitRequests get appended multiple times. After appending
newTrait in the else branch, update existingTraitIdx for that trait type to the
new position so later duplicates replace the first appended entry instead of
adding another. Keep the replacement behavior in the if branch unchanged.
---
Nitpick comments:
In `@agent-manager-service/models/agent_thunder_client.go`:
- Line 53: The model field currently named OrgName is mapped to the ou_id
column, which is misleading and can cause misuse in repository/service code
during the tenancy migration. Rename the field in AgentThunderClient to OUID or
OuID so its name matches the persisted semantics, and update any related
references or add an explicit compatibility comment if the rename must be
deferred.
In `@agent-manager-service/repositories/agent_thunder_client_repository.go`:
- Around line 169-199: `ClaimForAttempt` should follow the same deterministic
time handling as `FindDue` by not calling `time.Now()` internally. Update
`AgentThunderClientRepo.ClaimForAttempt` to accept an explicit `now time.Time`
parameter and compute `staleBefore` from that value, then adjust its call sites
accordingly so the stale-claim boundary can be tested consistently alongside
`FindDue`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f25dcf9-59c6-4a93-8d9d-85037182bbe3
📒 Files selected for processing (148)
agent-manager-service/clients/clientmocks/openchoreo_client_fake.goagent-manager-service/clients/openchoreosvc/client/builds.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/generic-workflows.goagent-manager-service/clients/openchoreosvc/client/git_secrets.goagent-manager-service/clients/openchoreosvc/client/infrastructure.goagent-manager-service/clients/openchoreosvc/client/projects.goagent-manager-service/clients/openchoreosvc/client/secret_references.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/agent_apikey_controller.goagent-manager-service/controllers/agent_build_options_controller.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/agent_controller.goagent-manager-service/controllers/agent_kind_controller.goagent-manager-service/controllers/agent_token_controller.goagent-manager-service/controllers/catalog_controller.goagent-manager-service/controllers/environment_controller.goagent-manager-service/controllers/evaluator_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/gateway_identity_provider_controller.goagent-manager-service/controllers/gateway_internal_controller.goagent-manager-service/controllers/git_secret_controller.goagent-manager-service/controllers/infra_resource_controller.goagent-manager-service/controllers/llm_controller.goagent-manager-service/controllers/llm_deployment_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/llm_proxy_deployment_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/controllers/monitor_controller.goagent-manager-service/controllers/monitor_scores_controller.goagent-manager-service/controllers/websocket_controller.goagent-manager-service/db_migrations/029_add_ou_id.goagent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/mcp/handlers/agent_handler.goagent-manager-service/mcp/handlers/build_handler.goagent-manager-service/mcp/handlers/deployment_handler.goagent-manager-service/mcp/handlers/observability_handler.goagent-manager-service/mcp/handlers/project_handler.goagent-manager-service/mcp/tools/agents.goagent-manager-service/mcp/tools/agents_specs_test.goagent-manager-service/mcp/tools/builds.goagent-manager-service/mcp/tools/builds_specs_test.goagent-manager-service/mcp/tools/deployments.goagent-manager-service/mcp/tools/deployments_specs_test.goagent-manager-service/mcp/tools/helpers.goagent-manager-service/mcp/tools/mock_test.goagent-manager-service/mcp/tools/observability.goagent-manager-service/mcp/tools/observability_specs_test.goagent-manager-service/mcp/tools/projects.goagent-manager-service/mcp/tools/projects_specs_test.goagent-manager-service/mcp/tools/setup_test.goagent-manager-service/mcp/tools/types.goagent-manager-service/middleware/authorization.goagent-manager-service/middleware/jwtassertion/auth.goagent-manager-service/middleware/orgresolver.goagent-manager-service/models/agent_config.goagent-manager-service/models/agent_configuration.goagent-manager-service/models/agent_kind.goagent-manager-service/models/agent_thunder_client.goagent-manager-service/models/ai_application.goagent-manager-service/models/apikey.goagent-manager-service/models/artifact.goagent-manager-service/models/association_mapping.goagent-manager-service/models/catalog.goagent-manager-service/models/custom_evaluator.goagent-manager-service/models/deployment.goagent-manager-service/models/gateway.goagent-manager-service/models/llm_provider_template.goagent-manager-service/models/monitor.goagent-manager-service/repositories/agent_config_repository.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/agent_env_config_variable_repository.goagent-manager-service/repositories/agent_kind_repository.goagent-manager-service/repositories/agent_thunder_client_repository.goagent-manager-service/repositories/ai_application_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/artifact_repository.goagent-manager-service/repositories/catalog_repository.goagent-manager-service/repositories/custom_evaluator_repository.goagent-manager-service/repositories/deployment_repository.goagent-manager-service/repositories/env_agent_mcp_mapping_repository.goagent-manager-service/repositories/evaluation_score_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_provider_repository.goagent-manager-service/repositories/llm_provider_template_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/mcp_proxy_repository.goagent-manager-service/repositories/monitor_repository.goagent-manager-service/repositories/org_publisher_credential_repository.goagent-manager-service/repositories/repomocks/agent_config_repository_mock.goagent-manager-service/repositories/repomocks/agent_configuration_repository_mock.goagent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.goagent-manager-service/repositories/repomocks/agent_kind_repository_mock.goagent-manager-service/repositories/repomocks/agent_thunder_client_repository_mock.goagent-manager-service/repositories/repomocks/ai_application_repository_mock.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.goagent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.goagent-manager-service/repositories/repomocks/evaluation_score_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/repositories/repomocks/monitor_repository_mock.goagent-manager-service/repositories/repomocks/org_publisher_credential_repository_mock.goagent-manager-service/services/agent_api_artifact.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_kind_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/agent_thunder_provisioning_service.goagent-manager-service/services/agent_token_manager.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/catalog_service.goagent-manager-service/services/catalog_service_unit_test.goagent-manager-service/services/environment_service.goagent-manager-service/services/evaluator_manager.goagent-manager-service/services/gateway_config_applier.goagent-manager-service/services/gateway_internal_service.goagent-manager-service/services/git_credentials_service.goagent-manager-service/services/git_secret_service.goagent-manager-service/services/infra_resource_manager.goagent-manager-service/services/infra_resource_manager_test.goagent-manager-service/services/infra_resource_manager_unit_test.goagent-manager-service/services/llm_deployment_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_provider_template_service.goagent-manager-service/services/llm_proxy_deployment_service.goagent-manager-service/services/llm_proxy_deployment_service_test.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/llm_proxy_service.goagent-manager-service/services/llm_template_seeder.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/monitor_executor.goagent-manager-service/services/monitor_manager.goagent-manager-service/services/monitor_scheduler.goagent-manager-service/services/monitor_scheduler_unit_test.goagent-manager-service/services/monitor_scores_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/services/publisher_credential_provisioner.goagent-manager-service/services/repository_service.goagent-manager-service/services/repository_service_unit_test.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
💤 Files with no reviewable changes (47)
- agent-manager-service/services/repository_service.go
- agent-manager-service/services/gateway_config_applier.go
- agent-manager-service/services/infra_resource_manager_test.go
- agent-manager-service/services/mcp_proxy_service.go
- agent-manager-service/services/agent_token_manager.go
- agent-manager-service/services/monitor_scheduler.go
- agent-manager-service/services/agent_api_artifact.go
- agent-manager-service/services/repository_service_unit_test.go
- agent-manager-service/repositories/repomocks/org_publisher_credential_repository_mock.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/services/git_credentials_service.go
- agent-manager-service/services/infra_resource_manager_unit_test.go
- agent-manager-service/services/gateway_internal_service.go
- agent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.go
- agent-manager-service/services/monitor_scheduler_unit_test.go
- agent-manager-service/services/llm_proxy_deployment_service_test.go
- agent-manager-service/services/monitor_executor.go
- agent-manager-service/services/monitor_scores_service.go
- agent-manager-service/services/git_secret_service.go
- agent-manager-service/services/llm_template_seeder.go
- agent-manager-service/services/ai_application_service.go
- agent-manager-service/services/catalog_service_unit_test.go
- agent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.go
- agent-manager-service/services/evaluator_manager.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/services/publisher_credential_provisioner.go
- agent-manager-service/services/llm_provider_template_service.go
- agent-manager-service/services/mcp_proxy_deployment.go
- agent-manager-service/repositories/repomocks/ai_application_repository_mock.go
- agent-manager-service/services/llm_proxy_service.go
- agent-manager-service/services/llm_deployment_service.go
- agent-manager-service/services/platform_gateway_service.go
- agent-manager-service/services/environment_service.go
- agent-manager-service/services/agent_thunder_provisioning_service.go
- agent-manager-service/repositories/repomocks/evaluation_score_repository_mock.go
- agent-manager-service/services/llm_proxy_provisioner.go
- agent-manager-service/services/catalog_service.go
- agent-manager-service/services/agent_kind_service.go
- agent-manager-service/repositories/repomocks/monitor_repository_mock.go
- agent-manager-service/services/agent_apikey_service.go
- agent-manager-service/services/llm_proxy_deployment_service.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/services/infra_resource_manager.go
- agent-manager-service/services/monitor_manager.go
✅ Files skipped from review due to trivial changes (11)
- agent-manager-service/mcp/tools/projects_specs_test.go
- agent-manager-service/controllers/agent_build_options_controller.go
- agent-manager-service/mcp/handlers/build_handler.go
- agent-manager-service/mcp/tools/builds_specs_test.go
- agent-manager-service/mcp/handlers/deployment_handler.go
- agent-manager-service/mcp/tools/deployments_specs_test.go
- agent-manager-service/mcp/tools/observability_specs_test.go
- agent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.go
- agent-manager-service/repositories/repomocks/agent_config_repository_mock.go
- agent-manager-service/mcp/tools/types.go
- agent-manager-service/repositories/repomocks/agent_kind_repository_mock.go
🚧 Files skipped from review as they are similar to previous changes (71)
- agent-manager-service/models/ai_application.go
- agent-manager-service/models/llm_provider_template.go
- agent-manager-service/config/config.go
- agent-manager-service/models/deployment.go
- agent-manager-service/models/custom_evaluator.go
- agent-manager-service/repositories/env_agent_mcp_mapping_repository.go
- agent-manager-service/models/catalog.go
- agent-manager-service/models/artifact.go
- agent-manager-service/models/agent_config.go
- agent-manager-service/repositories/llm_provider_repository.go
- agent-manager-service/models/agent_configuration.go
- agent-manager-service/models/apikey.go
- agent-manager-service/repositories/agent_config_repository.go
- agent-manager-service/models/association_mapping.go
- agent-manager-service/mcp/handlers/project_handler.go
- agent-manager-service/clients/openchoreosvc/client/generic-workflows.go
- agent-manager-service/repositories/agent_env_config_variable_repository.go
- agent-manager-service/repositories/artifact_repository.go
- agent-manager-service/middleware/jwtassertion/auth.go
- agent-manager-service/controllers/catalog_controller.go
- agent-manager-service/controllers/gateway_internal_controller.go
- agent-manager-service/models/gateway.go
- agent-manager-service/controllers/agent_token_controller.go
- agent-manager-service/mcp/tools/helpers.go
- agent-manager-service/clients/openchoreosvc/client/secret_references.go
- agent-manager-service/mcp/handlers/agent_handler.go
- agent-manager-service/clients/openchoreosvc/client/git_secrets.go
- agent-manager-service/controllers/git_secret_controller.go
- agent-manager-service/repositories/apikey_repository.go
- agent-manager-service/models/agent_kind.go
- agent-manager-service/middleware/orgresolver.go
- agent-manager-service/models/monitor.go
- agent-manager-service/mcp/tools/builds.go
- agent-manager-service/repositories/llm_provider_template_repository.go
- agent-manager-service/controllers/environment_controller.go
- agent-manager-service/config/config_loader.go
- agent-manager-service/repositories/ai_application_repository.go
- agent-manager-service/repositories/evaluation_score_repository.go
- agent-manager-service/controllers/agent_kind_controller.go
- agent-manager-service/repositories/org_publisher_credential_repository.go
- agent-manager-service/clients/openchoreosvc/client/builds.go
- agent-manager-service/repositories/catalog_repository.go
- agent-manager-service/clients/openchoreosvc/client/client.go
- agent-manager-service/controllers/gateway_identity_provider_controller.go
- agent-manager-service/repositories/custom_evaluator_repository.go
- agent-manager-service/controllers/websocket_controller.go
- agent-manager-service/mcp/tools/projects.go
- agent-manager-service/mcp/tools/deployments.go
- agent-manager-service/controllers/agent_apikey_controller.go
- agent-manager-service/mcp/tools/mock_test.go
- agent-manager-service/middleware/authorization.go
- agent-manager-service/controllers/evaluator_controller.go
- agent-manager-service/mcp/tools/observability.go
- agent-manager-service/controllers/monitor_scores_controller.go
- agent-manager-service/repositories/agent_kind_repository.go
- agent-manager-service/repositories/gateway_repository.go
- agent-manager-service/mcp/tools/agents.go
- agent-manager-service/controllers/monitor_controller.go
- agent-manager-service/mcp/tools/setup_test.go
- agent-manager-service/controllers/infra_resource_controller.go
- agent-manager-service/repositories/agent_configuration_repository.go
- agent-manager-service/clients/openchoreosvc/client/projects.go
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- agent-manager-service/controllers/llm_deployment_controller.go
- agent-manager-service/controllers/llm_proxy_deployment_controller.go
- agent-manager-service/repositories/deployment_repository.go
- agent-manager-service/repositories/mcp_proxy_repository.go
- agent-manager-service/repositories/llm_proxy_repository.go
- agent-manager-service/clients/openchoreosvc/client/infrastructure.go
- agent-manager-service/controllers/agent_controller.go
- agent-manager-service/controllers/gateway_controller.go
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/repositories/monitor_repository.go (1)
90-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winErrors aren't wrapped with context and
gorm.ErrRecordNotFoundisn't mapped to a sentinel.
GetMonitorByNamereturns the rawFirst()error directly (nofmt.Errorf("...: %w", err)wrap, no mapping ofgorm.ErrRecordNotFoundto a domain sentinel), andListMonitorsByAgent,ListMonitorsByAgentEnvironment, andFindActiveMonitorsByEvaluatorIdentifiersimilarly propagate raw GORM errors. This leaks GORM-specific error types up the stack instead of a stable, comparable sentinel, and loses debugging context.As per coding guidelines: "Map
gorm.ErrRecordNotFoundto the appropriate sentinel not-found error, wrap all other unexpected errors with context usingfmt.Errorf(\"...: %w\", err), and compare sentinels witherrors.Is."🐛 Example fix for GetMonitorByName
func (r *MonitorRepo) GetMonitorByName(ouID, projectName, agentName, monitorName string) (*models.Monitor, error) { var monitor models.Monitor if err := r.db.Where("name = ? AND ou_id = ? AND project_name = ? AND agent_name = ?", monitorName, ouID, projectName, agentName).First(&monitor).Error; err != nil { - return nil, err + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, ErrMonitorNotFound + } + return nil, fmt.Errorf("get monitor by name: %w", err) } return &monitor, nil }Also applies to: 109-124, 150-157
Source: Coding guidelines
agent-manager-service/clients/openchoreosvc/client/components.go (1)
1216-1220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the trait index after appending new traits.
If
traitRequestscontains the same new trait type twice, the second entry is appended again instead of replacing the first appended trait.Proposed fix
if idx, exists := existingTraitIdx[string(req.TraitType)]; exists { traits[idx] = newTrait } else { + existingTraitIdx[string(req.TraitType)] = len(traits) traits = append(traits, newTrait) }🤖 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 `@agent-manager-service/clients/openchoreosvc/client/components.go` around lines 1216 - 1220, The trait upsert logic in the existingTraitIdx/traits append block does not refresh the index after adding a new trait, so duplicate new trait types in traitRequests get appended multiple times. After appending newTrait in the else branch, update existingTraitIdx for that trait type to the new position so later duplicates replace the first appended entry instead of adding another. Keep the replacement behavior in the if branch unchanged.
🧹 Nitpick comments (2)
agent-manager-service/models/agent_thunder_client.go (1)
53-53: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRename the model field to reflect OU-ID semantics.
OrgNamenow persists toou_id, which makes future repository/service code easy to misuse during this tenancy migration. PreferOUID/OuIDconsistently, or add a compatibility comment if the rename is intentionally deferred.🤖 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 `@agent-manager-service/models/agent_thunder_client.go` at line 53, The model field currently named OrgName is mapped to the ou_id column, which is misleading and can cause misuse in repository/service code during the tenancy migration. Rename the field in AgentThunderClient to OUID or OuID so its name matches the persisted semantics, and update any related references or add an explicit compatibility comment if the rename must be deferred.agent-manager-service/repositories/agent_thunder_client_repository.go (1)
169-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ClaimForAttemptusestime.Now()directly instead of accepting an explicitnow.
FindDueacceptsnow time.Timefor determinism/testability, butClaimForAttemptcomputesstaleBeforefromtime.Now()internally (line 186), breaking that consistency and making the stale-claim boundary harder to unit test deterministically.♻️ Suggested consistency fix
- ClaimForAttempt(ctx context.Context, id uuid.UUID) (claimed bool, err error) + ClaimForAttempt(ctx context.Context, id uuid.UUID, now time.Time) (claimed bool, err error)-func (r *AgentThunderClientRepo) ClaimForAttempt(ctx context.Context, id uuid.UUID) (bool, error) { - staleBefore := time.Now().Add(-inProgressStaleThreshold) +func (r *AgentThunderClientRepo) ClaimForAttempt(ctx context.Context, id uuid.UUID, now time.Time) (bool, error) { + staleBefore := now.Add(-inProgressStaleThreshold)🤖 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 `@agent-manager-service/repositories/agent_thunder_client_repository.go` around lines 169 - 199, `ClaimForAttempt` should follow the same deterministic time handling as `FindDue` by not calling `time.Now()` internally. Update `AgentThunderClientRepo.ClaimForAttempt` to accept an explicit `now time.Time` parameter and compute `staleBefore` from that value, then adjust its call sites accordingly so the stale-claim boundary can be tested consistently alongside `FindDue`.
🤖 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 `@agent-manager-service/clients/openchoreosvc/client/components.go`:
- Around line 1135-1137: The ComponentExists method is deriving the namespace
twice by calling c.namespaceFor(ouID) and then passing that value into
GetComponent, which already performs namespaceFor(ouID) internally. Update
ComponentExists in openChoreoClient to pass the original ouID to GetComponent
instead of namespaceName, and keep the existing existence check logic unchanged
so the lookup uses the correct namespace path.
In `@agent-manager-service/clients/openchoreosvc/client/deployments.go`:
- Around line 700-709: `GetDeployments` is deriving the namespace twice before
calling migrated client methods. Update the `openChoreoClient.GetDeployments`
flow so `GetProjectDeploymentPipeline` and `ListEnvironments` are called with
`ouID` instead of `namespaceName`, since those methods now derive their own
namespace internally. Keep the existing `namespaceFor` usage only where a raw
namespace is actually required, and verify any other nested `openChoreoClient`
calls follow the same `ouID`-based contract.
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 948-950: The API-key create and rotate handlers are decoding
request bodies directly with json.NewDecoder without the same 1MB MaxBytesReader
protection used elsewhere in agent_configuration_controller.go. Update the
create path and both rotate decode paths to wrap r.Body with http.MaxBytesReader
before decoding into spec.CreateLLMAPIKeyRequest, using the same pattern as the
other write handlers in this controller.
- Around line 866-878: The per-config API key handlers still use orgName from
the request path as the tenant key, but they need to be switched to the resolved
ouID flow used by the other multi-tenant handlers. Update configEnvAPIKeyParams
and each List/Create/Rotate/Revoke*ConfigAPIKey call-site to resolve and
validate the caller organization against the target resource, perform the
missing-tenant check, and pass ouID instead of orgName into the service methods.
Rename the local variable at each affected handler to ouID so the intent matches
the Thunder OU ID scoping.
- Line 76: The agent_configuration_controller handlers are using
middleware.OUIDFromRequest without verifying whether it returned an empty tenant
key, so missing org context can flow into service/persistence calls. Add a local
checked helper or change OUIDFromRequest to return a (string, bool) so each
affected handler rejects the request early when the OU ID is missing. Update the
call sites in the controller methods that currently assign ouID from
middleware.OUIDFromRequest to fail before any service access when tenant
identity is absent.
In `@agent-manager-service/db_migrations/029_add_ou_id.go`:
- Around line 54-55: The migration loop in the statement execution path is
returning the raw GORM error, which loses useful context. Update the
`db.Exec(stmt).Error` failure handling in the migration function to wrap the
error with migration/table/statement context using `fmt.Errorf("...: %w", err)`,
so the returned error clearly identifies which ALTER statement failed while
preserving the original error.
In `@agent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.go`:
- Around line 29-35: Migration030 currently assumes ou_id is already populated,
but it is registered unconditionally and can swap constraints against rows still
carrying an empty tenant value. Update migration030.Migrate to either backfill
ou_id from the authoritative org-to-OU source before calling the constraint swap
logic, or explicitly detect empty ou_id rows and abort with an error before any
schema changes. Use the migration030 and Migrate flow to locate the fix, and
make sure the swap only proceeds when every row has a valid ou_id.
In `@agent-manager-service/mcp/tools/agents_specs_test.go`:
- Line 43: The specs in the agent-manager MCP tools tests still drive OU ID from
the old org_name tool input, so the new context/JWT-claims path is never
exercised. Update the test setup around the relevant cases in
agents_specs_test.go by removing stale org_name expectations and populating the
OU ID via the tool context/JWT claims instead. Keep the assertions aligned with
the new behavior by validating the resolved OU ID from the tool execution path
in the affected test cases.
---
Outside diff comments:
In `@agent-manager-service/clients/openchoreosvc/client/components.go`:
- Around line 1216-1220: The trait upsert logic in the existingTraitIdx/traits
append block does not refresh the index after adding a new trait, so duplicate
new trait types in traitRequests get appended multiple times. After appending
newTrait in the else branch, update existingTraitIdx for that trait type to the
new position so later duplicates replace the first appended entry instead of
adding another. Keep the replacement behavior in the if branch unchanged.
---
Nitpick comments:
In `@agent-manager-service/models/agent_thunder_client.go`:
- Line 53: The model field currently named OrgName is mapped to the ou_id
column, which is misleading and can cause misuse in repository/service code
during the tenancy migration. Rename the field in AgentThunderClient to OUID or
OuID so its name matches the persisted semantics, and update any related
references or add an explicit compatibility comment if the rename must be
deferred.
In `@agent-manager-service/repositories/agent_thunder_client_repository.go`:
- Around line 169-199: `ClaimForAttempt` should follow the same deterministic
time handling as `FindDue` by not calling `time.Now()` internally. Update
`AgentThunderClientRepo.ClaimForAttempt` to accept an explicit `now time.Time`
parameter and compute `staleBefore` from that value, then adjust its call sites
accordingly so the stale-claim boundary can be tested consistently alongside
`FindDue`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f25dcf9-59c6-4a93-8d9d-85037182bbe3
📒 Files selected for processing (148)
agent-manager-service/clients/clientmocks/openchoreo_client_fake.goagent-manager-service/clients/openchoreosvc/client/builds.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/generic-workflows.goagent-manager-service/clients/openchoreosvc/client/git_secrets.goagent-manager-service/clients/openchoreosvc/client/infrastructure.goagent-manager-service/clients/openchoreosvc/client/projects.goagent-manager-service/clients/openchoreosvc/client/secret_references.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/agent_apikey_controller.goagent-manager-service/controllers/agent_build_options_controller.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/agent_controller.goagent-manager-service/controllers/agent_kind_controller.goagent-manager-service/controllers/agent_token_controller.goagent-manager-service/controllers/catalog_controller.goagent-manager-service/controllers/environment_controller.goagent-manager-service/controllers/evaluator_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/gateway_identity_provider_controller.goagent-manager-service/controllers/gateway_internal_controller.goagent-manager-service/controllers/git_secret_controller.goagent-manager-service/controllers/infra_resource_controller.goagent-manager-service/controllers/llm_controller.goagent-manager-service/controllers/llm_deployment_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/llm_proxy_deployment_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/controllers/monitor_controller.goagent-manager-service/controllers/monitor_scores_controller.goagent-manager-service/controllers/websocket_controller.goagent-manager-service/db_migrations/029_add_ou_id.goagent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/mcp/handlers/agent_handler.goagent-manager-service/mcp/handlers/build_handler.goagent-manager-service/mcp/handlers/deployment_handler.goagent-manager-service/mcp/handlers/observability_handler.goagent-manager-service/mcp/handlers/project_handler.goagent-manager-service/mcp/tools/agents.goagent-manager-service/mcp/tools/agents_specs_test.goagent-manager-service/mcp/tools/builds.goagent-manager-service/mcp/tools/builds_specs_test.goagent-manager-service/mcp/tools/deployments.goagent-manager-service/mcp/tools/deployments_specs_test.goagent-manager-service/mcp/tools/helpers.goagent-manager-service/mcp/tools/mock_test.goagent-manager-service/mcp/tools/observability.goagent-manager-service/mcp/tools/observability_specs_test.goagent-manager-service/mcp/tools/projects.goagent-manager-service/mcp/tools/projects_specs_test.goagent-manager-service/mcp/tools/setup_test.goagent-manager-service/mcp/tools/types.goagent-manager-service/middleware/authorization.goagent-manager-service/middleware/jwtassertion/auth.goagent-manager-service/middleware/orgresolver.goagent-manager-service/models/agent_config.goagent-manager-service/models/agent_configuration.goagent-manager-service/models/agent_kind.goagent-manager-service/models/agent_thunder_client.goagent-manager-service/models/ai_application.goagent-manager-service/models/apikey.goagent-manager-service/models/artifact.goagent-manager-service/models/association_mapping.goagent-manager-service/models/catalog.goagent-manager-service/models/custom_evaluator.goagent-manager-service/models/deployment.goagent-manager-service/models/gateway.goagent-manager-service/models/llm_provider_template.goagent-manager-service/models/monitor.goagent-manager-service/repositories/agent_config_repository.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/agent_env_config_variable_repository.goagent-manager-service/repositories/agent_kind_repository.goagent-manager-service/repositories/agent_thunder_client_repository.goagent-manager-service/repositories/ai_application_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/artifact_repository.goagent-manager-service/repositories/catalog_repository.goagent-manager-service/repositories/custom_evaluator_repository.goagent-manager-service/repositories/deployment_repository.goagent-manager-service/repositories/env_agent_mcp_mapping_repository.goagent-manager-service/repositories/evaluation_score_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_provider_repository.goagent-manager-service/repositories/llm_provider_template_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/mcp_proxy_repository.goagent-manager-service/repositories/monitor_repository.goagent-manager-service/repositories/org_publisher_credential_repository.goagent-manager-service/repositories/repomocks/agent_config_repository_mock.goagent-manager-service/repositories/repomocks/agent_configuration_repository_mock.goagent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.goagent-manager-service/repositories/repomocks/agent_kind_repository_mock.goagent-manager-service/repositories/repomocks/agent_thunder_client_repository_mock.goagent-manager-service/repositories/repomocks/ai_application_repository_mock.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.goagent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.goagent-manager-service/repositories/repomocks/evaluation_score_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/repositories/repomocks/monitor_repository_mock.goagent-manager-service/repositories/repomocks/org_publisher_credential_repository_mock.goagent-manager-service/services/agent_api_artifact.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_kind_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/agent_thunder_provisioning_service.goagent-manager-service/services/agent_token_manager.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/catalog_service.goagent-manager-service/services/catalog_service_unit_test.goagent-manager-service/services/environment_service.goagent-manager-service/services/evaluator_manager.goagent-manager-service/services/gateway_config_applier.goagent-manager-service/services/gateway_internal_service.goagent-manager-service/services/git_credentials_service.goagent-manager-service/services/git_secret_service.goagent-manager-service/services/infra_resource_manager.goagent-manager-service/services/infra_resource_manager_test.goagent-manager-service/services/infra_resource_manager_unit_test.goagent-manager-service/services/llm_deployment_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_provider_template_service.goagent-manager-service/services/llm_proxy_deployment_service.goagent-manager-service/services/llm_proxy_deployment_service_test.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/llm_proxy_service.goagent-manager-service/services/llm_template_seeder.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/monitor_executor.goagent-manager-service/services/monitor_manager.goagent-manager-service/services/monitor_scheduler.goagent-manager-service/services/monitor_scheduler_unit_test.goagent-manager-service/services/monitor_scores_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/services/publisher_credential_provisioner.goagent-manager-service/services/repository_service.goagent-manager-service/services/repository_service_unit_test.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
💤 Files with no reviewable changes (47)
- agent-manager-service/services/repository_service.go
- agent-manager-service/services/gateway_config_applier.go
- agent-manager-service/services/infra_resource_manager_test.go
- agent-manager-service/services/mcp_proxy_service.go
- agent-manager-service/services/agent_token_manager.go
- agent-manager-service/services/monitor_scheduler.go
- agent-manager-service/services/agent_api_artifact.go
- agent-manager-service/services/repository_service_unit_test.go
- agent-manager-service/repositories/repomocks/org_publisher_credential_repository_mock.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/services/git_credentials_service.go
- agent-manager-service/services/infra_resource_manager_unit_test.go
- agent-manager-service/services/gateway_internal_service.go
- agent-manager-service/repositories/repomocks/env_agent_mcp_mapping_repository_mock.go
- agent-manager-service/services/monitor_scheduler_unit_test.go
- agent-manager-service/services/llm_proxy_deployment_service_test.go
- agent-manager-service/services/monitor_executor.go
- agent-manager-service/services/monitor_scores_service.go
- agent-manager-service/services/git_secret_service.go
- agent-manager-service/services/llm_template_seeder.go
- agent-manager-service/services/ai_application_service.go
- agent-manager-service/services/catalog_service_unit_test.go
- agent-manager-service/repositories/repomocks/custom_evaluator_repository_mock.go
- agent-manager-service/services/evaluator_manager.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/services/publisher_credential_provisioner.go
- agent-manager-service/services/llm_provider_template_service.go
- agent-manager-service/services/mcp_proxy_deployment.go
- agent-manager-service/repositories/repomocks/ai_application_repository_mock.go
- agent-manager-service/services/llm_proxy_service.go
- agent-manager-service/services/llm_deployment_service.go
- agent-manager-service/services/platform_gateway_service.go
- agent-manager-service/services/environment_service.go
- agent-manager-service/services/agent_thunder_provisioning_service.go
- agent-manager-service/repositories/repomocks/evaluation_score_repository_mock.go
- agent-manager-service/services/llm_proxy_provisioner.go
- agent-manager-service/services/catalog_service.go
- agent-manager-service/services/agent_kind_service.go
- agent-manager-service/repositories/repomocks/monitor_repository_mock.go
- agent-manager-service/services/agent_apikey_service.go
- agent-manager-service/services/llm_proxy_deployment_service.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/services/infra_resource_manager.go
- agent-manager-service/services/monitor_manager.go
✅ Files skipped from review due to trivial changes (11)
- agent-manager-service/mcp/tools/projects_specs_test.go
- agent-manager-service/controllers/agent_build_options_controller.go
- agent-manager-service/mcp/handlers/build_handler.go
- agent-manager-service/mcp/tools/builds_specs_test.go
- agent-manager-service/mcp/handlers/deployment_handler.go
- agent-manager-service/mcp/tools/deployments_specs_test.go
- agent-manager-service/mcp/tools/observability_specs_test.go
- agent-manager-service/repositories/repomocks/agent_env_config_variable_repository_mock.go
- agent-manager-service/repositories/repomocks/agent_config_repository_mock.go
- agent-manager-service/mcp/tools/types.go
- agent-manager-service/repositories/repomocks/agent_kind_repository_mock.go
🚧 Files skipped from review as they are similar to previous changes (71)
- agent-manager-service/models/ai_application.go
- agent-manager-service/models/llm_provider_template.go
- agent-manager-service/config/config.go
- agent-manager-service/models/deployment.go
- agent-manager-service/models/custom_evaluator.go
- agent-manager-service/repositories/env_agent_mcp_mapping_repository.go
- agent-manager-service/models/catalog.go
- agent-manager-service/models/artifact.go
- agent-manager-service/models/agent_config.go
- agent-manager-service/repositories/llm_provider_repository.go
- agent-manager-service/models/agent_configuration.go
- agent-manager-service/models/apikey.go
- agent-manager-service/repositories/agent_config_repository.go
- agent-manager-service/models/association_mapping.go
- agent-manager-service/mcp/handlers/project_handler.go
- agent-manager-service/clients/openchoreosvc/client/generic-workflows.go
- agent-manager-service/repositories/agent_env_config_variable_repository.go
- agent-manager-service/repositories/artifact_repository.go
- agent-manager-service/middleware/jwtassertion/auth.go
- agent-manager-service/controllers/catalog_controller.go
- agent-manager-service/controllers/gateway_internal_controller.go
- agent-manager-service/models/gateway.go
- agent-manager-service/controllers/agent_token_controller.go
- agent-manager-service/mcp/tools/helpers.go
- agent-manager-service/clients/openchoreosvc/client/secret_references.go
- agent-manager-service/mcp/handlers/agent_handler.go
- agent-manager-service/clients/openchoreosvc/client/git_secrets.go
- agent-manager-service/controllers/git_secret_controller.go
- agent-manager-service/repositories/apikey_repository.go
- agent-manager-service/models/agent_kind.go
- agent-manager-service/middleware/orgresolver.go
- agent-manager-service/models/monitor.go
- agent-manager-service/mcp/tools/builds.go
- agent-manager-service/repositories/llm_provider_template_repository.go
- agent-manager-service/controllers/environment_controller.go
- agent-manager-service/config/config_loader.go
- agent-manager-service/repositories/ai_application_repository.go
- agent-manager-service/repositories/evaluation_score_repository.go
- agent-manager-service/controllers/agent_kind_controller.go
- agent-manager-service/repositories/org_publisher_credential_repository.go
- agent-manager-service/clients/openchoreosvc/client/builds.go
- agent-manager-service/repositories/catalog_repository.go
- agent-manager-service/clients/openchoreosvc/client/client.go
- agent-manager-service/controllers/gateway_identity_provider_controller.go
- agent-manager-service/repositories/custom_evaluator_repository.go
- agent-manager-service/controllers/websocket_controller.go
- agent-manager-service/mcp/tools/projects.go
- agent-manager-service/mcp/tools/deployments.go
- agent-manager-service/controllers/agent_apikey_controller.go
- agent-manager-service/mcp/tools/mock_test.go
- agent-manager-service/middleware/authorization.go
- agent-manager-service/controllers/evaluator_controller.go
- agent-manager-service/mcp/tools/observability.go
- agent-manager-service/controllers/monitor_scores_controller.go
- agent-manager-service/repositories/agent_kind_repository.go
- agent-manager-service/repositories/gateway_repository.go
- agent-manager-service/mcp/tools/agents.go
- agent-manager-service/controllers/monitor_controller.go
- agent-manager-service/mcp/tools/setup_test.go
- agent-manager-service/controllers/infra_resource_controller.go
- agent-manager-service/repositories/agent_configuration_repository.go
- agent-manager-service/clients/openchoreosvc/client/projects.go
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- agent-manager-service/controllers/llm_deployment_controller.go
- agent-manager-service/controllers/llm_proxy_deployment_controller.go
- agent-manager-service/repositories/deployment_repository.go
- agent-manager-service/repositories/mcp_proxy_repository.go
- agent-manager-service/repositories/llm_proxy_repository.go
- agent-manager-service/clients/openchoreosvc/client/infrastructure.go
- agent-manager-service/controllers/agent_controller.go
- agent-manager-service/controllers/gateway_controller.go
🛑 Comments failed to post (8)
agent-manager-service/clients/openchoreosvc/client/components.go (1)
1135-1137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the original
ouIDintoGetComponent.
GetComponentalready callsc.namespaceFor(ouID), so passingnamespaceNamehere applies namespace derivation twice and can make existence checks look in the wrong namespace.Proposed fix
func (c *openChoreoClient) ComponentExists(ctx context.Context, ouID, projectName, componentName string) (bool, error) { - namespaceName := c.namespaceFor(ouID) - _, err := c.GetComponent(ctx, namespaceName, projectName, componentName) + _, err := c.GetComponent(ctx, ouID, projectName, componentName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (c *openChoreoClient) ComponentExists(ctx context.Context, ouID, projectName, componentName string) (bool, error) { _, err := c.GetComponent(ctx, ouID, projectName, componentName)🤖 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 `@agent-manager-service/clients/openchoreosvc/client/components.go` around lines 1135 - 1137, The ComponentExists method is deriving the namespace twice by calling c.namespaceFor(ouID) and then passing that value into GetComponent, which already performs namespaceFor(ouID) internally. Update ComponentExists in openChoreoClient to pass the original ouID to GetComponent instead of namespaceName, and keep the existing existence check logic unchanged so the lookup uses the correct namespace path.agent-manager-service/clients/openchoreosvc/client/deployments.go (1)
700-709: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid double namespace derivation for nested client calls.
GetProjectDeploymentPipelineandListEnvironmentsare part of the migrated OpenChoreo client surface, so they should receiveouID; passingnamespaceNamemakes them derive from an already-derived namespace.Proposed fix
- pipeline, err := c.GetProjectDeploymentPipeline(ctx, namespaceName, projectName) + pipeline, err := c.GetProjectDeploymentPipeline(ctx, ouID, projectName) @@ - environments, err := c.ListEnvironments(ctx, namespaceName) + environments, err := c.ListEnvironments(ctx, ouID)Based on PR context, these client methods were migrated to derive namespace from
ouID.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (c *openChoreoClient) GetDeployments(ctx context.Context, ouID, pipelineName, projectName, componentName string) ([]*models.DeploymentResponse, error) { namespaceName := c.namespaceFor(ouID) // Get the deployment pipeline for environment ordering pipeline, err := c.GetProjectDeploymentPipeline(ctx, ouID, projectName) if err != nil { return nil, fmt.Errorf("failed to get deployment pipeline: %w", err) } // Get all environments for display names environments, err := c.ListEnvironments(ctx, ouID)🤖 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 `@agent-manager-service/clients/openchoreosvc/client/deployments.go` around lines 700 - 709, `GetDeployments` is deriving the namespace twice before calling migrated client methods. Update the `openChoreoClient.GetDeployments` flow so `GetProjectDeploymentPipeline` and `ListEnvironments` are called with `ouID` instead of `namespaceName`, since those methods now derive their own namespace internally. Keep the existing `namespaceFor` usage only where a raw namespace is actually required, and verify any other nested `openChoreoClient` calls follow the same `ouID`-based contract.agent-manager-service/controllers/agent_configuration_controller.go (3)
76-76: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject requests when the resolved OU ID is missing.
OUIDFromRequestreturns""whenRequireOrgMatch/org resolution is missing, and these handlers pass that empty tenant key into service calls. Add a local checked helper or makeOUIDFromRequestreturn(string, bool)so missing tenant context fails before persistence/service access. As per coding guidelines, “missing tenant identity is an error, not a wildcard.”Suggested shape
+func requireOUIDFromRequest(w http.ResponseWriter, r *http.Request) (string, bool) { + ouID := middleware.OUIDFromRequest(r) + if ouID == "" { + utils.WriteErrorResponse(w, http.StatusUnauthorized, "Organization context is required") + return "", false + } + return ouID, true +} + - ouID := middleware.OUIDFromRequest(r) + ouID, ok := requireOUIDFromRequest(w, r) + if !ok { + return + }Also applies to: 156-156, 194-194, 228-228, 314-314, 359-359, 423-423, 454-454, 486-486, 559-559
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` at line 76, The agent_configuration_controller handlers are using middleware.OUIDFromRequest without verifying whether it returned an empty tenant key, so missing org context can flow into service/persistence calls. Add a local checked helper or change OUIDFromRequest to return a (string, bool) so each affected handler rejects the request early when the OU ID is missing. Update the call sites in the controller methods that currently assign ouID from middleware.OUIDFromRequest to fail before any service access when tenant identity is absent.Source: Coding guidelines
866-878: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Migrate the per-config API key handlers to resolved
ouID.These new handlers still read
orgNamefrom the path and pass it as the tenant key, while this PR scopes org data by Thunder OU ID. Use the same resolved OU ID path as the other handlers, including the missing-tenant check, before callingList/Create/Rotate/Revoke*ConfigAPIKey. As per coding guidelines, “Validate caller organization against the target resource for multi-tenant safety.”Suggested direction
-func (c *agentConfigurationController) configEnvAPIKeyParams(w http.ResponseWriter, r *http.Request) (orgName, projName, agentName, envName string, configUUID uuid.UUID, ok bool) { - orgName = r.PathValue(utils.PathParamOrgName) +func (c *agentConfigurationController) configEnvAPIKeyParams(w http.ResponseWriter, r *http.Request) (ouID, projName, agentName, envName string, configUUID uuid.UUID, ok bool) { + ouID, ok = requireOUIDFromRequest(w, r) + if !ok { + return "", "", "", "", uuid.Nil, false + } projName = r.PathValue(utils.PathParamProjName) agentName = r.PathValue(utils.PathParamAgentName) envName = r.PathValue("envName") @@ - return orgName, projName, agentName, envName, parsed, true + return ouID, projName, agentName, envName, parsed, true }Then rename each call-site local from
orgNametoouIDand passouIDinto the corresponding service method.Also applies to: 920-925, 943-958, 975-989, 1005-1011, 1023-1028, 1046-1061, 1078-1092, 1108-1114
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 866 - 878, The per-config API key handlers still use orgName from the request path as the tenant key, but they need to be switched to the resolved ouID flow used by the other multi-tenant handlers. Update configEnvAPIKeyParams and each List/Create/Rotate/Revoke*ConfigAPIKey call-site to resolve and validate the caller organization against the target resource, perform the missing-tenant check, and pass ouID instead of orgName into the service methods. Rename the local variable at each affected handler to ouID so the intent matches the Thunder OU ID scoping.Source: Coding guidelines
948-950: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cap API-key request bodies before JSON decoding.
The new create/rotate API-key endpoints decode
r.Bodywithout the 1MBMaxBytesReaderlimit used by the other write handlers in this controller. Add the same cap before decoding to avoid oversized-body memory/CPU abuse.Suggested fix
+ r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MB limit var specReq spec.CreateLLMAPIKeyRequest if err := json.NewDecoder(r.Body).Decode(&specReq); err != nil {Apply the same pattern before both rotate decoders as well.
Also applies to: 981-984, 1051-1052, 1084-1087
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 948 - 950, The API-key create and rotate handlers are decoding request bodies directly with json.NewDecoder without the same 1MB MaxBytesReader protection used elsewhere in agent_configuration_controller.go. Update the create path and both rotate decode paths to wrap r.Body with http.MaxBytesReader before decoding into spec.CreateLLMAPIKeyRequest, using the same pattern as the other write handlers in this controller.agent-manager-service/db_migrations/029_add_ou_id.go (1)
54-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the failed statement in the migration error.
This loop runs many ALTER statements; returning the raw GORM error makes failures hard to diagnose. Wrap it with migration/table/statement context.
As per coding guidelines, “wrap all other unexpected errors with context using
fmt.Errorf("...: %w", err).”Proposed fix
import ( + "fmt" + "gorm.io/gorm" ) @@ for _, stmt := range stmts { if err := db.Exec(stmt).Error; err != nil { - return err + return fmt.Errorf("migration 029 executing %q: %w", stmt, err) } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import ( "fmt" "gorm.io/gorm" ) ... if err := db.Exec(stmt).Error; err != nil { return fmt.Errorf("migration 029 executing %q: %w", stmt, err) } ...🤖 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 `@agent-manager-service/db_migrations/029_add_ou_id.go` around lines 54 - 55, The migration loop in the statement execution path is returning the raw GORM error, which loses useful context. Update the `db.Exec(stmt).Error` failure handling in the migration function to wrap the error with migration/table/statement context using `fmt.Errorf("...: %w", err)`, so the returned error clearly identifies which ALTER statement failed while preserving the original error.Source: Coding guidelines
agent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.go (1)
29-35: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Enforce or backfill the
ou_idprecondition before swapping constraints.Migration 029 backfills existing rows to
ou_id = '', but this migration is registered to run unconditionally. That can fail unique/PK creation for existing multi-org data, or leave migrated rows scoped under an empty tenant instead of the caller’s real OU ID. Add a real backfill from an authoritative org→OU mapping, or fail before any constraint swap when emptyou_idrows exist.As per coding guidelines, “missing tenant identity is an error, not a wildcard.”
🤖 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 `@agent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.go` around lines 29 - 35, Migration030 currently assumes ou_id is already populated, but it is registered unconditionally and can swap constraints against rows still carrying an empty tenant value. Update migration030.Migrate to either backfill ou_id from the authoritative org-to-OU source before calling the constraint swap logic, or explicitly detect empty ou_id rows and abort with an error before any schema changes. Use the migration030 and Migrate flow to locate the fix, and make sure the swap only proceeds when every row has a valid ou_id.Source: Coding guidelines
agent-manager-service/mcp/tools/agents_specs_test.go (1)
43-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the test setup, not just the assertion labels.
These assertions now say
ouID, but the specs still feedorg_namefrom tool args, so the tests can pass without exercising the new context-derived OU-ID path. Remove the staleorg_nameinput expectations and inject the OU ID through the tool context/JWT-claims setup instead.Based on PR context, MCP tools now resolve OU ID from JWT claims rather than org-name tool input.
Also applies to: 65-65, 86-86
🤖 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 `@agent-manager-service/mcp/tools/agents_specs_test.go` at line 43, The specs in the agent-manager MCP tools tests still drive OU ID from the old org_name tool input, so the new context/JWT-claims path is never exercised. Update the test setup around the relevant cases in agents_specs_test.go by removing stale org_name expectations and populating the OU ID via the tool context/JWT claims instead. Keep the assertions aligned with the new behavior by validating the resolved OU ID from the tool execution path in the affected test cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
agent-manager-service/middleware/jwtassertion/test_utils.go (1)
44-63: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject empty OU IDs in the mock middleware helper.
NewMockMiddlewareWithOUIDcan mintTokenClaimswithOuId: "", which lets scoped handler tests run with a missing tenant identity instead of failing closed.Proposed guard
func NewMockMiddlewareWithOUID(t *testing.T, ouID string) Middleware { t.Helper() + if ouID == "" { + t.Fatal("mock OU ID must not be empty; use a custom middleware to test missing tenant identity") + } return func(next http.Handler) http.Handler {As per coding guidelines, missing tenant identity is an error, not a wildcard.
🤖 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 `@agent-manager-service/middleware/jwtassertion/test_utils.go` around lines 44 - 63, Reject empty OU IDs in NewMockMiddlewareWithOUID by validating the ouID input before minting TokenClaims; if ouID is empty, fail the test immediately with a clear error rather than creating a middleware that sets OuId to "". Update the helper in test_utils.go so the middleware only constructs TokenClaims after this guard, keeping scoped handler tests aligned with the non-empty tenant identity requirement.Source: Coding guidelines
agent-manager-service/controllers/gateway_internal_controller.go (1)
94-102: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
gateway.OUIDbefore using it as the tenant scope.After
VerifyToken, these paths trustgateway.OUIDdirectly. If a legacy or malformed gateway record has an empty OU ID, downstream lookups run with missing tenant context. Add a fail-closed guard once after authentication in each path, or centralize it inVerifyToken.As per coding guidelines, missing tenant identity is an error, not a wildcard.
Also applies to: 161-169, 224-236, 337-339, 422-422
🤖 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 `@agent-manager-service/controllers/gateway_internal_controller.go` around lines 94 - 102, Validate gateway.OUID immediately after VerifyToken and before any downstream service call, because these controller paths currently pass it through as tenant scope without checking for emptiness. Add a fail-closed guard in gateway_internal_controller.go around the gateway.OUID reads used in the affected handlers, or centralize the check inside VerifyToken so every caller gets a non-empty tenant identity. Make sure the guard returns an error response instead of continuing when gateway.OUID is missing.Source: Coding guidelines
agent-manager-service/controllers/agent_configuration_controller.go (1)
70-76: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFail closed when
OUIDFromRequestreturns an empty tenant.
middleware.OUIDFromRequestreturns""whenRequireOrgMatchdid not populate the resolved org, but these handlers pass that value into service scoping. Add a guard or change the helper to return(string, error)and map the missing org context to an HTTP error before calling services.As per coding guidelines, missing tenant identity is an error, not a wildcard.
Also applies to: 151-156, 189-194, 223-228, 309-314, 354-359, 418-423, 449-454, 481-486, 554-559
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 70 - 76, The handlers using middleware.OUIDFromRequest are currently treating an empty org/tenant value as valid and passing it into service scoping, so fail closed by checking for a missing OUID before any service call. Add a guard in CreateAgentModelConfig and the other affected controller methods, or update OUIDFromRequest to return (string, error) and convert the missing org context into an HTTP error response. Make sure the early-return error path happens before any use of the scoped service/client in these controller methods.Source: Coding guidelines
agent-manager-service/controllers/environment_controller.go (1)
76-76: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftGuard missing OU ID before service calls.
OUIDFromRequestcan return""when org resolution middleware is missing, and these handlers pass that into environment service calls. Fail closed locally or update the helper contract so missing resolved-org context becomes an HTTP error.As per coding guidelines, missing tenant identity is an error, not a wildcard.
Also applies to: 118-121, 136-136, 172-172, 210-213, 226-229
🤖 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 `@agent-manager-service/controllers/environment_controller.go` at line 76, Missing OU ID is being passed from environment controller handlers into service calls, which violates the tenant-identity contract. Update the affected handlers in environment_controller.go to check the result of middleware.OUIDFromRequest(r) before calling the environment service methods, and return an HTTP error when it is empty instead of proceeding. Apply the same guard to each affected handler that uses OUIDFromRequest so the controller fails closed locally rather than treating missing org context as a wildcard.Source: Coding guidelines
agent-manager-service/repositories/llm_provider_repository.go (1)
37-43: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBring these repository methods up to the I/O/error contract.
The updated OU-scoped DB calls still cannot receive request cancellation/deadlines because the repository API lacks
context.Context, and unexpected DB errors are returned raw in multiple paths. Threadctxthrough the interface and useWithContext(ctx)plus contextual wrapping/sentinel mapping. As per coding guidelines, every I/O method must takecontext.Contextas the first parameter and propagate it, and unexpected errors should be wrapped with context while not-found errors map to sentinels.Also applies to: 62-97, 101-116, 129-145, 156-170, 238-252, 281-292
🤖 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 `@agent-manager-service/repositories/llm_provider_repository.go` around lines 37 - 43, The LLMProviderRepository interface methods still omit context and return raw DB errors, so update every I/O method signature to accept context.Context first and propagate it through the implementations. In the repository methods such as Create, GetByUUID, GetByHandle, List, Count, Update, and Delete, use WithContext(ctx) on the GORM calls, map not-found cases to the existing sentinel errors, and wrap all other unexpected errors with contextual information so callers can handle cancellation/deadlines and consistent error contracts.Source: Coding guidelines
agent-manager-service/repositories/mcp_proxy_repository.go (1)
99-117: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the proxy update before mutating the row.
Updatewritesmcp_proxiesby UUID before verifying the artifact belongs toorgUUID. Iftxis not an enclosing transaction, an org-mismatched request can persist config changes and then fail inartifactRepo.Update. Add theartifacts.ou_id/kind predicate to the first update. As per coding guidelines, validate caller organization against the target resource for multi-tenant safety.Suggested fix
result := tx.WithContext(ctx).Model(&models.MCPProxy{}). - Where("uuid = ?", p.UUID). + Where(`uuid = ? AND EXISTS ( + SELECT 1 FROM artifacts a + WHERE a.uuid = mcp_proxies.uuid + AND a.ou_id = ? + AND a.kind = ? + )`, p.UUID, orgUUID, models.KindMCPProxy). Updates(map[string]interface{}{🤖 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 `@agent-manager-service/repositories/mcp_proxy_repository.go` around lines 99 - 117, The MCP proxy update in Update currently mutates mcp_proxies by UUID before confirming the artifact belongs to orgUUID, which can leave partial writes in non-transactional paths. Update the initial gorm query on models.MCPProxy to include the same organization-scoping predicate used for the target artifact (artifacts.ou_id and the correct kind), so the row is only changed when the caller owns the resource. Keep the existing artifactRepo.Update call, and make sure the scoping check is applied using the Update method’s tx.WithContext(ctx) flow and the p.UUID/orgUUID values.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 `@agent-manager-service/models/ai_application.go`:
- Line 36: The `OUID` field in `AIApplication` is currently serialized with the
old `organizationName` JSON key, which exposes the Thunder OU ID under the wrong
contract. Update the struct tag on `OUID` so it is either omitted from JSON or
mapped to an explicit OU-specific key, and keep the change localized to the
`AIApplication` model definition.
In `@agent-manager-service/models/artifact.go`:
- Line 32: The Artifact model is serializing OUID under the wrong JSON field,
causing the OU ID to be exposed as organization_name. Update the struct tag on
Artifact.OUID to use the correct JSON name for the OU identifier, and keep the
GORM column mapping to ou_id unchanged so persistence still works as expected.
---
Outside diff comments:
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 70-76: The handlers using middleware.OUIDFromRequest are currently
treating an empty org/tenant value as valid and passing it into service scoping,
so fail closed by checking for a missing OUID before any service call. Add a
guard in CreateAgentModelConfig and the other affected controller methods, or
update OUIDFromRequest to return (string, error) and convert the missing org
context into an HTTP error response. Make sure the early-return error path
happens before any use of the scoped service/client in these controller methods.
In `@agent-manager-service/controllers/environment_controller.go`:
- Line 76: Missing OU ID is being passed from environment controller handlers
into service calls, which violates the tenant-identity contract. Update the
affected handlers in environment_controller.go to check the result of
middleware.OUIDFromRequest(r) before calling the environment service methods,
and return an HTTP error when it is empty instead of proceeding. Apply the same
guard to each affected handler that uses OUIDFromRequest so the controller fails
closed locally rather than treating missing org context as a wildcard.
In `@agent-manager-service/controllers/gateway_internal_controller.go`:
- Around line 94-102: Validate gateway.OUID immediately after VerifyToken and
before any downstream service call, because these controller paths currently
pass it through as tenant scope without checking for emptiness. Add a
fail-closed guard in gateway_internal_controller.go around the gateway.OUID
reads used in the affected handlers, or centralize the check inside VerifyToken
so every caller gets a non-empty tenant identity. Make sure the guard returns an
error response instead of continuing when gateway.OUID is missing.
In `@agent-manager-service/middleware/jwtassertion/test_utils.go`:
- Around line 44-63: Reject empty OU IDs in NewMockMiddlewareWithOUID by
validating the ouID input before minting TokenClaims; if ouID is empty, fail the
test immediately with a clear error rather than creating a middleware that sets
OuId to "". Update the helper in test_utils.go so the middleware only constructs
TokenClaims after this guard, keeping scoped handler tests aligned with the
non-empty tenant identity requirement.
In `@agent-manager-service/repositories/llm_provider_repository.go`:
- Around line 37-43: The LLMProviderRepository interface methods still omit
context and return raw DB errors, so update every I/O method signature to accept
context.Context first and propagate it through the implementations. In the
repository methods such as Create, GetByUUID, GetByHandle, List, Count, Update,
and Delete, use WithContext(ctx) on the GORM calls, map not-found cases to the
existing sentinel errors, and wrap all other unexpected errors with contextual
information so callers can handle cancellation/deadlines and consistent error
contracts.
In `@agent-manager-service/repositories/mcp_proxy_repository.go`:
- Around line 99-117: The MCP proxy update in Update currently mutates
mcp_proxies by UUID before confirming the artifact belongs to orgUUID, which can
leave partial writes in non-transactional paths. Update the initial gorm query
on models.MCPProxy to include the same organization-scoping predicate used for
the target artifact (artifacts.ou_id and the correct kind), so the row is only
changed when the caller owns the resource. Keep the existing artifactRepo.Update
call, and make sure the scoping check is applied using the Update method’s
tx.WithContext(ctx) flow and the p.UUID/orgUUID values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d374452-aa07-4850-9c86-355f1d17deeb
📒 Files selected for processing (97)
agent-manager-service/clients/clientmocks/openchoreo_client_fake.goagent-manager-service/clients/openchoreosvc/client/builds.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/generic-workflows.goagent-manager-service/clients/openchoreosvc/client/git_secrets.goagent-manager-service/clients/openchoreosvc/client/infrastructure.goagent-manager-service/clients/openchoreosvc/client/projects.goagent-manager-service/clients/openchoreosvc/client/secret_references.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/environment_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/gateway_internal_controller.goagent-manager-service/controllers/infra_resource_controller.goagent-manager-service/controllers/websocket_controller.goagent-manager-service/db_migrations/029_add_ou_id.goagent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/middleware/jwtassertion/test_utils.goagent-manager-service/models/agent_config.goagent-manager-service/models/agent_configuration.goagent-manager-service/models/agent_configuration_dto.goagent-manager-service/models/agent_kind.goagent-manager-service/models/agent_thunder_client.goagent-manager-service/models/ai_application.goagent-manager-service/models/apikey.goagent-manager-service/models/artifact.goagent-manager-service/models/association_mapping.goagent-manager-service/models/catalog.goagent-manager-service/models/custom_evaluator.goagent-manager-service/models/deployment.goagent-manager-service/models/gateway.goagent-manager-service/models/llm_provider_template.goagent-manager-service/models/monitor.goagent-manager-service/repositories/agent_thunder_client_repository_test.goagent-manager-service/repositories/artifact_repository.goagent-manager-service/repositories/deployment_repository.goagent-manager-service/repositories/env_agent_mcp_mapping_repository.goagent-manager-service/repositories/llm_provider_repository.goagent-manager-service/repositories/llm_provider_template_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/mcp_proxy_repository.goagent-manager-service/resources/builtin_llm_templates.goagent-manager-service/services/agent_api_artifact.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_kind_service.goagent-manager-service/services/agent_kind_service_unit_test.goagent-manager-service/services/agent_manager.goagent-manager-service/services/agent_thunder_provisioning_service.goagent-manager-service/services/agent_thunder_provisioning_service_test.goagent-manager-service/services/agent_thunder_reconciler_test.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/evaluator_manager.goagent-manager-service/services/evaluator_manager_unit_test.goagent-manager-service/services/llm_deployment_service.goagent-manager-service/services/llm_provider_template_service.goagent-manager-service/services/llm_proxy_deployment_service.goagent-manager-service/services/llm_template_seeder.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/monitor_executor.goagent-manager-service/services/monitor_manager.goagent-manager-service/services/monitor_scheduler.goagent-manager-service/services/monitor_scheduler_test.goagent-manager-service/services/monitor_scheduler_unit_test.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/services/publisher_credential_provisioner.goagent-manager-service/spec/model_agent_kind_response.goagent-manager-service/spec/model_agent_model_config_list_item.goagent-manager-service/spec/model_agent_model_config_response.goagent-manager-service/spec/model_data_plane.goagent-manager-service/spec/model_deployment_pipeline_response.goagent-manager-service/spec/model_gateway_environment_response.goagent-manager-service/spec/model_gateway_response.goagent-manager-service/spec/model_monitor_response.goagent-manager-service/spec/model_project_list_item.goagent-manager-service/spec/model_project_response.goagent-manager-service/spec/model_stored_api_key.goagent-manager-service/tests/apitestutils/mock_clients.goagent-manager-service/tests/build_agent_test.goagent-manager-service/tests/build_logs_test.goagent-manager-service/tests/create_agent_test.goagent-manager-service/tests/create_external_agent_test.goagent-manager-service/tests/create_project_test.goagent-manager-service/tests/delete_agent_test.goagent-manager-service/tests/delete_project_test.goagent-manager-service/tests/deploy_agent_test.goagent-manager-service/tests/deployment_pipeline_test.goagent-manager-service/tests/environment_test.goagent-manager-service/tests/evaluator_test.goagent-manager-service/tests/monitor_executor_test.goagent-manager-service/tests/monitor_test.goagent-manager-service/tests/score_repository_test.goagent-manager-service/tests/update_project_test.goagent-manager-service/utils/makeresults.goagent-manager-service/utils/spec_converter.go
💤 Files with no reviewable changes (2)
- agent-manager-service/controllers/infra_resource_controller.go
- agent-manager-service/docs/api_v1_openapi.yaml
✅ Files skipped from review due to trivial changes (2)
- agent-manager-service/services/agent_kind_service_unit_test.go
- agent-manager-service/controllers/websocket_controller.go
🚧 Files skipped from review as they are similar to previous changes (34)
- agent-manager-service/models/gateway.go
- agent-manager-service/db_migrations/029_add_ou_id.go
- agent-manager-service/models/apikey.go
- agent-manager-service/models/deployment.go
- agent-manager-service/repositories/env_agent_mcp_mapping_repository.go
- agent-manager-service/models/agent_config.go
- agent-manager-service/models/association_mapping.go
- agent-manager-service/clients/openchoreosvc/client/generic-workflows.go
- agent-manager-service/repositories/llm_provider_template_repository.go
- agent-manager-service/clients/openchoreosvc/client/git_secrets.go
- agent-manager-service/services/agent_api_artifact.go
- agent-manager-service/services/llm_template_seeder.go
- agent-manager-service/clients/openchoreosvc/client/projects.go
- agent-manager-service/clients/openchoreosvc/client/secret_references.go
- agent-manager-service/services/llm_provider_template_service.go
- agent-manager-service/clients/openchoreosvc/client/client.go
- agent-manager-service/services/monitor_scheduler_unit_test.go
- agent-manager-service/repositories/artifact_repository.go
- agent-manager-service/db_migrations/030_swap_org_constraints_to_ou_id.go
- agent-manager-service/services/agent_thunder_provisioning_service.go
- agent-manager-service/services/mcp_proxy_deployment.go
- agent-manager-service/repositories/deployment_repository.go
- agent-manager-service/services/agent_kind_service.go
- agent-manager-service/controllers/gateway_controller.go
- agent-manager-service/repositories/llm_proxy_repository.go
- agent-manager-service/clients/openchoreosvc/client/builds.go
- agent-manager-service/services/llm_deployment_service.go
- agent-manager-service/services/llm_proxy_deployment_service.go
- agent-manager-service/services/platform_gateway_service.go
- agent-manager-service/clients/openchoreosvc/client/deployments.go
- agent-manager-service/services/evaluator_manager.go
- agent-manager-service/clients/openchoreosvc/client/infrastructure.go
- agent-manager-service/services/monitor_manager.go
- agent-manager-service/clients/openchoreosvc/client/components.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@agent-manager-service/api/gateway_routes.go`:
- Around line 26-36: The gateway registration route is using the standard authz
helper, which blocks bootstrap/system-client access for root/admin OU clients.
Update the POST registration handler in gateway_routes.go by restoring
AllowRootOU support on the `RegisterGateway` route while leaving the environment
assignment, health, and token routes on the regular authz helper; use the
existing `ctrl.RegisterGateway` and `rr.HandleFuncWithValidationAndAuthz` setup
to locate the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c690725d-966a-43c7-a3e6-adcd19313cfe
⛔ Files ignored due to path filters (1)
cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (15)
agent-manager-service/api/gateway_routes.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/ai_application.goagent-manager-service/models/artifact.goagent-manager-service/services/monitor_manager.goagent-manager-service/spec/model_create_gateway_request.goagent-manager-service/tests/monitor_executor_test.goagent-manager-service/tests/monitor_test.goagent-manager-service/tests/promote_agent_test.goagent-manager-service/tests/update_project_test.gocli/pkg/cmd/project/create_test.gocli/pkg/cmd/project/get.gocli/pkg/cmd/project/get_test.gocli/pkg/cmd/project/list_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- agent-manager-service/docs/api_v1_openapi.yaml
- agent-manager-service/services/monitor_manager.go
- agent-manager-service/controllers/gateway_controller.go
menakaj
requested changes
Jul 8, 2026
menakaj
approved these changes
Jul 8, 2026
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.
Purpose
Resolves #1271
This PR:
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
Summary by CodeRabbit
New Features
Bug Fixes
200and204as successful outcomes.Chores
organizationNamefrom responses and addingorgUidto gateway creation).