Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/buzz-acp/src/acp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2655,7 +2655,7 @@ mod tests {
#[tokio::test]
async fn idle_resets_on_stdout_activity() {
// Send valid JSON (session/update notifications) to reset the idle timer.
// Non-JSON lines no longer reset idle (Finding #6 hardening).
// Non-JSON lines no longer reset idle — only valid JSON notifications do.
let mut client = spawn_script(
r#"for i in $(seq 1 10); do echo '{"jsonrpc":"2.0","method":"session/update","params":{"update":{"sessionUpdate":"agent_thought_chunk","content":{"text":"thinking"}}}}'; sleep 0.05; done; sleep 10"#,
)
Expand Down
15 changes: 4 additions & 11 deletions crates/buzz-acp/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,6 @@ impl Config {

let agent_command = args.agent_command;

// Finding #49a — agent_command must not be empty.
if agent_command.trim().is_empty() {
return Err(ConfigError::ConfigFile(
"agent_command must not be empty".into(),
Expand All @@ -807,7 +806,6 @@ impl Config {

let agent_args = normalize_agent_args(&agent_command, args.agent_args);

// Finding #49b — warn on invalid UUIDs in --channels.
if let Some(ref channels) = args.channels {
for ch in channels {
if ch.parse::<Uuid>().is_err() {
Expand All @@ -819,7 +817,6 @@ impl Config {
}
}

// Finding #49c — cap heartbeat interval at 86400s (24h).
let heartbeat_interval = if args.heartbeat_interval > 86400 {
tracing::warn!(
interval = args.heartbeat_interval,
Expand Down Expand Up @@ -885,9 +882,9 @@ impl Config {
}
};

// Finding #20 — idle_timeout must be strictly less than max_turn_duration.
// If idle_timeout >= max_turn_duration, the absolute wall-clock cap would
// fire before the idle timeout ever could, making idle_timeout a dead letter.
// idle_timeout must be strictly less than max_turn_duration. If idle_timeout
// >= max_turn_duration, the absolute wall-clock cap would fire before the idle
// timeout ever could, making idle_timeout a dead letter.
if idle_timeout_secs >= max_turn_duration_secs {
return Err(ConfigError::ConfigFile(format!(
"idle_timeout ({}s) must be less than max_turn_duration ({}s)",
Expand Down Expand Up @@ -1068,7 +1065,6 @@ pub fn load_rules(path: &std::path::Path) -> Result<Vec<SubscriptionRule>, Confi
)));
}

// Finding #49d — warn when Config mode has no rules; agent will receive nothing.
if config.rules.is_empty() {
tracing::warn!(
path = %path.display(),
Expand Down Expand Up @@ -1099,7 +1095,6 @@ pub fn load_rules(path: &std::path::Path) -> Result<Vec<SubscriptionRule>, Confi
}
// Fail fast: parse the expression at load time so typos don't
// silently produce dead rules at runtime.
// Finding #34 — store the compiled AST so match_event never re-parses.
match evalexpr::build_operator_tree(expr) {
Ok(node) => {
rule.compiled_filter = Some(Arc::new(node));
Expand All @@ -1121,9 +1116,7 @@ pub fn load_rules(path: &std::path::Path) -> Result<Vec<SubscriptionRule>, Confi
)));
}
}
// Initialise the consecutive-timeout counter (finding #25).
// Deserialization leaves it at default (new Arc<AtomicU32::new(0)>)
// but we set it explicitly here for clarity.
// Deserialization leaves consecutive_timeouts at its zero default; reset explicitly.
rule.consecutive_timeouts = Arc::new(AtomicU32::new(0));
}

Expand Down
20 changes: 10 additions & 10 deletions crates/buzz-acp/src/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,14 @@ pub struct SubscriptionRule {
/// Tag passed to the prompt template. Falls back to `name` if absent.
#[serde(default)]
pub prompt_tag: Option<String>,
/// Pre-compiled evalexpr AST for the `filter` expression (finding #34).
/// Pre-compiled evalexpr AST for the `filter` expression.
///
/// Populated by `load_rules()` at startup so `match_event` never re-parses
/// the expression string on the hot path. `None` when `filter` is `None`
/// or the rule was constructed without calling `load_rules()` (e.g. tests).
#[serde(skip)]
pub compiled_filter: Option<Arc<evalexpr::Node>>,
/// Consecutive filter-evaluation timeout counter (finding #25).
/// Consecutive filter-evaluation timeout counter.
///
/// Incremented on each timeout; reset on any successful evaluation.
/// When this reaches `MAX_CONSECUTIVE_TIMEOUTS`, the rule is treated as
Expand Down Expand Up @@ -164,7 +164,7 @@ const MAX_EXPR_LEN: usize = 4096;
/// Maximum wall-clock time allowed for a single evalexpr evaluation.
const EVAL_TIMEOUT: Duration = Duration::from_millis(100);

/// Maximum concurrent blocking filter evaluations (finding #13 / Issue 3).
/// Maximum concurrent blocking filter evaluations.
///
/// The semaphore permit is moved into each `spawn_blocking` closure so it is
/// held until the blocking thread finishes — not just until the caller's timeout
Expand All @@ -178,7 +178,7 @@ const MAX_CONCURRENT_FILTER_EVALS: usize = 4;
/// `OwnedSemaphorePermit` that can be moved into the `spawn_blocking` closure.
/// This ensures the permit is held until the blocking task actually finishes —
/// not just until the caller's timeout fires — so the semaphore truly bounds
/// the number of live blocking threads (finding #13 / Issue 3).
/// the number of live blocking threads.
static FILTER_EVAL_SEMAPHORE: std::sync::LazyLock<Arc<tokio::sync::Semaphore>> =
std::sync::LazyLock::new(|| Arc::new(tokio::sync::Semaphore::new(MAX_CONCURRENT_FILTER_EVALS)));

Expand All @@ -187,11 +187,11 @@ static FILTER_EVAL_SEMAPHORE: std::sync::LazyLock<Arc<tokio::sync::Semaphore>> =
/// - Caps expression length at [`MAX_EXPR_LEN`] bytes.
/// - Acquires an owned permit from [`FILTER_EVAL_SEMAPHORE`] and moves it into
/// the blocking closure so it is held until the task finishes, not just until
/// the caller's timeout fires (finding #13 / Issue 3).
/// the caller's timeout fires.
/// - Runs evaluation on a blocking thread with a [`EVAL_TIMEOUT`] hard timeout.
/// - When a pre-compiled `node` is provided (via `Arc`), uses
/// `node.eval_boolean_with_context()` instead of re-parsing the expression
/// string on every call (finding #34).
/// string on every call.
/// - Registers custom string helpers: `str_contains`, `str_starts_with`,
/// `str_ends_with`, `str_len` (duplicated intentionally from buzz-workflow).
pub async fn evaluate_filter(
Expand All @@ -212,7 +212,7 @@ pub async fn evaluate_filter(
// Acquire an *owned* permit so it can be moved into the spawn_blocking closure.
// The permit is held until the blocking task actually completes — not just until
// the caller's timeout fires — so the semaphore truly bounds the number of live
// blocking threads even when callers time out (Issue 3 / finding #13).
// blocking threads even when callers time out.
//
// The acquire itself is bounded by EVAL_TIMEOUT: if all permits are held by
// wedged blocking tasks, we time out instead of blocking the main event loop.
Expand Down Expand Up @@ -351,10 +351,10 @@ const MAX_CONSECUTIVE_TIMEOUTS: u32 = 5;
/// 2. **kinds** — if non-empty, the event kind must be in the list.
/// 3. **require_mention** — if `true`, a `p` tag matching `agent_pubkey_hex` must
/// exist. Tag kind is checked via `tag.as_slice()` for stable, library-independent
/// access (finding #45).
/// access.
/// 4. **filter** — if `Some`, the evalexpr expression must evaluate to `true`.
///
/// # Fail-closed filter error handling (finding #25)
/// # Fail-closed filter error handling
///
/// Any filter evaluation error — including timeout — causes the **entire
/// `match_event` call** to return `None` (no match for any rule). We never
Expand Down Expand Up @@ -386,7 +386,7 @@ pub async fn match_event(

// 3. Mention check — look for a `p` tag whose first element equals
// agent_pubkey_hex. Uses tag.as_slice() for stable, library-independent
// access (finding #45) — avoids relying on the Display impl of tag kind.
// access — avoids relying on the Display impl of tag kind.
if rule.require_mention {
let mentioned = event.tags.iter().any(|tag| {
let s = tag.as_slice();
Expand Down
27 changes: 12 additions & 15 deletions crates/buzz-acp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1157,11 +1157,10 @@ async fn tokio_main() -> Result<()> {
);
}

//
// Finding #10: one agent failing to start must not kill the whole pool.
// We attempt each spawn under a 60-second timeout; failures are logged and
// skipped. If ALL agents fail we return an error. A partial pool is valid —
// the harness continues with reduced capacity and logs a warning.
// One agent failing to start must not kill the whole pool. We attempt each
// spawn under a 60-second timeout; failures are logged and skipped. If ALL
// agents fail we return an error. A partial pool is valid — the harness
// continues with reduced capacity and logs a warning.
let mut agent_slots: Vec<Option<OwnedAgent>> = Vec::with_capacity(config.agents as usize);
for i in 0..config.agents as usize {
// Spawn OUTSIDE the timeout so we always own the child for cleanup.
Expand Down Expand Up @@ -1248,13 +1247,11 @@ async fn tokio_main() -> Result<()> {
tracing::info!("agent_pool_ready agents={}", live_count);
let mut pool = AgentPool::from_slots(agent_slots);

//
// Finding #22: capture a startup watermark BEFORE connecting to the relay.
// This timestamp is used for membership notification replay (via
// startup_watermark) and as the initial subscribe_since for channels
// discovered at startup. The Subscribe handler falls back to
// subscribe_since when last_seen is None, closing the blind spot
// between "agents ready" and "first REQ sent".
// Capture a startup watermark BEFORE connecting to the relay. This timestamp
// is used for membership notification replay (via startup_watermark) and as
// the initial subscribe_since for channels discovered at startup. The Subscribe
// handler falls back to subscribe_since when last_seen is None, closing the
// blind spot between "agents ready" and "first REQ sent".
let startup_watermark: u64 = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
Expand All @@ -1273,7 +1270,7 @@ async fn tokio_main() -> Result<()> {
.await
.map_err(|e| anyhow::anyhow!("relay connect error: {e}"))?;

// Finding #22: tell the relay background task the watermark so it can use
// Tell the relay background task the watermark so it can use
// `since = watermark - 5s` on the first REQ instead of `since=now`.
// Best-effort: a failure here is non-fatal (we just lose the startup window
// protection, which is the same as the pre-fix behaviour).
Expand Down Expand Up @@ -1697,8 +1694,8 @@ async fn tokio_main() -> Result<()> {
let (result_rx, join_set) = pool.rx_and_join_set();
tokio::select! {
biased;
// Finding #24: recv() returning None means all senders dropped
// (pool was torn down). Break cleanly instead of panicking.
// recv() returning None means all senders dropped (pool was torn down).
// Break cleanly instead of panicking.
r = result_rx.recv() => match r {
Some(result) => Some(PoolEvent::Result(Box::new(result))),
None => {
Expand Down
Loading
Loading