feat(py): middleware plugin + samples#5411
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive middleware system for Genkit model calls, allowing users to inspect or modify requests and tool executions. It includes core abstractions, five concrete middleware implementations (Retry, Fallback, ToolApproval, Skills, and Filesystem), integration into the generation pipeline, and updates to reflection endpoints.
Feedback on the changes highlights three key areas for improvement:
- In
_skills.py, the frontmatter parser_parse_skill_fileshould be updated to support CRLF line endings to prevent parsing failures on Windows systems. - In
utils.py(Google GenAI plugin), the logic for peeling off the outbound envelope from tool responses should be made more robust by verifying that thenamekey matches the actual function response name to avoid corrupting legitimate tool outputs. - In
_reflection.py, changing the return type annotation ofvaluestoResponsemay cause aNameErrorifResponseis not imported; keeping it asJSONResponseis safer and more specific.
I am having trouble creating individual review comments. Click here to see my feedback.
py/plugins/middleware/src/genkit/plugins/middleware/_skills.py (76-92)
The _parse_skill_file method currently checks content.startswith('---\n') and searches for \n---. This will fail to parse the frontmatter on Windows systems where files are saved with CRLF (\r\n) line endings, causing the middleware to fall back to using the directory name and losing the description. We should update this to support both LF and CRLF line endings.
def _parse_skill_file(self, path: Path) -> tuple[str, str]:
try:
content = path.read_text(encoding='utf-8').lstrip('\\ufeff')
except Exception:
return '', ''
if content.startswith('---\\r\\n'):
start_idx = 5
end_marker = '\\r\\n---'
elif content.startswith('---\\n'):
start_idx = 4
end_marker = '\\n---'
else:
return '', ''
end_idx = content.find(end_marker, start_idx)
if end_idx == -1:
return '', ''
try:
data = yaml.safe_load(content[start_idx:end_idx])
if not isinstance(data, dict):
return '', ''
return data.get('name', ''), data.get('description', '')
except Exception:
return '', ''py/plugins/google-genai/src/genkit/plugins/google_genai/models/utils.py (324-330)
The current check isinstance(output, dict) and 'name' in output and 'content' in output is used to peel off the outbound envelope. However, if a tool legitimately returns a dictionary containing generic name and content keys, this check will incorrectly trigger and strip away the other keys, corrupting the tool output. We can make this check extremely robust by verifying that the name key in the dictionary matches the actual part.function_response.name.
if part.function_response:
# If the model echoes back the ``{name, content}`` envelope we
# used on the outbound side, peel it off so the caller sees the
# original tool output.
output = part.function_response.response
if isinstance(output, dict) and output.get('name') == part.function_response.name and 'content' in output:
output = output['content']py/packages/genkit/src/genkit/_core/_reflection.py (180)
The return type annotation was changed from JSONResponse to Response. If Response is not imported in this file, this will raise a NameError at import time. Since both return paths in this function return a JSONResponse, it is safer and more specific to keep the return type annotation as JSONResponse.
async def values(req: Request) -> JSONResponse:
Summary