Several Agent fixes#413
Conversation
satti-hari-krishna-reddy
requested review from
Monilprajapati,
Copilot and
frikky
and removed request for
Copilot
Monilprajapati
force-pushed
the
agent-workflow
branch
from
fd774a8 to
1130c3f
Compare
| item := "node" // default | ||
| opType := "" | ||
| switch operation.Op { | ||
| case "add_node": |
There was a problem hiding this comment.
got a feeling that we could have done this more smartly.
would have been so much more cleaner if case could simply match up with item + opType. Is that possible?
…shuffle-shared into agent-workflow
…shuffle-shared into agent-workflow
frikky
left a comment
frikky
left a comment
There was a problem hiding this comment.
I'm scared of how many potential nil-pointers you may have introduced.
4 lines doing 1 thing each >>>>> 1 line doing 4 things
|
|
||
| // So lets decode into raw maps so we can inspect and mutate it | ||
| var raw []map[string]interface{} | ||
| if err := json.NewDecoder(strings.NewReader(s[i:])).Decode(&raw); err != nil { |
There was a problem hiding this comment.
This kind of stuff is scary. Always split into multiple at a time if possible, and verify indexing.
| continue | ||
| } | ||
|
|
||
| if len(raw) == 0 || raw[0]["action"] == nil { |
There was a problem hiding this comment.
What happens if "action" doesn't exist in the first object?
| } | ||
|
|
||
| // Fix the types (convert numbers/bools to strings) | ||
| normalizeDecisionFields(raw) |
There was a problem hiding this comment.
If it's gonna normalize, doesn't it need to return &raw?
Or are pointer mappings enough?
| continue | ||
| } | ||
|
|
||
| r := strings.NewReader(s[i:]) |
There was a problem hiding this comment.
Why s[i:]? Just to read from current index until end? Feels weird without a moving window
| continue | ||
| } | ||
|
|
||
| if raw["action"] != nil { |
| } | ||
|
|
||
| if raw["action"] != nil { | ||
| normalizeDecisionFields([]map[string]interface{}{raw}) |
There was a problem hiding this comment.
Try to avoid this kind of pointer magic please. And pre-declare values.
This isn't easily understandable/readable
| } | ||
|
|
||
| for _, f := range fields { | ||
| fm, ok := f.(map[string]interface{}) |
There was a problem hiding this comment.
I'm scared when we do casting -> check values (:
| // This MUST happen before unmarshalling into AgentDecision (whose Value field is a string). | ||
| func normalizeDecisionFields(decisions []map[string]interface{}) { | ||
| for _, d := range decisions { | ||
| fields, ok := d["fields"].([]interface{}) |
There was a problem hiding this comment.
Don't cast AND check if it exists at the same time. Separate.
3 participants
This PR includes several different fixes for the agent: