fix: consider only last input item when prompt recording for responses API #146
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
pawbana
force-pushed
the
pb/responses-last-user-prompt-fix
branch
from
ce610be to
00bdd0d
Compare
intercept/responses/base.go
Outdated
| if i.promptWasRecorded { | ||
| return | ||
| } | ||
| i.promptWasRecorded = true |
There was a problem hiding this comment.
Should this be set prospectively?
lastUserPrompt could fail, the prompt could be nil, etc.
There was a problem hiding this comment.
+1, after recorder.RecordPromptUsage ?
There was a problem hiding this comment.
If I understood correctly only first inner loop iteration should log user prompt and this is the main change in this PR. If it fails then it fails and that is it.
Subsequent inner loop iterations should fail to extract user prompt since last input item should be function call result / not contain user prompt anyway.
I've mostly added this check to not spam logs about failing to record prompt on each inner loop iteration.
There was a problem hiding this comment.
Ok, if there is no option to recover from failed recording, SGTM.
There was a problem hiding this comment.
Semantically I think it's odd to name something "was recorded" before we even try make a record attempt.
I'm fine to leave it but you can understand why this might cause confusion.
I've mostly added this check to not spam logs about failing to record prompt on each inner loop iteration.
I'm not sure why that would ever occur since the last user prompt should only be recorded when the last prompt was a human prompt, not within an agentic loop.
There was a problem hiding this comment.
perhaps rename it to firstIter bool or similar
There was a problem hiding this comment.
My point is that this is probably unnecessary.
There was a problem hiding this comment.
Refactored so prompt is extracted before inner loop and recorded only once after the loop.
intercept/responses/base.go
Outdated
| if i.promptWasRecorded { | ||
| return | ||
| } | ||
| i.promptWasRecorded = true |
There was a problem hiding this comment.
+1, after recorder.RecordPromptUsage ?
intercept/responses/base.go
Outdated
| // content can be a string or array of objects: | ||
| // https://platform.openai.com/docs/api-reference/responses/create#responses_create-input-input_item_list-input_message-content | ||
| content := lastItem.Get(string(constant.ValueOf[constant.Content]())) | ||
|
|
||
| // non array case, should be string | ||
| if !content.IsArray() { | ||
| if content.Type == gjson.String { | ||
| return &content.Str, nil | ||
| } | ||
| return nil, fmt.Errorf("unexpected input type: %v", content.Type.String()) | ||
| } | ||
|
|
||
| var sb strings.Builder | ||
| promptExists := false | ||
| for _, c := range content.Array() { | ||
| // ignore inputs of not `input_text` type | ||
| if c.Get(string(constant.ValueOf[constant.Type]())).Str != string(constant.ValueOf[constant.InputText]()) { | ||
| continue | ||
| } | ||
|
|
||
| text := c.Get(string(constant.ValueOf[constant.Text]())) | ||
| if text.Type == gjson.String { | ||
| promptExists = true | ||
| sb.WriteString(text.Str + "\n") | ||
| } else { | ||
| i.logger.Warn(ctx, fmt.Sprintf("unexpected input array type: %v", text.Type)) | ||
| } | ||
| } | ||
|
|
||
| if !promptExists { | ||
| return nil, nil | ||
| } | ||
|
|
||
| prompt := strings.TrimSuffix(sb.String(), "\n") | ||
| return &prompt, nil |
There was a problem hiding this comment.
nit: perhaps it is worth moving this logic to a separate function and cover with a few basic unit tests? to prevent dev errors in the future
|
|
||
| // Request was likely not human-initiated. | ||
| return "", nil | ||
| if lastItem.Get("role").Str != string(constant.ValueOf[constant.User]()) { |
There was a problem hiding this comment.
nit: I would hide the expression like this behind a descriptive function
|
|
||
| // content can be a string or array of objects: | ||
| // https://platform.openai.com/docs/api-reference/responses/create#responses_create-input-input_item_list-input_message-content | ||
| content := lastItem.Get(string(constant.ValueOf[constant.Content]())) |
There was a problem hiding this comment.
maybe a helper function? I see .Get(string(constant.ValueOf[constant.Content]() is repeated a few times
intercept/responses/base.go
Outdated
| // Check string variant | ||
| if i.req.Input.OfString.Valid() { | ||
| return i.req.Input.OfString.Value, nil | ||
| return &i.req.Input.OfString.Value, nil |
There was a problem hiding this comment.
nil -> no prompt found, "" -> prompt that is empty string
There was a problem hiding this comment.
I see. Thanks for the explanation.
The current form is correct, but I would assume the same rules as "comma, ok" pattern, so prompt, ok, err. Most likely, personal preference thing.
There was a problem hiding this comment.
changed to this pattern with moving prompt recording outside of inner loop. Pointers where also causing issues in injected tools tests / when prepareRequestForAgenticLoop was called.
intercept/responses/base.go
Outdated
| id uuid.UUID | ||
| req *ResponsesNewParamsWrapper | ||
| reqPayload []byte | ||
| promptWasRecorded bool |
There was a problem hiding this comment.
nit: maybe move it to the bottom with a new-line, looks more like a flow-control property
…njectedTool test)
pawbana
force-pushed
the
pb/responses-last-user-prompt-fix
branch
from
a156a20 to
77f0145
Compare
3 participants
Changes
lastUserPromptbehavior in responses API interceptors to only check last input item for user input.Fixes: #145