Template-based Prompt Extension System #181
Conversation
- Add support for template-based prompts that can be extended - Implement variable substitution in templates using existing variable system - Allow templates to extend other templates (inheritance chain) - Add proper error handling for circular dependencies or missing templates - Maintain backward compatibility with existing prompts Closes #180
WalkthroughThis pull request introduces a new YAML configuration for issue prompt templates, updates JSON schema formatting for readability, and enhances the variable replacement mechanism by introducing a composite processor along with a new interface. It also extends prompt functionality by adding support for prompt extensions, template resolution, and a new enumeration for prompt types. Additionally, error handling improvements and minor adjustments in HTTP tool request processing and null checking have been applied, along with the removal of an obsolete unit test. Several new feature test classes and YAML fixtures have been added to verify prompt, tool, and file source configurations and behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as PromptParserPlugin
participant Registry as PromptRegistry
participant Resolver as TemplateResolver
participant Logger as LoggerInterface
Plugin->>Registry: Retrieve registered prompts
loop For each prompt with extensions
Plugin->>Resolver: resolve(prompt)
Resolver->>Resolver: mergeMessages() & substituteMessages()
Resolver-->>Plugin: return resolved prompt
Plugin->>Registry: register(resolved prompt)
end
alt Resolution Error Occurs
Resolver-->>Plugin: throw TemplateResolutionException
Plugin->>Logger: log warning/error
Plugin-->>Registry: throw PromptParsingException
end
sequenceDiagram
participant Handler as HttpToolHandler
participant Resolver as VariableResolver
participant Composite as CompositeProcessor
Handler->>Resolver: with(ToolArgumentsProvider)
Resolver->>Composite: Process variable replacements (iterate processors)
Composite-->>Resolver: Return resolved string
Resolver-->>Handler: Return updated URL/headers/body
Possibly related issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)src/Config/Import/ImportRegistry.php (2)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/McpServer/Prompt/PromptType.php (1)
18-28: Well-implemented enum with smart fallback designThe
fromStringmethod provides a clean way to map string values to enum cases with appropriate default handling. The case-insensitive comparison is good practice.Consider logging or warning when an invalid type string is encountered, as silently defaulting to
Promptcould potentially hide configuration errors. This could help with debugging when users mistype template types.src/Lib/Variable/CompositeProcessor.php (1)
10-11: Consider using the interface type in PHPDoc.While functionally correct, the PHPDoc specifies
VariableReplacementProcessor[]but should useVariableReplacementProcessorInterface[]to follow the Liskov Substitution Principle and allow any implementation of the interface./** - * @param VariableReplacementProcessor[] $processors + * @param VariableReplacementProcessorInterface[] $processors */src/McpServer/Tool/Types/HttpToolHandler.php (1)
145-146: Incorrect comment in query parameters section.The comment is duplicated from above, but this section is processing body parameters, not query parameters.
- // Process query parameters + // Process body parameters if (isset($requestConfig['body']) && \is_array($requestConfig['body'])) {docs/example/config/prompt-templates.yml (1)
59-76: Consider adding a 'priority' property to the schema.You reference
{{priority}}in the final content but do not define it in the schema. To help users understand and populate the “priority” attribute, consider adding a corresponding schema property, for example:schema: properties: priority: description: The priority level of the issue required: - priority
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/example/config/prompt-templates.yml(1 hunks)json-schema.json(7 hunks)src/Application/Bootloader/VariableBootloader.php(2 hunks)src/Console/GenerateCommand.php(1 hunks)src/Lib/Variable/CompositeProcessor.php(1 hunks)src/Lib/Variable/VariableReplacementProcessor.php(1 hunks)src/Lib/Variable/VariableReplacementProcessorInterface.php(1 hunks)src/Lib/Variable/VariableResolver.php(1 hunks)src/McpServer/Prompt/Exception/PromptParsingException.php(1 hunks)src/McpServer/Prompt/Exception/TemplateResolutionException.php(1 hunks)src/McpServer/Prompt/Extension/PromptDefinition.php(3 hunks)src/McpServer/Prompt/Extension/PromptExtension.php(1 hunks)src/McpServer/Prompt/Extension/PromptExtensionArgument.php(1 hunks)src/McpServer/Prompt/Extension/PromptExtensionVariableProvider.php(1 hunks)src/McpServer/Prompt/Extension/TemplateResolver.php(1 hunks)src/McpServer/Prompt/PromptConfigFactory.php(3 hunks)src/McpServer/Prompt/PromptParserPlugin.php(3 hunks)src/McpServer/Prompt/PromptProviderInterface.php(2 hunks)src/McpServer/Prompt/PromptRegistry.php(3 hunks)src/McpServer/Prompt/PromptRegistryInterface.php(1 hunks)src/McpServer/Prompt/PromptType.php(1 hunks)src/McpServer/Tool/Types/HttpToolHandler.php(4 hunks)tests/src/Unit/Lib/Variable/VariableResolverTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/src/Unit/Lib/Variable/VariableResolverTest.php
🧰 Additional context used
🧬 Code Graph Analysis (14)
src/Lib/Variable/VariableReplacementProcessorInterface.php (2)
src/Lib/Variable/CompositeProcessor.php (1)
process(16-23)src/Lib/Variable/VariableReplacementProcessor.php (1)
process(29-44)
src/McpServer/Prompt/Exception/TemplateResolutionException.php (1)
src/McpServer/Prompt/Exception/PromptParsingException.php (1)
PromptParsingException(10-13)
src/Lib/Variable/CompositeProcessor.php (3)
src/Lib/Variable/VariableResolver.php (1)
__construct(12-14)src/Lib/Variable/VariableReplacementProcessor.php (2)
__construct(17-21)process(29-44)src/Lib/Variable/VariableReplacementProcessorInterface.php (1)
process(15-15)
src/Console/GenerateCommand.php (2)
src/Config/Registry/ConfigRegistryAccessor.php (1)
getImports(48-51)src/Console/Renderer/GenerateCommandRenderer.php (1)
renderImports(42-68)
src/McpServer/Tool/Types/HttpToolHandler.php (2)
src/Lib/Variable/VariableResolver.php (3)
VariableResolver(10-39)with(16-22)resolve(27-38)src/Lib/Variable/VariableReplacementProcessor.php (1)
VariableReplacementProcessor(15-70)
src/McpServer/Prompt/PromptRegistryInterface.php (3)
src/McpServer/Prompt/Extension/PromptDefinition.php (1)
PromptDefinition(11-70)src/McpServer/Prompt/PromptRegistry.php (2)
has(44-47)get(30-42)src/McpServer/Prompt/PromptProviderInterface.php (2)
has(16-16)get(24-24)
src/McpServer/Prompt/Extension/PromptExtension.php (2)
src/McpServer/Prompt/Extension/PromptExtensionArgument.php (2)
__construct(12-15)PromptExtensionArgument(10-16)src/McpServer/Prompt/Extension/PromptDefinition.php (1)
__construct(13-21)
src/McpServer/Prompt/PromptParserPlugin.php (4)
src/McpServer/Prompt/Extension/TemplateResolver.php (3)
LoggerPrefix(20-151)__construct(23-27)resolve(36-95)src/McpServer/Prompt/Exception/TemplateResolutionException.php (1)
TemplateResolutionException(10-13)src/McpServer/Prompt/PromptConfigFactory.php (1)
createFromConfig(27-65)src/McpServer/Prompt/PromptRegistryInterface.php (1)
register(14-14)
src/McpServer/Prompt/PromptConfigFactory.php (4)
src/McpServer/Prompt/Extension/PromptDefinition.php (1)
PromptDefinition(11-70)src/McpServer/Prompt/Extension/PromptExtension.php (2)
PromptExtension(10-49)fromArray(28-48)src/McpServer/Prompt/PromptType.php (1)
fromString(18-28)src/McpServer/Prompt/Exception/PromptParsingException.php (1)
PromptParsingException(10-13)
src/McpServer/Prompt/PromptRegistry.php (2)
src/McpServer/Prompt/Extension/PromptDefinition.php (2)
PromptDefinition(11-70)jsonSerialize(23-48)src/McpServer/Prompt/PromptRegistryInterface.php (2)
get(29-29)has(21-21)
src/Application/Bootloader/VariableBootloader.php (3)
src/Lib/Variable/CompositeProcessor.php (1)
CompositeProcessor(7-24)src/Lib/Variable/Provider/CompositeVariableProvider.php (1)
CompositeVariableProvider(10-75)src/Lib/Variable/VariableReplacementProcessor.php (1)
VariableReplacementProcessor(15-70)
src/McpServer/Prompt/Extension/PromptDefinition.php (1)
src/McpServer/Prompt/Extension/PromptExtension.php (1)
PromptExtension(10-49)
src/McpServer/Prompt/Extension/TemplateResolver.php (8)
src/Lib/Variable/VariableReplacementProcessor.php (1)
VariableReplacementProcessor(15-70)src/Lib/Variable/VariableResolver.php (4)
VariableResolver(10-39)__construct(12-14)resolve(27-38)with(16-22)src/McpServer/Prompt/Exception/TemplateResolutionException.php (1)
TemplateResolutionException(10-13)src/McpServer/Prompt/Extension/PromptExtensionArgument.php (1)
__construct(12-15)src/McpServer/Prompt/Extension/PromptExtension.php (1)
__construct(16-19)src/McpServer/Prompt/Extension/PromptExtensionVariableProvider.php (3)
__construct(22-25)get(32-35)PromptExtensionVariableProvider(12-53)src/McpServer/Prompt/PromptProviderInterface.php (1)
get(24-24)src/McpServer/Prompt/PromptRegistryInterface.php (1)
get(29-29)
src/Lib/Variable/VariableResolver.php (1)
src/Lib/Variable/CompositeProcessor.php (1)
CompositeProcessor(7-24)
🪛 GitHub Check: psalm (ubuntu-latest, 8.3, locked)
src/McpServer/Prompt/PromptRegistry.php
[failure] 44-44: ParamNameMismatch
src/McpServer/Prompt/PromptRegistry.php:44:32: ParamNameMismatch: Argument 1 of Butschster\ContextGenerator\McpServer\Prompt\PromptRegistry::has has wrong name $id, expecting $name as defined by Butschster\ContextGenerator\McpServer\Prompt\PromptProviderInterface::has (see https://psalm.dev/230)
[failure] 30-30: ParamNameMismatch
src/McpServer/Prompt/PromptRegistry.php:30:32: ParamNameMismatch: Argument 1 of Butschster\ContextGenerator\McpServer\Prompt\PromptRegistry::get has wrong name $id, expecting $name as defined by Butschster\ContextGenerator\McpServer\Prompt\PromptProviderInterface::get (see https://psalm.dev/230)
src/McpServer/Prompt/Extension/TemplateResolver.php
[failure] 134-134: UndefinedMagicPropertyFetch
src/McpServer/Prompt/Extension/TemplateResolver.php:134:24: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\ImageContent::$text is not defined (see https://psalm.dev/218)
[failure] 134-134: UndefinedMagicPropertyFetch
src/McpServer/Prompt/Extension/TemplateResolver.php:134:24: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\EmbeddedResource::$text is not defined (see https://psalm.dev/218)
🪛 GitHub Actions: 🔍 Static analysis
src/McpServer/Prompt/Extension/TemplateResolver.php
[error] 134-134: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\EmbeddedResource::$text is not defined (see https://psalm.dev/218)
🔇 Additional comments (54)
src/McpServer/Prompt/Exception/PromptParsingException.php (1)
10-10: Good architectural change to enable exception inheritanceRemoving the
finalkeyword is appropriate here since this class is now extended by the newTemplateResolutionException. This change enables a proper exception hierarchy for the template system while maintaining backward compatibility.src/Lib/Variable/CompositeProcessor.php (2)
7-14: Class declaration and constructor look good.The
CompositeProcessorclass implementation follows the composite design pattern, allowing for sequencing multiple processors. Thefinal readonlymodifier ensures immutability, which is a good practice for this type of utility class.
16-23: Implementation of process method is clean and effective.The method correctly iterates through each processor and applies them sequentially, passing the transformed text to each subsequent processor in the chain.
src/McpServer/Prompt/PromptRegistryInterface.php (4)
7-8: Import added correctly for PromptDefinition.The addition of this import supports the new method signatures that return the PromptDefinition type.
11-14: Updated documentation for register method.Documentation has been simplified while maintaining clarity.
16-22: Well-documented has method added.The new
hasmethod provides a clean way to check for prompt existence by ID with clear parameter documentation.
23-29: Well-documented get method added.The new
getmethod provides a standard way to retrieve prompt definitions with appropriate exception documentation when the prompt doesn't exist.src/Application/Bootloader/VariableBootloader.php (2)
9-16: Imports added for new composite processor pattern.The added imports correctly set up the dependencies needed for the new singleton definition.
56-61: Singleton definition for variable replacement processor interface.Good implementation of the composite pattern in the DI container. This allows for extending the variable replacement functionality with multiple processors while maintaining a clean interface.
src/McpServer/Tool/Types/HttpToolHandler.php (6)
12-12: Import added for VariableResolver.Correctly imports the VariableResolver class that replaces direct use of the VariableReplacementProcessor.
22-28: Constructor updated with VariableResolver dependency.The constructor now injects the VariableResolver, which is a good practice for dependency injection.
117-119: Improved variable replacement with composite pattern.Using the
withmethod on the VariableResolver creates a new composite processor that chains the existing processor with the new one for arguments, which is more flexible than directly creating a processor.
121-123: URL variable resolution using the resolver.The code now delegates to the VariableResolver for processing, which is cleaner and more consistent.
127-129: Headers variable resolution using the resolver.Headers are now processed consistently using the VariableResolver.
140-142: Variable resolution for query and body parameters.The code now uses the resolver to process variables in both query and body parameters.
Also applies to: 147-149
src/McpServer/Prompt/Extension/PromptExtension.php (1)
1-49: LGTM! Well-structured implementation of prompt extension mechanism.The
PromptExtensionclass is well-designed with appropriate immutability (final readonly), clear responsibility (representing template extension configuration), and robust validation in the fromArray method. The error messages are descriptive and will help users understand configuration issues.src/Lib/Variable/VariableResolver.php (2)
13-13: Good refactoring to support interface-based dependency injection.Changing the constructor parameter from concrete
VariableReplacementProcessortoVariableReplacementProcessorInterfacewith a defaultCompositeProcessorimproves flexibility while maintaining backward compatibility.
16-22: Well-implemented composition pattern for immutable objects.The new
with()method follows best practices for immutable objects by returning a new instance with combined processors rather than modifying the existing one. This approach maintains the object's integrity while allowing for flexible composition.src/McpServer/Prompt/PromptConfigFactory.php (4)
8-9: LGTM! Added necessary imports for the new extension functionality.
45-53: Good implementation of prompt type and extension parsing.The code correctly determines the prompt type and parses extensions from the configuration in a clean, logical manner.
62-64: LGTM! Updated constructor parameters properly.Constructor call updated to include the new type and extensions parameters.
67-103: Well-structured extension parsing with proper error handling.The
parseExtensionsmethod is well-implemented with:
- Clear validation of extension configurations
- Proper exception handling with contextual error messages
- Good use of the existing
PromptExtension::fromArraymethod- Appropriate conversion of generic exceptions to domain-specific ones with context
This implementation will provide clear, helpful error messages when extension configurations are invalid.
src/McpServer/Prompt/Extension/PromptDefinition.php (4)
5-7: LGTM! Namespace and import changes align with the new structure.Moving the class to the Extension namespace and adding the PromptType import makes sense given the new functionality.
18-20: Good extension of constructor parameters with appropriate defaults.Adding the prompt type and extensions parameters with sensible defaults ensures backward compatibility while enabling the new template functionality.
42-46: LGTM! Updated JSON serialization to include new properties.The serialization now includes the prompt type and extensions, which is necessary for the template-based prompt system.
50-69: Well-implemented extension serialization method.The
serializeExtensionsmethod:
- Handles empty arrays appropriately
- Correctly transforms extension objects to the configuration format
- Preserves the structure needed for recreation via the
fromArraymethodThis maintains a consistent format between the configuration and serialized output.
docs/example/config/prompt-templates.yml (1)
1-9: Looks great for the base template.The base
template-issuestructure appears correct, and the variable placeholders{{title}}and{{description}}are clearly integrated.src/McpServer/Prompt/PromptParserPlugin.php (2)
7-23: Constructor injection and logging prefix look fine.The addition of
TemplateResolverto the constructor and the usage of#[LoggerPrefix(prefix: 'prompt.parser')]attribute provide clarity and maintain consistency with the new template resolution approach.
69-92: Template resolution second pass is well-structured.Resolving template extensions after the initial registry pass is a clean approach to ensure all templates are registered first. This mechanism avoids partial or circular references and aligns well with the new
TemplateResolver.src/McpServer/Prompt/PromptRegistry.php (3)
54-60: Efficient template retrieval.
allTemplates()correctly filters prompts to return only those ofPromptType::Template. This aligns well with the new approach of separating regular prompts from templates.
70-83: Selective retrieval of regular prompts.Filtering out template-type prompts in
getItems()helps maintain consistent serialization and operations where only non-template prompts are needed.
87-89: Excluding templates from JSON serialization is appropriate.Only serializing the actual prompts rather than template definitions keeps the API and configuration output concise, preventing unnecessary clutter.
src/McpServer/Prompt/Extension/PromptExtensionVariableProvider.php (4)
12-25: All variables from extension arguments are properly stored.This constructor fully aligns with the typed property
$variablesdeclared above, ensuring type safety and clarity. No issues found.
27-31:hasmethod is straightforward and efficient.Using
array_key_existsis appropriate here, and returning a boolean aligns with typical usage.
32-35:getmethod is concise and handles the fallback correctly.Returning null when the variable is absent is a clear and safe approach.
37-52: Private methodcreateVariablesFromArgumentsis readable and safe.Populating
$variablesusing each extension argument’snameandvalueis sound. No recommendations at this time.src/McpServer/Prompt/Extension/TemplateResolver.php (2)
36-95: Extension resolution logic is coherent and respects circular dependency checks.This section effectively prevents infinite recursion and merges template messages if present. No issues flagged here.
97-115:mergeMessagesmethod covers different scenarios cleanly.The distinction between using template messages when the prompt has none, and retaining prompt messages otherwise, seems intentional and correct.
json-schema.json (10)
272-278: Tool type enumeration is clearly defined.Specifying
"run"and"http"for"type"clarifies the permitted tool types and sets"run"as a default. This is well-structured.
315-317: Conditional requirement ofcommandsorrequests.The schema now mandates
commandsiftypeis"run", andrequestsiftypeis"http", ensuring proper validation for each type of tool.Also applies to: 329-331
338-340: New required propertyurland extended HTTP methods.Marking
urlas required gives clarity for request generation. Enumerating"GET","POST","PUT","DELETE","PATCH","HEAD", and"OPTIONS"covers most common methods.Also applies to: 348-356
442-444:idis now a required property for prompt definitions.This ensures every prompt has a unique identifier. Good addition for referencing prompts.
454-462: Addedtypeproperty for prompt specification.The schema now supports
"prompt"or"template", aligning with the new template-based architecture.
488-488: Clarified placeholder usage in message content.Referring to both
${variableName}and{{variableName}}placeholders. Matches the variable substitution logic in the code.
492-514: Newextendproperty for template inheritance.Allowing an array of templates with arguments fosters modular and consistent prompts. The schema definition is aligned with the new extension approach.
526-538: Conditional requirement formessagesorextendwhentypeis"prompt".This ensures a prompt either directly defines messages or inherits them from one or more templates.
544-545:templaterequiresmessages.When
typeis"template", requiring messages maintains consistency by ensuring a template has base content to be inherited.Also applies to: 548-552
1346-1348: RefinedmaxFilestype definition incomposerSource.Restricting it to integer reflects its usage and clarifies expectation. No issues found.
src/McpServer/Prompt/Exception/TemplateResolutionException.php (1)
7-13: Properly structured exception handling for template resolution.The implementation follows good exception handling practices by creating a specialized exception type for template resolution failures. The class extends
PromptParsingExceptionappropriately, aligning with the hierarchical error model.src/McpServer/Prompt/PromptProviderInterface.php (1)
7-8: LGTM - Appropriate import for type hinting.Adding the import for
PromptDefinitionensures proper type hinting for the interface methods.src/Console/GenerateCommand.php (1)
96-99: Improved null safety when handling imports.This change properly checks if
$importsis null before attempting to render them, preventing potential null reference exceptions. ThegetImports()method can return null according to its signature inConfigRegistryAccessor.src/Lib/Variable/VariableReplacementProcessor.php (1)
15-15: Good application of the Interface Segregation Principle.Implementing the
VariableReplacementProcessorInterfaceimproves code modularity and allows for better dependency injection and composition. This change supports the new template-based prompt system by enabling more flexible variable replacement strategies.src/Lib/Variable/VariableReplacementProcessorInterface.php (1)
7-16: Well-defined interface with clear purpose and good documentation.The interface establishes a clean contract for variable replacement processors with appropriate PHPDoc comments. This abstraction will allow for different implementations (like the new CompositeProcessor mentioned in the summary) and makes the system more flexible and extensible.
src/McpServer/Prompt/Extension/PromptExtensionArgument.php (1)
7-16: Clean implementation with good use of PHP 8 features.This class is well-designed for representing template arguments with:
- Appropriate use of
final readonlyfor immutability- Constructor property promotion for concise code
- Clear naming and documentation explaining its purpose within the template system
The class fits well with the PR's objectives of implementing a template-based prompt extension system.
| /** | ||
| * Applies variable substitution to template messages. | ||
| * | ||
| * @param PromptMessage[] $messages The messages to process | ||
| * @param PromptExtensionArgument[] $arguments The variables to substitute | ||
| * @return PromptMessage[] The processed messages | ||
| */ | ||
| private function substituteMessages(array $messages, array $arguments): array | ||
| { | ||
| $result = []; | ||
|
|
||
| // Create a variable provider for the extension arguments | ||
| $variableProvider = new PromptExtensionVariableProvider($arguments); | ||
|
|
||
| // Create a resolver with the variable provider | ||
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); | ||
|
|
||
| foreach ($messages as $message) { | ||
| $content = $message->content->text; | ||
| $substitutedContent = $resolver->resolve($content); | ||
|
|
||
| $this->logger?->debug('Template message processed', [ | ||
| 'original' => $content, | ||
| 'resolved' => $substitutedContent, | ||
| ]); | ||
|
|
||
| // Create a new message with the substituted content | ||
| $result[] = new PromptMessage( | ||
| role: $message->role, | ||
| content: new TextContent(text: $substitutedContent), | ||
| ); | ||
| } | ||
|
|
||
| return $result; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential undefined property access in $message->content->text.
Static analysis reports [UndefinedMagicPropertyFetch] on line 134, indicating that $message->content may not always have a text property if it’s not a TextContent. This could cause a runtime error.
Consider adding a type check, for example:
- $content = $message->content->text;
+ if (!$message->content instanceof TextContent) {
+ $this->logger?->warning('Expected text-based content, got different type', [
+ 'content_class' => get_class($message->content),
+ ]);
+ continue;
+ }
+ $content = $message->content->text;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Applies variable substitution to template messages. | |
| * | |
| * @param PromptMessage[] $messages The messages to process | |
| * @param PromptExtensionArgument[] $arguments The variables to substitute | |
| * @return PromptMessage[] The processed messages | |
| */ | |
| private function substituteMessages(array $messages, array $arguments): array | |
| { | |
| $result = []; | |
| // Create a variable provider for the extension arguments | |
| $variableProvider = new PromptExtensionVariableProvider($arguments); | |
| // Create a resolver with the variable provider | |
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); | |
| foreach ($messages as $message) { | |
| $content = $message->content->text; | |
| $substitutedContent = $resolver->resolve($content); | |
| $this->logger?->debug('Template message processed', [ | |
| 'original' => $content, | |
| 'resolved' => $substitutedContent, | |
| ]); | |
| // Create a new message with the substituted content | |
| $result[] = new PromptMessage( | |
| role: $message->role, | |
| content: new TextContent(text: $substitutedContent), | |
| ); | |
| } | |
| return $result; | |
| } | |
| } | |
| /** | |
| * Applies variable substitution to template messages. | |
| * | |
| * @param PromptMessage[] $messages The messages to process | |
| * @param PromptExtensionArgument[] $arguments The variables to substitute | |
| * @return PromptMessage[] The processed messages | |
| */ | |
| private function substituteMessages(array $messages, array $arguments): array | |
| { | |
| $result = []; | |
| // Create a variable provider for the extension arguments | |
| $variableProvider = new PromptExtensionVariableProvider($arguments); | |
| // Create a resolver with the variable provider | |
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); | |
| foreach ($messages as $message) { | |
| + if (!$message->content instanceof TextContent) { | |
| + $this->logger?->warning('Expected text-based content, got different type', [ | |
| + 'content_class' => get_class($message->content), | |
| + ]); | |
| + continue; | |
| + } | |
| - $content = $message->content->text; | |
| + $content = $message->content->text; | |
| $substitutedContent = $resolver->resolve($content); | |
| $this->logger?->debug('Template message processed', [ | |
| 'original' => $content, | |
| 'resolved' => $substitutedContent, | |
| ]); | |
| // Create a new message with the substituted content | |
| $result[] = new PromptMessage( | |
| role: $message->role, | |
| content: new TextContent(text: $substitutedContent), | |
| ); | |
| } | |
| return $result; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: psalm (ubuntu-latest, 8.3, locked)
[failure] 134-134: UndefinedMagicPropertyFetch
src/McpServer/Prompt/Extension/TemplateResolver.php:134:24: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\ImageContent::$text is not defined (see https://psalm.dev/218)
[failure] 134-134: UndefinedMagicPropertyFetch
src/McpServer/Prompt/Extension/TemplateResolver.php:134:24: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\EmbeddedResource::$text is not defined (see https://psalm.dev/218)
🪛 GitHub Actions: 🔍 Static analysis
[error] 134-134: UndefinedMagicPropertyFetch: Magic instance property Mcp\Types\EmbeddedResource::$text is not defined (see https://psalm.dev/218)
| /** | ||
| * Gets all non-template prompts. | ||
| * | ||
| * @return array<string, PromptDefinition> | ||
| */ | ||
| public function allTemplates(): array; |
There was a problem hiding this comment.
Documentation and method name appear to be inconsistent.
The method name allTemplates() suggests it returns all templates, but the documentation comment states it returns "all non-template prompts". This discrepancy could cause confusion for developers implementing this interface.
- /**
- * Gets all non-template prompts.
- *
- * @return array<string, PromptDefinition>
- */
- public function allTemplates(): array;
+ /**
+ * Gets all template prompts.
+ *
+ * @return array<string, PromptDefinition>
+ */
+ public function allTemplates(): array;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Gets all non-template prompts. | |
| * | |
| * @return array<string, PromptDefinition> | |
| */ | |
| public function allTemplates(): array; | |
| /** | |
| * Gets all template prompts. | |
| * | |
| * @return array<string, PromptDefinition> | |
| */ | |
| public function allTemplates(): array; |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/McpServer/Prompt/Extension/TemplateResolver.php (1)
134-136:⚠️ Potential issueReplace assertion with runtime type check for production safety.
The current assertion
\assert($message->content instanceof TextContent)only checks the type during development and won't provide protection in production when assertions are disabled. This issue was previously flagged in static analysis.Replace with a proper runtime type check:
- \assert($message->content instanceof TextContent); - $content = $message->content->text; + if (!$message->content instanceof TextContent) { + $this->logger?->warning('Expected text-based content, got different type', [ + 'content_class' => get_class($message->content), + ]); + continue; + } + $content = $message->content->text;
🧹 Nitpick comments (4)
src/McpServer/Prompt/Extension/TemplateResolver.php (4)
105-114: Consider preserving template messages structure with prompt content.Currently, if the prompt has any messages, template messages are completely ignored. This may not be the intended behavior in all cases, especially when templates provide structure that should be preserved.
Consider a more sophisticated merging strategy that might combine messages from both sources or allow configuration of how merging should work:
private function mergeMessages(array $promptMessages, array $templateMessages, array $arguments): array { // If the prompt has no messages, use the template's messages with substitution if (empty($promptMessages)) { return $this->substituteMessages($templateMessages, $arguments); } - // Otherwise, keep the prompt's messages (extensions just provide structure) - return $promptMessages; + // Depending on your use case, you might want to consider different merging strategies: + + // Option 1: Apply variable substitution to prompt messages + return $this->substituteMessages($promptMessages, $arguments); + + // Option 2: Append template messages after prompt messages + // return array_merge($promptMessages, $this->substituteMessages($templateMessages, $arguments)); + + // Option 3: Keep current behavior + // return $promptMessages; }
59-65: Enhance exception message with prompt ID information.When a template is not found, it would be helpful to include the ID of the prompt that was attempting to extend it to assist with debugging.
try { $template = $this->promptProvider->get($extension->templateId); } catch (\InvalidArgumentException $e) { throw new TemplateResolutionException( - \sprintf('Template "%s" not found', $extension->templateId), + \sprintf('Template "%s" not found while resolving prompt "%s"', $extension->templateId, $prompt->id), previous: $e, ); }
67-72: Add logging when a non-template prompt is referenced as a template.When a prompt is incorrectly referenced as a template, it would be helpful to log additional information for debugging purposes.
// Ensure it's a template if ($template->type !== PromptType::Template) { + $this->logger?->warning('Prompt referenced as template has incorrect type', [ + 'template_id' => $extension->templateId, + 'actual_type' => $template->type->name, + ]); throw new TemplateResolutionException( \sprintf('Prompt "%s" is not a template', $extension->templateId), ); }
47-53: Implement more descriptive circular dependency detection.The current circular dependency detection only tells you that a circular dependency was detected, but not what the dependency chain looks like.
Include the dependency chain in the error message for better debugging:
foreach ($prompt->extensions as $extension) { // Prevent circular dependencies if (\in_array($extension->templateId, $processedExtensions, true)) { + $dependencyChain = implode(' -> ', array_merge($processedExtensions, [$extension->templateId])); throw new TemplateResolutionException( - \sprintf('Circular dependency detected for template "%s"', $extension->templateId), + \sprintf('Circular dependency detected: %s', $dependencyChain), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/McpServer/Prompt/Extension/TemplateResolver.php(1 hunks)src/McpServer/Prompt/PromptRegistry.php(3 hunks)src/McpServer/Prompt/PromptRegistryInterface.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/McpServer/Prompt/PromptRegistryInterface.php
- src/McpServer/Prompt/PromptRegistry.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/McpServer/Prompt/Extension/TemplateResolver.php (7)
src/Lib/Variable/VariableReplacementProcessor.php (1)
VariableReplacementProcessor(15-70)src/Lib/Variable/VariableResolver.php (4)
VariableResolver(10-39)__construct(12-14)resolve(27-38)with(16-22)src/McpServer/Prompt/Extension/PromptExtension.php (1)
__construct(16-19)src/McpServer/Prompt/Extension/PromptExtensionArgument.php (1)
__construct(12-15)src/McpServer/Prompt/Extension/PromptDefinition.php (2)
__construct(13-21)PromptDefinition(11-70)src/McpServer/Prompt/Extension/PromptExtensionVariableProvider.php (3)
__construct(22-25)get(32-35)PromptExtensionVariableProvider(12-53)src/McpServer/Prompt/PromptProviderInterface.php (1)
get(24-24)
🔇 Additional comments (1)
src/McpServer/Prompt/Extension/TemplateResolver.php (1)
36-95: Overall solid implementation of the template resolution mechanism.The template resolver is well-structured with clear separation of concerns between the main resolution logic and the message manipulation functions. The implementation correctly handles nested templates and prevents circular dependencies.
|
|
||
| // Create a resolver with the variable provider | ||
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate extension arguments before creating the variable provider.
The code doesn't validate that the elements in the $arguments array are instances of PromptExtensionArgument before passing them to the PromptExtensionVariableProvider constructor.
Add validation:
// Create a variable provider for the extension arguments
+ // Validate arguments
+ foreach ($arguments as $index => $argument) {
+ if (!$argument instanceof PromptExtensionArgument) {
+ $this->logger?->warning('Invalid argument type in array', [
+ 'index' => $index,
+ 'type' => get_debug_type($argument),
+ ]);
+ // Skip invalid arguments or throw an exception based on your error handling strategy
+ }
+ }
$variableProvider = new PromptExtensionVariableProvider($arguments);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a resolver with the variable provider | |
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); | |
| // Create a variable provider for the extension arguments | |
| // Validate arguments | |
| foreach ($arguments as $index => $argument) { | |
| if (!$argument instanceof PromptExtensionArgument) { | |
| $this->logger?->warning('Invalid argument type in array', [ | |
| 'index' => $index, | |
| 'type' => get_debug_type($argument), | |
| ]); | |
| // Skip invalid arguments or throw an exception based on your error handling strategy | |
| } | |
| } | |
| $variableProvider = new PromptExtensionVariableProvider($arguments); | |
| // Create a resolver with the variable provider | |
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); |
| private function substituteMessages(array $messages, array $arguments): array | ||
| { | ||
| $result = []; | ||
|
|
||
| // Create a variable provider for the extension arguments | ||
| $variableProvider = new PromptExtensionVariableProvider($arguments); | ||
|
|
||
| // Create a resolver with the variable provider | ||
| $resolver = $this->variableResolver->with(new VariableReplacementProcessor($variableProvider, $this->logger)); | ||
|
|
||
| foreach ($messages as $message) { | ||
| \assert($message->content instanceof TextContent); | ||
| $content = $message->content->text; | ||
| $substitutedContent = $resolver->resolve($content); | ||
|
|
||
| $this->logger?->debug('Template message processed', [ | ||
| 'original' => $content, | ||
| 'resolved' => $substitutedContent, | ||
| ]); | ||
|
|
||
| // Create a new message with the substituted content | ||
| $result[] = new PromptMessage( | ||
| role: $message->role, | ||
| content: new TextContent(text: $substitutedContent), | ||
| ); | ||
| } | ||
|
|
||
| return $result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for message array elements.
The method assumes all elements in the $messages array will have a content property that's a TextContent instance, but this isn't validated before iteration.
Add validation at the beginning of the method:
private function substituteMessages(array $messages, array $arguments): array
{
$result = [];
+ // Validate message array elements
+ foreach ($messages as $index => $message) {
+ if (!$message instanceof PromptMessage) {
+ $this->logger?->warning('Invalid message type in array', [
+ 'index' => $index,
+ 'type' => get_debug_type($message),
+ ]);
+ continue;
+ }
+ }
// Create a variable provider for the extension arguments
$variableProvider = new PromptExtensionVariableProvider($arguments);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/McpServer/Prompt/Extension/TemplateResolver.php (2)
121-125: Validate extension arguments before creating the variable provider.The code doesn't validate that the elements in the
$argumentsarray are instances ofPromptExtensionArgumentbefore passing them to thePromptExtensionVariableProviderconstructor.Add validation:
// Create a variable provider for the extension arguments + // Validate arguments + foreach ($arguments as $index => $argument) { + if (!$argument instanceof PromptExtensionArgument) { + $this->logger?->warning('Invalid argument type in array', [ + 'index' => $index, + 'type' => get_debug_type($argument), + ]); + // Skip invalid arguments or throw an exception based on your error handling strategy + } + } $variableProvider = new PromptExtensionVariableProvider($arguments);
127-136:⚠️ Potential issueCheck for content type safety before accessing
textproperty.The assertion
\assert($message->content instanceof TextContent)provides a type check in development, but assertions may be disabled in production. This matches the pattern that was flagged by static analysis in a previous review comment.I recommend using explicit type checking with error handling:
foreach ($messages as $message) { - \assert($message->content instanceof TextContent); - $content = $message->content->text; + if (!$message->content instanceof TextContent) { + $this->logger?->warning('Expected TextContent, got different content type', [ + 'content_type' => get_debug_type($message->content), + ]); + continue; + } + $content = $message->content->text; $substitutedContent = $resolver->resolve($content); $this->logger?->debug('Template message processed', [ 'original' => $content, 'resolved' => $substitutedContent, ]);
🧹 Nitpick comments (21)
tests/fixtures/McpServer/Tool/http_tools.yaml (1)
21-21: Remove trailing whitespace.There's a trailing space at the end of this line.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
tests/fixtures/McpServer/Tool/error_tools.yaml (2)
11-11: Remove trailing whitespace.There's a trailing space at the end of this line.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 11-11: trailing spaces
(trailing-spaces)
19-19: Remove trailing whitespace.There's a trailing space at the end of this line.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 19-19: trailing spaces
(trailing-spaces)
tests/src/context.yaml (1)
26-40: Add newline at end of file.The file is missing a newline character at the end, which is a best practice for text files.
- ../../../fixtures/Compiler - showTreeView: true + showTreeView: true +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 40-40: no new line character at the end of file
(new-line-at-end-of-file)
tests/src/Feature/Source/File/AbstractFileSourceTestCase.php (1)
47-49: Consider a more direct approach for retrieving source type.The current implementation serializes and deserializes an object just to extract the
typeproperty. This approach is inefficient and potentially fragile if serialization format changes.protected function getSourceType(SourceInterface $source): string { - return \json_decode(\json_encode($source), true)['type'] ?? ''; + // If SourceInterface has a getType() method, use it directly + if (method_exists($source, 'getType')) { + return $source->getType(); + } + + // Fallback to reflection to access protected/private type property + try { + $reflection = new \ReflectionProperty($source, 'type'); + $reflection->setAccessible(true); + return $reflection->getValue($source) ?? ''; + } catch (\ReflectionException $e) { + return \json_decode(\json_encode($source), true)['type'] ?? ''; + } }tests/src/Feature/McpServer/Prompt/JsonSerializationTest.php (1)
22-87: Refactor long test method into smaller focused tests.The
assertConfigItemsmethod is quite long and tests multiple different aspects of prompt serialization. Breaking it down into separate test methods would improve readability and maintainability.-protected function assertConfigItems(DocumentCompiler $compiler, ConfigRegistryAccessor $config): void +protected function assertConfigItems(DocumentCompiler $compiler, ConfigRegistryAccessor $config): void +{ + $this->assertCompletePromptSerialization($config); + $this->assertMinimalTemplateSerialization($config); + $this->assertExtensionPromptSerialization($config); + $this->assertRegistrySerialization($config); +} + +/** + * Tests serialization of a complete prompt with all fields + */ +private function assertCompletePromptSerialization(ConfigRegistryAccessor $config): void { // Test serialization of complete prompt $completePrompt = $config->getPrompts()->get('complete-prompt'); $this->assertInstanceOf(PromptDefinition::class, $completePrompt); $serialized = \json_encode($completePrompt); $this->assertNotFalse($serialized, 'JSON serialization failed'); $decoded = \json_decode($serialized, true); $this->assertIsArray($decoded); // Check that all important fields are present in the serialized output $this->assertArrayHasKey('id', $decoded); $this->assertArrayHasKey('type', $decoded); $this->assertArrayHasKey('description', $decoded); $this->assertArrayHasKey('schema', $decoded); $this->assertArrayHasKey('messages', $decoded); // Check schema structure $this->assertArrayHasKey('properties', $decoded['schema']); $this->assertArrayHasKey('required', $decoded['schema']); // Check that required field contains 'language' $this->assertContains('language', $decoded['schema']['required']); +} + +/** + * Tests serialization of a minimal template + */ +private function assertMinimalTemplateSerialization(ConfigRegistryAccessor $config): void +{ // Test serialization of minimal template $minimalTemplate = $config->getPrompts()->get('minimal-template'); $serializedTemplate = \json_encode($minimalTemplate); $this->assertNotFalse($serializedTemplate, 'Template JSON serialization failed'); $decodedTemplate = \json_decode($serializedTemplate, true); $this->assertIsArray($decodedTemplate); // Check that type is set to 'template' $this->assertSame('template', $decodedTemplate['type']); +} + +/** + * Tests serialization of a prompt that extends a template + */ +private function assertExtensionPromptSerialization(ConfigRegistryAccessor $config): void +{ // Test serialization of extension-only prompt $extensionPrompt = $config->getPrompts()->get('extension-only-prompt'); $serializedExtension = \json_encode($extensionPrompt); $this->assertNotFalse($serializedExtension, 'Extension prompt JSON serialization failed'); $decodedExtension = \json_decode($serializedExtension, true); $this->assertIsArray($decodedExtension); // Check that extend field is present and properly formatted $this->assertArrayHasKey('extend', $decodedExtension); $this->assertIsArray($decodedExtension['extend']); $this->assertCount(1, $decodedExtension['extend']); $this->assertSame('minimal-template', $decodedExtension['extend'][0]['id']); +} + +/** + * Tests registry serialization excludes templates + */ +private function assertRegistrySerialization(ConfigRegistryAccessor $config): void +{ // Test registry serialization $registry = $config->getPrompts(); $serializedRegistry = \json_encode($registry); $this->assertNotFalse($serializedRegistry, 'Registry JSON serialization failed'); $decodedRegistry = \json_decode($serializedRegistry, true); $this->assertIsArray($decodedRegistry); $this->assertArrayHasKey('prompts', $decodedRegistry); // The registry should only serialize regular prompts, not templates $promptsInSerialized = $decodedRegistry['prompts']; foreach ($promptsInSerialized as $prompt) { $this->assertNotEquals('template', $prompt['type'] ?? 'prompt'); } }tests/src/Feature/Compiler/AbstractCompilerTestCase.php (1)
29-167: Consider extracting expected test outputs to fixture files.The method contains large hardcoded string literals for expected test outputs, which can make maintenance difficult. Moving these to external fixture files would improve readability and maintainability.
protected function assertConfigItems(DocumentCompiler $compiler, ConfigRegistryAccessor $config): void { $outputPaths = []; $results = []; foreach ($config->getDocuments() as $document) { $outputPaths[] = $this->getContextsDir($document->outputPath); $compiledDocument = $compiler->compile($document); $results[$document->outputPath] = (string) $compiledDocument->content; } foreach ($outputPaths as $outputPath) { $this->assertFileExists($outputPath); } - $this->assertSame( - <<<'CONTENT' - # Project structure - ### - ``` - └── src/ [309.0 B, 309 chars] - └── dir1/ [217.0 B, 217 chars] - ├── Test1Class.php [68.0 B, 68 chars] - ├── TestClass.php [67.0 B, 67 chars] - ├── file.txt [7.0 B, 7 chars] - ├── subdir1/ [75.0 B, 75 chars] - │ └── Test3Class.php [68.0 B, 68 chars] - │ └── file.txt [7.0 B, 7 chars] - └── dir2/ [92.0 B, 92 chars] - └── Test2Class.php [85.0 B, 85 chars] - └── file.txt [7.0 B, 7 chars] - - ``` - CONTENT, - $results['structure.md'], - ); + $this->assertSame( + $this->getExpectedOutput('structure.md'), + $results['structure.md'] + ); - $this->assertSame( - <<<'CONTENT' - # This is a test document - ``` - // Structure of documents - └── src/ - └── dir1/ - └── Test1Class.php - └── TestClass.php - └── subdir1/ - └── Test3Class.php - - ``` - ### Path: `/src/dir1/Test1Class.php` - - ```php - <?php - - declare(strict_types=1); - - final readonly class Test1Class {} - - ``` - ### Path: `/src/dir1/TestClass.php` - - ```php - <?php - - declare(strict_types=1); - - final readonly class TestClass {} - - ``` - ### Path: `/src/dir1/subdir1/Test3Class.php` - - ```php - <?php - - declare(strict_types=1); - - final readonly class Test3Class {} - - ``` - <INSTRUCTION> - Foo Bar - </INSTRUCTION> - ------------------------------------------------------------ - CONTENT, - $results['test-document.md'], - ); + $this->assertSame( + $this->getExpectedOutput('test-document.md'), + $results['test-document.md'] + ); - $this->assertSame( - <<<'CONTENT' - # This is a test document 1 - ``` - // Structure of documents - └── src/ - └── dir2/ - └── Test2Class.php - - ``` - ### Path: `/src/dir2/Test2Class.php` - - ```php - <?php - - declare(strict_types=1); - - namespace dir2; - - final readonly class Test2Class {} - - ``` - <INSTRUCTION> - Foo Bar - </INSTRUCTION> - ------------------------------------------------------------ - CONTENT, - $results['test-document1.md'], - ); + $this->assertSame( + $this->getExpectedOutput('test-document1.md'), + $results['test-document1.md'] + ); - $this->assertSame( - <<<'CONTENT' - # This is a test document 2 - ``` - // Structure of documents - └── src/ - └── dir1/ - ├── file.txt - ├── subdir1/ - │ └── file.txt - └── dir2/ - └── file.txt - - ``` - ### Path: `/src/dir1/file.txt` - - ```txt - foo baf - ``` - ### Path: `/src/dir1/subdir1/file.txt` - - ```txt - foo bar - ``` - ### Path: `/src/dir2/file.txt` - - ```txt - foo baz - ``` - <INSTRUCTION> - Foo Bar - </INSTRUCTION> - ------------------------------------------------------------ - CONTENT, - $results['test-document2.md'], - ); + $this->assertSame( + $this->getExpectedOutput('test-document2.md'), + $results['test-document2.md'] + ); } +/** + * Gets the expected output for a given document + * + * @param string $documentName The document name + * @return string The expected output + */ +private function getExpectedOutput(string $documentName): string +{ + $expectedOutputFile = $this->getFixturesDir('Compiler/ExpectedOutputs') . '/' . $documentName; + + if (!file_exists($expectedOutputFile)) { + throw new \RuntimeException("Expected output file not found: {$expectedOutputFile}"); + } + + return file_get_contents($expectedOutputFile); +}src/McpServer/Prompt/Extension/PromptDefinition.php (2)
34-59: Add return type annotation to the jsonSerialize method.While the implementation is correct, adding a specific return type annotation would improve type safety and code clarity.
- public function jsonSerialize(): array + public function jsonSerialize(): arrayYou're correctly filtering out null and empty values, which keeps the JSON output clean and consistent.
61-80: Consider adding more specific array type annotation.The PHPDoc comment for
serializeExtensions()usesarray<mixed>which is quite generic. A more specific type annotation would improve code clarity and IDE support.- * @return array<mixed>|null The serialized extensions or null if empty + * @return array<int, array{id: string, arguments: array<string, mixed>}>|null The serialized extensions or null if emptytests/src/Feature/McpServer/Prompt/TemplatePromptTest.php (1)
16-59: Add test cases for error conditions.Consider adding tests for error conditions such as:
- Extending a non-existent template
- Circular dependencies between templates
- Missing required template variables
This would ensure robust error handling in the template resolution system.
tests/src/Feature/McpServer/Tool/ToolHandlerTest.php (2)
171-173: Enhance HTTP tool result validation.The current assertion only checks that the result contains "success". Consider adding more thorough validations:
- // Verify the result - $this->assertStringContainsString('success', $result['output']); + // Verify the result + $this->assertStringContainsString('success', $result['output']); + $this->assertTrue($result['success']); + $this->assertArrayHasKey('requests', $result); + $this->assertCount(1, $result['requests']); + $this->assertSame('GET', $result['requests'][0]['method']); + $this->assertStringContainsString('example.com/api/test', $result['requests'][0]['url']);
29-173: Add test cases for error handling scenarios.The current tests only verify the happy path. Consider adding tests for:
- HTTP request failures (network errors, 4xx/5xx responses)
- Command execution errors (non-zero exit codes)
- Missing required variables in templates
This would ensure the tool handlers properly handle error conditions.
tests/docs/feature-test-guidelines.md (2)
34-58: Comprehensive guide for creating test fixtures.The section covers important aspects:
- Fixture placement and organization
- Schema validation
- Example YAML structure
However, there's a minor grammatical issue in line 41.
-- Use the JSON schema from the project to ensure your YAML is valid -- Include a variety of configurations to test different edge cases +- Use the JSON schema from the project to ensure your YAML is valid +- Include a variety of configurations to test different edge casesConsider adding a dash before "Include" to maintain consistent list formatting.
🧰 Tools
🪛 LanguageTool
[grammar] ~41-~41: It seems that “to” is missing before the verb.
Context: ... project to ensure your YAML is valid - Include a variety of configurations to test dif...(MISSING_TO_BETWEEN_BE_AND_VB)
[style] ~41-~41: Consider using a synonym to be more concise.
Context: ... to ensure your YAML is valid - Include a variety of configurations to test different edge c...(A_VARIETY_OF)
1-239: Add section on test debugging and troubleshooting.Consider adding a section on debugging failing tests, including:
- Tips for interpreting common failure patterns
- How to troubleshoot configuration loading issues
- Debugging variable resolution problems
- Handling unexpected exceptions
This would make the document more complete as a reference guide.
🧰 Tools
🪛 LanguageTool
[grammar] ~41-~41: It seems that “to” is missing before the verb.
Context: ... project to ensure your YAML is valid - Include a variety of configurations to test dif...(MISSING_TO_BETWEEN_BE_AND_VB)
[style] ~41-~41: Consider using a synonym to be more concise.
Context: ... to ensure your YAML is valid - Include a variety of configurations to test different edge c...(A_VARIETY_OF)
[style] ~80-~80: Try using a synonym here to strengthen your writing.
Context: ...ConfigRegistryAccessorinstance which gives you access to the parsed configuration ```...(GIVE_PROVIDE)
src/McpServer/Prompt/Extension/TemplateResolver.php (3)
102-108: Simplify array merging operation.The current loop structure to merge arrays can be simplified to improve readability.
Consider using
array_merge()or the spread operator:- foreach ($this->substituteMessages($templateMessages, $arguments) as $templateMessage) { - $promptMessages[] = $templateMessage; - } - - // Otherwise, keep the prompt's messages (extensions just provide structure) - return $promptMessages; + // Merge template messages (with substitution) with the prompt's messages + return array_merge($promptMessages, $this->substituteMessages($templateMessages, $arguments));
47-62: Enhance circular dependency detection to prevent indirect cycles.The current circular dependency check only prevents direct circular dependencies (A extends A) but might not catch indirect ones (A extends B extends A).
Consider tracking the entire ancestry chain instead of just processed IDs:
public function resolve(PromptDefinition $prompt): PromptDefinition { // If no extensions, return the prompt as is if (empty($prompt->extensions)) { return $prompt; } // Process each extension $messages = $prompt->messages; - $processedExtensions = []; + return $this->resolveWithAncestry($prompt, $messages, []); +} + +/** + * Resolves a prompt definition with ancestry tracking to prevent circular dependencies. + * + * @param PromptDefinition $prompt The prompt definition to resolve + * @param array $messages Current accumulated messages + * @param array $ancestry Chain of template IDs in the current resolution path + * @return PromptDefinition The resolved prompt definition + * @throws TemplateResolutionException If the resolution fails + */ +private function resolveWithAncestry(PromptDefinition $prompt, array $messages, array $ancestry): PromptDefinition +{ + // Add current prompt to ancestry + $ancestry[] = $prompt->id; foreach ($prompt->extensions as $extension) { // Prevent circular dependencies - if (\in_array($extension->templateId, $processedExtensions, true)) { + if (\in_array($extension->templateId, $ancestry, true)) { + $this->logger?->warning('Circular dependency detected', [ + 'template_id' => $extension->templateId, + 'ancestry' => $ancestry, + ]); continue; } - $processedExtensions[] = $extension->templateId;
35-85: Consider adding method to resolve multiple prompts efficiently.The current design resolves templates one at a time, which could lead to redundant resolutions if multiple prompts extend the same templates.
Consider adding a method to resolve multiple prompts efficiently with template caching:
/** * Resolves multiple prompts efficiently by caching resolved templates. * * @param PromptDefinition[] $prompts The prompts to resolve * @return PromptDefinition[] The resolved prompts * @throws TemplateResolutionException If resolution fails */ public function resolveMany(array $prompts): array { $templateCache = []; $result = []; foreach ($prompts as $prompt) { $result[] = $this->resolveWithCache($prompt, $templateCache); } return $result; } /** * Resolves a prompt with template caching. * * @param PromptDefinition $prompt The prompt to resolve * @param array $templateCache Cache of already resolved templates * @return PromptDefinition The resolved prompt */ private function resolveWithCache(PromptDefinition $prompt, array &$templateCache): PromptDefinition { // Implementation that uses and updates the cache // ... }tests/src/Feature/FeatureTestCases.php (4)
36-45: Add error handling for directory deletion in tearDown.The current implementation doesn't check if the directory deletion was successful, which could leave test artifacts behind in case of failure.
Consider adding error handling:
protected function tearDown(): void { parent::tearDown(); if (\is_dir($this->getContextsDir())) { $files = new Files(); - $files->deleteDirectory($this->getContextsDir()); + try { + $files->deleteDirectory($this->getContextsDir()); + } catch (\Exception $e) { + $this->addWarning(sprintf('Failed to clean up context directory: %s', $e->getMessage())); + } } }
47-50: Simplify path construction in getContextsDir method.The current path creation is a bit verbose and could be simplified for better readability.
Consider using a more direct approach:
protected function getContextsDir(string $path = ''): string { - return (string) FSPath::create($this->getFixturesDir('Compiler/.context'))->join($path); + $contextDir = $this->getFixturesDir('Compiler/.context'); + return $path ? "$contextDir/$path" : $contextDir; }
59-64: Add error handling for configuration loading failures.The current implementation doesn't have error handling if the configuration fails to load, which could lead to unclear test failures.
Consider adding error handling:
private function compileDocuments(DocumentCompiler $compiler, ConfigurationProvider $configProvider): void { - $loader = $configProvider->fromPath($this->getConfigPath()); + try { + $loader = $configProvider->fromPath($this->getConfigPath()); + $this->assertConfigItems($compiler, new ConfigRegistryAccessor($loader->load())); + } catch (\Exception $e) { + $this->fail(sprintf('Failed to load configuration: %s', $e->getMessage())); + } - $this->assertConfigItems($compiler, new ConfigRegistryAccessor($loader->load())); }
25-34: Consider adding test annotations for better test organization.The
compile()method currently has only a basic#[Test]annotation, which doesn't provide much information about the test purpose.Consider adding more descriptive annotations:
-#[Test] +#[Test] +#[\PHPUnit\Framework\Attributes\Group('feature')] +#[\PHPUnit\Framework\Attributes\DisplayName('Compiles documents with configuration')] public function compile(): void { $this->getContainer()->runScope( bindings: new Scope( name: AppScope::Compiler, ), scope: $this->compileDocuments(...), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/Document/DocumentRegistry.php(2 hunks)src/McpServer/Action/Prompts/GetPromptAction.php(1 hunks)src/McpServer/Prompt/Extension/PromptDefinition.php(1 hunks)src/McpServer/Prompt/Extension/TemplateResolver.php(1 hunks)src/McpServer/Prompt/PromptMessageProcessor.php(1 hunks)src/McpServer/Prompt/PromptProviderInterface.php(2 hunks)src/McpServer/Prompt/PromptRegistry.php(6 hunks)tests/docs/feature-test-guidelines.md(1 hunks)tests/fixtures/McpServer/Prompts/basic_prompts.yaml(1 hunks)tests/fixtures/McpServer/Prompts/json_serialization.yaml(1 hunks)tests/fixtures/McpServer/Prompts/prompt_schema.yaml(1 hunks)tests/fixtures/McpServer/Prompts/registry_operations.yaml(1 hunks)tests/fixtures/McpServer/Prompts/templates.yaml(1 hunks)tests/fixtures/McpServer/Tool/basic_tools.yaml(1 hunks)tests/fixtures/McpServer/Tool/error_tools.yaml(1 hunks)tests/fixtures/McpServer/Tool/http_tools.yaml(1 hunks)tests/fixtures/McpServer/Tool/run_tools.yaml(1 hunks)tests/fixtures/Source/File/basic_file_source.yaml(1 hunks)tests/fixtures/Source/File/filtered_file_source.yaml(1 hunks)tests/fixtures/Source/File/invalid_file_source.yaml(1 hunks)tests/fixtures/Source/File/test_files/test_directory/nested_file.txt(1 hunks)tests/fixtures/Source/File/test_files/test_file.txt(1 hunks)tests/fixtures/Source/File/tree_view_file_source.yaml(1 hunks)tests/src/Feature/Compiler/AbstractCompilerTestCase.php(1 hunks)tests/src/Feature/FeatureTestCases.php(1 hunks)tests/src/Feature/McpServer/Prompt/BasicPromptTest.php(1 hunks)tests/src/Feature/McpServer/Prompt/JsonSerializationTest.php(1 hunks)tests/src/Feature/McpServer/Prompt/RegistryOperationsTest.php(1 hunks)tests/src/Feature/McpServer/Prompt/SchemaPromptTest.php(1 hunks)tests/src/Feature/McpServer/Prompt/TemplatePromptTest.php(1 hunks)tests/src/Feature/McpServer/Tool/BasicToolConfigTest.php(1 hunks)tests/src/Feature/McpServer/Tool/HttpToolTest.php(1 hunks)tests/src/Feature/McpServer/Tool/RunToolTest.php(1 hunks)tests/src/Feature/McpServer/Tool/ToolHandlerTest.php(1 hunks)tests/src/Feature/Source/File/AbstractFileSourceTestCase.php(1 hunks)tests/src/Feature/Source/File/FileSourceFilterTest.php(1 hunks)tests/src/Feature/Source/File/FileSourceSerializationTest.php(1 hunks)tests/src/Feature/Source/File/FileSourceTest.php(1 hunks)tests/src/Feature/Source/File/FileSourceTreeViewTest.php(1 hunks)tests/src/context.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- tests/fixtures/Source/File/basic_file_source.yaml
- tests/fixtures/Source/File/invalid_file_source.yaml
- tests/fixtures/Source/File/filtered_file_source.yaml
- tests/fixtures/McpServer/Tool/basic_tools.yaml
- tests/fixtures/Source/File/tree_view_file_source.yaml
- tests/fixtures/McpServer/Tool/run_tools.yaml
- tests/fixtures/McpServer/Prompts/registry_operations.yaml
- tests/fixtures/McpServer/Prompts/prompt_schema.yaml
- tests/fixtures/Source/File/test_files/test_directory/nested_file.txt
- tests/fixtures/McpServer/Prompts/basic_prompts.yaml
- tests/fixtures/McpServer/Prompts/templates.yaml
- tests/fixtures/McpServer/Prompts/json_serialization.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/McpServer/Prompt/PromptProviderInterface.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/src/Feature/McpServer/Tool/BasicToolConfigTest.php (5)
src/Config/Registry/ConfigRegistryAccessor.php (1)
ConfigRegistryAccessor(15-73)src/McpServer/Tool/Config/ToolDefinition.php (1)
ToolDefinition(10-146)tests/src/Feature/FeatureTestCases.php (1)
FeatureTestCases(17-65)tests/src/TestCase.php (1)
getFixturesDir(62-71)src/McpServer/Tool/Config/ToolSchema.php (1)
getProperties(54-57)
tests/src/Feature/McpServer/Tool/ToolHandlerTest.php (5)
src/Lib/Variable/VariableResolver.php (2)
VariableResolver(10-39)with(16-22)src/McpServer/Tool/Config/ToolArg.php (1)
ToolArg(10-77)src/McpServer/Tool/Config/ToolCommand.php (1)
ToolCommand(10-109)src/McpServer/Tool/Config/ToolDefinition.php (1)
ToolDefinition(10-146)tests/src/AppTestCase.php (1)
AppTestCase(12-99)
🪛 GitHub Check: psalm (ubuntu-latest, 8.3, locked)
src/Document/DocumentRegistry.php
[failure] 12-12: MissingTemplateParam
src/Document/DocumentRegistry.php:12:60: MissingTemplateParam: Butschster\ContextGenerator\Document\DocumentRegistry has missing template params when extending ArrayAccess, expecting 2 (see https://psalm.dev/182)
🪛 GitHub Actions: 🔍 Static analysis
src/Document/DocumentRegistry.php
[error] 12-12: Psalm: Missing template parameters when extending ArrayAccess, expecting 2 template params (MissingTemplateParam). See https://psalm.dev/182
🪛 LanguageTool
tests/docs/feature-test-guidelines.md
[grammar] ~41-~41: It seems that “to” is missing before the verb.
Context: ... project to ensure your YAML is valid - Include a variety of configurations to test dif...
(MISSING_TO_BETWEEN_BE_AND_VB)
[style] ~41-~41: Consider using a synonym to be more concise.
Context: ... to ensure your YAML is valid - Include a variety of configurations to test different edge c...
(A_VARIETY_OF)
[style] ~80-~80: Try using a synonym here to strengthen your writing.
Context: ...ConfigRegistryAccessor instance which gives you access to the parsed configuration ```...
(GIVE_PROVIDE)
tests/fixtures/Source/File/test_files/test_file.txt
[grammar] ~1-~1: Consider using either the past participle “tested” or the present participle “testing” here.
Context: This is test content for the file source test.
(BEEN_PART_AGREEMENT)
🪛 YAMLlint (1.35.1)
tests/fixtures/McpServer/Tool/error_tools.yaml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 24-24: trailing spaces
(trailing-spaces)
tests/fixtures/McpServer/Tool/http_tools.yaml
[error] 21-21: trailing spaces
(trailing-spaces)
tests/src/context.yaml
[error] 40-40: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (76)
tests/fixtures/Source/File/test_files/test_file.txt (1)
1-1: Simple, appropriate test content.The file content serves its purpose as a test fixture for file source tests.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Consider using either the past participle “tested” or the present participle “testing” here.
Context: This is test content for the file source test.(BEEN_PART_AGREEMENT)
tests/fixtures/McpServer/Tool/http_tools.yaml (2)
4-20: Well-structured HTTP GET tool configuration.The HTTP GET tool is properly defined with schema validation for endpoint and token parameters, and correctly uses variable substitution syntax for dynamic values.
22-37: Well-structured HTTP POST tool configuration.The HTTP POST tool is properly defined with appropriate headers, request method, and body structure. The variable substitution in the body is correctly implemented.
tests/fixtures/McpServer/Tool/error_tools.yaml (4)
4-10: Well-structured error test case for missing required field.This tool definition intentionally omits the required description field, which is appropriate for testing schema validation.
12-18: Good error test case for invalid tool type.This tool definition correctly tests the handling of an invalid tool type value.
20-24: Appropriate test case for missing commands array.This test case correctly omits the required commands array for a run-type tool.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
25-29: Good error test case for HTTP tool without requests.This test case correctly omits the required requests array for an HTTP tool.
tests/src/context.yaml (2)
3-14: Well-structured prompts test document configuration.The document group for prompts tests is properly organized with clear description, output path, and source paths.
15-25: Good tools test document configuration.The document group for tools tests is properly organized with appropriate source paths.
src/McpServer/Action/Prompts/GetPromptAction.php (2)
32-32: Good architectural improvement passing request attributes to prompt provider.This change properly delegates template processing to the prompt provider by passing request attributes, aligning with the new template-based prompt extension system.
35-35: Simplified response handling.This change streamlines the response by directly using the processed messages from the prompt, which is cleaner and better aligned with the separation of concerns principle.
src/Document/DocumentRegistry.php (2)
49-57: Implementation of offsetExists and offsetGet looks good.The read-only array access implementation is a good addition, allowing access to documents by their numeric index while maintaining the collection's integrity.
59-67: Proper protection against direct modification of the registry.Good implementation of write-protection methods that throw appropriate exceptions when someone tries to modify the collection directly, enforcing the use of the register() method instead.
tests/src/Feature/McpServer/Tool/HttpToolTest.php (5)
13-18: LGTM! Clear test class setup.The test class structure is clean and follows the established pattern for feature tests, with a clear method to specify the configuration path.
20-41: Well-structured test for HTTP GET tool configuration.The test correctly verifies the basic tool properties and dives into the HTTP-specific request configuration, making appropriate assertions about the URL, method, and headers.
42-55: Good validation of schema properties for variable replacement.This section properly verifies that the schema contains the expected parameters that can be used for variable substitution in the request URL and headers.
56-81: Comprehensive validation of HTTP POST tool configuration.The test thoroughly validates the POST tool configuration, including headers, body structure, and schema properties.
82-86: Good verification of handler instantiation.The test confirms that the correct handler class is instantiated for HTTP tools, which is an important aspect to validate.
tests/src/Feature/Source/File/FileSourceTest.php (4)
11-16: Well-documented test class with appropriate grouping.The class uses PHPUnit attributes for test grouping and includes a clear docblock explaining its purpose.
18-22: LGTM! Correct override implementation.The method is properly tagged with the
#[\Override]attribute and correctly implements the abstract method from the parent class.
24-36: Good validation of source configuration properties.The test properly verifies the source type, description, and file pattern configuration, as well as tree view settings.
37-50: Comprehensive output validation.The test validates both document metadata and that the compiled content contains the expected text from the fixture files. This ensures both the structure and content are correctly processed.
tests/src/Feature/McpServer/Tool/BasicToolConfigTest.php (4)
12-17: LGTM! Clean test class setup.The class extends the appropriate base class and correctly implements the method to specify the configuration path.
19-34: Good validation of basic tool properties.The test correctly verifies the tool registry's existence and validates the essential properties of the test command tool.
35-41: Thorough command validation.The test properly verifies the command structure, including the command name and arguments, ensuring the tool definition is parsed correctly.
42-47: Good schema property validation.The test correctly verifies that the tool schema contains the expected property and that it has the correct type.
tests/src/Feature/McpServer/Prompt/SchemaPromptTest.php (6)
1-21: Well-structured test setup for schema-based prompts.The class is properly organized with clear namespace, imports, and extends from the base
FeatureTestCasesclass. ThegetConfigPath()method correctly points to the fixture file for testing.
22-41: Good implementation of test assertions for basic schema prompt.The test properly retrieves the prompt definition and verifies its schema properties, including checking the argument count and initializing variables for detailed argument inspection.
43-51: Thorough validation of prompt argument properties.The test correctly validates both required and optional arguments, checking their descriptions and required flags to ensure proper schema configuration.
52-56: Proper variable placeholder validation in prompt messages.The test verifies that variable placeholders ({{language}} and {{experience}}) are correctly preserved in the prompt messages, which is essential for the template-based prompt extension system.
57-82: Comprehensive testing of complex schema prompt.The test thoroughly validates a more complex prompt configuration, including argument counts, required arguments filtering, and argument name verification. Good use of array functions for processing argument collections.
83-87: Complete variable placeholder verification for complex prompt.The test properly checks that all expected variable placeholders are present in the prompt message content, ensuring template functionality works correctly.
src/McpServer/Prompt/PromptMessageProcessor.php (2)
1-19: Well-designed processor class for handling prompt message variables.The class is properly declared as final readonly, uses constructor property promotion, and has appropriate namespacing and imports. Good dependency injection of the VariableResolver.
20-40: Clean implementation of message processing with variable substitution.The process method correctly:
- Takes a PromptDefinition and optional arguments array
- Maps over each message to replace variables in TextContent
- Creates new immutable instances rather than modifying existing ones
- Returns a new PromptDefinition with processed messages
This immutable approach ensures thread safety and prevents side effects.
tests/src/Feature/Source/File/FileSourceSerializationTest.php (4)
1-24: Well-structured test setup for FileSource serialization.The class is properly organized with appropriate namespace, imports, and group attributes for test categorization. The
getConfigPath()method correctly points to the basic file source fixture.
25-43: Thorough basic serialization test for FileSource.The test properly validates JSON serialization of a file source from config, checking that the serialized object contains all required fields with correct values.
44-62: Proper validation of minimal FileSource serialization.The test creates a FileSource with minimal parameters and verifies that optional fields are correctly omitted from the serialized output, preventing unnecessary data in the JSON.
63-98: Comprehensive testing of fully-populated FileSource serialization.The test creates a FileSource with all possible options and verifies that all fields are properly serialized, including nested objects like TreeViewConfig and numeric values. Good coverage of edge cases.
tests/src/Feature/McpServer/Tool/RunToolTest.php (5)
1-21: Well-structured test setup for run tools.The class is properly organized with appropriate namespace, imports, and extends from the base feature test case. The
getConfigPath()method correctly points to the run tools fixture.
22-37: Proper validation of tool registry and basic tool properties.The test correctly verifies the presence of the expected tool in the registry and validates its basic properties including id, description, and type.
38-53: Thorough testing of command arguments including variable placeholders.The test properly validates both simple variable placeholders and conditional arguments with "when" clauses, ensuring the tool's command structure is correctly configured.
54-66: Comprehensive schema property validation.The test verifies the schema properties including types and default values, ensuring that the tool's input validation will work correctly with both required and optional parameters.
67-71: Proper handler factory verification.The test correctly validates that the appropriate handler class (RunToolHandler) is instantiated for the tool type, ensuring the tool execution will use the correct implementation.
src/McpServer/Prompt/PromptRegistry.php (5)
22-24: Good use of dependency injection for the processor.Adding the
PromptMessageProcessordependency via constructor injection is a clean approach that follows dependency injection best practices and improves testability.
34-46: Nice implementation of variable substitution support.The updated
get()method now accepts an$argumentsarray, which enables the template variable substitution feature. The implementation correctly delegates the processing to the injected processor.
58-64: Good addition of template filtering functionality.The new
allTemplates()method provides a clean way to retrieve only template-type prompts from the registry, using array_filter with a type check. This supports the new template feature well.
79-87: Well-implemented prompt filtering.The
getItems()method has been updated to filter out templates and return only regular prompts. The use ofarray_values()ensures that the returned array maintains sequential numeric indexes.
89-94: Clean API approach for JSON serialization.Excluding templates from JSON serialization keeps the API clean and focused, as mentioned in the PR objectives. The comment clearly explains the intention behind this behavior.
tests/src/Feature/McpServer/Prompt/RegistryOperationsTest.php (6)
19-22: Good test fixture organization.Using a dedicated YAML fixture for the test provides clean separation of test data and test logic.
29-32: Thorough testing of the 'has' method.The test properly checks both positive and negative cases for the
has()method, including regular prompts, templates, and non-existent entries.
34-45: Comprehensive testing of the 'get' method.The test verifies retrieval of both regular prompts and templates, and properly checks exception handling for non-existent prompts.
54-61: Good validation of template filtering.The test confirms that
allTemplates()correctly returns only template-type prompts and verifies the expected count and type of each template.
62-68: Proper verification of regular prompt filtering.The test ensures that
getItems()returns only regular prompts (excluding templates) and verifies the expected count and type of each item.
73-80: Complete iterator interface testing.The test thoroughly verifies that the iterator implementation correctly yields only regular prompts and maintains the expected count.
tests/src/Feature/Source/File/FileSourceTreeViewTest.php (2)
30-38: Complete tree view configuration verification.The test thoroughly checks all tree view configuration options, ensuring they're correctly set according to the fixture.
44-51: Good output validation strategy.The test sensibly checks for the presence of key elements in the rendered output without being overly prescriptive about the exact format, which makes the test less brittle to formatting changes.
tests/src/Feature/Source/File/FileSourceFilterTest.php (3)
18-22: Good use of Override attribute.Using the
#[\Override]attribute explicitly indicates that this method is overriding a parent class method, improving code clarity.
31-34: Thorough filter configuration verification.The test properly validates all aspects of the file source filter configuration, including path restrictions, exclusion patterns, and content filters.
40-43: Comprehensive filter output validation.The test effectively verifies both inclusion and exclusion filtering by checking for the presence of expected content and the absence of filtered content.
tests/src/Feature/McpServer/Prompt/BasicPromptTest.php (1)
24-53: The test implementation looks clean and well-structured.This test effectively validates three prompt types (minimal, multi-message, and with variables) and thoroughly checks their structure, content, and variable resolution.
tests/src/Feature/Compiler/AbstractCompilerTestCase.php (1)
9-11: Class inheritance change looks appropriate.The refactoring to extend
FeatureTestCasesaligns with the PR objective of introducing a template-based system. This change likely centralizes common testing logic in the parent class.src/McpServer/Prompt/Extension/PromptDefinition.php (3)
1-11: Well-organized class structure with appropriate namespacing.The class is correctly placed in the namespace that reflects its role in the prompt extension system, with all necessary imports. The use of strict types is a good practice.
11-21: Good use of PHP 8 features and type safety.The class makes excellent use of PHP 8's features:
final readonlyensures immutability- Constructor property promotion for concise code
- Type declarations for all properties
- Default values for optional properties
- PHPDoc array annotations for collection properties
This implementation aligns well with the PR's objective of adding a template-based prompt extension system.
23-32: Properly implemented immutability pattern.The
withMessagesmethod correctly follows the immutability pattern by returning a new instance with modified messages while preserving all other properties. This is essential for maintaining state consistency when working with prompt definitions.tests/src/Feature/McpServer/Prompt/TemplatePromptTest.php (5)
1-17: Well-structured test class with clear purpose.The test class is properly defined with strict types, appropriate namespace, and necessary imports. The class documentation clearly explains its purpose of testing template-related prompt functionality.
18-21: Correctly implemented config path method.The
getConfigPath()method properly uses the helper method to locate the test fixture file. This follows the pattern established by the feature test framework.
23-37: Comprehensive base template validation.The test properly verifies:
- Instance type correctness
- Template type enumeration value
- Message count
- Content contains expected variables
This thoroughly validates the basic template functionality.
38-49: Thorough verification of template extension functionality.The test correctly validates single template extension by checking:
- Instance type
- Prompt type enumeration
- Extension count and reference
- Message resolution with variable substitution
This confirms the core functionality of template extension works as expected.
50-59: Comprehensive multi-template extension testing.The test verifies that multiple template extensions work correctly, including:
- Proper message merging
- Variable resolution across templates
- Comment noting that second extension takes precedence
This confirms that the inheritance chain for templates functions as designed.
tests/src/Feature/McpServer/Tool/ToolHandlerTest.php (2)
23-85: Well-structured run tool execution test.This test successfully validates:
- Command execution with variable argument replacement
- Proper construction of tool commands
- Result verification including success status and output
The test effectively mocks the command executor and verifies both the execution process and the result format.
87-173: HTTP tool variable replacement test is implemented correctly.The test effectively:
- Mocks the HTTP client with appropriate callback verification
- Sets up a variable resolver with test variables
- Creates a tool definition with variables in URL and headers
- Verifies that variables are correctly resolved in the request
Good use of willReturnCallback to validate the processed URL and headers.
tests/docs/feature-test-guidelines.md (5)
1-10: Excellent introduction to the feature test framework.The document provides a clear introduction explaining the purpose and context of the
FeatureTestCasesabstract class.
5-32: Well-structured code example for basic test structure.The PHP code example clearly illustrates how to extend the
FeatureTestCasesclass and implement the required methods. The code is well-formatted and includes necessary imports.
76-99: Clear explanation of ConfigRegistryAccessor usage.This section effectively demonstrates how to use the
ConfigRegistryAccessorto retrieve and validate configuration items. The code example is clear and demonstrates common assertion patterns.🧰 Tools
🪛 LanguageTool
[style] ~80-~80: Try using a synonym here to strengthen your writing.
Context: ...ConfigRegistryAccessorinstance which gives you access to the parsed configuration ```...(GIVE_PROVIDE)
101-138: Thorough coverage of advanced testing scenarios.The section on error case testing and temporary files for sub-tests provides valuable guidance for testing edge cases and error conditions.
140-157: Comprehensive best practices section.The best practices outlined provide excellent guidance for writing effective feature tests, covering:
- Test organization and focus
- Assertion thoroughness
- Edge case coverage
- Naming conventions
- Documentation
- Test isolation
This will help maintain test quality and consistency.
| $tempDir = $this->createTempDir(); | ||
|
|
||
| // Create a basic structure | ||
| $rootFile = $tempDir . '/root_file.txt'; | ||
| \file_put_contents($rootFile, 'Root file content'); | ||
|
|
||
| $subDir = $tempDir . '/subdir'; | ||
| \mkdir($subDir, 0777, true); | ||
|
|
||
| $nestedFile = $subDir . '/nested_file.txt'; | ||
| \file_put_contents($nestedFile, 'Nested file content'); | ||
|
|
||
| $deepDir = $subDir . '/deep'; | ||
| \mkdir($deepDir, 0777, true); | ||
|
|
||
| $deepFile = $deepDir . '/deep_file.txt'; | ||
| \file_put_contents($deepFile, 'Deep nested file content'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The file creation operations don't include error handling. File system operations can fail due to permission issues, disk space limitations, or other environmental factors.
protected function createTestFileStructure(): string
{
$tempDir = $this->createTempDir();
// Create a basic structure
$rootFile = $tempDir . '/root_file.txt';
- \file_put_contents($rootFile, 'Root file content');
+ if (false === \file_put_contents($rootFile, 'Root file content')) {
+ throw new \RuntimeException("Failed to create root file: $rootFile");
+ }
$subDir = $tempDir . '/subdir';
- \mkdir($subDir, 0777, true);
+ if (!is_dir($subDir) && !mkdir($subDir, 0777, true)) {
+ throw new \RuntimeException("Failed to create directory: $subDir");
+ }
$nestedFile = $subDir . '/nested_file.txt';
- \file_put_contents($nestedFile, 'Nested file content');
+ if (false === \file_put_contents($nestedFile, 'Nested file content')) {
+ throw new \RuntimeException("Failed to create nested file: $nestedFile");
+ }
$deepDir = $subDir . '/deep';
- \mkdir($deepDir, 0777, true);
+ if (!is_dir($deepDir) && !mkdir($deepDir, 0777, true)) {
+ throw new \RuntimeException("Failed to create directory: $deepDir");
+ }
$deepFile = $deepDir . '/deep_file.txt';
- \file_put_contents($deepFile, 'Deep nested file content');
+ if (false === \file_put_contents($deepFile, 'Deep nested file content')) {
+ throw new \RuntimeException("Failed to create deep file: $deepFile");
+ }
return $tempDir;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $tempDir = $this->createTempDir(); | |
| // Create a basic structure | |
| $rootFile = $tempDir . '/root_file.txt'; | |
| \file_put_contents($rootFile, 'Root file content'); | |
| $subDir = $tempDir . '/subdir'; | |
| \mkdir($subDir, 0777, true); | |
| $nestedFile = $subDir . '/nested_file.txt'; | |
| \file_put_contents($nestedFile, 'Nested file content'); | |
| $deepDir = $subDir . '/deep'; | |
| \mkdir($deepDir, 0777, true); | |
| $deepFile = $deepDir . '/deep_file.txt'; | |
| \file_put_contents($deepFile, 'Deep nested file content'); | |
| protected function createTestFileStructure(): string | |
| { | |
| $tempDir = $this->createTempDir(); | |
| // Create a basic structure | |
| $rootFile = $tempDir . '/root_file.txt'; | |
| if (false === \file_put_contents($rootFile, 'Root file content')) { | |
| throw new \RuntimeException("Failed to create root file: $rootFile"); | |
| } | |
| $subDir = $tempDir . '/subdir'; | |
| if (!is_dir($subDir) && !mkdir($subDir, 0777, true)) { | |
| throw new \RuntimeException("Failed to create directory: $subDir"); | |
| } | |
| $nestedFile = $subDir . '/nested_file.txt'; | |
| if (false === \file_put_contents($nestedFile, 'Nested file content')) { | |
| throw new \RuntimeException("Failed to create nested file: $nestedFile"); | |
| } | |
| $deepDir = $subDir . '/deep'; | |
| if (!is_dir($deepDir) && !mkdir($deepDir, 0777, true)) { | |
| throw new \RuntimeException("Failed to create directory: $deepDir"); | |
| } | |
| $deepFile = $deepDir . '/deep_file.txt'; | |
| if (false === \file_put_contents($deepFile, 'Deep nested file content')) { | |
| throw new \RuntimeException("Failed to create deep file: $deepFile"); | |
| } | |
| return $tempDir; | |
| } |
This PR implements a template-based prompt extension system for the MCP Server, which allows prompts to extend other prompts with variable substitution. This enables creating reusable prompt templates that can be customized for different use cases.
Features
templatethat can be used as a base for other prompts{{title}}and{{description}}in templatesExample
Backward Compatibility
promptif not specifiedAdditional notes
docs/example/config/prompt-templates.ymllSummary by CodeRabbit