Dashboard hardening + live job queue integration - #375
Merged
Conversation
added 17 commits
July 27, 2026 17:24
Adds two non-consent components so agentflare init and every SessionStart self-heal the setup needed to actually use flare-docs, flare-search, and lean-ctx through the gateway, instead of relying on hand-run CLI commands: - core-coaching: seeds/refreshes 4 builtin coaching rules (usedocs, usesearch, useleanctx, usetsearch) that nudge the flare gateway's docs/search/lean-ctx/tool-search wrappers over their native equivalents. Drift-protected across version bumps; a same-id rule the user has overridden to a different tier is left alone. - gateway-permissions: keeps ~/.claude/settings.json's permissions.allow containing the flare gateway tools (mcp__flare__docs, mcp__flare__search, mcp__flare__tool, ToolSearch) and strips superseded direct mcp__lean-ctx__* entries. Also widens coaching::store::list_rules to pub(crate) so components.rs can read existing rule state when deciding whether to seed or refresh.
Adds docs-site/, an Astro Starlight project documenting the flare-docs module (overview, supported languages, MCP tool reference, CLI reference, examples, and a comparison against Context7), reusing the landing page's exact design system (colors, fonts, mono headings) via customCss. site/ and docs-site/ are linked as sibling packages in an aube-workspace.yaml monorepo rooted at the repo root, with mise tasks (install/dev:site/dev:docs/ build:docs/deploy/tail) as the entry points. The two connect only at deploy time: site's deploy script builds docs-site out-of-tree and copies its output into site/public/docs before wrangler deploy, so day-to-day dev on either package never touches the other.
Adds Ecosystem::Python alongside Rust and npm: fetches a package's PyPI manifest, unpacks its pure-Python wheel, and indexes whichever typed source it carries — separate .pyi stubs, or (for PEP 561 inline-typed packages like click and pandas) the package's own annotated .py source with a py.typed marker — via a tree-sitter-python extraction pass mirroring the existing npm/.d.ts module. Falls back to typeshed's types-<package> convention on PyPI when a package ships neither. Wired into the CLI (`agentflare docs get <pkg> --ecosystem python`) and the MCP `docs` tool. Verified live against PyPI: requests correctly falls back to types-requests; click indexes 470 items from its own inline-typed source with no fallback needed.
Fills the one real gap Rust didn't have: rustdoc-JSON already carries a
crate's doc-comment Examples verbatim, but npm and Python previously
discarded the README/long-description entirely, keeping only .d.ts/.pyi
signatures.
Adds a shared, LLM-free fenced-code-block extractor (crates/flare-docs/src
/readme.rs) reused by both ecosystems at zero extra network cost: npm scans
every .md file already inside the tarball it downloads for .d.ts, and
Python reads info.description straight out of the PyPI manifest JSON it
already fetches (markdown releases only -- RST is explicitly out of scope
rather than mis-parsed). Each block is titled by its nearest heading and
indexed as npm-example/python-example, reconciled the same way API items
are.
Filters two kinds of noise before indexing: examples under process/meta
headings (Installation, Contributing, License, Testing, ...), and
shell-flavored blocks that are pure setup commands (git clone, pip install)
even under an otherwise-good heading like "Quick Start" -- verified live
against requests' real PyPI description, which previously surfaced its
"Cloning the repository" git commands as if they were usage examples.
Also fixes a path-matching gap in both the new markdown-file filter and the
existing Python inline-typed-package filter: a top-level `test/` directory
(no leading slash after tarball-prefix stripping) wasn't caught by a
mid-path `contains("/test/")` check.
… ones Now that flare-docs indexes usage examples too, add a comparison row and strengthen the "where flare-docs wins" case: examples are extracted verbatim from the package's own docs, not synthesized by an LLM the way Context7's snippets are -- so they can't drift from what the maintainer actually wrote.
…ment-components # Conflicts: # src/components.rs # src/mcp_server/flare_docs.rs
…ad of firing every call Previously the batching_nudge fired on every single tool call once a streak crossed the trailing-window threshold, spamming the same message. Now it only fires at streak milestones (3, 6, 12, 24, ...), computed statelessly from recent_tool_calls history.
… enforced rule Coaching rules were 100% advisory: a nudge in systemMessage the agent could ignore. Adds an enforced flag (# Enforce: true header, coaching enforce <id> [--off] CLI) that the PreToolUse hook checks before falling through to advisory nudges — a match denies the call with a redirect-framed reason instead of just suggesting the preferred tool. Adopts lean-ctx's own PreToolUse mechanism as the reference (studied in ~/workspace/refs/lean-ctx): it substitutes tool input rather than denying, which only works when the substitute is the same tool with different input (e.g. a Bash command rewrite). None of agentflare's coaching rules are same-tool swaps, so this instead reuses the existing deny-decision shape from hook_redirect.rs, with the reason phrased as a redirect. Of the four builtin rules, only usesearch (WebFetch/WebSearch) and usetsearch (ToolSearch) are marked enforced by default: they're clean 1:1 substitutes with no fallback the mandatory tool can't cover. useleanctx stays advisory since ctx_* can't handle every path/case native Bash/Read can (hard-blocking risks a self-lockout), and usedocs stays advisory because its trigger (Edit/Write) isn't a same-action substitute for mcp__flare__docs — blocking edits to redirect to a docs lookup would just break editing.
write_rule_file crossed clippy's default 7-arg threshold when the enforced param was added; allow it like the codebase's other multi-field constructors. The fmt diff is pre-existing drift in flare-docs and the coaching files from this branch's own history, not from this commit.
…, add HTTP tests Closes the PR #255 review backlog. data.rs accessors silently returned "[]" on any DB error, making a broken/missing DB indistinguishable from genuinely empty state in the server log — now every error arm logs before falling back. /api/cost?by= silently coerced unknown values to model grouping instead of surfacing caller typos as a 400. Binding to a non-loopback host exposed all PM/cost/webhook data with only an easy-to- miss stderr warning — it now refuses to start unless --yes-expose is passed. Also adds HTTP-layer test coverage for cost_handler, the /events SSE stream, and static_handler that the module previously lacked.
Exercises the --yes-expose gate end-to-end for the exempt path: run() with a 127.0.0.1 host and yes_expose=false actually binds and serves, rather than only asserting the pure is_local_bind() helper. The non-local refusal path isn't covered here since it calls std::process::exit, which isn't safe to trigger inside the test binary.
…up, test coverage Supervisor/Queue/WorkerPool had zero test coverage and several gaps before wiring into a real service: workers busy-polled every 200ms instead of waking on enqueue, stdout/stderr_total_bytes always reported 0 (missing DB columns), finished jobs and their log files accumulated forever, and JobInfo never surfaced the job's command/args to callers. Adds a Condvar-based notify with a bounded fallback poll, an additive byte-count migration, an hourly-eligible cleanup that also deletes log files, exposes command/args on JobInfo, and covers all of it plus Supervisor's timeout/kill path with new tests.
agentflare serve could be started multiple times concurrently on different ports: the hidden --_foreground-daemon flag start_daemon() tried to pass wasn't actually registered on the clap args, and start_daemon() itself spawned the bare binary without the serve subcommand, so daemon start silently failed to launch anything while direct agentflare serve invocations never checked for an existing instance at all. Adds the missing flag, fixes the spawn args to match what the systemd/launchd units already invoke, and makes ServeArgs::run check is_daemon_running()/write its own pid file so any invocation path refuses to start a second instance.
Wires agentflare-jobs into the dashboard daemon: opens a Queue against
the same agentflare.db, starts a 2-worker pool, and schedules an hourly
sweep of finished jobs older than 7 days (including their log files).
Adds POST /api/jobs (submit), GET /api/jobs (fetch/list), GET
/api/jobs/events (SSE live job list) and GET /api/jobs/{id}/stream (SSE
incremental stdout tail, closing with event: done on terminal state).
Adds dashboard/web/jobs.html (list + live-tailed detail view, Alpine.js
matching the existing pages) and a Jobs nav link across all pages.
|
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:
📝 WalkthroughWalkthroughThis change adds queue-backed job execution with persisted output metadata, worker notifications, dashboard REST and SSE endpoints, a Jobs dashboard page, daemon exposure controls, PID registration, and expanded runtime and endpoint tests. ChangesJobs runtime
Dashboard and serving
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant DashboardAPI
participant Queue
participant WorkerPool
Browser->>DashboardAPI: Submit job
DashboardAPI->>Queue: Enqueue AgentJob
Queue->>WorkerPool: Wake worker
WorkerPool->>Queue: Complete job and persist output
Browser->>DashboardAPI: Open SSE stream
DashboardAPI->>Queue: Read output and state
DashboardAPI-->>Browser: Stream output and done event
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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: 7
🧹 Nitpick comments (3)
src/dashboard/server.rs (3)
354-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilently collapsing DB/join errors into an empty list.
A persistent
queue.listfailure renders as "no jobs" in the UI with nothing in the logs. Log the error before falling back, consistent withspawn_job_cleanupand the newdata.rslogging.♻️ Log before falling back
- .await - .unwrap_or(Ok(vec![])) - .unwrap_or(vec![]); + .await; + let list = match list { + Ok(Ok(list)) => list, + Ok(Err(e)) => { + eprintln!("[dashboard] jobs_events: list failed: {e}"); + vec![] + } + Err(e) => { + eprintln!("[dashboard] jobs_events: task failed: {e}"); + vec![] + } + };🤖 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/dashboard/server.rs` around lines 354 - 361, Update the queue listing flow around spawn_blocking and queue.list to log both database errors and task-join errors before falling back to an empty list. Follow the existing logging pattern used by spawn_job_cleanup and data.rs, while preserving the current empty-list fallback and JSON serialization behavior.
306-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking SQLite work on the async request path.
queue.get/queue.listtake aparking_lot-style mutex and hit SQLite synchronously; the SSE handlers correctly wrap identical calls inspawn_blocking(Lines 354, 397), but these are awaited inline. Under contention with the two worker threads this can stall the runtime worker. Same applies to the 404 pre-check at Line 385.♻️ Wrap in spawn_blocking
- if let Some(id) = q.id { - return match queue.get(&id) { + if let Some(id) = q.id { + let got = tokio::task::spawn_blocking({ + let queue = queue.clone(); + move || queue.get(&id) + }) + .await; + return match got { + Ok(Ok(info)) => Json(info).into_response(), _ => (StatusCode::NOT_FOUND, "job not found").into_response(), }; }🤖 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/dashboard/server.rs` around lines 306 - 338, Update jobs_handler to execute the synchronous queue.get and queue.list calls inside tokio::task::spawn_blocking, matching the existing pattern used by the SSE handlers. Await the blocking tasks before constructing responses, preserve the current not-found, invalid-state, success, and internal-error behavior, and handle any spawn_blocking join failure appropriately.
547-550: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test_queue()drops itsTempDirbefore use.The
TempDiris deleted at the end oftest_queue(), so the returned queue'slog_dirpoints at a path that no longer exists. Harmless for tests that never touch logs, but latent — the log-streaming test at Line 774 has to build its own queue for exactly this reason. Consider keeping the dir alive (e.g. return(TempDir, Queue)or leak viainto_path()).🤖 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/dashboard/server.rs` around lines 547 - 550, Update the test_queue helper so the temporary directory remains alive for the returned Queue, such as by returning the TempDir alongside the Queue and updating callers to retain it, or by intentionally transferring/leaking ownership of the path. Ensure any tests using test_queue preserve the directory lifetime before accessing queue logs.
🤖 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 12-17: Make worker notifications durable by replacing the
stateless notify tuple in queue.rs (lines 12-17) with a shared predicate or
generation protected by the mutex, and update queue.rs lines 90-102 to wait in a
condition-checked loop that cannot miss enqueue/retry or shutdown changes.
Update worker.rs lines 36-38 and 74-76 to mutate/check the same shared state for
new work and stop requests, ensuring shutdown joins and worker wakeups respond
immediately without relying on the fallback poll.
In `@dashboard/web/jobs.html`:
- Around line 199-230: Update selectJob to close the existing detailSource
before resetting detailOutput and opening the new EventSource. Clear the prior
stream reference as part of the handoff so only the newly selected job’s stream
can append output, while preserving the existing backToList and done-handler
behavior.
- Around line 91-102: Update the job row action in the jobs template so the job
ID cell contains a focusable link targeting the job detail/output view,
preserving the displayed truncated ID and existing styling. Remove or avoid
relying solely on the row-level click handler, and ensure keyboard activation
and middle-click deep-linking invoke the same job selection behavior via the
link.
In `@src/cli/serve.rs`:
- Around line 36-41: Update the serve startup flow around daemon::write_pid_file
and dashboard::serve so PID registration occurs only after the dashboard
listener and queue initialization have completed successfully. Make PID-write
failure abort startup with an error instead of merely warning, and ensure daemon
start reports success only after registration is established.
In `@src/daemon.rs`:
- Around line 80-90: Make write_pid_file atomically claim the fixed PID path so
concurrent direct ServeArgs::run invocations cannot both succeed. Replace the
unconditional std::fs::write flow with a cross-process lock or
exclusive-create-and-revalidate protocol, preserving the existing directory
creation and Result<String> error reporting while rejecting an already-claimed
PID file.
In `@src/dashboard/server.rs`:
- Around line 260-298: Restrict submit_job_handler so unauthenticated job
submission is accepted only when the server is bound to a loopback address, or
enforce an equivalent shared-token check for non-local binds; ensure the check
occurs before constructing or enqueueing the AgentJob. Update the --yes-expose
warning/refusal text to explicitly state that exposed binds also permit
arbitrary command execution, not only PM and cost-data access.
- Around line 371-434: The jobs_stream_handler currently waits for info.output,
which is only available after terminal completion, so live stdout cannot be
streamed. Resolve the stdout log path independently using queue.log_dir() and
the supervisor’s existing job naming convention, then open and tail that file
while the job is queued or running; retain the existing terminal flushing and
done-event behavior.
---
Nitpick comments:
In `@src/dashboard/server.rs`:
- Around line 354-361: Update the queue listing flow around spawn_blocking and
queue.list to log both database errors and task-join errors before falling back
to an empty list. Follow the existing logging pattern used by spawn_job_cleanup
and data.rs, while preserving the current empty-list fallback and JSON
serialization behavior.
- Around line 306-338: Update jobs_handler to execute the synchronous queue.get
and queue.list calls inside tokio::task::spawn_blocking, matching the existing
pattern used by the SSE handlers. Await the blocking tasks before constructing
responses, preserve the current not-found, invalid-state, success, and
internal-error behavior, and handle any spawn_blocking join failure
appropriately.
- Around line 547-550: Update the test_queue helper so the temporary directory
remains alive for the returned Queue, such as by returning the TempDir alongside
the Queue and updating callers to retain it, or by intentionally
transferring/leaking ownership of the path. Ensure any tests using test_queue
preserve the directory lifetime before accessing queue logs.
🪄 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: cef478fd-a854-4fc1-bf65-1c8d59542ca0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlcrates/agentflare-jobs/src/queue.rscrates/agentflare-jobs/src/types.rscrates/agentflare-jobs/src/worker.rscrates/agentflare-jobs/tests/queue_test.rscrates/agentflare-jobs/tests/supervisor_test.rscrates/agentflare-jobs/tests/worker_test.rsdashboard/web/board.htmldashboard/web/claims.htmldashboard/web/cost.htmldashboard/web/item.htmldashboard/web/jobs.htmldashboard/web/list.htmldashboard/web/shell.htmldashboard/web/webhooks.htmlsrc/cli/serve.rssrc/daemon.rssrc/dashboard/data.rssrc/dashboard/mod.rssrc/dashboard/server.rs
Comment on lines
+12
to
+17
| // Wakes idle workers the moment a job becomes available (fresh enqueue, | ||
| // or a retry going back to 'queued'), instead of them finding out only | ||
| // on their next poll. The paired Mutex<()> exists only because | ||
| // Condvar::wait_for needs a guard to attach to — there's no shared Rust | ||
| // state to protect, the real "state" lives in SQLite. | ||
| notify: Arc<(Mutex<()>, Condvar)>, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' crates/agentflare-jobs/src/queue.rs && printf '\n--- worker.rs ---\n' && sed -n '1,180p' crates/agentflare-jobs/src/worker.rsRepository: getappz/agentflare
Length of output: 11121
Make worker wakeups durable. notify_all() can land after dequeue() returns None but before wait_for_work() starts waiting, so that wake is lost and workers sit on the 1s fallback. The same gap can delay shutdown() joins; gate the condvar on a shared predicate/generation so new work or stop requests can't be missed.
📍 Affects 2 files
crates/agentflare-jobs/src/queue.rs#L12-L17(this comment)crates/agentflare-jobs/src/queue.rs#L90-L102crates/agentflare-jobs/src/worker.rs#L36-L38crates/agentflare-jobs/src/worker.rs#L74-L76
🤖 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 12 - 17, Make worker
notifications durable by replacing the stateless notify tuple in queue.rs (lines
12-17) with a shared predicate or generation protected by the mutex, and update
queue.rs lines 90-102 to wait in a condition-checked loop that cannot miss
enqueue/retry or shutdown changes. Update worker.rs lines 36-38 and 74-76 to
mutate/check the same shared state for new work and stop requests, ensuring
shutdown joins and worker wakeups respond immediately without relying on the
fallback poll.
Comment on lines
+91
to
+102
| <template x-for="job in jobs" :key="job.id"> | ||
| <tr class="af-clickable-row" @click="selectJob(job.id)"> | ||
| <td class="af-mono" x-text="job.id.substring(0, 12) + '…'"></td> | ||
| <td class="af-mono af-truncate" x-text="jobCommand(job)"></td> | ||
| <td> | ||
| <span class="af-badge" :class="stateClass(job.state)" x-text="job.state"></span> | ||
| </td> | ||
| <td x-text="afFormatTime(job.created_at)"></td> | ||
| <td x-text="afFormatTime(job.started_at)"></td> | ||
| <td x-text="afFormatTime(job.finished_at)"></td> | ||
| </tr> | ||
| </template> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Job rows are mouse-only.
<tr @click> isn't focusable and has no key handler, so keyboard users can't open a job's detail/output view. Making the ID cell an actual link (which also gives free deep-linking and middle-click) is the cheapest fix.
♿ Focusable row action
- <tr class="af-clickable-row" `@click`="selectJob(job.id)">
- <td class="af-mono" x-text="job.id.substring(0, 12) + '…'"></td>
+ <tr class="af-clickable-row" `@click`="selectJob(job.id)">
+ <td class="af-mono">
+ <a :href="'?id=' + encodeURIComponent(job.id)"
+ `@click.prevent`="selectJob(job.id)"
+ x-text="job.id.substring(0, 12) + '…'"></a>
+ </td>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <template x-for="job in jobs" :key="job.id"> | |
| <tr class="af-clickable-row" @click="selectJob(job.id)"> | |
| <td class="af-mono" x-text="job.id.substring(0, 12) + '…'"></td> | |
| <td class="af-mono af-truncate" x-text="jobCommand(job)"></td> | |
| <td> | |
| <span class="af-badge" :class="stateClass(job.state)" x-text="job.state"></span> | |
| </td> | |
| <td x-text="afFormatTime(job.created_at)"></td> | |
| <td x-text="afFormatTime(job.started_at)"></td> | |
| <td x-text="afFormatTime(job.finished_at)"></td> | |
| </tr> | |
| </template> | |
| <template x-for="job in jobs" :key="job.id"> | |
| <tr class="af-clickable-row" `@click`="selectJob(job.id)"> | |
| <td class="af-mono"> | |
| <a :href="'?id=' + encodeURIComponent(job.id)" | |
| `@click.prevent`="selectJob(job.id)" | |
| x-text="job.id.substring(0, 12) + '…'"></a> | |
| </td> | |
| <td class="af-mono af-truncate" x-text="jobCommand(job)"></td> | |
| <td> | |
| <span class="af-badge" :class="stateClass(job.state)" x-text="job.state"></span> | |
| </td> | |
| <td x-text="afFormatTime(job.created_at)"></td> | |
| <td x-text="afFormatTime(job.started_at)"></td> | |
| <td x-text="afFormatTime(job.finished_at)"></td> | |
| </tr> | |
| </template> |
🤖 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 `@dashboard/web/jobs.html` around lines 91 - 102, Update the job row action in
the jobs template so the job ID cell contains a focusable link targeting the job
detail/output view, preserving the displayed truncated ID and existing styling.
Remove or avoid relying solely on the row-level click handler, and ensure
keyboard activation and middle-click deep-linking invoke the same job selection
behavior via the link.
Comment on lines
+36
to
+41
| if let Err(e) = crate::daemon::write_pid_file() { | ||
| eprintln!( | ||
| "warning: failed to record daemon pid ({e}); `agentflare daemon status`/`stop` won't see this instance." | ||
| ); | ||
| } | ||
| crate::dashboard::serve(&self.host, self.port, self.open, self.yes_expose); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make PID registration part of successful server startup.
The PID is written before dashboard::server::run binds its listener, while start_daemon treats a live PID as readiness (Lines 109-115 in src/daemon.rs). If binding or queue initialization fails, daemon start can report success even though the dashboard exits immediately. Conversely, a PID-write failure is only warned about, allowing an unregistered server to start. Register after successful bind and fail startup if registration cannot be established.
🤖 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/cli/serve.rs` around lines 36 - 41, Update the serve startup flow around
daemon::write_pid_file and dashboard::serve so PID registration occurs only
after the dashboard listener and queue initialization have completed
successfully. Make PID-write failure abort startup with an error instead of
merely warning, and ensure daemon start reports success only after registration
is established.
Comment on lines
+260
to
+298
| /// Body for `POST /api/jobs`. Only `command` is required; everything else | ||
| /// falls back to `AgentJob::new`'s defaults (300s timeout, 3 retries). | ||
| #[derive(Deserialize)] | ||
| struct SubmitJobRequest { | ||
| command: String, | ||
| #[serde(default)] | ||
| args: Vec<String>, | ||
| #[serde(default)] | ||
| env: Vec<(String, String)>, | ||
| cwd: Option<PathBuf>, | ||
| timeout_secs: Option<u64>, | ||
| } | ||
|
|
||
| async fn submit_job_handler( | ||
| State(queue): State<Queue>, | ||
| Json(req): Json<SubmitJobRequest>, | ||
| ) -> Response { | ||
| if req.command.trim().is_empty() { | ||
| return (StatusCode::BAD_REQUEST, "command must not be empty").into_response(); | ||
| } | ||
| let mut job = AgentJob::new(req.command).args(req.args); | ||
| for (k, v) in req.env { | ||
| job = job.env(k, v); | ||
| } | ||
| if let Some(cwd) = req.cwd { | ||
| job = job.cwd(cwd); | ||
| } | ||
| if let Some(secs) = req.timeout_secs { | ||
| job = job.timeout(secs); | ||
| } | ||
| match queue.enqueue(&job) { | ||
| Ok(info) => (StatusCode::CREATED, Json(info)).into_response(), | ||
| Err(e) => ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| format!("failed to enqueue job: {e}"), | ||
| ) | ||
| .into_response(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Unauthenticated POST /api/jobs is arbitrary command execution — gate it when the bind is non-local.
SubmitJobRequest.command/args/env/cwd flow straight into Queue::enqueue, and the worker's Supervisor::spawn runs Command::new(&self.command) with that env/cwd as the daemon user (crates/agentflare-jobs/src/supervisor.rs:46-139). Combined with --yes-expose (Line 500), any host on the network gets RCE, not just read access to PM/cost data as the refusal message claims.
Suggested minimum: refuse job submission unless the bind is loopback (or require a shared token), and correct the --yes-expose warning to state that it also exposes command execution.
🔒 Sketch: restrict submission on exposed binds
-fn jobs_router(queue: Queue) -> Router {
+fn jobs_router(queue: Queue, allow_submit: bool) -> Router {
Router::new()
- .route("/api/jobs", get(jobs_handler).post(submit_job_handler))
+ .route(
+ "/api/jobs",
+ if allow_submit {
+ get(jobs_handler).post(submit_job_handler)
+ } else {
+ get(jobs_handler)
+ },
+ )
.route("/api/jobs/events", get(jobs_events_handler))
.route("/api/jobs/{id}/stream", get(jobs_stream_handler))
.with_state(queue)
}🤖 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/dashboard/server.rs` around lines 260 - 298, Restrict submit_job_handler
so unauthenticated job submission is accepted only when the server is bound to a
loopback address, or enforce an equivalent shared-token check for non-local
binds; ensure the check occurs before constructing or enqueueing the AgentJob.
Update the --yes-expose warning/refusal text to explicitly state that exposed
binds also permit arbitrary command execution, not only PM and cost-data access.
added 4 commits
July 30, 2026 19:18
…orks while running
Code review on the jobs-streaming PR found the live tail was completely
non-functional for in-progress jobs: jobs_stream_handler only opened the
stdout file once JobInfo.output was populated, which only happens after
a job reaches a terminal state, and Supervisor generated its own random
id for log filenames independent of the job's own queue id — so there
was no way to even locate a running job's log path. The stream would
sit idle the whole time a job ran, then dump everything in one lump the
instant it finished. Fixes this by having Supervisor::new take the
job's own id and name its log files from it, so jobs_stream_handler can
derive the path directly via queue.log_dir().join("{id}.stdout") without
waiting on completion. Adds a regression test that drives a real job
through a real WorkerPool and asserts output arrives while the job is
still 'running', not just after it exits — the previous test only
covered an already-finished job, which is exactly why this slipped
through.
Also fixes two smaller issues from the same review pass in
src/dashboard/server.rs: jobs_handler and the jobs_stream_handler 404
pre-check ran synchronous SQLite calls inline on the async path instead
of via spawn_blocking (inconsistent with the SSE handlers doing the
same calls correctly), and jobs_events_handler silently swallowed list
errors into an empty list instead of logging them. Also fixes
test_queue() dropping its TempDir immediately, which left log_dir
pointing at an already-deleted path.
ServeArgs::run() checked is_daemon_running() then wrote its pid file as two separate steps with nothing serializing them, so two direct `agentflare serve` invocations (e.g. on different --port values) started within moments of each other could both pass the check before either had written its pid file, recreating the exact multi-instance bug this singleton check exists to prevent. Adds a dedicated short-timeout lock file for this check-then-write section. It has to be a different lock from daemon_start_lock_path(): start_daemon() holds that one for its whole ~5s spawn-and-poll window, and the process it spawns calls back into this same check via `serve --_foreground-daemon` — sharing one lock would deadlock the two.
selectJob() opened a new EventSource without closing detailSource from a previously selected job, so its events could keep appending into the newly selected job's detailOutput. Not reachable through the current UI (the list view is hidden while a job is selected), but a latent resource leak and cross-talk bug for any future code path that calls selectJob twice in a row.
…-logging-and-hardening # Conflicts: # Cargo.lock # Cargo.toml # src/cli/serve.rs # src/dashboard/server.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/agentflare-jobs/tests/queue_test.rs (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
test_queuehelper leaks a temp dir per call. Both files define the same helper that callsTempDir::keep(), disabling cleanup so every test permanently leaves alogsdirectory behind; scoping the guard to the test would avoid it and also remove the dependency on the newerkeep()API.
crates/agentflare-jobs/tests/queue_test.rs#L3-L8: return(TempDir, Queue)and let callers hold the guard instead of calling.keep().src/dashboard/server.rs#L587-L594: apply the same change here rather than re-copying the helper.🤖 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/tests/queue_test.rs` around lines 3 - 8, The test_queue helper in crates/agentflare-jobs/tests/queue_test.rs#L3-L8 and src/dashboard/server.rs#L587-L594 should return both the TempDir guard and Queue, remove the TempDir::keep() call, and update each caller to retain the guard for the test's lifetime so temporary log directories are cleaned up.src/dashboard/server.rs (1)
313-327: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
getmaps every error to 404, including real DB failures. The list path below distinguishes DB errors (500); here a genuine SQLite error is reported as "job not found" with no log, which will make queue corruption look like a missing id.♻️ Log before collapsing to 404
- Ok(Err(_)) => (StatusCode::NOT_FOUND, "job not found").into_response(), + Ok(Err(e)) => { + eprintln!("[dashboard/server] jobs: get failed: {e}"); + (StatusCode::NOT_FOUND, "job not found").into_response() + }🤖 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/dashboard/server.rs` around lines 313 - 327, Update the id lookup match in the dashboard handler so only a genuine missing-job result returns 404; distinguish queue/database errors and return 500 while logging the underlying error before responding. Preserve the existing successful Json(info) response and spawn_blocking task-failure handling.
🤖 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/cli/serve.rs`:
- Around line 33-38: Update the singleton-lock handling in the serve command to
fail closed: when crate::daemon::acquire_singleton_lock() returns an error,
report the failure and exit without entering the PID-file check-and-write
critical section. Preserve normal serving behavior only when the lock is
acquired successfully.
In `@src/daemon.rs`:
- Around line 46-47: Update start_daemon and cleanup_daemon_files so cleanup
never unlinks daemon.start.lock while start_daemon holds it. Keep the lock path
stable throughout is_daemon_running and stale-PID cleanup, and rely on LockGuard
to release the lock only after the guarded startup flow completes.
In `@src/dashboard/server.rs`:
- Around line 456-466: Update the File::open failure handling in the stdout_path
polling loop so a transient error for a still-running job does not break the
stream; retry on the next tick while preserving the existing terminal done event
behavior, or ensure done is emitted before termination.
- Around line 890-894: Update the Windows command in the command/args setup to
use an stdin-agnostic delay, such as redirect-safe ping against 127.0.0.1,
instead of timeout /t 2. Preserve the existing tick1 and tick2 output and the
Unix command unchanged so the job remains running for the intended interval
under Supervisor’s Stdio::null().
---
Nitpick comments:
In `@crates/agentflare-jobs/tests/queue_test.rs`:
- Around line 3-8: The test_queue helper in
crates/agentflare-jobs/tests/queue_test.rs#L3-L8 and
src/dashboard/server.rs#L587-L594 should return both the TempDir guard and
Queue, remove the TempDir::keep() call, and update each caller to retain the
guard for the test's lifetime so temporary log directories are cleaned up.
In `@src/dashboard/server.rs`:
- Around line 313-327: Update the id lookup match in the dashboard handler so
only a genuine missing-job result returns 404; distinguish queue/database errors
and return 500 while logging the underlying error before responding. Preserve
the existing successful Json(info) response and spawn_blocking task-failure
handling.
🪄 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: 222f61f5-02d7-4a14-acf0-bd61027bb7b8
📒 Files selected for processing (8)
crates/agentflare-jobs/src/supervisor.rscrates/agentflare-jobs/src/worker.rscrates/agentflare-jobs/tests/queue_test.rscrates/agentflare-jobs/tests/supervisor_test.rsdashboard/web/jobs.htmlsrc/cli/serve.rssrc/daemon.rssrc/dashboard/server.rs
Comment on lines
+33
to
+38
| let guard = crate::daemon::acquire_singleton_lock(); | ||
| if let Err(ref e) = guard { | ||
| eprintln!( | ||
| "warning: failed to acquire daemon singleton lock ({e}); proceeding without race protection against a concurrent `serve` invocation." | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when singleton-lock acquisition fails.
At Line 34, an acquisition error is only logged, but the check-and-write critical section still runs. This allows concurrent serve invocations to bypass serialization and overwrite the PID file. Exit with an error instead of proceeding.
Proposed fix
- let guard = crate::daemon::acquire_singleton_lock();
- if let Err(ref e) = guard {
- eprintln!(
- "warning: failed to acquire daemon singleton lock ({e}); proceeding without race protection against a concurrent `serve` invocation."
- );
- }
+ let _guard = match crate::daemon::acquire_singleton_lock() {
+ Ok(guard) => guard,
+ Err(e) => {
+ eprintln!("error: failed to acquire daemon singleton lock ({e})");
+ std::process::exit(1);
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let guard = crate::daemon::acquire_singleton_lock(); | |
| if let Err(ref e) = guard { | |
| eprintln!( | |
| "warning: failed to acquire daemon singleton lock ({e}); proceeding without race protection against a concurrent `serve` invocation." | |
| ); | |
| } | |
| let _guard = match crate::daemon::acquire_singleton_lock() { | |
| Ok(guard) => guard, | |
| Err(e) => { | |
| eprintln!("error: failed to acquire daemon singleton lock ({e})"); | |
| std::process::exit(1); | |
| } | |
| }; |
🤖 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/cli/serve.rs` around lines 33 - 38, Update the singleton-lock handling in
the serve command to fail closed: when crate::daemon::acquire_singleton_lock()
returns an error, report the failure and exit without entering the PID-file
check-and-write critical section. Preserve normal serving behavior only when the
lock is acquired successfully.
Comment on lines
+456
to
+466
| if stdout_path.exists() { | ||
| match std::fs::File::open(&stdout_path) { | ||
| Ok(f) => file = Some(f), | ||
| Err(_) => { | ||
| if terminal { | ||
| let _ = tx.send(Ok(Event::default().data("").event("done"))); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A transient open failure silently kills the stream of a still-running job. If File::open fails while the job is non-terminal, the task breaks without sending done, so the client's stream just ends mid-job and the UI never learns why. Prefer retrying on the next tick (or at minimum always emit done).
🩹 Retry instead of terminating
match std::fs::File::open(&stdout_path) {
Ok(f) => file = Some(f),
Err(_) => {
- if terminal {
- let _ = tx.send(Ok(Event::default().data("").event("done")));
- }
- break;
+ if terminal {
+ let _ = tx.send(Ok(Event::default().data("").event("done")));
+ break;
+ }
+ continue;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if stdout_path.exists() { | |
| match std::fs::File::open(&stdout_path) { | |
| Ok(f) => file = Some(f), | |
| Err(_) => { | |
| if terminal { | |
| let _ = tx.send(Ok(Event::default().data("").event("done"))); | |
| } | |
| break; | |
| } | |
| } | |
| } | |
| if stdout_path.exists() { | |
| match std::fs::File::open(&stdout_path) { | |
| Ok(f) => file = Some(f), | |
| Err(_) => { | |
| if terminal { | |
| let _ = tx.send(Ok(Event::default().data("").event("done"))); | |
| break; | |
| } | |
| continue; | |
| } | |
| } | |
| } |
🤖 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/dashboard/server.rs` around lines 456 - 466, Update the File::open
failure handling in the stdout_path polling loop so a transient error for a
still-running job does not break the stream; retry on the next tick while
preserving the existing terminal done event behavior, or ensure done is emitted
before termination.
added 3 commits
July 30, 2026 19:38
…test failure Master's CI workflows (fmt, clippy -D warnings, and a full Windows build+test job) never ran against this branch until the merge, and surfaced three issues: - cargo fmt had drifted (never run locally after the review-fix pass). - Supervisor::new crossed clippy's 7-arg threshold once it gained the id parameter; allowed like this codebase's other multi-field constructors (see 7e56a9c). - complete_persists_stdout_and_stderr_byte_counts and stdout_and_stderr_are_captured_separately both asserted stdout_total_bytes > 0 but used `echo hello 1>&1 & echo world 1>&2` on Windows — cmd.exe's redirection parser doesn't treat N>&N as the true no-op POSIX shells do, and can end up closing/breaking that handle instead, leaving stdout empty. Dropping the always-redundant `1>&1` (stdout is already fd 1) fixes it without changing what the test verifies.
timeout /t exits instantly with 'INPUT REDIRECTION IS NOT SUPPORTED' when stdin isn't a real console — which it never is under Supervisor (Stdio::null()). Both the new still-running-job stream test and timeout_kills_long_running_process used it as their Windows sleep stand-in, so the whole job finished near-instantly on Windows instead of actually running for the intended duration. Switches both to the standard ping-against-loopback idiom, which doesn't touch stdin.
…rkers shutdown_returns_promptly_even_when_workers_are_idle flaked in CI (1.0003s instead of <300ms) — notify_all() alone only wakes threads already parked in wait_for, so a wake_workers() call landing between a worker's dequeue-check and its wait_for_work call was silently lost until the 1s fallback timeout. This was a real, if narrow, race in the notify design, not just a CI timing fluke — low local contention just made it rare enough to not reproduce. Fixes it with the standard predicate+Condvar pattern: a pending-signal bool guarded by the same lock, checked before ever blocking. A wake that arrives first is observed immediately when wait_for_work runs; one that arrives during the wait still notifies as before. No more lost-wakeup window in either ordering. Verified with 15 repeated local runs after the fix, all well under the timeout.
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.
Summary
costendpoint'sby=param, gate non-local binds behind--yes-expose, add HTTP-level tests for the server (b5bd824,323a72c).agentflare-jobs: event-driven work pickup via Condvar (replacing 200ms busy-polling), a real fix forstdout_total_bytes/stderr_total_bytesalways reporting 0, automatic cleanup of finished jobs + their log files,JobInfonow surfacescommand/args, and new test coverage forSupervisor/Queue/WorkerPoolthat didn't exist before.agentflare servecould previously be started multiple times concurrently (a missing clap flag plus wrong spawn args meantdaemon startsilently did nothing while directserveinvocations never checked for an existing instance); now any invocation path refuses to start a second instance.agentflare-jobsQueue/WorkerPoolagainstagentflare.db, exposesPOST /api/jobs,GET /api/jobs,GET /api/jobs/events(SSE live list) andGET /api/jobs/{id}/stream(SSE live stdout tail), and adds a newJobsdashboard page.Test plan
cargo test -p agentflare-jobs— 19 tests passcargo test --bin agentflare dashboard::server— 16 tests passcargo clippy --workspace --all-targets— no new warnings introducedSummary by CodeRabbit
--yes-expose) before exposing unauthenticated data on non-local hosts.