Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the prompt building mechanism to allow for more flexible and structured instructions, introducing the ability to pass instruction lists to BasicCatalogItems.asCatalog and the PromptBuilder.chat constructor, leveraging PromptFragments for improved modularity and reusability. However, it introduces a high-severity security regression by omitting critical safety instructions (preventing surface overwrites) from the final prompt. Additionally, the refactoring of the PromptBuilder class is incomplete, leading to breaking changes in existing code and tests due to uncoordinated field renaming and incorrect function initialization logic.
| final fragments = <String>[ | ||
| ...instructions, | ||
| 'Use the provided tools to respond to the user using rich UI elements.', | ||
| ...catalog.instructions, | ||
| ''' | ||
| <a2ui_schema> | ||
| $a2uiSchema | ||
| </a2ui_schema> | ||
| ''', | ||
| BasicCatalogEmbed.basicCatalogRules, | ||
| ]; |
There was a problem hiding this comment.
The critical instruction requiring the LLM to use unique surfaceIds has been removed from the prompt construction. Although _SurfaceOperations.uniqueSurfaceId is defined on line 28, it is never included in the fragments list on line 69. This allows the LLM to overwrite existing surfaces, which can lead to unauthorized UI state manipulation.
| late final String | ||
| prompt = ({ChatMessage? userMessage, ChatMessage? context}) { |
There was a problem hiding this comment.
The renaming of systemPrompt to prompt on line 64 is a breaking change causing regressions in chat_session.dart and the test suite. Additionally, the prompt field is declared as late final String and initialized by an immediately invoked closure where parameters userMessage and context are unused. This is logically flawed; if the prompt is static, these parameters should be removed. If dynamic, prompt should be a function, e.g., late final String Function({ChatMessage? userMessage, ChatMessage? context}) prompt;.
| late final String | |
| prompt = ({ChatMessage? userMessage, ChatMessage? context}) { | |
| late final String | |
| prompt = () { |
| abstract class _SurfaceOperations { | ||
| static const String uniqueSurfaceId = | ||
| ''' | ||
| $_importancePrefix When you generate UI in a response, you MUST always create | ||
| a new surface with a unique `surfaceId`. Do NOT reuse or update | ||
| previously used `surfaceId`s. Each UI response must be in its own new surface. | ||
| '''; |
There was a problem hiding this comment.
The _SurfaceOperations abstract class and its uniqueSurfaceId constant are introduced but do not appear to be used anywhere in the provided code. Consider removing them if they are not intended for immediate use, or integrate them into the prompt generation if they are.
1 participant
No description provided.