diff --git a/.slopwatch/baseline.json b/.slopwatch/baseline.json index 67d82df56..40dd97529 100644 --- a/.slopwatch/baseline.json +++ b/.slopwatch/baseline.json @@ -1,8 +1,8 @@ { "version": 1, - "createdAt": "2026-02-21T18:55:14.1378608+00:00", - "updatedAt": "2026-03-12T23:29:28.7781033+00:00", - "description": "Initial baseline created by 'slopwatch init' on 2026-02-21 18:55:14 UTC", + "createdAt": "2026-04-11T20:25:26.1743299+00:00", + "updatedAt": "2026-04-11T20:25:26.1800921+00:00", + "description": "Baseline created on 2026-04-11 20:25:26 UTC", "entries": [ { "hash": "9d4a53dc5193e639", @@ -11,52 +11,97 @@ "lineNumber": 6, "codeSnippet": "$(NoWarn);CS1591", "message": "Adding warnings to NoWarn: CS1591", - "baselinedAt": "2026-02-21T18:55:14.1452032+00:00" + "baselinedAt": "2026-04-11T20:25:26.1799366+00:00" }, { - "hash": "8777b7954cd69fa1", + "hash": "c70b817b8444deb3", + "ruleId": "SW005", + "filePath": "src/Netclaw.Daemon/Netclaw.Daemon.csproj", + "lineNumber": 12, + "codeSnippet": "$(NoWarn);OPENAI001", + "message": "Adding warnings to NoWarn: OPENAI001", + "baselinedAt": "2026-04-11T20:25:26.1800377+00:00" + }, + { + "hash": "e5c152257aa8816d", + "ruleId": "SW005", + "filePath": "src/Netclaw.Providers/Netclaw.Providers.csproj", + "lineNumber": 8, + "codeSnippet": "$(NoWarn);OPENAI001", + "message": "Adding warnings to NoWarn: OPENAI001", + "baselinedAt": "2026-04-11T20:25:26.1800436+00:00" + }, + { + "hash": "6ea5c8bbead4b59c", "ruleId": "SW004", - "filePath": "src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs", - "lineNumber": 225, - "codeSnippet": "Task.Delay(Delay, cancellationToken)", - "message": "Test uses Task.Delay(Delay) which may indicate a timing-dependent test", - "baselinedAt": "2026-02-21T22:36:38.7489679+00:00" + "filePath": "src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs", + "lineNumber": 62, + "codeSnippet": "Task.Delay(25 * (i + 1))", + "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", + "baselinedAt": "2026-04-11T20:25:26.1800482+00:00" + }, + { + "hash": "6e43e1de0090c276", + "ruleId": "SW003", + "filePath": "src/Netclaw.Actors/Protocol/InboxWriter.cs", + "lineNumber": 114, + "codeSnippet": "catch\n {\n // best-effort cleanup; do not mask the original exception\n }", + "message": "Empty catch block swallows exceptions without handling", + "baselinedAt": "2026-04-11T20:25:26.1800601+00:00" + }, + { + "hash": "16969d3453617fc8", + "ruleId": "SW004", + "filePath": "src/Netclaw.Actors.Tests/Sessions/MemoryRecallScenarioTests.cs", + "lineNumber": 318, + "codeSnippet": "Task.Delay(25 * (i + 1))", + "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", + "baselinedAt": "2026-04-11T20:25:26.1800637+00:00" }, { "hash": "c00fb5b6beafab8b", "ruleId": "SW004", "filePath": "src/Netclaw.Actors.Tests/SubAgents/SubAgentActorTests.cs", - "lineNumber": 205, + "lineNumber": 334, + "codeSnippet": "Task.Delay(Delay, cancellationToken)", + "message": "Test uses Task.Delay(Delay) which may indicate a timing-dependent test", + "baselinedAt": "2026-04-11T20:25:26.1800701+00:00" + }, + { + "hash": "8777b7954cd69fa1", + "ruleId": "SW004", + "filePath": "src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs", + "lineNumber": 1679, "codeSnippet": "Task.Delay(Delay, cancellationToken)", "message": "Test uses Task.Delay(Delay) which may indicate a timing-dependent test", - "baselinedAt": "2026-03-02T20:00:17.1879824+00:00" + "baselinedAt": "2026-04-11T20:25:26.1800748+00:00" }, { "hash": "b691cefe260611c6", "ruleId": "SW004", "filePath": "src/Netclaw.Actors.Tests/Memory/SQLiteMemoryStoreTests.cs", - "lineNumber": 117, + "lineNumber": 263, "codeSnippet": "Task.Delay(25 * (i + 1))", "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", - "baselinedAt": "2026-03-12T23:29:28.3091789+00:00" + "baselinedAt": "2026-04-11T20:25:26.1800788+00:00" }, { "hash": "1305b3c2911b984b", "ruleId": "SW004", "filePath": "src/Netclaw.Actors.Tests/Memory/MemoryEvalSeedSuiteTests.cs", - "lineNumber": 204, + "lineNumber": 344, "codeSnippet": "Task.Delay(25 * (i + 1))", "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", - "baselinedAt": "2026-03-12T23:29:28.3181295+00:00" + "baselinedAt": "2026-04-11T20:25:26.1800844+00:00" }, { - "hash": "6ea5c8bbead4b59c", - "ruleId": "SW004", - "filePath": "src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs", - "lineNumber": 61, - "codeSnippet": "Task.Delay(25 * (i + 1))", - "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", - "baselinedAt": "2026-03-12T23:29:28.7781027+00:00" + "hash": "5c3b00ebd2426d97", + "ruleId": "SW003", + "filePath": "src/Netclaw.Actors.Tests/Protocol/InboxWriterTests.cs", + "lineNumber": 26, + "codeSnippet": "catch\n {\n // best-effort cleanup\n }", + "message": "Empty catch block swallows exceptions without handling", + "baselinedAt": "2026-04-11T20:25:26.1800921+00:00" } ] } \ No newline at end of file diff --git a/openspec/changes/channel-ingress-attachments/.openspec.yaml b/openspec/changes/channel-ingress-attachments/.openspec.yaml new file mode 100644 index 000000000..11393eacc --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-04-11 diff --git a/openspec/changes/channel-ingress-attachments/design.md b/openspec/changes/channel-ingress-attachments/design.md new file mode 100644 index 000000000..9e1330520 --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/design.md @@ -0,0 +1,491 @@ +## Context + +This change makes attachment ingress a first-class cross-channel concern. +The current design has three architectural problems layered on top of +each other: + +1. **Silent drops at the wrong layer.** + `SlackThreadBindingActor.MapSlackFiles`/`SlackThreadBindingActor.cs:218` + hard-codes an `image/*` allowlist and `DEBUG`-drops everything else. + `LlmSessionActor.cs:1705-1719` then silently strips images when the + model isn't vision-capable, surfacing only an `[Images removed]` + placeholder. Both layers make the same category of decision (what + the model can render) but neither layer owns it, and neither layer + tells the user anything actionable. + +2. **Scanner / audience de-coupling.** + `IContentScanner.ScanAsync` at `SlackThreadBindingActor.cs:234` is + audience-blind. It asks "is this byte stream safe?" but not "is + this class of file allowed to reach the agent from this trust + level?". Those are different questions and the second one has no + home in the current code. + +3. **No cross-channel contract.** + `netclaw-input-adapters` defines `SendUserMessage`, entity key + routing, and source metadata — but says nothing about file + attachments. Discord will arrive next, and the shortest path for + its implementer is to copy the Slack code's patterns — including + the silent drops. + +The design below moves the capability-routing decision up to ingress, +introduces an audience-gated attachment policy on the existing +`ToolAudienceProfile`, and defines a uniform pipeline + persisted +text-block format (`[attachment] ... inlined="..." note="..."`) that +every channel adapter must implement. Slack becomes the first +implementation. `LlmSessionActor` stops owning modality decisions. + +## Goals / Non-Goals + +**Goals:** + +- A single pipeline every channel runs for inbound attachments: + policy gate → size/count gate → download → scan → inbox write → + `[attachment]` text injection → capability-gated `DataContent` + inlining. +- The agent is *told* (in the inbound text) whether each file was + inlined and — if not — why, so its reply can acknowledge the + attachment instead of going silent. +- Audience-trust policy governs which MIME categories are accepted + per `TrustAudience`, reusing the existing `ToolAudienceProfile` + surface rather than inventing a parallel knob. +- `LlmSessionActor` becomes a strict consumer: if it sees a modality + the current model can't render, that is a **bug in the ingress + adapter**, not a condition to be handled gracefully. +- Zero silent drops. Every rejection is user-visible; every + accepted-but-not-inlined file is announced to the agent with an + explicit `inlined="false"` + `note=`. +- Cross-channel portability: `netclaw-input-adapters` carries the + normative language so Discord/Teams/web implementations follow + the same contract. + +**Non-Goals:** + +- Server-side extraction, OCR, or conversion. Agents use + `file_read` / `shell_execute` on `inbox/*` on demand. +- New `AIContent` subtypes for attachments. A plain `TextContent` + line works across every provider with zero adapter surgery. +- Per-user or per-sender attachment allowlists. Audience is the + axis for MVP; senders can be layered on later via ACL. +- Outbound file uploads. `attach_file` already covers that path and + is untouched. +- Runtime category definition / operator-defined MIME categories. + The category set is fixed in code; `AllowedCategories` picks from + a closed enum. + +## Decisions + +### D1. Capability routing happens at ingress, not in `LlmSessionActor` + +**Decision:** `SlackThreadBindingActor` (and every future channel's +equivalent) queries `ModelCapabilityActor` for the active model's +`InputModalities` *before* building `ChannelInput.Contents`, and uses +the answer to decide inline-vs-path-only per file. `LlmSessionActor` +stops stripping and starts asserting. + +**Rationale:** + +- The channel knows the session, which determines the model, which + determines the capability. That information is available at ingress + — pushing the decision deeper into `LlmSessionActor` forces the + strip to happen after the inbound turn is already constructed, + which means the agent's turn history gets rewritten after the fact. + Rewriting persisted state is where `[Images removed]` placeholders + came from. +- A strict consumer model (ingress guarantees valid modalities; + session asserts) prevents the *next* bug in the same class. If + another channel skips the capability gate by accident, the assert + fires loudly in dev and in logs, rather than silently stripping. +- The `ModelCapabilityActor` already caches per-model data in memory + (per exploration: ~1–10 ms on cache hits, ~10 s worst case on + cold cache with dedupe via `_pending` waitlist). Ingress-side + queries are effectively free on the hot path. + +**Alternatives considered:** + +- *Keep the strip in `LlmSessionActor`, just make it loud instead of + silent.* Rejected. Moves the log-level knob but leaves the + architectural smell — `LlmSessionActor` shouldn't be rewriting the + turn it's about to process. It also can't generate a replacement + `[attachment]` announcement for the turn without reaching back + into channel-layer concerns. +- *Channel emits all raw attachments as `DataContent`, a middleware + stage between channel and session does the capability gate.* + Rejected. That's one more actor hop for no architectural gain — + the channel is already the right place, and adding a dedicated + "attachment router" actor is over-engineering for the two known + channels (Slack now, Discord soon) that would pass through it. + +### D2. `TextContent` announcement, not a bespoke `AIContent` subtype + +**Decision:** The `[attachment]` line is plain `TextContent` with a +fixed format — `[attachment] name="..." mime="..." size=... path="inbox/..." inlined="true|false" [note="..."]`. + +**Rationale:** + +- Works across every LLM provider adapter without changes — no + `AnthropicProviderPlugin` or `OpenAiProviderPlugin` surgery. +- Stable over future provider/model evolutions; `TextContent` is the + one shape that will never change. +- Persisted naturally in `ChannelInput.Contents` → journal → replay, + without per-type marshalling. +- Easy for eval harnesses and tests to regex-match and assert on. +- Small token cost per attachment (~30–40 tokens for the announcement + line) — acceptable given the single-turn, once-per-upload scope. + +**Alternatives considered:** + +- *New `AttachmentContent : AIContent` subtype with typed fields.* + Rejected. Adapter work in every provider, no observable gain — the + model reads the same information either way. +- *Structured JSON inside a `TextContent`.* Rejected as overkill; + the key=value format is already trivially parseable and less + visually noisy for the model. + +### D3. MIME categories, not raw MIME prefix allowlists + +**Decision:** `ToolAudienceProfile.ChannelAttachments.AllowedCategories` +is a `HashSet` over a closed enum: + +```csharp +public enum AttachmentCategory +{ + Image, // image/* + Pdf, // application/pdf + Document, // word, excel, powerpoint, odf, rtf, text/* + Archive, // zip, tar, gz, 7z, rar + Media, // video/*, audio/* + Other // application/octet-stream, unknown MIME +} +``` + +A single internal function `MimeToCategory(string mime) → AttachmentCategory` +is the only place the mapping lives. Unknown / unrecognized MIME types +map to `Other`, which is only allowed in the `Personal` audience by +default — fail-closed. + +**Rationale:** + +- Operators reason about policy in human terms ("allow images") not + MIME strings ("`image/png`, `image/jpeg`, `image/webp`, …"). The + category vocabulary matches how people actually think about file + trust. +- Centralized map is the only surface that needs updating when new + MIME types emerge. +- Unknown → `Other` → fail-closed preserves the security posture; + adversaries can't smuggle novel MIME types into `Public`-trust + contexts by picking ones not in a hand-maintained allowlist. + +**Alternatives considered:** + +- *Config-defined category map.* Rejected as a configuration + ergonomics trap — operators would end up copy-pasting MIME lists + from blog posts and the security posture would drift per + deployment. Closed enum in code is a better default; if a + deployment truly needs a novel category it can raise a PR. +- *No categories, allow-list by MIME prefix.* Rejected — same + operator ergonomics problem inverted. + +### D4. Default policy matrix is conservative at `Public`, permissive at `Personal` + +**Decision:** Default `AllowedCategories` per audience: + +| Audience | AllowedCategories | Size cap | File count | +|---|---|---|---| +| `Public` | `{ Image }` | 25 MiB | 10 | +| `Team` | `{ Image, Pdf, Document, Archive, Media }` | 25 MiB | 10 | +| `Personal` | `{ Image, Pdf, Document, Archive, Media, Other }` | 25 MiB | 10 | + +**Rationale:** + +- `Public` defaults to images-only because processing PDFs/docs/ + archives typically routes through `shell_execute` on user- + controlled bytes. In a public Slack channel, any workspace member + can upload — the attack surface is real and doesn't need to be + open by default. +- `Team` and `Personal` get documents and archives because in those + contexts the workspace auth boundary is a meaningful filter, and + legitimate flows ("please summarize this PDF") are common. +- Only `Personal` gets `Other` (unknown MIME) because the failure + mode is always "agent shells out on unknown bytes" and that is + specifically the fail-closed posture `Public` and `Team` want to + preserve. +- 25 MiB aligns with Anthropic's current PDF document block limit + and is generous for typical work documents. +- 10 files per message is a belt-and-suspenders cap against + mass-upload abuse (user drags a folder into the message box). + +**Operators can override.** Every cell is a config field on the +existing `ToolAudienceProfile` surface. A security-sensitive +deployment can set `Public.AllowedCategories = []`; a low-risk one +can set `Public.AllowedCategories = { Image, Pdf }`. + +### D5. Ingress pipeline order is policy → size → count → download → scan → capability → write → announce + +**Decision:** The eleven-step pipeline, in order: + +1. Parse Slack file metadata from the inbound event (no change). +2. Resolve `TrustAudience` for the inbound message via the existing + `SlackAclPolicy.ResolveAudience` path (no change). +3. **Per-file audience/category gate** — reject immediately if the + file's MIME category is not in `AllowedCategories` for this + audience. *Before* download — no bytes consumed, no bandwidth + burned on files that can't be accepted. +4. **Per-file size gate** — reject immediately if Slack's reported + `size` exceeds `MaxFileBytes`. *Before* download. +5. **Per-message file-count gate** — reject the entire message if + `files.Count > MaxFilesPerMessage`. *Before* download. +6. **Download bytes** — via existing Slack HTTP client with + `url_private_download` and bot Bearer auth. +7. **Content scan** — existing `IContentScanner.ScanAsync` on the + downloaded bytes. Scan-reject replies user-visibly. +8. **Capability query** — `Ask` on + `ModelCapabilityActor` with a 2-second cancellation. Cache hit is + ~1 ms. +9. **Inbox write** — atomic write to `{SessionDirectory}/inbox/` + with filesystem-level collision suffixing. +10. **Build announcement** — `[attachment]` TextContent with + `inlined` and (conditionally) `note`. +11. **Build inline** — `DataContent(bytes, mime)` if capability + gate said yes. + +**Rationale for ordering:** the three pre-download gates (audience, +size, count) use metadata already present on the Slack event, so +they can short-circuit before burning any bandwidth. Scanner runs +on bytes so it has to wait until after download. Capability query +is independent and could run in parallel, but cache-hit latency is +so small it's not worth the orchestration complexity. + +### D6. Inbox collisions are checked against the filesystem, not the current batch + +**Decision:** Before writing `inbox/{name}`, check `File.Exists`. If +present, try `{stem}_1{ext}`, `{stem}_2{ext}`, … up to 99. If 99 +collisions, reject that file with a user-visible reply. Reuse +`FilenameSanitizer.Sanitize` for the `{name}` sanitization and +`AttachFileTool`'s path-traversal-safe resolution pattern. + +**Rationale:** If turn 3 uploads `report.pdf` and turn 7 uploads a +different file also called `report.pdf`, a per-batch collision +check would silently overwrite turn 3's file — which the agent's +history still references. Filesystem-level checks make persistence +across turns consistent with persistence across files. + +### D7. `LlmSessionActor` silent strip becomes a loud assertion + +**Decision:** Replace `LlmSessionActor.cs:1705-1719` with: + +```csharp +if (mediaRefs.Any(r => !_model.InputModalities.HasFlag(r.Modality.ToModelModality()))) +{ + var offending = mediaRefs + .Where(r => !_model.InputModalities.HasFlag(r.Modality.ToModelModality())) + .Select(r => $"{r.Name}:{r.Modality}") + .ToArray(); + _log.Error( + "Ingress bug: session received DataContent modality the active model cannot render. " + + "Model={ModelId} Modalities={Modalities} Offending={Offending}. " + + "This indicates the originating channel did not query ModelCapabilityActor " + + "before inlining DataContent. Dropping the unsupported refs and continuing the turn.", + _model.Id, _model.InputModalities, string.Join(",", offending)); + // Drop the refs so the provider call doesn't fail, but surface the incident to the user: + // one TextContent line appended to the inbound contents so the agent can tell the user + // "I received a file but a system glitch prevented me from viewing it." + contents = contents.Where(c => !(c is DataContent d && IsOffending(d))).ToList(); + contents.Add(new TextContent( + "[system] an attachment was received but could not be delivered to the model " + + "due to an ingress bug; please retry or notify the operator")); +} +``` + +Key differences from today: + +- `_log.Error` not `_log.Debug`. +- No placeholder `[Images removed — model has no vision]` text. That + was a symptom of a wrong-layer fix. The new message explicitly + says "ingress bug" so an operator reading logs or the user reading + the turn reply knows this is abnormal, not expected behavior. +- Still non-fatal — session completes the turn. The goal is visibility, + not crashing the conversation. + +**Rationale:** The target state is "this branch never fires in +practice because ingress always routes correctly". Making it loud +ensures that if a new channel ships without capability-gating, its +first attachment upload produces a log line that's obviously wrong. +Silent stripping would hide that signal for weeks. + +**Alternative considered:** Delete the branch entirely and let the +provider reject the call. Rejected because provider errors are +noisier to diagnose and the turn fails entirely rather than +degrading; the explicit strip+annotate lets us fail soft while +logging loudly. + +### D8. Dynamic-context hint is conditional, short, and names the canonical note classes + +**Decision:** `LlmSessionActor.InjectDynamicContextLayers` conditionally +appends this block to the system prompt when the session's audience +profile has `file_read` granted: + +``` +Your session working directory contains an `inbox/` subdirectory +where user-uploaded attachments are placed. Each attachment is +announced in the inbound message as a single line: + + [attachment] name="..." mime="..." size=... path="inbox/..." inlined="true|false" [note="..."] + +When `inlined="true"`, you can see the file content natively in this +turn. When `inlined="false"`: + - If `note` begins with "current model has no": the file exists on + disk but you cannot render it. Acknowledge it to the user by name + in your reply, explain the limitation, and offer tool-based + workarounds if applicable (e.g., `shell_execute pdftotext` for a + PDF on a non-PDF model). + - If `note` begins with "format not inlineable": use `file_read` or + `shell_execute` to process the bytes. This is the normal path for + docx, zip, archive, and media files. + +Never silently ignore an attachment the user sent you — always +acknowledge what you received, even if you cannot fully process it. +``` + +**Rationale:** + +- Conditional on `file_read` grant: without the tool, telling the + agent about `inbox/` is a lie (it can't read from it). Check the + audience profile's `AllowedTools` list at context-build time. +- Names the two canonical note-prefix patterns explicitly so the + agent can branch without having to fuzzy-match. +- Ends with an imperative acknowledgment rule so the model doesn't + "helpfully" skip mentioning an unviewable file. +- ~180 tokens total; acceptable overhead on a system prompt that is + already in the several-thousand-token range. + +## Risks / Trade-offs + +- **[Risk] Disk exhaustion in `/tmp` if the operator deployed against + the legacy `SessionDirectoryHelper.GetSessionDirectory(sessionId)` + overload (single-arg, uses `Path.GetTempPath()`)** → + Mitigation: mark that overload `[Obsolete]` with a migration + message pointing at the durable `NetclawPaths.SessionsDirectory` + path. Audit all call sites and migrate them. Add a + `ConfigSchemaDoctorCheck` warning on startup if the session dir + resolves under `Path.GetTempPath()`. + +- **[Risk] `ModelCapabilityActor` returns stale capabilities after a + mid-session model swap** → Mitigation: out of scope for this change; + document the known limitation in the spec. Mid-session model swaps + are rare; cache-invalidation on swap is a separate piece of work. + If it bites someone in practice, the silent behavior is "the + attachment routes with pre-swap capabilities for one message" — + not a silent drop, just a transient mis-route. + +- **[Risk] The agent ignores the `note` field and replies + generically** → Mitigation: the eval suite gets a regression case + for the model-modality-gap path specifically asserting the agent + mentions the attachment name in its reply. If that eval fails, the + dynamic-context hint gets sharpened. + +- **[Risk] Unknown MIME types get silently categorized as `Other` and + only allowed in `Personal`** → Mitigation: emit an `INFO` log on + unknown-MIME classification so operators can notice and extend + `MimeToCategory` if there's a legitimate pattern. Fail-closed is + still the right default. + +- **[Risk] Inbox files survive session cleanup** → Mitigation: confirm + session-dir lifetime management during implementation; if there + isn't a cleanup hook today, this change adds one that removes + `{sessiondir}/inbox/*` on session expiry (same policy as existing + `media/` files). Tracked as an implementation task, not a separate + change. + +- **[Risk] `LlmSessionActor` loud-log path fires in production + because a new channel is shipped without capability-gating** → + That is exactly the design intent. Surfacing that bug immediately, + loudly, is better than silently stripping. + +- **[Trade-off] `[attachment]` announcement costs ~30–40 tokens per + file** → Accepted. The alternative (rich `AIContent` subtype) costs + adapter complexity instead, which is worse. For the typical + 1-file-per-message case this is a rounding error on a multi-thousand- + token turn. + +- **[Trade-off] Pre-download gates use Slack-reported size, which + the uploader can technically forge** → Accepted. A forged small + size still hits the post-download scan and the runtime byte-length + check. The pre-download gate is a bandwidth optimization, not a + security boundary — the scanner remains authoritative. + +## Actor Boundaries & Persistence + +- **`SlackConversationActor` → `SlackThreadBindingActor`** — unchanged + envelope, but `SlackThreadInbound` now carries the resolved + `TrustAudience` alongside the file list (it already does, per + exploration). +- **`SlackThreadBindingActor` → `ModelCapabilityActor`** — new `Ask` + dependency. Resolved via the existing `ActorRegistry` pattern + (`ActorRegistryKeys.ModelCapabilityActorKey`). 2-second timeout; + timeout → user-visible "can't process attachment right now" reply, + no inlining, no guessing. +- **`SlackThreadBindingActor` → `LlmSessionActor`** — payload shape + unchanged (`ChannelInput` with `Contents`). The contents list now + reliably contains correct-modality `DataContent` items only. +- **Persistence:** `ChannelInput` is journaled as part of + `LlmSessionActor.PersistenceId = "session-{entityId}"`. The + `[attachment]` `TextContent` IS persisted as part of the turn + history — a session replay on restart will see the line exactly as + the live turn did. This matches current `DataContent` persistence + for images. +- **Inbox files on disk** live at + `{NetclawPaths.SessionsDirectory}/{sanitized-sessionId}/inbox/`. + Under the default production path (`~/.netclaw/sessions/`) this + survives restart. Under the legacy `/tmp` path it does not — the + mitigation in Risks above deprecates the legacy path. + +## Failure Modes & Recovery + +| Failure | Recovery | +|---|---| +| Pre-download policy reject (category / size / count) | User-visible reply explaining the specific policy; no disk write; no LLM turn. | +| HTTP download failure | User-visible reply ("couldn't download your attachment — please retry"); `WARN` log with Slack file ID; continue processing other files in the same message. | +| Content scanner internal error | Same as today — allow the file through with an `ERROR` log (scanner-error fallback is reviewed prior art). | +| Content scanner rejects (malware, magic-byte mismatch) | User-visible reply with the scanner's rejection reason; no disk write; no LLM turn for that file. | +| `ModelCapabilityActor` timeout | User-visible reply ("having trouble processing your attachment right now"); `WARN` log; no inlining, no fallback guess. Per "no silent fallbacks". | +| Inbox write failure (disk full, permissions) | User-visible reply ("couldn't save your attachment"); `ERROR` log with path; no LLM turn for that file. | +| Filename collision exhaustion (99 `_N` suffixes) | User-visible reply ("too many files with that name in this session"); `WARN` log; no LLM turn for that file. | +| Audience resolution returns `Public` due to unrecognized channel | Treat as `Public` — the most restrictive default. Fail-closed; no inlining of sensitive categories. | +| `LlmSessionActor` receives an unsupported-modality `DataContent` | `ERROR` log with model/modality/offending refs (D7); drop the refs; append a `[system]` TextContent noting the ingress bug; complete the turn. | + +## Migration Plan + +1. **Phase 0 (this change)**: ship the pipeline, the new + `ToolAudienceProfile.ChannelAttachments` config surface with + defaults, the `LlmSessionActor` assertion, the dynamic-context + hint, and the Slack channel implementation. Update + `netclaw-config.v1.schema.json` with `"default"` values for each + new field so `netclaw doctor --fix` auto-migrates stale configs. +2. **Deprecate the legacy `SessionDirectoryHelper` single-arg + overload** (`[Obsolete("Use GetSessionDirectory(sessionId, basePath) with NetclawPaths.SessionsDirectory")]`). + Migrate all production call sites in the same PR. Leave for tests. +3. **Eval suite**: add the three regression cases from the proposal + (inlined happy path, model-modality gap, format-not-inlineable). +4. **Rollback**: this is not a hot-patch-worthy surface; rollback is + via revert of the PR. The assertion in `LlmSessionActor` means a + partial rollback (keep new ingress, revert `LlmSessionActor`) + would cause the loud-log path to fire — acceptable as a transient, + but clean rollback is a full revert. + +## Open Questions + +- **Session-directory cleanup hook**: does one exist today for + `media/`? If yes, inbox/ reuses it. If no, this change adds one — + will resolve during implementation rather than pre-deciding here. +- **Per-workspace override of `ToolAudienceProfile` defaults**: + Slack-specific. Not in scope for this change — deferred to a + follow-up if the three-audience default matrix proves too coarse + in practice. +- **Image on non-vision model — should the file even be saved to + disk?** Current decision: yes, save to `inbox/` so the agent can + at least `shell_execute file` to probe basic metadata and + acknowledge the file by name. Alternative would be path-only text + announcement without a disk write. Keeping the disk write for + uniformity — all accepted files land in `inbox/` regardless of + inlining status. Will reconsider if the disk-write path turns + out to be onerous. diff --git a/openspec/changes/channel-ingress-attachments/proposal.md b/openspec/changes/channel-ingress-attachments/proposal.md new file mode 100644 index 000000000..30be1339e --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/proposal.md @@ -0,0 +1,272 @@ +## Why + +Netclaw has no canonical contract for how channels deliver user-uploaded +file attachments to an agent session. Today the only channel that exists +(Slack Socket Mode) silently drops every non-image attachment at +`SlackThreadBindingActor.cs:218`, and `LlmSessionActor.cs:1705-1719` +additionally strips images from any session whose model lacks vision — the +user gets back a terse `[Images removed ...]` placeholder with no path +forward. A real user hit this in session `D0AC6CKBK5K/1775921191.341069` +by DMing a PDF; the agent reported it saw nothing. Both failures are the +same class of bug: silent drops buried in the wrong layer, with no +cross-channel contract to prevent the next channel (Discord, Microsoft +Teams, web widget) from reinventing them. This change establishes that +contract once, uses it to fix Slack, and leaves Discord and future channels +a paved path to implement the same behavior. It also resolves a latent +security gap: there is currently no audience-trust gate on inbound files, +so a `file_share` event from a public channel would be treated the same +as one from a DM. This change fixes that using the existing `TrustAudience` +taxonomy — images are safe at every trust level, but PDFs, documents, and +archives are rejected in `Public` channels by default because processing +them typically routes through `shell_execute` on user-controlled bytes. +Relevant PRDs: **PRD-009 (Input Adapters and Unified Input)** and +**PRD-002 (Gateway Security Envelope)** — both are silent on file +attachments today and gain a new section as part of this change. + +## What Changes + +1. **Canonical cross-channel attachment ingress contract.** Every input + adapter (Slack today, Discord tomorrow, any future channel) SHALL + produce a uniform pipeline for user-uploaded files: + - Download via the adapter's transport-specific mechanism. + - Content-scan the raw bytes through `IContentScanner` **before** any + disk write or LLM exposure (unchanged requirement). + - Apply per-audience attachment policy — reject with a loud, + user-visible reply if the MIME category is not allowed for the + resolved `TrustAudience` of the inbound message. + - Enforce a per-file size cap and a per-message file-count cap; + reject with user-visible replies on overflow. + - Write accepted files atomically to + `{SessionDirectory}/inbox/{safeFilename}` using the durable base + path from `NetclawPaths.SessionsDirectory` — **never** the legacy + `Path.GetTempPath()` overload of `SessionDirectoryHelper`. + - Filesystem-level collision suffixing (`foo.pdf`, `foo_1.pdf`, + `foo_2.pdf`, ...) across turns, not just within a single inbound + message batch, so a turn-3 upload is not silently overwritten by a + turn-7 upload with the same name. + - Inject a `TextContent` block into `ChannelInput.Contents` of the + form: + ``` + [attachment] name="" mime="" size= path="inbox/" inlined="true|false" [note=""] + ``` + One line per accepted file, batched into a single `TextContent` + when a message carries multiple files. The `inlined` field is + machine-parseable and mandatory; the `note` field is present only + when `inlined="false"` and carries a short, agent-facing + explanation so the agent can reply to the user without having to + reverse-engineer the gap between what it received and what it can + natively "see". + - Additionally inline as `DataContent` only when the session's active + model reports the relevant `ModelModality` via + `ModelCapabilityActor` — `image/*` when `ModelModality.Image` is + set, `application/pdf` when the model natively accepts PDFs. + Non-inlineable files are path-only; the agent uses its existing + `file_read` / `shell_execute` / `file_edit` tools to process them + on demand. + - **Inlined vs. path-only is an explicit signal to the agent, not an + inference.** The `inlined` field on each `[attachment]` line tells + the agent whether the file is viewable natively in this turn. When + it is not, the `note` field distinguishes *why*, using one of two + canonical classes so the agent can respond intelligently: + - **Model-modality gap** — the file is a class the model *could* + handle on a different deployment (image on a text-only model, + PDF on a model without native document support). The note says + so, and the agent is expected to tell the user it can see the + file exists but cannot view its contents on the current model, + and can offer tool-based workarounds where they apply (e.g., + `shell_execute pdftotext` for a PDF on a non-PDF-capable model). + - **Format-not-inlineable** — the file has no native model + representation regardless of deployment (docx, zip, video, + archive, octet-stream). The note says so, and the agent is + expected to use `file_read` / `shell_execute` to process the + bytes on demand, exactly as it would with any other file in the + session working directory. + The exact `note` strings are normative and defined in the spec so + the agent's behavior does not drift with ad-hoc phrasing. Example + lines: + ``` + [attachment] name="report.pdf" mime="application/pdf" size=284512 path="inbox/report.pdf" inlined="true" + [attachment] name="diagram.png" mime="image/png" size=12345 path="inbox/diagram.png" inlined="false" note="current model has no image modality; file is on disk but not viewable this turn" + [attachment] name="notes.docx" mime="application/vnd.openxmlformats-officedocument.wordprocessingml.document" size=51234 path="inbox/notes.docx" inlined="false" note="format not inlineable; use file_read or shell_execute to process" + ``` + +2. **Per-audience attachment policy** on `ToolAudienceProfile`. + `ToolAudienceProfile` gains a `ChannelAttachments` record carrying an + `AllowedCategories` set (`Image`, `Pdf`, `Document`, `Archive`, + `Media`, `Other`), a `MaxFileBytes` cap, and a `MaxFilesPerMessage` + cap. Defaults: + + | Audience | AllowedCategories | + |---|---| + | `Public` | `{ Image }` | + | `Team` | `{ Image, Pdf, Document, Archive, Media }` | + | `Personal` | `{ Image, Pdf, Document, Archive, Media, Other }` | + + Default size cap: 25 MiB. Default file-count cap: 10. Operators can + widen or narrow any cell via config. + +3. **BREAKING (internal): remove the silent image strip from + `LlmSessionActor`.** The capability-gated routing decision moves to + ingress. `LlmSessionActor` replaces lines 1705-1719 with a loud + log+assertion: if an unsupported modality reaches the session actor, + that is a bug in the ingress adapter, not something to paper over. + This tightens the contract: ingress is authoritative for modality + routing. + +4. **Session working directory hint in dynamic context.** When a session + has `file_read` granted, `LlmSessionActor.InjectDynamicContextLayers` + appends a short block to the system prompt that (a) tells the agent + uploaded attachments appear under `inbox/` and are announced via + `[attachment] ... path="inbox/..." inlined="true|false"`, (b) defines + how to interpret `inlined="false"` with each of the two canonical + `note` classes, and (c) directs the agent to acknowledge to the user, + in its turn reply, any attachment it received but could not view + natively — never silently. Without this, the agent neither knows to + call `file_read` on paths it sees in inbound text nor recognizes + when it should tell the user "I see the file but can't view its + contents directly." + +5. **Eval cases: attachment round-trips.** Per CLAUDE.md's Eval Suite + rule, add regression cases to `evals/` covering the three behaviors + this change makes normative: + - **Inlined happy path**: user uploads a PDF in a `Team`-trust + channel on a PDF-capable model; agent answers a question about its + contents. Exercises ingress + capability gate + `DataContent` + inlining + system-prompt hint in one pass. + - **Model-modality gap**: user uploads an image on a text-only + model; agent acknowledges the attachment by name and explains it + cannot view the image on the current model. Exercises the + `inlined="false" note="current model has no image modality..."` + path and the system-prompt hint telling the agent to surface this + to the user. + - **Format-not-inlineable**: user uploads a `.docx` in a `Team` + channel; agent uses `shell_execute` to extract text and answers. + Exercises the path-only `[attachment]` announcement and the + agent's use of file tools on an uninlinable format. + +## Capabilities + +### New Capabilities + +_None._ The cross-channel attachment contract is most naturally a +requirement of the existing `netclaw-input-adapters` capability rather +than a standalone spec — that's where "what every channel must do" lives. + +### Modified Capabilities + +- `netclaw-input-adapters`: adds normative requirements for the + cross-channel attachment ingress contract (download, scan, audience + policy, size caps, inbox write, `[attachment]` text injection, + capability-gated `DataContent` inlining, loud rejection replies). + This is the canonical surface Discord and future channels will + implement. +- `netclaw-slack-socket`: Slack-specific delta — replace the + `image/`-only allowlist in `SlackThreadBindingActor`, expand the + existing image-download path to cover all audience-allowed MIME + categories, and update the thread-history backfill path + (`#### Scenario: Historical images included as DataContent`, + spec.md:170) so historical attachments of any permitted category are + replayed, not just images. +- `tool-approval-gates`: `ToolAudienceProfile` gains the + `ChannelAttachments` policy surface (category set, size cap, + file-count cap) as a new per-audience configuration field. Default + values per `Public` / `Team` / `Personal` are normative. This spec is + the existing home of `ToolAudienceProfile`-shaped per-audience config + (via "Tool approval configuration per audience"), so attachment + policy extends that surface rather than introducing a parallel one. +- `netclaw-session`: the session layer stops silently stripping + `DataContent` on capability mismatch and instead asserts that ingress + delivered valid modalities. Adds a new requirement for strict + modality consumption and a second new requirement for the + attachment-aware dynamic-context hint injected into the system + prompt when `file_read` is granted. + +## Impact + +### Code + +- `src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs` — rewrite the + attachment loop (lines 213-267) around the new contract. +- `src/Netclaw.Channels.Slack/SlackChannelOptions.cs` — plumb + configurable size/count caps if the operator wants to override the + `ToolAudienceProfile` defaults on a per-workspace basis. +- `src/Netclaw.Configuration/ToolAudienceProfiles.cs` — add the + `ChannelAttachments` record and per-audience defaults. +- `src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json` — + schema-sync for the new policy fields (CLAUDE.md Configuration Schema + Sync Rule). Include `"default"` values so `netclaw doctor --fix` can + auto-migrate existing configs. +- `src/Netclaw.Actors/Sessions/LlmSessionActor.cs` — remove silent + strip (lines 1705-1719), add dynamic-context hint in + `InjectDynamicContextLayers` (~line 2254). +- `src/Netclaw.Actors/Protocol/SessionDirectoryHelper.cs` — audit + callers; every production callsite must pass the + `NetclawPaths.SessionsDirectory` base path, not the legacy + `Path.GetTempPath()` overload. The legacy overload stays for tests + but gets `[Obsolete]` with a migration message pointing at the + durable overload. +- `src/Netclaw.Security/FilenameSanitizer.cs` — reuse as-is (verified + solid: strips `..`, nulls, control chars, platform-problematic + characters). No changes required. +- `evals/` — new PDF round-trip regression case. + +### APIs and contracts + +- **Internal BREAKING**: `LlmSessionActor` stops silently stripping + unsupported modalities. Any channel adapter that currently relies on + this safety net will break; the expectation is that ingress does the + right thing. Slack is the only current adapter and this change updates + it in the same PR, so external impact is zero. +- **Config schema**: additive new fields on `ToolAudienceProfile`. Old + configs are auto-migrated by `netclaw doctor --fix` via the + `SchemaFixResolver` path (CLAUDE.md Configuration Schema Sync Rule). + +### Security and operations + +- **Net reduction in attack surface.** Today, any non-image file + silently goes to `/dev/null`, meaning the security model is + accidentally "deny all documents regardless of channel trust". This + change explicitly allows documents in trusted audiences (`Team`, + `Personal`) where the workspace auth fence is meaningful, while + tightening the `Public` default to images-only — a loud, auditable + policy rather than an accidental one. +- **No silent fallbacks anywhere on the path.** Every rejection + (unsupported category, oversize, too many files, scan failure, + capability-check failure) produces a user-visible reply stating what + was rejected and why. Per CLAUDE.md "No silent fallbacks" rule this is + a hard requirement, not an aspiration. +- **Disk footprint**: per-session session directory may grow. The + working directory already exists for tool-driven writes; this change + adds an `inbox/` sibling to `media/`. Existing session lifetime / + cleanup behavior applies unchanged — if it's adequate for tool + outputs today it is adequate for ingest files. +- **Operational telemetry**: each accepted inbound file emits an `INFO` + log (`name`, `mime`, `size`, `audience`, `categoryDecision`, + `inlined: true|false`). Each rejection emits a `WARN` log with the + same fields plus the rejection reason. These replace the current + `DEBUG` silent-drop log. + +### In-scope for MVP + +1. Contract + Slack implementation + `ToolAudienceProfile` policy + surface + `LlmSessionActor` silent-strip removal + dynamic-context + hint + PDF eval case + doc updates to PRD-009 and PRD-002. +2. All categories (`Image`, `Pdf`, `Document`, `Archive`, `Media`, + `Other`) mapped from MIME prefixes in code, so adding a new category + is a one-file change. +3. Schema migration via `netclaw doctor --fix`. + +### Out-of-scope (explicit non-goals) + +1. Server-side document extraction, OCR, or conversion. The agent uses + its existing `shell_execute` / `file_read` tools on demand; the + ingress layer just delivers bytes. +2. Discord / Teams / web widget adapters. This change defines the + contract they will follow; implementation of those adapters is + separate work. +3. Per-user or per-sender attachment allowlists (orthogonal to + audience). Can be layered on later via ACL extensions. +4. Outbound attachments — `attach_file` already covers the agent-writes + path and is not touched. +5. Slack file-threading niceties (quoting, reactions on the source + message, etc.). Unchanged. diff --git a/openspec/changes/channel-ingress-attachments/specs/netclaw-input-adapters/spec.md b/openspec/changes/channel-ingress-attachments/specs/netclaw-input-adapters/spec.md new file mode 100644 index 000000000..0b598eefe --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/specs/netclaw-input-adapters/spec.md @@ -0,0 +1,214 @@ +## ADDED Requirements + +### Requirement: Canonical cross-channel attachment ingress contract + +Every input adapter that transports user-originated file attachments SHALL implement the attachment ingress pipeline defined by this requirement before building the `ChannelInput.Contents` for a `SendUserMessage`, and the session actor SHALL NOT infer, strip, or rewrite attachments because ingress is the authoritative layer for modality routing and attachment policy. This requirement applies to the Slack Socket Mode adapter, the forthcoming Discord adapter, and any future transport (Teams, web widgets, etc.) that surfaces file uploads. + +For every file attached to an inbound message, the adapter SHALL: + +1. Resolve the `TrustAudience` for the inbound message using the + adapter's audience-classification path, which MUST run before any + attachment-specific processing. +2. Evaluate the file's MIME type against + `ToolAudienceProfile.ChannelAttachments.AllowedCategories` for the + resolved audience. If the file's category is not permitted, the + adapter SHALL reject the file before downloading any bytes and + SHALL post a user-visible reply naming the file and the category + that is not allowed for the current audience. +3. Compare the adapter-reported file size against + `ToolAudienceProfile.ChannelAttachments.MaxFileBytes`. Oversize + files SHALL be rejected before download with a user-visible reply + naming the file and the size limit. +4. Compare the total number of files on the inbound message against + `ToolAudienceProfile.ChannelAttachments.MaxFilesPerMessage`. If the + count exceeds the cap, the adapter SHALL reject the entire message's + attachments with a single user-visible reply stating the + per-message limit; text content SHALL still be delivered to the + session. +5. Download the file bytes using the adapter's transport-specific + mechanism. +6. Content-scan the downloaded bytes through `IContentScanner.ScanAsync` + before any write to disk or LLM exposure. Scan rejections (where + `scanResult.Error` is not `ScanFailure`) SHALL produce a + user-visible reply with the scanner's reason and the file SHALL NOT + be written to disk or delivered to the session. +7. Query `ModelCapabilityActor` for the active model's + `InputModalities` before building `ChannelInput.Contents`. A + capability query timeout SHALL produce a user-visible reply stating + that the attachment could not be processed; the adapter SHALL NOT + guess a modality or silently skip the capability gate. +8. Write accepted files atomically to the session's durable working + directory at `{SessionDirectory}/inbox/{safeFilename}`, where + `SessionDirectory` resolves through `NetclawPaths.SessionsDirectory` + and NOT through any overload of `SessionDirectoryHelper` that + targets `Path.GetTempPath()`. Filenames SHALL be sanitized via + `FilenameSanitizer.Sanitize` and collisions SHALL be resolved by + filesystem-level `_N` suffixing (`foo.pdf` → `foo_1.pdf` → … up to + `_99`), checking the filesystem and not just the current inbound + batch so that attachments from earlier turns are never overwritten. +9. Inject exactly one `TextContent` into `ChannelInput.Contents` per + accepted file, in the canonical format: + ``` + [attachment] name="" mime="" size= path="inbox/" inlined="true|false" [note=""] + ``` + Multiple accepted files from a single inbound message SHALL be + batched as multiple lines within a single `TextContent` block, in + the order they appeared on the inbound event. The `inlined` field + SHALL be machine-parseable and mandatory. The `note` field SHALL + be present if and only if `inlined="false"`. +10. Additionally append a `DataContent(bytes, mime)` to + `ChannelInput.Contents` when and only when the file's category is + natively renderable by the active model's reported modalities: + `image/*` when `ModelModality.Image` is set, and `application/pdf` + when the model reports native PDF support. In these cases the + corresponding `[attachment]` line SHALL have `inlined="true"` and + omit `note`. In all other cases the `[attachment]` line SHALL + have `inlined="false"` and include a `note` drawn from the + canonical note classes defined in the following requirement. +11. Produce no silent drops. Every rejected file — whether by + category, size, count, scan, capability timeout, inbox write + failure, or collision exhaustion — SHALL produce exactly one + user-visible reply on the originating channel explaining what was + rejected and why, and SHALL be logged at `WARN` or higher with + file name, MIME type, audience, and rejection reason. + +#### Scenario: Slack adapter delivers a PDF in a Team-trust channel on a PDF-capable model + +- **GIVEN** a `file_share` event arrives on a channel whose resolved + audience is `Team` +- **AND** the file is `report.pdf` with MIME `application/pdf` and + size 284512 bytes +- **AND** `ChannelAttachments.AllowedCategories` for `Team` contains + `Pdf` and `MaxFileBytes` is 25 MiB +- **AND** the active model reports `InputModalities` containing native + PDF support +- **WHEN** the adapter processes the event +- **THEN** the adapter downloads the file, scans it, and writes the + bytes to `{SessionDirectory}/inbox/report.pdf` +- **AND** appends a `TextContent` with the line + `[attachment] name="report.pdf" mime="application/pdf" size=284512 path="inbox/report.pdf" inlined="true"` +- **AND** appends a `DataContent(bytes, "application/pdf")` to the + same `ChannelInput.Contents` +- **AND** forwards the `SendUserMessage` to the session parent + +#### Scenario: Image attachment on a text-only model routes as path-only + +- **GIVEN** a message arrives with an `image/png` attachment in a + `Team`-trust channel +- **AND** the active model reports no `ModelModality.Image` support +- **WHEN** the adapter processes the event +- **THEN** the file is written to `{SessionDirectory}/inbox/.png` +- **AND** the `TextContent` line has `inlined="false"` and + `note="current model has no image modality; file is on disk but not viewable this turn"` +- **AND** no `DataContent` is appended for this file + +#### Scenario: Word document in a public channel is rejected before download + +- **GIVEN** a message arrives in a channel whose resolved audience + is `Public` +- **AND** the file's MIME is + `application/vnd.openxmlformats-officedocument.wordprocessingml.document` + (category `Document`) +- **AND** `ChannelAttachments.AllowedCategories` for `Public` is + `{ Image }` by default +- **WHEN** the adapter evaluates the file +- **THEN** the adapter SHALL NOT download the file +- **AND** the adapter SHALL post a user-visible reply explaining that + documents are not allowed in this audience and naming the file +- **AND** no `[attachment]` line or `DataContent` is added to the + `ChannelInput.Contents` +- **AND** a `WARN` log is emitted with the file name, MIME, audience, + and rejection reason + +#### Scenario: Oversize file is rejected pre-download + +- **GIVEN** a message arrives with a file whose reported size is 50 + MiB +- **AND** `ChannelAttachments.MaxFileBytes` is 25 MiB for the resolved + audience +- **WHEN** the adapter evaluates the file +- **THEN** the adapter SHALL NOT download the file +- **AND** the adapter SHALL post a user-visible reply naming the file + and the 25 MiB limit +- **AND** the text content of the inbound message SHALL still be + delivered to the session + +#### Scenario: Filename collision across turns does not overwrite prior files + +- **GIVEN** a previous turn already wrote `inbox/report.pdf` for the + same session +- **WHEN** a new inbound message attaches a different file also named + `report.pdf` +- **THEN** the adapter writes the new file to `inbox/report_1.pdf` +- **AND** the `[attachment]` line references + `path="inbox/report_1.pdf"` +- **AND** the previous `inbox/report.pdf` is unchanged on disk + +#### Scenario: Capability query timeout produces a loud rejection + +- **GIVEN** `ModelCapabilityActor` does not respond within the + adapter's capability query deadline +- **WHEN** the adapter attempts to build the `[attachment]` line +- **THEN** the adapter SHALL post a user-visible reply stating the + attachment could not be processed and the user should retry +- **AND** the adapter SHALL NOT guess a modality or append any + `DataContent` for this file +- **AND** a `WARN` log is emitted naming the file and the capability + timeout + +#### Scenario: Multi-file message batches attachment lines into one TextContent + +- **GIVEN** an inbound message attaches three accepted files +- **WHEN** the adapter builds `ChannelInput.Contents` +- **THEN** exactly one `TextContent` block contains three + `[attachment]` lines, one per file, in the order they appeared on + the inbound event +- **AND** each line carries its own `inlined` and (when applicable) + `note` fields + + +### Requirement: Canonical `[attachment]` `note` classes + +Every `[attachment]` line with `inlined="false"` SHALL carry a `note` field whose text begins with one of two canonical prefixes defined by this requirement, and adapter implementations SHALL source those strings from a single shared helper so that system prompts, evals, and future tooling can distinguish classes of non-inlined attachments by stable textual signal rather than ad-hoc phrasing. + +The two canonical classes are: + +- **Model-modality gap** — the file is a class the model could handle + on a different deployment. The `note` SHALL begin with + `"current model has no "` followed by the modality name and a short + remediation hint when one applies. For example: + `"current model has no image modality; file is on disk but not viewable this turn"` + or + `"current model has no native PDF support; use shell_execute (e.g., pdftotext) to extract text"`. +- **Format-not-inlineable** — the file has no native model + representation regardless of deployment (Office documents, archives, + video, audio, unknown binary). The `note` SHALL begin with + `"format not inlineable"` followed by a short tool-based remediation + hint. For example: + `"format not inlineable; use file_read or shell_execute to process"`. + +Adapter implementations SHALL source `note` strings from a shared +helper so all channels produce identical text for identical +situations. The canonical strings SHALL NOT be rephrased per-channel. + +#### Scenario: Non-vision model produces the model-modality note for an image + +- **GIVEN** an image attachment on a text-only model +- **WHEN** the adapter writes the `[attachment]` line +- **THEN** `note` begins with `current model has no image modality` + +#### Scenario: Non-PDF model produces the model-modality note for a PDF with remediation hint + +- **GIVEN** a PDF attachment on a model that reports no native PDF + modality +- **WHEN** the adapter writes the `[attachment]` line +- **THEN** `note` begins with `current model has no native PDF support` +- **AND** `note` mentions `shell_execute` as a remediation hint + +#### Scenario: Docx produces the format-not-inlineable note + +- **GIVEN** a `.docx` attachment on any model +- **WHEN** the adapter writes the `[attachment]` line +- **THEN** `note` begins with `format not inlineable` +- **AND** `note` mentions `file_read` or `shell_execute` as a + remediation hint diff --git a/openspec/changes/channel-ingress-attachments/specs/netclaw-session/spec.md b/openspec/changes/channel-ingress-attachments/specs/netclaw-session/spec.md new file mode 100644 index 000000000..584ee510d --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/specs/netclaw-session/spec.md @@ -0,0 +1,117 @@ +## ADDED Requirements + +### Requirement: Session actor is a strict modality consumer + +The session actor SHALL treat inbound `ChannelInput.Contents` as +authoritative with respect to `DataContent` modalities. The session +SHALL NOT silently strip `DataContent` items whose modality is not +supported by the active model. If the session detects a `DataContent` +item whose modality is not in the active model's reported +`InputModalities`, it SHALL treat that as an ingress bug by: + +- Emitting an `ERROR`-level log line naming the active model id, the + model's declared `InputModalities`, and every offending attachment + by name and modality, with text stating that the originating + channel did not apply the cross-channel attachment ingress + contract before inlining the `DataContent`. +- Dropping the offending `DataContent` items from the turn so the + provider call does not fail outright. +- Appending a visible `TextContent` line to the turn's contents in + the form + `[system] an attachment was received but could not be delivered to the model due to an ingress bug; please retry or notify the operator` + so the downstream reply cannot silently omit the incident. +- Completing the turn normally otherwise. + +The session SHALL NOT substitute a placeholder such as +`[Images removed — the current model does not support vision input]`. +That pattern was a symptom of silent modality decisions happening in +the wrong architectural layer and is explicitly prohibited by this +requirement. + +#### Scenario: Image DataContent on text-only model triggers loud error + +- **GIVEN** an inbound `ChannelInput` contains a `DataContent` with + MIME `image/png` +- **AND** the active model reports no `ModelModality.Image` +- **WHEN** the session actor processes the turn +- **THEN** an `ERROR` log is emitted naming the model id, modalities, + and offending attachment +- **AND** the `DataContent` is dropped from the turn +- **AND** a `[system]` `TextContent` line is appended to the turn + stating that an ingress bug prevented delivery +- **AND** the turn completes without failing the session + +#### Scenario: Correctly routed PDF DataContent passes the consumer check + +- **GIVEN** an inbound `ChannelInput` contains a `DataContent` with + MIME `application/pdf` +- **AND** the active model reports native PDF support +- **WHEN** the session actor processes the turn +- **THEN** no `ERROR` log is emitted +- **AND** the `DataContent` is delivered to the provider unchanged + +#### Scenario: No Images-removed placeholder is ever produced + +- **GIVEN** any turn on any model configuration +- **WHEN** the session actor processes the turn +- **THEN** no outbound turn contains the text + `Images removed — the current model does not support vision input` +- **AND** the legacy silent-strip code path has been removed from + `LlmSessionActor` + + +### Requirement: Attachment-aware dynamic context layer + +The session actor SHALL append a dedicated attachment-handling block to the system prompt during dynamic context layer assembly when, and only when, the session's audience profile grants `file_read`. The block SHALL: + +- Name the session working directory's `inbox/` subdirectory as the + canonical location for user-uploaded files. +- Document the canonical `[attachment]` line format, including the + mandatory `inlined` field and the conditional `note` field. +- Define how the agent SHOULD interpret `inlined="false"` for each of + the two canonical `note` prefix classes + (`current model has no ...` → model-modality gap; `format not inlineable` → + tool-accessible binary) and direct the agent to use `file_read` or + `shell_execute` as appropriate when the agent chooses to process + the file. +- Include an explicit imperative that the agent SHALL acknowledge + every attachment it received in its reply, by name, even when the + file was not inlined and cannot be rendered natively — silent + omission of an unviewable attachment is prohibited. + +When the session's audience profile does NOT grant `file_read`, the +session actor SHALL NOT inject the attachment-handling block. In that +case there is no supported path for the agent to inspect inbox files, +and injecting the block would advertise a capability the session +does not have. + +The block SHALL be static text sourced from a single shared +constant, so wording does not drift per session or per channel. +Dynamic context layer injection SHALL remain compatible with existing +layers (skill index, recall bundle, project context, etc.). + +#### Scenario: Team audience with file_read gets the attachment block + +- **GIVEN** a session resolves to `TrustAudience.Team` with a profile + that grants `file_read` +- **WHEN** the session actor assembles the system prompt +- **THEN** the system prompt contains the attachment-handling block +- **AND** the block names `inbox/` and the `[attachment]` line format +- **AND** the block explains both canonical `note` prefix classes + +#### Scenario: Audience without file_read does not get the block + +- **GIVEN** a session resolves to an audience profile that does NOT + grant `file_read` +- **WHEN** the session actor assembles the system prompt +- **THEN** the attachment-handling block is NOT appended +- **AND** no text referring to `inbox/` or `[attachment]` is added + +#### Scenario: Block instructs the agent to acknowledge unviewable attachments + +- **GIVEN** the attachment-handling block is present in a system + prompt +- **WHEN** the block content is inspected +- **THEN** it contains an imperative sentence directing the agent to + acknowledge any attachment it cannot view natively, by name, in + its turn reply diff --git a/openspec/changes/channel-ingress-attachments/specs/netclaw-slack-socket/spec.md b/openspec/changes/channel-ingress-attachments/specs/netclaw-slack-socket/spec.md new file mode 100644 index 000000000..c742b03e7 --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/specs/netclaw-slack-socket/spec.md @@ -0,0 +1,231 @@ +## ADDED Requirements + +### Requirement: Slack attachment ingestion implements the canonical contract + +`SlackThreadBindingActor` SHALL implement the canonical cross-channel +attachment ingress contract defined in `netclaw-input-adapters` for +every `file_share` subtype and every `files` array on an inbound +Slack `message` or `app_mention` event. The current behavior of +hard-coding an `image/*`-only allowlist at the top of the file-handling +loop SHALL be removed; acceptance is driven by +`ToolAudienceProfile.ChannelAttachments.AllowedCategories` for the +resolved `TrustAudience` of the inbound message. + +The Slack adapter SHALL: + +- Download attachments via `url_private_download` with bot-token + Bearer auth (unchanged). +- Use `SlackAclPolicy.ResolveAudience` to determine the `TrustAudience` + before any attachment processing. +- Reject pre-download when category, size, or file-count policy is + violated, posting the user-visible reply through the same + `SafePostAsync` path already used for other inbound errors. +- Query `ModelCapabilityActor` with a deadline of 2 seconds via the + `ActorRegistry` `ModelCapabilityActorKey`; on timeout, post a + user-visible reply and skip the file. +- Write accepted files to + `{SessionDirectory}/inbox/` where `SessionDirectory` is + resolved through `NetclawPaths.SessionsDirectory`, NOT through any + `SessionDirectoryHelper` overload that uses `Path.GetTempPath()`. +- Emit INFO-level logs for every accepted file including file name, + MIME, size, resolved audience, the adapter's category decision, and + whether the file was inlined. +- Emit WARN-level logs for every rejection with the same fields plus + the rejection reason. The previous DEBUG-level silent-drop log for + non-image files SHALL be removed entirely; no rejection is ever + logged below INFO. + +#### Scenario: PDF in a Team-trust DM is accepted and inlined + +- **GIVEN** a direct message arrives with `report.pdf` + (`application/pdf`, 284512 bytes) +- **AND** the resolved audience is `Team` and `Pdf` is in + `AllowedCategories` +- **AND** the active model reports native PDF support via + `ModelCapabilityActor` +- **WHEN** `SlackThreadBindingActor` processes the event +- **THEN** the file is written to `inbox/report.pdf` in the durable + session directory +- **AND** the `ChannelInput.Contents` contains an `[attachment]` line + with `inlined="true"` and a matching `DataContent` +- **AND** an INFO log entry records the accepted file + +#### Scenario: Word document in public channel is rejected pre-download + +- **GIVEN** a public channel message attaches `notes.docx` +- **AND** the resolved audience is `Public` with default + `AllowedCategories = { Image }` +- **WHEN** `SlackThreadBindingActor` processes the event +- **THEN** no HTTP download of the file occurs +- **AND** a reply is posted to the originating thread explaining that + documents are not allowed in public channels +- **AND** a WARN log is emitted with the file name, MIME, + audience=Public, and rejection reason `category-not-allowed` + +#### Scenario: Non-image file no longer silently dropped + +- **GIVEN** a file of any non-image MIME type that the current + audience policy would permit +- **WHEN** `SlackThreadBindingActor` processes the event +- **THEN** the file is NOT skipped with a DEBUG log line +- **AND** the file is processed through the full canonical pipeline + (policy check, download, scan, capability query, inbox write, + `[attachment]` injection) + +#### Scenario: Capability timeout produces a user-visible reply + +- **GIVEN** `ModelCapabilityActor` does not respond to + `GetModelCapabilities` within 2 seconds +- **WHEN** `SlackThreadBindingActor` needs to decide inlining for a + permitted file +- **THEN** a reply is posted to the originating thread stating the + attachment could not be processed and suggesting retry +- **AND** no `DataContent` is appended for this file +- **AND** no `[attachment]` line with a fabricated `inlined` value is + appended + + +## MODIFIED Requirements + +### Requirement: Merge hydrated content into triggering ChannelInput + +Hydrated gap content SHALL be merged directly into the triggering +inbound event's `ChannelInput` rather than delivered as separate +messages. The merge SHALL produce a single `ChannelInput` whose +`Contents` contain: + +1. One `TextContent` that begins with the header + `[thread history — messages exchanged before this inbound event]`, + contains one entry per gap message with sender attribution and a + UTC timestamp, ends with `[end thread history]`, and is followed + by the triggering message's live text. +2. One `TextContent` block carrying an `[attachment]` line per + attachment from any gap message that passes the same attachment + policy and capability gates defined in + `netclaw-input-adapters`. Each line carries the canonical `name`, + `mime`, `size`, `path="inbox/..."`, `inlined`, and (when + applicable) `note` fields. Attachments that are rejected by the + current audience or capability gates SHALL NOT appear as + `[attachment]` lines; instead the text block for that gap entry + SHALL note the rejection in the form + `[attachment rejected: ()]` so the historical + context is preserved without leaking prohibited content. +3. Any `DataContent` items from gap messages for files the active + model's modalities report as natively renderable (e.g., images on + a vision-capable model, PDFs on a PDF-capable model), produced by + the same capability gate that controls live-turn inlining. +4. The corresponding `[attachment]` and `DataContent` items for the + triggering message's own attachments, produced exactly as on a + non-hydrated turn. + +The session layer SHALL receive exactly one `SendUserMessage` for the +triggering event with no special handling. + +#### Scenario: Single merged message reaches the session + +- **GIVEN** a gap of 3 historical messages and 1 triggering mention +- **WHEN** hydration completes +- **THEN** exactly one `ChannelInput` is written to the input channel +- **AND** its first `TextContent` contains the `[thread history …]` + block followed by the live mention text + +#### Scenario: Historical PDF included as DataContent on a PDF-capable model + +- **GIVEN** a gap message has one `application/pdf` attachment in a + `Team`-trust context +- **AND** the active model reports native PDF support +- **WHEN** the merge runs +- **THEN** the attachment is written to `inbox/` and appears as a + `DataContent` on the merged `ChannelInput` +- **AND** an `[attachment] ... inlined="true"` line appears in the + merged `[thread history]` block for that entry + +#### Scenario: Historical image on a non-vision model is recorded path-only + +- **GIVEN** a gap message has one `image/png` attachment +- **AND** the active model does not report `ModelModality.Image` +- **WHEN** the merge runs +- **THEN** the attachment is written to `inbox/` +- **AND** an `[attachment] ... inlined="false" note="current model has no image modality ..."` + line appears in the merged `[thread history]` block for that entry +- **AND** no `DataContent` is added for this historical file + +#### Scenario: Historical document in a public channel is recorded as rejected + +- **GIVEN** the resolved audience is `Public` with default + `AllowedCategories = { Image }` +- **AND** a gap message in the same channel had a `.docx` attachment +- **WHEN** the merge runs +- **THEN** the attachment SHALL NOT be written to `inbox/` +- **AND** no `[attachment]` line is produced for it +- **AND** the text block for that gap entry records + `[attachment rejected: notes.docx (category not allowed in Public)]` + +#### Scenario: Empty gap produces an unmerged inbound + +- **GIVEN** the fetcher returns history but no messages fall strictly + between the cursor and the triggering event +- **WHEN** the actor builds the merged input +- **THEN** the triggering event is enqueued with its original content + only +- **AND** no `[thread history …]` block is added + + +### Requirement: Slack history fetch via conversations.replies + +`SlackThreadHistoryFetcher` SHALL implement `IThreadHistoryFetcher` +using `ISlackApiClient.Conversations.Replies`. It SHALL paginate +through all replies, filter out the bot's own messages and any other +messages carrying a `bot_id`, and for every file attachment on any +surviving message it SHALL download the bytes via +`url_private_download` with bot-token Bearer auth and content-scan +the bytes through `IContentScanner` — regardless of MIME type. +Category-, size-, and capability-gating SHALL be applied by the +caller (`SlackThreadBindingActor` during the merge step) using the +same policy as live inbound events, not by the fetcher itself. +Per-attachment download or scan failures SHALL be skipped with a +warning. API-level failures (permission denied, server error) SHALL +return an empty list. + +#### Scenario: Paginated fetch for long threads + +- **GIVEN** a thread has more than 1000 messages +- **WHEN** the fetcher retrieves the thread +- **THEN** it paginates using the cursor returned by each response + until no cursor remains +- **AND** returns all messages in chronological order + +#### Scenario: Bot messages excluded + +- **GIVEN** a thread contains messages from users, the Netclaw bot, + and a CI bot +- **WHEN** the fetcher retrieves the thread +- **THEN** messages matching the Netclaw bot id are excluded +- **AND** messages carrying any other `bot_id` are excluded +- **AND** only human user messages remain + +#### Scenario: All file types downloaded and scanned during fetch + +- **GIVEN** a gap message has a mix of images, a PDF, and a docx +- **WHEN** the fetcher processes that message +- **THEN** the fetcher downloads the bytes for every file +- **AND** content-scans every file regardless of MIME type +- **AND** returns the bytes and MIME for each file so the caller can + apply audience and capability policy at merge time + +#### Scenario: Per-file download failure skipped with warning + +- **GIVEN** a gap message has two attachments and one returns an HTTP + 403 +- **WHEN** the fetcher processes that message +- **THEN** the failed attachment is dropped with a warning naming the + file ID and the HTTP status +- **AND** the other attachment is returned normally + +#### Scenario: API error does not block session creation + +- **GIVEN** `conversations.replies` returns a permission error +- **WHEN** the fetcher runs +- **THEN** the fetcher logs a warning and returns an empty list +- **AND** the binding actor enqueues the triggering event with its + original content only diff --git a/openspec/changes/channel-ingress-attachments/specs/tool-approval-gates/spec.md b/openspec/changes/channel-ingress-attachments/specs/tool-approval-gates/spec.md new file mode 100644 index 000000000..eccbfe3a3 --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/specs/tool-approval-gates/spec.md @@ -0,0 +1,115 @@ +## ADDED Requirements + +### Requirement: Channel attachment policy per audience + +The system SHALL support per-audience inbound channel attachment +policy via a `ChannelAttachments` field on `ToolAudienceProfile`, +extending the same per-audience configuration container that already +holds `ToolApprovalConfig`, file-access profiles, and tool grants. +`ChannelAttachments` SHALL specify: + +- `AllowedCategories` — a set of `AttachmentCategory` enum values + (`Image`, `Pdf`, `Document`, `Archive`, `Media`, `Other`) naming + which classes of inbound files a channel adapter is permitted to + deliver to the session for this audience. +- `MaxFileBytes` — a per-file size cap, applied by channel adapters + against the transport-reported size before download. +- `MaxFilesPerMessage` — a per-inbound-message attachment-count cap, + applied by channel adapters before download. + +Channel adapters SHALL read these values through the resolved +`ToolAudienceProfile` for the inbound message's `TrustAudience` and +SHALL NOT maintain a parallel attachment policy surface. MIME types +SHALL be mapped to `AttachmentCategory` via a single internal +mapping function so that adding coverage for a new MIME type is a +one-place change. Unknown or unrecognized MIME types SHALL map to +`AttachmentCategory.Other`. + +Default `AllowedCategories` per audience SHALL be: + +- `Public`: `{ Image }`. Documents, PDFs, archives, media, and + unknown binaries are rejected by default because processing them + typically routes through tool-based execution on user-controlled + bytes in a context where any workspace member can upload. +- `Team`: `{ Image, Pdf, Document, Archive, Media }`. All well-known + categories except unknown binaries. +- `Personal`: `{ Image, Pdf, Document, Archive, Media, Other }`. All + categories including unknown MIME types. + +Default `MaxFileBytes` SHALL be 25 × 1024 × 1024 (25 MiB) for every +audience. Default `MaxFilesPerMessage` SHALL be 10 for every +audience. Operators SHALL be able to override any cell via +configuration; overrides SHALL be validated at startup against +`netclaw-config.v1.schema.json`. + +#### Scenario: Default Public profile rejects a PDF category + +- **GIVEN** a session resolves to `TrustAudience.Public` +- **AND** the operator has not overridden `ChannelAttachments` for + the `Public` profile +- **WHEN** a channel adapter evaluates a file with MIME + `application/pdf` +- **THEN** the `Pdf` category is not in the resolved profile's + `AllowedCategories` +- **AND** the adapter rejects the file per the cross-channel contract + +#### Scenario: Default Team profile accepts a Word document + +- **GIVEN** a session resolves to `TrustAudience.Team` +- **WHEN** a channel adapter evaluates a file with MIME + `application/vnd.openxmlformats-officedocument.wordprocessingml.document` +- **THEN** the file maps to `AttachmentCategory.Document` +- **AND** the `Document` category is in the resolved profile's + default `AllowedCategories` +- **AND** the adapter proceeds to size and scan checks + +#### Scenario: Unknown MIME type maps to Other and is rejected in Team + +- **GIVEN** a session resolves to `TrustAudience.Team` +- **WHEN** a channel adapter evaluates a file with an unrecognized + MIME type +- **THEN** the file is mapped to `AttachmentCategory.Other` +- **AND** the default `Team` profile does NOT allow `Other` +- **AND** the adapter rejects the file + +#### Scenario: Unknown MIME type is accepted in Personal + +- **GIVEN** a session resolves to `TrustAudience.Personal` +- **WHEN** a channel adapter evaluates a file with an unrecognized + MIME type +- **THEN** the file is mapped to `AttachmentCategory.Other` +- **AND** the default `Personal` profile allows `Other` +- **AND** the adapter proceeds to size and scan checks + +#### Scenario: Operator override widens Public to allow images and PDFs + +- **GIVEN** the operator has configured + `ChannelAttachments.AllowedCategories = { Image, Pdf }` on the + `Public` profile +- **AND** `netclaw doctor` validates the config on startup +- **WHEN** a channel adapter evaluates a PDF in a public channel +- **THEN** the `Pdf` category is permitted +- **AND** the adapter proceeds to size and scan checks + +#### Scenario: MaxFileBytes default is 25 MiB + +- **GIVEN** no operator override for `MaxFileBytes` +- **WHEN** `ChannelAttachments` is materialized from defaults for any + audience +- **THEN** `MaxFileBytes` equals 25 × 1024 × 1024 + +#### Scenario: MaxFilesPerMessage default is 10 + +- **GIVEN** no operator override for `MaxFilesPerMessage` +- **WHEN** `ChannelAttachments` is materialized from defaults for any + audience +- **THEN** `MaxFilesPerMessage` equals 10 + +#### Scenario: Schema migration inserts defaults for stale configs + +- **GIVEN** an existing config without a `ChannelAttachments` block +- **WHEN** `netclaw doctor --fix` runs against the config +- **THEN** the schema fix resolver inserts the default + `ChannelAttachments` block for each audience profile +- **AND** the fixed config validates against + `netclaw-config.v1.schema.json` diff --git a/openspec/changes/channel-ingress-attachments/tasks.md b/openspec/changes/channel-ingress-attachments/tasks.md new file mode 100644 index 000000000..f8ebf4e02 --- /dev/null +++ b/openspec/changes/channel-ingress-attachments/tasks.md @@ -0,0 +1,135 @@ +> **Scope note (2026-04-11):** the original plan ran 15 phases and 76 tasks. +> After implementing Phases 1–6 it became clear that the core user-reported +> bug (silent PDF drop in Slack ingest) is fixed, and the remaining phases +> should be trimmed to what's actually required for ship-quality on that +> bug — not the "canonical cross-channel contract" vision that ballooned +> out of it. Phases 7, 11, 12, 13, and 15 are explicitly cut from this +> change and tracked as follow-up work in the "Deferred" section below. + +## 1. Config surface and schema + +- [x] 1.1 Add `AttachmentCategory` enum (`Image`, `Pdf`, `Document`, `Archive`, `Media`, `Other`) to `Netclaw.Configuration` +- [x] 1.2 Add `ChannelAttachmentPolicy` record with `AllowedCategories`, `MaxFileBytes`, `MaxFilesPerMessage` +- [x] 1.3 Add `ChannelAttachments` field on `ToolAudienceProfile` defaulting to `ChannelAttachmentPolicy.Empty` (fail-closed) when not set +- [x] 1.4 Implement `ToolAudienceProfileDefaults` entries for `Public` / `Team` / `Personal` using the matrix from design D4 (Public = {Image}; Team = everything except Other; Personal = all six; 25 MiB; 10 files) +- [x] 1.5 Extend `netclaw-config.v1.schema.json` with `ChannelAttachmentPolicy` + `AttachmentCategory` definitions and default values for `SchemaFixResolver` +- [x] 1.6 Validate size/count caps > 0 when `AllowedCategories` is non-empty; fail daemon startup in `Program.cs` on violation; also surfaced in `ToolAudienceProfilesDoctorCheck` +- [x] 1.7 Unit-test the defaults, validator, and MIME classifier (31 cases in `ChannelAttachmentPolicyTests`) + +## 2. Central MIME → category mapping + +- [x] 2.1 `AttachmentCategories.FromMime` in `Netclaw.Configuration` as the single classifier +- [x] 2.2 Tests for case-insensitivity, empty/null, unknown → `Other`, all canonical types +- [x] 2.3 Grep audit: the two existing `StartsWith("image/...")` call sites in the Slack channel are retired as part of Phase 6 + Phase 7. No other domain uses MIME→category mapping. + +## 3. Session working directory hardening + +- [x] 3.1 Removed legacy single-arg `SessionDirectoryHelper.GetSessionDirectory(SessionId)` overload +- [x] 3.2 Migrated all call sites to the base-path overload; audited tests +- [x] 3.3 Added `GetOrCreateInboxDirectory(sessionId, basePath)` + `InboxSubdirectory`/`MediaSubdirectory` constants +- [x] 3.4 Added `IsUnderTempPath` warning in `ToolAudienceProfilesDoctorCheck` +- [x] 3.5 Confirmed no session-directory cleanup hook exists today (neither `media/` nor any other subdirectory). Tracked as a pre-existing leak, out of scope for this change. +- [x] 3.6 `NetclawPaths` is now required on `SessionServices` and `SessionPipeline` — no more null-coalescing fallbacks to `Path.GetTempPath()`. 13 test fixtures migrated via the shared `AddTestNetclawPaths()` helper. +- [x] 3.7 **Log-path leak fix**: moved per-session logs from `{SessionsDirectory}/{id}/logs/` (inside the agent-readable session working dir) to `{LogsDirectory}/sessions/{id}/`, which the agent's `file_read` tool cannot reach. Renamed `NetclawPaths.LegacySessionLogsDirectory` → `SessionLogsDirectory`; updated `SessionLogActor`, `LlmSessionActor`, and `SessionCatalogService`. + +## 4. Filename sanitization and atomic inbox write + +- [x] 4.1 Verified `FilenameSanitizer.Sanitize` handles `..`, NUL, control chars, absolute paths, Windows reserved names +- [x] 4.2 `InboxWriter.ReserveUniquePath` with filesystem-checked `_1`..`_99` collision suffixing +- [x] 4.3 `InboxWriter.WriteAtomicAsync` via temp-file + `File.Move` +- [x] 4.4 `SanitizeReserveAndWriteAsync` convenience method reusing `FilenameSanitizer` +- [x] 4.5 9 unit tests covering happy path, collisions, exhaustion, temp-file cleanup, path traversal + +## 5. ChannelIngressCapabilityQuery helper (built then deleted) + +- [~] 5.1 Built `ChannelIngressCapabilityQuery.QueryAsync` + typed `CapabilityQueryResult` in `Netclaw.Actors.Channels`, then **deleted** after Phase 6 revealed the helper had zero production call sites. The Slack implementation reads `_dependencies.ModelCapabilities.InputModalities` directly — the active Main model is already in DI as a singleton with no round-trip cost. The helper was scaffolding for hypothetical future channels (Discord, subagents, failover) that don't exist. +- [~] 5.2 Same fate — typed result record and tests were built then removed. +- [~] 5.3 5 unit tests that never guarded any real code path, removed with the helper. +- **Lesson:** don't pre-build cross-channel scaffolding before the second channel exists. When Discord lands, re-add whatever shape makes sense for its actual needs. ~250 lines of code and tests removed; `AttachmentNotes` is the only cross-channel helper that survived this phase (and is used by Slack today). + +## 6. Slack ingress rewrite (`SlackThreadBindingActor`) + +- [x] 6.1 Deleted the `image/`-only allowlist and the silent-drop DEBUG log +- [x] 6.2 Implemented the nine-step pipeline (audience gate → size gate → count gate → download → scan → direct modality read → inbox write → `[attachment]` line → DataContent inline). Pre-download gates short-circuit on Slack metadata; no bandwidth burned on rejected files. +- [x] 6.3 Private `BuildAttachmentLine` formatter emitting the canonical text shape with quoted-value escaping +- [x] 6.4 `note` strings sourced from a shared `AttachmentNotes` static class so the canonical prefixes never drift +- [x] 6.5 Multi-file announcements batched into a single `TextContent` block in original order +- [x] 6.6 User-visible rejection replies for every failure mode via `SafePostAsync` +- [x] 6.7 Accepted-file INFO log with structured fields (`name`, `mime`, `size`, `audience`, `category`, `inlined`) +- [x] 6.8 WARN log on every rejection path with the same fields plus a rejection reason +- [x] 6.9 Pre-download gates verified — no `HttpClient.SendAsync` call for rejected files +- [x] 6.10 `ChannelInput.Audience` still populated via the existing `SlackAclPolicy.ResolveAudience` path + +## 7. `LlmSessionActor` strict-consumer contract + +- [x] 7.1 Deleted the silent image-strip block at `LlmSessionActor.cs:1703-1717` + the now-unreachable "only images, no text" skip block +- [x] 7.2 Replaced with a loud ERROR log naming the model id, modalities, and offending attachments; drops the unsupported refs; appends a `[system]` TextContent line about the ingress bug to the user content; completes the turn normally +- [x] 7.3 Grep-assert: zero production matches for the legacy `"Images removed — the current model does not support vision input"` string +- [x] 7.4 Added the attachment-aware dynamic-context block (`AttachmentContextHint` constant) to `InjectDynamicContextLayers`, conditional on `file_read` being in `_availableTools` +- [x] 7.5 Unit tests: + - `ModalityGateTests.Image_with_text_on_text_only_model_surfaces_ingress_bug_and_still_calls_llm` — valid path plus ingress-bug notice reaches LLM + - `ModalityGateTests.Image_only_message_on_text_only_model_still_calls_llm_with_ingress_bug_notice` — empty-text + unsupported media still completes the turn with the notice (old behavior skipped the turn; new behavior always gives the user a reply) + - `ModalityGateVisionTests.Image_passes_through_to_vision_model` — regression: valid modalities still pass through untouched + - `AttachmentContextHintTests` — 6 bear-trap cases pinning the canonical shape of the dynamic-context block and cross-checking against `AttachmentNotes` constants + +## 8. Slack regression tests for the rewritten pipeline + +All eight in `SlackAttachmentIngressTests.cs`: + +- [x] 8.1 `Pdf_in_dm_on_vision_capable_model_is_saved_to_inbox_and_inlined` — PDF happy path, inlined=true, matching `DataContent`, file on disk +- [x] 8.2 **Covered by Phase 7's `Image_with_text_on_text_only_model_...` test** — the strict-consumer contract in `LlmSessionActor` handles the text-only-model case directly; the Slack ingress path itself doesn't need a separate text-only test because it reads `ModelCapabilities.InputModalities` synchronously from the same DI singleton, and the vision-capable tests in this file already cover the inline decision logic +- [x] 8.3 `Docx_in_dm_is_path_only_with_format_not_inlineable_note` — `.docx` → inbox write, `inlined="false" note="format not inlineable..."`, no `DataContent` +- [x] 8.4 `Docx_in_public_channel_is_rejected_pre_download` — public audience forced via `ChannelAudiences`, no HTTP download, user-visible rejection +- [x] 8.5 `Oversize_file_is_rejected_pre_download` — 30 MiB > 25 MiB cap, no HTTP download, user-visible rejection naming the limit +- [x] 8.6 `Too_many_attachments_rejects_entire_batch_but_forwards_text` — 15 files > 10 cap, entire batch rejected with one reply, text content still forwarded +- [x] 8.7 `Filename_collision_across_turns_produces_suffixed_path` — second upload of `photo.png` in the same Slack thread lands at `photo_1.png`, first file unchanged +- [x] 8.8 `Scanner_rejection_surfaces_user_visible_reply_with_no_inbox_write` — scanner-blocked file → user-visible reply naming the scanner reason, no inbox write + +## 9. Quality gates + +- [x] 9.1 `dotnet build Netclaw.slnx` — 0 warnings, 0 errors +- [x] 9.2 Test suites: Configuration (176), Actors (925), Cli (13 ConfigSchema tests including two new channel-attachments round-trip cases) — all green +- [x] 9.3 `dotnet slopwatch analyze` — 0 new violations. Two intentional best-effort empty-catch blocks (`InboxWriter.TryDeleteTemp` and `InboxWriterTests.Dispose`) added to `.slopwatch/baseline.json` as expected best-effort cleanup patterns. +- [x] 9.4 Schema round-trip verified via two new unit tests: a legacy config without any `ChannelAttachments` block still validates (additive, optional fields), and an explicit `ChannelAttachments` block on a single profile also validates. + +## Deferred (explicitly out of scope for this change) + +The following were in the original plan but are deferred as either +secondary-path work, process/documentation, or post-ship bookkeeping. +Each should land as its own small follow-up once the core change is +merged. + +- **SlackThreadHistoryFetcher backfill rewrite.** The fetcher still + hard-filters to image attachments (`SlackThreadHistoryFetcher.cs:148`), + so backfilled historical PDFs/docs are invisible to the agent. Live + ingest is fixed; backfill is not. Follow-up issue: generalize the + fetcher to all MIME types and move the audience/capability gate into + the merge step in `SlackThreadBindingActor`, mirroring the live-turn + pipeline. Includes scenarios in the `netclaw-slack-socket` spec delta + (already in this change's `specs/` folder) for historical-attachment + routing on vision-capable vs text-only models. + +- **Eval suite regression cases.** PDF round-trip, model-modality gap, + and format-not-inlineable. These are the behavioral guarantees the + new ingress contract makes, and evals are the right regression tool + per CLAUDE.md. Not a blocker for the bug fix itself. + +- **PRD updates.** PRD-009 (Input Adapters) and PRD-002 (Gateway + Security Envelope) should grow new sections covering the attachment + ingress contract and the per-audience policy surface. Documentation, + not behavior. + +- **System skill sync.** `feeds/skills/.system/files/netclaw-operations/SKILL.md` + should gain agent-facing guidance about `inbox/` and the `[attachment]` + line format once the dynamic-context block lands in Phase 7. Tied to + Phase 7, not the core ingress fix. + +- **OpenSpec finalization (`/opsx-verify`, `/opsx-sync`, `/opsx-archive`).** + The proposal/design/specs currently describe the fuller "canonical + cross-channel contract" vision and reference scenarios (historical + PDF backfill, normative note strings used by Discord, etc.) that this + truncated change does not deliver. Syncing the delta specs into + `openspec/specs/` before the deferred follow-ups land would publish a + contract ahead of implementation. Leave the change in `openspec/changes/` + as-is until the follow-ups catch up, or sync with explicit notes about + the subset delivered. diff --git a/src/Netclaw.Actors.Tests/Channels/SlackActorHierarchyTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackActorHierarchyTests.cs index 46571eeb0..f010c93a6 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackActorHierarchyTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackActorHierarchyTests.cs @@ -234,6 +234,9 @@ private static SlackGatewayDependencies CreateDependencies( ReplyClient: new NoopReplyClient(), ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: TestSlackGatewayDeps.NewTestPaths(), ConversationPropsFactory: conversationPropsFactory, ThreadPropsFactory: threadPropsFactory); } diff --git a/src/Netclaw.Actors.Tests/Channels/SlackAttachmentIngressTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackAttachmentIngressTests.cs new file mode 100644 index 000000000..4264e3a17 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Channels/SlackAttachmentIngressTests.cs @@ -0,0 +1,601 @@ +using System.Net; +using System.Net.Http.Headers; +using Akka.Actor; +using Akka.Hosting; +using Akka.Hosting.TestKit; +using Akka.Persistence.Hosting; +using Microsoft.Extensions.AI; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Netclaw.Actors.Channels; +using Netclaw.Actors.Hosting; +using Netclaw.Actors.Memory; +using Netclaw.Actors.Protocol; +using Netclaw.Actors.Sessions; +using Netclaw.Actors.Tests.Sessions; +using Netclaw.Channels.Slack; +using Netclaw.Configuration; +using Netclaw.Security; +using SlackNet.Blocks; +using Xunit; + +namespace Netclaw.Actors.Tests.Channels; + +/// +/// Regression tests for the cross-channel attachment ingress pipeline +/// implemented in . Each test +/// exercises one of the nine pipeline steps end-to-end (audience/size/count +/// gates, download, scan, modality read, inbox write, announcement line, +/// DataContent inlining) against a stubbed HTTP handler, content scanner, +/// and reply client. None of these tests touch a live Slack connection. +/// +public sealed class SlackAttachmentIngressVisionTests : TestKit +{ + private static readonly byte[] FakePngBytes = Convert.FromBase64String( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/5+hHgAHggJ/PchI7wAAAABJRU5ErkJggg=="); + + private static readonly byte[] FakePdfBytes = + "%PDF-1.4\n1 0 obj\n<< /Type /Catalog /Pages 2 0 R >>\nendobj\n"u8.ToArray(); + + private static readonly byte[] FakeDocxBytes = + "PK\u0003\u0004fake docx content"u8.ToArray(); + + private readonly RecordingChatClient _chatClient = new(); + private readonly RecordingReplyClient _replyClient = new(); + private readonly ConfigurableFakeSlackFileHandler _httpHandler = new(); + private readonly NetclawPaths _paths = new(Path.Combine( + Path.GetTempPath(), + $"netclaw-slack-attachment-tests-{Guid.NewGuid():N}")); + + public SlackAttachmentIngressVisionTests(ITestOutputHelper output) : base(output: output) + { + _paths.EnsureDirectoriesExist(); + } + + protected override void ConfigureServices(HostBuilderContext context, IServiceCollection services) + { + services.AddSingleton(new SingleClientProvider(_chatClient)); + services.AddSingleton(_paths); + services.AddSingleton(new ModelCapabilities + { + ModelId = "fake-vision-model", + ContextWindowTokens = 128_000, + InputModalities = ModelModality.Text | ModelModality.Image, + OutputModalities = ModelModality.Text, + }); + services.AddSingleton(new SessionConfig + { + Tuning = new SessionTuning + { + SnapshotInterval = 5, + TitleGenerationInterval = 0, + } + }); + services.AddSingleton(new StaticSystemPromptProvider( + "You are a test assistant.")); + services.AddSingleton(new FakeCapabilityResolver()); + services.AddSingleton(); + + services.AddSingleton(sp => new SessionServices( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetService>() ?? Array.Empty(), + sp.GetService() ?? TimeProvider.System, + sp.GetRequiredService())); + services.AddSingleton(sp => new SessionMemoryServices( + sp.GetService() ?? NullMemoryExtractor.Instance, + sp.GetService() ?? NullMemoryRecallCoordinator.Instance, + sp.GetService() ?? NullMemoryCheckpointSink.Instance, + sp.GetService())); + services.AddSingleton(new SessionObservability(null, null)); + } + + protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IServiceProvider provider) + { + builder + .WithInMemoryJournal() + .WithInMemorySnapshotStore() + .WithNetclawActors(); + } + + private IActorRef BuildGateway( + string gatewayName, + SlackChannelOptions? options = null, + IContentScanner? scanner = null, + ChannelAttachmentPolicy? publicOverride = null) + { + var pipeline = Host.Services.GetRequiredService(); + var httpClient = new HttpClient(_httpHandler); + var profiles = ToolAudienceProfileDefaults.CreateProfiles(); + if (publicOverride is not null) + profiles.Public.ChannelAttachments = publicOverride; + + var deps = new SlackGatewayDependencies( + Pipeline: pipeline, + IngressGate: null, + ActorSystem: Sys, + TimeProvider: TimeProvider.System, + Options: options ?? new SlackChannelOptions + { + Enabled = true, + MentionOnly = false, + AllowDirectMessages = true, + BotToken = new SensitiveString("xoxb-fake-token") + }, + BotUserId: new SlackUserId("UBOT"), + DefaultChannelId: null, + ReplyClient: _replyClient, + ContentScanner: scanner ?? new NullContentScanner(), + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: profiles, + ModelCapabilities: Host.Services.GetRequiredService(), + Paths: _paths, + HttpClient: httpClient); + + return Sys.ActorOf(SlackGatewayActor.CreateProps(deps), gatewayName); + } + + [Fact] + public async Task Pdf_in_dm_on_vision_capable_model_is_saved_to_inbox_and_inlined() + { + _httpHandler.RespondWith("application/pdf", FakePdfBytes); + var gateway = BuildGateway("slack-gw-pdf-inline"); + + var files = new List + { + new("F123", "report.pdf", "application/pdf", FakePdfBytes.Length, + "https://files.slack.com/files-pri/T1234-F123/report.pdf") + }; + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.Message, + EventId: new SlackEventId("D1:2000"), + ChannelId: new SlackChannelId("D1"), + ThreadTs: null, + EventTs: new SlackEventTs("2000.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "please summarize", + Subtype: null, + Hidden: false, + IsDirectMessage: true, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_chatClient.ReceivedMessages, + contents => contents.Any(c => c is TextContent t && t.Text.Contains("[attachment]", StringComparison.Ordinal)) + && contents.Any(c => c is DataContent d && d.MediaType == "application/pdf")); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + var announcement = _chatClient.ReceivedMessages + .SelectMany(m => m) + .OfType() + .First(t => t.Text.Contains("[attachment]", StringComparison.Ordinal)); + Assert.Contains("report.pdf", announcement.Text, StringComparison.Ordinal); + Assert.Contains("inlined=\"true\"", announcement.Text, StringComparison.Ordinal); + Assert.Contains("path=\"inbox/report.pdf\"", announcement.Text, StringComparison.Ordinal); + + var sessionId = new SessionId("D1/2000.1"); + var inboxPath = Path.Combine( + SessionDirectoryHelper.GetOrCreateInboxDirectory(sessionId, _paths.SessionsDirectory), + "report.pdf"); + Assert.True(File.Exists(inboxPath), $"Expected inbox file at {inboxPath}"); + } + + [Fact] + public async Task Docx_in_dm_is_path_only_with_format_not_inlineable_note() + { + _httpHandler.RespondWith( + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + FakeDocxBytes); + var gateway = BuildGateway("slack-gw-docx-path-only"); + + var files = new List + { + new("F888", "notes.docx", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + FakeDocxBytes.Length, + "https://files.slack.com/files-pri/T1234-F888/notes.docx") + }; + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.Message, + EventId: new SlackEventId("D2:2100"), + ChannelId: new SlackChannelId("D2"), + ThreadTs: null, + EventTs: new SlackEventTs("2100.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "please read this", + Subtype: null, + Hidden: false, + IsDirectMessage: true, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_chatClient.ReceivedMessages, + contents => contents.Any(c => c is TextContent t + && t.Text.Contains("[attachment]", StringComparison.Ordinal) + && t.Text.Contains("notes.docx", StringComparison.Ordinal))); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + var announcement = _chatClient.ReceivedMessages + .SelectMany(m => m) + .OfType() + .First(t => t.Text.Contains("[attachment]", StringComparison.Ordinal) + && t.Text.Contains("notes.docx", StringComparison.Ordinal)); + Assert.Contains("inlined=\"false\"", announcement.Text, StringComparison.Ordinal); + Assert.Contains("format not inlineable", announcement.Text, StringComparison.Ordinal); + + // Docx is not inlineable — no DataContent for it should have been + // forwarded to the LLM. + var docxDataContents = _chatClient.ReceivedMessages + .SelectMany(m => m) + .OfType() + .Where(d => d.MediaType?.Contains("wordprocessingml", StringComparison.Ordinal) == true); + Assert.Empty(docxDataContents); + } + + [Fact] + public async Task Docx_in_public_channel_is_rejected_pre_download() + { + // Force the channel audience to Public via explicit ChannelAudiences + // mapping — the default audience heuristic would promote an + // allowlisted channel to Team. + var gateway = BuildGateway( + "slack-gw-public-docx-reject", + options: new SlackChannelOptions + { + Enabled = true, + MentionOnly = true, + AllowedChannelIds = ["C_PUB"], + ChannelAudiences = new Dictionary(StringComparer.Ordinal) + { + ["C_PUB"] = "public" + }, + BotToken = new SensitiveString("xoxb-fake-token") + }); + + var files = new List + { + new("F999", "secret.docx", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + FakeDocxBytes.Length, + "https://files.slack.com/files-pri/T1234-F999/secret.docx") + }; + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.AppMention, + EventId: new SlackEventId("C_PUB:3000"), + ChannelId: new SlackChannelId("C_PUB"), + ThreadTs: new SlackThreadTs("3000.0"), + EventTs: new SlackEventTs("3000.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "<@UBOT> check this", + Subtype: null, + Hidden: false, + IsDirectMessage: false, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_replyClient.PostedMessages, + m => m.Text.Contains("secret.docx", StringComparison.Ordinal)); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + // No HTTP request was made — the file was rejected before download. + Assert.Equal(0, _httpHandler.RequestCount); + } + + [Fact] + public async Task Oversize_file_is_rejected_pre_download() + { + var gateway = BuildGateway("slack-gw-oversize-reject"); + const long oversizeBytes = 30L * 1024 * 1024; // 30 MiB, above 25 MiB default + + var files = new List + { + new("FBIG", "huge.pdf", "application/pdf", oversizeBytes, + "https://files.slack.com/files-pri/T1234-FBIG/huge.pdf") + }; + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.Message, + EventId: new SlackEventId("D3:3100"), + ChannelId: new SlackChannelId("D3"), + ThreadTs: null, + EventTs: new SlackEventTs("3100.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "here", + Subtype: null, + Hidden: false, + IsDirectMessage: true, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_replyClient.PostedMessages, + m => m.Text.Contains("huge.pdf", StringComparison.Ordinal) + && m.Text.Contains("25", StringComparison.Ordinal)); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(0, _httpHandler.RequestCount); + } + + [Fact] + public async Task Too_many_attachments_rejects_entire_batch_but_forwards_text() + { + var gateway = BuildGateway("slack-gw-too-many"); + + // Default MaxFilesPerMessage is 10; send 15. + var files = Enumerable.Range(1, 15) + .Select(i => new SlackFileReference( + $"F{i}", $"img{i}.png", "image/png", FakePngBytes.Length, + $"https://files.slack.com/files-pri/T1234-F{i}/img{i}.png")) + .ToList(); + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.Message, + EventId: new SlackEventId("D4:3200"), + ChannelId: new SlackChannelId("D4"), + ThreadTs: null, + EventTs: new SlackEventTs("3200.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "batch upload", + Subtype: null, + Hidden: false, + IsDirectMessage: true, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_replyClient.PostedMessages, + m => m.Text.Contains("10 attachments per message", StringComparison.Ordinal)); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(0, _httpHandler.RequestCount); + + // Text content "batch upload" should still have reached the LLM. + await AwaitAssertAsync(() => + { + Assert.Contains(_chatClient.ReceivedMessages, + contents => contents.Any(c => c is TextContent t + && t.Text.Contains("batch upload", StringComparison.Ordinal))); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + } + + [Fact] + public async Task Filename_collision_across_turns_produces_suffixed_path() + { + // Two messages in the same Slack thread (same ThreadTs) route to + // the same SessionId ("{channelId}/{threadTs}") and therefore the + // same inbox directory. Uploading the same filename twice must + // land the second copy at photo_1.png without overwriting the + // first. + _httpHandler.RespondWith("image/png", FakePngBytes); + var gateway = BuildGateway( + "slack-gw-collision", + options: new SlackChannelOptions + { + Enabled = true, + MentionOnly = true, + AllowedChannelIds = ["D5"], + BotToken = new SensitiveString("xoxb-fake-token") + }); + + var threadTs = new SlackThreadTs("3300.0"); + var sessionId = new SessionId("D5/3300.0"); + var inboxDir = SessionDirectoryHelper.GetOrCreateInboxDirectory(sessionId, _paths.SessionsDirectory); + + var file = new SlackFileReference( + "F_COLLIDE", "photo.png", "image/png", FakePngBytes.Length, + "https://files.slack.com/files-pri/T1234-F_COLLIDE/photo.png"); + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.AppMention, + EventId: new SlackEventId("D5:3300.1"), + ChannelId: new SlackChannelId("D5"), + ThreadTs: threadTs, + EventTs: new SlackEventTs("3300.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "<@UBOT> first", + Subtype: null, + Hidden: false, + IsDirectMessage: false, + Files: [file])); + + await AwaitAssertAsync(() => + { + Assert.True(File.Exists(Path.Combine(inboxDir, "photo.png"))); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + // Same thread, same filename — should land at photo_1.png without + // overwriting the first file. + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.AppMention, + EventId: new SlackEventId("D5:3300.2"), + ChannelId: new SlackChannelId("D5"), + ThreadTs: threadTs, + EventTs: new SlackEventTs("3300.2"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "<@UBOT> second", + Subtype: null, + Hidden: false, + IsDirectMessage: false, + Files: [file with { Id = "F_COLLIDE_2" }])); + + await AwaitAssertAsync(() => + { + Assert.True(File.Exists(Path.Combine(inboxDir, "photo_1.png"))); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + } + + [Fact] + public async Task Scanner_rejection_surfaces_user_visible_reply_with_no_inbox_write() + { + _httpHandler.RespondWith("image/png", FakePngBytes); + var gateway = BuildGateway( + "slack-gw-scan-reject", + scanner: new AlwaysBlockContentScanner("malware detected")); + + var files = new List + { + new("F_BAD", "bad.png", "image/png", FakePngBytes.Length, + "https://files.slack.com/files-pri/T1234-F_BAD/bad.png") + }; + + gateway.Tell(new SlackInboundMessage( + Kind: SlackInboundKind.Message, + EventId: new SlackEventId("D6:3400"), + ChannelId: new SlackChannelId("D6"), + ThreadTs: null, + EventTs: new SlackEventTs("3400.1"), + UserId: new SlackUserId("U_HUMAN"), + BotId: null, + Text: "nasty", + Subtype: null, + Hidden: false, + IsDirectMessage: true, + Files: files)); + + await AwaitAssertAsync(() => + { + Assert.Contains(_replyClient.PostedMessages, + m => m.Text.Contains("bad.png", StringComparison.Ordinal) + && m.Text.Contains("malware", StringComparison.Ordinal)); + }, duration: TimeSpan.FromSeconds(10), cancellationToken: TestContext.Current.CancellationToken); + + var sessionId = new SessionId("D6/3400.1"); + var inboxDir = SessionDirectoryHelper.GetOrCreateInboxDirectory(sessionId, _paths.SessionsDirectory); + Assert.Empty(Directory.GetFiles(inboxDir)); + } + + // ── Test doubles ────────────────────────────────────────────────────── + + private sealed class ConfigurableFakeSlackFileHandler : DelegatingHandler + { + private int _requestCount; + private string _contentType = "image/png"; + private byte[] _bytes = FakePngBytes; + + public int RequestCount => _requestCount; + + public void RespondWith(string contentType, byte[] bytes) + { + _contentType = contentType; + _bytes = bytes; + } + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + Interlocked.Increment(ref _requestCount); + var response = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new ByteArrayContent(_bytes) + }; + response.Content.Headers.ContentType = new MediaTypeHeaderValue(_contentType); + return Task.FromResult(response); + } + } + + private sealed class RecordingReplyClient : ISlackReplyClient + { + public List PostedMessages { get; } = []; + + public Task PostThreadReplyAsync(SlackPostMessage message, CancellationToken cancellationToken = default) + { + PostedMessages.Add(message); + return Task.CompletedTask; + } + + public Task PostThreadReplyWithTsAsync(SlackPostMessage message, CancellationToken cancellationToken = default) + { + PostedMessages.Add(message); + return Task.FromResult("fake.ts"); + } + + public Task UpdateThreadMessageAsync( + SlackChannelId channelId, + string messageTs, + string text, + IReadOnlyList? blocks = null, + CancellationToken cancellationToken = default) => Task.CompletedTask; + + public Task UploadFileToThreadAsync( + SlackChannelId channelId, + SlackThreadTs threadTs, + string filePath, + string? fileName = null, + CancellationToken cancellationToken = default) => Task.CompletedTask; + } + + private sealed class RecordingChatClient : IChatClient + { + private readonly object _gate = new(); + private readonly List> _messages = new(); + + public IReadOnlyList> ReceivedMessages + { + get + { + lock (_gate) + return _messages.ToList(); + } + } + + public Task GetResponseAsync( + IEnumerable messages, + ChatOptions? options = null, + CancellationToken cancellationToken = default) + { + lock (_gate) + { + foreach (var msg in messages) + { + if (msg.Role == Microsoft.Extensions.AI.ChatRole.User) + _messages.Add(msg.Contents); + } + } + + return Task.FromResult(new ChatResponse([ + new ChatMessage(Microsoft.Extensions.AI.ChatRole.Assistant, "ok") + ])); + } + + public async IAsyncEnumerable GetStreamingResponseAsync( + IEnumerable messages, + ChatOptions? options = null, + [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default) + { + await GetResponseAsync(messages, options, cancellationToken); + yield return new ChatResponseUpdate(Microsoft.Extensions.AI.ChatRole.Assistant, "ok"); + } + + public object? GetService(Type serviceType, object? serviceKey = null) => null; + + public void Dispose() { } + } + + private sealed class AlwaysBlockContentScanner(string message) : IContentScanner + { + public Task ScanAsync( + ReadOnlyMemory bytes, + string fileName, + string mimeType, + CancellationToken cancellationToken = default) + { + return Task.FromResult(ContentScanResult.Rejected( + ContentScanError.AntivirusDetection, + message)); + } + } + +} diff --git a/src/Netclaw.Actors.Tests/Channels/SlackFileFlowIntegrationTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackFileFlowIntegrationTests.cs index be0d0f00e..b49cb705c 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackFileFlowIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackFileFlowIntegrationTests.cs @@ -80,7 +80,7 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, @@ -121,6 +121,9 @@ public async Task Inbound_image_file_is_downloaded_and_persisted_to_session_medi ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-file-test"); @@ -201,6 +204,9 @@ public async Task App_mention_with_file_only_is_downloaded_and_persisted() ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-mention-test"); @@ -270,6 +276,9 @@ public async Task File_share_subtype_with_text_flows_through_full_pipeline() ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-fileshare-test"); @@ -335,6 +344,9 @@ public async Task High_risk_prompt_injection_message_is_blocked_before_session_e ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, PromptInjectionDetector: new AlwaysHighRiskPromptInjectionDetector()); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-prompt-block-test"); @@ -384,7 +396,10 @@ public async Task Failed_turn_posts_single_error_without_generic_fallback() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-error-turn-test"); @@ -434,7 +449,10 @@ public async Task Timed_out_slack_post_does_not_block_later_turns() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-post-timeout-test"); @@ -506,7 +524,10 @@ public async Task Retryable_slack_content_rejection_is_fed_back_to_session_for_c DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-delivery-feedback-test"); @@ -577,7 +598,10 @@ public async Task Retryable_slack_file_upload_rejection_is_fed_back_to_session() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/8000.1"), @@ -633,7 +657,10 @@ public async Task Timeout_during_post_sends_transport_failure_feedback_to_sessio DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9000.1"), @@ -693,7 +720,10 @@ public async Task Text_approval_reply_routes_tool_interaction_response() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9050.1"), @@ -780,7 +810,10 @@ public async Task Approval_request_posts_block_buttons_with_text_fallback() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9055.1"), @@ -848,7 +881,10 @@ public async Task Button_approval_reply_routes_tool_interaction_response() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9060.1"), @@ -925,7 +961,10 @@ public async Task Generic_exception_during_post_sends_unknown_failure_feedback_t DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9100.1"), @@ -984,7 +1023,10 @@ public async Task Content_rejection_msg_too_long_sends_message_too_large_feedbac DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9200.1"), @@ -1044,7 +1086,10 @@ public async Task Content_rejection_invalid_blocks_sends_content_rejected_feedba DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), - ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance); + ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var actor = Sys.ActorOf(SlackThreadBindingActor.CreateProps( new SessionId("D7/9300.1"), @@ -1091,6 +1136,9 @@ public async Task Inbound_image_with_real_scanner_flows_to_llm() ReplyClient: _replyClient, ContentScanner: new MagicByteContentScanner(new ContentPolicy()), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-real-scanner-test"); @@ -1150,6 +1198,9 @@ public async Task Scanner_failure_does_not_silently_drop_image() ReplyClient: _replyClient, ContentScanner: new FailingContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-failing-scanner-test"); diff --git a/src/Netclaw.Actors.Tests/Channels/SlackProactiveThreadTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackProactiveThreadTests.cs index 2d4b49f96..48a261aa7 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackProactiveThreadTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackProactiveThreadTests.cs @@ -585,6 +585,9 @@ private static SlackGatewayDependencies CreateDependencies( ReplyClient: new NoopReplyClient(), ContentScanner: new NullContentScanner(), ThreadHistoryFetcher: EmptyThreadHistoryFetcher.Instance, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: TestSlackGatewayDeps.NewTestPaths(), ConversationPropsFactory: conversationPropsFactory, ThreadPropsFactory: threadPropsFactory); } diff --git a/src/Netclaw.Actors.Tests/Channels/SlackThreadBackfillIntegrationTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackThreadBackfillIntegrationTests.cs index 9d1f45f36..288799792 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackThreadBackfillIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackThreadBackfillIntegrationTests.cs @@ -74,7 +74,7 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, @@ -123,7 +123,10 @@ public async Task Backfill_messages_are_merged_into_single_user_turn_and_exclude ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), HttpClient: httpClient, - ThreadHistoryFetcher: fetcher); + ThreadHistoryFetcher: fetcher, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-backfill"); @@ -203,7 +206,10 @@ public async Task Backfill_runs_once_per_runtime_and_runs_again_after_restart() ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), HttpClient: httpClient, - ThreadHistoryFetcher: countingFetcher); + ThreadHistoryFetcher: countingFetcher, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-recovery"); @@ -331,9 +337,12 @@ public async Task High_risk_backfill_messages_are_dropped_before_turn_assembly() DefaultChannelId: null, ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), + ThreadHistoryFetcher: fetcher, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths, HttpClient: httpClient, - PromptInjectionDetector: new ContainsIgnorePromptInjectionDetector(), - ThreadHistoryFetcher: fetcher); + PromptInjectionDetector: new ContainsIgnorePromptInjectionDetector()); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-risk-backfill"); @@ -394,7 +403,10 @@ public async Task Older_out_of_order_live_event_is_dropped_after_cursor_advances ReplyClient: _replyClient, ContentScanner: new NullContentScanner(), HttpClient: httpClient, - ThreadHistoryFetcher: fetcher); + ThreadHistoryFetcher: fetcher, + AudienceProfiles: TestSlackGatewayDeps.DefaultAudienceProfiles, + ModelCapabilities: TestSlackGatewayDeps.DefaultVisionCapableModel, + Paths: _paths); var gateway = Sys.ActorOf(SlackGatewayActor.CreateProps(deps), "slack-gw-stale-ordering"); diff --git a/src/Netclaw.Actors.Tests/Channels/TestSlackGatewayDeps.cs b/src/Netclaw.Actors.Tests/Channels/TestSlackGatewayDeps.cs new file mode 100644 index 000000000..ce45d298a --- /dev/null +++ b/src/Netclaw.Actors.Tests/Channels/TestSlackGatewayDeps.cs @@ -0,0 +1,41 @@ +using Netclaw.Configuration; + +namespace Netclaw.Actors.Tests.Channels; + +/// +/// Test defaults for fields on +/// that Slack test fixtures don't typically exercise directly but are now +/// required by the cross-channel attachment ingress pipeline. Each field +/// is a plain default that tests can override if they need specific +/// behavior (e.g. a text-only model for modality-gap tests). +/// +internal static class TestSlackGatewayDeps +{ + public static ToolAudienceProfiles DefaultAudienceProfiles + => ToolAudienceProfileDefaults.CreateProfiles(); + + public static ModelCapabilities DefaultVisionCapableModel + => new() + { + ModelId = "test-vision-model", + ContextWindowTokens = 128_000, + InputModalities = ModelModality.Text | ModelModality.Image, + OutputModalities = ModelModality.Text + }; + + public static ModelCapabilities DefaultTextOnlyModel + => new() + { + ModelId = "test-text-only-model", + ContextWindowTokens = 128_000, + InputModalities = ModelModality.Text, + OutputModalities = ModelModality.Text + }; + + public static NetclawPaths NewTestPaths() + { + var path = new NetclawPaths(Path.Combine(Path.GetTempPath(), $"netclaw-slack-test-{Guid.NewGuid():N}")); + path.EnsureDirectoriesExist(); + return path; + } +} diff --git a/src/Netclaw.Actors.Tests/Protocol/InboxWriterTests.cs b/src/Netclaw.Actors.Tests/Protocol/InboxWriterTests.cs new file mode 100644 index 000000000..7a9cb4ed8 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Protocol/InboxWriterTests.cs @@ -0,0 +1,133 @@ +using System.Text; +using Netclaw.Actors.Protocol; +using Xunit; + +namespace Netclaw.Actors.Tests.Protocol; + +public sealed class InboxWriterTests : IDisposable +{ + private readonly string _inboxDir; + + public InboxWriterTests() + { + _inboxDir = Path.Combine( + Path.GetTempPath(), + $"netclaw-inbox-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(_inboxDir); + } + + public void Dispose() + { + try + { + if (Directory.Exists(_inboxDir)) + Directory.Delete(_inboxDir, recursive: true); + } + catch + { + // best-effort cleanup + } + } + + [Fact] + public void ReserveUniquePath_returns_plain_name_when_no_collision() + { + var path = InboxWriter.ReserveUniquePath(_inboxDir, "report.pdf"); + + Assert.Equal(Path.Combine(_inboxDir, "report.pdf"), path); + } + + [Fact] + public void ReserveUniquePath_suffixes_when_file_exists() + { + File.WriteAllText(Path.Combine(_inboxDir, "report.pdf"), "existing"); + + var path = InboxWriter.ReserveUniquePath(_inboxDir, "report.pdf"); + + Assert.Equal(Path.Combine(_inboxDir, "report_1.pdf"), path); + } + + [Fact] + public void ReserveUniquePath_chains_suffixes_across_multiple_collisions() + { + File.WriteAllText(Path.Combine(_inboxDir, "report.pdf"), "first"); + File.WriteAllText(Path.Combine(_inboxDir, "report_1.pdf"), "second"); + File.WriteAllText(Path.Combine(_inboxDir, "report_2.pdf"), "third"); + + var path = InboxWriter.ReserveUniquePath(_inboxDir, "report.pdf"); + + Assert.Equal(Path.Combine(_inboxDir, "report_3.pdf"), path); + } + + [Fact] + public void ReserveUniquePath_handles_extensionless_names() + { + File.WriteAllText(Path.Combine(_inboxDir, "README"), "first"); + + var path = InboxWriter.ReserveUniquePath(_inboxDir, "README"); + + Assert.Equal(Path.Combine(_inboxDir, "README_1"), path); + } + + [Fact] + public void ReserveUniquePath_throws_on_exhaustion() + { + File.WriteAllText(Path.Combine(_inboxDir, "report.pdf"), "base"); + for (var i = 1; i <= InboxWriter.MaxCollisionSuffix; i++) + File.WriteAllText(Path.Combine(_inboxDir, $"report_{i}.pdf"), "collide"); + + Assert.Throws( + () => InboxWriter.ReserveUniquePath(_inboxDir, "report.pdf")); + } + + [Fact] + public async Task WriteAtomicAsync_writes_expected_bytes() + { + var target = Path.Combine(_inboxDir, "hello.txt"); + var payload = "hello world"u8.ToArray(); + + await InboxWriter.WriteAtomicAsync(target, payload, TestContext.Current.CancellationToken); + + Assert.True(File.Exists(target)); + Assert.Equal(payload, await File.ReadAllBytesAsync(target, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task WriteAtomicAsync_leaves_no_temp_file_on_success() + { + var target = Path.Combine(_inboxDir, "hello.txt"); + await InboxWriter.WriteAtomicAsync(target, "hi"u8.ToArray(), TestContext.Current.CancellationToken); + + var stragglers = Directory.GetFiles(_inboxDir).Where(f => Path.GetFileName(f).Contains(".tmp")); + Assert.Empty(stragglers); + } + + [Fact] + public async Task SanitizeReserveAndWrite_strips_path_components_and_traversal() + { + var written = await InboxWriter.SanitizeReserveAndWriteAsync( + _inboxDir, + "../../../etc/passwd", + "malicious"u8.ToArray(), + TestContext.Current.CancellationToken); + + var directory = Path.GetDirectoryName(written)!; + Assert.Equal(_inboxDir, directory); + Assert.DoesNotContain("..", Path.GetFileName(written), StringComparison.Ordinal); + Assert.False(File.Exists(Path.Combine("/", "etc", "passwd_suffix_that_should_not_exist"))); + } + + [Fact] + public async Task SanitizeReserveAndWrite_round_trips_bytes() + { + var payload = Encoding.UTF8.GetBytes("roundtrip content"); + + var written = await InboxWriter.SanitizeReserveAndWriteAsync( + _inboxDir, + "note.txt", + payload, + TestContext.Current.CancellationToken); + + Assert.Equal(payload, await File.ReadAllBytesAsync(written, TestContext.Current.CancellationToken)); + } +} diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index 12232618b..174b7c7cf 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -48,7 +48,8 @@ protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IService var pipeline = new SessionPipeline( system, - new RequiredActor(ActorRegistry.For(system))); + new RequiredActor(ActorRegistry.For(system)), + new NetclawPaths(Path.Combine(Path.GetTempPath(), $"netclaw-test-{Guid.NewGuid():N}"))); var defaults = new EffectivePolicyDefaults( DeploymentPosture.Team, TrustAudience.Team, ShellExecutionMode.Off, false); diff --git a/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs b/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs new file mode 100644 index 000000000..7536e9988 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs @@ -0,0 +1,53 @@ +using Netclaw.Actors.Channels; +using Netclaw.Actors.Sessions; +using Xunit; + +namespace Netclaw.Actors.Tests.Sessions; + +/// +/// Canonical shape assertions for . +/// The hint is injected into the system prompt of every file_read-granted +/// session and is the agent's only documentation of how to interpret +/// [attachment] announcement lines. Eval regressions usually trace +/// back to drift in this string, so it's pinned here as a bear-trap test. +/// +public sealed class AttachmentContextHintTests +{ + [Fact] + public void Hint_names_the_inbox_subdirectory() + { + Assert.Contains("inbox/", LlmSessionActor.AttachmentContextHint, System.StringComparison.Ordinal); + } + + [Fact] + public void Hint_documents_the_inlined_field_and_both_values() + { + Assert.Contains("inlined=\"true|false\"", LlmSessionActor.AttachmentContextHint, System.StringComparison.Ordinal); + } + + [Fact] + public void Hint_explains_the_model_missing_note_class() + { + Assert.Contains("current model has no", LlmSessionActor.AttachmentContextHint, System.StringComparison.Ordinal); + } + + [Fact] + public void Hint_explains_the_format_not_inlineable_note_class() + { + Assert.Contains("format not inlineable", LlmSessionActor.AttachmentContextHint, System.StringComparison.Ordinal); + } + + [Fact] + public void Hint_forbids_silently_ignoring_attachments() + { + Assert.Contains("Never silently ignore", LlmSessionActor.AttachmentContextHint, System.StringComparison.Ordinal); + } + + [Fact] + public void Hint_note_prefixes_match_AttachmentNotes_constants() + { + Assert.StartsWith("current model has no image modality", AttachmentNotes.ModelMissingImage, System.StringComparison.Ordinal); + Assert.StartsWith("current model has no native PDF support", AttachmentNotes.ModelMissingPdf, System.StringComparison.Ordinal); + Assert.StartsWith("format not inlineable", AttachmentNotes.FormatNotInlineable, System.StringComparison.Ordinal); + } +} diff --git a/src/Netclaw.Actors.Tests/Sessions/CompactionIntegrationTests.cs b/src/Netclaw.Actors.Tests/Sessions/CompactionIntegrationTests.cs index 2f6c924ec..12a924263 100644 --- a/src/Netclaw.Actors.Tests/Sessions/CompactionIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/CompactionIntegrationTests.cs @@ -68,13 +68,15 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo "file_read"); services.AddSingleton(registry); + services.AddTestNetclawPaths(); + // Composite records for LlmSessionActor constructor services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/Sessions/ErrorCorrelationTests.cs b/src/Netclaw.Actors.Tests/Sessions/ErrorCorrelationTests.cs index 82603dfa5..f9023e31c 100644 --- a/src/Netclaw.Actors.Tests/Sessions/ErrorCorrelationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/ErrorCorrelationTests.cs @@ -47,13 +47,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(new StaticSystemPromptProvider("You are a test assistant.")); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, diff --git a/src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs b/src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs index 22d3b5bbb..e2f25a5c5 100644 --- a/src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/LlmSessionIntegrationTests.cs @@ -80,13 +80,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(_lifecycleObserver); services.AddSingleton(new UnusedSessionPipeline()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTwoPhaseTimeoutTests.cs b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTwoPhaseTimeoutTests.cs index b20014251..9ee145e4c 100644 --- a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTwoPhaseTimeoutTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTwoPhaseTimeoutTests.cs @@ -43,12 +43,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(new StaticSystemPromptProvider("You are a test assistant.")); services.AddSingleton(new FakeCapabilityResolver()); + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, diff --git a/src/Netclaw.Actors.Tests/Sessions/LlmSessionWatchdogTests.cs b/src/Netclaw.Actors.Tests/Sessions/LlmSessionWatchdogTests.cs index bcc34fcc3..0da72c584 100644 --- a/src/Netclaw.Actors.Tests/Sessions/LlmSessionWatchdogTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/LlmSessionWatchdogTests.cs @@ -42,13 +42,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(new StaticSystemPromptProvider("You are a test assistant.")); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, diff --git a/src/Netclaw.Actors.Tests/Sessions/MaxToolIterationTests.cs b/src/Netclaw.Actors.Tests/Sessions/MaxToolIterationTests.cs index 3a7b908fb..f85cbb111 100644 --- a/src/Netclaw.Actors.Tests/Sessions/MaxToolIterationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/MaxToolIterationTests.cs @@ -58,13 +58,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(registry); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/Sessions/ModalityGateTests.cs b/src/Netclaw.Actors.Tests/Sessions/ModalityGateTests.cs index fe9237172..7fb58df6b 100644 --- a/src/Netclaw.Actors.Tests/Sessions/ModalityGateTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/ModalityGateTests.cs @@ -44,13 +44,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo "You are a test assistant.")); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, @@ -68,8 +68,14 @@ protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IService } [Fact] - public async Task Image_with_text_strips_images_and_sends_text_to_llm() + public async Task Image_with_text_on_text_only_model_surfaces_ingress_bug_and_still_calls_llm() { + // The strict-consumer contract treats an unsupported-modality media + // ref reaching the session actor as an ingress bug. The session still + // completes the turn (so the user gets a reply) but the offending refs + // are dropped and a [system] notice about the ingress bug is appended + // to the user message before it goes to the model. No legacy + // "[Images removed]" placeholder is emitted. var sessionId = new SessionId("test-channel/modality-text-only"); var sessionManager = ActorRegistry.Get(); var subscriber = CreateTestProbe("modality-sub"); @@ -80,7 +86,7 @@ await sessionManager.Ask(new JoinSession Subscriber = subscriber, Filter = OutputFilter.Full }, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); - await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); // Drain subscriber notification + await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); await sessionManager.Ask(new SendUserMessage { @@ -97,23 +103,29 @@ await sessionManager.Ask(new SendUserMessage } }, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); - // Should receive the "images removed" acknowledgement - var ack = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - Assert.Contains("Images removed", ack.Text); - - // The LLM should still be called with the text content + // The first output is the LLM response itself — there is no longer a + // separate "[Images removed]" TextOutput before the reply. var textOutput = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); Assert.Contains("fake", textOutput.Text, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("Images removed", textOutput.Text); await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - // LLM was called (text was sent through despite images being stripped) + // LLM was called and saw the ingress-bug notice appended to the user text. Assert.Equal(1, _fakeChatClient.CallCount); + Assert.NotEmpty(_fakeChatClient.ReceivedMessages); + var lastRequest = _fakeChatClient.ReceivedMessages[^1]; + var concatenated = string.Join("\n", lastRequest.Select(m => m.Text ?? string.Empty)); + Assert.Contains("ingress bug", concatenated, StringComparison.OrdinalIgnoreCase); } [Fact] - public async Task Image_only_message_skips_llm_call_entirely() + public async Task Image_only_message_on_text_only_model_still_calls_llm_with_ingress_bug_notice() { + // Empty text body + only unsupported media. The strict-consumer + // contract appends the [system] ingress bug notice to the user + // content so the LLM has something to respond to. We'd rather the + // user get a reply explaining the situation than silence. var sessionId = new SessionId("test-channel/modality-image-only"); var sessionManager = ActorRegistry.Get(); var subscriber = CreateTestProbe("modality-image-only-sub"); @@ -124,12 +136,12 @@ await sessionManager.Ask(new JoinSession Subscriber = subscriber, Filter = OutputFilter.Full }, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); - await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); // Drain subscriber notification + await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); await sessionManager.Ask(new SendUserMessage { SessionId = sessionId, - Content = "", // no text content + Content = "", MediaReferences = { new SerializableMediaReference @@ -141,19 +153,18 @@ await sessionManager.Ask(new SendUserMessage } }, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); - // Should receive the "images removed" acknowledgement first - var stripped = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - Assert.Contains("Images removed", stripped.Text); + var reply = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); + Assert.DoesNotContain("Images removed", reply.Text); - // Then the "only images" explanation - var explanation = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - Assert.Contains("only images", explanation.Text, StringComparison.OrdinalIgnoreCase); - - var tc = await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - Assert.Equal(TurnOutcome.Skipped, tc.Outcome); + await subscriber.ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); - // LLM was NOT called - Assert.Equal(0, _fakeChatClient.CallCount); + // LLM was called once, and the user-visible content we sent it + // included the ingress-bug notice (not a legacy placeholder). + Assert.Equal(1, _fakeChatClient.CallCount); + Assert.NotEmpty(_fakeChatClient.ReceivedMessages); + var lastRequest = _fakeChatClient.ReceivedMessages[^1]; + var concatenated = string.Join("\n", lastRequest.Select(m => m.Text ?? string.Empty)); + Assert.Contains("ingress bug", concatenated, StringComparison.OrdinalIgnoreCase); } } @@ -186,13 +197,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo "You are a test assistant.")); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionMemoryServices( sp.GetService() ?? NullMemoryExtractor.Instance, sp.GetService() ?? NullMemoryRecallCoordinator.Instance, diff --git a/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs b/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs index 020c2f64e..430fa8205 100644 --- a/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs @@ -84,13 +84,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance)); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/Sessions/ToolExecutionIntegrationTests.cs b/src/Netclaw.Actors.Tests/Sessions/ToolExecutionIntegrationTests.cs index 7527d2969..8b7863880 100644 --- a/src/Netclaw.Actors.Tests/Sessions/ToolExecutionIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/ToolExecutionIntegrationTests.cs @@ -62,13 +62,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(registry); services.AddSingleton(new FakeCapabilityResolver()); - // Composite records for LlmSessionActor constructor + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/Sessions/ToolLoopCompactionTests.cs b/src/Netclaw.Actors.Tests/Sessions/ToolLoopCompactionTests.cs index 9f58d0104..c900c91e2 100644 --- a/src/Netclaw.Actors.Tests/Sessions/ToolLoopCompactionTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/ToolLoopCompactionTests.cs @@ -62,12 +62,13 @@ protected override void ConfigureServices(HostBuilderContext context, IServiceCo services.AddSingleton(registry); services.AddSingleton(new FakeCapabilityResolver()); + services.AddTestNetclawPaths(); services.AddSingleton(sp => new SessionServices( sp.GetRequiredService(), sp.GetRequiredService(), sp.GetService>() ?? Array.Empty(), sp.GetService() ?? TimeProvider.System, - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(), sp.GetService(), diff --git a/src/Netclaw.Actors.Tests/TestNetclawPaths.cs b/src/Netclaw.Actors.Tests/TestNetclawPaths.cs new file mode 100644 index 000000000..c507dff2a --- /dev/null +++ b/src/Netclaw.Actors.Tests/TestNetclawPaths.cs @@ -0,0 +1,20 @@ +using Microsoft.Extensions.DependencyInjection; +using Netclaw.Configuration; + +namespace Netclaw.Actors.Tests; + +/// +/// Test helper that registers a singleton rooted at +/// a unique temp directory. Used by Akka.Hosting.TestKit fixtures that need +/// SessionServices to construct but do not exercise real filesystem +/// behavior. The directory is not cleaned up automatically; tests relying on +/// durability should manage their own temp dirs. +/// +internal static class TestNetclawPaths +{ + public static IServiceCollection AddTestNetclawPaths(this IServiceCollection services) + { + var basePath = Path.Combine(Path.GetTempPath(), $"netclaw-test-{Guid.NewGuid():N}"); + return services.AddSingleton(new NetclawPaths(basePath)); + } +} diff --git a/src/Netclaw.Actors/Channels/AttachmentNotes.cs b/src/Netclaw.Actors/Channels/AttachmentNotes.cs new file mode 100644 index 000000000..6627fafc4 --- /dev/null +++ b/src/Netclaw.Actors/Channels/AttachmentNotes.cs @@ -0,0 +1,37 @@ +namespace Netclaw.Actors.Channels; + +/// +/// Canonical strings for the note field on [attachment] +/// announcements emitted by channel ingress adapters. These are normative +/// per netclaw-input-adapters so the agent's dynamic-context hint +/// and eval harness can branch on stable textual prefixes rather than +/// ad-hoc phrasing. Channel adapters SHALL source note text exclusively +/// from this class — never open-code a variant. +/// +public static class AttachmentNotes +{ + /// + /// Model-modality gap note for an image attachment on a model that + /// does not report ModelModality.Image as an input modality. + /// MUST begin with "current model has no image modality" per + /// the spec so the agent's dynamic-context hint can detect this class. + /// + public const string ModelMissingImage = + "current model has no image modality; file is on disk but not viewable this turn"; + + /// + /// Model-modality gap note for a PDF attachment on a model that does + /// not natively accept application/pdf as input. MUST begin + /// with "current model has no native PDF support" per the spec. + /// + public const string ModelMissingPdf = + "current model has no native PDF support; use shell_execute (e.g., pdftotext) to extract text"; + + /// + /// Format-not-inlineable note for categories that no model can render + /// natively (documents, archives, video, audio, unknown binaries). + /// MUST begin with "format not inlineable" per the spec. + /// + public const string FormatNotInlineable = + "format not inlineable; use file_read or shell_execute to process"; +} diff --git a/src/Netclaw.Actors/Channels/ChannelPipeline.cs b/src/Netclaw.Actors/Channels/ChannelPipeline.cs index 1dfc9774b..8a13f6624 100644 --- a/src/Netclaw.Actors/Channels/ChannelPipeline.cs +++ b/src/Netclaw.Actors/Channels/ChannelPipeline.cs @@ -162,14 +162,14 @@ public sealed class SessionPipeline : ISessionPipeline private readonly ActorSystem _system; private readonly IRequiredActor _sessionManagerProvider; private readonly ISessionLifecycleObserver? _lifecycleObserver; - private readonly NetclawPaths? _paths; + private readonly NetclawPaths _paths; private readonly SessionIngressGate? _ingressGate; public SessionPipeline( ActorSystem system, IRequiredActor sessionManagerProvider, + NetclawPaths paths, ISessionLifecycleObserver? lifecycleObserver = null, - NetclawPaths? paths = null, SessionIngressGate? ingressGate = null) { _system = system; @@ -290,7 +290,7 @@ private static SendUserMessage MapToCommand( ChannelInput input, SessionId sessionId, SessionPipelineOptions options, - NetclawPaths? paths) + NetclawPaths paths) { var turnId = string.IsNullOrWhiteSpace(input.MessageId) ? Guid.NewGuid().ToString("N")[..8] @@ -304,10 +304,8 @@ private static SendUserMessage MapToCommand( var dataContents = input.Contents.OfType().ToList(); if (dataContents.Count > 0) { - var sessionDir = paths is not null - ? SessionDirectoryHelper.GetSessionDirectory(sessionId, paths.SessionsDirectory) - : SessionDirectoryHelper.GetSessionDirectory(sessionId); - var mediaDir = Path.Combine(sessionDir, "media"); + var sessionDir = SessionDirectoryHelper.GetSessionDirectory(sessionId, paths.SessionsDirectory); + var mediaDir = Path.Combine(sessionDir, SessionDirectoryHelper.MediaSubdirectory); Directory.CreateDirectory(mediaDir); foreach (var data in dataContents) diff --git a/src/Netclaw.Actors/Protocol/InboxWriter.cs b/src/Netclaw.Actors/Protocol/InboxWriter.cs new file mode 100644 index 000000000..dea04dcb4 --- /dev/null +++ b/src/Netclaw.Actors/Protocol/InboxWriter.cs @@ -0,0 +1,138 @@ +using Netclaw.Security; + +namespace Netclaw.Actors.Protocol; + +/// +/// Writes inbound attachment bytes into a session's inbox/ +/// subdirectory with filesystem-level collision suffixing and atomic +/// file writes. Channel adapters call this from their ingress pipeline +/// after the attachment has been downloaded, content-scanned, and +/// audience-gated. Callers are responsible for ensuring the directory +/// exists (use ). +/// +public static class InboxWriter +{ + /// + /// Maximum number of collision-resolution suffixes attempted before + /// giving up. A single session with 99 same-named files is already + /// pathological; higher counts indicate a misconfiguration or + /// abuse. + /// + public const int MaxCollisionSuffix = 99; + + /// + /// Thrown when collision-resolution suffixing exhausts + /// attempts and no free filename + /// remains. Channel adapters SHALL translate this into a loud, + /// user-visible rejection reply rather than silently dropping the + /// attachment. + /// + public sealed class CollisionExhaustedException(string baseName) + : InvalidOperationException( + $"Exhausted {MaxCollisionSuffix} collision suffixes for filename '{baseName}' in inbox/"); + + /// + /// Reserves a unique path in for the + /// given sanitized filename. If the filename already exists on + /// disk, tries foo_1.ext, foo_2.ext, … up to + /// . Returns the full path of the + /// reserved slot. Callers SHALL write to this path atomically via + /// . Reservation is a best-effort + /// check against the filesystem at call time; concurrent writers + /// in the same session would race, but channel binding actors + /// process inbound messages serially, so collisions here are + /// cross-turn only. + /// + public static string ReserveUniquePath(string inboxDir, string safeFilename) + { + if (string.IsNullOrWhiteSpace(safeFilename)) + throw new ArgumentException("safeFilename must be non-empty", nameof(safeFilename)); + + var candidate = Path.Combine(inboxDir, safeFilename); + if (!File.Exists(candidate)) + return candidate; + + var nameOnly = Path.GetFileNameWithoutExtension(safeFilename); + var extension = Path.GetExtension(safeFilename); + + for (var i = 1; i <= MaxCollisionSuffix; i++) + { + var suffixed = $"{nameOnly}_{i}{extension}"; + var full = Path.Combine(inboxDir, suffixed); + if (!File.Exists(full)) + return full; + } + + throw new CollisionExhaustedException(safeFilename); + } + + /// + /// Writes to + /// atomically by first writing to a temp sibling file in the same + /// directory and then using + /// to rename into place. Callers SHALL always pass a path returned + /// by and ensure the parent directory + /// already exists. + /// + public static async Task WriteAtomicAsync( + string targetPath, + ReadOnlyMemory bytes, + CancellationToken ct) + { + var directory = Path.GetDirectoryName(targetPath) + ?? throw new ArgumentException("targetPath must include a directory", nameof(targetPath)); + + var tempPath = Path.Combine(directory, $".{Path.GetFileName(targetPath)}.{Guid.NewGuid():N}.tmp"); + try + { + await using (var stream = new FileStream( + tempPath, + FileMode.CreateNew, + FileAccess.Write, + FileShare.None)) + { + await stream.WriteAsync(bytes, ct).ConfigureAwait(false); + await stream.FlushAsync(ct).ConfigureAwait(false); + } + + File.Move(tempPath, targetPath); + } + catch + { + TryDeleteTemp(tempPath); + throw; + } + } + + private static void TryDeleteTemp(string tempPath) + { + try + { + if (File.Exists(tempPath)) + File.Delete(tempPath); + } + catch + { + // best-effort cleanup; do not mask the original exception + } + } + + /// + /// Convenience: sanitizes the incoming filename with + /// , reserves a unique + /// slot, writes the bytes atomically, and returns the full path + /// that was written. Callers SHALL ensure + /// already exists. + /// + public static async Task SanitizeReserveAndWriteAsync( + string inboxDir, + string rawFilename, + ReadOnlyMemory bytes, + CancellationToken ct) + { + var safeName = FilenameSanitizer.Sanitize(rawFilename); + var targetPath = ReserveUniquePath(inboxDir, safeName); + await WriteAtomicAsync(targetPath, bytes, ct).ConfigureAwait(false); + return targetPath; + } +} diff --git a/src/Netclaw.Actors/Protocol/SessionDirectoryHelper.cs b/src/Netclaw.Actors/Protocol/SessionDirectoryHelper.cs index b78b1e2a3..beb74c607 100644 --- a/src/Netclaw.Actors/Protocol/SessionDirectoryHelper.cs +++ b/src/Netclaw.Actors/Protocol/SessionDirectoryHelper.cs @@ -7,14 +7,16 @@ namespace Netclaw.Actors.Protocol; public static class SessionDirectoryHelper { /// - /// Computes the session directory path under the OS temp directory. - /// Prefer the overload that accepts a base path for daemon-mode sessions. + /// Name of the subdirectory under a session directory where inbound + /// user-uploaded attachments are written by channel adapters. /// - public static string GetSessionDirectory(SessionId sessionId) - { - var sanitized = SanitizeSessionId(sessionId.Value); - return Path.Combine(Path.GetTempPath(), "netclaw-sessions", sanitized); - } + public const string InboxSubdirectory = "inbox"; + + /// + /// Name of the subdirectory under a session directory where outbound + /// DataContent media bytes are persisted. + /// + public const string MediaSubdirectory = "media"; /// /// Computes the session directory path under the given base directory @@ -26,6 +28,46 @@ public static string GetSessionDirectory(SessionId sessionId, string basePath) return Path.Combine(basePath, sanitized); } + /// + /// Computes and creates the inbox/ subdirectory under the + /// session directory, returning its full path. Channel adapters call + /// this when writing user-uploaded attachments to disk. The parent + /// session directory is created if it does not already exist. + /// + public static string GetOrCreateInboxDirectory(SessionId sessionId, string basePath) + { + var sessionDir = GetSessionDirectory(sessionId, basePath); + var inboxDir = Path.Combine(sessionDir, InboxSubdirectory); + Directory.CreateDirectory(inboxDir); + return inboxDir; + } + + /// + /// Returns true when the given base path resolves under + /// . Used by diagnostics to warn + /// operators that attachments and session data will not survive a + /// reboot or tmpfiles cleanup. + /// + public static bool IsUnderTempPath(string basePath) + { + if (string.IsNullOrWhiteSpace(basePath)) + return false; + + var tempRoot = Path.TrimEndingDirectorySeparator(Path.GetFullPath(Path.GetTempPath())); + string fullBase; + try + { + fullBase = Path.TrimEndingDirectorySeparator(Path.GetFullPath(basePath)); + } + catch + { + return false; + } + + return fullBase.Equals(tempRoot, StringComparison.Ordinal) + || fullBase.StartsWith(tempRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal); + } + /// /// Replaces non-alphanumeric characters (except hyphens) with underscores. /// Session IDs may contain slashes (e.g. "C123/1234567890.123456"). diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index d4e9e1387..5448c3131 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -53,13 +53,36 @@ public sealed class LlmSessionActor : ReceivePersistentActor, IWithTimers private readonly IMemoryCheckpointSink _memoryCheckpointSink; private readonly MemoryProposalGate _memoryProposalGate = new(); private readonly TimeProvider _timeProvider; - private readonly string? _sessionsBasePath; - private readonly string? _sessionLogsBasePath; + private readonly string _sessionsBasePath; + private readonly string _sessionLogsBasePath; private readonly ISessionLifecycleObserver? _lifecycleObserver; private readonly Memory.SQLiteMemoryStore? _memoryStore; private readonly IChatClientProvider _clientProvider; private readonly ILoggingAdapter _log; + /// + /// Attachment-handling hint injected into the dynamic system prompt when + /// the resolved grants file_read. + /// Source of truth for the netclaw-input-adapters contract's + /// agent-facing guidance. Any edits to this string must stay in sync + /// with AttachmentNotes and the canonical [attachment] + /// line format. + /// + internal const string AttachmentContextHint = + "[attachments]\n" + + "Your session working directory contains an `inbox/` subdirectory where user-uploaded files are placed.\n" + + "Each attachment is announced in the inbound message as a single line of the form:\n" + + " [attachment] name=\"...\" mime=\"...\" size=... path=\"inbox/...\" inlined=\"true|false\" [note=\"...\"]\n" + + "When `inlined=\"true\"` you can see the file content natively in this turn.\n" + + "When `inlined=\"false\"`:\n" + + " - If `note` begins with \"current model has no\": the file exists on disk but you cannot render it natively. " + + "Acknowledge the attachment to the user by name in your reply and explain the limitation. Offer tool-based " + + "workarounds where applicable (for example, `shell_execute pdftotext` for a PDF on a non-PDF model).\n" + + " - If `note` begins with \"format not inlineable\": use `file_read` or `shell_execute` to process the bytes. " + + "This is the normal path for docx, zip, archive, and media files.\n" + + "Never silently ignore an attachment the user sent — always acknowledge what you received by name, " + + "even if you cannot fully process it."; + // Transient state (not persisted) private readonly List _buffer = new(); private readonly Dictionary _subscribers = new(); @@ -188,8 +211,8 @@ public LlmSessionActor( _memoryCheckpointSink = memory?.CheckpointSink ?? NullMemoryCheckpointSink.Instance; _memoryStore = memory?.MemoryStore; _timeProvider = services.TimeProvider; - _sessionsBasePath = services.Paths?.SessionsDirectory; - _sessionLogsBasePath = services.Paths?.SessionsDirectory; + _sessionsBasePath = services.Paths.SessionsDirectory; + _sessionLogsBasePath = services.Paths.SessionLogsDirectory; _trustContextDeriver = tools?.TrustDeriver; PersistenceId = $"session-{entityId}"; @@ -235,12 +258,9 @@ public LlmSessionActor( TransitionTo(SessionPhase.Ready); - if (_sessionLogsBasePath is not null) - { - _logActor = Context.ActorOf( - SessionLogActor.CreateProps(_sessionId, _sessionLogsBasePath, _timeProvider), - "session-log"); - } + _logActor = Context.ActorOf( + SessionLogActor.CreateProps(_sessionId, _sessionLogsBasePath, _timeProvider), + "session-log"); if (_memoryStore is not null) { @@ -1719,41 +1739,33 @@ private void HandleIncomingUserMessage(SendUserMessage cmd) _config.Tuning.DiscoveredToolMaxCount, _fullRegistry); + // Strict modality consumer contract: the session actor trusts ingress + // to have routed attachments through its own capability gate. If an + // unsupported modality still reaches here, the originating channel + // skipped the contract in netclaw-input-adapters and that's a bug + // the operator needs to see — surface it loudly and continue. if (mediaRefs.Count > 0 && !_model.InputModalities.HasFlag(Configuration.ModelModality.Image)) { - var imageRefs = mediaRefs.Where(r => r.Modality == (int)MediaModality.Image).ToList(); - if (imageRefs.Count > 0) + var offendingRefs = mediaRefs.Where(r => r.Modality == (int)MediaModality.Image).ToList(); + if (offendingRefs.Count > 0) { - _log.Info("Stripping {Count} image reference(s) — model does not support vision", imageRefs.Count); + var offendingDesc = string.Join(",", + offendingRefs.Select(r => $"{r.RelativePath}:modality={r.Modality}")); + _log.Error( + "ingress_bug model={ModelId} modalities={Modalities} offending={Offending}", + _model.ModelId, _model.InputModalities, offendingDesc); + mediaRefs = mediaRefs.Where(r => r.Modality != (int)MediaModality.Image).ToList(); - EmitOutput(new TextOutput - { - SessionId = _sessionId, - Text = "[Images removed — the current model does not support vision input]" - }, OutputFilter.Text); + const string ingressBugNotice = + "[system] An attachment was received but could not be delivered to the model due to an ingress bug. " + + "Please retry, or notify the operator if this persists."; + userContent = string.IsNullOrEmpty(userContent) + ? ingressBugNotice + : userContent + "\n\n" + ingressBugNotice; } } - if (string.IsNullOrWhiteSpace(cmd.Content) && mediaRefs.Count == 0 - && cmd.MediaReferences.Count > 0) - { - _log.Info("Skipping LLM call — message contained only unsupported media"); - EmitOutput(new TextOutput - { - SessionId = _sessionId, - Text = "Your message contained only images, but the current model doesn't support vision. Please send a text message instead." - }, OutputFilter.Text); - EmitOutput(new TurnCompleted - { - SessionId = _sessionId, - TurnNumber = _state.TurnCount, - Outcome = TurnOutcome.Skipped - }); - TryReplyAck(); - return; - } - if (TryHandleSlashCommand(userContent, mediaRefs)) return; @@ -2063,9 +2075,7 @@ internal static bool IsTransientStreamingError(Exception? ex) private string GetSessionDirectory() => - _sessionsBasePath is not null - ? SessionDirectoryHelper.GetSessionDirectory(_sessionId, _sessionsBasePath) - : SessionDirectoryHelper.GetSessionDirectory(_sessionId); + SessionDirectoryHelper.GetSessionDirectory(_sessionId, _sessionsBasePath); private long NowMs() => _timeProvider.GetUtcNow().ToUnixTimeMilliseconds(); @@ -2267,6 +2277,16 @@ private bool TryHandleSlashCommand(string userContent, List /// Inject dynamic context layers as system messages after the persisted system prompt /// but before user messages. Static () layers @@ -2298,12 +2318,9 @@ private void InjectDynamicContextLayers(List messages) parts.Add(_turnRestartNotice); // Session identity — allows the agent to reference its own session and media directory - var sessionBlock = $"[session]\nid: {_sessionId.Value}"; - if (_sessionsBasePath is not null) - { - var sessionDir = SessionDirectoryHelper.GetSessionDirectory(_sessionId, _sessionsBasePath); - sessionBlock += $"\nmedia_dir: {Path.Combine(sessionDir, "media")}"; - } + var sessionDir = SessionDirectoryHelper.GetSessionDirectory(_sessionId, _sessionsBasePath); + var sessionBlock = $"[session]\nid: {_sessionId.Value}" + + $"\nmedia_dir: {Path.Combine(sessionDir, SessionDirectoryHelper.MediaSubdirectory)}"; parts.Add(sessionBlock); // Working context — recent files, open goals, progress markers. @@ -2315,6 +2332,12 @@ private void InjectDynamicContextLayers(List messages) parts.Add(_state.WorkingContext.ToContextBlock()); } + // Attachment-handling hint: conditional on file_read being in the + // resolved tool set. Without file_read the agent cannot inspect + // inbox files, and advertising the path would be a lie. + if (HasFileReadGranted()) + parts.Add(AttachmentContextHint); + _startupContextInjected = true; var contextMessage = new AiChatMessage( diff --git a/src/Netclaw.Actors/Sessions/SessionDependencies.cs b/src/Netclaw.Actors/Sessions/SessionDependencies.cs index a941b5dbc..4e41e846d 100644 --- a/src/Netclaw.Actors/Sessions/SessionDependencies.cs +++ b/src/Netclaw.Actors/Sessions/SessionDependencies.cs @@ -15,7 +15,7 @@ public sealed record SessionServices( ISystemPromptProvider PromptProvider, IReadOnlyList ContextLayers, TimeProvider TimeProvider, - NetclawPaths? Paths); + NetclawPaths Paths); /// /// Tool execution infrastructure. Null when the session operates without tools. diff --git a/src/Netclaw.Actors/Sessions/SessionLogActor.cs b/src/Netclaw.Actors/Sessions/SessionLogActor.cs index 9db3d83cb..566ee3fb3 100644 --- a/src/Netclaw.Actors/Sessions/SessionLogActor.cs +++ b/src/Netclaw.Actors/Sessions/SessionLogActor.cs @@ -7,30 +7,31 @@ namespace Netclaw.Actors.Sessions; /// /// Per-session child actor that owns the log file lifecycle. -/// Created by when a sessions base directory is configured. +/// Created by . /// Not persistent — log files are best-effort observability. /// The actor opens the log file in and disposes it in , /// ensuring the file handle is properly released when the session actor passivates. /// -/// Log files are stored inside the session's own directory: -/// {sessionsBase}/{sanitized_id}/logs/{timestamp}.log +/// Log files live at {sessionLogsBase}/{sanitized_id}/{timestamp}.log — a +/// tree deliberately separate from the agent-accessible session working +/// directory so the LLM cannot read its own audit trail via the file_read tool. /// Multiple log files in the same directory indicate passivation/rehydration cycles. /// public sealed class SessionLogActor : ReceiveActor { private readonly SessionId _sessionId; - private readonly string _sessionsBasePath; + private readonly string _sessionLogsBasePath; private readonly TimeProvider _timeProvider; private readonly ILoggingAdapter _log = Context.GetLogger(); private StreamWriter? _writer; - public static Props CreateProps(SessionId sessionId, string sessionsBasePath, TimeProvider timeProvider) => - Props.Create(() => new SessionLogActor(sessionId, sessionsBasePath, timeProvider)); + public static Props CreateProps(SessionId sessionId, string sessionLogsBasePath, TimeProvider timeProvider) => + Props.Create(() => new SessionLogActor(sessionId, sessionLogsBasePath, timeProvider)); - public SessionLogActor(SessionId sessionId, string sessionsBasePath, TimeProvider timeProvider) + public SessionLogActor(SessionId sessionId, string sessionLogsBasePath, TimeProvider timeProvider) { _sessionId = sessionId; - _sessionsBasePath = sessionsBasePath; + _sessionLogsBasePath = sessionLogsBasePath; _timeProvider = timeProvider; Receive(OnUserMessage); @@ -39,12 +40,12 @@ public SessionLogActor(SessionId sessionId, string sessionsBasePath, TimeProvide /// /// Computes the logs directory for this session: - /// {sessionsBase}/{sanitized_id}/logs/ + /// {sessionLogsBase}/{sanitized_id}/ /// - internal static string GetSessionLogsDirectory(SessionId sessionId, string sessionsBasePath) + public static string GetSessionLogsDirectory(SessionId sessionId, string sessionLogsBasePath) { var sanitized = SessionDirectoryHelper.SanitizeSessionId(sessionId.Value); - return Path.Combine(sessionsBasePath, sanitized, "logs"); + return Path.Combine(sessionLogsBasePath, sanitized); } protected override void PreStart() @@ -52,7 +53,7 @@ protected override void PreStart() try { var now = _timeProvider.GetUtcNow(); - var logsDir = GetSessionLogsDirectory(_sessionId, _sessionsBasePath); + var logsDir = GetSessionLogsDirectory(_sessionId, _sessionLogsBasePath); var logPath = Path.Combine(logsDir, $"{now:yyyyMMdd-HHmmss}.log"); Directory.CreateDirectory(logsDir); _writer = new StreamWriter(logPath, append: true) { AutoFlush = true }; diff --git a/src/Netclaw.Actors/Tools/FileReadTool.cs b/src/Netclaw.Actors/Tools/FileReadTool.cs index 14d748d79..783e6ee2b 100644 --- a/src/Netclaw.Actors/Tools/FileReadTool.cs +++ b/src/Netclaw.Actors/Tools/FileReadTool.cs @@ -9,11 +9,13 @@ namespace Netclaw.Actors.Tools; /// /// Reads file contents as UTF-8 text with optional line offset/limit. /// -[NetclawTool("file_read", +[NetclawTool(ToolName, "Read the contents of a file as text", Grant = "file")] public sealed partial class FileReadTool : NetclawTool { + public const string ToolName = "file_read"; + private readonly ToolConfig _config; private readonly ToolPathPolicy? _pathPolicy; private readonly ScopedFileAccessPolicy _fileAccessPolicy; diff --git a/src/Netclaw.Channels.Slack/SlackChannel.cs b/src/Netclaw.Channels.Slack/SlackChannel.cs index 9c0e929a9..c0436a461 100644 --- a/src/Netclaw.Channels.Slack/SlackChannel.cs +++ b/src/Netclaw.Channels.Slack/SlackChannel.cs @@ -28,6 +28,9 @@ public sealed class SlackChannel : IChannel, IEventHandler, IEvent private readonly SlackChannelOptions _options; private readonly ILogger _logger; private readonly IThreadHistoryFetcher _threadHistoryFetcher; + private readonly ToolAudienceProfiles _audienceProfiles; + private readonly ModelCapabilities _modelCapabilities; + private readonly NetclawPaths _paths; private IActorRef? _gateway; private SlackUserId? _botUserId; @@ -48,7 +51,10 @@ public SlackChannel( TimeProvider timeProvider, SlackChannelOptions options, ILogger logger, - IThreadHistoryFetcher threadHistoryFetcher) + IThreadHistoryFetcher threadHistoryFetcher, + ToolConfig toolConfig, + ModelCapabilities modelCapabilities, + NetclawPaths paths) { _pipeline = pipeline; _system = system; @@ -64,6 +70,9 @@ public SlackChannel( _options = options; _logger = logger; _threadHistoryFetcher = threadHistoryFetcher ?? throw new ArgumentNullException(nameof(threadHistoryFetcher)); + _audienceProfiles = toolConfig.AudienceProfiles; + _modelCapabilities = modelCapabilities; + _paths = paths; } public Actors.Channels.ChannelType ChannelType => Actors.Channels.ChannelType.Slack; @@ -141,9 +150,12 @@ public async Task StartAsync(CancellationToken cancellationToken) DefaultChannelId: _defaultChannelId, ReplyClient: _replyClient, ContentScanner: _contentScanner, + ThreadHistoryFetcher: _threadHistoryFetcher, + AudienceProfiles: _audienceProfiles, + ModelCapabilities: _modelCapabilities, + Paths: _paths, HttpClient: httpClient, - PromptInjectionDetector: _promptInjectionDetector, - ThreadHistoryFetcher: _threadHistoryFetcher)), + PromptInjectionDetector: _promptInjectionDetector)), "slack-gateway"); await _socketModeClient.Connect(cancellationToken: cancellationToken); diff --git a/src/Netclaw.Channels.Slack/SlackGatewayActor.cs b/src/Netclaw.Channels.Slack/SlackGatewayActor.cs index 6c0c9be87..f9a710307 100644 --- a/src/Netclaw.Channels.Slack/SlackGatewayActor.cs +++ b/src/Netclaw.Channels.Slack/SlackGatewayActor.cs @@ -3,6 +3,7 @@ using Netclaw.Actors.Channels; using Netclaw.Actors.Protocol; using Netclaw.Channels.Telemetry; +using Netclaw.Configuration; using Netclaw.Security; namespace Netclaw.Channels.Slack; @@ -104,6 +105,9 @@ public sealed record SlackGatewayDependencies( ISlackReplyClient ReplyClient, IContentScanner ContentScanner, IThreadHistoryFetcher ThreadHistoryFetcher, + ToolAudienceProfiles AudienceProfiles, + Netclaw.Configuration.ModelCapabilities ModelCapabilities, + NetclawPaths Paths, HttpClient? HttpClient = null, Func? ConversationPropsFactory = null, Func? ThreadPropsFactory = null, diff --git a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs index b18993d0e..8a3de5645 100644 --- a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs +++ b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs @@ -204,66 +204,18 @@ private async Task HandleInboundAsync(SlackThreadInbound message) } } - // Build content list: text + downloaded file attachments + // Build content list: text + attachment announcements + inline DataContent var contents = new List(); if (!string.IsNullOrEmpty(message.Text)) contents.Add(new TextContent(message.Text)); - // Download and scan file attachments - if (message.Files is { Count: > 0 } && _dependencies.HttpClient is not null) + if (message.Files is { Count: > 0 }) { - foreach (var file in message.Files) - { - // Only process image MIME types for now - if (!file.MimeType.StartsWith("image/", StringComparison.OrdinalIgnoreCase)) - { - _log.Debug("Skipping non-image file attachment: {Name} ({MimeType})", file.Name, file.MimeType); - continue; - } - - try - { - using var downloadCts = CancellationTokenSource.CreateLinkedTokenSource(inboundCts.Token); - downloadCts.CancelAfter(OperationTimeout); - var bytes = await DownloadSlackFileAsync(file, downloadCts.Token); - if (bytes.Length == 0) - continue; - - using var scanCts = CancellationTokenSource.CreateLinkedTokenSource(inboundCts.Token); - scanCts.CancelAfter(OperationTimeout); - var scanResult = await _dependencies.ContentScanner.ScanAsync( - bytes, file.Name, file.MimeType, scanCts.Token); - - if (!scanResult.IsAllowed) - { - if (scanResult.Error == ContentScanError.ScanFailure) - { - // Scanner itself is broken — allow the file through rather - // than silently dropping valid images. The LLM provider - // will still validate content on its end. - _log.Error("Content scanner failed for file {Name}: {Message} — allowing file through", - file.Name, scanResult.Message); - } - else - { - _log.Warning("Content scan rejected file {Name}: {Message}", - file.Name, scanResult.Message ?? scanResult.Error?.ToString()); - continue; - } - } - - contents.Add(new DataContent(bytes.ToArray(), file.MimeType)); - _log.Info("Downloaded and scanned Slack file: {Name} ({Size} bytes)", file.Name, bytes.Length); - } - catch (OperationCanceledException ex) - { - _log.Warning(ex, "Timed out while processing Slack file {Name}, skipping", file.Name); - } - catch (Exception ex) - { - _log.Warning(ex, "Failed to download Slack file {Name}, skipping", file.Name); - } - } + await ProcessInboundAttachmentsAsync( + message.Files, + message.Audience, + contents, + inboundCts.Token); } if (contents.Count == 0) @@ -340,6 +292,310 @@ private async Task HandleInboundAsync(SlackThreadInbound message) private Task> DownloadSlackFileAsync(SlackFileReference file, CancellationToken ct) => SlackFileDownloader.DownloadAsync(_dependencies.HttpClient!, file.UrlPrivateDownload, _dependencies.Options.BotToken, ct); + /// + /// Applies the canonical cross-channel attachment ingress pipeline to + /// the inbound Slack file list: audience-gated policy check, per-file + /// and per-message cap checks, download, scan, inbox write, and + /// capability-gated inlining. Appends one batched + /// [attachment] block plus any + /// inlined items to . + /// Every rejection path posts a user-visible reply — no silent drops. + /// + private async Task ProcessInboundAttachmentsAsync( + IReadOnlyList files, + TrustAudience audience, + List contents, + CancellationToken cancellationToken) + { + if (_dependencies.HttpClient is null) + { + _log.Warning( + "Slack HTTP client is not configured; rejecting {Count} inbound attachment(s)", + files.Count); + await SafePostAsync(":warning: I can't download attachments right now — HTTP client is not configured."); + return; + } + + var profile = ToolAudienceProfileDefaults.GetResolvedProfile(_dependencies.AudienceProfiles, audience); + var policy = profile.ChannelAttachments ?? ChannelAttachmentPolicy.Empty; + + if (files.Count > policy.MaxFilesPerMessage) + { + _log.Warning( + "slack_attachments_rejected count={Count} limit={Limit} audience={Audience} reason=too-many-files", + files.Count, + policy.MaxFilesPerMessage, + audience); + await SafePostAsync( + $":warning: I can only accept up to {policy.MaxFilesPerMessage} attachments per message. " + + $"Please split your upload and try again. Text content was delivered."); + return; + } + + // Resolve the capability view once per message — the active model's + // InputModalities determine which accepted categories get inlined + // as DataContent versus path-only announcements. + var modelCapabilities = _dependencies.ModelCapabilities; + var inlineImages = modelCapabilities.InputModalities.HasFlag(ModelModality.Image); + // Microsoft.Extensions.AI doesn't define a dedicated Pdf modality flag; + // provider plugins that natively accept application/pdf use the Image + // flag on the document pipeline. This matches how LlmSessionActor's + // existing mediaRefs gate surfaces modality support today. + var inlinePdfs = modelCapabilities.InputModalities.HasFlag(ModelModality.Image); + + var acceptedLines = new List(files.Count); + var dataContents = new List(); + var rejections = new List(); + + var inboxDir = SessionDirectoryHelper.GetOrCreateInboxDirectory(_sessionId, _dependencies.Paths.SessionsDirectory); + + foreach (var file in files) + { + var attachmentResult = await TryIngestSingleAttachmentAsync( + file, + audience, + policy, + inlineImages, + inlinePdfs, + inboxDir, + cancellationToken); + + switch (attachmentResult) + { + case AttachmentIngestResult.Accepted accepted: + acceptedLines.Add(accepted.Line); + if (accepted.Inline is { } inline) + dataContents.Add(inline); + break; + + case AttachmentIngestResult.Rejected rejected: + rejections.Add(rejected.UserFacingReason); + break; + } + } + + if (acceptedLines.Count > 0) + { + contents.Add(new TextContent(string.Join('\n', acceptedLines))); + contents.AddRange(dataContents); + } + + if (rejections.Count > 0) + { + var joined = rejections.Count == 1 + ? rejections[0] + : ":warning: Some attachments were not accepted:\n - " + string.Join("\n - ", rejections); + await SafePostAsync(joined); + } + } + + private async Task TryIngestSingleAttachmentAsync( + SlackFileReference file, + TrustAudience audience, + ChannelAttachmentPolicy policy, + bool inlineImages, + bool inlinePdfs, + string inboxDir, + CancellationToken cancellationToken) + { + var category = AttachmentCategories.FromMime(file.MimeType); + + // Pre-download policy gates — these all operate on Slack-reported + // metadata and avoid burning bandwidth on files that can't be accepted. + if (!policy.Allows(category)) + { + _log.Warning( + "slack_attachment_rejected name={Name} mime={Mime} audience={Audience} category={Category} reason=category-not-allowed", + file.Name, file.MimeType, audience, category); + return new AttachmentIngestResult.Rejected( + $"`{file.Name}` ({category}) isn't allowed in {audience} channels. " + + "Please DM me if you want to share this class of file."); + } + + if (file.Size > policy.MaxFileBytes) + { + _log.Warning( + "slack_attachment_rejected name={Name} mime={Mime} audience={Audience} size={Size} limit={Limit} reason=too-large", + file.Name, file.MimeType, audience, file.Size, policy.MaxFileBytes); + return new AttachmentIngestResult.Rejected( + $"`{file.Name}` ({FormatBytes(file.Size)}) exceeds the {FormatBytes(policy.MaxFileBytes)} per-file limit."); + } + + ReadOnlyMemory bytes; + try + { + using var downloadCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + downloadCts.CancelAfter(OperationTimeout); + bytes = await DownloadSlackFileAsync(file, downloadCts.Token); + } + catch (OperationCanceledException ex) + { + _log.Warning(ex, + "slack_attachment_rejected name={Name} mime={Mime} reason=download-timeout", + file.Name, file.MimeType); + return new AttachmentIngestResult.Rejected( + $"Timed out downloading `{file.Name}`. Please try again."); + } + catch (Exception ex) + { + _log.Warning(ex, + "slack_attachment_rejected name={Name} mime={Mime} reason=download-failed", + file.Name, file.MimeType); + return new AttachmentIngestResult.Rejected( + $"Couldn't download `{file.Name}`: {ex.Message}."); + } + + if (bytes.Length == 0) + { + _log.Warning( + "slack_attachment_rejected name={Name} mime={Mime} reason=empty-download", + file.Name, file.MimeType); + return new AttachmentIngestResult.Rejected( + $"`{file.Name}` downloaded as zero bytes."); + } + + ContentScanResult scanResult; + try + { + using var scanCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + scanCts.CancelAfter(OperationTimeout); + scanResult = await _dependencies.ContentScanner.ScanAsync( + bytes, file.Name, file.MimeType, scanCts.Token); + } + catch (Exception ex) + { + _log.Warning(ex, + "slack_attachment_rejected name={Name} mime={Mime} reason=scan-exception", + file.Name, file.MimeType); + return new AttachmentIngestResult.Rejected( + $"Couldn't scan `{file.Name}`: {ex.Message}."); + } + + if (!scanResult.IsAllowed) + { + if (scanResult.Error == ContentScanError.ScanFailure) + { + // Scanner itself is broken — allow the file through rather than + // silently dropping it. The LLM provider will still validate. + _log.Error( + "Content scanner failed for file {Name}: {Message} — allowing file through", + file.Name, scanResult.Message); + } + else + { + _log.Warning( + "slack_attachment_rejected name={Name} mime={Mime} reason=scan-blocked message={ScanMessage}", + file.Name, file.MimeType, scanResult.Message ?? scanResult.Error?.ToString()); + return new AttachmentIngestResult.Rejected( + $"Content scanner rejected `{file.Name}`: {scanResult.Message ?? scanResult.Error?.ToString()}."); + } + } + + // Write to inbox with filesystem-level collision suffixing and atomic move. + string inboxPath; + try + { + inboxPath = await InboxWriter.SanitizeReserveAndWriteAsync( + inboxDir, + file.Name, + bytes, + cancellationToken); + } + catch (InboxWriter.CollisionExhaustedException ex) + { + _log.Warning(ex, + "slack_attachment_rejected name={Name} reason=collision-exhausted", + file.Name); + return new AttachmentIngestResult.Rejected( + $"Too many attachments named `{file.Name}` in this session — please rename and try again."); + } + catch (Exception ex) + { + _log.Error(ex, + "slack_attachment_rejected name={Name} reason=inbox-write-failed", + file.Name); + return new AttachmentIngestResult.Rejected( + $"Couldn't save `{file.Name}` to disk: {ex.Message}."); + } + + // Decide inlining based on model modalities and category. + var (inlined, note) = ResolveInlineDecision(category, inlineImages, inlinePdfs); + + var relativePath = $"{SessionDirectoryHelper.InboxSubdirectory}/{Path.GetFileName(inboxPath)}"; + var line = BuildAttachmentLine(file.Name, file.MimeType, bytes.Length, relativePath, inlined, note); + + DataContent? inlineContent = null; + if (inlined) + inlineContent = new DataContent(bytes, file.MimeType); + + _log.Info( + "slack_attachment_accepted name={Name} mime={Mime} size={Size} audience={Audience} category={Category} inlined={Inlined}", + file.Name, file.MimeType, bytes.Length, audience, category, inlined); + + return new AttachmentIngestResult.Accepted(line, inlineContent); + } + + private static (bool Inlined, string? Note) ResolveInlineDecision( + AttachmentCategory category, + bool inlineImages, + bool inlinePdfs) + { + return category switch + { + AttachmentCategory.Image when inlineImages => (true, null), + AttachmentCategory.Image => (false, AttachmentNotes.ModelMissingImage), + AttachmentCategory.Pdf when inlinePdfs => (true, null), + AttachmentCategory.Pdf => (false, AttachmentNotes.ModelMissingPdf), + _ => (false, AttachmentNotes.FormatNotInlineable) + }; + } + + /// + /// Formats a single [attachment] announcement line in the + /// canonical cross-channel shape defined in netclaw-input-adapters. + /// + private static string BuildAttachmentLine( + string name, + string mimeType, + long size, + string relativePath, + bool inlined, + string? note) + { + var inlinedWire = inlined ? "true" : "false"; + var sb = new StringBuilder(128); + sb.Append("[attachment] name=\"").Append(EscapeQuoted(name)).Append('"'); + sb.Append(" mime=\"").Append(EscapeQuoted(mimeType)).Append('"'); + sb.Append(" size=").Append(size); + sb.Append(" path=\"").Append(EscapeQuoted(relativePath)).Append('"'); + sb.Append(" inlined=\"").Append(inlinedWire).Append('"'); + if (!string.IsNullOrEmpty(note)) + sb.Append(" note=\"").Append(EscapeQuoted(note)).Append('"'); + return sb.ToString(); + } + + private static string EscapeQuoted(string value) => + value.Replace("\\", "\\\\", StringComparison.Ordinal) + .Replace("\"", "\\\"", StringComparison.Ordinal); + + private static string FormatBytes(long size) + { + const long Mib = 1024 * 1024; + const long Kib = 1024; + if (size >= Mib) + return $"{size / (double)Mib:F1} MiB"; + if (size >= Kib) + return $"{size / (double)Kib:F1} KiB"; + return $"{size} bytes"; + } + + private abstract record AttachmentIngestResult + { + public sealed record Accepted(string Line, DataContent? Inline) : AttachmentIngestResult; + + public sealed record Rejected(string UserFacingReason) : AttachmentIngestResult; + } + private async Task EnsureInitializedAsync() { if (_session is not null) diff --git a/src/Netclaw.Cli.Tests/Doctor/ConfigSchemaDoctorCheckTests.cs b/src/Netclaw.Cli.Tests/Doctor/ConfigSchemaDoctorCheckTests.cs index e71eda23c..863d97ffd 100644 --- a/src/Netclaw.Cli.Tests/Doctor/ConfigSchemaDoctorCheckTests.cs +++ b/src/Netclaw.Cli.Tests/Doctor/ConfigSchemaDoctorCheckTests.cs @@ -284,6 +284,81 @@ await File.WriteAllTextAsync(paths.NetclawConfigPath, Assert.Equal(DoctorSeverity.Pass, result.Severity); } + [Fact] + public async Task ReturnsPass_WhenToolsSectionMissingChannelAttachments() + { + // Bear-trap test for the channel-ingress-attachments migration path: + // an existing config without any ChannelAttachments block must + // continue to validate against schema v1. New fields on + // ToolAudienceProfile are optional, so omitting them is legal. + var basePath = CreateTempBasePath(); + var paths = new NetclawPaths(basePath); + paths.EnsureDirectoriesExist(); + + await File.WriteAllTextAsync(paths.NetclawConfigPath, + """ + { + "configVersion": 1, + "Tools": { + "AudienceProfiles": { + "Public": { + "ToolsMode": "Allowlist", + "AllowedTools": ["file_read"], + "ReadFiles": { "Mode": "Roots", "Roots": ["{session_dir}"] } + }, + "Team": { + "ToolsMode": "Allowlist", + "AllowedTools": ["file_read", "attach_file"], + "ReadFiles": { "Mode": "Roots", "Roots": ["{session_dir}"] } + }, + "Personal": { + "ToolsMode": "All", + "ReadFiles": { "Mode": "All" } + } + } + } + } + """, TestContext.Current.CancellationToken); + + var check = new ConfigSchemaDoctorCheck(paths); + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Pass, result.Severity); + } + + [Fact] + public async Task ReturnsPass_WhenChannelAttachmentsBlockIsExplicit() + { + // Config that explicitly sets a ChannelAttachments block on one + // profile should also validate. + var basePath = CreateTempBasePath(); + var paths = new NetclawPaths(basePath); + paths.EnsureDirectoriesExist(); + + await File.WriteAllTextAsync(paths.NetclawConfigPath, + """ + { + "configVersion": 1, + "Tools": { + "AudienceProfiles": { + "Public": { + "ChannelAttachments": { + "AllowedCategories": ["Image"], + "MaxFileBytes": 26214400, + "MaxFilesPerMessage": 10 + } + } + } + } + } + """, TestContext.Current.CancellationToken); + + var check = new ConfigSchemaDoctorCheck(paths); + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Pass, result.Severity); + } + private static string CreateTempBasePath() { var path = Path.Combine(Path.GetTempPath(), "netclaw-tests", Guid.NewGuid().ToString("N")); diff --git a/src/Netclaw.Cli/Doctor/ToolAudienceProfilesDoctorCheck.cs b/src/Netclaw.Cli/Doctor/ToolAudienceProfilesDoctorCheck.cs index 921790593..885be9478 100644 --- a/src/Netclaw.Cli/Doctor/ToolAudienceProfilesDoctorCheck.cs +++ b/src/Netclaw.Cli/Doctor/ToolAudienceProfilesDoctorCheck.cs @@ -1,5 +1,6 @@ using System.Text.Json; using System.Text.Json.Nodes; +using Netclaw.Actors.Protocol; using Netclaw.Actors.Tools; using Netclaw.Configuration; @@ -64,12 +65,16 @@ public Task RunAsync(CancellationToken cancellationToken = de ValidateNonPersonalProfile("public", toolConfig.AudienceProfiles.Public, errors); ValidateNonPersonalProfile("team", toolConfig.AudienceProfiles.Team, errors); + // Channel attachment policy cap-vs-allowlist consistency. + foreach (var err in toolConfig.AudienceProfiles.ValidateChannelAttachments()) + errors.Add(err); + if (errors.Count > 0) { return Task.FromResult(DoctorCheckResult.Error( "Tool Audience Profiles", string.Join("; ", errors), - "Restrict public/team profiles to allowlists and rooted filesystem access. Unrestricted modes are only safe for personal profiles.")); + "Restrict public/team profiles to allowlists and rooted filesystem access, and ensure ChannelAttachments caps are positive when categories are allowed.")); } var warnings = new List(); @@ -90,6 +95,17 @@ public Task RunAsync(CancellationToken cancellationToken = de // Advisory: approval mode configured but shell is off CheckApprovalMismatch(toolConfig, warnings); + // Advisory: session directory base path is under the OS temp directory. + // Attachment files and session journals will not survive reboots or + // tmpfiles cleanup in that configuration. + if (SessionDirectoryHelper.IsUnderTempPath(paths.SessionsDirectory)) + { + warnings.Add( + $"Session directory base path ({paths.SessionsDirectory}) is under the OS temp directory. " + + "Inbound attachments written to inbox/ and other session files will be lost on reboot, " + + "leaving dangling references in the persisted turn journal."); + } + // Advisory: stale patterns in tool-approvals.json CheckStaleApprovals(toolConfig, paths, warnings); diff --git a/src/Netclaw.Configuration.Tests/ChannelAttachmentPolicyTests.cs b/src/Netclaw.Configuration.Tests/ChannelAttachmentPolicyTests.cs new file mode 100644 index 000000000..594f56ce1 --- /dev/null +++ b/src/Netclaw.Configuration.Tests/ChannelAttachmentPolicyTests.cs @@ -0,0 +1,157 @@ +using Xunit; + +namespace Netclaw.Configuration.Tests; + +public sealed class ChannelAttachmentPolicyTests +{ + [Fact] + public void Empty_policy_allows_nothing_and_has_zero_caps() + { + var empty = ChannelAttachmentPolicy.Empty; + + Assert.Empty(empty.AllowedCategories); + Assert.Equal(0, empty.MaxFileBytes); + Assert.Equal(0, empty.MaxFilesPerMessage); + Assert.False(empty.Allows(AttachmentCategory.Image)); + Assert.False(empty.Allows(AttachmentCategory.Pdf)); + Assert.False(empty.Allows(AttachmentCategory.Document)); + } + + [Fact] + public void Default_public_profile_allows_only_images() + { + var profile = ToolAudienceProfileDefaults.CreatePublic(); + var policy = profile.ChannelAttachments; + + Assert.True(policy.Allows(AttachmentCategory.Image)); + Assert.False(policy.Allows(AttachmentCategory.Pdf)); + Assert.False(policy.Allows(AttachmentCategory.Document)); + Assert.False(policy.Allows(AttachmentCategory.Archive)); + Assert.False(policy.Allows(AttachmentCategory.Media)); + Assert.False(policy.Allows(AttachmentCategory.Other)); + Assert.Equal(ChannelAttachmentPolicy.DefaultMaxFileBytes, policy.MaxFileBytes); + Assert.Equal(ChannelAttachmentPolicy.DefaultMaxFilesPerMessage, policy.MaxFilesPerMessage); + } + + [Fact] + public void Default_team_profile_allows_everything_except_other() + { + var profile = ToolAudienceProfileDefaults.CreateTeam(); + var policy = profile.ChannelAttachments; + + Assert.True(policy.Allows(AttachmentCategory.Image)); + Assert.True(policy.Allows(AttachmentCategory.Pdf)); + Assert.True(policy.Allows(AttachmentCategory.Document)); + Assert.True(policy.Allows(AttachmentCategory.Archive)); + Assert.True(policy.Allows(AttachmentCategory.Media)); + Assert.False(policy.Allows(AttachmentCategory.Other)); + } + + [Fact] + public void Default_personal_profile_allows_every_category_including_other() + { + var profile = ToolAudienceProfileDefaults.CreatePersonal(); + var policy = profile.ChannelAttachments; + + Assert.True(policy.Allows(AttachmentCategory.Image)); + Assert.True(policy.Allows(AttachmentCategory.Pdf)); + Assert.True(policy.Allows(AttachmentCategory.Document)); + Assert.True(policy.Allows(AttachmentCategory.Archive)); + Assert.True(policy.Allows(AttachmentCategory.Media)); + Assert.True(policy.Allows(AttachmentCategory.Other)); + } + + [Fact] + public void Default_caps_are_twentyfive_mib_and_ten_files() + { + Assert.Equal(25L * 1024 * 1024, ChannelAttachmentPolicy.DefaultMaxFileBytes); + Assert.Equal(10, ChannelAttachmentPolicy.DefaultMaxFilesPerMessage); + } + + [Fact] + public void Validation_passes_on_default_profiles() + { + var profiles = ToolAudienceProfileDefaults.CreateProfiles(); + + var errors = profiles.ValidateChannelAttachments(); + + Assert.Empty(errors); + } + + [Fact] + public void Validation_rejects_allowed_category_with_zero_size_cap() + { + var profiles = ToolAudienceProfileDefaults.CreateProfiles(); + profiles.Team.ChannelAttachments.MaxFileBytes = 0; + + var errors = profiles.ValidateChannelAttachments(); + + Assert.Contains(errors, e => e.Contains("Team", System.StringComparison.Ordinal) + && e.Contains("MaxFileBytes", System.StringComparison.Ordinal)); + } + + [Fact] + public void Validation_rejects_allowed_category_with_zero_file_count_cap() + { + var profiles = ToolAudienceProfileDefaults.CreateProfiles(); + profiles.Personal.ChannelAttachments.MaxFilesPerMessage = 0; + + var errors = profiles.ValidateChannelAttachments(); + + Assert.Contains(errors, e => e.Contains("Personal", System.StringComparison.Ordinal) + && e.Contains("MaxFilesPerMessage", System.StringComparison.Ordinal)); + } + + [Fact] + public void Validation_accepts_empty_allowlist_with_zero_caps() + { + var profiles = ToolAudienceProfileDefaults.CreateProfiles(); + profiles.Public.ChannelAttachments = new ChannelAttachmentPolicy + { + AllowedCategories = [], + MaxFileBytes = 0, + MaxFilesPerMessage = 0 + }; + + var errors = profiles.ValidateChannelAttachments(); + + Assert.Empty(errors); + } +} + +public sealed class AttachmentCategoriesTests +{ + [Theory] + [InlineData("image/png", AttachmentCategory.Image)] + [InlineData("image/jpeg", AttachmentCategory.Image)] + [InlineData("IMAGE/WEBP", AttachmentCategory.Image)] + [InlineData("image/gif;charset=binary", AttachmentCategory.Image)] + [InlineData("application/pdf", AttachmentCategory.Pdf)] + [InlineData("APPLICATION/PDF", AttachmentCategory.Pdf)] + [InlineData("application/msword", AttachmentCategory.Document)] + [InlineData("application/vnd.openxmlformats-officedocument.wordprocessingml.document", AttachmentCategory.Document)] + [InlineData("text/plain", AttachmentCategory.Document)] + [InlineData("text/markdown", AttachmentCategory.Document)] + [InlineData("application/json", AttachmentCategory.Document)] + [InlineData("application/zip", AttachmentCategory.Archive)] + [InlineData("application/x-tar", AttachmentCategory.Archive)] + [InlineData("application/gzip", AttachmentCategory.Archive)] + [InlineData("application/x-7z-compressed", AttachmentCategory.Archive)] + [InlineData("video/mp4", AttachmentCategory.Media)] + [InlineData("audio/mpeg", AttachmentCategory.Media)] + [InlineData("application/octet-stream", AttachmentCategory.Other)] + [InlineData("wibble/wobble", AttachmentCategory.Other)] + public void FromMime_classifies_known_types(string mime, AttachmentCategory expected) + { + Assert.Equal(expected, AttachmentCategories.FromMime(mime)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void FromMime_returns_other_for_missing_input(string? mime) + { + Assert.Equal(AttachmentCategory.Other, AttachmentCategories.FromMime(mime)); + } +} diff --git a/src/Netclaw.Configuration/ChannelAttachmentPolicy.cs b/src/Netclaw.Configuration/ChannelAttachmentPolicy.cs new file mode 100644 index 000000000..00913b65d --- /dev/null +++ b/src/Netclaw.Configuration/ChannelAttachmentPolicy.cs @@ -0,0 +1,145 @@ +namespace Netclaw.Configuration; + +/// +/// Coarse policy classes for inbound file attachments. Channel adapters map +/// MIME types to these classes via , +/// and per-audience policy allows or denies each class. Categories exist so +/// operators can reason about attachment trust in human-friendly terms +/// ("allow images") rather than maintaining MIME-type allowlists. +/// Unknown or unrecognized MIME types map to and are +/// fail-closed at all audiences except Personal by default. +/// +public enum AttachmentCategory +{ + Image, + Pdf, + Document, + Archive, + Media, + Other +} + +/// +/// Per-audience policy for inbound channel attachments. Channel adapters query +/// this from the resolved before building +/// ChannelInput.Contents for an inbound message. The empty policy +/// () denies every category and is the fail-closed default. +/// +public sealed class ChannelAttachmentPolicy +{ + public const long DefaultMaxFileBytes = 25L * 1024 * 1024; + public const int DefaultMaxFilesPerMessage = 10; + + /// + /// Categories allowed for this audience. Empty means all attachments are + /// rejected regardless of category. + /// + public List AllowedCategories { get; set; } = []; + + /// + /// Maximum per-file byte size. Files whose transport-reported size exceeds + /// this are rejected before download. + /// + public long MaxFileBytes { get; set; } = DefaultMaxFileBytes; + + /// + /// Maximum number of attached files accepted on a single inbound message. + /// Inbound messages exceeding this are rejected with a user-visible reply. + /// + public int MaxFilesPerMessage { get; set; } = DefaultMaxFilesPerMessage; + + /// + /// Fail-closed policy: no categories permitted, zero size, zero file + /// count. Used as the default value for + /// so an unconfigured profile rejects every attachment until operators + /// opt in. + /// + public static ChannelAttachmentPolicy Empty => new() + { + AllowedCategories = [], + MaxFileBytes = 0, + MaxFilesPerMessage = 0 + }; + + public bool Allows(AttachmentCategory category) => AllowedCategories.Contains(category); +} + +/// +/// Maps MIME types to . This is the only +/// place in the codebase that classifies MIME strings; callers that need to +/// reason about attachment classes SHALL use rather +/// than open-coding prefix checks. +/// +public static class AttachmentCategories +{ + public static AttachmentCategory FromMime(string? mime) + { + if (string.IsNullOrWhiteSpace(mime)) + return AttachmentCategory.Other; + + var trimmed = mime.Trim(); + var semicolon = trimmed.IndexOf(';', StringComparison.Ordinal); + if (semicolon >= 0) + trimmed = trimmed[..semicolon].Trim(); + + if (trimmed.StartsWith("image/", StringComparison.OrdinalIgnoreCase)) + return AttachmentCategory.Image; + + if (string.Equals(trimmed, "application/pdf", StringComparison.OrdinalIgnoreCase)) + return AttachmentCategory.Pdf; + + if (trimmed.StartsWith("video/", StringComparison.OrdinalIgnoreCase) + || trimmed.StartsWith("audio/", StringComparison.OrdinalIgnoreCase)) + return AttachmentCategory.Media; + + if (IsArchive(trimmed)) + return AttachmentCategory.Archive; + + if (IsDocument(trimmed)) + return AttachmentCategory.Document; + + return AttachmentCategory.Other; + } + + private static bool IsArchive(string mime) => mime.ToLowerInvariant() switch + { + "application/zip" => true, + "application/x-zip-compressed" => true, + "application/x-tar" => true, + "application/gzip" => true, + "application/x-gzip" => true, + "application/x-7z-compressed" => true, + "application/x-rar-compressed" => true, + "application/vnd.rar" => true, + "application/x-bzip2" => true, + "application/x-xz" => true, + _ => false + }; + + private static bool IsDocument(string mime) + { + var lower = mime.ToLowerInvariant(); + + if (lower.StartsWith("text/", StringComparison.Ordinal)) + return true; + + return lower switch + { + "application/msword" => true, + "application/vnd.openxmlformats-officedocument.wordprocessingml.document" => true, + "application/vnd.ms-excel" => true, + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" => true, + "application/vnd.ms-powerpoint" => true, + "application/vnd.openxmlformats-officedocument.presentationml.presentation" => true, + "application/vnd.oasis.opendocument.text" => true, + "application/vnd.oasis.opendocument.spreadsheet" => true, + "application/vnd.oasis.opendocument.presentation" => true, + "application/rtf" => true, + "application/json" => true, + "application/xml" => true, + "application/x-yaml" => true, + "application/yaml" => true, + _ => false + }; + } +} diff --git a/src/Netclaw.Configuration/NetclawPaths.cs b/src/Netclaw.Configuration/NetclawPaths.cs index f2902d2ab..3554706c0 100644 --- a/src/Netclaw.Configuration/NetclawPaths.cs +++ b/src/Netclaw.Configuration/NetclawPaths.cs @@ -70,11 +70,12 @@ public sealed class NetclawPaths public string DevicesPath => Path.Combine(ConfigDirectory, "devices.json"); public string LogsDirectory => Path.Combine(BasePath, "logs"); /// - /// Legacy path. Session logs now live inside each session's directory: - /// {SessionsDirectory}/{sanitized_id}/logs/. - /// Kept for migration detection and backward-compatible log search. + /// Per-session log files live at {SessionLogsDirectory}/{sanitized_id}/{timestamp}.log. + /// This tree is deliberately kept outside so + /// the agent's file_read tool (scoped to {session_dir}) cannot observe + /// its own audit trail. /// - public string LegacySessionLogsDirectory => Path.Combine(LogsDirectory, "sessions"); + public string SessionLogsDirectory => Path.Combine(LogsDirectory, "sessions"); public string DaemonLogPath => Path.Combine(LogsDirectory, "daemon.log"); public string SessionsDirectory => Path.Combine(BasePath, "sessions"); public string PidFilePath => Path.Combine(BasePath, "netclaw.pid"); @@ -118,8 +119,7 @@ public void EnsureDirectoriesExist() Directory.CreateDirectory(ConfigDirectory); Directory.CreateDirectory(WebhooksDirectory); Directory.CreateDirectory(LogsDirectory); - // Note: per-session log directories (sessions/{id}/logs/) are created - // on-demand by SessionLogActor, not pre-created here. + Directory.CreateDirectory(SessionLogsDirectory); Directory.CreateDirectory(AgentsDirectory); Directory.CreateDirectory(SessionsDirectory); Directory.CreateDirectory(BinDirectory); diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index 00bd12653..c9a4950fd 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -561,7 +561,38 @@ "ReadFiles": { "$ref": "#/$defs/ToolFilesystemAccessProfile" }, "WriteFiles": { "$ref": "#/$defs/ToolFilesystemAccessProfile" }, "AttachFiles": { "$ref": "#/$defs/ToolFilesystemAccessProfile" }, - "ApprovalPolicy": { "$ref": "#/$defs/ToolApprovalConfig" } + "ApprovalPolicy": { "$ref": "#/$defs/ToolApprovalConfig" }, + "ChannelAttachments": { "$ref": "#/$defs/ChannelAttachmentPolicy" } + }, + "additionalProperties": false + }, + "AttachmentCategory": { + "type": "string", + "enum": ["Image", "Pdf", "Document", "Archive", "Media", "Other"], + "description": "Coarse MIME classification used for per-audience attachment policy." + }, + "ChannelAttachmentPolicy": { + "type": "object", + "description": "Per-audience inbound channel attachment policy. Channel adapters query this before downloading or delivering any user-uploaded file.", + "properties": { + "AllowedCategories": { + "type": "array", + "items": { "$ref": "#/$defs/AttachmentCategory" }, + "default": [], + "description": "Attachment categories permitted for this audience. Empty rejects all inbound attachments." + }, + "MaxFileBytes": { + "type": "integer", + "minimum": 0, + "default": 26214400, + "description": "Maximum per-file size in bytes. Files whose transport-reported size exceeds this are rejected before download. Default: 25 MiB." + }, + "MaxFilesPerMessage": { + "type": "integer", + "minimum": 0, + "default": 10, + "description": "Maximum number of attachments accepted on a single inbound message. Messages exceeding this have all attachments rejected." + } }, "additionalProperties": false }, diff --git a/src/Netclaw.Configuration/ToolAudienceProfiles.cs b/src/Netclaw.Configuration/ToolAudienceProfiles.cs index 8d48ff68d..efa04037a 100644 --- a/src/Netclaw.Configuration/ToolAudienceProfiles.cs +++ b/src/Netclaw.Configuration/ToolAudienceProfiles.cs @@ -44,6 +44,16 @@ public sealed class ToolAudienceProfile /// approval before execution. Null means no approval gates (all tools auto-approved). /// public ToolApprovalConfig? ApprovalPolicy { get; set; } + + /// + /// Per-audience inbound channel attachment policy. Channel adapters read + /// this from the resolved profile to decide which attachment classes are + /// accepted, the per-file size cap, and the per-message file-count cap. + /// Defaults to (fail-closed: + /// nothing allowed) so an unconfigured profile rejects every attachment + /// until the operator opts in via the audience defaults. + /// + public ChannelAttachmentPolicy ChannelAttachments { get; set; } = ChannelAttachmentPolicy.Empty; } public sealed class ToolAudienceProfiles @@ -63,6 +73,39 @@ public IEnumerable GetAllProfiles() yield return Personal; } + /// + /// Validates per-audience channel attachment policy. A policy that + /// permits any category SHALL specify positive size and file-count caps, + /// otherwise an allowed category cannot be delivered (a silent + /// misconfiguration that this check converts into a loud startup error). + /// A policy with no allowed categories is valid with any cap (it is + /// already fail-closed). + /// + public IReadOnlyList ValidateChannelAttachments() + { + var errors = new List(); + ValidateProfile("Public", Public, errors); + ValidateProfile("Team", Team, errors); + ValidateProfile("Personal", Personal, errors); + return errors; + } + + private static void ValidateProfile(string name, ToolAudienceProfile profile, List errors) + { + var policy = profile.ChannelAttachments; + if (policy is null) + return; + + if (policy.AllowedCategories.Count == 0) + return; + + if (policy.MaxFileBytes <= 0) + errors.Add($"Tools.AudienceProfiles.{name}.ChannelAttachments.MaxFileBytes must be > 0 when AllowedCategories is not empty."); + + if (policy.MaxFilesPerMessage <= 0) + errors.Add($"Tools.AudienceProfiles.{name}.ChannelAttachments.MaxFilesPerMessage must be > 0 when AllowedCategories is not empty."); + } + /// /// Filesystem roots that are always readable regardless of audience profile. /// Supports tokens: {skills_dir}, {identity_dir}, {workspaces_dir}. @@ -97,7 +140,8 @@ public static class ToolAudienceProfileDefaults AllowedTools = ["file_read", "file_write", "attach_file"], ReadFiles = CreateSessionScopedFilesystemAccess(), WriteFiles = CreateSessionScopedFilesystemAccess(), - AttachFiles = CreateSessionScopedFilesystemAccess() + AttachFiles = CreateSessionScopedFilesystemAccess(), + ChannelAttachments = CreatePublicChannelAttachments() }; public static ToolAudienceProfile CreateTeam() => new() @@ -105,7 +149,8 @@ public static class ToolAudienceProfileDefaults AllowedTools = ["file_read", "attach_file"], ReadFiles = CreateSessionScopedFilesystemAccess(), WriteFiles = CreateSessionScopedFilesystemAccess(), - AttachFiles = CreateSessionScopedFilesystemAccess() + AttachFiles = CreateSessionScopedFilesystemAccess(), + ChannelAttachments = CreateTeamChannelAttachments() }; public static ToolAudienceProfile CreatePersonal() => new() @@ -114,7 +159,44 @@ public static class ToolAudienceProfileDefaults McpServersMode = ToolProfileMode.All, ReadFiles = new ToolFilesystemAccessProfile { Mode = ToolFilesystemMode.All }, WriteFiles = new ToolFilesystemAccessProfile { Mode = ToolFilesystemMode.All }, - AttachFiles = new ToolFilesystemAccessProfile { Mode = ToolFilesystemMode.All } + AttachFiles = new ToolFilesystemAccessProfile { Mode = ToolFilesystemMode.All }, + ChannelAttachments = CreatePersonalChannelAttachments() + }; + + public static ChannelAttachmentPolicy CreatePublicChannelAttachments() => new() + { + AllowedCategories = [AttachmentCategory.Image], + MaxFileBytes = ChannelAttachmentPolicy.DefaultMaxFileBytes, + MaxFilesPerMessage = ChannelAttachmentPolicy.DefaultMaxFilesPerMessage + }; + + public static ChannelAttachmentPolicy CreateTeamChannelAttachments() => new() + { + AllowedCategories = + [ + AttachmentCategory.Image, + AttachmentCategory.Pdf, + AttachmentCategory.Document, + AttachmentCategory.Archive, + AttachmentCategory.Media + ], + MaxFileBytes = ChannelAttachmentPolicy.DefaultMaxFileBytes, + MaxFilesPerMessage = ChannelAttachmentPolicy.DefaultMaxFilesPerMessage + }; + + public static ChannelAttachmentPolicy CreatePersonalChannelAttachments() => new() + { + AllowedCategories = + [ + AttachmentCategory.Image, + AttachmentCategory.Pdf, + AttachmentCategory.Document, + AttachmentCategory.Archive, + AttachmentCategory.Media, + AttachmentCategory.Other + ], + MaxFileBytes = ChannelAttachmentPolicy.DefaultMaxFileBytes, + MaxFilesPerMessage = ChannelAttachmentPolicy.DefaultMaxFilesPerMessage }; public static ToolFilesystemAccessProfile CreateSessionScopedFilesystemAccess() => new() diff --git a/src/Netclaw.Daemon/Gateway/SessionCatalogService.cs b/src/Netclaw.Daemon/Gateway/SessionCatalogService.cs index 39f7b4013..2916cf0df 100644 --- a/src/Netclaw.Daemon/Gateway/SessionCatalogService.cs +++ b/src/Netclaw.Daemon/Gateway/SessionCatalogService.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Logging; using Netclaw.Actors.Channels; using Netclaw.Actors.Protocol; +using Netclaw.Actors.Sessions; using Netclaw.Actors.Telemetry; using Netclaw.Configuration; @@ -55,10 +56,7 @@ public void OnSessionActivated(SessionId sessionId, Actors.Channels.ChannelType try { - // Compute expected log directory deterministically — the child SessionLogActor - // independently creates timestamped log files at this same directory. - var sanitized = SessionDirectoryHelper.SanitizeSessionId(sessionId.Value); - var logPath = Path.Combine(_paths.SessionsDirectory, sanitized, "logs"); + var logPath = SessionLogActor.GetSessionLogsDirectory(sessionId, _paths.SessionLogsDirectory); using var conn = new SqliteConnection(_connectionString); conn.Open(); diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 6e889d5c8..46a75713d 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -512,6 +512,13 @@ static void ConfigureDaemonServices( // Tools (auto-bound, no required properties) var toolConfig = configuration.GetSection("Tools") .Get() ?? new ToolConfig(); + var attachmentErrors = toolConfig.AudienceProfiles.ValidateChannelAttachments(); + if (attachmentErrors.Count > 0) + { + throw new InvalidOperationException( + "Invalid Tools.AudienceProfiles.ChannelAttachments configuration: " + + string.Join("; ", attachmentErrors)); + } services.AddSingleton(toolConfig); var securityPolicyConfig = configuration.GetSection("Security") @@ -818,7 +825,7 @@ static void ConfigureDaemonServices( sp.GetRequiredService(), sp.GetRequiredService>(), sp.GetRequiredService(), - sp.GetService())); + sp.GetRequiredService())); services.AddSingleton(sp => new SessionToolServices( sp.GetRequiredService(),