M7: preserve skill provenance through LLM enhancement and conversation extraction

Author: jkyberneees

AGENTS.md

@@ -178,6 +178,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **MCP inputSchema hardening** (`cmd/odek/mcp_approval.go`) — every string in an MCP tool's `inputSchema` is recursively guard-scanned for injection patterns; schemas larger than 256 KiB are rejected; the interactive approval prompt shows a SHA-256 hash and byte size of the schema so operators can detect changes.
 - **MCP tool batch classification** (`internal/loop/loop.go`) — MCP tools (`<server>__<tool>`) are classified as `unknown` by `classifyToolCall`, so the batch approval gate shows them and untrusted sub-agents force them to `deny`.
 - **Sub-agent trust defaults + delegate_tasks gate** (`cmd/odek/subagent.go`, `internal/loop/loop.go`) — a missing `trust_level` in `delegate_tasks` defaults to `untrusted`; `delegate_tasks` itself is classified as `system_write` so it requires explicit approval before spawning child processes.
+- **Skill learn-loop provenance propagation** (`internal/skills/learnloop.go`) — conversation-extracted suggestions and LLM-enhanced suggestions both retain the session's `SkillProvenance`, so tainted sessions cannot produce clean-looking auto-saved skills.
 - **Session ID entropy + session-scoped auth tokens** (`internal/session/session.go`, `cmd/odek/serve.go`) — session IDs now carry 128 bits of randomness (16 bytes / 32 hex chars); each session stores a 256-bit `AuthToken` required by `GET/DELETE/POST /api/sessions/<id>`, `POST /api/cancel`, and WebSocket session-resume messages via `X-Session-Token` header, `session_token` cookie, or `auth_token` WS field. Per-IP rate limiting (60/min) on session lookups adds a brute-force backstop.
 - **Skill/episode untrusted wrapper** (`internal/loop/loop.go` + `odek.go`) — skill context and retrieved session-episode context are passed through the caller-provided untrusted wrapper (the same nonce'd `<untrusted_content_*>` boundary used for tool output) before being injected into the model's system context. This prevents a compromised or tainted skill/episode from being treated as trusted system instructions.
 - **`env` / `printenv` environment-dump gate** (`internal/danger/classifier.go`) — bare `env` and `printenv` invocations are classified as `system_write` because they can leak process-environment secrets that the redaction scanner does not recognise. `env VAR=value <cmd>` still classifies `<cmd>` normally.

docs/SECURITY.md

@@ -589,6 +589,15 @@ Now:
 - `cmd/odek/subagent.go::applySubagentTrust` treats a missing `trust_level` as `untrusted`, forcing `non_interactive: deny` and denying Destructive/CodeExecution/Install/SystemWrite/NetworkEgress/Unknown/Blocked.
 - `internal/loop/loop.go::classifyToolCall` classifies `delegate_tasks` as `system_write`, so spawning sub-agents requires explicit operator approval and cannot be used to escape the parent's approval gate.
 
+### 39h. Skill learn-loop provenance propagation
+
+Pattern-detected suggestions in `internal/skills/learnloop.go` were tagged with `DeriveProvenance`, but conversation-extracted suggestions (`ExtractSkillsFromConversation`) were appended without provenance, and the LLM enhancement step (`GenerateSkillWithLLM`) replaced the whole `SkillSuggestion` with the LLM-generated version, dropping the provenance. A tainted session could therefore produce a clean-looking auto-saved skill.
+
+Now:
+
+- Conversation-extracted suggestions receive the session provenance before being added to the suggestion list.
+- The enhancement loop copies the original suggestion's `Provenance` onto the LLM-enhanced result, so `IsTainted()` remains true and `AutoSaveSuggestions` declines the skill unless `allowUntrusted` is set.
+
 ### 40. `/api/resources` result limit cap
 
 The `/api/resources?q=...&limit=N` autocomplete endpoint previously accepted any positive `limit` value. It is now capped to 100 results both in the HTTP handler and in `Registry.Search`. This prevents a prompt-injected or attacker-forged request from forcing an unbounded directory walk and returning a multi-megabyte JSON response.

internal/skills/learnloop.go

@@ -41,15 +41,19 @@ func AnalyzeMessages(messages []LlmMessage, userMessages []string, sm *SkillMana
 				cmds = append(cmds, c.Input)
 			}
 			convSkill.CommandLog = cmds
+			convSkill.Provenance = prov
 			suggestions = append(suggestions, *convSkill)
 		}
 	}
 
-	// Apply LLM enhancement to each suggestion.
+	// Apply LLM enhancement to each suggestion. The LLM only produces name,
+	// description, and body — it must not be allowed to strip the provenance
+	// that was derived from the session's tool calls.
 	if llmLearn && llmClient != nil {
 		calls := ExtractToolCalls(messages)
 		for i := range suggestions {
 			if enhanced := GenerateSkillWithLLM(llmClient, calls, userMessages, suggestions[i].Heuristic); enhanced != nil {
+				enhanced.Provenance = suggestions[i].Provenance
 				enhanced.CommandLog = suggestions[i].CommandLog
 				enhanced.Heuristic = suggestions[i].Heuristic
 				suggestions[i] = *enhanced

internal/skills/learnloop_test.go

@@ -2,6 +2,7 @@ package skills
 
 import (
 	"bytes"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -132,3 +133,97 @@ func TestRunAutoSaveLoop_VerboseWriterReceivesFailedMessage(t *testing.T) {
 		t.Errorf("verbose output should mention Quality gate failed for empty-body suggestion, got:\n%s", buf.String())
 	}
 }
+
+// validLLMSkillResponse returns the exact format GenerateSkillWithLLM and
+// ExtractSkillsFromConversation expect, with a body long enough to pass the
+// quality gate.
+func validLLMSkillResponse(name string) string {
+	return fmt.Sprintf(`NAME: %s
+DESCRIPTION: A test skill
+TOPICS: test, ci
+ACTIONS: create, run
+BODY:
+## Overview
+This is a test skill body with enough padding to exceed the two hundred character minimum required by the quality gate. Keep typing filler words until we are safely over the limit for sure.
+
+## Step-by-Step
+1. Do the thing
+
+## Common Pitfalls
+- Nothing
+
+## Verification
+- Check output
+`, name)
+}
+
+// TestAnalyzeMessages_PreservesProvenanceThroughEnhancement verifies that
+// LLM-enhanced suggestions keep the provenance derived from the session, so
+// tainted sessions cannot produce clean-looking auto-saved skills.
+func TestAnalyzeMessages_PreservesProvenanceThroughEnhancement(t *testing.T) {
+	sm := NewSkillManager(t.TempDir(), t.TempDir())
+	messages := []LlmMessage{
+		{Role: "user", Content: "fetch a page"},
+		{Role: "assistant", Content: "ok", ToolCalls: []LlmToolCall{
+			{Function: struct {
+				Name      string
+				Arguments string
+			}{Name: "browser", Arguments: `{"action":"navigate","url":"https://example.com"}`}},
+		}},
+		{Role: "tool", Content: "page text"},
+	}
+	userMsgs := []string{"fetch a page"}
+
+	llm := &mockLLMClient{resp: validLLMSkillResponse("enhanced-skill")}
+	got := AnalyzeMessages(messages, userMsgs, sm, llm, true, true)
+	if len(got) == 0 {
+		t.Fatal("expected at least one suggestion")
+	}
+	for _, s := range got {
+		if !s.Provenance.Untrusted {
+			t.Errorf("suggestion %q lost tainted provenance after LLM enhancement: %+v", s.Name, s.Provenance)
+		}
+		if len(s.Provenance.Sources) == 0 || s.Provenance.Sources[0] != "browser" {
+			t.Errorf("suggestion %q lost provenance sources after LLM enhancement: %+v", s.Name, s.Provenance.Sources)
+		}
+	}
+}
+
+// TestAnalyzeMessages_ConversationExtractedIsTagged verifies that skills
+// extracted from the full conversation receive the session provenance before
+// they enter the auto-save pipeline.
+func TestAnalyzeMessages_ConversationExtractedIsTagged(t *testing.T) {
+	sm := NewSkillManager(t.TempDir(), t.TempDir())
+	messages := []LlmMessage{
+		{Role: "user", Content: "how do I do this"},
+		{Role: "assistant", Content: "use curl", ToolCalls: []LlmToolCall{
+			{Function: struct {
+				Name      string
+				Arguments string
+			}{Name: "http_batch", Arguments: `{"urls":["https://example.com"]}`}},
+		}},
+		{Role: "tool", Content: "ok"},
+	}
+	userMsgs := []string{"how do I do this"}
+
+	llm := &mockLLMClient{resp: validLLMSkillResponse("conversation-skill")}
+	got := AnalyzeMessages(messages, userMsgs, sm, llm, true, true)
+	if len(got) == 0 {
+		t.Fatal("expected at least one suggestion")
+	}
+	found := false
+	for _, s := range got {
+		if s.Heuristic == "conversation-extracted" {
+			found = true
+			if !s.Provenance.Untrusted {
+				t.Errorf("conversation-extracted skill %q should be tainted, got %+v", s.Name, s.Provenance)
+			}
+		}
+	}
+	if !found {
+		t.Errorf("expected a conversation-extracted suggestion, got heuristics: ")
+		for _, s := range got {
+			t.Logf("  %s: %s", s.Name, s.Heuristic)
+		}
+	}
+}