mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-13 16:14:57 +00:00
feat(tools): local-first document extraction for read_document
Squash merge PR #111 after resolving docs/changelog conflicts. Local checks covered tools/config and both Go builds; PR CI run 26704622503 passed release-versioning, go, and web.
This commit is contained in:
1 parent
80893a1e6c
commit
9ab3d6dfe5
9 files changed
+770
-2
No files matched your search
@@ -10,6 +10,7 @@ import (
|
||||
"github.com/google/uuid"
|
||||
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/agent"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
||||
@@ -86,6 +87,12 @@ func wireExtras(
|
||||
// Register media analysis tools (need mediaStore for file access).
|
||||
readDocumentTool := tools.NewReadDocumentTool(providerReg, mediaStore)
|
||||
readDocumentTool.SetUsageCapService(usageCapSvc)
|
||||
readDocumentTool.SetLocalParser(tools.NewLocalExtractParser(tools.LocalExtractConfig{
|
||||
Enabled: appCfg.Tools.DocumentParser.LocalFirstEnabled(),
|
||||
MaxPages: appCfg.Tools.DocumentParser.MaxPages,
|
||||
Timeout: time.Duration(appCfg.Tools.DocumentParser.TimeoutSec) * time.Second,
|
||||
MinTextLen: appCfg.Tools.DocumentParser.MinTextLen,
|
||||
}))
|
||||
toolsReg.Register(readDocumentTool)
|
||||
readAudioTool := tools.NewReadAudioTool(providerReg, mediaStore)
|
||||
readAudioTool.SetUsageCapService(usageCapSvc)
|
||||
|
||||
+18
-1
@@ -167,7 +167,7 @@ Memory layers: L1 (`memory_search`) returns ranked abstracts; L2 (`memory_expand
|
||||
|---|---|
|
||||
| `read_image` | Analyze/describe images using vision AI (Gemini, Anthropic, OpenRouter, DashScope) |
|
||||
| `read_audio` | Transcribe audio to text using Gemini File API, native OpenAI audio input, or OpenAI-compatible transcription models; unsupported provider/model routes fail closed instead of sending audio as image data |
|
||||
| `read_document` | Extract and analyze documents (PDF, images) via Gemini or Resolve service |
|
||||
| `read_document` | Extract and analyze documents (PDF, DOCX, images). Supports local-first extraction via pdftotext/pandoc before falling back to cloud vision (opt-in via config) |
|
||||
| `read_video` | Analyze/transcribe video content |
|
||||
|
||||
### Skills & Content
|
||||
@@ -217,6 +217,23 @@ User-facing parameter schemas for the most commonly configured tools.
|
||||
- `primary`: provider (`elevenlabs`, `openai`, `edge`, `minimax`).
|
||||
- Per-agent overrides: `agent.other_config.tts_voice_id`, `agent.other_config.tts_model_id`.
|
||||
|
||||
### `document_parser` config shape
|
||||
```json
|
||||
{
|
||||
"local_first": false,
|
||||
"max_pages": 200,
|
||||
"timeout_sec": 30,
|
||||
"min_text_len": 16
|
||||
}
|
||||
```
|
||||
Controls local-first document text extraction in the `read_document` tool.
|
||||
- `local_first`: Enable local extraction via `pdftotext` (PDF) and `pandoc` (DOCX) before cloud vision fallback (default `false` — opt-in). Requires binaries on PATH; present in `full` Docker variant or builds with `ENABLE_FULL_SKILLS=true`.
|
||||
- `max_pages`: Page limit for PDF extraction (default 200). Passed to `pdftotext -l`.
|
||||
- `timeout_sec`: Per-extraction timeout in seconds (default 30). Process group killed on timeout.
|
||||
- `min_text_len`: Minimum characters (after trim) to consider extraction successful; shorter output triggers cloud fallback (default 16).
|
||||
|
||||
**Note:** Config values are captured at tool construction (startup) and not picked up by hot-reload. Binary availability is re-checked per call, so runtime binary installations are detected without restart. Any extraction miss (disabled, unsupported mime, missing binary, timeout, empty output) transparently falls back to the cloud vision chain with no caller-visible difference.
|
||||
|
||||
### Custom tool definition
|
||||
```json
|
||||
{
|
||||
|
||||
@@ -0,0 +1,86 @@
|
||||
# Local-First Document Text Extraction (PDF/DOCX)
|
||||
|
||||
**Date**: 2026-05-29 17:26
|
||||
**Severity**: Medium
|
||||
**Component**: read_document tool / DocumentParser
|
||||
**Status**: Shipped
|
||||
|
||||
## What Happened
|
||||
|
||||
Delivered local-first text extraction adapter for `read_document` tool. PDF/DOCX files now optionally extract text via `pdftotext` (poppler) / `pandoc` (DOCX sandbox mode) *before* falling back to cloud vision LLM chain. Trades off extraction quality (plaintext loses layout, tables, multi-column semantics) for privacy + token cost savings on text-rich documents.
|
||||
|
||||
Opt-in by default OFF — users must explicitly set `document_parser.local_first: true` in config. This deliberate choice avoids shipping a regression: scanned PDFs + image-only documents yield empty local output, forcing cloud fallback anyway, wasting latency on a guaranteed miss.
|
||||
|
||||
## The Brutal Truth
|
||||
|
||||
This feature feels overdue and also fragile simultaneously. On one hand, it's a no-brainer privacy win for local deployments — why burn Gemini's vision context on a 100-page plaintext PDF when you can `pdftotext` it in 50ms locally? On the other, plaintext extraction fundamentally cannot preserve document structure. Users who expect formatted table output from local parsing *will* be disappointed; they need the vision chain.
|
||||
|
||||
The default-OFF toggle was the pragmatic choice: let power users opt in, don't punish document quality for the majority. But that also means the feature ships with zero real-world usage data. We'll learn on production deployments whether the latency win justifies the extraction quality gap.
|
||||
|
||||
## Technical Details
|
||||
|
||||
**New interface:** `DocumentParser` in `internal/tools/document_parser.go` — two methods:
|
||||
- `Extract(ctx, path, mime) (string, error)` — returns plaintext or sentinel error (`ErrParserUnsupported`, `ErrParserUnavailable`, `ErrParserEmpty`)
|
||||
- `Supports(mime) bool` — runtime check: config enabled + binary resolvable via LookPath
|
||||
|
||||
**LocalExtractParser implementation:**
|
||||
|
||||
1. **No-shell subprocess** (`exec.Command` not `sh -c`) — direct argument array, never string concat.
|
||||
2. **Process-group kill on timeout** — `SIGTERM` → 3s grace → `SIGKILL` via `setProcessGroup/killProcessGroup`. Pandoc forks helper processes; killing the group reaps grandchildren. NOT `exec.CommandContext` (simpler, but would only kill direct child, orphaning helpers).
|
||||
3. **Minimal subprocess env** — `buildCredentialedEnv(nil)` captures PATH/HOME/LANG/USER only. Never inherit gateway env (holds DB DSNs, API keys, encryption keys). Same posture as exec tool.
|
||||
4. **Bounded stdout** — `cappedWriter` buffers up to 500KB (`documentMaxTextBytes`), silently discards overflow, always reports full write so child never blocks on pipe. Appends truncation marker matching the existing direct-text truncation behavior.
|
||||
5. **Safe binary args:**
|
||||
- `pdftotext` omits `--` terminator (poppler's parser doesn't treat it as option-end; would consume it as positional, clobbering the real path).
|
||||
- `pandoc --sandbox` for untrusted DOCX (prevents fetch/read-arbitrary-files during conversion).
|
||||
6. **Extracted text = untrusted** — never changes trust model. Inline with existing direct-text fast-path; no new injection vector.
|
||||
|
||||
**Integration into read_document:**
|
||||
|
||||
Local-first block sits *after* existing guards (archive check, text-readable fast-path, 20MB read guard) *before* vision chain. Path validated at exec boundary (`validateExecPath` reuses `resolvePathWithAllowed` + `checkDeniedPath` so MediaRef-derived paths get same workspace confinement check).
|
||||
|
||||
Any miss (disabled / unsupported mime / missing binary / empty output / exec error / timeout) falls through silently to vision chain — never errors. Clean hit returns plaintext with **zero Provider/Model/Usage** (no LLM spend).
|
||||
|
||||
## What We Tried
|
||||
|
||||
N/A — new feature, no prior attempts.
|
||||
|
||||
## Root Cause Analysis
|
||||
|
||||
Motivation: issue #84 (token cost + privacy on local deployments with text-heavy PDFs). Vision LLM chains waste 250+ tokens per document page for purely textual content; local extraction avoids that spend + keeps data off-cloud for privacy-sensitive deployments.
|
||||
|
||||
Decision to default OFF stems from observing plaintext extraction limitations: scanned PDFs (image-only) + multi-column layouts + formatted tables all lose semantic information when extracted as flat text. Forcing this on all users regresses experience. Opt-in preserves quality for majority, enables cost savings for explicit users.
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
1. **Subprocess timeout strategy matters.** Process-group kill is cleanest for extractors that fork helpers. CommandContext would orphan grandchildren. In production, timeouts are less about hung processes (modern tools timeout internally) and more about defense-in-depth; document a timeout will never be hit in practice, but it saves from catastrophic hangs.
|
||||
|
||||
2. **Untrusted input + subprocess = attack surface.** Spent care on: minimal env (no secrets), no-shell exec (no globbing), `pandoc --sandbox` (no file I/O), bounded output (no OOM), path validation at exec boundary (no traversal). These are defensive layers; a single layer's failure shouldn't cascade. Validation + env isolation are the hardest wins.
|
||||
|
||||
3. **Opt-in vs. opt-out cuts both ways.** Default-OFF means the feature ships with zero usage data. Production will tell us if local extraction is enough for the use cases pushing for it (and if not, whether vision fallback makes it moot). Default-ON would have been faster to adoption but would have regressed document quality perception immediately. Chose sustainable over frictionless.
|
||||
|
||||
4. **Truncation markers must match.** Both direct-text and local extraction append the same `[... truncated at 500KB ...]` marker. Downstream (agent, logs) sees identical output shape. Small detail, huge payoff: no code changes needed to handle truncation; it just works.
|
||||
|
||||
5. **Testing on stubbed binaries is essential.** Tests mock `LookPath` so `pdftotext`/`pandoc` don't need to be installed to verify logic (timeout kill, capped write, path validation). Leaves open the question of real binary output quality, but that's expected to vary by tool version.
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. **Monitoring:** Instrument extraction hits vs. misses in production (`slog.Info` already in place). Monitor fallback rate to vision chain — if >90% miss, feature is unused; if <10% miss, local extraction is carrying load.
|
||||
|
||||
2. **Real-world feedback:** Opt-in means early power users will report quality issues or token savings. Collect feedback on whether plaintext extraction is good enough for their PDFs. May need advanced config (e.g., OCR pipeline for scanned docs, pandoc output format tuning).
|
||||
|
||||
3. **Binary availability docs:** Desktop Lite (`sqliteonly` build) doesn't include `pdftotext`/`pandoc`. Clarify that local-first requires full server variant (or binaries installed at runtime). Update install guides.
|
||||
|
||||
4. **Potential enhancements (deferred):**
|
||||
- OCR pipeline for scanned PDFs (tesseract) — adds latency, changes computation model
|
||||
- Fallback to LLM-only if local extraction is disabled (not yet exposed as easy toggle)
|
||||
- Per-document extraction strategy (hint: "scanned" vs. "text" → choose pipeline)
|
||||
|
||||
**Owned by:** Tools team.
|
||||
**Deadline:** Monitor week 1 post-ship for adoption metrics.
|
||||
|
||||
---
|
||||
|
||||
**Status:** SHIPPED
|
||||
**Test result:** 13 unit + integration tests pass (9 parser + 4 tool), 581 tools + 48 config tests pass, race-clean
|
||||
**Code review:** 5/5 acceptance criteria + 10/10 security constraints verified
|
||||
**Files changed:** 8 files, 686 insertions; new `document_parser.go` (222 LOC), test files, config update
|
||||
@@ -6,6 +6,56 @@ Significant changes, features, and fixes in reverse chronological order.
|
||||
|
||||
## 2026-05-29
|
||||
|
||||
### Local-first document text extraction adapter (read_document privacy optimization)
|
||||
|
||||
Adds optional local extraction pipeline to the `read_document` tool, allowing
|
||||
PDF and DOCX parsing via `pdftotext` (poppler-utils) and `pandoc` before
|
||||
falling back to cloud vision. Reduces token costs and improves privacy for
|
||||
document analysis when local binaries are available.
|
||||
|
||||
**New**
|
||||
|
||||
- `DocumentParser` interface + `LocalExtractParser` implementation in
|
||||
`internal/tools/document_parser.go`. Handles PDF (`pdftotext`) and DOCX
|
||||
(`pandoc`) with no-shell subprocess exec, binary detection, process-group
|
||||
timeout kill (SIGTERM → 3s grace → SIGKILL), minimal subprocess environment,
|
||||
and 500KB output limit.
|
||||
- `DocumentParserConfig` in `internal/config/config_channels.go` under `tools` section:
|
||||
`local_first` (opt-in, default false), `max_pages` (default 200), `timeout_sec`
|
||||
(default 30), `min_text_len` (default 16). Config is read at tool construction;
|
||||
binary availability re-checked per call for runtime installs.
|
||||
- Integration in `read_document` tool: `SetLocalParser()` wired at gateway
|
||||
startup (`cmd/gateway_managed.go`). On a clean extraction hit, returns text
|
||||
with no LLM usage (zero tokens, no Provider/Model/Usage fields). Any miss
|
||||
(disabled, unsupported mime, missing binary, timeout, empty output, exec
|
||||
error) transparently falls back to the cloud vision chain unchanged.
|
||||
- Security: subprocess exec uses `exec.Command` (no shell), docPath validated
|
||||
at the exec boundary, extracted text treated as untrusted, minimal env
|
||||
(no gateway secrets), `pandoc --sandbox` for DOCX parsing.
|
||||
|
||||
**Requirements**
|
||||
|
||||
- Requires `pdftotext` and `pandoc` on PATH for local extraction. Present in:
|
||||
- Docker `full` variant (`ghcr.io/nextlevelbuilder/goclaw:vX.Y.Z-full`)
|
||||
- Builds with `-tags "" -X main.enableFullSkills=true` (when available)
|
||||
- Desktop Lite edition: local extraction wired but not user-configurable
|
||||
(remains in code for future mobile/CLI usage; server/Docker only now).
|
||||
|
||||
**Docs**
|
||||
|
||||
- `docs/03-tools-system.md` § 5 — new `document_parser` config block with all
|
||||
four fields, defaults, and fallback semantics.
|
||||
- `docs/03-tools-system.md` § 4 (Media Reading) — updated `read_document`
|
||||
description to mention local-first extraction option.
|
||||
|
||||
**Testing**
|
||||
|
||||
- Behavior unchanged when `local_first: false` (default).
|
||||
- Verify local extraction when enabled: create test PDF/DOCX, enable config,
|
||||
inspect tool output for zero-token response vs cloud vision (Gemini, etc.).
|
||||
|
||||
---
|
||||
|
||||
### Skill selected download export (issue #80)
|
||||
|
||||
**Features**
|
||||
|
||||
@@ -425,6 +425,30 @@ type ToolsConfig struct {
|
||||
RateLimitPerHour int `json:"rate_limit_per_hour,omitempty"` // max tool executions per hour per session (0 = disabled)
|
||||
ScrubCredentials *bool `json:"scrub_credentials,omitempty"` // auto-redact API keys/tokens in tool output (default true)
|
||||
McpServers map[string]*MCPServerConfig `json:"mcp_servers,omitempty"` // external MCP server connections
|
||||
DocumentParser DocumentParserConfig `json:"document_parser"` // local-first document text extraction
|
||||
}
|
||||
|
||||
// DocumentParserConfig controls local-first document text extraction in the
|
||||
// read_document tool. Local-first is OPT-IN (disabled by default): plain
|
||||
// pdftotext/pandoc output mis-orders tables/multi-column layouts and ignores
|
||||
// the analysis prompt, so the cloud vision path remains the default. When
|
||||
// enabled, PDF/DOCX are extracted via local binaries before any cloud LLM
|
||||
// call; missing binaries or empty output fall back to the provider chain.
|
||||
//
|
||||
// Changing these values requires a gateway restart — they are captured at tool
|
||||
// construction and not picked up by config hot-reload. (Binary availability,
|
||||
// however, is re-checked per call, so runtime installs are seen without a
|
||||
// restart.)
|
||||
type DocumentParserConfig struct {
|
||||
LocalFirst *bool `json:"local_first,omitempty"` // nil => false (default OFF)
|
||||
MaxPages int `json:"max_pages,omitempty"` // 0 => 200
|
||||
TimeoutSec int `json:"timeout_sec,omitempty"` // 0 => 30s
|
||||
MinTextLen int `json:"min_text_len,omitempty"` // 0 => 16
|
||||
}
|
||||
|
||||
// LocalFirstEnabled reports whether local-first extraction is turned on.
|
||||
func (c DocumentParserConfig) LocalFirstEnabled() bool {
|
||||
return c.LocalFirst != nil && *c.LocalFirst
|
||||
}
|
||||
|
||||
// CommandKeywordAllowlistRule scopes product/security vocabulary that may appear
|
||||
|
||||
@@ -0,0 +1,222 @@
|
||||
package tools
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os/exec"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
)
|
||||
|
||||
// Sentinel outcomes. Callers fall back to the cloud vision LLM chain on any of
|
||||
// these — they distinguish "this parser cannot/should-not handle the document"
|
||||
// from a genuine extraction success.
|
||||
var (
|
||||
ErrParserUnsupported = errors.New("document_parser: unsupported mime")
|
||||
ErrParserUnavailable = errors.New("document_parser: extractor binary not installed")
|
||||
ErrParserEmpty = errors.New("document_parser: extraction produced no usable text")
|
||||
)
|
||||
|
||||
// MIME types the local extractor recognizes.
|
||||
const (
|
||||
mimePDF = "application/pdf"
|
||||
mimeDOCX = "application/vnd.openxmlformats-officedocument.wordprocessingml.document"
|
||||
)
|
||||
|
||||
// docTruncationMarker matches the marker read_document.go appends so truncated
|
||||
// local output reads identically to truncated direct-text output downstream.
|
||||
const docTruncationMarker = "\n\n[... truncated at 500KB ...]"
|
||||
|
||||
// Default config values applied when a field is left zero.
|
||||
const (
|
||||
defaultMaxPages = 200
|
||||
defaultTimeout = 30 * time.Second
|
||||
defaultMinTextLen = 16
|
||||
// killGrace is how long a process group has to exit after SIGTERM before
|
||||
// it is escalated to SIGKILL.
|
||||
killGrace = 3 * time.Second
|
||||
// stderrCap bounds captured stderr so a chatty extractor cannot block on a
|
||||
// full pipe; only used for error messages.
|
||||
stderrCap = 8 * 1024
|
||||
)
|
||||
|
||||
// DocumentParser extracts plain text from a document file. Implementations MUST
|
||||
// be safe on untrusted input and MUST NOT execute embedded content. A future
|
||||
// LiteParse backend can satisfy this same interface without touching callers.
|
||||
type DocumentParser interface {
|
||||
// Extract returns plain text, or a sentinel/wrapped error signalling the
|
||||
// caller to fall back to the cloud vision path. mime selects the tool.
|
||||
Extract(ctx context.Context, path, mime string) (string, error)
|
||||
// Supports reports whether this parser handles the mime AND its binary is
|
||||
// currently available (LookPath succeeds at call time).
|
||||
Supports(mime string) bool
|
||||
}
|
||||
|
||||
// LocalExtractConfig tunes LocalExtractParser. Zero values get sane defaults.
|
||||
type LocalExtractConfig struct {
|
||||
Enabled bool // local-first on/off (default OFF — opt-in)
|
||||
MaxPages int // 0 => 200; passed to pdftotext -l
|
||||
Timeout time.Duration // per-extraction timeout (0 => 30s)
|
||||
MinTextLen int // below this (after trim) => ErrParserEmpty (0 => 16)
|
||||
}
|
||||
|
||||
// LocalExtractParser extracts text from PDF (pdftotext) and DOCX (pandoc) via
|
||||
// no-shell subprocess exec, with binary detection, process-group timeout kill,
|
||||
// a minimal environment, and output limits.
|
||||
type LocalExtractParser struct {
|
||||
cfg LocalExtractConfig
|
||||
// lookPath is injectable so tests can stub binary discovery without a
|
||||
// PATH-prepend race. Defaults to exec.LookPath. Re-resolved per call so a
|
||||
// binary installed at runtime is picked up without a restart.
|
||||
lookPath func(string) (string, error)
|
||||
}
|
||||
|
||||
// NewLocalExtractParser builds a parser, filling zero-valued config fields with
|
||||
// defaults. Enabled is taken as given — the construction site owns the toggle.
|
||||
func NewLocalExtractParser(cfg LocalExtractConfig) *LocalExtractParser {
|
||||
if cfg.MaxPages <= 0 {
|
||||
cfg.MaxPages = defaultMaxPages
|
||||
}
|
||||
if cfg.Timeout <= 0 {
|
||||
cfg.Timeout = defaultTimeout
|
||||
}
|
||||
if cfg.MinTextLen <= 0 {
|
||||
cfg.MinTextLen = defaultMinTextLen
|
||||
}
|
||||
return &LocalExtractParser{cfg: cfg, lookPath: exec.LookPath}
|
||||
}
|
||||
|
||||
// commandFor maps a mime to its extractor binary name and arguments.
|
||||
//
|
||||
// pdftotext takes NO "--" option terminator: poppler's argument parser does not
|
||||
// treat "--" as one and would consume it as the input-file positional, pushing
|
||||
// the real path into the output-file slot (silently overwriting the source).
|
||||
// The path is the sole positional; the trailing "-" routes text to stdout.
|
||||
//
|
||||
// pandoc runs with --sandbox so an untrusted DOCX cannot make pandoc fetch
|
||||
// remote resources or read arbitrary local files during conversion.
|
||||
func (p *LocalExtractParser) commandFor(mime, path string) (bin string, args []string, err error) {
|
||||
switch mime {
|
||||
case mimePDF:
|
||||
return "pdftotext", []string{"-q", "-enc", "UTF-8", "-l", strconv.Itoa(p.cfg.MaxPages), path, "-"}, nil
|
||||
case mimeDOCX:
|
||||
return "pandoc", []string{"--sandbox", "-t", "plain", "--wrap=none", path}, nil
|
||||
default:
|
||||
return "", nil, ErrParserUnsupported
|
||||
}
|
||||
}
|
||||
|
||||
// Supports reports whether the mime is handled and its binary is resolvable now.
|
||||
func (p *LocalExtractParser) Supports(mime string) bool {
|
||||
if !p.cfg.Enabled {
|
||||
return false
|
||||
}
|
||||
bin, _, err := p.commandFor(mime, "")
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
_, err = p.lookPath(bin)
|
||||
return err == nil
|
||||
}
|
||||
|
||||
// Extract runs the extractor for mime against path and returns plain text.
|
||||
// A timeout or non-zero exit always yields an error (never partial text) so the
|
||||
// caller falls back rather than treating a half-extracted document as complete.
|
||||
func (p *LocalExtractParser) Extract(ctx context.Context, path, mime string) (string, error) {
|
||||
if !p.cfg.Enabled {
|
||||
return "", ErrParserUnsupported
|
||||
}
|
||||
bin, args, err := p.commandFor(mime, path)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
binPath, err := p.lookPath(bin)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("%w: %s", ErrParserUnavailable, bin)
|
||||
}
|
||||
|
||||
cmd := exec.Command(binPath, args...) // NOT CommandContext — see kill() below
|
||||
setProcessGroup(cmd)
|
||||
// Minimal allowlist environment (PATH/HOME/LANG/USER); never inherit the
|
||||
// gateway process env, which holds DB DSNs, API keys, and encryption keys.
|
||||
cmd.Env = buildCredentialedEnv(nil)
|
||||
|
||||
stdout := &cappedWriter{cap: documentMaxTextBytes}
|
||||
stderr := &cappedWriter{cap: stderrCap}
|
||||
cmd.Stdout = stdout
|
||||
cmd.Stderr = stderr
|
||||
|
||||
if err := cmd.Start(); err != nil {
|
||||
return "", fmt.Errorf("document_parser: start %s: %w", bin, err)
|
||||
}
|
||||
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- cmd.Wait() }()
|
||||
|
||||
timer := time.NewTimer(p.cfg.Timeout)
|
||||
defer timer.Stop()
|
||||
|
||||
select {
|
||||
case werr := <-done:
|
||||
if werr != nil {
|
||||
return "", fmt.Errorf("document_parser: %s failed: %w (stderr: %s)",
|
||||
bin, werr, strings.TrimSpace(stderr.buf.String()))
|
||||
}
|
||||
// Clean exit 0 — the only path that may return text.
|
||||
case <-ctx.Done():
|
||||
p.kill(cmd, done)
|
||||
return "", fmt.Errorf("document_parser: %s cancelled: %w", bin, ctx.Err())
|
||||
case <-timer.C:
|
||||
p.kill(cmd, done)
|
||||
return "", fmt.Errorf("document_parser: %s timed out after %s", bin, p.cfg.Timeout)
|
||||
}
|
||||
|
||||
text := stdout.buf.String()
|
||||
if len(strings.TrimSpace(text)) < p.cfg.MinTextLen {
|
||||
// Scanned/image-only PDFs yield whitespace; route to vision fallback.
|
||||
return "", ErrParserEmpty
|
||||
}
|
||||
if stdout.overflow {
|
||||
text += docTruncationMarker
|
||||
}
|
||||
return text, nil
|
||||
}
|
||||
|
||||
// kill terminates the whole process group: SIGTERM, a grace period, then
|
||||
// SIGKILL. Killing the group (not just the direct child) reaps helper children
|
||||
// that pandoc/pdftotext may fork, so a timeout cannot orphan grandchildren.
|
||||
func (p *LocalExtractParser) kill(cmd *exec.Cmd, done <-chan error) {
|
||||
_ = killProcessGroup(cmd, syscallSIGTERM)
|
||||
select {
|
||||
case <-done:
|
||||
case <-time.After(killGrace):
|
||||
_ = killProcessGroup(cmd, syscallSIGKILL)
|
||||
<-done
|
||||
}
|
||||
}
|
||||
|
||||
// cappedWriter buffers up to cap bytes and silently discards the rest, while
|
||||
// always reporting a full write. Discarding past the cap keeps the child from
|
||||
// blocking on a full pipe; overflow records that truncation occurred.
|
||||
type cappedWriter struct {
|
||||
buf bytes.Buffer
|
||||
cap int
|
||||
overflow bool
|
||||
}
|
||||
|
||||
func (w *cappedWriter) Write(p []byte) (int, error) {
|
||||
if remaining := w.cap - w.buf.Len(); remaining > 0 {
|
||||
if len(p) <= remaining {
|
||||
w.buf.Write(p)
|
||||
} else {
|
||||
w.buf.Write(p[:remaining])
|
||||
w.overflow = true
|
||||
}
|
||||
} else if len(p) > 0 {
|
||||
w.overflow = true
|
||||
}
|
||||
return len(p), nil
|
||||
}
|
||||
@@ -0,0 +1,199 @@
|
||||
package tools
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// writeStubScript writes an executable POSIX stub and returns its path.
|
||||
func writeStubScript(t *testing.T, dir, name, body string) string {
|
||||
t.Helper()
|
||||
p := filepath.Join(dir, name)
|
||||
if err := os.WriteFile(p, []byte(body), 0o755); err != nil {
|
||||
t.Fatalf("write stub %s: %v", name, err)
|
||||
}
|
||||
return p
|
||||
}
|
||||
|
||||
// stubLookPath returns a lookPath that resolves only the given binary→path map.
|
||||
func stubLookPath(m map[string]string) func(string) (string, error) {
|
||||
return func(name string) (string, error) {
|
||||
if p, ok := m[name]; ok {
|
||||
return p, nil
|
||||
}
|
||||
return "", errors.New("not found: " + name)
|
||||
}
|
||||
}
|
||||
|
||||
func newStubParser(cfg LocalExtractConfig, lp func(string) (string, error)) *LocalExtractParser {
|
||||
p := NewLocalExtractParser(cfg)
|
||||
p.lookPath = lp
|
||||
return p
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_SupportsRespectsEnabledAndBinary(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("POSIX stub scripts not runnable on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
pdftotext := writeStubScript(t, dir, "pdftotext", "#!/bin/sh\nprintf 'x'\n")
|
||||
|
||||
// Disabled => never supports, even with a resolvable binary.
|
||||
off := newStubParser(LocalExtractConfig{Enabled: false}, stubLookPath(map[string]string{"pdftotext": pdftotext}))
|
||||
if off.Supports(mimePDF) {
|
||||
t.Error("disabled parser should not support any mime")
|
||||
}
|
||||
|
||||
// Enabled + binary present => supported; binary absent => not.
|
||||
on := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{"pdftotext": pdftotext}))
|
||||
if !on.Supports(mimePDF) {
|
||||
t.Error("expected PDF supported when pdftotext resolvable")
|
||||
}
|
||||
if on.Supports(mimeDOCX) {
|
||||
t.Error("expected DOCX unsupported when pandoc absent")
|
||||
}
|
||||
if on.Supports("text/plain") {
|
||||
t.Error("expected unsupported mime to be false")
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractSuccessPDFAndDOCX(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("POSIX stub scripts not runnable on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
pdftotext := writeStubScript(t, dir, "pdftotext", "#!/bin/sh\nprintf 'pdf text content here'\n")
|
||||
pandoc := writeStubScript(t, dir, "pandoc", "#!/bin/sh\nprintf 'docx text content here'\n")
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true},
|
||||
stubLookPath(map[string]string{"pdftotext": pdftotext, "pandoc": pandoc}))
|
||||
|
||||
got, err := p.Extract(context.Background(), "/tmp/doc.pdf", mimePDF)
|
||||
if err != nil {
|
||||
t.Fatalf("pdf extract: %v", err)
|
||||
}
|
||||
if got != "pdf text content here" {
|
||||
t.Errorf("pdf text = %q", got)
|
||||
}
|
||||
|
||||
got, err = p.Extract(context.Background(), "/tmp/doc.docx", mimeDOCX)
|
||||
if err != nil {
|
||||
t.Fatalf("docx extract: %v", err)
|
||||
}
|
||||
if got != "docx text content here" {
|
||||
t.Errorf("docx text = %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractEmptyOutput(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("POSIX stub scripts not runnable on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
empty := writeStubScript(t, dir, "pdftotext", "#!/bin/sh\nprintf ' \\n \\t '\n")
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{"pdftotext": empty}))
|
||||
|
||||
_, err := p.Extract(context.Background(), "/tmp/scan.pdf", mimePDF)
|
||||
if !errors.Is(err, ErrParserEmpty) {
|
||||
t.Fatalf("expected ErrParserEmpty, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractBinaryAbsent(t *testing.T) {
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{}))
|
||||
_, err := p.Extract(context.Background(), "/tmp/doc.pdf", mimePDF)
|
||||
if !errors.Is(err, ErrParserUnavailable) {
|
||||
t.Fatalf("expected ErrParserUnavailable, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractDisabled(t *testing.T) {
|
||||
p := newStubParser(LocalExtractConfig{Enabled: false}, stubLookPath(map[string]string{}))
|
||||
_, err := p.Extract(context.Background(), "/tmp/doc.pdf", mimePDF)
|
||||
if !errors.Is(err, ErrParserUnsupported) {
|
||||
t.Fatalf("expected ErrParserUnsupported when disabled, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractUnsupportedMime(t *testing.T) {
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{}))
|
||||
_, err := p.Extract(context.Background(), "/tmp/a.txt", "text/plain")
|
||||
if !errors.Is(err, ErrParserUnsupported) {
|
||||
t.Fatalf("expected ErrParserUnsupported, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractNonZeroExit(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("POSIX stub scripts not runnable on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
failing := writeStubScript(t, dir, "pdftotext", "#!/bin/sh\necho boom >&2\nexit 1\n")
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{"pdftotext": failing}))
|
||||
|
||||
_, err := p.Extract(context.Background(), "/tmp/doc.pdf", mimePDF)
|
||||
if err == nil {
|
||||
t.Fatal("expected error on non-zero exit")
|
||||
}
|
||||
// Must not be a sentinel that hides the real failure.
|
||||
if errors.Is(err, ErrParserEmpty) {
|
||||
t.Errorf("non-zero exit should not map to ErrParserEmpty: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalExtractParser_ExtractTruncatesAtCap(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("POSIX stub scripts not runnable on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
// Emit well over the 500KB cap so the draining writer marks overflow.
|
||||
big := writeStubScript(t, dir, "pdftotext", "#!/bin/sh\nyes A | head -c 600000\n")
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true}, stubLookPath(map[string]string{"pdftotext": big}))
|
||||
|
||||
got, err := p.Extract(context.Background(), "/tmp/big.pdf", mimePDF)
|
||||
if err != nil {
|
||||
t.Fatalf("extract: %v", err)
|
||||
}
|
||||
if !strings.HasSuffix(got, docTruncationMarker) {
|
||||
t.Errorf("expected truncation marker suffix; tail=%q", got[max(0, len(got)-40):])
|
||||
}
|
||||
if len(got) > documentMaxTextBytes+len(docTruncationMarker) {
|
||||
t.Errorf("output %d exceeds cap %d + marker", len(got), documentMaxTextBytes)
|
||||
}
|
||||
}
|
||||
|
||||
// TestLocalExtractParser_TimeoutReapsProcessGroup proves a timeout kills the
|
||||
// whole process group (not just the direct child): the stub forks a grandchild
|
||||
// sleep, records its PID, and the test asserts that PID is gone afterwards.
|
||||
func TestLocalExtractParser_TimeoutReapsProcessGroup(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("process-group kill not supported on Windows")
|
||||
}
|
||||
dir := t.TempDir()
|
||||
pidFile := filepath.Join(dir, "child.pid")
|
||||
body := fmt.Sprintf("#!/bin/sh\nsleep 30 &\necho $! > %s\nwait\n", pidFile)
|
||||
slow := writeStubScript(t, dir, "pdftotext", body)
|
||||
|
||||
p := newStubParser(LocalExtractConfig{Enabled: true, Timeout: 1 * time.Second},
|
||||
stubLookPath(map[string]string{"pdftotext": slow}))
|
||||
|
||||
got, err := p.Extract(context.Background(), "/tmp/slow.pdf", mimePDF)
|
||||
if err == nil {
|
||||
t.Fatal("expected timeout error")
|
||||
}
|
||||
if got != "" {
|
||||
t.Errorf("expected no partial text on timeout, got %d bytes", len(got))
|
||||
}
|
||||
|
||||
pids := waitForRecordedPIDs(t, pidFile, 1, 2*time.Second)
|
||||
time.Sleep(200 * time.Millisecond) // let the OS reap the killed group
|
||||
if orphans := findLivePIDs(t, pids); len(orphans) > 0 {
|
||||
t.Errorf("grandchild sleep not reaped after timeout: %v", orphans)
|
||||
}
|
||||
}
|
||||
@@ -70,6 +70,7 @@ type ReadDocumentTool struct {
|
||||
registry *providers.Registry
|
||||
mediaLoader MediaPathLoader
|
||||
usageCaps *usagecaps.Service
|
||||
localParser DocumentParser
|
||||
}
|
||||
|
||||
func NewReadDocumentTool(registry *providers.Registry, mediaLoader MediaPathLoader) *ReadDocumentTool {
|
||||
@@ -80,6 +81,12 @@ func (t *ReadDocumentTool) SetUsageCapService(svc *usagecaps.Service) {
|
||||
t.usageCaps = svc
|
||||
}
|
||||
|
||||
// SetLocalParser injects the local-first document extractor. When nil (default)
|
||||
// or disabled, read_document behaves exactly as before — straight to the chain.
|
||||
func (t *ReadDocumentTool) SetLocalParser(p DocumentParser) {
|
||||
t.localParser = p
|
||||
}
|
||||
|
||||
func (t *ReadDocumentTool) Name() string { return "read_document" }
|
||||
|
||||
func (t *ReadDocumentTool) Description() string {
|
||||
@@ -146,12 +153,31 @@ func (t *ReadDocumentTool) Execute(ctx context.Context, args map[string]any) *Re
|
||||
if textReadableMIMEs[docMime] || strings.HasPrefix(docMime, "text/") {
|
||||
content := string(data)
|
||||
if len(data) > documentMaxTextBytes {
|
||||
content = content[:documentMaxTextBytes] + "\n\n[... truncated at 500KB ...]"
|
||||
content = content[:documentMaxTextBytes] + docTruncationMarker
|
||||
}
|
||||
slog.Info("read_document: returning text content directly", "mime", docMime, "size", len(data))
|
||||
return NewResult(content)
|
||||
}
|
||||
|
||||
// Local-first extraction (opt-in): for PDF/DOCX with an available local
|
||||
// binary, extract text without any cloud LLM call. Any miss — disabled,
|
||||
// unsupported mime, missing binary, empty/poor output, or exec error —
|
||||
// falls through unchanged to the vision chain below. The file was already
|
||||
// read + 20MB-checked above; the extractor opens that same path itself.
|
||||
if t.localParser != nil && t.localParser.Supports(docMime) {
|
||||
// Defense-in-depth: confirm the path is workspace-confined before
|
||||
// handing it to a subprocess. A rejection routes to vision rather than
|
||||
// erroring, preserving today's behavior for MediaRef-derived paths.
|
||||
if safePath, verr := t.validateExecPath(ctx, docPath); verr != nil {
|
||||
slog.Warn("security.read_document_local_path_rejected", "path", docPath, "reason", verr.Error())
|
||||
} else if text, err := t.localParser.Extract(ctx, safePath, docMime); err == nil {
|
||||
slog.Info("read_document: local extraction hit", "mime", docMime, "bytes", len(text))
|
||||
return NewResult(text) // no Provider/Model/Usage => no LLM spend
|
||||
} else {
|
||||
slog.Info("read_document: local extraction miss, falling back", "mime", docMime, "reason", err.Error())
|
||||
}
|
||||
}
|
||||
|
||||
chain := ResolveMediaProviderChain(ctx, "read_document", "", "",
|
||||
documentProviderPriority, documentModelDefaults, t.registry)
|
||||
|
||||
@@ -176,3 +202,20 @@ func (t *ReadDocumentTool) Execute(ctx context.Context, args map[string]any) *Re
|
||||
result.Model = chainResult.Model
|
||||
return result
|
||||
}
|
||||
|
||||
// validateExecPath confirms a resolved document path is workspace-confined
|
||||
// before it is passed to a local extractor subprocess. It reuses the same
|
||||
// allow/deny resolution as the explicit-path argument branch so MediaRef-derived
|
||||
// paths get the same boundary check at the exec boundary. Callers fall back to
|
||||
// the vision chain on error rather than surfacing it to the user.
|
||||
func (t *ReadDocumentTool) validateExecPath(ctx context.Context, path string) (string, error) {
|
||||
workspace := ToolWorkspaceFromCtx(ctx)
|
||||
resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, true), allowedWithTeamWorkspace(ctx, nil))
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if err := checkDeniedPath(resolved, workspace, nil); err != nil {
|
||||
return "", err
|
||||
}
|
||||
return resolved, nil
|
||||
}
|
||||
@@ -0,0 +1,120 @@
|
||||
package tools
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/providers"
|
||||
)
|
||||
|
||||
// emptyRegistry is a non-nil provider registry with no providers configured, so
|
||||
// the fallback chain resolves to an error result instead of panicking on a nil
|
||||
// receiver. It lets the fall-through tests reach the chain without a live LLM.
|
||||
func emptyRegistry() *providers.Registry {
|
||||
return providers.NewRegistry(func(context.Context) uuid.UUID { return uuid.Nil })
|
||||
}
|
||||
|
||||
// recordingParser is a test double for DocumentParser that records calls.
|
||||
type recordingParser struct {
|
||||
supports bool
|
||||
text string
|
||||
err error
|
||||
supportsCalled bool
|
||||
extractCalled bool
|
||||
}
|
||||
|
||||
func (f *recordingParser) Supports(string) bool {
|
||||
f.supportsCalled = true
|
||||
return f.supports
|
||||
}
|
||||
|
||||
func (f *recordingParser) Extract(context.Context, string, string) (string, error) {
|
||||
f.extractCalled = true
|
||||
return f.text, f.err
|
||||
}
|
||||
|
||||
// newPDFTool sets up a ReadDocumentTool over a workspace holding a small PDF and
|
||||
// returns the tool, the args selecting that file, and the ctx with workspace.
|
||||
func newPDFTool(t *testing.T, parser DocumentParser) (*ReadDocumentTool, map[string]any, context.Context) {
|
||||
t.Helper()
|
||||
workspace := t.TempDir()
|
||||
docPath := filepath.Join(workspace, "doc.pdf")
|
||||
if err := os.WriteFile(docPath, []byte("%PDF-1.4 raw bytes"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
tool := NewReadDocumentTool(emptyRegistry(), nil)
|
||||
tool.SetLocalParser(parser)
|
||||
ctx := WithToolWorkspace(context.Background(), workspace)
|
||||
return tool, map[string]any{"prompt": "summarize", "path": "doc.pdf"}, ctx
|
||||
}
|
||||
|
||||
func TestReadDocument_LocalHitReturnsTextWithNoSpend(t *testing.T) {
|
||||
parser := &recordingParser{supports: true, text: "EXTRACTED DOCUMENT TEXT"}
|
||||
tool, args, ctx := newPDFTool(t, parser)
|
||||
|
||||
result := tool.Execute(ctx, args)
|
||||
|
||||
if !parser.extractCalled {
|
||||
t.Fatal("expected Extract to be called on a supported mime")
|
||||
}
|
||||
if result.ForLLM != "EXTRACTED DOCUMENT TEXT" {
|
||||
t.Errorf("ForLLM = %q, want extracted text", result.ForLLM)
|
||||
}
|
||||
if result.Usage != nil {
|
||||
t.Errorf("local hit must report no Usage, got %+v", result.Usage)
|
||||
}
|
||||
if result.Provider != "" {
|
||||
t.Errorf("local hit must report empty Provider, got %q", result.Provider)
|
||||
}
|
||||
if result.IsError {
|
||||
t.Error("local hit should not be an error result")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadDocument_LocalMissFallsThroughToChain(t *testing.T) {
|
||||
parser := &recordingParser{supports: true, err: ErrParserEmpty}
|
||||
tool, args, ctx := newPDFTool(t, parser)
|
||||
|
||||
result := tool.Execute(ctx, args)
|
||||
|
||||
if !parser.extractCalled {
|
||||
t.Fatal("expected Extract to be attempted")
|
||||
}
|
||||
// With a nil registry the chain cannot resolve a provider, so falling
|
||||
// through surfaces an error result — proving we did not short-circuit.
|
||||
if !result.IsError {
|
||||
t.Errorf("expected fall-through to the (unconfigured) chain, got ForLLM=%q", result.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadDocument_DisabledNeverConsultsParser(t *testing.T) {
|
||||
// Supports=false models a disabled / unavailable parser.
|
||||
parser := &recordingParser{supports: false}
|
||||
tool, args, ctx := newPDFTool(t, parser)
|
||||
|
||||
_ = tool.Execute(ctx, args)
|
||||
|
||||
if parser.extractCalled {
|
||||
t.Error("Extract must not be called when Supports returns false")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadDocument_NilParserUnchanged(t *testing.T) {
|
||||
// No parser injected => the local-first block is skipped entirely.
|
||||
workspace := t.TempDir()
|
||||
docPath := filepath.Join(workspace, "doc.pdf")
|
||||
if err := os.WriteFile(docPath, []byte("%PDF-1.4 raw bytes"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
tool := NewReadDocumentTool(emptyRegistry(), nil)
|
||||
ctx := WithToolWorkspace(context.Background(), workspace)
|
||||
result := tool.Execute(ctx, map[string]any{"prompt": "summarize", "path": "doc.pdf"})
|
||||
// Reaches the chain (nil registry) => error result, no panic.
|
||||
if result == nil {
|
||||
t.Fatal("expected non-nil result")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user