Discord's CDN edges occasionally advertise `content-encoding: gzip` (or
deflate/brotli) on PNG/JPEG passthroughs while the body is raw,
uncompressed image bytes. With the default `reqwest::Client::new()` and
the workspace's gzip/deflate/brotli features all enabled, reqwest's
transparent-decompression layer chokes on the PNG/JPEG header and
returns "error decoding response body" only on `bytes().await` (not on
`send()`), causing `download_image_to_blocks` to silently fall back to a
text-only block — the user's image never reaches the model.
Build the client explicitly with no_gzip/no_deflate/no_brotli so the
request advertises identity encoding and the body is read raw. Also set
a User-Agent (some CDN edges 403 clients without one) and a 30s timeout
aligned with the upstream 5 MB cap.
Repro: send an image attachment via Discord; the daemon logs
`Failed to read image bytes: error decoding response body` and the turn
appends as text-only with `appended_has_image=false`. After this fix the
PNG bytes are read and emitted as an Image content block as intended.
Discord MESSAGE_CREATE payloads with attachments were previously parsed
in a way that either dropped the attachment (when text was present, only
the text was kept) or dropped the whole message (when text was empty,
the early `content.is_empty()` return killed bare-image posts). The
result on text-only providers like claude-code: silent drops, then
hallucinated acknowledgements of content the model never saw.
This rewires the inbound path end-to-end:
* types: add ChannelContent::Multipart(Vec<ChannelContent>) so a single
inbound message can carry a caption + one or more attachments as
sibling blocks. Doc forbids nesting; consumers debug_assert.
* discord: classify attachments by MIME (with extension fallback for
bot-relayed payloads that omit content_type) and a 5 MB vision-size
cap matching Anthropic's image block limit. Vision-eligible images
become ChannelContent::Image; everything else becomes File. Emit
Multipart whenever text and attachments coexist, or when there are
multiple attachments.
* bridge: flat-map Multipart in both dispatch paths — into Vec<ContentBlock>
for multimodal-capable providers, and into a newline-joined text
descriptor for text-flatten providers.
* telegram: add the Multipart arm to send_to_user for exhaustive-match
parity; flattens defensively.
* claude_code driver: render Image blocks as
"[attachment: <mime> image, ~N KB — not viewable on this provider]"
instead of dropping them. The model still cannot see the image, but
it can acknowledge it coherently rather than confabulating.
Adds 9 discord parser tests covering all (text, attachment-count) shapes
plus MIME edge cases, and 2 claude_code driver tests covering captioned
and bare-image rendering.
Adds a single tracing::debug! at the top of parse_discord_message that
dumps the full payload JSON. Silent at default `info` level; enable with
`RUST_LOG=openfang_channels::discord=debug` to capture real attachment
JSON when developing the file-passing parse code.
Logs before any filters (bot, allowed_users, allowed_guilds, empty
content) so attachment-only messages are visible too.
Pick 3a-bis of the Discord file-passing plan: teach the multimodal
image fetcher to handle file:// URLs by reading from local disk
instead of going through reqwest. PR-A (Discord inbound) will
materialize attachments to a shared inbox dir and emit
ChannelContent::Image { url: "file://..." }, so this branch is what
unblocks vision on inbox-materialized images after the Discord CDN
URL has expired.
Implementation:
- Branch on url.strip_prefix("file://"); local read uses tokio::fs::read.
- HTTP path unchanged. Both paths converge on (Vec<u8>, Option<String>)
before the existing 5MB cap, magic-byte sniffing, and base64 path.
- No content-type header on file:// — magic-byte detection and URL
extension fallback do all the media-type work, which is fine since
detect_image_magic and media_type_from_url already exist.
- No new deps. Vec<u8> instead of bytes::Bytes to avoid pulling in
the bytes crate as a direct dep.
- No URL percent-decoding: the inbox writer (PR-A) controls filenames
and avoids characters that would need encoding.
Refs: projects/openfang-fork/discord-file-passing-plan.md (step 2)
Pick 3a of the Discord file-passing plan: extend the URL-flavored File
variant with optional mime and size metadata so adapters can pass
attachment context through to bridges. FileData (bytes-flavored) is
unchanged; size is implicit in data.len() and mime_type already exists.
Match-arm sites in bridge.rs, telegram.rs, whatsapp.rs use `..` to stay
forward-compatible. Construction sites in telegram.rs and kernel.rs
pass `mime: None, size: None` for now; Discord inbound (PR-A) will
populate them.
Refs: projects/openfang-fork/discord-file-passing-plan.md
The TOML-vs-DB change detection at boot only checked a subset of fields,
causing edits to workspace, schedule, resources, autonomous, and exec_policy
to be silently ignored after a restart.
Add the missing fields to the changed-detection predicate so the kernel
properly reflects all agent.toml edits in the SQL database. The workspace
comparison is intentionally guarded — if the TOML omits workspace (None),
the kernel-assigned default path already stored in the DB is kept rather
than being overwritten with None.
Derive PartialEq on ScheduleMode, AutonomousConfig, ResourceQuota, and
ExecPolicy to enable the comparisons without manual field-by-field expansion.
Co-authored-by: octo-patch <octo-patch@github.com>
* fix(channels): key router on user, not channel (Discord/Slack)
Discord and Slack adapters set sender.platform_id to the channel/conversation
ID (needed for the send path), so router.resolve(channel, sender.platform_id, ..)
was matching peer_id bindings against the channel ID and never finding the
user-keyed binding. The sender_user_id() helper already existed but only the
rate-limit/authz paths used it; the routing reads did not.
Read-path fix:
- discord.rs / slack.rs: stash author/user ID in metadata["sender_user_id"]
- bridge.rs: route the text and audio paths through sender_user_id(message)
- bridge.rs: thread user_id through handle_command() so the 6 CLI slash-command
resolves (/new, /compact, /model, /stop, /usage, /think) also key on user.
Tests updated for the new signature.
Write-path follow-up (set_user_default + broadcast routing) deferred to a
separate commit so this change can be validated in isolation.
* fix(channels): close write-path keying gap; broadcast user-scoped
Completes the router keying fix started in 6a90aa0. The read path
resolves on user_id, but four write sites and the broadcast lookup
were still keyed on sender.platform_id (channel ID on Discord/Slack),
producing the split-keying state that surfaced in GAP-008.
- 4 x set_user_default writes (text/audio fallback, /agent existing,
/agent spawned) now key on sender_user_id(message)
- 2 x broadcast lookups (has_broadcast / resolve_broadcast) switched
to sender_user_id(message), matching the upstream test's intent
(router.rs:521-547 keys on "vip_user", not a channel)
- boot-time log warning on Discord/Slack adapter start: any
pre-existing /agent default may need to be re-run once
- new test test_handle_command_agent_select_keys_on_user_id_not_
platform_id locks in the round-trip
Adds an optional `audio_base_url` field to `MediaConfig` that overrides
the hardcoded provider URLs in `media_understanding::transcribe_audio`,
allowing the same OpenAI-compatible multipart wire format to be sent to
a local Whisper service (speaches, faster-whisper-server, LM Studio,
etc.) instead of api.openai.com / api.groq.com.
Closes#1051.
## Why
Self-hosted, sovereignty-conscious, or rate-limited deployments often
need to route audio transcription to a local Whisper backend while
keeping `media_transcribe` / `speech_to_text` working as native tools
(no helper scripts, no shell_exec workarounds). Today the URLs in
`media_understanding.rs:118-128` are literal `&'static str` so neither
`OPENAI_BASE_URL` nor `provider_urls` (which the LLM drivers do
respect) is read for audio. The same problem existed for embeddings
and was already addressable via `provider_urls`, so this change keeps
the pattern symmetric for media at the simplest possible surface area.
## Wire format
The endpoint shape and Authorization header remain identical:
POST <audio_base_url>/v1/audio/transcriptions
Authorization: Bearer $<provider>_API_KEY
Content-Type: multipart/form-data
fields: file (binary), model, response_format=text
This means **any OpenAI-compatible Whisper server is drop-in**
(Speaches, faster-whisper-server, LM Studio's Whisper server, etc.).
Local servers typically accept any non-empty bearer string, so users
can keep `OPENAI_API_KEY=anything` for the auth header.
## Configuration
```toml
[media]
audio_provider = "openai"
audio_base_url = "http://127.0.0.1:8000"
# → POST http://127.0.0.1:8000/v1/audio/transcriptions
```
Or for Groq-compatible local servers:
```toml
[media]
audio_provider = "groq"
audio_base_url = "http://127.0.0.1:9000"
# → POST http://127.0.0.1:9000/v1/audio/transcriptions
```
Trailing slash on the user-supplied base is stripped to avoid double
slashes in the final URL.
## Backward compatibility
- `MediaConfig` already uses `#[serde(default)]`, so existing
configs without `audio_base_url` deserialize as `None` and behave
exactly as before (cloud provider URLs).
- `Default` impl extended; `audio_base_url: None`.
- `parakeet-mlx` provider path unaffected (it's a separate code branch).
- No new dependencies, no breaking changes to public API.
## Tests
- `test_media_config_default` extended to assert `audio_base_url.is_none()`.
- `test_media_config_audio_base_url_serde_roundtrip` — set + JSON roundtrip.
- `test_media_config_backward_compat_no_audio_base_url` — legacy JSON
parses with the new field as None.
- `test_audio_base_url_override_logic` — pure-function test that
exercises the URL building branch (default URLs preserved when
unset, override applied for both providers, trailing-slash strip).
The runtime branch in `transcribe_audio` was kept as a straight
`if Some/else default` rather than a helper function to minimize the
diff and keep the patch obviously safe to review.
## Operational note
This change does not affect anyone running the cloud provider URLs
out of the box. The override is opt-in via a single optional config
field. Useful for users like myself running a local Speaches container
behind a reverse proxy and a chat-only LLM key (z.ai Coding Plan)
that can't satisfy openai.com's audio endpoint.
Linked: #1051 (Configurable STT/TTS/image URLs and local backends).
Co-authored-by: Miguel Guerrero <kortux@gmail.com>
Follow-up to 79aa34c. The previous commit added the public surface
(DriverConfig field + OPENFANG_SUBPROCESS_TIMEOUT_SECS env var) but
left every DriverConfig construction site hardcoded to None — so the
struct field was wired but had no on-disk source feeding it. The env
var was the only operator-facing knob.
This commit plumbs the missing layer: the timeout is now deserializable
from config.toml on both the primary and global-fallback providers.
Public surface
- DefaultModelConfig.subprocess_timeout_secs: Option<u64>
- FallbackProviderConfig.subprocess_timeout_secs: Option<u64>
- Both fields are #[serde(default)] — existing config.toml files
without the field deserialize cleanly to None (no breaking change).
Placement rationale
- Per-provider on each config struct, not a top-level field or a new
[driver] section. This matches the existing per-provider config shape
and lets operators set different timeouts for primary vs. fallback
(e.g. tighter timeout on a fast fallback to fail over sooner). If a
second driver-level setting ever lands, refactoring two struct fields
into a [driver] section is cheap; we don't pre-pay for it now.
Wiring (kernel.rs)
- L663 primary driver ........ pulls config.default_model.subprocess_timeout_secs
- L687 auto-detect path ...... inherits default_model intent (the swap
is replacing the *provider*, not the
timeout policy)
- L736 global fallback loop .. pulls fb.subprocess_timeout_secs
- L5031 agent primary ......... inherits effective_default's value when
agent_provider == default_provider;
None for cross-provider overrides
- L5108 agent manifest fallback inherits dm's value when the manifest
fallback resolves to "default" (matching
the existing fb.provider sentinel logic);
None for explicit cross-provider entries
- L5139 global fallback (per-agent loop) — pulls fb.subprocess_timeout_secs
Sites kept as None (intentional)
- agent_loop.rs:1146, 1330: ModelNotFound recovery iterates over the
agent manifest's fallback_models (FallbackModel, not the config-toml
type) — no per-provider config in scope.
- routes.rs:7701: provider connectivity test endpoint; no config source.
- routes.rs:7529: dashboard hot-update path constructs a fresh DM with
defaults (None) — operator sets timeout via config.toml, not via the
set-key flow.
Tests
- test_subprocess_timeout_secs_in_toml: round-trips a TOML doc with
default_model.subprocess_timeout_secs = 600 and one fallback at 180,
one fallback omitted; asserts each value (or None) reaches the parsed
config struct.
- test_subprocess_timeout_secs_omitted_defaults_to_none: asserts a
legacy-shaped config.toml (no timeout fields) parses cleanly with
both fields = None — backward-compat guard.
- 4 existing claude_code driver timeout tests still pass.
Mechanical pass-throughs
- 8 test fixtures across openfang-kernel/tests and openfang-api/tests
gain subprocess_timeout_secs: None on their DefaultModelConfig
literals.
- 1 production literal in routes.rs gains the same field.
- The existing FallbackProviderConfig serde-roundtrip test gains
subprocess_timeout_secs: None plus an assertion.
Precedence comment in drivers/mod.rs::create_driver updated to reflect
that the config-field path is now real, with explicit pointers to the
kernel.rs wiring sites for future contributors.
Validated: cargo check --workspace --tests is clean; openfang-types
(362), openfang-runtime (933), and openfang-kernel (260) lib tests
all pass.
The claude-code driver hardcodes its per-message turn timeout inside
ClaudeCodeDriver and exposed no operator-facing knob, so long-running
CC subprocess turns (large prompt-caches, deep tool chains) hit the
internal default with no escape hatch. Adds a public config surface,
honored today only by the claude-code driver, designed so future
subprocess drivers can opt in without re-shaping the API.
Public surface
- DriverConfig.subprocess_timeout_secs: Option<u64> (llm_driver.rs)
- OPENFANG_SUBPROCESS_TIMEOUT_SECS env var (drivers/mod.rs)
- Precedence in create_driver(): env var > config field > driver default
Naming rationale
- Field/env are scope-flavored, not semantic, on purpose: the name
telegraphs that HTTP providers (default/Anthropic, openai, bedrock,
qwen-code) accept-but-silently-ignore the field today. A semantic
name (message_timeout_secs) would have invited the same silent-no-op
footgun on those providers.
- Driver-internal field in claude_code.rs intentionally kept as
message_timeout_secs — it's not on the public boundary and the
semantic name accurately describes what it stores.
Tests (drivers/mod.rs)
- default_when_unset: no env, no config -> driver default
- config_set: config field flows through
- env_overrides_config: env var wins over config (construction-only
assertion; trait-object opacity prevents reading the value back)
- malformed_env_falls_through: unparseable env silently falls through
to config, matching the .parse::<u64>().ok() chain in production
- All four tests scrub OPENFANG_SUBPROCESS_TIMEOUT_SECS pre/post to
avoid cross-test pollution
Mechanical pass-throughs
- 12 x DriverConfig { .. } test fixtures in drivers/mod.rs gain
subprocess_timeout_secs: None
- routes.rs (1), kernel.rs (6), agent_loop.rs (2): same pass-through
fills in DriverConfig literals; no logic touched
Forward-compat note
- A NOTE block in drivers/mod.rs flags the scope-vs-implementation
gap so the next contributor adding a subprocess driver knows
exactly where to wire the config in.
Validated end-to-end against a live daemon: dry-run + full deploy
(deploy-local.sh, all 7 phases) + post-swap agent_send round-trip
through the claude-code dispatch path.
Completes the router keying fix started in 6a90aa0. The read path
resolves on user_id, but four write sites and the broadcast lookup
were still keyed on sender.platform_id (channel ID on Discord/Slack),
producing the split-keying state that surfaced in GAP-008.
- 4 x set_user_default writes (text/audio fallback, /agent existing,
/agent spawned) now key on sender_user_id(message)
- 2 x broadcast lookups (has_broadcast / resolve_broadcast) switched
to sender_user_id(message), matching the upstream test's intent
(router.rs:521-547 keys on "vip_user", not a channel)
- boot-time log warning on Discord/Slack adapter start: any
pre-existing /agent default may need to be re-run once
- new test test_handle_command_agent_select_keys_on_user_id_not_
platform_id locks in the round-trip
Discord and Slack adapters set sender.platform_id to the channel/conversation
ID (needed for the send path), so router.resolve(channel, sender.platform_id, ..)
was matching peer_id bindings against the channel ID and never finding the
user-keyed binding. The sender_user_id() helper already existed but only the
rate-limit/authz paths used it; the routing reads did not.
Read-path fix:
- discord.rs / slack.rs: stash author/user ID in metadata["sender_user_id"]
- bridge.rs: route the text and audio paths through sender_user_id(message)
- bridge.rs: thread user_id through handle_command() so the 6 CLI slash-command
resolves (/new, /compact, /model, /stop, /usage, /think) also key on user.
Tests updated for the new signature.
Write-path follow-up (set_user_default + broadcast routing) deferred to a
separate commit so this change can be validated in isolation.
`fire_reaction` calls `setMessageReaction` fire-and-forget on every
agent lifecycle event. When Telegram returns a terminal error like
`REACTION_INVALID` (emoji not in the bot's free-reaction allowlist),
`REACTION_NOT_AVAILABLE` (chat admin restricted this emoji), or
`REACTION_TOO_MANY` (per-message cap), retrying on every subsequent
turn is pointless log spam and wasted API quota.
This adds a per-bot-instance `HashSet<(i64, String)>` keyed by
`(chat_id, emoji)` that records terminal rejections and short-circuits
future calls for the same pair. Keyed by chat, not just emoji, because
`Chat.available_reactions` varies across chats and is admin-mutable
(https://core.telegram.org/bots/api#setmessagereaction) — an emoji
rejected in chat A may still be valid in chat B. Cache is
per-process; on restart it rebuilds naturally, which handles any
runtime allowlist change without needing persistence.
The terminal-error match uses a small private helper
`is_terminal_reaction_error` that substring-matches the three
permanent errors. Transient errors (429, 5xx, `MESSAGE_NOT_MODIFIED`,
unrelated 400s) are deliberately NOT cached.
Concurrency: the cache uses `std::sync::Mutex` — critical section is
two `HashSet` ops (contains + insert), never held across `.await`.
Endorsed by the Tokio shared-state tutorial
(https://tokio.rs/tokio/tutorial/shared-state) for exactly this shape.
Two concurrent `fire_reaction` calls for the same (chat, emoji) can
both pass the cache check before either rejection lands, producing up
to N duplicate API calls on the first rejection; the duplicate
`insert` is idempotent so this is benign and self-limits on the
second turn. Documented in-code.
Tests: 6 new tests covering terminal-error matching, cache insertion,
per-chat key isolation, and non-caching of transient and successful
responses. Total 47 telegram tests pass (41 existing + 6 new). No new
clippy warnings.
The six outbound helpers in the Telegram adapter (sendMessage, sendPhoto,
sendDocument, sendDocument_upload, sendVoice, sendLocation) previously
logged a `warn!` on HTTP non-success and still returned `Ok(())`. Callers
interpreted that as successful delivery and told the agent "Message sent"
even when Telegram had rejected the request (e.g. 400 Bad Request from
malformed HTML entities with parse_mode=HTML). The agent recorded phantom
success in its session history, corrupting subsequent behavior.
The fix returns `Err(format!(...).into())` on HTTP non-success in all six
helpers, matching the error-handling convention documented in
CONTRIBUTING.md.
`api_send_message` is slightly different because it splits long messages
into chunks via `split_message(4096)`. Naively returning `Err` on any
chunk failure would create a partial-delivery-then-error regression —
worse than the original silent success. The function now tracks
`delivered_any` across chunks:
- First-chunk failure (nothing delivered yet) → return `Err` to surface
the failure. This is where the motivating HTML-parse-error bug lives,
so the fix is fully effective.
- Subsequent-chunk failure (user already received preceding chunks) →
log `warn!` and continue with best-effort delivery, matching the
convention used by every other adapter in the crate that calls
`split_message` (Discord, Gitter, Mattermost, Nextcloud, Twitch,
Pumble, etc.).
Tests: 4 new tests using a small in-crate stub server (axum on an
ephemeral port, reached via the existing `api_url` constructor seam —
zero new dependencies). 41 telegram tests pass (37 existing + 4 new).
Node/npx-backed stdio MCP servers (Gmail, AgentMail, Exa, etc.) need a
usable HOME directory for npm cache and temp-file scratch space. Without
it, npm errors with EACCES on /nonexistent or silently falls over when
trying to write cache entries.
Previously these three variables were only passed on Windows. Linux and
macOS hosts launching stdio MCP servers through npx would get an empty
env for HOME/TMP/TEMP, breaking most community MCP servers.
Move the HOME/TMP/TEMP passthrough above the cfg!(windows) block so it
applies to every platform. Remove the now-redundant entries from the
Windows-only list.
Fixes#1089
run_agent_loop_streaming skipped the touch_agent() call that the
non-streaming run_agent_loop performs before every LLM request. On slow
local inference (e.g. Ollama qwen3.5:35b, multi-minute generations),
last_active went stale and the heartbeat monitor flagged the agent as
unresponsive, triggering crash recovery mid-stream. With multiple agents
sharing one Ollama instance, queued agents appeared frozen while the
active one generated.
Mirror the non-streaming behavior: stamp last_active immediately before
stream_with_retry so the heartbeat window covers the full LLM call.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ben Hoverter