feat(chat): support PDF and text/code file attachments end-to-end#3094
Conversation
Dashboard's attachment picker only allowed image/png|jpeg|webp|gif and
the agent loop's resolve_attachments() silently dropped anything else,
so uploading a .md or .pdf produced a 200 OK upload that the LLM never
saw — the model would respond "please paste the content you want me to
summarize" because the attachment text never reached the prompt.
This change wires the missing types end-to-end:
- librefang-runtime/pdf_text: new module wrapping
pdf_extract::extract_text_from_mem with panic isolation (lopdf can
panic on malformed/encrypted PDFs) and a 200K-char truncation cap
so a 200-page report does not blow the context window.
- routes/agents::resolve_attachments: branches on image/* (existing),
application/pdf (new — extract_text_from_pdf), and text-like files
(new — covers text/*, application/{json,xml,yaml,toml,…}, plus
extension fallback for code files browsers tag with empty or
octet-stream content_type: .rs, .py, .go, .ts, .json, .toml, etc.).
Unknown types now warn so future drops are diagnosable instead of
silent. Adds INFO logs at every successful resolve / inject so it is
obvious from daemon.log whether an upload made it into the LLM.
- routes/agents::is_allowed_content_type: extended exact-match list
with json / yaml / toml / x-ipynb+json / sql / graphql / javascript
/ typescript, plus a text/* allow rule that explicitly excludes
text/html and text/xml (XSS / XXE per the existing security
rationale). All audio/image curated lists unchanged.
- session_repair: after merging consecutive same-role messages, run a
new coalesce_adjacent_text_blocks pass per message so the resulting
Blocks contain at most one Text run plus image/tool/thinking blocks.
This matters because the inject_attachments_into_session +
agent_loop user-text flow naturally produces Blocks([Text(attach),
Text(prompt)]); small chat-tuned local models behind Ollama /
llama.cpp / vLLM / LM Studio frequently attend only to the first or
last Text part of a multi-part user message and drop the rest.
Doing this at the repair layer means every driver downstream
(Anthropic, OpenAI, Gemini, Groq, ChatGPT, …) gets a normalized
shape for free — no per-driver special case.
- llm-drivers/openai: drop the obsolete is_ollama_like() flatten
branch added during initial debugging; session_repair handles it
generically now. Keep the "single Text part → plain string" fast
path because some OAI-compatible backends mishandle 1-element
arrays too.
- dashboard/ChatPage: ATTACHMENT_ACCEPT now lists the wider MIME +
extension set; isSupportedFile mirrors backend's is_text_like
decision so drag/paste cannot bypass with an unsupported type;
attach + mic + send buttons switched to items-start so they stay
pinned to the top of the textarea when it grows multi-line.
- ws.rs: WebSocket lifecycle gets per-connection conn_id (uuid) +
client_ip + explicit_session on connect, and reason / duration_secs
/ close_code on disconnect, so concurrent reconnects (same
agent_id, different conn_id) and idle-vs-explicit-close can be
distinguished from the daemon log.
- agent_loop / session_repair: per-merge DEBUG with target_idx, role,
char counts; WARN now carries messages_before / messages_after so
you can see exactly how a 21-message session collapsed to 17.
Tests: pdf_text panic-resistance + truncation; session_repair coalesce
across single-block / image-boundary / text-only / empty cases; an
end-to-end test that simulates inject_attachments_into_session +
user-text-message + repair and asserts the LLM-bound Blocks contain a
single coalesced Text in source order.
Verified locally with just dev + ollama gemma4: uploading a Chinese
markdown file plus the prompt "总结一下" now produces a real summary
instead of "please paste the content".
The dashboard's tsc has been rejecting the build for a while because: - PageHeader doesn't declare a `badge` prop, but ten pages (A2APage, AgentsPage, AnalyticsPage, ApprovalsPage, ChannelsPage, CommsPage, GoalsPage, HandsPage, RuntimePage, SchedulerPage, SessionsPage, SettingsPage, SkillsPage, TelemetryPage, WorkflowsPage) pass one. Add the optional prop and render it as a small label next to the title — matches the existing visual treatment in those pages. - SchemaForm.validateSectionRequired / validateRequired pass `f.required` (boolean | undefined) to `hasInvalidNumberValue`, which expects boolean. Coerce with `?? false`. Surfaced while building this PR; not strictly part of the attachment work but the typecheck blocks any dashboard build.
c077667 to
51ec03e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c077667e90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if existing_meta.is_none() { | ||
| *existing_meta = provider_metadata; |
There was a problem hiding this comment.
Preserve per-part metadata when coalescing text blocks
coalesce_adjacent_text_blocks merges adjacent ContentBlock::Text entries but drops later provider_metadata whenever both sides are populated, because only the first metadata value is retained. Gemini uses per-part thought_signature in this field and expects those signatures to be echoed on subsequent turns, so a history containing adjacent Gemini text parts can lose required signatures during repair and cause follow-up requests to fail. This coalescing should either be limited to metadata-free attachment/user text or preserve metadata for every merged part.
Useful? React with 👍 / 👎.
| } | ||
| Err(e) => { | ||
| tracing::warn!(file_id = %att.file_id, error = %e, "Failed to read upload for attachment"); | ||
| } else if content_type == "application/pdf" { |
There was a problem hiding this comment.
Normalize PDF MIME before attachment type dispatch
resolve_attachments checks PDFs with an exact content_type == "application/pdf" match, but uploads are accepted after mime_base normalization and can legally include parameters (for example application/pdf; charset=binary). Those parameterized PDFs pass upload validation but miss this branch and are skipped, so the attachment text is silently dropped from model context. Use normalized MIME for dispatch here as well.
Useful? React with 👍 / 👎.
Two CI failures on this PR: 1. crates/librefang-runtime/src/pdf_text.rs:58 — clippy `explicit_counter_loop` lint (denied via `-D warnings`). Replace the manual `count` counter with `.enumerate()`. Behavior unchanged: the loop still pushes `MAX_PDF_TEXT_CHARS` chars and then appends the truncation marker. 2. crates/librefang-api/src/routes/agents.rs:4929 — unit test `test_upload_mime_allowlist_rejects_previously_accepted_types` listed `application/javascript` as an MIME that must be rejected, but this PR explicitly added it (and its siblings) to `EXTRA_ALLOWED_UPLOAD_TYPES` to support code-file attachments — the stated feature of the PR. The server stores uploads, never executes them, and the agent loop reads them as plain UTF-8 (per the doc-comment on `is_allowed_content_type`), so JS is safe to inline as a code attachment. Drop it from the rejected list. `text/html`, `text/xml`, `application/octet-stream` and the dangerous image subtypes remain blocked.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
resolve_attachments() compared raw header values against exact MIMEs
("application/pdf", "image/..."). A request with charset parameters or
mixed case ("Application/PDF; charset=binary") would pass the upload
allowlist (which uses mime_base) but skip every branch in the resolver
and silently drop the attachment. Normalize via mime_base before
matching to keep the two paths consistent.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…g / sdk additions Round 2 of the zh translation pass — picks up the seven mid-priority files that the first commit didn't cover. All sections mirror the EN content from #3189 but read naturally as Chinese (terms like agent / session / fallback / NoEntry left in English where they're API surface, prose translated to 中文). - agent/templates: Live Context section, .identity/ vs root fallback, cache_context = true opt-out. - api/agents: attachment-resolution table + trajectory export endpoint (#3025 / #3094 / #3097). - api/system: POST /api/tools/{name}/invoke endpoint + fail-closed allowlist explanation. - cli/commands: 'Slash Commands in Chat' section listing CLI registry commands and pointing at channel commands. - providers/tools: 'CLI logins as first-class default providers' detection priority and override. - troubleshooting: 'Audit checks' subsection + table. - sdk: 'Auto-generated from openapi.json' section.
…pi / observability (#3189) * docs(providers): add Novita / SearXHG / Bedrock entries * docs(config): document [parallel_tools] section + max_history_messages * docs(scheduler): cron multi-delivery + pre_script + silent_marker * docs: live context.md, chat attachments, CLI slash commands Salvages three of the six items from the (quota-killed) misc-features agent run. Each is a focused append, no rewrite of existing content. - agent/templates: live Live Context section explains the per-turn re-read of `context.md` (#3115), the .identity/ vs root path lookup, the 32 KB cap with stale-cache fallback, and the cache_context opt-out. - integrations/api/agents: attachment-resolution table (image / PDF / text+code) covering the supported MIME / extension list and the 200 KB truncation, plus a note about adjacent-text coalescing for small chat-tuned local models (#3094). - integrations/cli/commands: 'Slash Commands in Chat' section listing the registry-backed CLI commands (/model /status /help /clear /kill /exit) and pointing at channel-side commands for the broader set (#3122 #3123). * docs: tool invoke API, trajectory export, doctor audit, CLI default-pick, SDK codegen Picks up the items the second misc-features agent didn't get to before hitting the org quota. - integrations/api/system: POST /api/tools/{name}/invoke (#3025) — fail- closed allowlist, agent_id requirement for approval-gated tools. - integrations/api/agents: GET /sessions/{id}/trajectory (#3097) — json/jsonl formats, redactor pipeline, audit use case. - operations/troubleshooting: 'Audit checks' subsection enumerating the three registered AuditChecks (#3082) — VaultKey, ApiListenAddr, ConfigTomlSchema — with what each asserts and how to extend. - configuration/providers/tools: 'CLI logins as first-class default providers' section (#3061) — detection priority claude-code > codex- cli > gemini-cli > qwen-code, override via [default_model] in config.toml. - integrations/sdk: 'Auto-generated from openapi.json' section (#3046) — scripts/codegen-sdks.py, pre-commit hook, openai-tag skip rule. * docs(zh): mirror providers / config-core / cron / features-index sections * docs(zh): mirror agent / api / cli / providers-tools / troubleshooting / sdk additions Round 2 of the zh translation pass — picks up the seven mid-priority files that the first commit didn't cover. All sections mirror the EN content from #3189 but read naturally as Chinese (terms like agent / session / fallback / NoEntry left in English where they're API surface, prose translated to 中文). - agent/templates: Live Context section, .identity/ vs root fallback, cache_context = true opt-out. - api/agents: attachment-resolution table + trajectory export endpoint (#3025 / #3094 / #3097). - api/system: POST /api/tools/{name}/invoke endpoint + fail-closed allowlist explanation. - cli/commands: 'Slash Commands in Chat' section listing CLI registry commands and pointing at channel commands. - providers/tools: 'CLI logins as first-class default providers' detection priority and override. - troubleshooting: 'Audit checks' subsection + table. - sdk: 'Auto-generated from openapi.json' section. * docs: refresh /new slash-command semantics (#3071 review fix) PR review on #3189 (houko) caught two stale references describing the pre-#3071 behavior of /new ('wipe the session' / 'clear history'). The new semantics fork off a fresh session while the previous one stays resumable on the same channel — phrasing updated in 4 places (en + zh × 2 files), plus a one-line clarifier in the slash-command roster section so the cross-reference doesn't silently inherit the old mental model: - configuration/channels: 'wipe the session' → 'fork off into a new session (the prior conversation stays resumable on the channel)' - integrations/api/realtime: '(clear history)' → 'forks into a fresh session — the prior session is preserved and remains resumable' - integrations/cli/commands roster paragraph: explicit note that /new is a fork, not a wipe. Same wording mirrored to docs/src/app/zh/. * docs(channels): 9 channel-related Apr 22-25 PRs (en + zh) Covers the channel + bridge feature wave: - #3020 per-agent ChannelOverrides — fixes the contradicting 'without modifying the agent manifest' phrasing in both en and zh. - #3077 prefix_agent_name + Signal plain-text default - #3009 Slack reactions_enabled + #3005 Feishu @mention preservation consolidated into a 'Reactions and Processing State' section - #3008 Signal media attachments - #3012 WhatsApp send_voice + dm_policy / group_policy - #3011 Webhook deliver_only mode - #2972 channel file downloads (file_download_dir / max_bytes config) * docs(config-core): provider timeout / browser CDP / cron session caps (en + zh) Three new `config.toml` sections that landed Apr 22-23: - #3004 `[provider_request_timeout_secs]` map - #2993 / #2991 `[browser].cdp_endpoint` + `cdp_auth_token_env` - #2994 `[kernel].cron_session_max_tokens` / `cron_session_max_messages` * docs(api): GET /attach + Owner Notice envelope (en + zh) - #3078 SSE attach endpoint for multi-client co-watching — describes the per-session SessionStreamHub fan-out, attacher behaviour, and the curl example. - #2965 ReplyEnvelope.owner_notice + notify_owner tool — message body field, StreamEvent variant, WhatsApp OWNER_JIDS fan-out. * docs(observability + ops): Tempo stack, cache_hit_ratio metric, daemon log tee (en + zh) - #3064 + #3149 — bundled observability stack (otel-collector / Tempo / Grafana), bring-up commands, [telemetry] config, business spans, cache_hit_ratio metric formula and where it surfaces - #3022 — `librefang start --foreground` now tees logs to ~/.librefang/logs/daemon-YYYY-MM-DD.log; same-day append; 7-day rotation pruned on daemon start * docs: hooks transform / lazy tools / mcp taint / capability detection / providers extras Final batch of catch-up docs covering the remaining Apr 22-25 user-facing features: - agent/hooks: transform_tool_result hook contract (#3003) - agent/templates: tool_search / tool_load lazy loading + lazy_tools opt-out (#3047) - integrations/mcp-a2a: TaintRuleId enum + per-tool skip_rules policy (#2999) - configuration/features: compaction summaries in user's conversation language (#3007) - configuration/providers/local: embedding auto-detection priority list (#3099) + Ollama capability detection (Modality / metadata pipeline, #3074 / #3133 / #3134 / #3140) - configuration/providers/management: custom image-gen provider via generic OpenAI-compat driver (#2998) + Moonshot file-upload helper via /v1/files (#2966) All sections mirrored to docs/src/app/zh/. * docs: address houko fix-PR audit (8 items, en + zh) PR review caught 1 stale-doc contradiction + 7 undocumented behaviour changes from the fix-PR cohort. - #3114 — configuration/core: 'api_key empty = unauthenticated' was stale; now documents the loopback-only policy (non-loopback bypass was closed) - #3170 — features/observability: auto_start is opt-in; home_dir- scoped Docker labels; RAII cleanup - #2989 — api/agents POST /message: per-request session_id override - #3048 — Interrupting a Running Agent Turn: /stop cascades into agent_send subagents (recursive) - #3056 + #3000 — desktop: dedicated Connection screen via custom URI scheme with retry / open-in-browser / uninstall - #2952 — triggers: response routes to agent's home channel - #2955 — triggers: task_posted assignee_match = 'self' | 'any' - #3069 — providers/local: 60-second reprobe + manual 'Test connection' refresh All sections mirrored in docs/src/app/zh/.
Summary
.mdor.pdfreturned200 OKbutresolve_attachments()silently dropped it, so the model replied "please paste the content you want me to summarize."librefang_runtime::pdf_text(panic-isolatedpdf-extractwrapper, 200K-char cap) and a new text-like branch inresolve_attachments()coveringtext/*,application/{json,xml,yaml,toml,…}, plus extension fallback for code files browsers tag with empty /application/octet-streamcontent_type (.rs,.py,.go,.ts, …).session_repair: after merging same-role messages, run a newcoalesce_adjacent_text_blockspass so the resultingBlockscontain at most one Text run plus image / tool / thinking blocks. Without this, small chat-tuned models behind Ollama / llama.cpp / vLLM / LM Studio attended only to the first or last Text part of a multi-part user message and dropped the attachment. Doing it at the repair layer means every driver downstream gets a normalized shape — no per-driver special case.conn_id+client_ip+explicit_sessionon connect,reason+duration_secs+close_codeon disconnect;messages_before/messages_afteron the session-repair WARN; per-merge DEBUG with role + char counts; INFO at every successful attachment resolve / inject.ATTACHMENT_ACCEPTwidened to the full MIME + extension set,isSupportedFile()mirrors backend'sis_text_like_attachment()so drag/paste cannot bypass with an unsupported type, attach + mic + send buttons switched toitems-startso they pin to the textarea top when it grows multi-line.PageHeaderbadgeprop andSchemaForm'sboolean | undefinedcoercion — both block any dashboard build today.Why now
Reported by a user uploading a Chinese-named markdown to gemma4 via Ollama:
200 OKupload +4096 in / 100 outLLM call + "please paste the content" reply. Three independent bugs stacked: backend silently dropped non-image,is_allowed_content_typerejected most code MIMEs anyway, multi-part user content broke local small models. All three are addressed.Test plan