fix(skills): harden auto-learn pipeline and enable skills guard scan (#89)

Makes the skill auto-learn system safe to use by default:

1. Local-scan floor: ScanSuggestionBody, scanSkill, and applyGuardToSkills
   now always run the local rule scan via guard.ScanContentWithScope -
   previously 'skills scan disabled' (the default everywhere) meant zero
   scanning, so a learned skill body containing injection patterns saved
   and loaded unchecked.

2. NeedsReview is now enforced at trigger time: lazy matchers are built
   from the non-NeedsReview skill set, so flagged/tainted skills can no
   longer be trigger-injected on a single keyword match. Previously
   NeedsReview only pinned skills out of auto-load, leaving per-turn
   injection wide open.

3. Project-dir skills (./.odek/skills) are distrusted like ./odek.json:
   forced NeedsReview with a 'project' source, so a cloned repo can no
   longer auto-inject a SKILL.md. 'odek skill promote' now falls back to
   the project dir so these skills can be reviewed and promoted.

4. Taint coverage: web_search, vision, and delegate_tasks now taint the
   session (like browser/http_batch), so skills learned from such
   sessions are declined by auto-save. shell is deliberately excluded
   (primary work tool - tainting it would taint nearly every session).
   Auto-load skill context and skill_load tool output are now wrapped
   with the nonce'd untrusted-content boundary.

5. The 'skills' guard scan scope now defaults to enabled (config +
   docker configs + docs), adding the PiGuard semantic second opinion on
   top of the local floor at save and load time.

Tests: 12+ new/updated tests across skills, memory, guard, config, and
CLI packages; full suite, -race on skills, and repo-wide golangci-lint
all clean.

Author: jkyberneees

AGENTS.md

@@ -110,7 +110,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Untrusted-content wrapper** (`cmd/odek/untrusted.go`) — every tool whose output sources from outside the trust boundary (`browser`, `read_file`, `shell`, `search_files`, `multi_grep`, `transcribe`, `head_tail`, `diff`, `tr`, `sort`, `json_query`, `batch_patch`, `glob`, `file_info`, `tree`, `base64` file mode, `session_search`, `@-resources`, `--ctx` files, any MCP tool) wraps results in `<untrusted_content_<nonce> source="...">…</untrusted_content_<nonce>>`. Browser page title and interactive-element text are wrapped in addition to the main content. Per-call nonce defeats wrapper-escape via literal close tag.
 - **Audit log** (`cmd/odek/audit.go` + `internal/session/audit.go`) — every `wrapUntrusted` call records source + content-hash + turn into `<sessions>/audit/<id>.json`. After each turn a divergence heuristic flags `suspicious_divergence=true` when the agent ingested untrusted content AND its actions or final response reference resources that either did not appear in the user's message or were introduced by the untrusted content itself (closing response-only exfiltration and reused-resource injection bypasses). Inspect with `odek audit <session-id>` / `odek audit --list`.
 - **Memory taint** (`internal/memory/provenance.go`) — `EpisodeProvenance` tracks Untrusted/Sources/UserApproved. Tainted episodes are stored but `Search()` filters them out, so a one-shot injection cannot persist via the episode pipeline. User must explicitly promote.
-- **Skill provenance gate** (`internal/skills/loader.go` + `cache.go` + `tools.go`) — `Skill.Provenance{Untrusted, Sources, NeedsReview}`. NeedsReview skills pin to Lazy regardless of `auto_load`. The auto-save path declines tainted suggestions by default. Agent-created skills via `skill_save` and patched skills via `skill_patch` are forced to `Untrusted` + `NeedsReview`, and `skill_patch` refuses edits that touch the YAML frontmatter, blocking an injected agent from flipping `auto_load` or clearing `needs_review`. `odek skill promote <name> --force` clears the flag after explicit user review.
+- **Skill provenance gate** (`internal/skills/loader.go` + `cache.go` + `tools.go`) — `Skill.Provenance{Untrusted, Sources, NeedsReview}`. NeedsReview skills pin to Lazy regardless of `auto_load` and are excluded from lazy trigger matching (the matchers in `reloadLocked` are built only from promotable skills), so a flagged skill cannot be injected into context on a keyword match. Skills scanned from the project-local `./.odek/skills/` directory are distrusted like `./odek.json`: they are forced to `NeedsReview` with `"project"` in `Sources` until promoted. The auto-save path declines tainted suggestions by default. Agent-created skills via `skill_save` and patched skills via `skill_patch` are forced to `Untrusted` + `NeedsReview`, and `skill_patch` refuses edits that touch the YAML frontmatter, blocking an injected agent from flipping `auto_load` or clearing `needs_review`. All skill-body scans (load, save, patch, suggestion) go through `guard.ScanContentWithScope`, so the fast local rule scan runs even when the `skills` guard scope or the guard itself is disabled; the sidecar second opinion only runs when the scope is enabled (default: on). Taint derives from `memory.ToolCallTaints`, whose always-external set covers `browser` / `http_batch` / `transcribe` / `session_search` / `web_search` / `vision` / `delegate_tasks` (`shell` is deliberately excluded as the primary work tool). `odek skill promote <name> --force` clears the flag after explicit user review.
 - **Sub-agent damage cap** (`cmd/odek/subagent.go::applySubagentTrust`) — `delegate_tasks` carries `trust_level` + `max_risk`. The mutated `DangerousConfig` is passed into the sub-agent's `odek.Config` so the engine enforces the cap. Untrusted ⇒ NonInteractive=deny, Destructive/CodeExec/Install/SystemWrite/NetworkEgress/Unknown/Blocked all forced to Deny. `max_risk` ⇒ everything above cap forced to Deny. MCP tools are not loaded into untrusted sub-agents, because the MCP `ToolAdapter` does not perform its own danger check and would otherwise bypass the cap.
 - **FD-based API key handoff** (`cmd/odek/subagent_key.go`) — parent writes key to a 0600 tempfile, immediately `unlink()`s, passes the FD via `cmd.ExtraFiles`. Sub-agent reads from `$ODEK_API_KEY_FD` and closes. Key never in `/proc/<pid>/environ`.
 - **Approver friction** (`internal/danger/approver.go`, `cmd/odek/wsapprover.go`, `cmd/odek/shell.go`, `cmd/odek/perf_tools.go`) — both TTYApprover and WSApprover engage friction mode after 3 approvals of the same class in 60s: require typing literal `approve`, 1.5s pause. Trust-class shortcut disabled for `destructive` + `blocked` regardless. CLI `shell`/`parallel_shell` reuse a single TTYApprover instance per process so the counter and trust cache persist across prompts.
@@ -194,7 +194,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **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.
 - **Audit ingest recording for @-refs, `--ctx`, and Web-UI attachments** (`cmd/odek/refs.go`, `cmd/odek/serve.go`, `cmd/odek/audit.go`) — the per-session ingest recorder is attached before `@`-reference/`--ctx`/attachment resolution, `recordTurnAudit` scans `user` messages for untrusted wrappers in addition to `tool` messages, and the divergence heuristic compares agent actions against the original pre-enrichment prompt so attacker-injected resources are not treated as user-mentioned.
 - **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. `X-Forwarded-For` / `X-Real-Ip` headers are only honored when the direct remote address is in the configured `trusted_proxies` list, so spoofed headers cannot bypass the limiters.
-- **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.
+- **Skill/episode untrusted wrapper** (`internal/loop/loop.go` + `odek.go`) — skill context (both auto-load and lazy trigger-matched), the `skill_load` tool output, 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.
 - **Web UI attachment wrapping** (`cmd/odek/serve.go` + `cmd/odek/ui/app.js`) — files attached through the browser are sent separately from the user's text and wrapped with the nonce'd `<untrusted_content_*>` boundary (`source="attachment:<filename>"`) before injection into the prompt.
 - **Episode index session ID validation** (`internal/memory/episode_index.go` + `internal/session/session.go`) — `readAllSummaries` treats `index.json` as untrusted input and validates every `session_id` with `session.ValidateSessionID` before building the `filepath.Join(dir, sessionID+".md")` path. Invalid / traversal / separator-containing IDs are skipped with a warning, preventing a tampered episode index from pulling arbitrary files (e.g. `~/.odek/config.json`, `IDENTITY.md`) into the embedding space.

cmd/odek/injection_hardening_test.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
@@ -13,6 +15,7 @@ import (
 	"github.com/BackendStack21/odek/internal/danger"
 	"github.com/BackendStack21/odek/internal/guard"
 	"github.com/BackendStack21/odek/internal/loop"
+	"github.com/BackendStack21/odek/internal/skills"
 )
 
 func testSanitizeMCPDescription(server, tool, desc string) string {
@@ -360,6 +363,46 @@ func TestBuiltinTools_SessionSearchWrappedAsUntrusted(t *testing.T) {
 	}
 }
 
+// skill_load returns skill bodies, which are externally-sourced content
+// (project dirs, prior auto-saves). Its output must be wrapped as untrusted
+// at registration so a poisoned skill cannot pose as instructions.
+func TestBuiltinTools_SkillLoadWrappedAsUntrusted(t *testing.T) {
+	dir := t.TempDir()
+	skillDir := filepath.Join(dir, "test-skill")
+	if err := os.MkdirAll(skillDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	content := "---\nname: test-skill\ndescription: d\nodek:\n  auto_load: true\n---\n\n## Overview\nTest body.\n"
+	if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0644); err != nil {
+		t.Fatal(err)
+	}
+	sm := skills.NewSkillManager(dir, "")
+
+	tools := builtinTools(danger.DangerousConfig{}, sm, nil, 4, "", toolConfig{}, nil)
+
+	var sl odek.Tool
+	for _, tool := range tools {
+		if tool.Name() == "skill_load" {
+			sl = tool
+			break
+		}
+	}
+	if sl == nil {
+		t.Fatal("skill_load tool not found in builtinTools output")
+	}
+
+	out, err := sl.Call(`{"name":"test-skill"}`)
+	if err != nil {
+		t.Fatalf("skill_load: %v", err)
+	}
+	if !hasUntrustedWrapper(out) {
+		t.Errorf("skill_load output is not wrapped as untrusted: %s", out)
+	}
+	if !strings.Contains(out, "Test body.") {
+		t.Errorf("expected skill body in output, got: %s", out)
+	}
+}
+
 // ════════════════════════════════════════════════════════════════════════
 // Fix #5 — the source attribute cannot break out of the opening tag.
 // ════════════════════════════════════════════════════════════════════════

cmd/odek/main.go

@@ -1673,7 +1673,10 @@ func builtinTools(dc danger.DangerousConfig, sm *skills.SkillManager, approver d
 
 	if sm != nil {
 		tools = append(tools,
-			&skills.SkillLoadTool{Manager: sm},
+			// skill_load returns skill bodies, which are externally-sourced
+			// content (project dirs, prior auto-saves). Wrap the output as
+			// untrusted so a poisoned skill cannot pose as instructions.
+			&untrustedToolWrapper{inner: &skills.SkillLoadTool{Manager: sm}, source: "skill_load"},
 			&skills.SkillListTool{Manager: sm},
 			&skills.SkillSaveTool{Manager: sm},
 			&skills.SkillPatchTool{Manager: sm},

cmd/odek/skill_promote.go

@@ -10,7 +10,10 @@ import (
 )
 
 // promoteSkill clears Provenance.NeedsReview on a skill stored under
-// userDir, allowing it to be auto-loaded. The skill body is left
+// userDir, allowing it to be auto-loaded. If the skill is not in userDir,
+// the project skills dir (./.odek/skills) is searched as a fallback —
+// project-dir skills are forced NeedsReview at scan time (see ScanDirs),
+// so they must be promotable too. The skill body is left
 // unchanged. The user is expected to have read the body before
 // invoking this command; we do not enforce a confirmation prompt
 // because shipping a noisy mandatory `--yes` flag teaches users to
@@ -24,7 +27,12 @@ func promoteSkill(userDir, name string, force bool) error {
 	skillDir := filepath.Join(userDir, name)
 	path := filepath.Join(skillDir, "SKILL.md")
 	if _, err := os.Stat(path); err != nil {
-		return fmt.Errorf("promote: skill %q not found at %s", name, path)
+		projDir := filepath.Join(skills.ProjectSkillsDir(), name)
+		projPath := filepath.Join(projDir, "SKILL.md")
+		if _, perr := os.Stat(projPath); perr != nil {
+			return fmt.Errorf("promote: skill %q not found at %s", name, path)
+		}
+		skillDir, path = projDir, projPath
 	}
 
 	// We re-parse via the scanner to keep all other fields intact, then

cmd/odek/skill_promote_test.go

@@ -191,3 +191,29 @@ func TestScanSingleSkill_MissingFile(t *testing.T) {
 		t.Errorf("expected nil for missing file, got %+v", got)
 	}
 }
+
+func TestPromoteSkill_ProjectDirFallback(t *testing.T) {
+	// Project-dir skills are forced NeedsReview at scan time, so they must
+	// be promotable even though they don't live in the user dir.
+	t.Chdir(t.TempDir())
+	userDir := t.TempDir()
+	projSkills := ".odek/skills"
+	writeTestSkill(t, projSkills, "proj-skill",
+		"name: proj-skill\ndescription: a project skill\nodek:\n  provenance:\n    needs_review: true\n    sources: project\n",
+		"# body\n")
+
+	// Project skills carry Sources, so promotion requires --force.
+	if err := promoteSkill(userDir, "proj-skill", false); err == nil {
+		t.Fatal("expected refusal without --force for sourced project skill")
+	}
+	if err := promoteSkill(userDir, "proj-skill", true); err != nil {
+		t.Fatalf("promote with --force: %v", err)
+	}
+	data, err := os.ReadFile(filepath.Join(projSkills, "proj-skill", "SKILL.md"))
+	if err != nil {
+		t.Fatalf("read after promote: %v", err)
+	}
+	if strings.Contains(string(data), "needs_review: true") {
+		t.Errorf("needs_review should be cleared after promote, got:\n%s", data)
+	}
+}

docker/config.godmode.json

@@ -38,7 +38,7 @@
       "memory": true,
       "system_prompt": true,
       "mcp_descriptions": true,
-      "skills": false,
+      "skills": true,
       "tool_outputs": false,
       "telegram": false
     }

docker/config.restricted.json

@@ -38,7 +38,7 @@
       "memory": true,
       "system_prompt": true,
       "mcp_descriptions": true,
-      "skills": false,
+      "skills": true,
       "tool_outputs": false,
       "telegram": false
     }

docs/CONFIG.md

@@ -176,7 +176,7 @@ The guard is **off by default** in the sense that no sidecar is needed; the loca
       "memory": true,
       "system_prompt": true,
       "mcp_descriptions": true,
-      "skills": false,
+      "skills": true,
       "tool_outputs": false,
       "telegram": false
     }
@@ -203,11 +203,11 @@ The guard is **off by default** in the sense that no sidecar is needed; the loca
 | `memory` | `true` | `memory` add/replace/consolidate, legacy facts, auto-extracted facts, session buffer, and Extended Memory atom extraction/addition/recall/user-model inference |
 | `system_prompt` | `true` | `~/.odek/IDENTITY.md`, explicit `--system` / `ODEK_SYSTEM`, and project-level `AGENTS.md` |
 | `mcp_descriptions` | `true` | MCP server tool descriptions supplied via `tools/list` |
-| `skills` | `false` | Skill bodies loaded at startup; skill save/patch suggestions |
+| `skills` | `true` | Skill bodies loaded at startup; skill save/patch suggestions |
 | `tool_outputs` | `false` | External tool outputs wrapped as `<untrusted_content_*>` (warning-only scan) |
 | `telegram` | `false` | Telegram photo captions and voice transcripts before injection |
 
-When a scope is not explicitly set, the core surfaces (`memory`, `system_prompt`, `mcp_descriptions`) default to `true`; the optional expansion surfaces default to `false`.
+When a scope is not explicitly set, the core surfaces (`memory`, `system_prompt`, `mcp_descriptions`, `skills`) default to `true`; the optional expansion surfaces default to `false`. Regardless of scope, the fast local rule scan always runs on every guarded surface — the scope only toggles the sidecar second opinion.
 
 ### Examples