Conversation
…ions Adds a central call_tool override that injects next-step hints via a table-driven next_hint() lookup (item #39), and wires MCP progress notifications into create_worktree/push_and_open_pr (item #40). Progress attribution is scoped with a tokio task_local (PROGRESS_SENDER) rather than a server-wide Mutex<Option<ProgressSender>> field, so a call without its own progress token can never pick up a stale sender left by a prior request, and concurrent calls can't overwrite each other's sender.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe change adds task-scoped MCP progress notifications for worktree operations and centralizes ChangesProgress and protocol integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant ServerHandler
participant WorktreeOperations
participant ProgressSender
participant RMCPPeer
MCPClient->>ServerHandler: call item tool
ServerHandler->>WorktreeOperations: claim or complete item
WorktreeOperations->>ProgressSender: send lifecycle progress
ProgressSender->>RMCPPeer: notify_progress
WorktreeOperations-->>ServerHandler: return tool JSON
ServerHandler-->>MCPClient: response with optional next guidance
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mcp_server.rs (3)
2571-2577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate task-local-with-fallback boilerplate between claim and done.
Both sites repeat the identical
PROGRESS_SENDER.try_with(|ps| ...).unwrap_or_else(|_| ... None)shape, differing only in which worktree function is invoked. Extract a small helper to avoid the duplication.♻️ Proposed refactor
fn with_progress<T>(f: impl Fn(Option<&ProgressSender>) -> T) -> T { PROGRESS_SENDER.try_with(|ps| f(ps.as_ref())).unwrap_or_else(|_| f(None)) }- let worktree_path = match (&item, &target_branch) { - (Some(item), Some(target)) => PROGRESS_SENDER - .try_with(|ps| { - crate::worktree::create_worktree(item, &repo_root, target, ps.as_ref()) - }) - .unwrap_or_else(|_| { - crate::worktree::create_worktree(item, &repo_root, target, None) - }), - _ => None, - }; + let worktree_path = match (&item, &target_branch) { + (Some(item), Some(target)) => with_progress(|ps| { + crate::worktree::create_worktree(item, &repo_root, target, ps) + }), + _ => None, + };Apply the analogous change to the
donepath (Lines 2657-2663) callingpush_and_open_pr.Also applies to: 2657-2663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_server.rs` around lines 2571 - 2577, Extract a shared with_progress helper for the repeated PROGRESS_SENDER.try_with(...).unwrap_or_else(... None) pattern in the claim and done paths. Define it near the relevant progress handling code, then use it for create_worktree and push_and_open_pr while preserving each operation’s existing arguments and fallback behavior.
3388-3416: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
call_toolpays a JSON round-trip cost on every single tool call, even onesnext_hintcan never annotate.
serde_json::to_value(&result.content)(and the subsequent parse of the text payload) runs unconditionally for all ~30 tools, most of which always hitnext_hint's_ => Nonearm. For tools with sizeable text payloads (e.g.asset's inline base64 content up to 1MB,artifact_get,skill_load,gateway_execute), this adds an avoidable serialize/deserialize/reserialize pass on the hot dispatch path for every request.⚡ Proposed fix: gate on tool_name before touching content
let mut result = PROGRESS_SENDER .scope(progress_sender, Self::tool_router().call(tcc)) .await?; + // next_hint only ever returns Some for these tools — skip the JSON + // round-trip entirely for everything else. + if !matches!(tool_name.as_str(), "item" | "handoff") { + return Ok(result); + } let mut content_json = serde_json::to_value(&result.content).unwrap_or(serde_json::Value::Null);Note this hardcodes the tool-name check in two places (
matchhere and insidenext_hint); consider deriving both from one source of truth if the mapping grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_server.rs` around lines 3388 - 3416, Update call_tool so content serialization, text parsing, hint insertion, and content reconstruction run only for tool names that can produce a next hint, rather than for every dispatch. Reuse a shared tool-name eligibility source with next_hint where practical, keeping existing annotation behavior unchanged for eligible tools and avoiding all JSON round trips for ineligible tools.
3401-3414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJSON-mutation logic inside
call_toolhas no direct test coverage.Only
next_hint()itself is unit-tested (Lines 5722-5759); the surrounding parse/inject/reserialize logic that mutatesresult.contentis exercised only indirectly, and — per the existing comment onRequestContext<RoleServer>/Peerconstruction elsewhere in this file — an end-to-endcall_tooltest isn't feasible without theclientfeature. Extracting this logic into a plain function overVec<Content>(or the JSONValue) would let it be unit-tested without needing aRequestContext.fn inject_next_hint(content: &mut serde_json::Value, tool_name: &str) { if let Some(arr) = content.as_array_mut() && let Some(first) = arr.first_mut() && let Some(text) = first.get("text").and_then(|v| v.as_str()) && let Ok(mut json) = serde_json::from_str::<serde_json::Value>(text) && let Some(hint) = next_hint(tool_name, &json) && let serde_json::Value::Object(ref mut map) = json { map.insert("next".into(), serde_json::Value::String(hint)); first["text"] = serde_json::Value::String( serde_json::to_string_pretty(&map).unwrap_or_else(|_| text.into()), ); } }
call_toolwould then just callinject_next_hint(&mut content_json, &tool_name)and re-deriveresult.contentfrom it, and a unit test could build a smallserde_json::Valuearray directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_server.rs` around lines 3401 - 3414, Extract the JSON parse, next-hint injection, and reserialization logic from call_tool into a plain inject_next_hint function accepting mutable serde_json::Value and tool_name. Have call_tool invoke this helper before converting content_json back into result.content, preserving current behavior for non-array, invalid, or non-object content. Add direct unit tests for the helper using a small JSON array, covering successful injection and unchanged cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/worktree.rs`:
- Around line 189-220: Replace the blocking .output() calls used by the worktree
command flows with run_output_timeout, including git worktree add, git push, and
gh pr create. Pass each command’s existing arguments, working directory, and
appropriate timeout_secs, while preserving their current output handling and
error propagation.
---
Nitpick comments:
In `@src/mcp_server.rs`:
- Around line 2571-2577: Extract a shared with_progress helper for the repeated
PROGRESS_SENDER.try_with(...).unwrap_or_else(... None) pattern in the claim and
done paths. Define it near the relevant progress handling code, then use it for
create_worktree and push_and_open_pr while preserving each operation’s existing
arguments and fallback behavior.
- Around line 3388-3416: Update call_tool so content serialization, text
parsing, hint insertion, and content reconstruction run only for tool names that
can produce a next hint, rather than for every dispatch. Reuse a shared
tool-name eligibility source with next_hint where practical, keeping existing
annotation behavior unchanged for eligible tools and avoiding all JSON round
trips for ineligible tools.
- Around line 3401-3414: Extract the JSON parse, next-hint injection, and
reserialization logic from call_tool into a plain inject_next_hint function
accepting mutable serde_json::Value and tool_name. Have call_tool invoke this
helper before converting content_json back into result.content, preserving
current behavior for non-array, invalid, or non-object content. Add direct unit
tests for the helper using a small JSON array, covering successful injection and
unchanged cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f67c3764-c34d-45a8-b0be-7deb6ce29b39
📒 Files selected for processing (4)
src/main.rssrc/mcp_server.rssrc/progress.rssrc/worktree.rs
Comment on lines
+189
to
+220
| /// Runs `program` with a deadline, returning its output. Spawns the | ||
| /// child, waits on a background thread, and returns a timeout error | ||
| /// if it doesn't finish in time. The spawned thread eventually cleans | ||
| /// up — no orphaned processes, just a late reaping. | ||
| fn run_output_timeout( | ||
| program: &str, | ||
| args: &[&str], | ||
| cwd: &Path, | ||
| timeout_secs: u64, | ||
| ) -> Result<std::process::Output, String> { | ||
| let child = Command::new(program) | ||
| .args(args) | ||
| .current_dir(cwd) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn() | ||
| .map_err(|e| format!("{program}: spawn failed: {e}"))?; | ||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
| std::thread::spawn(move || { | ||
| let result = child.wait_with_output(); | ||
| let _ = tx.send(result); | ||
| }); | ||
| match rx.recv_timeout(Duration::from_secs(timeout_secs)) { | ||
| Ok(result) => result, | ||
| Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { | ||
| Err(format!("{program} timed out after {timeout_secs}s")) | ||
| } | ||
| Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { | ||
| Err("child thread panicked".into()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm run_output_timeout has no call sites anywhere in the repo.
rg -n --type=rust '\brun_output_timeout\s*\(' src/Repository: getappz/agentflare
Length of output: 199
Wire the timeout helper into the worktree commands run_output_timeout is never called, so git worktree add, git push, and gh pr create still use blocking .output() calls and can hang indefinitely.
🧰 Tools
🪛 GitHub Actions: ci / 1_clippy.txt
[error] 212-212: Rust compilation failed (E0308): mismatched types in function returning Result<std::process::Output, String>. The match arm Ok(result) => result returns Result<Output, Error> (expected Result<Output, String>, found Result<Output, std::io::Error>).
🪛 GitHub Actions: ci / 3_build (macos-latest).txt
[error] 212-212: Rust compilation failed with error[E0308]: mismatched types. Function expected Result<std::process::Output, String> but got Result<Output, std::io::Error> at Ok(result) => result,.
🪛 GitHub Actions: ci / 4_fmt.txt
[error] 213-215: cargo fmt --check failed due to formatting differences. Rustfmt expected the Disconnected match arm to be formatted as a single line: Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => Err("child thread panicked".into()),.
🪛 GitHub Actions: ci / 5_build (ubuntu-latest).txt
[error] 212-212: Rust compilation error E0308 (mismatched types). Function expected Result<std::process::Output, String> but returned Result<Output, std::io::Error> at Ok(result) => result. Compiler: expected Result<Output, String>, found Result<Output, Error>.
🪛 GitHub Actions: ci / build (macos-latest)
[error] 198-212: Rust compile error (E0308): mismatched types. Function returns Result<std::process::Output, String>, but Ok(result) contains Result<Output, std::io::Error>. The Ok(result) => result branch returns the wrong error type (std::io::Error instead of String).
🪛 GitHub Actions: ci / build (ubuntu-latest)
[error] 212-212: rustc E0308: mismatched types. Function expected Result<std::process::Output, String> but Ok(result) => result returns Result<Output, std::io::Error>.
🪛 GitHub Actions: ci / clippy
[error] 212-212: Rust compilation failed (E0308): mismatched types in return value. Function returns Result<Output, String>, but code returns Result<Output, io::Error>.
[error] 198-198: Type mismatch expected Result<std::process::Output, String> because of function signature, but found Result<std::process::Output, io::Error> in match arm.
🪛 GitHub Actions: ci / fmt
[error] 213-213: cargo fmt --check failed. Formatting diff: the match arm Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { Err("child thread panicked".into()) } needs to be formatted as a single line: Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => Err("child thread panicked".into()),.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/worktree.rs` around lines 189 - 220, Replace the blocking .output() calls
used by the worktree command flows with run_output_timeout, including git
worktree add, git push, and gh pr create. Pass each command’s existing
arguments, working directory, and appropriate timeout_secs, while preserving
their current output handling and error propagation.
…here Swept in from a concurrent uncommitted edit in the shared working tree (item #38's timeout work, not part of this PR's scope) when this branch was first committed. It was never called anywhere on this branch and failed to compile (CI: E0308 mismatched types), which is exactly the bug item #38's own work later fixed on its own branch.
4 tasks
getappz
added a commit
that referenced
this pull request
Jul 26, 2026
ArtifactStore gains with_store(Store) constructor backed by documents + blobs (content-addressed, gzip, deduped). Metadata serialized to documents.metadata JSON. Version history via doc_history. FTS5 search replaces flat-file scan (fixes #180). Dashboard serves artifacts under /artifacts/ via axum routes (index, artifact page, version page, versions JSON, SSE live). No more random-port URL — uses dashboard port. Integration: ensure_artifact_server, CLI handoff, standalone serve all default to store backend, fall back to flat-file. 32 tests pass (26 original + 6 new store-backed).
getappz
added a commit
that referenced
this pull request
Jul 26, 2026
…story tracking (#335) * store: migrate artifact storage onto agentflare-store documents+blobs ArtifactStore gains with_store(Store) constructor backed by documents + blobs (content-addressed, gzip, deduped). Metadata serialized to documents.metadata JSON. Version history via doc_history. FTS5 search replaces flat-file scan (fixes #180). Dashboard serves artifacts under /artifacts/ via axum routes (index, artifact page, version page, versions JSON, SSE live). No more random-port URL — uses dashboard port. Integration: ensure_artifact_server, CLI handoff, standalone serve all default to store backend, fall back to flat-file. 32 tests pass (26 original + 6 new store-backed). * hook_redirect: block agent shell commands from deleting agentflare db files opencode ran an rm mid-migration and silently wiped store.db's metadata, recovered by hand from a pre-migration flat-file backup that happened to still exist. Extend the shared PreToolUse classifier to deny destructive shell commands targeting agentflare's own db files or the data dir itself, for Bash and PowerShell tool calls. * store: fix doc_history skip check for blob-backed callers, clippy nits DocUpsertOpts gained a track_history field upstream since this branch's artifact-store migration was written; doc_upsert_with_opts's history-skip check compared old_content != content, but blob-backed callers (artifacts) always pass an empty content string and store the real payload via blob_hash, so the check was always a no-op false and history rows never got recorded. Compare blob_hash too. Also fixes two clippy findings (io_other_error, needless_borrow) surfaced by -D warnings. * coaching: embedded agent-managed rules with tier/sync, materialization, staleness, CLI RuleTier (Builtin/Override), sync fields on CoachingRule, parse/write. apply_rule/remove_rule carry tier+sync, sync_targets_for_host query. rule_targets merges coaching rules per host (per-file or joined). Snapshot previous body on builtin overwrite; is_stale_rule checks it. sync_now/unsync_host for immediate materialization. CLI: --tier/--sync flags, sync subcommand. Tested: 112 pass across coaching/components/init/hook. * coaching: prune stale coaching-rule entries from opencode.jsonc wire_opencode_instructions's doc comment promised removing entries for rules no longer synced, but only the hardcoded legacy engram.md path was ever pruned -- unsync_host deleting a coaching rule's file left a dangling reference to it in opencode.jsonc forever. Retain only array entries under our rules_dir whose filename is still expected. * fmt + clippy fixes for CI Fixes the fmt and clippy failures on PR #335, introduced by the coaching tier/sync commits added after the PR's original CI pass: - cargo fmt --all across the files rustfmt flagged (hook_redirect.rs, coaching/{cli,mod,rule,store}.rs, cli/coaching.rs, components.rs, dashboard/artifacts.rs, init.rs, mcp_server.rs, agentflare-artifacts/store.rs) - coaching/cli.rs: surface CoachingRule's tier and applied_at fields in `agentflare coaching list` output (was clippy dead_code; also useful to a user checking whether a rule is builtin vs override, and when it was applied) - coaching/store.rs: drop needless borrow in create_dir_all(rules_dir()) - init.rs: use Vec::contains instead of iter().any() for a plain equality check
getappz
added a commit
that referenced
this pull request
Jul 26, 2026
* store: migrate artifact storage onto agentflare-store documents+blobs ArtifactStore gains with_store(Store) constructor backed by documents + blobs (content-addressed, gzip, deduped). Metadata serialized to documents.metadata JSON. Version history via doc_history. FTS5 search replaces flat-file scan (fixes #180). Dashboard serves artifacts under /artifacts/ via axum routes (index, artifact page, version page, versions JSON, SSE live). No more random-port URL — uses dashboard port. Integration: ensure_artifact_server, CLI handoff, standalone serve all default to store backend, fall back to flat-file. 32 tests pass (26 original + 6 new store-backed). * hook_redirect: block agent shell commands from deleting agentflare db files opencode ran an rm mid-migration and silently wiped store.db's metadata, recovered by hand from a pre-migration flat-file backup that happened to still exist. Extend the shared PreToolUse classifier to deny destructive shell commands targeting agentflare's own db files or the data dir itself, for Bash and PowerShell tool calls. * store: fix doc_history skip check for blob-backed callers, clippy nits DocUpsertOpts gained a track_history field upstream since this branch's artifact-store migration was written; doc_upsert_with_opts's history-skip check compared old_content != content, but blob-backed callers (artifacts) always pass an empty content string and store the real payload via blob_hash, so the check was always a no-op false and history rows never got recorded. Compare blob_hash too. Also fixes two clippy findings (io_other_error, needless_borrow) surfaced by -D warnings. * coaching: embedded agent-managed rules with tier/sync, materialization, staleness, CLI RuleTier (Builtin/Override), sync fields on CoachingRule, parse/write. apply_rule/remove_rule carry tier+sync, sync_targets_for_host query. rule_targets merges coaching rules per host (per-file or joined). Snapshot previous body on builtin overwrite; is_stale_rule checks it. sync_now/unsync_host for immediate materialization. CLI: --tier/--sync flags, sync subcommand. Tested: 112 pass across coaching/components/init/hook. * coaching: prune stale coaching-rule entries from opencode.jsonc wire_opencode_instructions's doc comment promised removing entries for rules no longer synced, but only the hardcoded legacy engram.md path was ever pruned -- unsync_host deleting a coaching rule's file left a dangling reference to it in opencode.jsonc forever. Retain only array entries under our rules_dir whose filename is still expected. * task/366: wip config_loader scaffold * flare-git-core: add config_loader for ~/.agentflare/config.toml * flare-git-core: add policy_config to merge git_shim config.toml layers * flare-git-core: thread ResolvedGitShimPolicy through classify_pure and resolve_trust_root_touch * flare-git-core: end-to-end test for config.toml relaxing git-shim policy * fmt: apply cargo fmt and sync Cargo.lock for new deps CI's fmt job was failing on unformatted coaching/dashboard/mcp_server changes, and clippy's --locked check was failing because Cargo.lock hadn't been regenerated after adding toml/thiserror/agentflare-store/ blake3 as dependencies. * flare-git-core: box toml::de::Error in LoaderError to fix clippy::result_large_err toml::de::Error is >128 bytes, so embedding it directly in LoaderError tripped clippy::result_large_err (denied via -D warnings) on every function returning Result<_, LoaderError>. * paths: fix test-isolation race in with_temp_home/with_temp_cwd Root cause of the intermittent build (windows-latest) CI failures in state::tests::* and vent::capture::tests::*: with_temp_home/with_temp_cwd reused a single fixed directory name across every call. A mutex serialized the env-var mutation itself, but under cargo test's default parallel runner and heavy concurrent filesystem load elsewhere in the 811-test suite, a previous call's directory could still be non-empty (or its file handles not yet released) by the time the next call reused the same path, leaking persisted state (SQLite store contents, vent log entries) from one test into an unrelated one. Fixed by giving each call a uniquely-named tempfile::tempdir() instead of a shared fixed name, so no two calls can ever collide on the same directory regardless of timing. Also made both helpers panic-safe via Drop guards, so a failing assertion inside the wrapped closure can no longer leave AGENTFLARE_HOME_OVERRIDE (or the cwd) permanently altered for whatever the test binary runs next. Verified: 7 consecutive clean cargo test --workspace / -p agentflare runs (0 failures) after the fix, versus 100% reproducible failure before it. Added two regression tests exercising with_temp_home under real thread contention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements item #39 (central call_tool hook for next hints) and #40 (progress notifications wired into create_worktree/push_and_open_pr), reviewed under item #44.
Review found the initial implementation stored the per-call ProgressSender in a server-wide Mutex<Option> field, which let a stale sender leak into a later call missing its own progress token, and let concurrent calls race on the same field. Fixed by scoping it with a tokio task_local (PROGRESS_SENDER) instead, plus two regression tests covering the isolation.
Verification: 430/430 tests pass, cargo clippy clean, cargo fmt clean.
Summary by CodeRabbit