feat(prompt): add communication guidance - #1214
tautvydasLiekis wants to merge 2 commits into
Conversation
Add a ## Communication section (answer-first, milestone reporting, evidence-based completion to single-model and orchestrator prompts. Replaces the forced per-tool-result text rule and the single-model orientation paragraph. Subagents keep their JSON protocol: append-mode inheritance strips the section. Budgets raised to match (systemPrompt 2050→2450, measured 2287 est tokens, 7.1% headroom). Unit + TUI e2e coverage added; user docs in docs/communication.md. Co-Authored-By: Kimchi <noreply@kimchi.dev> EOF )
Master's timeout-guidance rewording (#1201) adds 78 chars to the canonical prompt; combined with the communication section the merged tree measures 2453 est tokens, 3 over the 2450 budget. Raise systemPrompt to 2625 and total to 2705 (~7% headroom over the measured 2453/2532). Co-Authored-By: Kimchi <noreply@kimchi.dev>
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 88/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Coverage is strong: unit tests assert the new 📝 Found 4 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 88/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Coverage is strong: unit tests assert the new ## Communication section appears exactly once in single and orchestrator modes, is excluded from subagent mode, and is positioned after project context; negative assertions guard against removed wording regressing. prompts.test.ts verifies stripHumanResponseStyle removes the section from inherited worker prompts with and without activeToolNames, and a new e2e test confirms the guidance is sent unchanged on successive turns. The token budget test was updated consistently (2453 * ~1.07 ≈ 2625, and 2625 + 80 = 2705). The only gap is that the strip regex is tested only in the Communication-is-last position, not mid-prompt.
📝 Found 4 issue(s). See inline comments for details.
| return [...new Set(toolNames)].filter(Boolean) | ||
| } | ||
|
|
||
| function stripHumanResponseStyle(prompt: string): string { |
There was a problem hiding this comment.
ℹ️🐛 Bug
The regex in stripHumanResponseStyle requires a newline immediately after the ## Communication header ([^\n]*\n). If a parent prompt ever ends exactly at the header with no trailing newline or body, the replacement silently no-ops and the bare ## Communication header leaks into the worker prompt. Prompts built by buildPrompt always include a body, so this is a latent edge case rather than an active bug.
💡 Suggestion: Make the trailing newline optional: /(^|\n)## Communication\b[^\n]*(?:\n|$)[\s\S]*?(?=\n#{1,2} |\n*$)/g, or add a comment documenting the invariant that the Communication section always has a body.
| } | ||
|
|
||
| describe("default agents — subagent system prompt snapshot", () => { | ||
| it.each([ |
There was a problem hiding this comment.
ℹ️🧪 Testing
The new stripHumanResponseStyle tests only cover the case where ## Communication is the final section of the parent prompt (exercising the \n*$ lookahead branch). The mid-prompt case — where Communication is followed by another ## heading and the lookahead (?=\n#{1,2} ) must terminate the lazy match — is untested, so a future edit to the lookahead pattern could regress middle-section stripping without a failing test.
💡 Suggestion: Add a case with a parent prompt like "## Rules\n...\n## Communication\n...\n## Environment\n..." asserting that Communication content is removed while the trailing ## Environment section is preserved.
| return [...new Set(toolNames)].filter(Boolean) | ||
| } | ||
|
|
||
| function stripHumanResponseStyle(prompt: string): string { |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
stripHumanResponseStyle uses lookahead (?=\n#{1,2} |\n*$) while the adjacent stripAvailableToolsSection uses (?=\n#+ |\n*$). If the COMMUNICATION constant ever contains a line starting with # or ## (e.g., an embedded markdown or code example), stripping would stop early and leak the remainder of the section into worker prompts. The current constant has no such lines, so this is documentation-level risk.
💡 Suggestion: Add a brief comment on stripHumanResponseStyle noting that the COMMUNICATION section must not contain level-1/2 headings, or align the lookahead with the sibling function's #+ pattern if behavior parity is intended.
| }) | ||
| for (const prompt of prompts) { | ||
| expect(prompt).toContain("## Communication") | ||
| expect(prompt).toContain("Explicit output formats, including code-only or structured data, take precedence") |
There was a problem hiding this comment.
ℹ️🧪 Testing
The assertion expect(prompts[1]).toBe(prompts[0]) enforces byte-identical system prompts across turns. This is a reasonable stability guard, but it will fail on any future legitimate addition of per-turn dynamic content (e.g., timestamps, turn counters, telemetry markers) to the system prompt, and the failure message won't explain the intent.
💡 Suggestion: Keep the strict equality if prompt stability is a hard requirement, but rename the trace step or add a comment clarifying it guards prompt-identity for cache stability; otherwise compare a normalized form (e.g., strip known-volatile segments) instead.
1 participant
Linked issue
Closes #1213
What does this PR do?
Adds a
## Communicationsection with answer-first, milestone-based reporting guidance to single-model and orchestrator system prompts, replacing the forced per-tool-result text rule and the single-model orientation paragraph. Subagents keep their JSON output protocol: append-mode prompt inheritance strips the section. Assumption handling softens from confirm-before-acting to investigate-with-tools, ask only when materially needed.Context budgets raised to match (systemPrompt 2625 / total 2705 — merged canonical prompt measures 2453 est tokens, ~7% headroom, accommodating #1201's timeout rewording).
Verification: 85 affected unit tests, new TUI e2e test (system prompt carries the section and is stable across turns), and a live tmux session on
glm-5.3-flashconfirming answer-first responses and zero inter-tool narration.Checklist
pnpm run test)pnpm run check)