Skip to content

Commit 58a3ace

Browse files
committed
fix: address code review findings from crossfire review
Review surfaced 10 findings across 5 independent sources (3 Claude specialists + Codex + Gemini). Key changes: - Fix incorrect CLI syntax in base_prompt.md (missing --channel flag, positional arg that should be named) and stale nest_agents.md reference - Apply line-start validation to END_MARKER in find_managed_markers, matching the BEGIN_MARKER contract documented in the function comment - Extract prepend_base_prompt helper to deduplicate [Base] injection across format_prompt, dispatch_heartbeat, and initial_message paths - Move base_prompt_file read from main.rs panic to Config::from_cli with proper ConfigError propagation and 1 MB size guard - Extract try_regenerate_nest helper to consolidate 11 identical error-handling blocks across command files - Sanitize relay_url before AGENTS.md injection (strip CR/LF) - Fix greedy newline strip in strip_orphan_begin_marker - Add doc comments for PromptContext.base_prompt lifetime constraint
1 parent af6a62c commit 58a3ace

11 files changed

Lines changed: 89 additions & 68 deletions

File tree

crates/sprout-acp/src/base_prompt.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ MCP tools (via `sprout-mcp`) are also available but the CLI is preferred for bat
2626

2727
- Address agents and humans with plain `@name` — do NOT bold or italicize mention text (formatting prevents alert delivery).
2828
- Use `sprout messages thread` or MCP `get_thread()` when responding in-thread; post new messages for new topics.
29-
- No push notifications — poll with `sprout messages get --since=<unix_ts>` or MCP `get_messages(since=<ts>)`. When `since` is set without `before`, results are oldest-first (chronological).
29+
- No push notifications — poll with `sprout messages get --channel <UUID> --since <ts>` or MCP `get_messages(channel_id, since=<ts>)`. When `since` is set without `before`, results are oldest-first (chronological).
3030

3131
## Startup Recovery
3232

3333
1. `sprout feed get` (or MCP `get_feed()`) — surface pending mentions and action items. Filter by type: `mentions`, `needs_action`, `activity`, `agent_activity`.
34-
2. `sprout messages get <channel_id>` on assigned channels — catch up on recent history.
34+
2. `sprout messages get --channel <UUID>` on assigned channels — catch up on recent history.
3535
3. Check `AGENTS.md` in your working directory for team context.
3636
4. Check `RESEARCH/`, `GUIDES/`, `PLANS/` before searching externally. Use `sprout messages search --query "..."` for cross-channel keyword lookups.
3737

@@ -49,4 +49,4 @@ Your persistent workspace is in your working directory:
4949
| `REPOS/` | Checked-out source repositories |
5050
| `.scratch/` | Ephemeral working files |
5151

52-
Knowledge files use `ALL_CAPS_WITH_UNDERSCORES.md` naming. `AGENTS.md` lists active agents and roles. See `nest_agents.md` in your working directory for full workspace conventions.
52+
Knowledge files use `ALL_CAPS_WITH_UNDERSCORES.md` naming. `AGENTS.md` lists active agents and roles. See `AGENTS.md` in your working directory for full workspace conventions.

crates/sprout-acp/src/config.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -448,8 +448,10 @@ pub struct Config {
448448
pub agent_owner: Option<String>,
449449
/// Disable the [Base] platform-context section prepended to every prompt.
450450
pub no_base_prompt: bool,
451-
/// Path to a custom base prompt file that overrides the compiled-in default.
452-
pub base_prompt_file: Option<PathBuf>,
451+
/// Resolved content from `--base-prompt-file`, read and validated in
452+
/// `from_cli()`. `None` when using the compiled-in default or when
453+
/// `--no-base-prompt` is set.
454+
pub base_prompt_content: Option<String>,
453455
}
454456

455457
/// Validate and deduplicate allowlist entries: each must be exactly 64 hex chars.
@@ -579,6 +581,22 @@ impl Config {
579581
None
580582
};
581583

584+
let base_prompt_content = if args.no_base_prompt {
585+
None
586+
} else if let Some(ref path) = args.base_prompt_file {
587+
let content = std::fs::read_to_string(path)?;
588+
if content.len() > 1_048_576 {
589+
return Err(ConfigError::ConfigFile(format!(
590+
"base prompt file {} exceeds 1 MB limit ({} bytes)",
591+
path.display(),
592+
content.len()
593+
)));
594+
}
595+
Some(content)
596+
} else {
597+
None
598+
};
599+
582600
if matches!(args.subscribe, SubscribeMode::Config) {
583601
if args.kinds.is_some() {
584602
tracing::warn!("--kinds is ignored in config mode");
@@ -787,7 +805,7 @@ impl Config {
787805
relay_observer: args.relay_observer,
788806
agent_owner: args.agent_owner.map(|s| s.trim().to_ascii_lowercase()),
789807
no_base_prompt: args.no_base_prompt,
790-
base_prompt_file: args.base_prompt_file,
808+
base_prompt_content,
791809
};
792810

793811
Ok(config)
@@ -1150,7 +1168,7 @@ mod tests {
11501168
relay_observer: false,
11511169
agent_owner: None,
11521170
no_base_prompt: false,
1153-
base_prompt_file: None,
1171+
base_prompt_content: None,
11541172
}
11551173
}
11561174

crates/sprout-acp/src/lib.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use pool::{
2323
AgentPool, CancelMode, OwnedAgent, PromptContext, PromptOutcome, PromptResult, PromptSource,
2424
SessionState,
2525
};
26-
use queue::{EventQueue, QueuedEvent, ThreadTags};
26+
use queue::{prepend_base_prompt, EventQueue, QueuedEvent, ThreadTags};
2727
use relay::{HarnessRelay, RelayEventPublisher};
2828
use sprout_core::kind::{
2929
KIND_MEMBER_ADDED_NOTIFICATION, KIND_MEMBER_REMOVED_NOTIFICATION, KIND_STREAM_MESSAGE,
@@ -792,7 +792,7 @@ async fn tokio_main() -> Result<()> {
792792
.compact()
793793
.init();
794794

795-
let config = Config::from_cli().map_err(|e| anyhow::anyhow!("configuration error: {e}"))?;
795+
let mut config = Config::from_cli().map_err(|e| anyhow::anyhow!("configuration error: {e}"))?;
796796
tracing::info!("sprout-acp starting: {}", config.summary());
797797

798798
let observer = config
@@ -1067,6 +1067,7 @@ async fn tokio_main() -> Result<()> {
10671067
let dedup_mode = config.dedup_mode;
10681068
let mut queue = EventQueue::new(dedup_mode);
10691069

1070+
let base_prompt_content = config.base_prompt_content.take();
10701071
let ctx = Arc::new(PromptContext {
10711072
mcp_servers: build_mcp_servers(&config),
10721073
initial_message: config.initial_message.clone(),
@@ -1076,10 +1077,7 @@ async fn tokio_main() -> Result<()> {
10761077
system_prompt: config.system_prompt.clone(),
10771078
base_prompt: if config.no_base_prompt {
10781079
None
1079-
} else if let Some(ref path) = config.base_prompt_file {
1080-
let content = std::fs::read_to_string(path).unwrap_or_else(|e| {
1081-
panic!("failed to read base prompt file {}: {e}", path.display())
1082-
});
1080+
} else if let Some(content) = base_prompt_content {
10831081
Some(Box::leak(content.into_boxed_str()))
10841082
} else {
10851083
Some(include_str!("base_prompt.md"))
@@ -2273,7 +2271,7 @@ fn dispatch_heartbeat(
22732271
.clone()
22742272
.unwrap_or_else(default_heartbeat_prompt);
22752273
let prompt_text = match ctx.base_prompt {
2276-
Some(bp) => format!("[Base]\n{}\n\n{prompt_text}", bp.trim_end()),
2274+
Some(bp) => prepend_base_prompt(bp, &prompt_text),
22772275
None => prompt_text,
22782276
};
22792277
let result_tx = pool.result_tx();
@@ -2770,7 +2768,7 @@ mod build_mcp_servers_tests {
27702768
relay_observer: false,
27712769
agent_owner: None,
27722770
no_base_prompt: false,
2773-
base_prompt_file: None,
2771+
base_prompt_content: None,
27742772
}
27752773
}
27762774

crates/sprout-acp/src/pool.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ use crate::acp::{
3535
use crate::config::{DedupMode, PermissionMode};
3636
use crate::observer;
3737
use crate::queue::{
38-
ContextMessage, ConversationContext, FlushBatch, PromptChannelInfo, PromptProfile,
39-
PromptProfileLookup,
38+
prepend_base_prompt, ContextMessage, ConversationContext, FlushBatch, PromptChannelInfo,
39+
PromptProfile, PromptProfileLookup,
4040
};
4141
use crate::relay::{ChannelInfo, RestClient};
4242

@@ -187,6 +187,12 @@ pub struct PromptContext {
187187
pub dedup_mode: DedupMode,
188188
pub system_prompt: Option<String>,
189189
pub heartbeat_prompt: Option<String>,
190+
/// Base prompt content, or `None` if `--no-base-prompt` was passed.
191+
///
192+
/// `'static` because `PromptContext` is `Arc`-shared across async tasks.
193+
/// Content from `--base-prompt-file` is promoted via `Box::leak` in `main.rs`
194+
/// after validated file read in `Config::from_cli()`. The compiled-in default
195+
/// (`include_str!`) is inherently `'static`.
190196
pub base_prompt: Option<&'static str>,
191197
pub cwd: String,
192198
/// REST client for pre-prompt context fetches (thread/DM history).
@@ -749,7 +755,7 @@ pub async fn run_prompt_task(
749755
);
750756
// Prepend base prompt to initial_message for platform orientation.
751757
let init_msg = match ctx.base_prompt {
752-
Some(bp) => format!("[Base]\n{}\n\n{initial_msg}", bp.trim_end()),
758+
Some(bp) => prepend_base_prompt(bp, initial_msg),
753759
None => initial_msg.to_string(),
754760
};
755761
let init_result = agent

crates/sprout-acp/src/queue.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -952,6 +952,15 @@ pub struct FormatPromptArgs<'a> {
952952
pub profile_lookup: Option<&'a PromptProfileLookup>,
953953
}
954954

955+
/// Prepend the `[Base]` platform-context section to a prompt body.
956+
///
957+
/// Used by the heartbeat and initial-message paths so the `[Base]` format
958+
/// is defined in exactly one place. (`format_prompt` uses a sections-vec
959+
/// approach instead, but the resulting `[Base]\n{content}` format is identical.)
960+
pub fn prepend_base_prompt(base: &str, body: &str) -> String {
961+
format!("[Base]\n{}\n\n{body}", base.trim_end())
962+
}
963+
955964
/// Format a [`FlushBatch`] into a prompt string for the agent.
956965
///
957966
/// Produces a stable prompt with these sections (in order):

desktop/src-tauri/src/commands/agent_models.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ use crate::{
88
managed_agents::{
99
build_managed_agent_summary, default_agent_workdir, find_managed_agent_mut,
1010
load_managed_agents, managed_agent_avatar_url, missing_command_message,
11-
normalize_agent_args, regenerate_nest_context, resolve_command, save_managed_agents,
12-
sync_managed_agent_processes, AgentModelInfo, AgentModelsResponse,
13-
UpdateManagedAgentRequest, UpdateManagedAgentResponse,
11+
normalize_agent_args, resolve_command, save_managed_agents, sync_managed_agent_processes,
12+
try_regenerate_nest, AgentModelInfo, AgentModelsResponse, UpdateManagedAgentRequest,
13+
UpdateManagedAgentResponse,
1414
},
1515
relay::{relay_ws_url_with_override, sync_managed_agent_profile},
1616
util::now_iso,
@@ -247,9 +247,7 @@ pub async fn update_managed_agent(
247247
(summary, sync_params)
248248
}; // lock dropped here
249249

250-
if let Err(error) = regenerate_nest_context(&app) {
251-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
252-
}
250+
try_regenerate_nest(&app);
253251

254252
// Phase 2: relay profile sync (async, best-effort, outside lock)
255253
let profile_sync_error =

desktop/src-tauri/src/commands/agents.rs

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ use crate::{
88
build_managed_agent_summary, discover_provider_candidates, ensure_persona_is_active,
99
find_managed_agent_mut, invoke_provider, load_managed_agents, load_personas,
1010
managed_agent_avatar_url, managed_agent_log_path, managed_agents_base_dir,
11-
normalize_agent_args, provider_deploy, read_log_tail, regenerate_nest_context,
12-
resolve_provider_binary, save_managed_agents, start_managed_agent_process,
13-
stop_managed_agent_process, sync_managed_agent_processes, validate_provider_config,
14-
BackendKind, BackendProviderInfo, CreateManagedAgentRequest, CreateManagedAgentResponse,
11+
normalize_agent_args, provider_deploy, read_log_tail, resolve_provider_binary,
12+
save_managed_agents, start_managed_agent_process, stop_managed_agent_process,
13+
sync_managed_agent_processes, try_regenerate_nest, validate_provider_config, BackendKind,
14+
BackendProviderInfo, CreateManagedAgentRequest, CreateManagedAgentResponse,
1515
ManagedAgentLogResponse, ManagedAgentRecord, ManagedAgentSummary, DEFAULT_ACP_COMMAND,
1616
DEFAULT_AGENT_COMMAND, DEFAULT_AGENT_PARALLELISM, DEFAULT_AGENT_TURN_TIMEOUT_SECONDS,
1717
DEFAULT_MCP_COMMAND,
@@ -454,9 +454,7 @@ pub async fn create_managed_agent(
454454
(agent, spawn_error)
455455
};
456456

457-
if let Err(error) = regenerate_nest_context(&app) {
458-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
459-
}
457+
try_regenerate_nest(&app);
460458

461459
// ── Phase 4: sync agent profile on relay (async, outside lock) ───────────
462460
let avatar_url = input
@@ -722,9 +720,7 @@ pub fn delete_managed_agent(
722720
}
723721
save_managed_agents(&app, &records)?;
724722
}
725-
if let Err(error) = regenerate_nest_context(&app) {
726-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
727-
}
723+
try_regenerate_nest(&app);
728724
Ok(())
729725
}
730726

desktop/src-tauri/src/commands/personas.rs

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::{
77
managed_agents::{
88
encode_persona_json, import_persona_pack, list_installed_packs, load_managed_agents,
99
load_personas, load_teams, parse_json_persona, parse_md_persona, parse_png_persona,
10-
parse_zip_personas, regenerate_nest_context, save_managed_agents, save_personas,
10+
parse_zip_personas, save_managed_agents, save_personas, try_regenerate_nest,
1111
uninstall_persona_pack as do_uninstall_persona_pack, validate_persona_activation_change,
1212
validate_persona_deletion, CreatePersonaRequest, PackSummary, ParsePersonaFilesResult,
1313
PersonaRecord, UpdatePersonaRequest,
@@ -85,9 +85,7 @@ pub fn create_persona(
8585
};
8686
personas.push(persona.clone());
8787
save_personas(&app, &personas)?;
88-
if let Err(error) = regenerate_nest_context(&app) {
89-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
90-
}
88+
try_regenerate_nest(&app);
9189
Ok(persona)
9290
}
9391

@@ -138,9 +136,7 @@ pub fn update_persona(
138136
.into_iter()
139137
.find(|record| record.id == input.id)
140138
.ok_or_else(|| format!("persona {} disappeared unexpectedly", input.id))?;
141-
if let Err(error) = regenerate_nest_context(&app) {
142-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
143-
}
139+
try_regenerate_nest(&app);
144140
Ok(result)
145141
}
146142

@@ -186,9 +182,7 @@ pub fn delete_persona(
186182
if changed_agents {
187183
save_managed_agents(&app, &agents)?;
188184
}
189-
if let Err(error) = regenerate_nest_context(&app) {
190-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
191-
}
185+
try_regenerate_nest(&app);
192186

193187
Ok(())
194188
}
@@ -237,9 +231,7 @@ pub fn set_persona_active(
237231

238232
let updated = persona.clone();
239233
save_personas(&app, &personas)?;
240-
if let Err(error) = regenerate_nest_context(&app) {
241-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
242-
}
234+
try_regenerate_nest(&app);
243235
Ok(updated)
244236
}
245237

@@ -394,9 +386,7 @@ pub fn install_persona_pack(
394386
return Err(format!("pack path is not a directory: {path}"));
395387
}
396388
let result = import_persona_pack(&app, &source)?;
397-
if let Err(error) = regenerate_nest_context(&app) {
398-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
399-
}
389+
try_regenerate_nest(&app);
400390
Ok(result)
401391
}
402392

@@ -411,9 +401,7 @@ pub fn uninstall_persona_pack(
411401
.lock()
412402
.map_err(|e| e.to_string())?;
413403
do_uninstall_persona_pack(&app, &pack_id)?;
414-
if let Err(error) = regenerate_nest_context(&app) {
415-
eprintln!("sprout-desktop: nest context regeneration failed: {error}");
416-
}
404+
try_regenerate_nest(&app);
417405
Ok(())
418406
}
419407

desktop/src-tauri/src/commands/workspace.rs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use serde::Serialize;
33
use tauri::{AppHandle, State};
44

55
use crate::app_state::AppState;
6-
use crate::managed_agents::regenerate_nest_context;
6+
use crate::managed_agents::try_regenerate_nest;
77
use crate::relay;
88

99
#[derive(Serialize)]
@@ -53,11 +53,7 @@ pub fn apply_workspace(
5353
*keys_guard = keys;
5454
}
5555

56-
if let Err(error) = regenerate_nest_context(&app) {
57-
eprintln!(
58-
"sprout-desktop: failed to regenerate nest context after workspace switch: {error}"
59-
);
60-
}
56+
try_regenerate_nest(&app);
6157

6258
Ok(())
6359
}

desktop/src-tauri/src/lib.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ use huddle::{
2424
speak_agent_message, start_huddle, start_stt_pipeline,
2525
};
2626
use managed_agents::{
27-
ensure_nest, kill_stale_tracked_processes, load_managed_agents, regenerate_nest_context,
27+
ensure_nest, kill_stale_tracked_processes, load_managed_agents,
2828
restore_managed_agents_on_launch, save_managed_agents, sync_managed_agent_processes,
29-
BackendKind, ManagedAgentProcess,
29+
try_regenerate_nest, BackendKind, ManagedAgentProcess,
3030
};
3131
use std::sync::{
3232
atomic::{AtomicBool, Ordering},
@@ -393,9 +393,7 @@ pub fn run() {
393393
eprintln!("sprout-desktop: failed to create nest: {error}");
394394
}
395395

396-
if let Err(error) = regenerate_nest_context(&app_handle) {
397-
eprintln!("sprout-desktop: failed to regenerate nest context: {error}");
398-
}
396+
try_regenerate_nest(&app_handle);
399397

400398
// Pre-download voice models in the background so they're ready
401399
// when the user starts their first huddle. Idempotent — no-op if

0 commit comments

Comments
 (0)