Fix action schema response. Use key for name and name for display name. - #80
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Registrar as ActionRegistrar
participant Config as ActionCfg.Arguments
participant Builder as FieldBuilder
participant Connector
participant Handler as handleQueryAction
Registrar->>Builder: Build fields for action (actionKey, actionCfg)
Builder->>Config: Iterate (k, argCfg)
note right of Config: `DisplayName` removed from schema\n`argCfg.Name` remains
Builder->>Builder: Create Field(Name = k, DisplayName = argCfg.Name, Type/Defaults...)
Builder->>Registrar: Add Field
Registrar->>Connector: Register action callback (captures actionKey)
Connector->>Handler: Invoke handleQueryAction(ctx, actionKey, actionCfg, args)
note right of Handler: New param `actionKey` passed into handler
Handler-->>Connector: Return result / annotations / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fe949d3 to
7f1cc06
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7f1cc06 to
b26b58a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/action.go (1)
171-186: Critical: Argument lookup uses v.Name but schema exposes k (nil deref/empty values).Schema sets Field.Name=k, but handler reads args.Fields[v.Name]. Use k consistently to fetch values.
Apply:
- case "string": - argMap[k] = args.Fields[v.Name].GetStringValue() + case "string": + argMap[k] = args.Fields[k].GetStringValue() - case "boolean": - argMap[k] = args.Fields[v.Name].GetBoolValue() + case "boolean": + argMap[k] = args.Fields[k].GetBoolValue() - case "number": - argMap[k] = args.Fields[v.Name].GetNumberValue() + case "number": + argMap[k] = args.Fields[k].GetNumberValue() - case "string_list": - values := args.Fields[v.Name].GetListValue().GetValues() + case "string_list": + values := args.Fields[k].GetListValue().GetValues() - case "string_map": - argMap[k] = args.Fields[v.Name].GetStructValue().AsMap() + case "string_map": + argMap[k] = args.Fields[k].GetStructValue().AsMap()
🧹 Nitpick comments (4)
pkg/connector/action.go (4)
60-66: Tighter default-value validation messages; fix bool string parse shadowing.Improve diagnostics to include argument key and expected type, and avoid shadowing that reports the wrong type on parse failure.
- return nil, fmt.Errorf("invalid string default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected string, got %T", k, actionKey, defaultValue) - case "boolean": + case "boolean": boolField := &config_sdk.BoolField{} switch v := defaultValue.(type) { case nil: case bool: boolField.DefaultValue = v case string: - defaultValue, err := strconv.ParseBool(v) - if err != nil { - return nil, fmt.Errorf("invalid boolean default for %s: %T", actionKey, defaultValue) - } - boolField.DefaultValue = defaultValue + parsed, err := strconv.ParseBool(v) + if err != nil { + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected boolean, got %q", k, actionKey, v) + } + boolField.DefaultValue = parsed default: - return nil, fmt.Errorf("invalid boolean default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected boolean, got %T", k, actionKey, defaultValue) } - return nil, fmt.Errorf("invalid numeric default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected number, got %T", k, actionKey, defaultValue) - return nil, fmt.Errorf("invalid string slice default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected []string, got %T", k, actionKey, defaultValue) - return nil, fmt.Errorf("invalid string map default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid default for argument %q in action %q: expected map[string]*anypb.Any, got %T", k, actionKey, defaultValue)Also applies to: 76-76, 85-92, 104-106, 114-116, 124-126
60-61: Deterministic argument order in schema (sort keys).Map iteration order is random; sort keys to keep a stable UX and predictable diffs.
- for k, argCfg := range actionCfg.Arguments { + keys := make([]string, 0, len(actionCfg.Arguments)) + for k := range actionCfg.Arguments { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + argCfg := actionCfg.Arguments[k] arg := &config_sdk.Field{ Name: k, DisplayName: argCfg.Name,Add import:
import ( "context" "fmt" + "sort" "reflect" "strconv"Also applies to: 128-129
3-19: Import grouping/order per guidelines.Group stdlib, then third-party, then project imports; alphabetize within groups. Also include the new sort import if you adopt key sorting.
-import ( - "context" - "fmt" - "reflect" - "strconv" - - config_sdk "github.com/conductorone/baton-sdk/pb/c1/config/v1" - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" - "github.com/conductorone/baton-sdk/pkg/actions" - "github.com/conductorone/baton-sdk/pkg/annotations" - "github.com/conductorone/baton-sdk/pkg/connectorbuilder" - "github.com/conductorone/baton-sql/pkg/bsql" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" - "go.uber.org/zap" - "google.golang.org/protobuf/types/known/anypb" - "google.golang.org/protobuf/types/known/structpb" -) +import ( + // Standard library + "context" + "fmt" + "reflect" + "sort" + "strconv" + + // Third-party + config_sdk "github.com/conductorone/baton-sdk/pb/c1/config/v1" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/actions" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/structpb" + + // Project + "github.com/conductorone/baton-sql/pkg/bsql" +)As per coding guidelines.
21-28: Add doc comment for exported symbol or make it unexported.Exported ActionTypeMap lacks a comment. Either document it or unexport it if not needed outside the package.
- var ActionTypeMap = map[string]v2.ActionType{ + // ActionTypeMap maps YAML/JSON action_type strings to the connector v2 ActionType enum. + var ActionTypeMap = map[string]v2.ActionType{As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/bsql/config.go(0 hunks)pkg/connector/action.go(5 hunks)
💤 Files with no reviewable changes (1)
- pkg/bsql/config.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/connector/action.go
🧬 Code graph analysis (1)
pkg/connector/action.go (2)
pkg/connector/connector.go (1)
Connector(18-23)pkg/bsql/config.go (1)
ActionConfig(412-422)
🔇 Additional comments (2)
pkg/connector/action.go (2)
60-66: Schema name/display name mapping change LGTM.Using the map key for Field.Name and argCfg.Name for DisplayName aligns with config changes and fixes prior ambiguity.
Please confirm no remaining reads rely on argCfg.Name as the input field key outside this file (e.g., template variable resolution).
135-139: No change required — module targets Go 1.25 so per-iteration loop variables are used.go.mod declares go 1.25; Go 1.22 changed for-loop semantics so each iteration creates fresh variables and the classic closure-capture issue no longer applies. (tip.golang.org)
Copying actionKey (ak := actionKey) is harmless but optional.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/action.go (2)
80-95: Fix shadowed variable and improve boolean default error message.Shadowing defaultValue changes %T to bool in the error path; also omit arg key.
- case string: - defaultValue, err := strconv.ParseBool(v) + case string: + parsed, err := strconv.ParseBool(v) if err != nil { - return nil, fmt.Errorf("invalid boolean default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid boolean default for action %q arg %q: %q", actionKey, k, v) } - boolField.DefaultValue = defaultValue + boolField.DefaultValue = parsedAs per coding guidelines.
96-107: Validate non-integer numeric defaults and accept numeric strings; include arg key.Silent float→int truncation is risky; also allow "42" strings.
case "number": intField := &config_sdk.IntField{} switch v := defaultValue.(type) { case nil: case int, int32, int64: intField.DefaultValue = reflect.ValueOf(v).Int() case float32, float64: - intField.DefaultValue = int64(reflect.ValueOf(v).Float()) + f := reflect.ValueOf(v).Float() + if math.Trunc(f) != f { + return nil, fmt.Errorf("invalid numeric default for action %q arg %q: non-integer %v", actionKey, k, v) + } + intField.DefaultValue = int64(f) + case string: + if i, err := strconv.ParseInt(v, 10, 64); err == nil { + intField.DefaultValue = i + } else { + return nil, fmt.Errorf("invalid numeric default for action %q arg %q: %q", actionKey, k, v) + } default: - return nil, fmt.Errorf("invalid numeric default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid numeric default for action %q arg %q: %T", actionKey, k, defaultValue) } arg.Field = &config_sdk.Field_IntField{IntField: intField}As per coding guidelines.
🧹 Nitpick comments (6)
pkg/connector/action.go (6)
21-28: Add doc comment for exported identifier.ActionTypeMap is exported and should have a sentence doc comment per guidelines.
+// ActionTypeMap maps YAML action_type strings to their connector proto enum values. var ActionTypeMap = map[string]v2.ActionType{As per coding guidelines.
3-19: Re-group imports and add packages used in suggested fixes (math, sort).Follow stdlib / third‑party / project grouping; alphabetize within groups.
import ( - "context" - "fmt" - "reflect" - "strconv" - - config_sdk "github.com/conductorone/baton-sdk/pb/c1/config/v1" - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" - "github.com/conductorone/baton-sdk/pkg/actions" - "github.com/conductorone/baton-sdk/pkg/annotations" - "github.com/conductorone/baton-sdk/pkg/connectorbuilder" - "github.com/conductorone/baton-sql/pkg/bsql" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" - "go.uber.org/zap" - "google.golang.org/protobuf/types/known/anypb" - "google.golang.org/protobuf/types/known/structpb" + "context" + "fmt" + "math" + "reflect" + "sort" + "strconv" + + config_sdk "github.com/conductorone/baton-sdk/pb/c1/config/v1" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/actions" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/structpb" + + "github.com/conductorone/baton-sql/pkg/bsql" )As per coding guidelines.
60-66: Schema naming fix LGTM; iterate arguments in a stable order.Using the map key for Field.Name and cfg.Name for DisplayName is correct. Iterate deterministically to keep UI and tests stable.
- for k, argCfg := range actionCfg.Arguments { + // Iterate deterministically over argument keys. + keys := make([]string, 0, len(actionCfg.Arguments)) + for k := range actionCfg.Arguments { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + argCfg := actionCfg.Arguments[k] arg := &config_sdk.Field{ Name: k, DisplayName: argCfg.Name,
71-79: Include argument key in invalid string default error.Current error cites only the action. Include the argument key for clarity.
- return nil, fmt.Errorf("invalid string default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid string default for action %q arg %q: %T", actionKey, k, defaultValue)As per coding guidelines.
118-127: Optional: accept simple string map defaults and convert to Any.If YAML unmarshals to map[string]string or map[string]any, support conversion.
case "string_map": stringMapField := &config_sdk.StringMapField{} switch v := defaultValue.(type) { case nil: case map[string]*anypb.Any: stringMapField.DefaultValue = v + case map[string]string: + conv := make(map[string]*anypb.Any, len(v)) + for kk, vv := range v { + av, _ := anypb.New(&structpb.Value{Kind: &structpb.Value_StringValue{StringValue: vv}}) + conv[kk] = av + } + stringMapField.DefaultValue = conv + case map[string]any: + conv := make(map[string]*anypb.Any, len(v)) + for kk, vv := range v { + av, _ := anypb.New(structpb.NewStructValue(&structpb.Struct{Fields: map[string]*structpb.Value{"value": structpb.NewValue(vv)}})) + conv[kk] = av + } + stringMapField.DefaultValue = conv default: - return nil, fmt.Errorf("invalid string map default for %s: %T", actionKey, defaultValue) + return nil, fmt.Errorf("invalid string map default for action %q arg %q: %T", actionKey, k, defaultValue) } arg.Field = &config_sdk.Field_StringMapField{StringMapField: stringMapField}
163-164: Include action key in required-argument error.Adds context when multiple actions exist.
- return nil, nil, fmt.Errorf("argument %s is required", k) + return nil, nil, fmt.Errorf("argument %q is required for action %q", k, actionKey)As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/bsql/config.go(0 hunks)pkg/connector/action.go(5 hunks)
💤 Files with no reviewable changes (1)
- pkg/bsql/config.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/connector/action.go
🧬 Code graph analysis (1)
pkg/connector/action.go (2)
pkg/connector/connector.go (1)
Connector(18-23)pkg/bsql/config.go (2)
ActionConfig(412-422)Annotations(118-124)
🔇 Additional comments (3)
pkg/connector/action.go (3)
137-139: Handler registration change LGTM.Passing actionKey to the handler and capturing cfg avoids closure gotchas and aligns logs/errors to the key.
150-153: Handler signature/log updates LGTM.Using the action key in logs improves traceability.
160-188: Resolved: retrieval now uses argument key (k).Previous review flagged v.Name vs key; this change correctly indexes args by k across types.
| case "string": | ||
| argMap[k] = args.Fields[v.Name].GetStringValue() | ||
| argMap[k] = args.Fields[k].GetStringValue() | ||
| case "boolean": | ||
| argMap[k] = args.Fields[v.Name].GetBoolValue() | ||
| argMap[k] = args.Fields[k].GetBoolValue() | ||
| case "number": | ||
| argMap[k] = args.Fields[v.Name].GetNumberValue() | ||
| argMap[k] = args.Fields[k].GetNumberValue() | ||
| case "string_list": |
There was a problem hiding this comment.
Ensure “number” args are integers at runtime and keep types consistent.
GetNumberValue yields float64; schema uses IntField. Validate and cast to int64 to avoid mixed types and SQL/CEL mismatches.
- case "number":
- argMap[k] = args.Fields[k].GetNumberValue()
+ case "number":
+ n := args.Fields[k].GetNumberValue()
+ if math.Trunc(n) != n {
+ return nil, nil, fmt.Errorf("argument %q must be an integer for action %q: got %v", k, actionKey, n)
+ }
+ argMap[k] = int64(n)As per coding guidelines.
📝 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.
| case "string": | |
| argMap[k] = args.Fields[v.Name].GetStringValue() | |
| argMap[k] = args.Fields[k].GetStringValue() | |
| case "boolean": | |
| argMap[k] = args.Fields[v.Name].GetBoolValue() | |
| argMap[k] = args.Fields[k].GetBoolValue() | |
| case "number": | |
| argMap[k] = args.Fields[v.Name].GetNumberValue() | |
| argMap[k] = args.Fields[k].GetNumberValue() | |
| case "string_list": | |
| case "string": | |
| argMap[k] = args.Fields[k].GetStringValue() | |
| case "boolean": | |
| argMap[k] = args.Fields[k].GetBoolValue() | |
| case "number": | |
| n := args.Fields[k].GetNumberValue() | |
| if math.Trunc(n) != n { | |
| return nil, nil, fmt.Errorf("argument %q must be an integer for action %q: got %v", k, actionKey, n) | |
| } | |
| argMap[k] = int64(n) | |
| case "string_list": |
🤖 Prompt for AI Agents
In pkg/connector/action.go around lines 171 to 177, the "number" branch
currently takes GetNumberValue() (float64) which can produce mixed types against
an IntField schema; change it to validate the float is an integer at runtime
(e.g., compare to its truncated value or use modulus), fail/return an error if
not integral, then cast the value to int64 and store that int64 in argMap so
downstream SQL/CEL sees a consistent integer type.
1 participant
Description
Useful links:
Note
Use argument map keys as names and config
nameas display label; removedisplay_namefield; pass and logactionKeyin handler and read args by key.nameand setdisplayNamefromArgumentConfig.name; remove fallback logic.actionKeytohandleQueryAction, update logs to use it, and read argument values by key; improve error messages to reference keys.ArgumentConfig.display_namefrompkg/bsql/config.go.Written by Cursor Bugbot for commit b26b58a. This will update automatically on new commits. Configure here.
Summary by CodeRabbit