Skip to content

Commit ee8d159

Browse files
committed
fix: don't mutate params in kb connectors (breaks retries)
Addresses PR review (Codex P2). The engine resolves a step's params once and reuses that map across retry attempts, so `delete(params, "_credential")` in kb/upsert and kb/query made attempt 2+ fail with a missing credential instead of retrying a transient DB error. Read the credential without mutating the shared map. Adds regression tests asserting _credential survives a failed Execute. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKgCRfMvFcZAkoVbr8k79
1 parent 2297623 commit ee8d159

2 files changed

Lines changed: 37 additions & 2 deletions

File tree

packages/engine/internal/connector/kb.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,6 @@ func (c *KBUpsertConnector) Execute(ctx context.Context, params map[string]any)
287287
if err != nil {
288288
return nil, fmt.Errorf("kb/upsert: %w", err)
289289
}
290-
delete(params, "_credential")
291290

292291
query, args, err := prepareUpsert(params)
293292
if err != nil {
@@ -316,7 +315,6 @@ func (c *KBQueryConnector) Execute(ctx context.Context, params map[string]any) (
316315
if err != nil {
317316
return nil, fmt.Errorf("kb/query: %w", err)
318317
}
319-
delete(params, "_credential")
320318

321319
query, args, err := prepareQuery(params)
322320
if err != nil {

packages/engine/internal/connector/kb_test.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,47 @@
11
package connector
22

33
import (
4+
"context"
45
"strings"
56
"testing"
67
)
78

9+
// A malformed connection string so pgx.Connect fails immediately at parse time
10+
// (no network), letting us assert the connector didn't mutate its params.
11+
const kbBadCredURL = "postgres://bad host"
12+
13+
// The engine resolves a step's params once and reuses that map across retry
14+
// attempts, so a connector must not mutate it — deleting _credential would make
15+
// attempt 2 fail with a missing credential instead of retrying.
16+
func TestKBUpsert_DoesNotMutateCredential(t *testing.T) {
17+
params := map[string]any{
18+
"_credential": map[string]string{"url": kbBadCredURL},
19+
"table": "kb_documents",
20+
"content": "x",
21+
"vector": "[1]",
22+
}
23+
if _, err := (&KBUpsertConnector{}).Execute(context.Background(), params); err == nil {
24+
t.Fatal("expected a connection error")
25+
}
26+
if _, ok := params["_credential"]; !ok {
27+
t.Error("_credential was removed from params (would break retries)")
28+
}
29+
}
30+
31+
func TestKBQuery_DoesNotMutateCredential(t *testing.T) {
32+
params := map[string]any{
33+
"_credential": map[string]string{"url": kbBadCredURL},
34+
"table": "kb_documents",
35+
"vector": "[1]",
36+
}
37+
if _, err := (&KBQueryConnector{}).Execute(context.Background(), params); err == nil {
38+
t.Fatal("expected a connection error")
39+
}
40+
if _, ok := params["_credential"]; !ok {
41+
t.Error("_credential was removed from params (would break retries)")
42+
}
43+
}
44+
845
func TestPrepareUpsert_SingleRow(t *testing.T) {
946
sql, args, err := prepareUpsert(map[string]any{
1047
"table": "kb_documents",

0 commit comments

Comments
 (0)