-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ai-promp-guard) add config option to prompt guard for scanning "system" and "assistant" messages #13183
Conversation
0c16706
to
1e9711c
Compare
c6206a9
to
8d05045
Compare
8d05045
to
596526c
Compare
@jschmid1 could you take another look on this? Thanks! |
596526c
to
065a6f5
Compare
If you add a test for backwards compat verification, we can move this forward. In the meanwhile, I'll request another reviewer @Kong/gateway-moderators |
065a6f5
to
a8475a1
Compare
@@ -26,16 +26,21 @@ local execute do | |||
-- @tparam table request The deserialized JSON body of the request | |||
-- @tparam table conf The plugin configuration | |||
-- @treturn[1] table The decorated request (same table, content updated) | |||
-- @treturn[2] nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shouldn't be removed, it breaks the docs format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's another treturn[2]
below, this is a typo i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each line is a (typed) value, the numbers indicate the return "options";
- a valid result, 1 value, being a table
- an error which returns 2 values:
- a nil value
- an error message
so 3 lines in total
see first example here: https://lunarmodules.github.io/ldoc/examples/multiple.lua.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, in other places however we use following patterns:
-- @treturn table something
-- @treturn string|nil error
should we align with that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with @treturn string|nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, that breaks it even more. The original was correct. Not sure where you found those other references, but they are also wrong most likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's PDK and other plugins uses this pattern, a took a snippet below:
kong/db/schema/topological_sort.lua:-- @treturn nil|string nil if the schemas were sorted, or a message if a cycle was found
kong/db/declarative/init.lua:-- @treturn nil|string error message, only if error happened
kong/db/declarative/init.lua:-- @treturn nil|table err_t, only if error happened
kong/db/declarative/init.lua:-- @treturn nil|string error message, only if error happened
kong/db/declarative/init.lua:-- @treturn nil|table err_t, only if error happened
kong/db/declarative/init.lua:-- @treturn nil|string error message if error
kong/db/declarative/init.lua:-- @treturn nil|table err_t if error
kong/tools/stream_api.lua:-- @treturn nil|string error
kong/tools/stream_api.lua:-- @treturn nil|string error an error string
kong/tools/stream_api.lua:-- @treturn nil|string error an error string, returned for protocol or socket I/O failures
kong/tools/stream_api.lua:-- @treturn nil|string error
kong/tools/mime_type.lua:-- @treturn nil|nil|nil Invalid mime-type
kong/workspaces/counters.lua:-- @treturn nil|string error
kong/openid-connect/claims.lua:-- @return status(nil|boolean), error(string)
kong/openid-connect/claims.lua:-- @return status(nil|boolean), error(string)
kong/openid-connect/claims.lua:-- @return status(nil|boolean), error(string)
kong/openid-connect/claims.lua:-- @return status(nil|boolean), error(string)
kong/pdk/client/tls.lua: -- @treturn nil|err Returns `nil` if successful, or an error message if it fails.
kong/pdk/client/tls.lua: -- @treturn nil|err Returns `nil` if successful, or an error message if it fails.
kong/pdk/client/tls.lua: -- @treturn nil|err Returns `nil` if successful, or an error message if it fails.
kong/pdk/request.lua: -- @treturn nil|string An error message.
kong/pdk/client.lua: -- @treturn nil|err `nil` if successful, or an error message if it fails.
kong/pdk/client.lua: -- @treturn nil|err `nil` if successful, or an error message if it fails.
Right now, only the newly added ai-prompt-guard and ai-azure-content-safety plugin uses the new pattern. I'm not sure how we have two different patterns and our docs are rendering fine with the PDK ldoc.
Anyway, I'm going to just revert this change. I do not use ldoc locally so I don't have clue how to verify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know who wrote that, but most of that is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you take another look at this PR? @Tieske
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
a8475a1
to
7f08ef9
Compare
is set to false, the first user request is selected instead of the last one
7f08ef9
to
409cf9c
Compare
409cf9c
to
4543567
Compare
Summary
This PR also fixes an issue when allow_all_conversation_history
is set to false, the first user request is selected instead of the last
one.
Unit tests are refactored to so that it's cleaner and has wider coverage.
Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
AG-58