Prompt eval changes #455
Prompt eval changes #455
Conversation
WalkthroughAdds a feature-flagged prompts processing step to the bundler: when BundleContext.PromptsEvalsFF is true, bundler parses prompts.yaml, generates JS/TS prompt artifacts into the SDK-generated directory before dependency installation. CLI wiring, prompt parsing/generation, tests, and supporting docs were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CLI (cmd/dev.go / cmd/bundle.go)
participant Bundler as Bundler (bundler.go)
participant Prompts as prompts.ProcessPrompts
participant SDK as SDK Generated Dir
participant NPM as Dependency Installer
User->>CLI: run dev/bundle
CLI->>Bundler: Bundle(BundleContext{PromptsEvalsFF: true/false})
alt PromptsEvalsFF enabled
Bundler->>Prompts: ProcessPrompts(logger, projectDir)
Prompts->>Prompts: FindPromptsYAML()
alt prompts.yaml found
Prompts->>Prompts: ParsePromptsYAML() -> []Prompt (with Templates)
Prompts->>Prompts: NewCodeGenerator -> Generate JS/TS artifacts
Prompts->>Prompts: FindSDKGeneratedDir()
Prompts->>SDK: write `_index.js`, `index.d.ts`
SDK-->>Prompts: OK
else no prompts.yaml
Prompts-->>Bundler: noop
end
Prompts-->>Bundler: result / error
else disabled
Note over Bundler: Skip prompts processing
end
Bundler->>NPM: install dependencies
NPM-->>Bundler: complete
Bundler-->>CLI: done
CLI-->>User: exit
sequenceDiagram
autonumber
participant Parser as YAML/Template Parser
participant Gen as CodeGenerator
participant FS as Filesystem
Parser->>Parser: ParsePromptsYAML(data) -> []Prompt (with Parsed Templates)
Parser-->>Gen: prompts list
Gen->>Gen: GenerateJavaScript()
Gen->>Gen: GenerateTypeScriptTypes()
Gen-->>FS: write `_index.js`
Gen-->>FS: write `index.d.ts`
FS-->>Gen: write results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
PROMPT_MIGRATION_PLAN.md (1)
338-382: Add language identifiers to fenced code blocks.The static analysis tool flagged several fenced code blocks missing language specifiers. Adding identifiers improves rendering and clarity.
Apply these language identifiers to the code blocks:
-``` +```text agentuity bundle ├── Parse agentuity.yamlSimilar changes needed for blocks at lines 349, 363, and 376.
internal/bundler/prompts.go (3)
36-59: Consider additional validation for prompt definitions.The current validation checks required fields but could be strengthened:
- Duplicate slugs: Multiple prompts with the same slug would cause issues in generated code with duplicate object keys.
- Slug format: Ensure slugs follow kebab-case convention for consistent camelCase conversion.
- Description field: While defined in the struct, it's not validated as required. Confirm whether it should be optional or required.
Example validation addition:
// After line 56, add duplicate slug detection slugsSeen := make(map[string]int) for i, prompt := range promptsData.Prompts { if prevIndex, exists := slugsSeen[prompt.Slug]; exists { return nil, fmt.Errorf("duplicate slug '%s' at indices %d and %d", prompt.Slug, prevIndex, i) } slugsSeen[prompt.Slug] = i }
79-121: Consider sorting variables for deterministic code generation.The variable extraction logic is correct, but the map-to-slice conversion in
GetAllVariables(lines 114-118) produces non-deterministic ordering. This could cause unnecessary diffs in generated files when variables are discovered in different orders.Add sorting for deterministic output:
// After line 118 sort.Strings(variables) return variablesDon't forget to import "sort" at the top of the file.
123-131: Add escaping for backticks to prevent template literal injection.The current escaping handles basic string literal escapes but misses backticks. If the generated code were to use template literals (backtick strings), unescaped backticks in the YAML content could break the generated syntax or enable injection.
Add backtick escaping:
func EscapeTemplateString(s string) string { s = strings.ReplaceAll(s, "\\", "\\\\") + s = strings.ReplaceAll(s, "`", "\\`") s = strings.ReplaceAll(s, "\"", "\\\"") s = strings.ReplaceAll(s, "\n", "\\n") s = strings.ReplaceAll(s, "\r", "\\r") s = strings.ReplaceAll(s, "\t", "\\t") return s }Note: Currently the generated code uses double-quoted strings (lines 243, 251, 313, 321), so this isn't an immediate vulnerability, but it's good defensive practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
PROMPT_MIGRATION_PLAN.md(1 hunks)internal/bundler/bundler.go(1 hunks)internal/bundler/importers.go(2 hunks)internal/bundler/importers_test.go(3 hunks)internal/bundler/prompts.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/bundler/bundler.go (1)
internal/bundler/prompts.go (1)
ProcessPrompts(375-419)
internal/bundler/prompts.go (1)
internal/bundler/bundler.go (1)
BundleContext(40-50)
🪛 markdownlint-cli2 (0.18.1)
PROMPT_MIGRATION_PLAN.md
338-338: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
349-349: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
363-363: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
376-376: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (macos-latest)
🔇 Additional comments (9)
internal/bundler/importers.go (1)
256-288: LGTM!The whitespace adjustments maintain code readability without altering functionality.
internal/bundler/bundler.go (1)
424-427: LGTM!The placement of
ProcessPromptsbefore dependency installation is correct, ensuring generated prompt artifacts are available during the build process. The error handling appropriately wraps and returns errors from prompt processing.internal/bundler/importers_test.go (2)
16-34: LGTM!Expanding the mockLogger with additional interface methods improves compatibility and test infrastructure robustness.
169-203: LGTM!The field alignment improves readability without changing test behavior.
internal/bundler/prompts.go (5)
13-31: LGTM!The data structures are well-designed for YAML parsing with appropriate field tags and optional field handling.
61-77: LGTM!The function handles multiple file locations and extensions appropriately, with graceful fallback when no prompts file exists.
133-148: LGTM!The camelCase conversion is correct. Input validation for kebab-case format would be handled by the earlier suggestion to validate slug format in
ParsePromptsYAML.
243-254: Template replacement regex matches extraction pattern.The generated runtime regex
/\{\{([^}]+)\}\}/gcorrectly matches the extraction pattern used in Go (line 33), ensuring consistency between variable detection and runtime substitution.
374-419: LGTM!The
ProcessPromptsorchestration is well-structured with appropriate logging, graceful handling when prompts.yaml is absent, and clear error wrapping throughout the flow.
- Make prompt field optional in validation (only require name, slug, and at least one of system/prompt) - Always generate both system and prompt fields in output (empty strings for missing ones) - Make variables parameter optional in both JS and TS generation - Update code generation rules to prevent optional chaining requirement - Fixes CLI-0006 error for prompts with only system or only prompt fields
- Extract variables separately for system and prompt fields - Generate different TypeScript types for each field based on actual variables used - System and prompt compile functions now have correct variable signatures - Fixes TypeScript errors where wrong variables were passed to compile functions
- TypeScript types now correctly show different variable types for system vs prompt - Each field only accepts the variables it actually uses - Fixes TypeScript compilation errors in agent code - Removes debug output
- TypeScript types now correctly separated for system vs prompt fields - All TypeScript compilation errors resolved - Agent code works with proper variable types
- Only include system field if prompt.System is not empty - Only include prompt field if prompt.Prompt is not empty - Applies to both TypeScript and JavaScript generation - System-only and prompt-only prompts now generate correctly - Fixes unused variable warnings
- Add @name, @description, @System, and @prompt tags - Include full prompt content in JSDoc for better IDE support - Apply to both TypeScript definitions and JavaScript implementation - Improves developer experience with better IntelliSense
- Keep only slug field in generated objects - Name and description are already in JSDoc comments - Reduces object size and eliminates duplication - Cleaner generated code with same functionality
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/bundler/prompts.go (3)
127-135: Consider escaping backticks.The function escapes most special characters but doesn't handle backticks (`). If prompts contain backticks and the generated code uses template literals, this could cause syntax errors.
func EscapeTemplateString(s string) string { s = strings.ReplaceAll(s, "\\", "\\\\") s = strings.ReplaceAll(s, "\"", "\\\"") + s = strings.ReplaceAll(s, "`", "\\`") s = strings.ReplaceAll(s, "\n", "\\n") s = strings.ReplaceAll(s, "\r", "\\r") s = strings.ReplaceAll(s, "\t", "\\t") return s }
232-233: Placeholder types exported asany.
PromptConfigandPromptNameare exported asany, which defeats type safety. Consider either implementing proper types or removing these exports if they're not yet needed.
28-31: Remove unusedVariableInfostruct
No references toVariableInfowere found outside its own declaration; delete the struct or implement its intended usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/rules/code-generation.mdc(1 hunks)internal/bundler/prompts.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .cursor/rules/code-generation.mdc
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/prompts.go (1)
internal/bundler/bundler.go (1)
BundleContext(40-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
internal/bundler/prompts.go (11)
1-11: LGTM!The package declaration and imports are appropriate for the prompt processing functionality.
33-33: LGTM!The regex pattern correctly matches
{{variable}}template placeholders.
35-63: LGTM!The YAML parsing and validation logic is thorough, with proper error handling and clear validation rules.
65-81: LGTM!The file discovery logic appropriately searches common locations and handles the case where no prompts file exists.
83-100: LGTM!The variable extraction correctly handles deduplication and whitespace trimming.
102-125: LGTM!The function correctly aggregates and deduplicates variables from both system and prompt fields.
137-152: Verify camelCase conversion for mixed-case inputs.The function converts subsequent parts to lowercase before capitalizing the first character. This means
"API-Key"becomes"apiKey"instead of preserving the internal casing. Ensure this is the intended behavior for your slug conventions.
262-264: Verify silent fallback for missing variables.When a variable is missing, the code returns the original
{{varName}}placeholder rather than throwing an error. Ensure this is the desired behavior, as it could hide bugs where variables are misspelled or not provided.
312-344: LGTM!The directory creation error handling has been improved based on previous feedback. Errors are now logged and the function continues searching other locations.
346-391: LGTM!The orchestration logic correctly handles optional prompts, generates both JavaScript and TypeScript definitions, and includes appropriate error handling and logging.
154-235: Verify inclusion ofevalsin the TypeScript generator
ThePromptstruct (internal/bundler/prompts.go) defines anEvals []stringfield parsed from YAML, butGenerateTypeScriptTypesdoesn’t expose it. Confirm whether evals should be added to the generated TypeScript definitions or removed if unused.
internal/bundler/prompts.go
Outdated
|
|
||
| // Generate index.js file (overwrite SDK's placeholder, following POC pattern) | ||
| jsContent := GenerateJavaScript(prompts) | ||
| jsPath := filepath.Join(sdkGeneratedDir, "_index.js") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify _index.js naming convention.
The JavaScript file is written as _index.js (with underscore prefix) while the TypeScript declaration is index.d.ts (without underscore). Ensure this naming difference is intentional and aligns with the SDK's module resolution expectations.
🏁 Script executed:
#!/bin/bash
# Check how the SDK imports these generated files
rg -n --type=ts --type=js 'generated.*index' node_modules/@agentuity/sdk/ 2>/dev/null || echo "SDK not found in node_modules, check SDK source code"Length of output: 52
🏁 Script executed:
#!/bin/bash
# Search for references to generated _index.js and index.d.ts across the repository
rg -n "_index\\.js" -C2 || echo "No occurrences of _index.js found"
rg -n "index\\.d\\.ts" -C2 || echo "No occurrences of index.d.ts found"Length of output: 2860
Align generated JS filename with comments and module resolution
In internal/bundler/prompts.go (lines 374–378), the code writes to _index.js but both the comment and error message refer to index.js. Either rename the file to index.js or update the comment and error text to reference _index.js so they’re consistent and the SDK placeholder is correctly overwritten.
🤖 Prompt for AI Agents
In internal/bundler/prompts.go around line 376, the code writes the SDK
placeholder to "_index.js" while surrounding comments and error messages
reference "index.js"; make these consistent by either renaming the target file
to "index.js" (update jsPath to filepath.Join(sdkGeneratedDir, "index.js")) or
by updating the comment and any error text to reference "_index.js" so the
filename mentioned in comments, errors, and the actual write target all match.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.cursor/rules/unit-testing.mdc(1 hunks)internal/bundler/bundler.go(2 hunks)internal/bundler/prompts/code_generator.go(1 hunks)internal/bundler/prompts/code_generator_test.go(1 hunks)internal/bundler/prompts/prompt_parser.go(1 hunks)internal/bundler/prompts/prompts.go(1 hunks)internal/bundler/prompts/template_parser.go(1 hunks)internal/bundler/prompts/template_parser_test.go(1 hunks)internal/bundler/prompts/types.go(1 hunks)internal/bundler/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/unit-testing.mdc
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bundler/bundler.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/bundler/prompts/types.go (1)
internal/bundler/prompts/template_parser.go (1)
Template(18-21)
internal/bundler/prompts/prompt_parser.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/template_parser.go (1)
ParseTemplate(105-165)
internal/bundler/types.go (1)
internal/mcp/config.go (1)
Install(265-333)
internal/bundler/prompts/template_parser.go (1)
internal/bundler/prompts/types.go (1)
Prompt(4-15)
internal/bundler/prompts/prompts.go (2)
internal/bundler/prompts/prompt_parser.go (1)
ParsePromptsYAML(15-45)internal/bundler/prompts/code_generator.go (1)
NewCodeGenerator(16-20)
internal/bundler/prompts/code_generator.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/template_parser.go (1)
ParseTemplate(105-165)
internal/bundler/prompts/code_generator_test.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/code_generator.go (1)
NewCodeGenerator(16-20)
internal/bundler/prompts/template_parser_test.go (2)
internal/bundler/prompts/template_parser.go (3)
ParseTemplate(105-165)ParsePrompt(213-221)ParseTemplateVariables(169-204)internal/bundler/prompts/types.go (1)
Prompt(4-15)
🪛 GitHub Actions: Go Build and Test
internal/bundler/prompts/code_generator.go
[error] 7-7: no required module provides package github.com/iancoleman/strcase; to add it: go get github.com/iancoleman/strcase (or run 'go mod tidy')
🪛 GitHub Actions: Upgrade Path Test
internal/bundler/prompts/code_generator.go
[error] 7-7: no required module provides package github.com/iancoleman/strcase; to add it:
🪛 GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204-arm)
internal/bundler/prompts/code_generator.go
[failure] 7-7:
no required module provides package github.com/iancoleman/strcase; to add it:
🪛 GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
internal/bundler/prompts/code_generator.go
[failure] 7-7:
no required module provides package github.com/iancoleman/strcase; to add it:
🪛 GitHub Check: Build and Test (macos-latest)
internal/bundler/prompts/code_generator.go
[failure] 7-7:
no required module provides package github.com/iancoleman/strcase; to add it:
🪛 GitHub Check: Test CLI Upgrade Path (macos-latest)
internal/bundler/prompts/code_generator.go
[failure] 7-7:
no required module provides package github.com/iancoleman/strcase; to add it:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
internal/bundler/types.go (1)
11-23: LGTM! Clean context type for bundling operations.The
BundleContextstruct provides a well-organized collection of execution state, configuration flags, and I/O interfaces for the bundling workflow. The newPromptsEvalsFFfield integrates cleanly with the existing fields.internal/bundler/prompts/template_parser_test.go (4)
10-123: Excellent test coverage for template parsing.The test suite thoroughly covers all syntax variations (legacy
{{variable}}, new{variable:default}, required{!variable}, mixed syntax) and edge cases (empty templates, duplicates). The assertions correctly validate both the parsed structure and metadata fields likeOriginalSyntax.
125-194: Well-structured tests for Template helper methods.The tests properly validate all filtering and extraction methods on the
Templatetype, using order-independent assertions where appropriate. The approach of testing all methods against a single complex template is efficient and ensures consistency.
196-212: LGTM!The test correctly validates that
ParsePromptextracts and parses templates from both theSystemandPromptfields.
214-244: Good coverage of variable extraction.The tests properly validate
ParseTemplateVariablesacross all syntax variants and correctly useElementsMatchfor order-independent assertions.internal/bundler/prompts/prompt_parser.go (2)
9-12: LGTM!Clean wrapper type for YAML unmarshaling with appropriate field tags.
14-45: Good validation logic with descriptive errors.The function properly validates required fields and provides clear error messages with indices. Template enrichment is correctly applied only to non-empty fields.
Minor consideration: The validation accepts whitespace-only strings for
SystemandPrompt. If this is intentional (allowing whitespace-only templates), the current implementation is fine. Otherwise, consider addingstrings.TrimSpacechecks.Do you want to validate that
SystemandPromptfields contain non-whitespace content, or is whitespace-only acceptable for templates?internal/bundler/prompts/types.go (1)
3-15: Well-designed Prompt type with appropriate tags.The struct cleanly separates YAML input fields from JSON-serialized parsed template data. The use of
omitemptyon optional fields is appropriate. Validation of required fields is correctly handled inParsePromptsYAMLrather than at the type level.internal/bundler/prompts/code_generator_test.go (5)
9-94: Comprehensive test coverage for code generation.The test thoroughly validates JavaScript and TypeScript output generation, including proper import statements, object structure, compile functions, and variable type definitions. The negative assertions ensuring no TypeScript syntax leaks into JavaScript output are particularly valuable.
96-121: Good edge case coverage for empty prompts.The test correctly validates that the code generator handles empty input gracefully, producing valid but empty JavaScript/TypeScript artifacts.
123-161: LGTM!The test properly validates handling of prompts with only a system template or only a prompt template, ensuring the generator handles optional fields correctly.
163-214: Excellent coverage of complex templates.The test validates proper handling of multiline templates with escaped newlines and multiple variables, ensuring the generator produces correct output for realistic use cases.
216-236: LGTM!The test validates that all variable syntax types (legacy, new with default, required) are correctly extracted and typed in the generated TypeScript output.
| // getAllVariables gets all unique variables from both system and prompt templates | ||
| func (cg *CodeGenerator) getAllVariables(prompt Prompt) []string { | ||
| allVars := make(map[string]bool) | ||
|
|
||
| // Parse system template if not already parsed | ||
| systemTemplate := prompt.SystemTemplate | ||
| if len(systemTemplate.Variables) == 0 && prompt.System != "" { | ||
| systemTemplate = ParseTemplate(prompt.System) | ||
| } | ||
|
|
||
| // Parse prompt template if not already parsed | ||
| promptTemplate := prompt.PromptTemplate | ||
| if len(promptTemplate.Variables) == 0 && prompt.Prompt != "" { | ||
| promptTemplate = ParseTemplate(prompt.Prompt) | ||
| } | ||
|
|
||
| // Add variables from system template | ||
| for _, variable := range systemTemplate.VariableNames() { | ||
| allVars[variable] = true | ||
| } | ||
|
|
||
| // Add variables from prompt template | ||
| for _, variable := range promptTemplate.VariableNames() { | ||
| allVars[variable] = true | ||
| } | ||
|
|
||
| // Convert map to slice | ||
| var variables []string | ||
| for variable := range allVars { | ||
| variables = append(variables, variable) | ||
| } | ||
|
|
||
| return variables | ||
| } |
There was a problem hiding this comment.
Stabilize variable ordering in generated artifacts.
getAllVariables feeds a Go map straight into the generated JS/TS. Map iteration order is randomized, so successive runs emit different signatures and object literals, which will churn SDK outputs and break deterministic builds. Collect the keys into a slice and sort.Strings before returning (remember to add the sort import).
-import (
- "fmt"
- "strings"
+import (
+ "fmt"
+ "sort"
+ "strings"
)
@@
- var variables []string
- for variable := range allVars {
- variables = append(variables, variable)
- }
-
- return variables
+ variables := make([]string, 0, len(allVars))
+ for variable := range allVars {
+ variables = append(variables, variable)
+ }
+ sort.Strings(variables)
+
+ return variables📝 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.
| // getAllVariables gets all unique variables from both system and prompt templates | |
| func (cg *CodeGenerator) getAllVariables(prompt Prompt) []string { | |
| allVars := make(map[string]bool) | |
| // Parse system template if not already parsed | |
| systemTemplate := prompt.SystemTemplate | |
| if len(systemTemplate.Variables) == 0 && prompt.System != "" { | |
| systemTemplate = ParseTemplate(prompt.System) | |
| } | |
| // Parse prompt template if not already parsed | |
| promptTemplate := prompt.PromptTemplate | |
| if len(promptTemplate.Variables) == 0 && prompt.Prompt != "" { | |
| promptTemplate = ParseTemplate(prompt.Prompt) | |
| } | |
| // Add variables from system template | |
| for _, variable := range systemTemplate.VariableNames() { | |
| allVars[variable] = true | |
| } | |
| // Add variables from prompt template | |
| for _, variable := range promptTemplate.VariableNames() { | |
| allVars[variable] = true | |
| } | |
| // Convert map to slice | |
| var variables []string | |
| for variable := range allVars { | |
| variables = append(variables, variable) | |
| } | |
| return variables | |
| } | |
| // internal/bundler/prompts/code_generator.go | |
| import ( | |
| "fmt" | |
| "sort" | |
| "strings" | |
| ) | |
| // getAllVariables gets all unique variables from both system and prompt templates | |
| func (cg *CodeGenerator) getAllVariables(prompt Prompt) []string { | |
| allVars := make(map[string]bool) | |
| // Parse system template if not already parsed | |
| systemTemplate := prompt.SystemTemplate | |
| if len(systemTemplate.Variables) == 0 && prompt.System != "" { | |
| systemTemplate = ParseTemplate(prompt.System) | |
| } | |
| // Parse prompt template if not already parsed | |
| promptTemplate := prompt.PromptTemplate | |
| if len(promptTemplate.Variables) == 0 && prompt.Prompt != "" { | |
| promptTemplate = ParseTemplate(prompt.Prompt) | |
| } | |
| // Add variables from system template | |
| for _, variable := range systemTemplate.VariableNames() { | |
| allVars[variable] = true | |
| } | |
| // Add variables from prompt template | |
| for _, variable := range promptTemplate.VariableNames() { | |
| allVars[variable] = true | |
| } | |
| // Convert map to slice | |
| variables := make([]string, 0, len(allVars)) | |
| for variable := range allVars { | |
| variables = append(variables, variable) | |
| } | |
| // Stabilize iteration order for deterministic output | |
| sort.Strings(variables) | |
| return variables | |
| } |
🤖 Prompt for AI Agents
internal/bundler/prompts/code_generator.go around lines 188 to 221:
getAllVariables builds a map of variable names then returns them by iterating
the map, which yields non-deterministic ordering; collect the map keys into a
slice, call sort.Strings on that slice before returning to stabilize ordering of
generated artifacts, and add the "sort" import to the file.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/bundler/prompts/code_generator.go (2)
7-8: Verify that the strcase dependency was added to go.mod.A previous review flagged that
github.com/iancoleman/strcasewas imported but not added togo.mod, causing CI failures. Please confirm thatgo get github.com/iancoleman/strcaseorgo mod tidyhas been run and thatgo.modandgo.sumhave been updated accordingly.
111-118: Fix compile() to conditionally pass variables parameter.When a prompt has no variables,
generatePromptObjectgenerates a compile function with signature() => ..., butgenerateTemplateValuealways emitsinterpolateTemplate(..., variables), resulting inReferenceError: variables is not definedat runtime. The function should only passvariableswhenallVariablesis non-empty.Apply this diff:
func (cg *CodeGenerator) generateTemplateValue(template string, allVariables []string) string { - if template == "" { - return "interpolateTemplate('', variables)" - } - - return fmt.Sprintf("interpolateTemplate(%q, variables)", template) + if len(allVariables) == 0 { + return fmt.Sprintf("interpolateTemplate(%q)", template) + } + + return fmt.Sprintf("interpolateTemplate(%q, variables)", template) }
🧹 Nitpick comments (4)
internal/bundler/prompts/template_parser.go (2)
136-140: Remove redundant colon-removal logic.Lines 138-140 attempt to remove a
:defaultsuffix fromvarName, butmatch[2](the source ofvarName) already excludes the colon and default value—those are captured separately inmatch[3]. This code will never find a colon invarNameand can be safely removed.Apply this diff:
if isRequired { varName = varName[1:] // Remove ! prefix } - if hasDefault { - // Remove :default suffix - if idx := strings.Index(varName, ":"); idx != -1 { - varName = varName[:idx] - } + if hasDefault { // Handle :- syntax for required variables with defaults if strings.HasPrefix(defaultValue, "-") { defaultValue = defaultValue[1:] // Remove leading dash } }
191-194: Remove redundant colon-removal logic.Lines 192-194 attempt to remove a
:defaultsuffix fromvarName, but as with the similar code inParseTemplate,match[2]already excludes the colon and default value. This conditional will never match and can be removed.Apply this diff:
if strings.HasPrefix(varName, "!") { varName = varName[1:] } - // Remove :default suffix if present - if idx := strings.Index(varName, ":"); idx != -1 { - varName = varName[:idx] - } if !seen[varName] { variables = append(variables, varName) seen[varName] = trueinternal/bundler/prompts/code_generator.go (2)
120-127: Remove unused function.
generateSystemCompileis never called—generatePromptObjectusesgenerateTemplateValuefor both system and prompt templates. Remove this dead code to reduce maintenance burden.
505-533: Remove unused function.
wrapLineis never called in the codebase. Remove this dead code to reduce maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.cursor/rules/prompt-docstrings.mdc(1 hunks)go.mod(1 hunks)internal/bundler/prompts/code_generator.go(1 hunks)internal/bundler/prompts/template_parser.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/bundler/prompts/code_generator.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/template_parser.go (2)
Variable(9-15)ParseTemplate(105-165)
internal/bundler/prompts/template_parser.go (1)
internal/bundler/prompts/types.go (1)
Prompt(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
| ## Implementation | ||
|
|
||
| ### Code Generator Updates | ||
| 1. **PromptsCollection JSDoc**: Add JSDoc comments to each property in the `PromptsCollection` type | ||
| 2. **Template Escaping**: Handle JSDoc comment characters in templates | ||
| 3. **Line Break Handling**: Split templates by newlines and add proper JSDoc formatting | ||
| 4. **Prompt-Only Focus**: Only include `@prompt` section, not `@system` | ||
|
|
||
| ### File Structure | ||
| - **`_index.js`**: Contains actual prompt objects (no JSDoc on individual objects) | ||
| - **`index.d.ts`**: Contains TypeScript types with JSDoc comments on `PromptsCollection` properties | ||
|
|
There was a problem hiding this comment.
Inconsistency between documentation and implementation.
Line 46 states "Only include @prompt section, not @system", but the actual implementation in internal/bundler/prompts/code_generator.go (lines 315-337 in generatePromptPropertyJSDoc) includes both @system and @prompt sections. Either update this documentation to reflect the actual behavior or adjust the code generator to match the documented intent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/bundler/prompts/code_generator.go (1)
111-118: Fix generateTemplateValue to omit variables argument when no variables exist.The current implementation always passes
variablesas the second argument tointerpolateTemplate, even whenallVariablesis empty. This causes aReferenceError: variables is not definedat runtime when the compile function has novariablesparameter.Apply this diff to fix the issue:
-func (cg *CodeGenerator) generateTemplateValue(template string, allVariables []string) string { - if template == "" { - return "interpolateTemplate('', variables)" - } - - return fmt.Sprintf("interpolateTemplate(%q, variables)", template) +func (cg *CodeGenerator) generateTemplateValue(template string, allVariables []string) string { + if len(allVariables) == 0 { + if template == "" { + return "interpolateTemplate('')" + } + return fmt.Sprintf("interpolateTemplate(%q)", template) + } + + if template == "" { + return "interpolateTemplate('', variables)" + } + return fmt.Sprintf("interpolateTemplate(%q, variables)", template) }
🧹 Nitpick comments (2)
internal/bundler/prompts/code_generator.go (2)
120-127: Remove unused generateSystemCompile function.The
generateSystemCompilefunction is not referenced anywhere in the codebase. Additionally, it has the same issue asgenerateTemplateValue: it always includesvariablesas a parameter regardless of whether variables exist.Apply this diff to remove the unused function:
-// generateSystemCompile generates the system compile function body -func (cg *CodeGenerator) generateSystemCompile(template string, allVariables []string) string { - if template == "" { - return "interpolateTemplate('', variables);" - } - - return fmt.Sprintf("interpolateTemplate(%q, variables);", template) -} -
509-537: Remove unused wrapLine function.The
wrapLinefunction is not referenced anywhere in the codebase. Consider removing it to reduce maintenance burden.Apply this diff to remove the unused function:
-// wrapLine wraps a long line at the specified width -func (cg *CodeGenerator) wrapLine(line string, width int) []string { - if len(line) <= width { - return []string{line} - } - - var wrapped []string - words := strings.Fields(line) - var currentLine strings.Builder - - for _, word := range words { - // If adding this word would exceed the width, start a new line - if currentLine.Len() > 0 && currentLine.Len()+len(word)+1 > width { - wrapped = append(wrapped, currentLine.String()) - currentLine.Reset() - } - - if currentLine.Len() > 0 { - currentLine.WriteString(" ") - } - currentLine.WriteString(word) - } - - if currentLine.Len() > 0 { - wrapped = append(wrapped, currentLine.String()) - } - - return wrapped -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/rules/testing.mdc(1 hunks)internal/bundler/prompts/code_generator.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/testing.mdc
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/prompts/code_generator.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/template_parser.go (2)
Variable(9-15)ParseTemplate(105-165)
🔇 Additional comments (11)
internal/bundler/prompts/code_generator.go (11)
10-20: LGTM!The
CodeGeneratorstruct and constructor are straightforward and correct.
22-42: LGTM!The JavaScript generation logic is clear and correctly structures the output with imports and the prompts collection.
44-63: LGTM!The TypeScript type generation logic is clear and correctly structures the output with type definitions and the PromptsCollection type.
65-76: LGTM!The TypeScript interface generation logic is clear and correctly structures the output.
78-109: LGTM!The prompt object generation correctly includes the
variablesparameter in compile functions only when variables are present, avoiding undefined identifier issues.
386-428: LGTM!The variable extraction methods correctly parse templates and return variables in a deterministic order (based on the order of regex matches in the template).
129-171: LGTM!The prompt type generation correctly constructs TypeScript types with optional variables and compile function signatures.
173-198: LGTM!The prompt interface generation correctly constructs TypeScript interfaces with compile function signatures.
200-225: LGTM!The variable type generation correctly handles required, optional, and default-valued variables for TypeScript types.
227-384: LGTM!The docstring generation functions correctly handle multi-line templates, escape JSDoc closing sequences, and format output appropriately.
430-467: LGTM!The type docstring generation correctly formats templates with JSDoc comments and memberof annotations.
| // addNaturalLineBreaks adds line breaks at natural break points | ||
| func (cg *CodeGenerator) addNaturalLineBreaks(line string) string { | ||
| // If line is short enough, return as is | ||
| if len(line) <= 60 { | ||
| return line | ||
| } | ||
|
|
||
| // Look for natural break points: periods, commas, or spaces | ||
| // Split at periods followed by space | ||
| parts := strings.Split(line, ". ") | ||
| if len(parts) > 1 { | ||
| var result []string | ||
| for i, part := range parts { | ||
| if i > 0 { | ||
| part = part + "." | ||
| } | ||
| if len(part) > 60 { | ||
| // Further split long parts at commas | ||
| commaParts := strings.Split(part, ", ") | ||
| if len(commaParts) > 1 { | ||
| for j, commaPart := range commaParts { | ||
| if j > 0 { | ||
| commaPart = commaPart + "," | ||
| } | ||
| result = append(result, commaPart) | ||
| } | ||
| } else { | ||
| result = append(result, part) | ||
| } | ||
| } else { | ||
| result = append(result, part) | ||
| } | ||
| } | ||
| // Use HTML line breaks instead of newlines | ||
| return strings.Join(result, ".<br/> * ") | ||
| } | ||
|
|
||
| return line | ||
| } |
There was a problem hiding this comment.
Fix double-period bug in addNaturalLineBreaks.
When splitting by ". ", the last part retains its trailing period (if the input ends with a period). Line 483 unconditionally adds another period to all parts except the first, causing a double period on the last segment.
Apply this diff to fix the issue:
parts := strings.Split(line, ". ")
if len(parts) > 1 {
var result []string
for i, part := range parts {
- if i > 0 {
+ // Add period back after split, but only if part doesn't already end with one
+ if i > 0 && !strings.HasSuffix(part, ".") {
part = part + "."
}📝 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.
| // addNaturalLineBreaks adds line breaks at natural break points | |
| func (cg *CodeGenerator) addNaturalLineBreaks(line string) string { | |
| // If line is short enough, return as is | |
| if len(line) <= 60 { | |
| return line | |
| } | |
| // Look for natural break points: periods, commas, or spaces | |
| // Split at periods followed by space | |
| parts := strings.Split(line, ". ") | |
| if len(parts) > 1 { | |
| var result []string | |
| for i, part := range parts { | |
| if i > 0 { | |
| part = part + "." | |
| } | |
| if len(part) > 60 { | |
| // Further split long parts at commas | |
| commaParts := strings.Split(part, ", ") | |
| if len(commaParts) > 1 { | |
| for j, commaPart := range commaParts { | |
| if j > 0 { | |
| commaPart = commaPart + "," | |
| } | |
| result = append(result, commaPart) | |
| } | |
| } else { | |
| result = append(result, part) | |
| } | |
| } else { | |
| result = append(result, part) | |
| } | |
| } | |
| // Use HTML line breaks instead of newlines | |
| return strings.Join(result, ".<br/> * ") | |
| } | |
| return line | |
| } | |
| // addNaturalLineBreaks adds line breaks at natural break points | |
| func (cg *CodeGenerator) addNaturalLineBreaks(line string) string { | |
| // If line is short enough, return as is | |
| if len(line) <= 60 { | |
| return line | |
| } | |
| // Look for natural break points: periods, commas, or spaces | |
| // Split at periods followed by space | |
| parts := strings.Split(line, ". ") | |
| if len(parts) > 1 { | |
| var result []string | |
| for i, part := range parts { | |
| // Add period back after split, but only if part doesn't already end with one | |
| if i > 0 && !strings.HasSuffix(part, ".") { | |
| part = part + "." | |
| } | |
| if len(part) > 60 { | |
| // Further split long parts at commas | |
| commaParts := strings.Split(part, ", ") | |
| if len(commaParts) > 1 { | |
| for j, commaPart := range commaParts { | |
| if j > 0 { | |
| commaPart = commaPart + "," | |
| } | |
| result = append(result, commaPart) | |
| } | |
| } else { | |
| result = append(result, part) | |
| } | |
| } else { | |
| result = append(result, part) | |
| } | |
| } | |
| // Use HTML line breaks instead of newlines | |
| return strings.Join(result, ".<br/> * ") | |
| } | |
| return line | |
| } |
🤖 Prompt for AI Agents
internal/bundler/prompts/code_generator.go lines 469-507: the function
addNaturalLineBreaks appends a "." to every part for i>0 after splitting on ".
", which causes a double period when the last part already ends with a period;
change the logic so you only append a period when the part does not already end
with one (e.g., check strings.HasSuffix(part, ".") before adding), leaving
existing trailing periods intact and preventing the double-period bug.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/bundler/prompts/code_generator.go (2)
111-118: Critical:variablesis undefined when prompts have no variables.Although
generatePromptObjectcorrectly omits thevariablesparameter from the function signature when there are no variables,generateTemplateValuestill unconditionally emitsinterpolateTemplate(..., variables). This causes aReferenceErrorat runtime for zero-variable prompts.The previous review suggested passing
allVariablestogenerateTemplateValueso it can conditionally include the second argument, but that parameter is missing from the current signature.Apply this diff to fix the issue:
// generateTemplateValue generates the value for a template (either compile function or direct interpolateTemplate call) -func (cg *CodeGenerator) generateTemplateValue(template string) string { +func (cg *CodeGenerator) generateTemplateValue(template string, hasVariables bool) string { if template == "" { return `""` } - return fmt.Sprintf("interpolateTemplate(%q, variables)", template) + if hasVariables { + return fmt.Sprintf("interpolateTemplate(%q, variables)", template) + } + return fmt.Sprintf("interpolateTemplate(%q)", template) }Then update the call sites in
generatePromptObject:system: { compile: (%s) => { - return %s + return %s } }, prompt: { compile: (%s) => { - return %s + return %s } } -};`, strcase.ToLowerCamel(prompt.Slug), prompt.Slug, systemParamStr, cg.generateTemplateValue(prompt.System), promptParamStr, cg.generateTemplateValue(prompt.Prompt)) +};`, strcase.ToLowerCamel(prompt.Slug), prompt.Slug, systemParamStr, cg.generateTemplateValue(prompt.System, len(systemVariables) > 0), promptParamStr, cg.generateTemplateValue(prompt.Prompt, len(promptVariables) > 0))
460-498: Fix the double-period and double-comma bugs inaddNaturalLineBreaks.When splitting by
". ", the last part retains its trailing period. Lines 473-474 unconditionally append another period to all parts except the first, causingBaz.to becomeBaz... The same issue exists for comma splitting at lines 481-482.Apply this diff to fix both issues:
parts := strings.Split(line, ". ") if len(parts) > 1 { var result []string for i, part := range parts { - if i > 0 { + // Add period back after split, but only if part doesn't already end with one + if i > 0 && !strings.HasSuffix(part, ".") { part = part + "." } if len(part) > 60 { // Further split long parts at commas commaParts := strings.Split(part, ", ") if len(commaParts) > 1 { for j, commaPart := range commaParts { - if j > 0 { + // Add comma back after split, but only if part doesn't already end with one + if j > 0 && !strings.HasSuffix(commaPart, ",") { commaPart = commaPart + "," } result = append(result, commaPart)
🧹 Nitpick comments (2)
internal/bundler/prompts/template_parser.go (2)
140-144: Remove redundant code that checks for:in variable name.The regex capture group
([!]?[^}:]+)explicitly excludes:characters, sovarNamewill never contain a:at this point. Lines 142-144 are unreachable dead code.Apply this diff to remove the dead code:
if hasDefault { - // Remove :default suffix - if idx := strings.Index(varName, ":"); idx != -1 { - varName = varName[:idx] - } // Handle :- syntax for required variables with defaults if len(defaultValue) > 0 && strings.HasPrefix(defaultValue, "-") { defaultValue = defaultValue[1:] // Remove leading dash } }
194-197: Remove redundant code that checks for:in variable name.Similar to the issue in
ParseTemplate, the regex ensuresvarNamewon't contain:at this point, making lines 195-196 unreachable.Apply this diff:
// Remove ! prefix if present if strings.HasPrefix(varName, "!") { varName = varName[1:] } - // Remove :default suffix if present - if idx := strings.Index(varName, ":"); idx != -1 { - varName = varName[:idx] - } if !seen[varName] { variables = append(variables, varName) seen[varName] = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bundler/prompts/code_generator.go(1 hunks)internal/bundler/prompts/template_parser.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/bundler/prompts/template_parser.go (1)
internal/bundler/prompts/types.go (1)
Prompt(4-15)
internal/bundler/prompts/code_generator.go (2)
internal/bundler/prompts/types.go (1)
Prompt(4-15)internal/bundler/prompts/template_parser.go (2)
Variable(9-15)ParseTemplate(105-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
internal/bundler/prompts/template_parser.go (5)
1-6: LGTM!Package declaration and imports are appropriate for the template parsing functionality.
8-15: LGTM!The
Variablestruct appropriately captures all necessary metadata for template variables, including required/optional status and default values.
17-96: LGTM!The
Templatestruct and its helper methods provide a clean API for accessing filtered subsets of variables. The implementations are correct and follow consistent patterns.
98-101: LGTM!The regex pattern correctly handles both legacy
{{variable}}and new{variable:default}syntax, including required variables marked with!.
215-224: LGTM!
ParsePromptcorrectly delegates toParseTemplatefor both system and prompt templates and composes the results.internal/bundler/prompts/code_generator.go (9)
3-8: LGTM!Imports are appropriate. The
strcasedependency issue mentioned in past review comments has been addressed.
10-20: LGTM!The
CodeGeneratorstruct and constructor are clean and straightforward.
22-76: LGTM!The three main generation methods (
GenerateJavaScript,GenerateTypeScriptTypes,GenerateTypeScriptInterfaces) follow a consistent pattern and correctly compose the output from helper functions.
78-109: LGTM!The function correctly handles cases with and without variables by checking
len(systemVariables)andlen(promptVariables)before adding thevariablesparameter. This fixes the previous issue where zero-variable prompts would reference an undefinedvariablesidentifier.
120-216: LGTM!Type and interface generation methods correctly handle variable metadata, including required/optional flags and default values. The TypeScript types accurately reflect the JavaScript runtime behavior.
218-375: LGTM!JSDoc generation correctly escapes template content and properly formats multi-line templates with appropriate indentation.
377-419: LGTM!Variable getter methods correctly handle both pre-parsed and unparsed templates, ensuring the Template data is always available.
421-458: LGTM!Template docstring generation correctly formats templates into JSDoc comments with proper escaping and line breaks. However, see the separate comment about the
addNaturalLineBreaksissue.
500-528: LGTM!The
wrapLinefunction implements a standard word-wrapping algorithm correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/bundler/prompts/template_parser_test.go (2)
10-123: LGTM! Comprehensive test coverage for template parsing.The test suite thoroughly covers:
- Empty templates and templates without variables
- Legacy
{{variable}}syntax- New syntax variants: optional with defaults, required, and required with defaults
- Mixed syntax scenarios
- Duplicate variable deduplication
The assertions correctly validate all Variable fields including Name, IsRequired, HasDefault, DefaultValue, and OriginalSyntax.
Optional: Consider adding edge case tests.
While the current coverage is solid, consider adding tests for:
- Whitespace handling in variable names:
{ role }vs{role}- Special characters in variable names:
{user-id},{user_name}- Malformed syntax:
{role,role},{{role}, nested braces- Empty variable names:
{:default},{!}- Very long default values
- Default values containing special characters like
:,},{These edge cases would help ensure robust parsing behavior and clearer error boundaries.
125-194: LGTM! Good coverage of Template helper methods.The tests validate all 7 helper methods (RequiredVariables, OptionalVariables, VariablesWithDefaults, VariablesWithoutDefaults, VariableNames, RequiredVariableNames, OptionalVariableNames) using appropriate assertions. Using maps for set membership checks is the right approach when order doesn't matter.
Optional: Add boundary test cases for helper methods.
Consider testing helper methods with edge case templates:
- Template with no variables
- Template with only required variables
- Template with only optional variables
- Template where all variables have defaults
- Template where no variables have defaults
This would ensure the helper methods handle edge cases correctly and return empty slices appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)internal/bundler/prompts/template_parser.go(1 hunks)internal/bundler/prompts/template_parser_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/bundler/prompts/template_parser.go
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/prompts/template_parser_test.go (1)
internal/bundler/prompts/template_parser.go (1)
ParseTemplate(105-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
Summary by CodeRabbit
New Features
Tests
Documentation