Repository navigation
fix(ai-prompt): align prompt handling across protocols - #14139
shreemaan-abhishek wants to merge 2 commits into
Conversation
`make install` enumerates each plugin subdirectory, so apisix/plugins/ai-prompt was left out of the installed tree and the CI install check failed with "Only in apisix/plugins: ai-prompt".
|
@shreemaan-abhishek, could you confirm these two regressions on
|
| for _, block in ipairs(system_blocks(body.system)) do | ||
| core.table.insert(merged, block) | ||
| end | ||
| body.system = merged |
There was a problem hiding this comment.
This turns body.system into a block array, but get_messages (line 329) only reads system when it's a string, so the system prompt disappears from the normalized messages as soon as the decorator prepends one.
The decorator runs in rewrite, and ai-prompt-guard (match_all_roles) and ai-lakera-guard read get_messages in access. So a client sending a string system with denied content now gets through, and the decorated content isn't checked either. Before this PR both were checked. A client sending an array system could already skip this, so having get_messages reuse extract_system_content would fix both.
| for _, template in ipairs(conf.templates) do | ||
| if template.name == template_name then | ||
| return template.template | ||
| local function render_template(configured, input) |
There was a problem hiding this comment.
Same regression membphis raised above, just adding to the list: system, input and messages[].name are affected too, not only user and metadata. There's no error, the literal {{ x }} just goes to the LLM, so nobody will notice after an upgrade until they look at the model output.
|
|
||
| return body_tab | ||
| local output | ||
| ok, output = pcall(render, input) |
There was a problem hiding this comment.
render(input) uses the request body as the context directly, which drops the _ctx, _body, _escape_json, _escape_xml and _multipart helpers that body-transformer injected through the metatable.
An existing template like ip {{ _ctx.var.remote_addr }} or {* _escape_json(x) *} now returns 503 on every request with attempt to index global '_ctx' (a nil value). I reproduced that one with a small resty script. Neither the docs nor the PR description mention this as a breaking change.
|
|
||
| function _M.rewrite(conf, ctx) | ||
| local body_tab, err = get_request_body_table(conf.max_req_body_size) | ||
| local body_tab, status, err = body.read(conf.max_req_body_size) |
There was a problem hiding this comment.
fail_mode doesn't cover this path. body.read returns before protocol detection, so skip and warn never apply to read or parse failures.
With the default fail_mode=skip, a Consumer-bound decorator returns 400 for an empty-body GET or a non-JSON form post, while binding.lua and the docs say that kind of non-AI traffic should pass through. ai-prompt-guard already sends non-JSON bodies to binding.on_unsupported (line 119), so the two plugins disagree here too.
|
|
||
| _M.messages = { | ||
| type = "array", | ||
| minItems = 1, |
There was a problem hiding this comment.
Sharing minItems = 1 with the decorator's prepend and append tightens the schema for configs already in etcd. {"prepend": [], "append": [{...}]} is easy to get from generated config, and the append works fine today.
After an upgrade that route fails check_route, config_etcd logs failed to check item data and skips it, so its traffic gets a 404. That's a lot worse than the "ineffective config" the PR description calls it. I'd either keep tolerating empty arrays or give this a migration path.
| return 400, err | ||
| return status, err | ||
| end | ||
| local template_name = body_tab.template_name |
There was a problem hiding this comment.
Also pre-existing, but the PR adds a strict body helper and this still gets through: body.read doesn't check that the top level is an object and template_name isn't type checked. A POST body of 123 or true throws here and comes back as a 500 with an error log, and {"template_name": {}} throws at the string concat on line 140.
The decorator has the same problem on /v1/messages. {"messages": "x"} throws at the ipairs on line 392 of anthropic-messages.lua because matches only checks the body is a table, while the Bedrock path guards with type(body.messages) == "table". These should all be 4xx.
There was a problem hiding this comment.
Confirmed that these malformed-shape failures predate this PR. On the base branch, ai-prompt-template indexes non-object decoded JSON and concatenates a non-string template_name, while the Anthropic decorator path can iterate a non-table messages value. I documented the validation requirements and sanitized 4xx acceptance criteria in #14155. We will fix those separately; the newly introduced cache and rendering regressions remain in scope for this PR.
|
|
||
|
|
||
| function _M.read(max_size) | ||
| local body, err = core.request.get_json_request_body_table(max_size) |
There was a problem hiding this comment.
membphis already covered the size limit bypass on the cache-hit path. There's a second problem from the same cache: the type check.
Say two routes share a URI, one with vars on post_arg.model and a fallback carrying the prompt plugin. A JSON body sent as application/x-www-form-urlencoded (what curl -d does by default) gets parsed and cached as form while the first route's vars are evaluated. It then matches the fallback, and body.read returns 400 request body type mismatch, where the old get_body plus decode parsed the JSON fine.
| return nil, { message = "could not parse JSON request body: " .. (err or "invalid JSON") } | ||
|
|
||
| local function render_string(value, input) | ||
| local raw_template = string_gsub(value, "{{(.-)}}", "{*%1*}") |
There was a problem hiding this comment.
Rewriting {{ }} to {* *} with gsub breaks resty.template's escape and raw semantics. literal \{{ x }} end now outputs literal {* x *} end and {-raw-}{{ x }}{-raw-} outputs {* x *}, where both used to give a literal {{ x }}.
A Lua table constructor like {{a=1}} inside {% %} gets rewritten too and fails to compile. And because each string is compiled on its own now, \r\n in config gets normalized to \n by the Lua long string, which the old flow kept through JSON escaping. All of these reproduced with a resty script.
| ) | ||
| local rendered, render_err = render_template(template, body_tab) | ||
| if not rendered then | ||
| return 503, {message = render_err} |
There was a problem hiding this comment.
The old flow logged render failures with core.log.error in body-transformer (body-transformer.lua:218), and that's gone here, so the error only goes back to the client.
A template with {{ nested.name }} and a request without nested returns 503 with no gateway log, so ops see a 5xx with nothing to trace, when it's really a client error that should be a 4xx. I get not logging the rendered body, but the error text doesn't contain it, so this log line can stay. The response also exposes a chunk fragment like [string "context=... or {}..."]:5.
| location /t { | ||
| content_by_lua_block { | ||
| local t = require("lib.test_admin").test | ||
| local code, body = t('/apisix/admin/routes/23', |
There was a problem hiding this comment.
A few test issues. TEST 7 and TEST 8 here and TEST 5 in ai-prompt-template.t send a request right after PUTting a new route with no wait, and t() doesn't sleep, so slow config sync gives a 404 instead of the expected 400 or 413. TEST 4 here uses ngx.sleep(1) and TEST 4 in the template test uses ngx.sleep(0.2).
Route 20 (line 113) is never cleaned up and matches /v1/responses with the same priority as route 1 in TEST 16 and TEST 22. From reading the code TEST 17 only passes because route 1 loads first in etcd key order, I haven't run it.
Also the default fail_mode=skip and warn have no tests at all, only error does.
Description
The two prompt plugins used separate request-body and message schemas, which allowed malformed prompt items and caused protocol-specific fields to be handled as OpenAI Chat messages. This change introduces shared strict prompt/body helpers, renders templates structurally so quotes, newlines, and markup remain valid JSON, and decorates OpenAI Chat, OpenAI Responses, Anthropic Messages, and Bedrock Converse requests in their native fields.
Unsupported request formats can now be handled with fail_mode; oversized bodies return 413; rendered prompt bodies are no longer logged by template processing. Existing valid configurations remain compatible, while empty prompt arrays and non-object prompt entries are now rejected instead of being accepted as ineffective configuration.
Which issue(s) this PR fixes:
N/A
Validation
Checklist