Transform Lace persona system: Orchestrator-first approach#317
Transform Lace persona system: Orchestrator-first approach#317
Conversation
## Summary - Transform default 'lace' persona from coder to thoughtful orchestrator - Create explicit 'coder' persona for programming tasks - Mark internal personas with underscore prefix (_helper-agent, _session-summary) ## Changes ### New Lace Persona (Orchestrator) - Focuses on understanding user goals through contextual elicitation - Asks intelligent follow-up questions based on what's been shared - Delegates implementation work to specialized agents (particularly 'coder') - Knows how to use task management tools (task_add, task_list, etc.) - Understands async vs sync delegation patterns ### Persona Restructuring - Renamed old 'lace.md' to 'coder.md' (the coding-focused persona) - Consolidated 'coding-agent' content into 'coder' and removed duplicate - Marked internal personas with underscore prefix for clarity - Created new 'lace.md' focused on orchestration and goal clarification ### Task Delegation - Documented how to use task_add with "new:coder:provider/model" format - Explained difference between async tasks (parallel work) and sync delegation - Added clear instructions for spawning multiple agents for independent work ## Test Updates - Updated all tests to reflect new persona names - Added comprehensive tests for new lace persona behavior - All tests passing (except external service integration tests) This change makes Lace default to being a thoughtful partner who clarifies goals before jumping into implementation, improving the overall user experience.
WalkthroughPersona identifiers renamed in tests (coding-agent → coder; helper-agent → _helper-agent) and a new internal persona (_session-summary) added. coder.md inserts interaction-patterns and environment includes. lace.md is rewritten to a single explicit persona spec. New lace persona test suite added; other tests updated to new identifiers and expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Caller
participant PM as PromptManager
participant PR as personaRegistry
participant FS as Persona Files
Dev->>PM: generateSystemPrompt(personaKey?)
alt personaKey omitted
PM->>PR: getDefaultPersona()
PR-->>PM: "lace"
else personaKey provided
PM-->>PM: use provided key
end
PM->>PR: hasPersona(key)
PR-->>PM: true/false
alt found
PM->>PR: getPersonaPath(key)
PR-->>PM: path to *.md
PM->>FS: load persona content
FS-->>PM: persona markdown
PM-->>Dev: system prompt (persona-specific)
else not found
PM-->>Dev: error
end
sequenceDiagram
autonumber
actor Orchestrator as Lace Persona
participant Registry as personaRegistry
participant Coder as coder persona
note over Orchestrator: From Understanding to Action
Orchestrator->>Orchestrator: Parse intent, ask follow-ups
Orchestrator->>Coder: delegate(task, context)
Coder-->>Orchestrator: result
Orchestrator->>Orchestrator: Synthesize & Validate
Orchestrator-->>Orchestrator: Iterate or finalize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Comment |
Code Review: Transform Lace persona system - APPROVEDThis is an excellent architectural improvement that transforms Lace from a reactive coder to a thoughtful orchestrator. The changes are clean, well-tested, and maintain backward compatibility while fundamentally improving the user experience. Key Strengths
Suggestions for Future Improvements
The implementation follows YAGNI principles, maintains code quality standards, and introduces no security concerns. Ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/core/config/agent-personas/lace.md (2)
72-76: Align NewAgentSpec example with actual format/models used in testsUse a model name we already exercise elsewhere to avoid confusion and ensure copy/paste works.
- - Example: `task_add({ tasks: [{ title: "Implement auth", prompt: "...", assignedTo: "new:coder:anthropic/claude-3" }]})` + - Example: `task_add({ tasks: [{ title: "Implement auth", prompt: "...", assignedTo: "new:coder:anthropic/claude-3-sonnet" }]})`
39-48: Add an explicit security/PII guardrail to Working PrinciplesMake it explicit not to log secrets or sensitive data; aligns with repo guidelines.
## Working Principles @@ **Maintain Momentum**: While thorough understanding is important, don't get stuck in analysis paralysis. Know when you have enough information to proceed effectively. + +**Protect Sensitive Data**: Never log API keys, credentials, or PII. Prefer structured logs with minimal metadata, and mask tokens in any shared context.packages/core/src/tasks/agent-spawning-personas.test.ts (3)
1-1: Add ABOUTME header per repo guidelinesTop-of-file ABOUTME comment is required for all *.ts files.
+// ABOUTME: Tests agent spawning via NewAgentSpec, ensuring persona/provider/model plumb through and events fire. import { describe, it, expect, beforeEach, vi, type MockedFunction } from 'vitest';
37-45: Remove unused variable in test
_taskIdis never used.- const _taskId = await taskManager.createTask( + await taskManager.createTask( { title: 'Test Task', prompt: 'Do something', assignedTo: agentSpec, }, context );
150-156: Rename test to match actual event payloadEvent does not include persona; either include persona in the event (requires prod code change) or rename for accuracy. Suggest renaming the test title now.
- it('emits agent spawned event with persona information', async () => { + it('emits agent spawned event with provider/model information', async () => {packages/core/src/integration/persona-system.test.ts (3)
1-1: Add ABOUTME header per repo guidelinesMissing ABOUTME comment.
+// ABOUTME: Integration tests for persona registry, prompt generation, NewAgentSpec parsing, and task workflow. import { describe, it, expect, beforeEach, vi } from 'vitest';
161-174: Eliminateanyin tests; guard then cast to branded typeFollow “assert by parsing to unknown then validating before casting”.
- for (const spec of specs) { - const agentSpec = spec as any; // Type assertion for test - - // Should be recognized as valid NewAgentSpec - expect(isNewAgentSpec(agentSpec)).toBe(true); - - // Should parse correctly - const parsed = parseNewAgentSpec(agentSpec); + for (const spec of specs) { + // Should be recognized as valid NewAgentSpec + expect(isNewAgentSpec(spec)).toBe(true); + // Should parse correctly (safe cast after guard) + const parsed = parseNewAgentSpec(spec as ReturnType<typeof String> & import('~/threads/types').NewAgentSpec);Additional import (optional for clarity):
+// import type { NewAgentSpec } from '~/threads/types';
177-184: Remove unnecessaryanyin negative-format testNo cast needed; these are plain strings.
- for (const oldFormat of oldFormats) { - const spec = oldFormat as any; - expect(isNewAgentSpec(spec)).toBe(false); - } + for (const oldFormat of oldFormats) { + expect(isNewAgentSpec(oldFormat)).toBe(false); + }packages/core/src/config/example-personas.test.ts (1)
1-1: Add ABOUTME header per repo guidelinesABOUTME header is required for *.ts files.
+// ABOUTME: Sanity checks for bundled personas: discovery, paths, prompts, and validation. import { describe, it, expect } from 'vitest';packages/core/src/config/lace-persona.test.ts (1)
55-66: Strengthen default-persona assertionActually exercise the default parameter path by omitting the persona argument.
- it('default agent uses lace persona', async () => { + it('default agent uses lace persona', async () => { // This is implicit in Agent constructor: this._persona = config.persona || 'lace'; // We're verifying the fallback behavior const promptManager = new PromptManager({}); - // When no persona is specified, it should use 'lace' - const defaultPrompt = await promptManager.generateSystemPrompt('lace'); + // When no persona is specified, it should use 'lace' + const defaultPrompt = await promptManager.generateSystemPrompt(); const explicitLacePrompt = await promptManager.generateSystemPrompt('lace'); expect(defaultPrompt).toBe(explicitLacePrompt); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (6)
packages/core/config/agent-personas/coder.md(3 hunks)packages/core/config/agent-personas/lace.md(1 hunks)packages/core/src/config/example-personas.test.ts(4 hunks)packages/core/src/config/lace-persona.test.ts(1 hunks)packages/core/src/integration/persona-system.test.ts(4 hunks)packages/core/src/tasks/agent-spawning-personas.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/core/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In core, use
~/*path aliases for internal imports and omit file extensions in import specifiers
Files:
packages/core/src/config/lace-persona.test.tspackages/core/src/tasks/agent-spawning-personas.test.tspackages/core/src/config/example-personas.test.tspackages/core/src/integration/persona-system.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use inline dynamic imports in type positions; declare imports at the top instead
Each file starts with a// ABOUTME:comment describing its purpose
TypeScript strict: preferunknownoverany; avoidanyentirely
Always await Promises or explicitly usevoid(no floating promises)
No unsafe assignments; do not assign values of typeanywithout proper typing or narrowing
Remove unused imports and variables
Explain the WHY any time a linting rule is disabled in code
Never log API keys or sensitive content; prefer structured log objects with metadata
Files:
packages/core/src/config/lace-persona.test.tspackages/core/src/tasks/agent-spawning-personas.test.tspackages/core/src/config/example-personas.test.tspackages/core/src/integration/persona-system.test.ts
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use console.* for server-side debugging; use the repository logger
Files:
packages/core/src/config/lace-persona.test.tspackages/core/src/tasks/agent-spawning-personas.test.tspackages/core/src/config/example-personas.test.tspackages/core/src/integration/persona-system.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{ts,tsx}: Co-locate tests next to sources; name tests *.test.ts(x) or *.spec.ts(x)
Tests should import real production types and use typed mocks
Use factory fixtures with Partial for building test data
Assert by parsing to unknown then validating before casting in tests
Bananyand@ts-ignorein tests; if unavoidable, localize to one line with a TODO
Use tempdir utilities when creating temporary directories in tests
Files:
packages/core/src/config/lace-persona.test.tspackages/core/src/tasks/agent-spawning-personas.test.tspackages/core/src/config/example-personas.test.tspackages/core/src/integration/persona-system.test.ts
🧬 Code graph analysis (4)
packages/core/src/config/lace-persona.test.ts (1)
packages/core/src/config/prompt-manager.ts (1)
PromptManager(27-143)
packages/core/src/tasks/agent-spawning-personas.test.ts (1)
packages/core/src/threads/types.ts (1)
createNewAgentSpec(538-540)
packages/core/src/config/example-personas.test.ts (1)
packages/core/src/config/persona-registry.ts (1)
personaRegistry(162-162)
packages/core/src/integration/persona-system.test.ts (5)
packages/core/src/agents/agent.ts (3)
AgentConfig(33-44)provider(684-689)model(696-699)packages/core/src/tasks/task-manager.ts (2)
AgentCreationCallback(11-16)TaskManager(18-336)packages/core/src/threads/types.ts (2)
createNewAgentSpec(538-540)isNewAgentSpec(534-536)packages/core/src/config/prompt-manager.ts (1)
PromptManager(27-143)packages/core/src/config/prompts.ts (1)
loadPromptConfig(58-75)
🔇 Additional comments (2)
packages/core/config/agent-personas/lace.md (1)
87-99: Verified: template variables and task/delegate tools are presentFound {{context.workingDirectory}} and {{context.platform}} in packages/core/config/agent-personas/lace.md; {{context.disclaimer}} in packages/core/config/agent-personas/coder.md and _helper-agent.md; context tests in packages/core/src/tools/context-working-directory.test.ts. Tools 'delegate' and task verbs ('task_add','task_list','task_view','task_add_note','task_complete') are implemented/registered in packages/core/src/tools/implementations/delegate.ts, packages/core/src/tools/implementations/task-manager/tools.ts and packages/core/src/tools/tool-catalog.ts.
packages/core/config/agent-personas/coder.md (1)
22-25: Includes exist; duplicates found — confirm render order before toolsinteraction-patterns.md and environment.md exist at packages/core/config/agent-personas/sections and are included in:
- packages/core/config/agent-personas/coder.md — lines 22, 24
- packages/core/config/agent-personas/_helper-agent.md — lines 22, 24
Ensure these includes are rendered before any tools sections and remove or consolidate the duplicate includes if unintentional.
1 participant
Summary
This PR transforms Lace's persona system to default to a thoughtful orchestrator that understands user goals before delegating implementation, rather than immediately jumping into coding.
Problem
Currently, Lace acts as a "yes man" coder - when users say "build me a todo list", it just builds one without understanding what they're actually trying to accomplish. This often leads to building the wrong thing.
Solution
Transform the default
lacepersona from a coder to an intelligent orchestrator that:Key Changes
1. New Lace Persona (Orchestrator)
lace.mdfocused on goal clarification and orchestrationtask_add,task_list, etc.)"new:coder:provider/model"format2. Explicit Coder Persona
lace.mdtocoder.mdfor explicit coding taskscoding-agentcontent intocoder(removed duplicate)3. Internal Persona Marking
helper-agent→_helper-agentsession-summary→_session-summaryTesting
Impact
This change makes Lace default to being a thoughtful partner who:
The goal is to make the human partner "superhuman" through excellent project management and coordination, not just immediate code generation.
Breaking Changes
None for end users. The system still defaults to the 'lace' persona, it just behaves differently now (better!). Users who want the old behavior can explicitly request the 'coder' persona.