Conversation
) Replace three independent copy-pasted agentic loops (dispatcher, worker, container runtime) with a single shared engine in `agentic_loop.rs` that all consumers customize via the `LoopDelegate` trait. Phase 1 — Shared engine (`src/agent/agentic_loop.rs`, 205 lines): - `run_agentic_loop()` owns the core LLM → tool exec → repeat cycle - `LoopDelegate` trait (Send + Sync, &dyn dispatch) with 6 hook points - Tool intent nudge logic consolidated (was duplicated in 3 files) - Iteration limit + force-text behavior preserved Phase 2 — Three delegate implementations: - `ChatDelegate` (dispatcher.rs): 3-phase approval flow, hooks, cost guard, context compaction, skill attenuation, interruption - `JobDelegate` (worker/job.rs): planning pre-loop phase, parallel JoinSet exec, mark_completed/stuck/failed, SSE streaming, self-repair - `ContainerDelegate` (worker/container.rs): sequential tool exec, HTTP-proxied LLM, container-safe tools, credential injection Phase 3 — File moves and cleanup: - Delete `src/agent/worker.rs` — job logic moved to `src/worker/job.rs` - Rename `src/worker/runtime.rs` → `src/worker/container.rs` - Re-export `Worker`/`WorkerDeps` from `crate::worker` in `agent/mod.rs` - Update `scheduler.rs` imports to new worker location Shared helpers (`src/tools/execute.rs`): - `execute_tool_with_safety()` replaces 4 copies of validate → timeout → execute → serialize - `process_tool_result()` replaces 3 copies of sanitize → wrap → ChatMessage (also used by thread_ops.rs approval resume paths) Net result: -2,408 lines, zero duplicated loop logic, single code path for tool intent nudge and completion detection. Closes #654 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. scheduler.rs: Replace `unwrap_or` fallback with proper error propagation when parsing tool output JSON — surfaces bugs instead of silently changing the output type. 2. worker/job.rs: Drop MutexGuard before the cancellation `.await` in `check_signals()` to avoid holding a lock across an async I/O call (prevents `await_holding_lock` lint). 3. worker/job.rs: Restore consecutive rate-limit counter (MAX_CONSECUTIVE_RATE_LIMITS = 10) so sustained rate limiting marks the job stuck with "Persistent rate limiting" instead of silently burning through max_iterations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge staging's changes into the refactored JobDelegate: - Add token budget tracking in call_llm (update_context/add_tokens) - mark_stuck → mark_failed for iteration cap and rate-limit exhaustion (aligns with staging's #788 fix) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address all 6 review points from zmanian on PR #800: 1. Replace LoopOutcome::Custom(Box<dyn Any>) with typed LoopOutcome::NeedApproval(Box<PendingApproval>) — eliminates type erasure and downcast, resolves clippy large_enum_variant. 2. Remove dead max_tool_iterations field from ChatDelegate struct. 3. Add on_tool_intent_nudge() hook to LoopDelegate trait with implementations in Job and Container delegates for observability. 4. Fix SSE events in job worker to emit raw sanitized content instead of XML-wrapped <tool_output> tags. 5. Remove 4 duplicate completion tests from job.rs that were already covered by the shared util module. 6. Avoid logging full tool results — use result_size_bytes in debug logs (execute.rs, job.rs). Also updates path references in CLAUDE.md, COVERAGE_PLAN.md, and add-sse-event.md command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 16 tests covering the two new critical shared modules: agentic_loop.rs (10 tests): - Text response exits loop immediately - Tool call → text response continuation - LoopSignal::Stop exits before LLM call - LoopSignal::InjectMessage adds user message to context - Max iterations terminates with LoopOutcome::MaxIterations - Tool intent nudge fires twice then caps - before_llm_call early exit bypasses LLM - truncate_for_preview: short string, long string, multibyte safety execute.rs (6 tests): - execute_tool_with_safety success path - Missing tool returns ToolError::NotFound - Tool execution failure propagates - Per-tool timeout enforcement (50ms) - process_tool_result XML wrapping on success - process_tool_result error formatting All 2,777 unit tests pass, 0 clippy warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ntic-loops # Conflicts: # src/main.rs # src/worker/mod.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…container
CRITICAL fixes:
- Rate-limit exhaustion now returns Err(LlmError::RateLimited) instead of
Ok(Text("")), stopping the loop immediately with no ghost iteration.
Below-threshold retries still use Text("") with an explicit empty-string
guard in handle_text_response to skip injection.
- check_signals drains the entire message channel before returning,
prioritizing Stop over UserMessage. Previously returned early on first
UserMessage, silently dropping any queued Stop or additional messages.
- check_signals now detects all non-progressing job states (Cancelled,
Failed, Stuck, Completed, Submitted, Accepted) instead of only
Cancelled and Failed.
HIGH fixes:
- Error path in process_tool_result_job applies truncate_for_preview to
bound error strings in SSE/DB events (was unbounded).
- Document Send+Sync lifetime constraint on LoopDelegate trait.
- Test mock before_llm_call refactored from double-lock to single lock
acquisition, eliminating deadlock risk on refactor.
MEDIUM fixes:
- CompletionReport includes actual iteration count via shared
Arc<Mutex<u32>> tracker (was hardcoded 0).
- process_tool_result_job return type changed from Result<bool> to
Result<()> — the bool was always false (dead API).
- Deduplicate truncate in container.rs; now uses truncate_for_preview
from agentic_loop.
Verified: 0 clippy warnings, 2781 tests pass, cargo fmt clean.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflicts: - dispatcher.rs: take PR version (delegate-based loop already includes cost guardrails and nudge) - runtime.rs: accept deletion (worker runtime moved into unified loop) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
scope: agent
|
Superseded by combined PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the core agentic reasoning and execution logic by unifying three previously distinct loops (for chat, background jobs, and container workers) into a single, reusable Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by unifying the three agentic loops (chat, job, container) into a single engine driven by a LoopDelegate trait, greatly reducing code duplication and improving overall architecture. The introduction of a shared execute_tool_with_safety function is a good step towards consolidation. My review focuses on ensuring this new abstraction is applied consistently and correctly, particularly in areas concerning duplicated tool execution logic and precise job state management, aligning with established repository rules for code quality and resource handling.
Note: Security Review did not run due to the size of the PR.
| let result_size = serde_json::to_string(&output.result) | ||
| .map(|s| s.len()) | ||
| .unwrap_or(0); | ||
| tracing::debug!( | ||
| tool = %tool_name, | ||
| elapsed_ms = elapsed.as_millis() as u64, | ||
| result = %result_str, | ||
| result_size_bytes = result_size, | ||
| "Tool call succeeded" | ||
| ); |
There was a problem hiding this comment.
This function (execute_tool_inner) duplicates a significant amount of logic from the new execute_tool_with_safety function in src/tools/execute.rs. To align with the goal of this PR to unify logic, this function should be refactored to use the shared tool execution logic after performing its job-specific pre-flight checks (approval, rate limiting, hooks). This might require adjusting execute_tool_with_safety to return more information (like the ToolOutput and execution duration) to allow for action recording.
References
- When an issue is found in duplicated code, prefer refactoring into a shared function over applying localized fixes.
| | JobState::Submitted | ||
| | JobState::Accepted |
There was a problem hiding this comment.
Including JobState::Submitted and JobState::Accepted in this check seems incorrect. A worker should only be active for a job in an InProgress state. Stopping the loop for jobs in these initial states might mask a potential logic error in the scheduler. This check should likely only include terminal or stuck states (Cancelled, Failed, Stuck, Completed). This aligns with the principle of distinguishing between truly active states and terminal or intermediate states to ensure proper resource management and prevent orphaned resources.
References
- When reaping resources based on job state, distinguish between truly active states (e.g.,
InProgress,Stuck) and terminal or intermediate states (e.g.,Completed,Submitted). Do not skip cleanup for jobs in terminal or intermediate states, as their resources may be orphaned.
2 participants
Summary
Rebased version of #800 with merge conflicts resolved against current staging.
src/agent/dispatcher.rs(delegate-based loop already includes cost guardrails and nudge from staging)src/worker/runtime.rs(worker runtime moved into unified loop)Original PR: #800 by @qbit-glitch
Supersedes: #800
Test plan
Generated with Claude Code