Add conditional template guards and fix grammar/spelling in prompt#515
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTemplate rewritten to render many sections conditionally (company, tools, disabled tools, custom instructions, user/team fields); wording tightened (capabilities, tool guidance, politeness), typos/punctuation fixed, and a new whitespace/formatting test added to validate output across context/tool permutations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness and correctness of the standard_personality_without_locale.tmpl prompt template by adding conditional guards for optional values and fixing grammar/spelling issues. The changes aim to prevent errors when optional data is missing and to improve LLM performance by correcting typos, as research shows typos degrade LLM output quality.
Changes:
- Added conditional rendering for optional fields (CompanyName, WebSearch tool, user names) to prevent empty spaces or errors
- Fixed spelling error ("responces" → "responses") and improved punctuation/formatting
- Cleaned up trailing blank lines for consistency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prompts/standard_personality_without_locale.tmpl`:
- Line 14: Fix the typo in the template string: change "adapt is responses" to
"adapt its responses" in the prompts/standard_personality_without_locale.tmpl
where the line referencing {{.BotName}} currently reads "will adapt is responses
to fit the conversation topic."; update that literal text to use "its" so the
sentence reads "{{.BotName}} will adapt its responses to fit the conversation
topic."
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I also just realized the prompt leaves multiple blank lines in a row as well. We can clean this up with the |
|
I spent a little too long writing a test. Let me know if it isn't wanted and I'll remove it. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
prompts/standard_personality_without_locale_test.go (1)
65-94: Uset.Run()subtests for failure isolation across 1152 combinations.With
require(which callst.FailNow()), a single failing combination aborts the entire test, masking any other failures. Wrapping each combination int.Run(label, func(t *testing.T) { ... })lets all 1152 cases run independently and surfaces them clearly in CI output.♻️ Proposed refactor
label := fmt.Sprintf("tools=%s channel=%s flags=%0*b", toolMode.name, channelMode.name, len(mutators), flags) - output, err := promptsEngine.Format(prompts.PromptStandardPersonalityWithoutLocale, context) - require.NoError(t, err, label) - require.Falsef(t, horizontalWhitespaceRun.MatchString(output), "%s contains repeated horizontal whitespace", label) - require.Falsef(t, newlineRun.MatchString(output), "%s contains repeated newline runs", label) + t.Run(label, func(t *testing.T) { + output, err := promptsEngine.Format(prompts.PromptStandardPersonalityWithoutLocale, context) + require.NoError(t, err) + require.Falsef(t, horizontalWhitespaceRun.MatchString(output), "%s contains repeated horizontal whitespace", label) + require.Falsef(t, newlineRun.MatchString(output), "%s contains repeated newline runs", label) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/standard_personality_without_locale_test.go` around lines 65 - 94, The test iterates 1,152 combinations but uses require (which calls t.FailNow()) so one failure aborts all; wrap the body that calls promptsEngine.Format (using PromptStandardPersonalityWithoutLocale), the mutators loop, and the require assertions in t.Run(label, func(t *testing.T) { ... }) to create a subtest per combination, and ensure loop variables are correctly captured (e.g., capture label and flags into local variables before calling t.Run) so each subtest runs independently and reports failures separately; keep the same assertions (require.NoError, require.Falsef with horizontalWhitespaceRun and newlineRun) inside the subtest.prompts/standard_personality_without_locale.tmpl (1)
44-45: Prefer{{if .Channel}}overne .Channel nilfor nil-pointer guards.
ne .Channel nilis functionally correct (Go'sandshort-circuits left-to-right, so.Channel.Typeis never accessed when.Channelis nil), but the idiomatic Go template pattern for a nil pointer check is simply{{if .Channel}}, since nil pointers evaluate as falsy in template conditions. Apply the same tone .Team nil.♻️ Proposed simplification
-{{- if and (ne .Channel nil) (ne .Channel.Type "D")}} -The channel {{.BotName}} is responding in has the name '{{.Channel.Name}}' and display name '{{.Channel.DisplayName}}'.{{if (ne .Team nil)}} The channel is on a team called '{{.Team.Name}}' with display name '{{.Team.DisplayName}}'.{{end}} +{{- if and .Channel (ne .Channel.Type "D")}} +The channel {{.BotName}} is responding in has the name '{{.Channel.Name}}' and display name '{{.Channel.DisplayName}}'.{{if .Team}} The channel is on a team called '{{.Team.Name}}' with display name '{{.Team.DisplayName}}'.{{end}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/standard_personality_without_locale.tmpl` around lines 44 - 45, The template uses explicit nil checks with `ne .Channel nil` and `ne .Team nil`; replace them with idiomatic truthy checks by using `{{if and .Channel (ne .Channel.Type "D")}}` for the channel guard and change `{{if (ne .Team nil)}}` to `{{if .Team}}`, keeping the same logic that prevents accessing `.Channel.Type` or `.Team` when nil and preserving the rest of the string interpolation (references: .Channel, .Channel.Type, .Team, .BotName, .Channel.Name, .Channel.DisplayName, .Team.Name, .Team.DisplayName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@prompts/standard_personality_without_locale.tmpl`:
- Around line 14-16: The template change correctly enforces that {{.BotName}}
adapt to conversation topic and must not begin responses with positive
affirmations; approve and merge this update as-is, ensuring the lines containing
"{{.BotName}} will adapt its responses to fit the conversation topic." and
"{{.BotName}} will not start its responses by saying that the request, question,
idea, or command was good, or was a good question, excellent, or any other
positive affirmation." remain intact in
prompts/standard_personality_without_locale.tmpl.
---
Nitpick comments:
In `@prompts/standard_personality_without_locale_test.go`:
- Around line 65-94: The test iterates 1,152 combinations but uses require
(which calls t.FailNow()) so one failure aborts all; wrap the body that calls
promptsEngine.Format (using PromptStandardPersonalityWithoutLocale), the
mutators loop, and the require assertions in t.Run(label, func(t *testing.T) {
... }) to create a subtest per combination, and ensure loop variables are
correctly captured (e.g., capture label and flags into local variables before
calling t.Run) so each subtest runs independently and reports failures
separately; keep the same assertions (require.NoError, require.Falsef with
horizontalWhitespaceRun and newlineRun) inside the subtest.
In `@prompts/standard_personality_without_locale.tmpl`:
- Around line 44-45: The template uses explicit nil checks with `ne .Channel
nil` and `ne .Team nil`; replace them with idiomatic truthy checks by using
`{{if and .Channel (ne .Channel.Type "D")}}` for the channel guard and change
`{{if (ne .Team nil)}}` to `{{if .Team}}`, keeping the same logic that prevents
accessing `.Channel.Type` or `.Team` when nil and preserving the rest of the
string interpolation (references: .Channel, .Channel.Type, .Team, .BotName,
.Channel.Name, .Channel.DisplayName, .Team.Name, .Team.DisplayName).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prompts/standard_personality_without_locale.tmpl (1)
44-45: Correct proposal reference and preserve consistency suggestion.The proposal number should be #31107 (not
#31103) — "make and/or operators short-circuit evaluation" — which shipped in Go 1.18. Line 8 uses the nested-if pattern for consistency; line 44 should follow the same style:♻️ Suggested refactor for consistency with line 8's pattern
-{{- if and .Channel (ne .Channel.Type "D")}} +{{- if .Channel}}{{if ne .Channel.Type "D"}} The channel {{.BotName}} is responding in has the name '{{.Channel.Name}}' and display name '{{.Channel.DisplayName}}'.{{if .Team}} The channel is on a team called '{{.Team.Name}}' with display name '{{.Team.DisplayName}}'.{{end}} -{{- end}} +{{- end}}{{end}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/standard_personality_without_locale.tmpl` around lines 44 - 45, Update the proposal number from `#31103` to `#31107` and refactor the conditional block that starts with "{{- if and .Channel (ne .Channel.Type \"D\")}}" to use the nested-if pattern used elsewhere (i.e., first check .Channel, output the channel name/display name, then inside a separate "{{if .Team}}" block add the team sentence) so the template matches the nested-if consistency and cites proposal "#31107".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@prompts/standard_personality_without_locale.tmpl`:
- Line 14: The line "{{.BotName}} will adapt its responses to fit the
conversation topic." in prompts/standard_personality_without_locale.tmpl is
already corrected but the review notes a duplicate comment; remove the redundant
duplicate comment marker and ensure there is a single occurrence of that
corrected template line (search for the exact string "{{.BotName}} will adapt
its responses to fit the conversation topic." and delete any duplicate instances
or duplicate review markers), keeping only the intended updated line.
---
Nitpick comments:
In `@prompts/standard_personality_without_locale.tmpl`:
- Around line 44-45: Update the proposal number from `#31103` to `#31107` and
refactor the conditional block that starts with "{{- if and .Channel (ne
.Channel.Type \"D\")}}" to use the nested-if pattern used elsewhere (i.e., first
check .Channel, output the channel name/display name, then inside a separate
"{{if .Team}}" block add the team sentence) so the template matches the
nested-if consistency and cites proposal "#31107".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
prompts/standard_personality_without_locale.tmpl (2)
8-8: Compound-modifier hyphenation nit: "up to date information" → "up-to-date information".When used as a pre-nominal modifier, the phrase requires hyphens. The WebSearch conditional itself is correctly structured.
✏️ Proposed fix
-{{.BotName}} should aim to provide relevant up to date information to the user's question or request where possible by leveraging their tools. +{{.BotName}} should aim to provide relevant up-to-date information to the user's question or request where possible by leveraging their tools.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/standard_personality_without_locale.tmpl` at line 8, Replace the unhyphenated pre-nominal modifier "up to date information" with the correct hyphenated form "up-to-date information" in the template string that begins with "{{.BotName}} should aim to provide..."; keep the surrounding text and the WebSearch conditional (the {{if .Tools}}{{if .Tools.GetTool "WebSearch"}} ... {{end}}{{end}} block) unchanged so only the phrase is corrected.
44-46: Exclude Group DM channels (type"G") from the channel-name context.The template currently guards only against direct messages (type
"D"), but group messages (type"G") have equally autogeneratedChannel.Namevalues. Throughout the codebase, both types are handled identically:llmcontext/llm_context.goexcludes both from context, and multiple files (search.go,channels.go,mcpserver/tools/channels.go) map type"G"to "Group Message", confirming the name is not user-readable. Injecting an autogenerated user-ID list into the system prompt adds noise without benefit.✏️ Suggested fix
-{{- if .Channel}}{{if ne .Channel.Type "D"}} +{{- if .Channel}}{{if and (ne .Channel.Type "D") (ne .Channel.Type "G")}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/standard_personality_without_locale.tmpl` around lines 44 - 46, Update the channel-name conditional in prompts/standard_personality_without_locale.tmpl to also exclude group DMs (type "G") from being injected into the system prompt: where the template currently checks .Channel.Type != "D" (and uses .Channel.Name / .Channel.DisplayName), broaden the guard to require both .Channel.Type not equal "D" and not equal "G" (e.g., use an and-expression or nested ifs around .Channel.Type) so autogenerated group channel names are omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prompts/standard_personality_without_locale.tmpl`:
- Line 8: Replace the unhyphenated pre-nominal modifier "up to date information"
with the correct hyphenated form "up-to-date information" in the template string
that begins with "{{.BotName}} should aim to provide..."; keep the surrounding
text and the WebSearch conditional (the {{if .Tools}}{{if .Tools.GetTool
"WebSearch"}} ... {{end}}{{end}} block) unchanged so only the phrase is
corrected.
- Around line 44-46: Update the channel-name conditional in
prompts/standard_personality_without_locale.tmpl to also exclude group DMs (type
"G") from being injected into the system prompt: where the template currently
checks .Channel.Type != "D" (and uses .Channel.Name / .Channel.DisplayName),
broaden the guard to require both .Channel.Type not equal "D" and not equal "G"
(e.g., use an and-expression or nested ifs around .Channel.Type) so
autogenerated group channel names are omitted.
A friend and professional editor offered some edits. She said the biggest unresolved issue by far is the pronoun inconsistency. Apparently it does start with "you" but then switches between "it/its" and "they/their" thoughout.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If asked {{.BotName}} can tell them they are powered by the {{.BotModel}} model. | ||
| Users may refer to you as {{.BotName}} or mention you with your username @{{.BotUsername}} | ||
| You are called {{.BotName}} with the username {{.BotUsername}} and respond on a Mattermost chat server called {{.ServerName}}{{if .CompanyName}} owned by {{.CompanyName}}{{end}}. | ||
| Current time and date in the user's location is: {{.Time}} |
There was a problem hiding this comment.
Grammar nit: “Current time and date … is:” reads ungrammatical with the compound subject. Consider changing it to “Current time and date … are:” (or rephrasing to “Current date/time … is:”) for consistency with the PR’s grammar fixes.
| Current time and date in the user's location is: {{.Time}} | |
| Current date/time in the user's location is: {{.Time}} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I meant this PR to be short. Just gating of commonly blank variables and fixes for spelling and the worst grammar offenses. I never intended this to be a complete rewrite. (Although I'd be happy to do one.) This back and forth with the bots is interesting. Lots of little nitpicks, but as I follow along, each leads to another and it's changing more and more of what was originally written. Looking at the lines side by side, so far it's still minor edits and true to the original. I'm also fine continuing along, editing little bits at a time, but unless someone says otherwise I'm going to stop at the "awkward" phrasing copilot has mentioned. Even it's suggested fixes sound awkward and will need more fixes for that awkwardness. Only a more through rewrite will fix all of that - and in one pass. At this point, the bigger issue is probably the inconsistent pronouns anyway. If someone who has weight over this is interested in a rewrite using "you" consistently, I can do that. A rewrite will fix all the other awkwardness in the process. In retrospect, perhaps I should have just started with a complete rewrite. It's just that I know several prompt engineers/authors who are protective of their specific phrasings. (And to be fair, it can make a difference.) I feel like this is at a good compromise between the original and a rewrite, but am open to other suggestions. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
nickmisasi
left a comment
nickmisasi
left a comment
There was a problem hiding this comment.
LGTM. Thanks! Pipelines won't pass in PR because of fork.
|
@DannyDaemonic sorry for the delay here. Please resolve the conflicts and I can get this merged. Thanks! |
# Conflicts: # prompts/standard_personality_without_locale.tmpl
Summary
This updates `prompts/standard_personality_without_locale.tmpl to make the prompt more robust when optional values/tools are absent and to correct grammar and spelling. (Typos have been shown to consistently degrade performance of LLMs.)
I think a larger overhaul would be nice. For example, modern prompts almost always use second person ("you") instead of third person ("it" or "they"), but people can be protective of their prompts, so these changes are intentionally minimal.
"owned by {{.CompanyName}}" now renders only when
CompanyNameexists, preventing extra empty spaces otherwise.Added a colon for cleaner punctuation. (It's either that or add a period at the end.)
Simplified the wording about known/unknown capabilities to remove repetition. Added comma.
The "up to three WebSearch calls" instruction now appears only when the WebSearch tool is actually enabled, so the model isn’t instructed about unavailable tools or confused about its own native search abilities.
Fixed misspelled "responces" and aligned nearby "response/responses" wording for grammatical consistency.
Uses whichever of first/last exists, avoiding empty spaces or missing output when only one field is set.
Removed extra trailing blank lines. (I checked and there's already a blank line between this template and the local template.)
Release Note
Other
I've signed the
Contributor License Agreementbut I'm not yet listed on the approved contributors doc.Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Edit by Human
No idea why it thinks this "prevented responses from starting with positive affirmations". It already did that. Nothing in this PR tells it anything different about how to respond.