fix: prevent SQL injection via role attributes (PLAT-685)#436
Open
moizpgedge wants to merge 2 commits into
Open
fix: prevent SQL injection via role attributes (PLAT-685)#436moizpgedge wants to merge 2 commits into
moizpgedge wants to merge 2 commits into
Conversation
Validate database_users[].attributes against a fixed keyword allowlist in postgres.CreateUserRole instead of interpolating them unescaped into ALTER ROLE. Also bump containerd, go-git, go-billy, and the same pgx/otel/x-crypto/x-net/chi/Go-toolchain set from PLAT-686 to close the remaining Codacy findings. Risk-accept the unfixed docker/docker CVEs in .trivy/pgedge-control-plane.trivyignore.yaml. PLAT-685
|
Warning Review limit reached
Next review available in: 43 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds Docker command and PostgreSQL role-attribute validation, refactors API, database, orchestration, monitoring, migration, and workflow paths into helpers, updates Go dependencies and toolchains, pins build inputs, refreshes license records, and expands Trivy suppressions. ChangesSecurity and API validation
Configuration and lifecycle refactoring
Orchestration, migration, monitoring, and workflows
Dependency and build refresh
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/Dockerfile (1)
3-6: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
--no-cache-dirto reduce the image size.Consider adding the
--no-cache-dirflag to thepip installcommand. This preventspipfrom caching the downloaded packages, which is generally unnecessary in Docker containers and helps keep the final image size smaller.♻️ Proposed refactor
-RUN pip install \ +RUN pip install --no-cache-dir \ pymdown-extensions==11.0.1 \ mkdocs-redoc-tag==0.2.0 \ mkdocs-github-admonitions-plugin==0.1.1🤖 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 `@docs/Dockerfile` around lines 3 - 6, Update the pip install command in the Dockerfile to include the --no-cache-dir option while preserving the existing package versions and multiline installation structure.
🤖 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.
Nitpick comments:
In `@docs/Dockerfile`:
- Around line 3-6: Update the pip install command in the Dockerfile to include
the --no-cache-dir option while preserving the existing package versions and
multiline installation structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49658f9e-8be4-4fe5-bc88-f991d5845294
📒 Files selected for processing (5)
NOTICE.txtdocker/control-plane-ci/Dockerfiledocs/Dockerfileserver/internal/docker/docker.goserver/internal/docker/docker_test.go
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/orchestrator.go (1)
302-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly reuse
ResolvedImagewhen every image-selection input is unchanged.A same-version service-type change reuses the old service’s image. Adding a user
Imageoverride also leaves bothImageandResolvedImageset, violating their mutual-exclusion contract.Proposed fix
func serviceVersionUnchanged(old, new *database.ServiceInstanceSpec) bool { - return old != nil && old.ServiceSpec != nil && new.ServiceSpec != nil && - old.ServiceSpec.Version == new.ServiceSpec.Version + if old == nil || old.ServiceSpec == nil || new.ServiceSpec == nil { + return false + } + if old.ServiceSpec.ServiceType != new.ServiceSpec.ServiceType || + old.ServiceSpec.Version != new.ServiceSpec.Version { + return false + } + opts := new.ServiceSpec.OrchestratorOpts + return opts == nil || opts.Swarm == nil || opts.Swarm.Image == "" }Also applies to: 316-337
🤖 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 `@server/internal/orchestrator/swarm/orchestrator.go` around lines 302 - 310, Update ReconcileServiceInstanceSpec and its serviceVersionUnchanged decision so the old ResolvedImage is carried forward only when all image-selection inputs, including service type and user Image override, are unchanged. When any such input changes, clear the new Swarm.ResolvedImage so resolution uses the current manifest or override, preserving the mutual exclusion between Image and ResolvedImage.
🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)
190-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the error message for invalid source node chaining.
The error message "source node must refer to an existing node" is slightly misleading here, as the node does exist in the spec (it passed the
seenNodeNamescheck above). The actual failure is that the referenced node is also being provisioned (it declares its ownsource_node). Updating the message will help users diagnose the validation failure faster.💡 Proposed fix
if newNodesWithSource.Has(src) { errs = append(errs, validation.NewError( - errors.New("source node must refer to an existing node"), + errors.New("source node cannot refer to a node that is also being provisioned from a source"), srcPath, )) }🤖 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 `@server/internal/api/apiv1/validate.go` around lines 190 - 195, Update the validation error created in the newNodesWithSource check to state that source nodes cannot themselves define a source_node or be provisioned from another source, rather than claiming the referenced node does not exist. Keep the existing validation condition and srcPath unchanged.
🤖 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 `@server/internal/api/apiv1/pre_init_handlers.go`:
- Around line 106-108: In the join-credentials handling around
decodeJoinCredentials, validate opts.Credentials for nil before passing it to
the decoder. Return an appropriate API error when credentials are absent, and
preserve the existing decode error path for non-nil credentials.
In `@server/internal/config/manager.go`:
- Around line 107-126: Correct the duplicated error messages in the
configuration merge and unmarshal paths. Update each fmt.Errorf call around
combinedK.Merge and generatedK/combinedK.Unmarshal to identify the specific
operation and configuration layer that failed, while preserving the existing
wrapped errors and return behavior.
In `@server/internal/monitor/instance_monitor.go`:
- Around line 90-93: Update checkStatus and the available-state transition
around updateInstanceStatus so failures from UpdateInstanceState are returned
instead of discarded. Ensure checkStatus does not publish a healthy status when
the instance state update fails, while preserving the existing status update
flow on success.
In `@server/internal/orchestrator/swarm/mcp_config.go`:
- Around line 246-273: In server/internal/orchestrator/swarm/mcp_config.go lines
246-273, update buildMCPKBConfig to assume validated input: remove the defensive
nil checks and related comment, and change it to return only *mcpKBConfig. In
server/internal/orchestrator/swarm/mcp_config.go lines 114-117, update the
caller to assign the direct result without handling an error or retaining the
removed error variable.
In `@server/internal/orchestrator/swarm/service_spec.go`:
- Around line 155-158: Update the extra-label merge in the swarm service
specification so entries from swarmOpts.ExtraLabels cannot overwrite the
control-plane identity labels pgedge.component or pgedge.service.instance.id.
Preserve all other user-provided labels, and ensure the control-plane values
remain authoritative for ServiceInspectByLabels reconciliation.
In `@server/internal/workflows/backend/etcd/etcd.go`:
- Around line 661-668: Update the transaction error handling around the
`b.store.Txn(...).Commit(ctx)` call so only the specific transaction-conflict
error returns `(nil, nil)` as a lock race; propagate all other etcd or
serialization errors to the caller instead of suppressing them.
- Around line 647-652: Update the lock reassignment branch in the activity-lock
handling around lock.CanBeReassignedTo so the Update operation uses
b.options.ActivityLockTimeout instead of b.options.WorkflowLockTimeout. Keep the
existing reassignment and update flow unchanged.
In `@server/internal/workflows/delete_database.go`:
- Around line 127-130: Update deleteDatabaseResources so the error returned by
operations.UpdateDatabase is passed through handleError before returning,
instead of being returned directly. Preserve the existing planning error context
while ensuring the caller receives handleError’s result and the database/task
failure state is recorded.
In `@server/internal/workflows/failover.go`:
- Around line 79-82: Update the skipped-candidate branch in the failover
workflow to propagate errors returned by updateFailoverTask instead of
discarding them. Preserve the existing success response when task completion
succeeds, but return the completion error so the workflow cannot report success
while the task remains running.
---
Outside diff comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 302-310: Update ReconcileServiceInstanceSpec and its
serviceVersionUnchanged decision so the old ResolvedImage is carried forward
only when all image-selection inputs, including service type and user Image
override, are unchanged. When any such input changes, clear the new
Swarm.ResolvedImage so resolution uses the current manifest or override,
preserving the mutual exclusion between Image and ResolvedImage.
---
Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 190-195: Update the validation error created in the
newNodesWithSource check to state that source nodes cannot themselves define a
source_node or be provisioned from another source, rather than claiming the
referenced node does not exist. Keep the existing validation condition and
srcPath unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 885d23dc-d6a3-4da6-981d-98d183d4cb8f
📒 Files selected for processing (49)
api/apiv1/design/database.goapi/apiv1/design/task.godocker/control-plane/Dockerfiledocs/Dockerfileserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/pre_init_handlers.goserver/internal/api/apiv1/validate.goserver/internal/config/config.goserver/internal/config/manager.goserver/internal/database/instance_resource.goserver/internal/database/operations/end.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/update_database.goserver/internal/database/postgres_database.goserver/internal/database/reconcile_versions.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/script.goserver/internal/database/service.goserver/internal/database/spec.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/etcd/provide.goserver/internal/etcd/rbac.goserver/internal/ipam/service.goserver/internal/migrate/migrations/add_task_scope.goserver/internal/monitor/instance_monitor.goserver/internal/monitor/provide.goserver/internal/monitor/service_instance_monitor.goserver/internal/orchestrator/common/patroni_config.goserver/internal/orchestrator/common/postgres_certs.goserver/internal/orchestrator/swarm/mcp_config.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/pgbackrest_restore.goserver/internal/orchestrator/swarm/rag_config.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/systemd/pgbackrest_restore.goserver/internal/orchestrator/systemd/provide.goserver/internal/orchestrator/systemd/unit_options.goserver/internal/pgbackrest/config.goserver/internal/postgres/hba/parse.goserver/internal/resource/migrations/1_0_0.goserver/internal/resource/migrations/1_2_0.goserver/internal/resource/migrations/schematool/identifier.goserver/internal/resource/topo.goserver/internal/workflows/activities/check_cluster_health.goserver/internal/workflows/activities/select_candidate_instance.goserver/internal/workflows/backend/etcd/etcd.goserver/internal/workflows/delete_database.goserver/internal/workflows/failover.go
Comment on lines
+106
to
+108
| credentials, err := decodeJoinCredentials(opts.Credentials) | ||
| if err != nil { | ||
| return apiErr(fmt.Errorf("failed to decode CA certificate: %w", err)) | ||
| } | ||
| clientCert, err := base64.StdEncoding.DecodeString(opts.Credentials.ClientCert) | ||
| if err != nil { | ||
| return apiErr(fmt.Errorf("failed to decode client certificate: %w", err)) | ||
| } | ||
| clientKey, err := base64.StdEncoding.DecodeString(opts.Credentials.ClientKey) | ||
| if err != nil { | ||
| return apiErr(fmt.Errorf("failed to decode client key: %w", err)) | ||
| } | ||
| serverCert, err := base64.StdEncoding.DecodeString(opts.Credentials.ServerCert) | ||
| if err != nil { | ||
| return apiErr(fmt.Errorf("failed to decode server certificate: %w", err)) | ||
| } | ||
| serverKey, err := base64.StdEncoding.DecodeString(opts.Credentials.ServerKey) | ||
| if err != nil { | ||
| return apiErr(fmt.Errorf("failed to decode server key: %w", err)) | ||
| return apiErr(err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent nil-pointer dereference from external API response.
opts.Credentials is derived from an external API call (cli.GetJoinOptions) and passed directly into decodeJoinCredentials. If the response omits credentials, decodeJoinCredentials will panic when attempting to access creds.CaCert. Add a nil guard before processing the credentials.
🛡️ Proposed fix
+ if opts.Credentials == nil {
+ return apiErr(errors.New("cluster credentials are required but were not provided"))
+ }
credentials, err := decodeJoinCredentials(opts.Credentials)
if err != nil {
return apiErr(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.
Suggested change
| credentials, err := decodeJoinCredentials(opts.Credentials) | |
| if err != nil { | |
| return apiErr(fmt.Errorf("failed to decode CA certificate: %w", err)) | |
| } | |
| clientCert, err := base64.StdEncoding.DecodeString(opts.Credentials.ClientCert) | |
| if err != nil { | |
| return apiErr(fmt.Errorf("failed to decode client certificate: %w", err)) | |
| } | |
| clientKey, err := base64.StdEncoding.DecodeString(opts.Credentials.ClientKey) | |
| if err != nil { | |
| return apiErr(fmt.Errorf("failed to decode client key: %w", err)) | |
| } | |
| serverCert, err := base64.StdEncoding.DecodeString(opts.Credentials.ServerCert) | |
| if err != nil { | |
| return apiErr(fmt.Errorf("failed to decode server certificate: %w", err)) | |
| } | |
| serverKey, err := base64.StdEncoding.DecodeString(opts.Credentials.ServerKey) | |
| if err != nil { | |
| return apiErr(fmt.Errorf("failed to decode server key: %w", err)) | |
| return apiErr(err) | |
| if opts.Credentials == nil { | |
| return apiErr(errors.New("cluster credentials are required but were not provided")) | |
| } | |
| credentials, err := decodeJoinCredentials(opts.Credentials) | |
| if err != nil { | |
| return apiErr(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 `@server/internal/api/apiv1/pre_init_handlers.go` around lines 106 - 108, In
the join-credentials handling around decodeJoinCredentials, validate
opts.Credentials for nil before passing it to the decoder. Return an appropriate
API error when credentials are absent, and preserve the existing decode error
path for non-nil credentials.
Comment on lines
+107
to
+126
| if err := combinedK.Merge(generatedK); err != nil { | ||
| return nil, fmt.Errorf("failed to merge user and generated configs: %w", err) | ||
| } | ||
| if err := combinedK.Merge(userK); err != nil { | ||
| return nil, fmt.Errorf("failed to merge user and generated configs: %w", err) | ||
| } | ||
|
|
||
| return combinedK, nil | ||
| } | ||
|
|
||
| func unmarshalConfigs(generatedK, combinedK *koanf.Koanf) (Config, Config, error) { | ||
| var generated Config | ||
| if err := generatedK.Unmarshal("", &generated); err != nil { | ||
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %w", err) | ||
| } | ||
|
|
||
| var combined Config | ||
| if err := combinedK.Unmarshal("", &combined); err != nil { | ||
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %w", err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix copy-paste errors in wrapped error messages.
The error wrapping messages are duplicated across different merge and unmarshal steps. Correcting them will make it much easier to identify which specific configuration layer failed to load or merge.
🛠️ Proposed fix
if err := combinedK.Merge(generatedK); err != nil {
- return nil, fmt.Errorf("failed to merge user and generated configs: %w", err)
+ return nil, fmt.Errorf("failed to merge generated config: %w", err)
}
if err := combinedK.Merge(userK); err != nil {
- return nil, fmt.Errorf("failed to merge user and generated configs: %w", err)
+ return nil, fmt.Errorf("failed to merge user config: %w", err)
}
return combinedK, nil
}
func unmarshalConfigs(generatedK, combinedK *koanf.Koanf) (Config, Config, error) {
var generated Config
if err := generatedK.Unmarshal("", &generated); err != nil {
- return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %w", err)
+ return Config{}, Config{}, fmt.Errorf("failed to unmarshal generated config: %w", err)
}
var combined Config📝 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.
Suggested change
| if err := combinedK.Merge(generatedK); err != nil { | |
| return nil, fmt.Errorf("failed to merge user and generated configs: %w", err) | |
| } | |
| if err := combinedK.Merge(userK); err != nil { | |
| return nil, fmt.Errorf("failed to merge user and generated configs: %w", err) | |
| } | |
| return combinedK, nil | |
| } | |
| func unmarshalConfigs(generatedK, combinedK *koanf.Koanf) (Config, Config, error) { | |
| var generated Config | |
| if err := generatedK.Unmarshal("", &generated); err != nil { | |
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %w", err) | |
| } | |
| var combined Config | |
| if err := combinedK.Unmarshal("", &combined); err != nil { | |
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %w", err) | |
| } | |
| if err := combinedK.Merge(generatedK); err != nil { | |
| return nil, fmt.Errorf("failed to merge generated config: %w", err) | |
| } | |
| if err := combinedK.Merge(userK); err != nil { | |
| return nil, fmt.Errorf("failed to merge user config: %w", err) | |
| } | |
| return combinedK, nil | |
| } | |
| func unmarshalConfigs(generatedK, combinedK *koanf.Koanf) (Config, Config, error) { | |
| var generated Config | |
| if err := generatedK.Unmarshal("", &generated); err != nil { | |
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal generated config: %w", err) | |
| } | |
| var combined Config | |
| if err := combinedK.Unmarshal("", &combined); err != nil { | |
| return Config{}, Config{}, fmt.Errorf("failed to unmarshal combined config: %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 `@server/internal/config/manager.go` around lines 107 - 126, Correct the
duplicated error messages in the configuration merge and unmarshal paths. Update
each fmt.Errorf call around combinedK.Merge and generatedK/combinedK.Unmarshal
to identify the specific operation and configuration layer that failed, while
preserving the existing wrapped errors and return behavior.
Comment on lines
+90
to
+93
| if err := m.refreshInstanceStateIfNeeded(ctx); err != nil { | ||
| return m.updateInstanceErrStatus(ctx, status, err) | ||
| } | ||
| return m.updateInstanceStatus(ctx, status) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate failures when transitioning the instance to available.
Line 126 discards UpdateInstanceState errors, so checkStatus can publish a healthy status while the instance remains in its previous state.
Proposed fix
if currentInstance != nil && currentInstance.State != database.InstanceStateAvailable {
- _ = m.dbSvc.UpdateInstanceState(ctx, &database.InstanceStateUpdateOptions{
+ if err := m.dbSvc.UpdateInstanceState(ctx, &database.InstanceStateUpdateOptions{
InstanceID: m.instanceID,
DatabaseID: m.databaseID,
State: database.InstanceStateAvailable,
- })
+ }); err != nil {
+ return fmt.Errorf("failed to mark instance available: %w", err)
+ }
}Also applies to: 120-132
🤖 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 `@server/internal/monitor/instance_monitor.go` around lines 90 - 93, Update
checkStatus and the available-state transition around updateInstanceStatus so
failures from UpdateInstanceState are returned instead of discarded. Ensure
checkStatus does not publish a healthy status when the instance state update
fails, while preserving the existing status update flow on success.
Comment on lines
+246
to
+273
| // buildMCPKBConfig builds the knowledgebase section, or nil when kb_enabled is | ||
| // not true. Returns an error if kb_enabled is true but required fields are nil, | ||
| // which indicates upstream validation was bypassed. | ||
| func buildMCPKBConfig(cfg *database.MCPServiceConfig) (*mcpKBConfig, error) { | ||
| if cfg.KBEnabled == nil || !*cfg.KBEnabled { | ||
| return nil, nil | ||
| } | ||
| if cfg.KBEmbeddingProvider == nil || cfg.KBEmbeddingModel == nil || cfg.KBEmbeddingAPIKey == nil { | ||
| return nil, fmt.Errorf("internal: KB provider/model/key nil despite kb_enabled=true; validation was bypassed") | ||
| } | ||
| containerPath := "/app/kb/nla-kb.db" | ||
| if cfg.KBDatabaseHostPath != nil { | ||
| containerPath = "/app/kb/" + filepath.Base(*cfg.KBDatabaseHostPath) | ||
| } | ||
| k := &mcpKBConfig{ | ||
| Enabled: true, | ||
| DatabasePath: containerPath, | ||
| EmbeddingProvider: *cfg.KBEmbeddingProvider, | ||
| EmbeddingModel: *cfg.KBEmbeddingModel, | ||
| } | ||
| switch *cfg.KBEmbeddingProvider { | ||
| case "voyage": | ||
| k.EmbeddingVoyageAPIKey = *cfg.KBEmbeddingAPIKey | ||
| case "openai": | ||
| k.EmbeddingOpenAIAPIKey = *cfg.KBEmbeddingAPIKey | ||
| } | ||
| return k, nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove defensive nil guards and assume validated input.
Internal helpers called through controlled paths should assume validated input. Based on learnings, do not add defensive nil guards (like checking if validation was bypassed) inside these internal helpers.
server/internal/orchestrator/swarm/mcp_config.go#L246-L273: Remove the defensive nil checks, update the function signature to return only*mcpKBConfig, and remove the related comment.server/internal/orchestrator/swarm/mcp_config.go#L114-L117: Update the caller to remove the error check and variable assignment.
🛠️ Proposed refactor
--- server/internal/orchestrator/swarm/mcp_config.go
+++ server/internal/orchestrator/swarm/mcp_config.go
@@ -113,10 +113,7 @@
- kb, err := buildMCPKBConfig(cfg)
- if err != nil {
- return nil, err
- }
+ kb := buildMCPKBConfig(cfg)
yamlCfg := &mcpYAMLConfig{
@@ -245,14 +242,10 @@
-// buildMCPKBConfig builds the knowledgebase section, or nil when kb_enabled is
-// not true. Returns an error if kb_enabled is true but required fields are nil,
-// which indicates upstream validation was bypassed.
-func buildMCPKBConfig(cfg *database.MCPServiceConfig) (*mcpKBConfig, error) {
+// buildMCPKBConfig builds the knowledgebase section, or nil when kb_enabled is not true.
+func buildMCPKBConfig(cfg *database.MCPServiceConfig) *mcpKBConfig {
if cfg.KBEnabled == nil || !*cfg.KBEnabled {
- return nil, nil
- }
- if cfg.KBEmbeddingProvider == nil || cfg.KBEmbeddingModel == nil || cfg.KBEmbeddingAPIKey == nil {
- return nil, fmt.Errorf("internal: KB provider/model/key nil despite kb_enabled=true; validation was bypassed")
+ return nil
}
containerPath := "/app/kb/nla-kb.db"
if cfg.KBDatabaseHostPath != nil {
containerPath = "/app/kb/" + filepath.Base(*cfg.KBDatabaseHostPath)
}
k := &mcpKBConfig{
Enabled: true,
DatabasePath: containerPath,
EmbeddingProvider: *cfg.KBEmbeddingProvider,
EmbeddingModel: *cfg.KBEmbeddingModel,
}
switch *cfg.KBEmbeddingProvider {
case "voyage":
k.EmbeddingVoyageAPIKey = *cfg.KBEmbeddingAPIKey
case "openai":
k.EmbeddingOpenAIAPIKey = *cfg.KBEmbeddingAPIKey
}
- return k, nil
+ return k
}📝 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.
Suggested change
| // buildMCPKBConfig builds the knowledgebase section, or nil when kb_enabled is | |
| // not true. Returns an error if kb_enabled is true but required fields are nil, | |
| // which indicates upstream validation was bypassed. | |
| func buildMCPKBConfig(cfg *database.MCPServiceConfig) (*mcpKBConfig, error) { | |
| if cfg.KBEnabled == nil || !*cfg.KBEnabled { | |
| return nil, nil | |
| } | |
| if cfg.KBEmbeddingProvider == nil || cfg.KBEmbeddingModel == nil || cfg.KBEmbeddingAPIKey == nil { | |
| return nil, fmt.Errorf("internal: KB provider/model/key nil despite kb_enabled=true; validation was bypassed") | |
| } | |
| containerPath := "/app/kb/nla-kb.db" | |
| if cfg.KBDatabaseHostPath != nil { | |
| containerPath = "/app/kb/" + filepath.Base(*cfg.KBDatabaseHostPath) | |
| } | |
| k := &mcpKBConfig{ | |
| Enabled: true, | |
| DatabasePath: containerPath, | |
| EmbeddingProvider: *cfg.KBEmbeddingProvider, | |
| EmbeddingModel: *cfg.KBEmbeddingModel, | |
| } | |
| switch *cfg.KBEmbeddingProvider { | |
| case "voyage": | |
| k.EmbeddingVoyageAPIKey = *cfg.KBEmbeddingAPIKey | |
| case "openai": | |
| k.EmbeddingOpenAIAPIKey = *cfg.KBEmbeddingAPIKey | |
| } | |
| return k, nil | |
| } | |
| kb := buildMCPKBConfig(cfg) | |
| yamlCfg := &mcpYAMLConfig{ | |
| // ... | |
| } | |
| // buildMCPKBConfig builds the knowledgebase section, or nil when kb_enabled is not true. | |
| func buildMCPKBConfig(cfg *database.MCPServiceConfig) *mcpKBConfig { | |
| if cfg.KBEnabled == nil || !*cfg.KBEnabled { | |
| return nil | |
| } | |
| containerPath := "/app/kb/nla-kb.db" | |
| if cfg.KBDatabaseHostPath != nil { | |
| containerPath = "/app/kb/" + filepath.Base(*cfg.KBDatabaseHostPath) | |
| } | |
| k := &mcpKBConfig{ | |
| Enabled: true, | |
| DatabasePath: containerPath, | |
| EmbeddingProvider: *cfg.KBEmbeddingProvider, | |
| EmbeddingModel: *cfg.KBEmbeddingModel, | |
| } | |
| switch *cfg.KBEmbeddingProvider { | |
| case "voyage": | |
| k.EmbeddingVoyageAPIKey = *cfg.KBEmbeddingAPIKey | |
| case "openai": | |
| k.EmbeddingOpenAIAPIKey = *cfg.KBEmbeddingAPIKey | |
| } | |
| return k | |
| } |
📍 Affects 1 file
server/internal/orchestrator/swarm/mcp_config.go#L246-L273(this comment)server/internal/orchestrator/swarm/mcp_config.go#L114-L117
🤖 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 `@server/internal/orchestrator/swarm/mcp_config.go` around lines 246 - 273, In
server/internal/orchestrator/swarm/mcp_config.go lines 246-273, update
buildMCPKBConfig to assume validated input: remove the defensive nil checks and
related comment, and change it to return only *mcpKBConfig. In
server/internal/orchestrator/swarm/mcp_config.go lines 114-117, update the
caller to assign the direct result without handling an error or retaining the
removed error variable.
Source: Learnings
Comment on lines
155
to
158
| if swarmOpts != nil { | ||
| for k, v := range swarmOpts.ExtraLabels { | ||
| labels[k] = v | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent extra labels from overriding control-plane identity labels.
User labels can replace pgedge.component or pgedge.service.instance.id, breaking subsequent ServiceInspectByLabels reconciliation and potentially creating duplicate services.
Proposed fix
- if swarmOpts != nil {
- for k, v := range swarmOpts.ExtraLabels {
- labels[k] = v
- }
- }
+ if swarmOpts != nil {
+ for k, v := range swarmOpts.ExtraLabels {
+ if !strings.HasPrefix(k, "pgedge.") {
+ labels[k] = v
+ }
+ }
+ }📝 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.
Suggested change
| if swarmOpts != nil { | |
| for k, v := range swarmOpts.ExtraLabels { | |
| labels[k] = v | |
| } | |
| if swarmOpts != nil { | |
| for k, v := range swarmOpts.ExtraLabels { | |
| if !strings.HasPrefix(k, "pgedge.") { | |
| labels[k] = v | |
| } | |
| } | |
| } |
🤖 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 `@server/internal/orchestrator/swarm/service_spec.go` around lines 155 - 158,
Update the extra-label merge in the swarm service specification so entries from
swarmOpts.ExtraLabels cannot overwrite the control-plane identity labels
pgedge.component or pgedge.service.instance.id. Preserve all other user-provided
labels, and ensure the control-plane values remain authoritative for
ServiceInspectByLabels reconciliation.
Comment on lines
+647
to
+652
| case lock.CanBeReassignedTo(b.workerID, b.workerInstanceID): | ||
| lock.WorkerID = b.workerID | ||
| lock.WorkerInstanceID = b.workerInstanceID | ||
| lockOp = b.store.ActivityLock. | ||
| Update(lock). | ||
| WithTTL(b.options.WorkflowLockTimeout) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the activity lock timeout when reassigning activity locks.
New locks use ActivityLockTimeout, but reassigned locks use WorkflowLockTimeout. This changes activity lock expiry based solely on whether the lock was reassigned.
lockOp = b.store.ActivityLock.
Update(lock).
- WithTTL(b.options.WorkflowLockTimeout)
+ WithTTL(b.options.ActivityLockTimeout)📝 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.
Suggested change
| case lock.CanBeReassignedTo(b.workerID, b.workerInstanceID): | |
| lock.WorkerID = b.workerID | |
| lock.WorkerInstanceID = b.workerInstanceID | |
| lockOp = b.store.ActivityLock. | |
| Update(lock). | |
| WithTTL(b.options.WorkflowLockTimeout) | |
| case lock.CanBeReassignedTo(b.workerID, b.workerInstanceID): | |
| lock.WorkerID = b.workerID | |
| lock.WorkerInstanceID = b.workerInstanceID | |
| lockOp = b.store.ActivityLock. | |
| Update(lock). | |
| WithTTL(b.options.ActivityLockTimeout) |
🤖 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 `@server/internal/workflows/backend/etcd/etcd.go` around lines 647 - 652,
Update the lock reassignment branch in the activity-lock handling around
lock.CanBeReassignedTo so the Update operation uses
b.options.ActivityLockTimeout instead of b.options.WorkflowLockTimeout. Keep the
existing reassignment and update flow unchanged.
Comment on lines
+661
to
+668
| err = b.store.Txn( | ||
| lockOp, | ||
| b.store.ActivityQueueItem.Update(item), | ||
| ).Commit(ctx) | ||
| if err != nil { | ||
| // Another worker managed to lock this first | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not treat every transaction failure as a lock race.
Line 665 suppresses all commit errors, including etcd or serialization failures. Only the transaction-conflict error should return (nil, nil); propagate other failures so workers do not silently stop receiving tasks.
🤖 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 `@server/internal/workflows/backend/etcd/etcd.go` around lines 661 - 668,
Update the transaction error handling around the `b.store.Txn(...).Commit(ctx)`
call so only the specific transaction-conflict error returns `(nil, nil)` as a
lock race; propagate all other etcd or serialization errors to the caller
instead of suppressing them.
Comment on lines
+127
to
+130
| func (w *Workflows) deleteDatabaseResources(ctx workflow.Context, input *DeleteDatabaseInput, current *resource.State, handleError func(error) error) error { | ||
| plans, err := operations.UpdateDatabase(operations.UpdateDatabaseOptions{}, current, nil, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to plan database delete: %w", err) | ||
| return fmt.Errorf("failed to plan database delete: %w", err) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Route planning failures through handleError.
This is the only failure in deleteDatabaseResources returned directly. The caller also returns it unchanged, so the database and task are not marked failed.
plans, err := operations.UpdateDatabase(operations.UpdateDatabaseOptions{}, current, nil, nil)
if err != nil {
- return fmt.Errorf("failed to plan database delete: %w", err)
+ return handleError(fmt.Errorf("failed to plan database delete: %w", 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.
Suggested change
| func (w *Workflows) deleteDatabaseResources(ctx workflow.Context, input *DeleteDatabaseInput, current *resource.State, handleError func(error) error) error { | |
| plans, err := operations.UpdateDatabase(operations.UpdateDatabaseOptions{}, current, nil, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to plan database delete: %w", err) | |
| return fmt.Errorf("failed to plan database delete: %w", err) | |
| func (w *Workflows) deleteDatabaseResources(ctx workflow.Context, input *DeleteDatabaseInput, current *resource.State, handleError func(error) error) error { | |
| plans, err := operations.UpdateDatabase(operations.UpdateDatabaseOptions{}, current, nil, nil) | |
| if err != nil { | |
| return handleError(fmt.Errorf("failed to plan database delete: %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 `@server/internal/workflows/delete_database.go` around lines 127 - 130, Update
deleteDatabaseResources so the error returned by operations.UpdateDatabase is
passed through handleError before returning, instead of being returned directly.
Preserve the existing planning error context while ensuring the caller receives
handleError’s result and the database/task failure state is recorded.
Comment on lines
+79
to
+82
| if candidateID == leaderInstanceID { | ||
| logger.Info("candidate is already the leader; skipping failover", "candidate", candidateID) | ||
| _ = w.updateFailoverTask(ctx, in, task.UpdateComplete()) | ||
| return true, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate task-completion failures on the skipped path.
Ignoring updateFailoverTask here can return workflow success while leaving the task in running.
if candidateID == leaderInstanceID {
logger.Info("candidate is already the leader; skipping failover", "candidate", candidateID)
- _ = w.updateFailoverTask(ctx, in, task.UpdateComplete())
+ if err := w.updateFailoverTask(ctx, in, task.UpdateComplete()); err != nil {
+ return false, fmt.Errorf("failed to mark skipped failover complete: %w", err)
+ }
return true, nil
}📝 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.
Suggested change
| if candidateID == leaderInstanceID { | |
| logger.Info("candidate is already the leader; skipping failover", "candidate", candidateID) | |
| _ = w.updateFailoverTask(ctx, in, task.UpdateComplete()) | |
| return true, nil | |
| if candidateID == leaderInstanceID { | |
| logger.Info("candidate is already the leader; skipping failover", "candidate", candidateID) | |
| if err := w.updateFailoverTask(ctx, in, task.UpdateComplete()); err != nil { | |
| return false, fmt.Errorf("failed to mark skipped failover complete: %w", err) | |
| } | |
| return true, nil |
🤖 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 `@server/internal/workflows/failover.go` around lines 79 - 82, Update the
skipped-candidate branch in the failover workflow to propagate errors returned
by updateFailoverTask instead of discarding them. Preserve the existing success
response when task completion succeeds, but return the completion error so the
workflow cannot report success while the task remains running.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/internal/docker/docker.go (1)
81-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize the executable path before checking the shell denylist.
Equivalent paths such as
/usr/bin/sh,/usr/local/bin/bash, and./dashbypass the exact-string set and still enable-ccommand interpretation. Comparepath.Base(command[0])against interpreter names instead.Proposed fix
- if shellInterpreters.Has(command[0]) { + if shellInterpreters.Has(path.Base(command[0])) {Add the standard-library
pathimport.🤖 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 `@server/internal/docker/docker.go` around lines 81 - 82, Update the shell interpreter check in the command validation logic to compare path.Base(command[0]) with shellInterpreters instead of the raw executable path. Add the standard-library path import and preserve the existing ErrShellInterpreterNotAllowed error behavior.server/internal/database/service.go (1)
771-775: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore allocated fields during rollback.
The rollback releases newly allocated ports but leaves
spec.Portandspec.PatroniPortpopulated. A retry can therefore skip allocation and persist a port that has already returned to the pool.Proposed fix
func (s *Service) allocateInstanceSpecPorts(ctx context.Context, spec *InstanceSpec) (func(error) error, error) { + originalPort := spec.Port + originalPatroniPort := spec.PatroniPort var allocated []int rollback := func(cause error) error { rollbackCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() - return errors.Join(cause, s.portsSvc.ReleasePort(rollbackCtx, spec.HostID, allocated...)) + releaseErr := s.portsSvc.ReleasePort(rollbackCtx, spec.HostID, allocated...) + spec.Port = originalPort + spec.PatroniPort = originalPatroniPort + return errors.Join(cause, releaseErr) }Based on learnings, reconciliation retries must remain idempotent and non-destructive.
Also applies to: 778-794
🤖 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 `@server/internal/database/service.go` around lines 771 - 775, Update the rollback closure to restore the allocation fields after releasing the ports: clear spec.Port and spec.PatroniPort before returning, while preserving the existing joined-error behavior and timeout cleanup. Ensure retries re-enter port allocation rather than reusing released ports.Source: Learnings
server/internal/workflows/backend/etcd/etcd.go (1)
591-639: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStop routing events to an already-existing sub-workflow.
When the child exists,
appendSubWorkflowStartOpsenqueuesSubWorkflowFailed, but Line 599 still sends the original start event to that existing execution. Return a routing flag and skip the event loop for this collision.Proposed control-flow fix
-ops, err = b.appendSubWorkflowStartOps(...) +var routeEvents bool +ops, routeEvents, err = b.appendSubWorkflowStartOps(...) if err != nil { return nil, err } +if !routeEvents { + continue +}🤖 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 `@server/internal/workflows/backend/etcd/etcd.go` around lines 591 - 639, Update appendSubWorkflowStartOps and its caller to return a routing flag indicating whether the child already exists; when that collision is detected, enqueue SubWorkflowFailed but mark the event batch as not routable. In the caller’s event-processing flow, skip the loop that appends PendingEvent records for the original start event when the flag is false, while preserving normal routing for newly created sub-workflows.server/internal/workflows/service.go (1)
503-516: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not orphan a running remove-host workflow when task linkage fails.
CreateWorkflowInstancemay succeed beforeUpdateTaskfails.abortTasksonly changes task metadata, so host removal can continue while the caller receives an error and potentially retries.Persist a deterministic workflow instance ID before dispatch, or cancel the created workflow when linkage cannot be stored.
🤖 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 `@server/internal/workflows/service.go` around lines 503 - 516, The remove-host flow around CreateWorkflowInstance and UpdateTask must not leave a successfully created workflow running when task linkage fails. Persist a deterministic workflow instance ID before dispatch where supported, or cancel the created workflow instance before returning the UpdateTask error; retain abortTasks and the existing error propagation.
🤖 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 `@server/internal/api/apiv1/post_init_handlers.go`:
- Line 510: Normalize the optional req.Options before calling
startBackupWorkflow in the relevant initialization handlers, including the paths
around the calls at lines 510 and 569-572. Ensure omitted options remain safely
represented as nil or are replaced with the established default without allowing
startBackupWorkflow to dereference a nil value.
- Around line 597-601: Make the switchover creation flow around
checkNoActiveSwitchover and startSwitchoverWorkflow atomic so concurrent
requests cannot create multiple switchovers. Use a transaction-scoped lock or
deterministic unique key/workflow instance ID enforced by the persistence layer,
and ensure conflicting creations are rejected while preserving the existing
successful workflow path.
In `@server/internal/database/mcp_service_config.go`:
- Around line 202-216: Update parseLLMFields so ollamaURL is parsed whenever
either the LLM configuration uses Ollama or embeddingProvider is Ollama,
including the llm_enabled path. Preserve the existing parseLLMFieldsEnabled
results while supplying the shared ollama_url for OpenAI-LLM/Ollama-embedding
configurations.
In `@server/internal/etcd/embedded.go`:
- Around line 667-675: Update finalizeClientModeConfig to persist the
client-mode generated configuration before calling os.RemoveAll(e.etcdDir()). If
cfg.UpdateGeneratedConfig fails, return the existing wrapped error without
deleting the embedded data; only remove the data directory after the
configuration update succeeds, preserving the existing removal error handling.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 1210-1241: Populate the InstanceID field in every
database.ValidationResult constructed by the validation paths around
parseImageTag and the additional reported ranges, using the current instance’s
identifier from cur. Preserve the existing validation status, node, host, and
error contents while ensuring all returned results carry the corresponding
InstanceID.
In `@server/internal/postgres/create_db.go`:
- Around line 184-210: Update subInterfaceSwapStatements to generate a
collision-resistant interfaceName suffix instead of using time.Now().Unix()
second resolution. Preserve the existing providerName-based naming format and
ensure concurrent updates or immediate retries produce distinct names before
node_add_interface executes.
In `@server/internal/workflows/switchover.go`:
- Around line 183-186: Update the candidateID == leaderInstanceID skip path to
capture and propagate errors returned by w.updateSwitchoverTask when marking the
task complete. Return the existing success result only when the completion
update succeeds; otherwise return the update error instead of ignoring it.
---
Outside diff comments:
In `@server/internal/database/service.go`:
- Around line 771-775: Update the rollback closure to restore the allocation
fields after releasing the ports: clear spec.Port and spec.PatroniPort before
returning, while preserving the existing joined-error behavior and timeout
cleanup. Ensure retries re-enter port allocation rather than reusing released
ports.
In `@server/internal/docker/docker.go`:
- Around line 81-82: Update the shell interpreter check in the command
validation logic to compare path.Base(command[0]) with shellInterpreters instead
of the raw executable path. Add the standard-library path import and preserve
the existing ErrShellInterpreterNotAllowed error behavior.
In `@server/internal/workflows/backend/etcd/etcd.go`:
- Around line 591-639: Update appendSubWorkflowStartOps and its caller to return
a routing flag indicating whether the child already exists; when that collision
is detected, enqueue SubWorkflowFailed but mark the event batch as not routable.
In the caller’s event-processing flow, skip the loop that appends PendingEvent
records for the original start event when the flag is false, while preserving
normal routing for newly created sub-workflows.
In `@server/internal/workflows/service.go`:
- Around line 503-516: The remove-host flow around CreateWorkflowInstance and
UpdateTask must not leave a successfully created workflow running when task
linkage fails. Persist a deterministic workflow instance ID before dispatch
where supported, or cancel the created workflow instance before returning the
UpdateTask error; retain abortTasks and the existing error propagation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0006f6b-2b21-430a-8446-c3d0a4b50b59
📒 Files selected for processing (42)
api/apiv1/design/database.goapi/apiv1/design/task.gomqtt/endpoint.goserver/cmd/root.goserver/internal/api/apiv1/convert.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/validate.goserver/internal/app/app.goserver/internal/certificates/service.goserver/internal/config/config.goserver/internal/database/mcp_service_config.goserver/internal/database/operations/update_database.goserver/internal/database/postgrest_service_config.goserver/internal/database/service.goserver/internal/docker/docker.goserver/internal/etcd/embedded.goserver/internal/orchestrator/common/etcd_creds.goserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_config.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/spec.goserver/internal/orchestrator/systemd/orchestrator.goserver/internal/postgres/create_db.goserver/internal/resource/migrations/1_0_0.goserver/internal/resource/migrations/1_2_0.goserver/internal/resource/migrations/schemas/v1_2_0/swarm.goserver/internal/resource/migrations/schematool/inliner.goserver/internal/resource/migrations/schematool/output.goserver/internal/resource/migrations/schematool/parser.goserver/internal/resource/state.goserver/internal/storage/txn.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/backend/etcd/etcd.goserver/internal/workflows/common.goserver/internal/workflows/create_pgbackrest_backup.goserver/internal/workflows/pgbackrest_restore.goserver/internal/workflows/plan_update.goserver/internal/workflows/refresh_current_state.goserver/internal/workflows/service.goserver/internal/workflows/switchover.goserver/internal/workflows/update_database.go
moizpgedge
force-pushed
the
Bug/PLAT-685/Security-Codacy-Triage-security-issues
branch
from
July 18, 2026 21:21
c7043e7 to
05f074d
Compare
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
1 participant
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.
No description provided.