Skip to content
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

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

fffonion
Copy link
Contributor

@fffonion fffonion commented Jun 7, 2024

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

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

AG-58

@github-actions github-actions bot added schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee plugins/ai-prompt-guard labels Jun 7, 2024
@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from 0c16706 to 1e9711c Compare June 7, 2024 07:24
@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch 3 times, most recently from c6206a9 to 8d05045 Compare June 24, 2024 07:55
@fffonion
Copy link
Contributor Author

fffonion commented Jul 5, 2024

@jschmid1 could you take another look on this? Thanks!

@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from 596526c to 065a6f5 Compare July 8, 2024 06:47
@jschmid1
Copy link
Contributor

jschmid1 commented Jul 9, 2024

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

@jschmid1 jschmid1 requested a review from a team July 9, 2024 11:39
@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from 065a6f5 to a8475a1 Compare July 10, 2024 06:51
@@ -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
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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";

  1. a valid result, 1 value, being a table
  2. 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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from a8475a1 to 7f08ef9 Compare July 11, 2024 11:23
is set to false, the first user request is selected instead of the last
one
@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from 7f08ef9 to 409cf9c Compare July 11, 2024 11:41
@fffonion fffonion force-pushed the prompt-gaurd-system-assistent branch from 409cf9c to 4543567 Compare July 15, 2024 06:55
@fffonion fffonion merged commit da28aea into master Jul 16, 2024
27 checks passed
@fffonion fffonion deleted the prompt-gaurd-system-assistent branch July 16, 2024 02:21
@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Jul 16, 2024
@kikito kikito removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Jul 16, 2024
@Kong Kong deleted a comment from team-gateway-bot Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants