suggest_next: end turn automatically and add summary parameter

[m-aebrer]

Closes #210

Make suggest_next end the agent turn automatically (like wait and subagent) and add an optional summary parameter so the agent can consolidate its wrap-up into the tool call — saving one LLM round-trip per suggestion.

Implementation plan posted as a comment below.

[m-aebrer]

Implementation Plan

Problem

suggest_next doesn't set endTurn: true, causing a wasted LLM round-trip after every suggestion. Models already write summary text before calling suggest_next — consolidating both into one tool call that ends the turn saves tokens and prevents junk follow-ups from weaker models.

Deliverables

1. suggest_next ends the turn on success (endTurn: true)
2. Optional summary parameter for the agent to provide a markdown wrap-up of previous work
3. Summary rendered in the TUI above the suggestion arrow
4. Updated prompt guidelines instructing models to use summary and noting turn ends automatically

Files to Modify

File	Changes
packages/coding-agent/src/core/tools/suggest-next.ts	Add summary to schema, extend details type, add endTurn: true on success, update renderResult to show summary, update promptGuidelines
packages/coding-agent/test/suggest-next.test.ts	Assert endTurn: true on success, assert no endTurn on error, test summary in details

Design Decisions

• Error path: No endTurn on error (invalid command) — model can retry
• Rendering: Use Box + Text in renderResult — summary as themed text above the → /command line
• Schema: summary is Type.Optional(Type.String(...)) — backward compatible
• Details type: SuggestNextDetails gets summary?: string

Testing Approach

• Assert endTurn: true present on successful execution
• Assert endTurn absent/undefined on error paths
• Assert summary stored in details when provided
• Assert tool works without summary (backward compat)
• Existing sanitization/validation tests unchanged

Acceptance Criteria

• suggest_next success result includes endTurn: true
• suggest_next error result does NOT include endTurn: true
• Optional summary parameter accepted and stored in details
• renderResult displays summary text above the suggestion arrow when present
• promptGuidelines instruct models to use summary and note turn ends automatically
• All existing tests pass, new tests cover endTurn and summary

---

Plan created by mach6

[m-aebrer]

Implementation Complete

Changes in b7c03c7:

packages/coding-agent/src/core/tools/suggest-next.ts

• Added optional summary parameter to schema (markdown string for work wrap-up)
• Extended SuggestNextDetails type with summary?: string
• Added endTurn: true on success return (error path intentionally omits it for retry)
• Updated formatSuggestNextResult to render summary above the → /command line
• Added new prompt guideline: "Include a brief summary of work done in the summary parameter"

packages/coding-agent/test/suggest-next.test.ts

• Updated helper type to include summary param and endTurn in return
• Added endTurn behavior describe block (3 tests: success sets it, errors don't)
• Added summary parameter describe block (4 tests: included in details, backward compat, trimming, empty-as-undefined)

All 3146 tests pass. Build clean.

---

Progress update by mach6

[m-aebrer]

Code Review

Important

Finding 1: summary field not sanitized for control characters, unlike command (confidence: 87)
File: packages/coding-agent/src/core/tools/suggest-next.ts (lines ~90–103)

The command parameter is explicitly scrubbed of control characters before use:

const command = rawCommand?.replace(/[\x00-\x1f\x7f]/g, "").trim();

But summary only gets .trim():

details: { suggestion: command, summary: summary?.trim() || undefined },

Both reach Text.setText() via formatSuggestNextResult. The Text component's render() normalizes \t → spaces but passes all other control characters through to the terminal (\r, \x00, \x07, \x1b, etc.), which can corrupt TUI rendering — the exact concern cited in the inline comment on command. A summary like "Done.\x1b[2J" would survive the guard and corrupt the terminal.

Suggested fix — strip non-newline control characters from summary (preserving \n since multi-line markdown summaries are valid):

const sanitizedSummary = summary?.replace(/[\x00-\x09\x0b-\x1f\x7f]/g, "").trim() || undefined;
details: { suggestion: command, summary: sanitizedSummary },

Finding 2: formatSuggestNextResult rendering is completely untested (confidence: 95)
File: packages/coding-agent/test/suggest-next.test.ts

The test suite only calls execute() and inspects the raw return value (result.details, result.endTurn, result.content). It never calls renderResult or formatSuggestNextResult, so the entire rendering path — which is how the summary is actually displayed to the user — is untested. The function has three distinct branches (no details/error, summary present, no summary), none covered.

A test should create a minimal theme mock and assert:

• With summary: output contains the summary text, then a blank line, then → /some-cmd
• Without summary: output is just → /some-cmd
• Error case (no details): output contains the error text from content[0]

Suggestions

Finding 3: Array-push-join pattern could be a simple ternary (confidence: 93)
File: packages/coding-agent/src/core/tools/suggest-next.ts (lines 56–62)

The 7-line array-push-join pattern can be expressed as a 2-line template literal with identical behavior:

const arrow = theme.fg("toolOutput", `→ ${details.suggestion}`);
return details.summary ? `${details.summary}\n\n${arrow}` : arrow;

Strengths

• endTurn: true correctly set only on the success path, matching wait.ts and subagent.ts patterns
• Error paths deliberately omit endTurn, allowing the model to retry — good design
• summary?.trim() || undefined correctly collapses whitespace-only strings, consistent with wait.ts's params.reason?.trim() || undefined
• TypeBox schema uses the standard Type.Optional(Type.String({...})) pattern
• Test suite has thorough coverage of execute-path behaviors: endTurn on success, no endTurn on both error variants, summary trimming, empty-as-undefined, backward compat
• All issue 210 acceptance criteria are fully met

Agents run: code-reviewer, error-auditor, completeness-checker, test-reviewer, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

#214 (comment)

Classifications

Finding	Classification	Reasoning
1: summary not sanitized for control characters	Genuine issue	The command field gets explicit control char stripping but summary only gets .trim(). Both reach Text.setText() which passes control chars (except \t) through to the terminal. Inconsistency is a real correctness gap — same corruption risk as command.
2: formatSuggestNextResult rendering untested	Nitpick	Claim is accurate but consistent with project testing patterns — no other tool tests its renderResult except subagent (which is far more complex). The function is a 10-line pure string concatenation.
3: Array-push-join could be a ternary	Nitpick	Pure style preference. Current code is clear and readable. Does not affect correctness or maintainability.

Action Plan

1. Sanitize summary for control characters — apply stripping similar to command but preserve \n (valid in markdown summaries). Use /[\x00-\x09\x0b\x0c\x0e-\x1f\x7f]/g to keep newlines and tabs (which Text.render() already handles).

---

Assessment by mach6

[m-aebrer]

Progress Update

Fixed markdown rendering in suggest_next summary and addressed review finding 1 (control character sanitization):

• Markdown rendering: renderResult now uses Markdown component (via Container + Markdown + Text) instead of raw Text, so summaries with headings, tables, lists, etc. render properly in the TUI
• Control character sanitization: summary now gets replace(/[\x00-\x09\x0b\x0c\x0e-\x1f\x7f]/g, "") — strips harmful control chars while preserving \n (valid for markdown) and \r is stripped, matching the pattern used for command
• Tests added: 2 new tests for summary sanitization (preserves newlines, control-char-only → undefined)
• Dead code removed: formatSuggestNextResult helper was only used by old renderResult — removed

Commit: d117c8d

---

Progress tracked by mach6

[m-aebrer]

Code Review

Important

Finding 1: renderResult is entirely untested — all 3 branches (confidence: 100)
File: packages/coding-agent/src/core/tools/suggest-next.ts (renderResult)

renderResult has three distinct code paths, none exercised in the test file:

1. No details (error case): extracts text from result.content[0], renders as Text
2. Details with summary: constructs Container with Markdown child + Text child (→ suggestion)
3. Details without summary: renders a plain Text with → suggestion

The test suite only calls execute() and inspects raw return values. The rendering path — how the summary is actually displayed — is completely untested.

Finding 2: promptGuidelines does not explicitly state the turn ends automatically (confidence: 80)
File: packages/coding-agent/src/core/tools/suggest-next.ts (promptGuidelines)

Issue 210 AC item 4 requires: "Update the tool's promptGuidelines to note that the turn ends automatically." The current guideline says "this is your last chance to communicate before the turn ends" but this is embedded in the summary parameter instruction rather than stated as a standalone behavioral note. A model could interpret it as "say something because you're nearing the end of your response" rather than "this tool call terminates the agent loop."

Missing explicit guideline like: "Calling suggest_next ends the turn immediately — no further LLM call will be made."

Finding 3: Carriage return (\x0d) survives summary sanitization (confidence: 85)
File: packages/coding-agent/src/core/tools/suggest-next.ts (line 85)

The summary sanitization regex /[\x00-\x09\x0b\x0c\x0e-\x1f\x7f]/g has a gap: \x0d (CR) falls between \x0c (12) and \x0e (14) and is not matched. The stated intent is "preserve only \n, strip everything else," but CR survives. While marked normalizes bare CR to \n (mitigating terminal corruption), "Fixed bug: use\rReplace" silently produces an unintended line break. The command sanitization strips all controls including CR, making this inconsistent.

Fix: [\x00-\x09\x0b\x0c\x0d\x0e-\x1f\x7f] or equivalently [\x00-\x09\x0b-\x0d\x0e-\x1f\x7f]

Suggestions

Finding 4: Duplicated Text reuse pattern in renderResult (confidence: 85)
File: packages/coding-agent/src/core/tools/suggest-next.ts (renderResult)

The error and no-summary branches both do (context.lastComponent as Text | undefined) ?? new Text("", 0, 0) identically. Could be refactored to check the Container/summary case first as an early return, then share a single Text creation for the remaining two branches.

Finding 5: bind(tool) is a no-op in the test helper (confidence: 80)
File: packages/coding-agent/test/suggest-next.test.ts (line ~10)

tool.execute.bind(tool) — execute never references this (closure over onSuggest is all the state it needs). The .bind(tool) has no effect; only the type cast matters.

Finding 6: Redundant truthiness guard in renderResult (confidence: 92)
File: packages/coding-agent/src/core/tools/suggest-next.ts (renderResult, no-details branch)

content?.type === "text" && content.text ? content.text : "" — the && content.text guard is redundant since TextContent.text is typed as string (non-optional). When text is "", both forms evaluate to "".

Strengths

• endTurn: true correctly set only on the success path, matching wait.ts and subagent.ts patterns exactly
• Error paths deliberately omit endTurn, allowing the model to retry — good design
• Control character sanitization for summary preserves \n for markdown while stripping harmful chars
• || undefined idiom correctly collapses whitespace-only summaries
• SuggestNextCallback correctly receives only command (not summary) — ghost text only needs the command string
• (result as any).details cast matches the idiomatic pattern used in wait.ts
• Thorough execute-path test coverage: endTurn, summary trimming, empty/whitespace, control chars, backward compat
• All issue 210 acceptance criteria functionally met

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

#214 (comment)

Classifications

Finding	Classification	Reasoning
1: renderResult entirely untested	Deferred	Other tool tests (bash-truncation, tmp-read, etc.) also don't test their renderResult. Project pattern is to test rendering at the integration layer in tool-execution-component.test.ts. Consistent with conventions — out of scope for this PR.
2: promptGuidelines doesn't explicitly state turn ends automatically	Genuine issue	Issue 210 AC item 4 requires: "Update the tool's promptGuidelines to note that the turn ends automatically (models don't need to call wait afterwards)." Current text implies it but doesn't explicitly state the turn ends automatically or that wait is unnecessary. Easy one-line fix.
3: CR (\x0d) survives summary sanitization	Genuine issue	Confirmed: regex /[\x00-\x09\x0b\x0c\x0e-\x1f\x7f]/g skips \x0d (gap between \x0c and \x0e). The command regex strips all controls including CR. Intent is "preserve only \n" — CR surviving is inconsistent. Fix: /[\x00-\x09\x0b-\x1f\x7f]/g (skips only \x0a).
4: Duplicated Text reuse pattern	Nitpick	Standard render-method boilerplate in adjacent branches. Extracting adds indirection for negligible benefit.
5: bind(tool) is a no-op	Nitpick	Correct that execute never references this, but harmless defensive coding in test utility. Removing doesn't improve anything.
6: Redundant truthiness guard	Nitpick	TextContent.text is typed string (non-optional). Guard is redundant but harmless.

Action Plan

1. Fix CR gap in summary sanitization regex — change /[\x00-\x09\x0b\x0c\x0e-\x1f\x7f]/g to /[\x00-\x09\x0b-\x1f\x7f]/g (strips everything except \x0a)
2. Add explicit "turn ends automatically" guideline — standalone entry like "Calling this tool ends your turn automatically — do not call wait afterwards"

Deferred: Finding 1 (renderResult test coverage) — consistent with project patterns; no tools currently test renderResult individually. Not creating a tracking issue since this is a project-wide concern, not specific to this tool.

---

Assessment by mach6

[m-aebrer]

Progress Update

Addressed review findings 2 and 3, plus two additional improvements:

• Finding 2 fix: Added explicit prompt guideline: "Calling this tool ends your turn automatically — do not call wait afterwards"
• Finding 3 fix: Fixed CR (\x0d) gap in summary sanitization regex — changed to /[\x00-\x09\x0b-\x1f\x7f]/g
• Literal \n rendering: Summary now converts literal \n sequences to actual newlines (LLMs emit these in XML tool calls)
• Subagent exclusion: Added suggest_next to SUBAGENT_EXCLUDED_TOOLS and made alwaysActiveBuiltins conditional in sdk.ts so subagent child processes don't get the tool (verified empirically — tool no longer appears in subagent tool lists)

Commit: 12fdd97

---

Progress tracked by mach6

[m-aebrer]

Code Review

Important

Finding 1: alwaysActiveBuiltins conditional in sdk.ts is untested (confidence: 92)
File: packages/coding-agent/src/core/sdk.ts (lines 262–267)

The new conditional:

const alwaysActiveBuiltins = options.tools
    ? ["skill", "tasks_update", "search"]
    : ["skill", "tasks_update", "search", "suggest_next"];

…determines whether suggest_next is auto-activated. When options.tools is explicitly provided (subagent child processes pass --tools), suggest_next is excluded. When it's omitted (normal interactive use), suggest_next is included.

No test verifies either behavior. A regression here could cause suggest_next to end subagent turns prematurely or stop appearing in interactive sessions, neither caught by the current suite.

Finding 2: renderResult is entirely untested — all 3 branches (confidence: 97)
File: packages/coding-agent/src/core/tools/suggest-next.ts (renderResult)

renderResult has three distinct branches:

1. Error branch (no details): returns a Text with raw error message
2. Summary branch (details.summary present): returns a Container with Markdown + Text children
3. No-summary branch: returns a Text with → <suggestion>

The summary branch is non-trivial (calls getMarkdownTheme(), constructs Container, calls container.clear()). A regression in child ordering, clear behavior, or theme would be invisible to the test suite.

Suggestions

Finding 3: JSDoc for tools option doesn't document suggest_next's conditional activation (confidence: 88)
File: packages/coding-agent/src/core/sdk.ts (line 66)

The CreateAgentSessionOptions.tools JSDoc mentions skill, tasks_update, and search as always-active but doesn't note that suggest_next is active by default yet excluded when tools is specified. SDK consumers passing tools: [readTool] silently opt out of suggest_next as a side effect.

Finding 4: renderResult branches 1 and 3 duplicate Text creation (confidence: 90)
File: packages/coding-agent/src/core/tools/suggest-next.ts (lines 107–133)

Branches 1 (error/no details) and 3 (details but no summary) both get-or-create a Text, call setText, and return it. The only difference is the message string. Merging them collapses three branches to two (one per component type) with identical behavior:

if (details?.summary) {
    // Container branch (unchanged)
}
const text = (context.lastComponent as Text | undefined) ?? new Text("", 0, 0);
const msg = details
    ? `→ ${details.suggestion}`
    : content?.type === "text" && content.text ? content.text : "";
text.setText(theme.fg("toolOutput", msg));
return text;

Finding 5: suggest_next in SUBAGENT_EXCLUDED_TOOLS is redundant (confidence: 83)
File: packages/coding-agent/src/core/tools/subagent.ts (line ~159)

suggest_next is a factory-created tool (not in allTools singleton). It cannot activate in subagents even without SUBAGENT_EXCLUDED_TOOLS filtering — the alwaysActiveBuiltins conditional and the .filter((n) => n in allTools) gate both independently prevent it. The defense-in-depth is defensible but architecturally redundant.

Strengths

• endTurn: true correctly set only on the success path, matching wait.ts and subagent.ts patterns
• Error paths deliberately omit endTurn, allowing the model to retry
• Sanitization order is correct: \\n → \n before control-char stripping preserves markdown newlines
• Double-exclusion from subagents (belt-and-suspenders) is well-commented
• renderResult component reuse is safe: render slots are tracked separately and results are immutable
• getMarkdownTheme() is lazy and only rendered in initialized TUI contexts — no crash risk
• All issue 210 acceptance criteria fully met
• Thorough execute-path test coverage: endTurn, summary trimming, empty-to-undefined, control chars, CR stripping, literal \n expansion
• All 547 tests pass

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

#214 (comment)

Classifications

Finding	Classification	Reasoning
1: alwaysActiveBuiltins conditional untested	Nitpick	Integration wiring in sdk.ts requires mocking half the system to test in isolation. The unit-level equivalent (filterSubagentTools) IS tested in wait.test.ts. The conditional is configuration glue with a clear inline comment explaining intent.
2: renderResult entirely untested — all 3 branches	Genuine issue	PR adds a non-trivial renderResult with 3 branches, including one constructing Container+Markdown+Text. New rendering logic added by this PR should have tests.
3: JSDoc doesn't document suggest_next's conditional activation	Nitpick	The inline comment directly above alwaysActiveBuiltins explains the intent clearly. This is an internal API — code-level comment is sufficient.
4: renderResult branches duplicate Text creation	Nitpick	2 lines of shared boilerplate across 2 branches is not a correctness issue. Branches are readable as-is.
5: suggest_next in SUBAGENT_EXCLUDED_TOOLS is redundant	False positive	Intentional defense-in-depth. The list also serves as documentation of intent ("these tools must never reach subagents"). Removing reduces clarity for no benefit.

Action Plan

1. Add renderResult tests for all 3 branches — test error/no-details path (returns Text with error message), details+summary path (returns Container with Markdown + Text), and details-without-summary path (returns Text with arrow + suggestion)

---

Assessment by mach6

[m-aebrer]

Progress Update

Addressed review finding 2 (renderResult untested). Added 5 tests covering all 3 branches:

• Error branch (no details): verifies Text component with error message
• No-summary branch (details present, no summary): verifies Text with → /command
• Summary branch (details + summary): verifies Container with Markdown child + Text arrow
• Component reuse (Text): verifies lastComponent is reused on re-render
• Component reuse (Container): verifies lastComponent Container is reused on re-render

All 3155 tests pass (including 24 suggest-next tests). Build clean.

Commit: 4967bb1

---

Progress tracked by mach6