Conversation
…it, not just timeouts Agentflare-Agent: claude-code_2-1-226_agent Agentflare-Branch: task/43 Agentflare-Item: 43
item::claim() deliberately stores the raw claim owner (<agent>:<instance>) into assignee_agent on acquire, not just the bare agent name — intentional, pinned by existing tests. But resolve_confirmed_agent (supervisor.rs's discovery tick) and resolve_agent's assignee fallback (cli/work.rs) matched that field against the agent registry with an exact string comparison, so once an item had been claimed at least once, its own assignee could never be recognized again on a later dispatch attempt — it would silently get skipped or fall through to other routing rules. Both now reuse item::agent_part (already used internally by claim()'s own handoff-freeze comparison) to strip the instance suffix before matching, instead of duplicating that logic or changing the write side. Agentflare-Agent: claude-code_2-1-226_agent Agentflare-Branch: task/43 Agentflare-Item: 43
…LARE_WORK_MAX_CONCURRENCY Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…etry instead of requeuing instantly Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…retry delay to a failure Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…reuse the same classifier Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…sting cooldown table Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…try delay to the job queue Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
…imit cooldown Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds structured job failures with optional retry delays, persists delayed retry eligibility, classifies rate-limit failures, records authentication cooldowns, and prevents dispatch during cooldowns. It also canonicalizes suffixed agent identifiers, includes headless diagnostics, and configures worker concurrency. ChangesRetry-aware agent execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkArgs
participant auth_runner
participant auth_db
participant InProcessExecutor
participant Worker
participant Queue
participant Supervisor
WorkArgs->>auth_runner: Classify failure output
WorkArgs->>auth_db: Record rate-limit cooldown
WorkArgs->>InProcessExecutor: Return WorkOutcome
InProcessExecutor->>Worker: Return JobFailure with retry delay
Worker->>Queue: Store failure and retry delay
Supervisor->>auth_db: Check agent cooldown
Supervisor->>Queue: Enqueue eligible work
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/agentflare-jobs/src/queue.rs`:
- Around line 213-218: Update the retry scheduling logic around retry_after_secs
to safely convert the u64 delay to i64 and use saturating timestamp addition,
preventing oversized delays from becoming immediately eligible. Preserve normal
delays, and add a regression test covering Some(u64::MAX) that verifies the
queued job is not immediately retried.
In `@src/agent_launch.rs`:
- Around line 378-382: Update diagnostic_suffix and its callers in
classify_and_cooldown so non-empty stdout does not suppress stderr; include
bounded diagnostics from both output streams, preserving the existing size
limits. Add a test covering non-empty stdout combined with rate-limit stderr and
verify the failure is classified as a rate limit.
In `@src/auth_runner.rs`:
- Around line 74-76: Update is_rate_limited to require a rate-limit-specific
indicator alongside any generic “try again” match, preventing unrelated failures
from being classified as rate limited. Adjust RATE_LIMIT_PATTERNS or the
matching logic while preserving valid rate-limit detection, and add a negative
test covering a non-rate-limit failure containing “try again”.
🪄 Autofix
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
Run ID: 67b24ff2-af5d-411c-8058-39b977b79527
📒 Files selected for processing (13)
crates/agentflare-backend/src/item.rscrates/agentflare-jobs/src/executor.rscrates/agentflare-jobs/src/lib.rscrates/agentflare-jobs/src/queue.rscrates/agentflare-jobs/src/worker.rscrates/agentflare-jobs/tests/in_process_test.rscrates/agentflare-jobs/tests/queue_test.rssrc/agent_launch.rssrc/auth_db.rssrc/auth_runner.rssrc/cli/work.rssrc/dashboard/server.rssrc/supervisor.rs
| let not_before = retry_after_secs.map(|s| now + s as i64); | ||
| conn.execute( | ||
| "UPDATE agent_jobs | ||
| SET state = 'queued', retries = retries + 1, error = ?1, started_at = NULL | ||
| WHERE id = ?2", | ||
| params![error, id], | ||
| SET state = 'queued', retries = retries + 1, error = ?1, started_at = NULL, not_before = ?2 | ||
| WHERE id = ?3", | ||
| params![error, not_before, id], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent an oversized retry delay from becoming an immediate retry.
Line 213 casts u64 to i64 without validation. A delay above i64::MAX becomes negative. The queued job then satisfies not_before <= now and retries immediately. Use checked conversion and saturating timestamp arithmetic. Add a regression test with Some(u64::MAX).
Proposed fix
- let not_before = retry_after_secs.map(|s| now + s as i64);
+ let not_before = retry_after_secs.map(|seconds| {
+ now.saturating_add(i64::try_from(seconds).unwrap_or(i64::MAX))
+ });🤖 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 `@crates/agentflare-jobs/src/queue.rs` around lines 213 - 218, Update the retry
scheduling logic around retry_after_secs to safely convert the u64 delay to i64
and use saturating timestamp addition, preventing oversized delays from becoming
immediately eligible. Preserve normal delays, and add a regression test covering
Some(u64::MAX) that verifies the queued job is not immediately retried.
| Ok(c) => HeadlessOutcome::Failed(format!( | ||
| "{} exited non-zero{}", | ||
| spec.display_name, | ||
| diagnostic_suffix(&c) | ||
| )), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve stderr when stdout is also present.
diagnostic_suffix omits stderr when stdout is non-empty. If an agent writes normal stdout and a 429 response to stderr, classify_and_cooldown cannot detect the rate limit. Include bounded diagnostics from both streams. Add a test with non-empty stdout and rate-limit stderr.
Also applies to: 870-889
🤖 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/agent_launch.rs` around lines 378 - 382, Update diagnostic_suffix and its
callers in classify_and_cooldown so non-empty stdout does not suppress stderr;
include bounded diagnostics from both output streams, preserving the existing
size limits. Add a test covering non-empty stdout combined with rate-limit
stderr and verify the failure is classified as a rate limit.
| pub(crate) fn is_rate_limited(text: &str) -> bool { | ||
| let lower = text.to_lowercase(); | ||
| RATE_LIMIT_PATTERNS.iter().any(|p| lower.contains(p)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restrict the shared rate-limit classifier.
RATE_LIMIT_PATTERNS accepts "try again" without a rate-limit indicator. A failed work run with that text then receives a 30-minute retry delay and blocks new dispatch for that agent. Require a rate-limit-specific signal before returning true. Add a negative test for a non-rate-limit failure that contains "try again".
Also applies to: 201-206
🤖 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/auth_runner.rs` around lines 74 - 76, Update is_rate_limited to require a
rate-limit-specific indicator alongside any generic “try again” match,
preventing unrelated failures from being classified as rate limited. Adjust
RATE_LIMIT_PATTERNS or the matching logic while preserving valid rate-limit
detection, and add a negative test covering a non-rate-limit failure containing
“try again”.
…holder (#419) push_and_open_pr hardcoded "Auto-opened on \`item done\` for {id}." as every PR body, regardless of whether real information about the change was available. agentflare work already parses the headless agent's own final reply (parse_claude_reply) -- its prompt explicitly asks the agent to "summarize what you changed and why" -- but that text only ever reached the item as a comment, never the PR itself. Reviewers opened these PRs cold, with a generic placeholder as the only description, no matter how good the agent's own summary was. - types.rs: ItemRequest gains an optional `summary` field, scoped to `done` (mirrors the existing `push` field's scoping), documented in the item tool's own description. - worktree.rs: push_and_open_pr takes an optional summary and uses it as the PR body when non-blank, falling back to the old placeholder otherwise. Extracted the body-construction logic into a small pure pr_body() so it's directly unit-testable without needing a real repo remote/gh client (push_and_open_pr itself soft-fails without one). - item.rs: item_done threads req.summary through to both push_and_open_pr call sites. - work.rs: agentflare work's own item_done call (the common path -- a headless run that just replies with text and lets the wrapper handle `done`) now passes the already-parsed reply_text as the summary. When the agent instead calls `done` itself mid-session (as item #43's PR #417 did), it can pass its own `summary` directly per the updated tool description. Agentflare-Agent: claude-code Agentflare-Branch: fix-pr-summary-uses-agent-reply Co-authored-by: shiva <shiva@gosysinfo.tech>
# Conflicts: # src/cli/work.rs Agentflare-Agent: claude-code Agentflare-Branch: task/43 Agentflare-Item: 43
work autonomous job runnerwork autonomous job runner
Auto-opened on
item donefor BUOUHucae6tOCRVRYKCcI.Summary by CodeRabbit
New Features
Bug Fixes
Tests