diff --git a/AGENTS.md b/AGENTS.md index 660b579..9771a93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,7 +132,7 @@ System prompt priority: `--system` flag > `~/.odek/IDENTITY.md` > compiled-in de Layered prompt-injection / approval-fatigue defenses. The full per-mitigation list lives in [docs/SECURITY.md](docs/SECURITY.md); `cmd/odek/security_report_validation_test.go` is the regression bar. Summary by layer: - **Untrusted-content boundary** (`cmd/odek/untrusted.go`) — every externally-sourced tool result (browser, file/shell/search tools, MCP, session_search, @-refs, --ctx, attachments) is wrapped in a per-call nonce'd `>` tag; tool-result delimiters are also nonce'd (`internal/loop`). Skill/episode context injected into the system prompt is wrapped too. The per-session audit log (`cmd/odek/audit.go`) records every ingest and flags divergence between user-mentioned resources and agent actions. -- **Provenance gates** — tainted memory episodes are stored but never auto-replayed; skills from untrusted sources (agent-created, project `./.odek/skills/`, LLM-suggested) are pinned `NeedsReview` until `odek skill promote --force`, excluded from trigger matching, and blocked from frontmatter edits via `skill_patch`. Skill auto-save applies recurrence, near-duplicate, secret-scan, and scope gates (project skills never promoted to global). `odek` self-invocation via shell is `system_write` so the agent can't reach its own trust mutations. +- **Provenance gates** — tainted memory episodes are stored but never auto-replayed; skills from untrusted sources (agent-created, project `./.odek/skills/`, LLM-suggested) are pinned `NeedsReview` until `odek skill promote --force`, excluded from trigger matching, and blocked from frontmatter edits via `skill_patch`. Skill auto-save applies a substance bar (multi-step sequences dominated by read-only inspection commands, or carrying oversized/multi-line session-specific steps, are rejected as exploration transcripts), content-keyed recurrence (fingerprint = heuristic + name + normalized command-log digest), near-duplicate, secret-scan, and scope gates (project skills never promoted to global). `odek` self-invocation via shell is `system_write` so the agent can't reach its own trust mutations. - **Danger classifier** (`internal/danger/classifier.go`) — bypass-resistant normalization ($IFS, command substitution, wrappers, backslashes, basenames); covers awk/sed/editor escapes, pipe-fed xargs composition, root-level mutation targets, git data-loss verbs, `gh` as network egress, `git -c`/config code exec, find/rsync destructive flags, env dumps, shell operand/redirect path classification (writes to shell rc files, ~/.ssh, ~/.odek escalate to system_write). Trust anchors under `~/.odek` are write-protected from generic file tools. - **Approval friction** — TTY/WS/Telegram approvers engage friction after 3 same-class approvals in 60s (type `approve`, pause, trust shortcut hidden); `destructive`/`blocked`/`unknown` never get trust shortcuts. TTY prompts are process-wide serialized. - **Sub-agent caps** — `delegate_tasks` carries trust_level + max_risk enforced via the sub-agent's DangerousConfig; MCP tools withheld from untrusted sub-agents; API keys handed off via unlinked-tempfile FD, never env. diff --git a/cmd/odek/main_test.go b/cmd/odek/main_test.go index 84d5c10..fb55039 100644 --- a/cmd/odek/main_test.go +++ b/cmd/odek/main_test.go @@ -1525,7 +1525,9 @@ func TestParseReplFlags_ExtraArgsIgnored(t *testing.T) { // multiTurnServer returns an httptest server that simulates a multi-turn // conversation: n terminal tool calls followed by a final text response. -// Each tool call executes echo step N (safe, no side effects). +// Each tool call executes `true step N` — exits 0 with no side effects, and +// is an action verb so the multi-step substance bar lets the sequence +// through (a pure-inspection sequence like `echo ...` is now rejected). // Handles /models discovery requests from llm.DiscoverModelContext. func multiTurnServer(t *testing.T, terminalCalls int) *httptest.Server { t.Helper() @@ -1542,7 +1544,7 @@ func multiTurnServer(t *testing.T, terminalCalls int) *httptest.Server { callCount++ w.Header().Set("Content-Type", "application/json") if callCount <= terminalCalls { - fmt.Fprintf(w, `{"choices":[{"message":{"content":"Running step %d.","tool_calls":[{"id":"call_%d","function":{"name":"shell","arguments":"{\"command\":\"echo step %d\"}"}}]}}]}`, + fmt.Fprintf(w, `{"choices":[{"message":{"content":"Running step %d.","tool_calls":[{"id":"call_%d","function":{"name":"shell","arguments":"{\"command\":\"true step %d\"}"}}]}}]}`, callCount, callCount, callCount) } else { w.Write([]byte(`{"choices":[{"message":{"content":"All steps completed successfully."}}]}`)) @@ -1608,7 +1610,7 @@ func TestRunLearn_MultiStepProcedure(t *testing.T) { } // Skill file written to disk — poll since the goroutine may still be writing. - skillDir := filepath.Join(homeDir, ".odek", "skills", "procedure-echo") + skillDir := filepath.Join(homeDir, ".odek", "skills", "procedure-true") skillFile := filepath.Join(skillDir, "SKILL.md") for i := 0; i < 10; i++ { if _, err := os.Stat(skillFile); err == nil { @@ -1678,7 +1680,7 @@ func TestRunLearn_InteractiveReject(t *testing.T) { } // Verify no skill file written - skillDir := filepath.Join(homeDir, ".odek", "skills", "procedure-echo") + skillDir := filepath.Join(homeDir, ".odek", "skills", "procedure-true") skillFile := filepath.Join(skillDir, "SKILL.md") if _, err := os.Stat(skillFile); !os.IsNotExist(err) { t.Errorf("skill file should NOT exist after rejection: %s", skillFile) diff --git a/docs/LEARNING.md b/docs/LEARNING.md index 49666f7..d330529 100644 --- a/docs/LEARNING.md +++ b/docs/LEARNING.md @@ -60,7 +60,7 @@ odek run --learn "set up CI with GitHub Actions" - **Learning is non-blocking** — skill detection and auto-save run in a background goroutine after the agent's response is delivered. The process exits immediately; learning completes asynchronously on a best-effort basis. - **Tainted skills require explicit promotion** — skills learned from `browser`, MCP tools, or sensitive file reads are saved with `Provenance.Untrusted=true` and `NeedsReview=true`. They cannot be auto-loaded until you run `odek skill promote --force` after reviewing the body. - **Scope gates keep garbage out** — machine-specific suggestions (absolute home-directory paths) are dropped entirely. Project-specific suggestions (repo-rooted `./scripts/...` invocations, hardcoded release version tags) are redirected to the project skills dir `./.odek/skills` instead of the global `~/.odek/skills` — project-related skills are never promoted to global, and project-dir skills stay pinned to `NeedsReview` until promoted locally. Auto-curation (merge/prune) is confined to the global dir, so project skills can never be merged into or deleted by the global curator. -- **Recurrence before persistence** — a pattern must be seen in at least `auto_save.min_occurrences` distinct sessions (default 2) before it is eligible for auto-save; first sightings are recorded in `~/.odek/skills/.candidates.json` and reported as pending. +- **Recurrence before persistence** — a pattern must be seen in at least `auto_save.min_occurrences` distinct sessions (default 2) before it is eligible for auto-save; first sightings are recorded in `~/.odek/skills/.candidates.json` and reported as pending. The recurrence fingerprint keys on heuristic + name + a digest of the normalized command log, so two unrelated sessions that merely share a generic name (`procedure-ls`) do not count as a recurrence. - **Save-time hygiene** — suggestions are score-ranked (verification section, command-log evidence, LLM-judged) so the `max_per_run` budget goes to the strongest candidates; near-duplicates of existing skills (Jaccard word-set similarity ≥ 0.85) are skipped instead of creating parallel skills; and every SKILL.md write is secret-scanned, with detected credentials replaced by `[REDACTED]` and the skill pinned to `NeedsReview`. - **LLM curation** — with `llm_curate` enabled, merge bodies are synthesized by the model (deduplicated, coherent) instead of mechanically concatenated; failures fall back to concatenation. @@ -167,6 +167,10 @@ Detects **4 or more sequential successful terminal calls**. Failed commands brea - 4+ consecutive `shell` tool calls that all succeeded (no `error:` in output) - Non-terminal tools (read_file, write_file, etc.) are skipped — they don't count but don't break the sequence +**Substance bar (keeps exploration transcripts out):** +- Every step must be a short single-line command (≤ 200 chars) — a step carrying a session-specific payload (full commit message, heredoc) disqualifies the whole sequence. +- Read-only inspection commands (`ls`, `grep`, `cat`, `head`, `sed -n`, `echo`, `git status/log/diff`, `go test/vet`, …) must not dominate: if more than half the steps only observe state, the sequence was exploration, not a reusable procedure, and no suggestion is made. + **Example:** ```bash git clone https://github.com/example/repo.git # step 1 diff --git a/internal/skills/candidates.go b/internal/skills/candidates.go index 39a6853..613f005 100644 --- a/internal/skills/candidates.go +++ b/internal/skills/candidates.go @@ -1,9 +1,12 @@ package skills import ( + "crypto/sha256" + "encoding/hex" "encoding/json" "os" "path/filepath" + "strings" "time" ) @@ -13,9 +16,12 @@ import ( // CandidateStore persists how often each suggestion fingerprint has been // seen across sessions (/.candidates.json); a suggestion is only // eligible for saving once its count reaches -// AutoSaveConfig.MinOccurrences. Suggestion names are deterministic per -// pattern ("corrected-git", "procedure-docker", ...), so heuristic+name is -// a stable fingerprint even though body text varies between sessions. +// AutoSaveConfig.MinOccurrences. The fingerprint keys on heuristic, name, +// AND a digest of the normalized command log — keying on the name alone +// let unrelated sessions collide ("procedure-ls" fires in any session that +// starts with `ls`), so the recurrence gate passed without the pattern +// actually recurring. With the content digest, only the same normalized +// command pattern seen again counts as a recurrence. // CandidateFileName is the store file inside the skills user dir. const CandidateFileName = ".candidates.json" @@ -36,9 +42,17 @@ type CandidateStore struct { Candidates map[string]CandidateEntry `json:"candidates"` } -// candidateFingerprint identifies a suggestion across sessions. +// candidateFingerprint identifies a suggestion across sessions: heuristic +// and name, plus a short digest of the normalized command log so that two +// unrelated sessions sharing only a generic name ("procedure-ls") do not +// count as the same pattern recurring. func candidateFingerprint(s SkillSuggestion) string { - return s.Heuristic + "|" + s.Name + norm := make([]string, 0, len(s.CommandLog)) + for _, c := range s.CommandLog { + norm = append(norm, normalizeCommand(c)) + } + sum := sha256.Sum256([]byte(strings.Join(norm, "\n"))) + return s.Heuristic + "|" + s.Name + "|" + hex.EncodeToString(sum[:])[:12] } // LoadCandidates reads the candidate store from disk. A missing or diff --git a/internal/skills/candidates_test.go b/internal/skills/candidates_test.go index 9adc28b..ce20d2e 100644 --- a/internal/skills/candidates_test.go +++ b/internal/skills/candidates_test.go @@ -64,6 +64,47 @@ func TestAutoSaveSuggestions_RecurrenceDisabled(t *testing.T) { } } +// TestCandidateFingerprint_KeysOnCommandLog pins the fix for generic-name +// collisions: two sessions that both yield "procedure-ls" but ran +// different commands are NOT the same pattern recurring, so the gate must +// keep them pending forever. +func TestCandidateFingerprint_KeysOnCommandLog(t *testing.T) { + a := SkillSuggestion{Name: "procedure-ls", Heuristic: "multi-step", CommandLog: []string{"ls -d cmd/*", "go test ./...", "git status", "make build"}} + b := SkillSuggestion{Name: "procedure-ls", Heuristic: "multi-step", CommandLog: []string{"ls -la", "grep -rn foo .", "git log", "git push"}} + if candidateFingerprint(a) == candidateFingerprint(b) { + t.Error("same generic name with different command logs must not share a fingerprint") + } + if candidateFingerprint(a) != candidateFingerprint(a) { + t.Error("identical suggestion must have a stable fingerprint") + } +} + +// TestAutoSaveSuggestions_NoFalseRecurrence: the same generic suggestion +// name from two unrelated sessions must not reach MinOccurrences. +func TestAutoSaveSuggestions_NoFalseRecurrence(t *testing.T) { + dir := t.TempDir() + cfg := DefaultSkillsConfig() // MinOccurrences: 2 + cfg.AutoSave.MaxPerRun = 5 + + session1 := []SkillSuggestion{ + {Name: "procedure-ls", Heuristic: "multi-step", Body: recurringBody, CommandLog: []string{"ls -d cmd/*", "go build ./...", "git status", "make test"}}, + } + session2 := []SkillSuggestion{ + {Name: "procedure-ls", Heuristic: "multi-step", Body: recurringBody, CommandLog: []string{"ls -la /tmp", "go build ./cmd/app", "git diff", "make lint"}}, + } + + if r := AutoSaveSuggestions(session1, dir, "", cfg, nil, guard.Config{}, false); len(r.Pending) != 1 { + t.Fatalf("session 1 should pend, got %+v", r) + } + r := AutoSaveSuggestions(session2, dir, "", cfg, nil, guard.Config{}, false) + if len(r.Saved) != 0 { + t.Errorf("unrelated sessions sharing a generic name must not trigger recurrence, saved %v", r.Saved) + } + if len(r.Pending) != 1 { + t.Errorf("session 2 should also pend as a distinct pattern, got %+v", r) + } +} + // TestCandidateStore_PrunesStaleEntries covers the age-based pruning that // bounds the store file. func TestCandidateStore_PrunesStaleEntries(t *testing.T) { diff --git a/internal/skills/selfimprove.go b/internal/skills/selfimprove.go index 6680b0a..bda4c83 100644 --- a/internal/skills/selfimprove.go +++ b/internal/skills/selfimprove.go @@ -334,6 +334,9 @@ func buildSuggestionFromSequence(seq []ToolCall, heuristic string) *SkillSuggest if len(seq) < 4 { return nil } + if !isReusableProcedure(seq) { + return nil + } topic := extractTopic(seq[0].Input) var steps []string @@ -350,6 +353,122 @@ func buildSuggestionFromSequence(seq []ToolCall, heuristic string) *SkillSuggest } } +// maxProcedureStepLen caps one step's command length. Longer commands are +// session-specific payloads (full commit messages, heredocs, one-off +// pipelines) that can never be replayed as written. +const maxProcedureStepLen = 200 + +// inspectionVerbs are read-only commands: they observe state but change +// nothing. A sequence dominated by them is an exploration transcript, not +// a reusable procedure. +var inspectionVerbs = map[string]bool{ + "ls": true, "cat": true, "head": true, "tail": true, "grep": true, + "find": true, "wc": true, "echo": true, "printf": true, "pwd": true, + "file": true, "stat": true, "tree": true, "which": true, "env": true, + "gofmt": true, "sort": true, "uniq": true, "diff": true, "jq": true, + "less": true, "more": true, +} + +// gitReadOnlySubcommands are the git verbs that only observe repository +// state; every other subcommand (commit, push, tag, add, ...) mutates and +// counts as an action. +var gitReadOnlySubcommands = map[string]bool{ + "status": true, "log": true, "diff": true, "show": true, "blame": true, + "describe": true, "rev-parse": true, "ls-files": true, "grep": true, + "shortlog": true, "branch": true, "remote": true, +} + +// isInspectionCommand reports whether a command only observes state. +// Pipelines and separators are judged by their lead command — `grep foo +// file | head` is inspection, `go test ./... | tail` is an action. +func isInspectionCommand(cmd string) bool { + verb := leadVerb(cmd) + switch verb { + case "git": + return gitLeadSubcommandIsReadOnly(cmd) + case "go": + // `go test/vet` verify; `go build/install/run` produce effects. + // Neither dominates a real procedure on its own, but test/vet are + // the verbs that pad exploration transcripts, so count them as + // inspection. + sub := secondToken(cmd) + return sub == "test" || sub == "vet" || sub == "list" || sub == "env" || sub == "doc" + case "sed", "awk": + // Without -i, sed/awk only print. + return !strings.Contains(cmd, "-i") + } + return inspectionVerbs[verb] +} + +// secondToken returns the first non-flag token after the lead verb, or "". +func secondToken(cmd string) string { + fields := strings.Fields(strings.TrimSpace(cmd)) + seenVerb := false + for _, f := range fields { + if isPlumbingToken(f) { + continue + } + if !seenVerb { + seenVerb = true + continue + } + if strings.HasPrefix(f, "-") { + continue + } + return strings.Trim(f, "\"'`") + } + return "" +} + +// gitLeadSubcommandIsReadOnly extracts the subcommand of the leading `git` +// invocation (skipping global flags and their values) and reports whether +// it only reads state. Unrecognized subcommands count as actions — a +// conservative default, since unknown git verbs usually mutate. +func gitLeadSubcommandIsReadOnly(cmd string) bool { + fields := strings.Fields(strings.TrimSpace(cmd)) + seenGit := false + skipNext := false + for _, f := range fields { + if !seenGit { + if f == "git" { + seenGit = true + } + continue + } + if skipNext { + skipNext = false + continue + } + if strings.HasPrefix(f, "-") { + // Global flags that take a separate value (-C dir, -c k=v). + if f == "-C" || f == "-c" { + skipNext = true + } + continue + } + return gitReadOnlySubcommands[strings.Trim(f, "\"'`")] + } + return false +} + +// isReusableProcedure applies the substance bar to a candidate sequence: +// every step must be a short single-line command (no embedded transcripts), +// and read-only inspection commands must not dominate — a session that was +// mostly `ls`/`grep`/`sed -n` exploration is not a reusable procedure, no +// matter how often its first verb recurs. +func isReusableProcedure(seq []ToolCall) bool { + inspection := 0 + for _, c := range seq { + if strings.Contains(c.Input, "\n") || len(c.Input) > maxProcedureStepLen { + return false + } + if isInspectionCommand(c.Input) { + inspection++ + } + } + return inspection*2 <= len(seq) +} + func generateProcedureBody(topic string, steps []string) string { var b strings.Builder fmt.Fprintf(&b, "## Overview\n\nProcedure for: %s\n\n", topic) diff --git a/internal/skills/selfimprove_test.go b/internal/skills/selfimprove_test.go index 4a90936..30b9f67 100644 --- a/internal/skills/selfimprove_test.go +++ b/internal/skills/selfimprove_test.go @@ -2,6 +2,7 @@ package skills import ( "fmt" + "strings" "testing" "github.com/BackendStack21/odek/internal/guard" @@ -131,6 +132,73 @@ func TestDetectMultiStepProcedure_FailureBreaksSequence(t *testing.T) { } } +// TestDetectMultiStepProcedure_RejectsExplorationTranscript pins the +// substance bar: a session that was mostly read-only inspection +// (ls/grep/sed -n/head/echo/git status/go test) is an exploration +// transcript, not a reusable procedure, and must not become a skill. +func TestDetectMultiStepProcedure_RejectsExplorationTranscript(t *testing.T) { + calls := []ToolCall{ + {Tool: "shell", Input: "ls -d cmd/* internal/*", ExitCode: 0}, + {Tool: "shell", Input: "head -15 internal/foo/SKILL.md", ExitCode: 0}, + {Tool: "shell", Input: "go test ./internal/foo/... -count=1 2>&1 | tail -15", ExitCode: 0}, + {Tool: "shell", Input: "grep -rn \"validate\" internal/foo/*_test.go | head", ExitCode: 0}, + {Tool: "shell", Input: "sed -n 40,140p internal/foo/foo_test.go; echo ---; grep -n foo internal/foo/bar.go", ExitCode: 0}, + {Tool: "shell", Input: "git status --short && git log --oneline -3", ExitCode: 0}, + } + if got := DetectMultiStepProcedure(calls); len(got) != 0 { + t.Errorf("exploration transcript must not produce a skill, got %+v", got) + } +} + +// TestDetectMultiStepProcedure_RejectsOversizedStep: a step carrying a +// session-specific payload (a full commit message) can never be replayed +// as written — the whole sequence is transcript, not procedure. +func TestDetectMultiStepProcedure_RejectsOversizedStep(t *testing.T) { + longCommit := `git commit -m "` + strings.Repeat("very detailed session-specific message ", 10) + `"` + calls := []ToolCall{ + {Tool: "shell", Input: "npm install", ExitCode: 0}, + {Tool: "shell", Input: "npm run build", ExitCode: 0}, + {Tool: "shell", Input: "npm test", ExitCode: 0}, + {Tool: "shell", Input: longCommit, ExitCode: 0}, + } + if got := DetectMultiStepProcedure(calls); len(got) != 0 { + t.Errorf("sequence with oversized one-off step must be rejected, got %+v", got) + } +} + +// TestDetectMultiStepProcedure_RejectsMultilineStep: multi-line commands +// (heredocs, embedded scripts) are session transcripts, not steps. +func TestDetectMultiStepProcedure_RejectsMultilineStep(t *testing.T) { + calls := []ToolCall{ + {Tool: "shell", Input: "npm install", ExitCode: 0}, + {Tool: "shell", Input: "npm run build", ExitCode: 0}, + {Tool: "shell", Input: "cat < out.txt\nhello\nEOF", ExitCode: 0}, + {Tool: "shell", Input: "npm test", ExitCode: 0}, + } + if got := DetectMultiStepProcedure(calls); len(got) != 0 { + t.Errorf("sequence with multi-line step must be rejected, got %+v", got) + } +} + +// TestDetectMultiStepProcedure_AcceptsActionSequence: a genuine build / +// release procedure (mostly mutating commands) must still be detected. +func TestDetectMultiStepProcedure_AcceptsActionSequence(t *testing.T) { + calls := []ToolCall{ + {Tool: "shell", Input: "git status --short", ExitCode: 0}, + {Tool: "shell", Input: "make build", ExitCode: 0}, + {Tool: "shell", Input: "make test", ExitCode: 0}, + {Tool: "shell", Input: "git tag v1.2.3", ExitCode: 0}, + {Tool: "shell", Input: "git push --tags", ExitCode: 0}, + } + got := DetectMultiStepProcedure(calls) + if len(got) != 1 { + t.Fatalf("action sequence should produce one suggestion, got %d", len(got)) + } + if got[0].Heuristic != "multi-step" { + t.Errorf("Heuristic = %q", got[0].Heuristic) + } +} + func TestDetectErrorRecovery_Found(t *testing.T) { calls := []ToolCall{ {Tool: "terminal", Input: "docker build .", ExitCode: 1, Turn: 0},