fix(api): route comms_task through kernel wrapper; surface task system events#2789
Conversation
…m events in comms feed Two related fixes for the task event bus integration (librefang#2788): 1. `POST /api/comms/task` called `memory_substrate().task_post()` directly, bypassing `Kernel::task_post` which publishes `SystemEvent::TaskPosted`. Tasks created via HTTP never hit the event bus, so triggers with `pattern: "TaskPosted"` didn't fire for UI-posted tasks. Route through the `KernelHandle::task_post` wrapper instead. 2. Extend `filter_to_comms_event` to map `SystemEvent::TaskPosted`, `TaskClaimed`, and `TaskCompleted` to their corresponding `CommsEventKind` variants (which already existed) so they appear in `GET /api/comms/events` alongside messages and lifecycle events.
houko
left a comment
There was a problem hiding this comment.
Fix direction is correct — routing through KernelHandle::task_post publishes SystemEvent::TaskPosted on the bus (kernel/mod.rs:12048), which the direct substrate call was skipping. Signature matches, and Result<String, String> is format-compatible with the existing format!("…{e}") error branch.
🐛 Bug: TaskCompleted.source_id will be a junk UUID
SystemEvent::TaskCompleted { task_id, result } => Some(CommsEvent {
...
source_id: event.source.to_string(),
source_name: resolve_name(&event.source.to_string()),In kernel/mod.rs:12125 the event is built with Event::new(AgentId::new(), …) — event.source is a freshly-minted random UUID, not the agent that completed the task. You already correctly avoid this for TaskPosted (uses payload created_by) and TaskClaimed (uses claimed_by). But SystemEvent::TaskCompleted's payload is only { task_id, result } — no completer field. So the comms feed will render a random UUID as the source, and resolve_name will fall back to printing that UUID verbatim.
Options:
- Minimal:
source_id: String::new()+source_name: String::new(), matching howTaskClaimedleavestarget_idblank. - Proper: add
completed_by: StringtoSystemEvent::TaskCompletedand haveKernel::task_completepopulate it (the call site already knows the agent).
Rest looks good.
Addresses houko's review on librefang#2789: SystemEvent::TaskCompleted was constructed with Event::new(AgentId::new(), …), so event.source was a freshly-minted random UUID. The comms feed rendered that UUID as the completer, and resolve_name fell back to printing it verbatim. Proper fix (parallel to task_claim/TaskClaimed): - Add completed_by: String to SystemEvent::TaskCompleted payload. - KernelHandle::task_complete gains an agent_id parameter; kernel impl resolves it via AgentId::from_str → registry.find_by_name, matching task_claim's resolution path. - tool_task_complete now requires caller_agent_id and forwards it. - filter_to_comms_event populates source_id/source_name from completed_by instead of the bogus event.source UUID. - triggers.rs describe-path renders "completed by {agent}" too. Test doubles and the triggers match arm updated for the new field. Workspace build + cargo test -p librefang-api -p librefang-runtime (1035 passed) clean; clippy -D warnings clean.
|
@houko thanks — went with the proper fix in 5b2281e. Threaded completer identity through the whole stack, mirroring
No HTTP handler was affected — there's no Live: |
houko
left a comment
There was a problem hiding this comment.
LGTM。主修复(comms_task 走 kernel.task_post() wrapper)正确,事件发布路径收敛在 kernel 层,mock 和 destructure 点都同步更新了。
一条非阻塞建议:新版 KernelHandle::task_complete 在 agent_id 既非 UUID 又不在 registry 时会早退出错,导致任务不会被标记完成(老实现不校验)。tool_runner 正常路径安全(传 caller UUID),但 trait 是 public 的,建议在 doc comment 里注明这个新前置条件。后续 PR 可以考虑。
…m events (#2789) * fix(api): route comms_task through kernel wrapper; surface task system events in comms feed Two related fixes for the task event bus integration (#2788): 1. `POST /api/comms/task` called `memory_substrate().task_post()` directly, bypassing `Kernel::task_post` which publishes `SystemEvent::TaskPosted`. Tasks created via HTTP never hit the event bus, so triggers with `pattern: "TaskPosted"` didn't fire for UI-posted tasks. Route through the `KernelHandle::task_post` wrapper instead. 2. Extend `filter_to_comms_event` to map `SystemEvent::TaskPosted`, `TaskClaimed`, and `TaskCompleted` to their corresponding `CommsEventKind` variants (which already existed) so they appear in `GET /api/comms/events` alongside messages and lifecycle events. * fix(api,kernel): thread completer identity through task_complete Addresses houko's review on #2789: SystemEvent::TaskCompleted was constructed with Event::new(AgentId::new(), …), so event.source was a freshly-minted random UUID. The comms feed rendered that UUID as the completer, and resolve_name fell back to printing it verbatim. Proper fix (parallel to task_claim/TaskClaimed): - Add completed_by: String to SystemEvent::TaskCompleted payload. - KernelHandle::task_complete gains an agent_id parameter; kernel impl resolves it via AgentId::from_str → registry.find_by_name, matching task_claim's resolution path. - tool_task_complete now requires caller_agent_id and forwards it. - filter_to_comms_event populates source_id/source_name from completed_by instead of the bogus event.source UUID. - triggers.rs describe-path renders "completed by {agent}" too. Test doubles and the triggers match arm updated for the new field. Workspace build + cargo test -p librefang-api -p librefang-runtime (1035 passed) clean; clippy -D warnings clean.
* feat(embedding): add native Cohere driver
Cohere was advertised in the embedding warning text and `detect_embedding_provider`
list, but `create_embedding_driver` had no matching branch — setting
`embedding_provider = "cohere"` fell through to the generic OpenAI-compatible
driver, which built a broken `https://cohere/v1` URL and failed DNS. Users
following the on-screen instructions to set `COHERE_API_KEY` silently got no
embeddings.
Add a dedicated `CohereEmbeddingDriver` that speaks Cohere's native API:
- Endpoint `/v1/embed` (not `/v1/embeddings`)
- Request body `{ texts, model, input_type }` instead of OpenAI's `{ input, model }`
- `input_type` hardcoded to `search_document` — the right choice for indexing
memory/documents, which is the primary embedding path today
- Batch limit of 96 texts per request, failing fast with a clear error
Wire it into `create_embedding_driver` before the OpenAI fall-through, mirroring
the Bedrock special-case. When the caller hands us an OpenAI-style model name
(common when auto-detect fires on a config left at the default
`text-embedding-3-small`), transparently remap to `embed-english-v3.0` with a
warning so users notice and can pin a specific Cohere model in config.
Dimensions are inferred from the official Cohere model list (1024 for v3 full,
384 for v3 light, matching v2 variants).
* refactor(embedding): add InvalidInput variant and make Cohere input_type overridable
Two small cleanups flagged in review of the Cohere driver:
1. Batch-over-limit was returning `EmbeddingError::Unsupported`, which is
semantically meant for "driver doesn't implement this feature" (e.g.
image embeddings). A caller sending 200 texts isn't asking for an
unsupported feature — they're just wrong about the size. Add an
`InvalidInput(String)` variant and route the batch check through it.
2. `input_type` was hardcoded to `search_document`. That's the right
default for indexing-dominant workloads (memory/document ingest), but
query-time and clustering callers would silently get the wrong
semantic bucket. Add a `LIBREFANG_COHERE_INPUT_TYPE` env var override
with whitelist validation against the four valid v3 values
(`search_document`, `search_query`, `classification`, `clustering`).
Invalid values log a warning and fall back to the default rather than
pass through — otherwise Cohere returns a cryptic 400 with no
context.
Env var (rather than a plumbed-through config field) is intentional: it
avoids adding a Cohere-specific knob to the shared `EmbeddingConfig` or
`create_embedding_driver` signature, and it's the kind of setting that
typically gets flipped per-deployment, not per-user.
* refactor(embedding): migrate Cohere driver to v2 API and honor catalog base_url
Two related fixes, both prompted by reconciling this PR with the existing
`librefang-registry` entry for Cohere:
1. **Migrate CohereEmbeddingDriver from `/v1/embed` to `/v2/embed`.**
`librefang-llm-drivers` already targets `api.cohere.com/v2` for chat,
and the registry ships `base_url = "https://api.cohere.com/v2"`, so the
v1 default in this driver was a version split waiting to bite. v2 also
changes the response shape — `embeddings` becomes a dict keyed by
`embedding_types` entry (`{ "embeddings": { "float": [[...]] } }`)
rather than a flat array. Request now sends `embedding_types: ["float"]`
(the only representation we consume), response is deserialized via a
new `CohereEmbeddingsByType` intermediate struct.
2. **Plumb catalog-derived `base_url` to `create_embedding_driver`.**
Both embedding-driver construction paths in `kernel::boot_with_config`
(explicit `[memory].embedding_provider` and auto-detect) were reading
`config.provider_urls` directly, which only contains user overrides
from `config.toml` and ignores the registry-synced catalog. That meant
a provider like Cohere whose registry entry pins `v2` silently lost
that URL and fell back to the hardcoded default in
`create_embedding_driver`. Now we read from `model_catalog` first
(which already has `provider_urls` overrides applied earlier during
boot), fall through to raw `provider_urls`, and only as a last resort
hit the per-provider hardcoded default. Net effect: registry edits to
any provider's `base_url` now actually reach every embedding driver,
not just OpenAI-compatible ones.
Test fixtures updated to v2 URLs.
* refactor(embedding): remove cloud provider URL defaults, require catalog plumbing
Hardcoded base_url defaults for cloud providers (openai, openrouter, groq,
together, fireworks, mistral, cohere, plus the OpenAI-compatible "other"
fallback) are the exact bug class this PR is trying to eliminate: a stale
baked-in URL silently overrides whatever the registry ships, and when the
registry bumps a version (Cohere v1 → v2 being the concrete example that
prompted this PR) nothing in the code breaks — the wrong URL just keeps
getting used.
Now that `kernel::boot_with_config` plumbs `model_catalog`'s base_url
through to `create_embedding_driver`, these defaults serve no real purpose
and just re-introduce the drift risk. Remove them. If a cloud provider
shows up at `create_embedding_driver` without a caller-supplied base_url,
return `EmbeddingError::InvalidInput` pointing the operator at either the
catalog TOML or `config.toml`'s `provider_urls` — far better than
fabricating a URL that might be wrong.
Local providers (`ollama`, `vllm`, `lmstudio`) keep their hardcoded
defaults: their `localhost:PORT` URLs aren't registry-tracked, the ports
are stable conventions, and the fallback is useful for direct/test
callers that construct drivers without a kernel.
Tests updated:
- Cohere-with-key and Cohere-model-remap now pass an explicit v2 base_url
(they previously relied on the deleted Cohere fallback).
- New test `test_create_embedding_driver_cloud_provider_requires_base_url`
pins the "cloud without URL errors out" behavior so it can't regress.
* chore(embedding): stop advertising Groq as an embedding provider
Groq's API has no `/v1/embeddings` endpoint — `api.groq.com/openai/v1/models`
returns only chat completions and Whisper, and a real POST with any embedding
model name 404s with `model_not_found`. But the codebase kept listing Groq
as a valid embedding provider in three places, which meant a user setting
only `GROQ_API_KEY` would hit the auto-detect happy path, build an
`OpenAIEmbeddingDriver`, and then get silent 404s at the first memory
operation — with no clear indication that Groq was the wrong pick.
Fix the three misdirections:
1. Remove `("GROQ_API_KEY", "groq")` from `detect_embedding_provider`'s
priority list. Auto-detect now skips Groq; users who set only
`GROQ_API_KEY` correctly see "no embedding provider available" with
actionable next steps.
2. Update the "No embedding provider" error/warn text in both
`embedding.rs` and `kernel/mod.rs` to drop `GROQ_API_KEY` and add an
explicit parenthetical ("Groq does not expose an embeddings endpoint")
so users who were hoping Groq would work don't go in circles.
3. Update the module-level docstring and `OpenAIEmbeddingDriver` doc to
remove Groq from the "works with" list and point to
`CohereEmbeddingDriver` for Cohere's non-OpenAI-compatible path.
The `kernel::boot_with_config` auto-detect `match detected` arm for `"groq"`
is removed in the same change — it's now unreachable since detect never
returns `"groq"`.
Unrelated lookups that still mention Groq (e.g. `provider_default_key_env`
and the `needs_v1` URL-normalization match) are intentionally left alone:
they fire only when a user explicitly sets `embedding_provider = "groq"`,
which is user-error territory and will surface as a legitimate 404 from
Groq's API with its real error body — far more discoverable than the
previous silent auto-pick path.
* test(embedding): pin `detect_embedding_provider` priority, especially Groq exclusion
`detect_embedding_provider` had zero test coverage. The previous commit
removed Groq from its priority list because Groq has no embeddings
endpoint, but there was nothing stopping a future "completeness" PR from
adding it back and silently reintroducing the 404 bug.
Add four tests that lock the behavior in:
1. `test_detect_embedding_provider_ignores_groq` — with only GROQ_API_KEY
set, detect returns None. This is the regression guard for the Groq
removal.
2. `test_detect_embedding_provider_picks_cohere_when_only_cohere_set` —
exercises the Cohere arm added earlier in this PR.
3. `test_detect_embedding_provider_priority_openai_beats_cohere` — pins
OpenAI at the top of the priority list so reordering can't quietly
change auto-detect behavior for anyone who has multiple keys set.
4. `test_detect_embedding_provider_none_when_nothing_set` — baseline:
empty env returns None.
Each test uses `clear_detect_env`/`restore_detect_env` helpers to avoid
leaking host env state between tests or with other tests that set these
vars (the existing Cohere tests do).
* fix(embedding): drop `.unwrap_err()` on results whose Ok type lacks Debug
Ubuntu/macOS/Windows CI all failed to compile with:
- `CohereEmbeddingDriver` doesn't implement `Debug`
- `dyn EmbeddingDriver + Send + Sync` doesn't implement `Debug`
Both come from `.unwrap_err()` — which requires the Ok type to implement
Debug so it can panic-print on an unexpected Ok. The drivers deliberately
don't derive Debug (would leak api_key through the struct field), and
`Box<dyn EmbeddingDriver>` can't derive Debug unless the trait gains a
Debug supertrait.
Rewrite the two affected tests to match on the `Result` directly via
`matches!(&result, Err(...))`, printing `result.as_ref().err()` on failure
so only `EmbeddingError: Debug` is required.
* chore(embedding): address self-review nits — broader cloud-URL test, Cohere input_type docstring
Follow-up polish from reviewing the Cohere driver additions against the
rest of this PR:
1. **Broaden `test_create_embedding_driver_cloud_provider_requires_base_url`.**
The original test was named for "cloud providers" but only exercised
Cohere. Rename to the plural form and loop across every cloud provider
that should refuse construction without a base_url (openai, openrouter,
mistral, together, fireworks, cohere). Cohere has an earlier api_key
check so we seed COHERE_API_KEY to force the URL branch to fire.
2. **Clarify `CohereEmbeddingDriver.input_type` docstring.** The previous
"v3 models" wording could be read as a Cohere v3 API version when it
meant the `embed-*` v3 model family, and it didn't flag the asymmetric-
retrieval quality implication at all. Rewrite to spell out:
- Cohere's asymmetric retrieval contract (`search_document` at index
time, `search_query` at query time, quality measurably better),
- why librefang pins a single value per driver (the
`EmbeddingDriver` trait has no indexing/querying distinction today),
- why `search_document` is the right single-value default here (memory
ingest dominates),
- how to override for query/clustering-heavy deployments
(`LIBREFANG_COHERE_INPUT_TYPE` env var),
- that a per-call override needs a trait change and is tracked as
follow-up.
* fix(api): route comms_task through kernel wrapper; surface task system events (#2789)
* fix(api): route comms_task through kernel wrapper; surface task system events in comms feed
Two related fixes for the task event bus integration (#2788):
1. `POST /api/comms/task` called `memory_substrate().task_post()` directly,
bypassing `Kernel::task_post` which publishes `SystemEvent::TaskPosted`.
Tasks created via HTTP never hit the event bus, so triggers with
`pattern: "TaskPosted"` didn't fire for UI-posted tasks. Route through
the `KernelHandle::task_post` wrapper instead.
2. Extend `filter_to_comms_event` to map `SystemEvent::TaskPosted`,
`TaskClaimed`, and `TaskCompleted` to their corresponding
`CommsEventKind` variants (which already existed) so they appear in
`GET /api/comms/events` alongside messages and lifecycle events.
* fix(api,kernel): thread completer identity through task_complete
Addresses houko's review on #2789: SystemEvent::TaskCompleted was
constructed with Event::new(AgentId::new(), …), so event.source was a
freshly-minted random UUID. The comms feed rendered that UUID as the
completer, and resolve_name fell back to printing it verbatim.
Proper fix (parallel to task_claim/TaskClaimed):
- Add completed_by: String to SystemEvent::TaskCompleted payload.
- KernelHandle::task_complete gains an agent_id parameter; kernel impl
resolves it via AgentId::from_str → registry.find_by_name, matching
task_claim's resolution path.
- tool_task_complete now requires caller_agent_id and forwards it.
- filter_to_comms_event populates source_id/source_name from
completed_by instead of the bogus event.source UUID.
- triggers.rs describe-path renders "completed by {agent}" too.
Test doubles and the triggers match arm updated for the new field.
Workspace build + cargo test -p librefang-api -p librefang-runtime
(1035 passed) clean; clippy -D warnings clean.
* feat(dashboard): add stop button to interrupt in-flight agent streams (#2787)
* feat(dashboard): add stop button to interrupt in-flight agent streams
The backend already exposes POST /api/agents/{id}/stop (wired to the
kernel's AbortHandle in running_tasks), and the client helper
stopAgent() existed in api.ts, but no UI surfaced it — users had no
way to cancel a long-running agent loop from the chat view.
While a response is streaming, the Send button now morphs into a red
Stop button (Square icon, same position). Clicking it calls stopAgent
for the current agent and optimistically finalizes the streaming
message + releases the per-agent loading flag, so the textarea, voice
button, and Send button re-enable immediately without waiting for the
WebSocket to emit a terminal event. Late response/error frames from
the backend still reconcile the message content via the existing
handlers.
Adds chat.stop_hint translations for en and zh.
* fix(dashboard): prevent stop button from triggering HTTP retry + route via mutation hook
Lift per-turn lifecycle (responded, fallbackTimer, cleanup) into an
activeTurnsRef keyed by agent id so stopMessage can cancel the 180s/30s
watchdog that would otherwise re-send the user's message via HTTP.
Add useStopAgent mutation hook and route ChatPage through it per
dashboard CLAUDE.md convention.
Addresses review on PR #2787.
* feat(channels): show tool execution progress in channel replies (#2792)
Surface tool invocations and failures in the user's channel reply so they
see what the agent is doing — not a silent 8-second wait followed by a
final answer.
Implementation lives in two places:
1. start_stream_text_bridge (api/channel_bridge.rs) — translates
StreamEvent::ToolUseStart into a "🔧 \`tool_name\`" line and
StreamEvent::ToolExecutionResult { is_error: true } into a
"⚠️ \`tool_name\` failed" line, injected into the text delta stream.
Successful tools stay quiet (the model's next prose iteration is
signal enough). PhaseChange events are not surfaced — they fire on
every iteration and are too noisy. A last_progress_tool guard
dedupes back-to-back duplicate ToolUseStart events.
2. dispatch_message non-streaming-adapter branch (channels/bridge.rs)
— now routes through the streaming kernel call so progress events
reach Discord/Slack/Matrix/QQ/Feishu/etc. too. Accumulates deltas
and sends once via send_response so output_format and thread_id
are still honored.
To do this safely, a new trait method send_message_streaming_with_sender_status
returns the kernel's terminal Result alongside the text receiver. The
new branch uses that status to:
- emit AgentPhase::Done vs Error reaction correctly
- record_delivery(success=...) with real outcome
- journal status Completed vs Failed with error string
- honor adapter.suppress_error_responses() so Mastodon (public-feed)
does not leak sanitized agent errors to its timeline
Telegram (only supports_streaming=true adapter) keeps its existing
in-place edit behavior — the progress lines just become part of the
live-edited message. All ~49 other adapters get the progress for free
without any per-adapter code change because the injection happens at
the shared bridge layer.
Tests:
- test_stream_bridge_surfaces_tool_use_progress: ToolUseStart
becomes a visible "🔧" line followed by post-tool prose
- test_stream_bridge_surfaces_tool_failure: failed tools emit
"⚠️ name failed"
- test_stream_bridge_quiet_on_tool_success: successful tools stay
silent (no "✓" spam for chained tools)
- test_stream_bridge_dedupes_consecutive_tool_progress: same tool
name back-to-back emits one line, not three
- test_stream_bridge_status_success / _status_error: oneshot
correctly resolves to Ok/Err
- test_stream_bridge_group_error_suppresses_text_but_reports_err:
is_group=true keeps text out of the channel but the status
oneshot still surfaces the error so metrics and reactions stay
accurate
* fix(embedding): default Cohere auto-fallback to multilingual v3
The auto-fallback that kicks in when `[memory].embedding_model` isn't a
`embed-*` name was hardcoded to `embed-english-v3.0`. For non-English
deployments (librefang is used in Chinese/Japanese/Korean workflows)
that silently embeds multilingual corpora with an English-only model,
which degrades retrieval quality in ways the user can't see until they
notice worse search results.
`embed-multilingual-v3.0` is the right default:
- Same 1024 dims as `embed-english-v3.0` (infer_cohere_dimensions
already handles both; no code path changes)
- Same per-token price
- Supports 100+ languages including English (English quality is only
marginally lower than the English-specialized model)
The warning text is updated to point users at `embed-english-v3.0` and
the `*-light-v3.0` variants as deliberate alternatives, so English-only
deployments can opt back in explicitly rather than drift into the
multilingual default.
* chore(embedding): sync test comment with multilingual-v3 fallback
Previous commit changed the Cohere auto-fallback from
`embed-english-v3.0` to `embed-multilingual-v3.0` but left the test's
doc-comment describing the old behavior. The assertion itself still
passes (both models are 1024 dims), so CI was silent — but the comment
was now actively lying to future readers. Update the comment and
explicitly note that the assertion checks dimensions only, with the
actual fallback target pinned by the warn!() log line rather than the
test.
---------
Co-authored-by: Vignesh Jagadeesh <[email protected]>
Fixes #2788.
Summary
POST /api/comms/tasknow calls the kernel'stask_postwrapper (viaKernelHandle) instead of the memory substrate directly, soSystemEvent::TaskPostedis actually published on the event bus. Triggers withpattern: "TaskPosted"now fire for UI-posted tasks.filter_to_comms_eventnow surfacesSystemEvent::TaskPosted,TaskClaimed, andTaskCompletedas their matchingCommsEventKindvariants, so they appear inGET /api/comms/events.Why
Kernel::task_post(crates/librefang-kernel/src/kernel/mod.rs:12048) wraps the memory write withpublish_event(SystemEvent::TaskPosted{…}). The HTTP handler was calling the substrate beneath it, skipping the publish. Same class of bug as if a CLI command bypassed audit logging.Test plan
cargo build -p librefang-api --libcargo clippy -p librefang-api --lib -- -D warningscargo test -p librefang-api --lib(416 passed)POST /api/triggerswith"pattern":"TaskPosted"+POST /api/comms/task→ verifyfire_countincrements and event appears inGET /api/comms/events.