Skip to content

Commit b7932ba

Browse files
jkyberneeesjkyberneees
committed
v0.8.3: 8 bug fixes + 10 regression tests
Bug fixes: - Config: KODE_SKILLS_LEARN / --learn no longer clobber skills config (merge instead of replace) - Loop: SkillLoader now fires once per unique user message, not every iteration (context leak) - Skills: LastUsed removed from scanDir (was always set to now(), breaking staleness detection) - Skills: RecordUsage method added — tracks LastUsed + UsageCount when skill is actually loaded - Skills: normalizeCommand now preserves flag=value tokens (was losing -count=1 etc.) - Danger: Duplicate fork bomb check removed from isBlocked (isRawBlocked catches it first) - Danger: isSystemPath helper extracted — shared by isSystemWrite and hasSystemRedirectTarget - Danger: Fixed misleading comment claiming Safe/LocalWrite overrides were impossible - CLI: Dead "kode skill save" subcommand removed (returned "not yet supported" error) - Learn: SkillManager.reload() replaced with Reload() export; learn loop uses it directly Tests (10 new): - TestLoadConfig_SkillsLearnEnvDoesNotClobberSkillsConfig - TestLoadConfig_SkillsLearnCLIDoesNotClobberSkillsConfig - TestEngine_SkillLoader_CalledOncePerInput - TestRecordUsage_UpdatesLastUsedAndUsageCount - TestRecordUsage_Concurrent (race-free) - TestNormalizeCommand expanded (boolean flags, =value tokens) - TestIsSystemPath (15 cases) - TestClassify_ForkBomb_StillDetected - TestClassify_SystemRedirectTarget
1 parent 2a08439 commit b7932ba

13 files changed

Lines changed: 401 additions & 44 deletions

File tree

‎cmd/kode/main.go‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -892,10 +892,8 @@ func runLearnLoop(messages []llm.Message, task string, sm *skills.SkillManager)
892892
fmt.Fprintf(os.Stderr, " ✗ Error saving skill: %v\n", err)
893893
} else {
894894
fmt.Fprintf(os.Stderr, " ✓ Saved skill %q\n", s.Name)
895-
// Reload the skill manager
896-
sm2 := skills.NewSkillManager(userDir, "./.kode/skills")
897-
// Update the caller's manager reference if possible
898-
_ = sm2
895+
// Reload the skill manager to pick up the new skill
896+
sm.Reload()
899897
}
900898
} else {
901899
fmt.Fprintf(os.Stderr, " Skipped.\n")
@@ -954,12 +952,6 @@ func skillCmd(args []string) error {
954952
fmt.Println(result)
955953
return nil
956954

957-
case "save":
958-
if len(subArgs) < 3 {
959-
return fmt.Errorf("usage: kode skill save --name <name> --description <desc> --body <body>")
960-
}
961-
return fmt.Errorf("interactive save not yet supported — use the agent's skill_save tool")
962-
963955
case "delete":
964956
if len(subArgs) == 0 {
965957
return fmt.Errorf("usage: kode skill delete <name>")

‎internal/config/loader.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,10 @@ func LoadConfig(cli CLIFlags) ResolvedConfig {
332332
// Skills env vars
333333
if v := envString("SKILLS_LEARN"); v != "" {
334334
b, _ := strconv.ParseBool(v)
335-
cfg.Skills = &SkillsConfig{Learn: &b}
335+
if cfg.Skills == nil {
336+
cfg.Skills = &SkillsConfig{}
337+
}
338+
cfg.Skills.Learn = &b
336339
}
337340

338341
// Layer 4: CLI flags (highest priority)
@@ -358,7 +361,10 @@ func LoadConfig(cli CLIFlags) ResolvedConfig {
358361
cfg.NoAgents = cli.NoAgents
359362
}
360363
if cli.Learn != nil {
361-
cfg.Skills = &SkillsConfig{Learn: cli.Learn}
364+
if cfg.Skills == nil {
365+
cfg.Skills = &SkillsConfig{}
366+
}
367+
cfg.Skills.Learn = cli.Learn
362368
}
363369
if cli.System != "" {
364370
cfg.System = cli.System

‎internal/config/loader_test.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,3 +414,91 @@ func TestProjectConfigPath(t *testing.T) {
414414
t.Errorf("ProjectConfigPath() = %q, want absolute path", path)
415415
}
416416
}
417+
418+
func TestLoadConfig_SkillsLearnEnvDoesNotClobberSkillsConfig(t *testing.T) {
419+
// Regression: KODE_SKILLS_LEARN env var should merge Learn into
420+
// existing skills config from files, not replace the entire struct.
421+
dir := t.TempDir()
422+
423+
prevHome := os.Getenv("HOME")
424+
os.Setenv("HOME", dir)
425+
defer os.Setenv("HOME", prevHome)
426+
427+
// Create project file with skills settings
428+
cwd, _ := os.Getwd()
429+
os.Chdir(dir)
430+
defer os.Chdir(cwd)
431+
432+
if err := os.WriteFile(filepath.Join(dir, "kode.json"), []byte(`{
433+
"skills": {
434+
"max_auto_load": 5,
435+
"max_lazy_slots": 10,
436+
"dirs": ["/custom/skills"],
437+
"import": {
438+
"max_size_bytes": 524288,
439+
"timeout_seconds": 10,
440+
"require_https": true
441+
},
442+
"curation": {
443+
"staleness_days": 60,
444+
"auto_prune": true
445+
}
446+
}
447+
}`), 0644); err != nil {
448+
t.Fatal(err)
449+
}
450+
451+
// Set KODE_SKILLS_LEARN — should NOT clobber other skills fields
452+
os.Setenv("KODE_SKILLS_LEARN", "true")
453+
defer os.Unsetenv("KODE_SKILLS_LEARN")
454+
455+
cfg := LoadConfig(CLIFlags{})
456+
if !cfg.Skills.Learn {
457+
t.Error("Skills.Learn should be true from env")
458+
}
459+
if cfg.Skills.MaxAutoLoad != 5 {
460+
t.Errorf("Skills.MaxAutoLoad = %d, want 5 (survives env override)", cfg.Skills.MaxAutoLoad)
461+
}
462+
if cfg.Skills.MaxLazySlots != 10 {
463+
t.Errorf("Skills.MaxLazySlots = %d, want 10", cfg.Skills.MaxLazySlots)
464+
}
465+
if len(cfg.Skills.Dirs) != 1 || cfg.Skills.Dirs[0] != "/custom/skills" {
466+
t.Errorf("Skills.Dirs = %v, want [\"/custom/skills\"]", cfg.Skills.Dirs)
467+
}
468+
if !cfg.Skills.Import.RequireHTTPS {
469+
t.Error("Skills.Import.RequireHTTPS should be true")
470+
}
471+
if cfg.Skills.Curation.StalenessDays != 60 {
472+
t.Errorf("Skills.Curation.StalenessDays = %d, want 60", cfg.Skills.Curation.StalenessDays)
473+
}
474+
}
475+
476+
func TestLoadConfig_SkillsLearnCLIDoesNotClobberSkillsConfig(t *testing.T) {
477+
// Regression: --learn CLI flag should merge, not replace.
478+
dir := t.TempDir()
479+
480+
cwd, _ := os.Getwd()
481+
os.Chdir(dir)
482+
defer os.Chdir(cwd)
483+
484+
if err := os.WriteFile(filepath.Join(dir, "kode.json"), []byte(`{
485+
"skills": {
486+
"max_auto_load": 7,
487+
"curation": {"staleness_days": 30}
488+
}
489+
}`), 0644); err != nil {
490+
t.Fatal(err)
491+
}
492+
493+
b := true
494+
cfg := LoadConfig(CLIFlags{Learn: &b})
495+
if !cfg.Skills.Learn {
496+
t.Error("Skills.Learn should be true from CLI")
497+
}
498+
if cfg.Skills.MaxAutoLoad != 7 {
499+
t.Errorf("Skills.MaxAutoLoad = %d, want 7 (survives CLI override)", cfg.Skills.MaxAutoLoad)
500+
}
501+
if cfg.Skills.Curation.StalenessDays != 30 {
502+
t.Errorf("Skills.Curation.StalenessDays = %d, want 30", cfg.Skills.Curation.StalenessDays)
503+
}
504+
}

‎internal/danger/classifier.go‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,8 @@ var defaultActions = map[RiskClass]Action{
8585
// ActionFor returns the configured action for the given risk class.
8686
// Falls back to defaults for unknown classes and unconfigured classes.
8787
func (c *DangerousConfig) ActionFor(cls RiskClass) Action {
88-
// Built-in safety: Safe and LocalWrite always allow at minimum
89-
// (user can't accidentally make "ls" or "echo > file" require approval)
88+
// If the user explicitly configured an action for this class, use it.
89+
// Falls back to built-in defaults for unconfigured classes.
9090
if c.Classes != nil {
9191
if a, ok := c.Classes[cls]; ok {
9292
return a
@@ -435,11 +435,6 @@ func classifySegment(tokens []string) RiskClass {
435435
// ── Detection helpers ──────────────────────────────────────────────────
436436

437437
func isBlocked(tokens []string) bool {
438-
// Fork bomb
439-
cmd := strings.Join(tokens, " ")
440-
if cmd == ":(){ :|:& };:" {
441-
return true
442-
}
443438
// dd to block device
444439
if len(tokens) >= 4 && tokens[0] == "dd" {
445440
for i, tok := range tokens {
@@ -515,10 +510,7 @@ func isSystemWrite(first string, tokens []string) bool {
515510
if tok == ">" || tok == ">>" {
516511
continue
517512
}
518-
if strings.HasPrefix(tok, "/etc/") || strings.HasPrefix(tok, "/usr/") ||
519-
strings.HasPrefix(tok, "/bin/") || strings.HasPrefix(tok, "/lib/") ||
520-
strings.HasPrefix(tok, "/var/") || strings.HasPrefix(tok, "/opt/") ||
521-
strings.HasPrefix(tok, "/boot/") || strings.HasPrefix(tok, "/sbin/") {
513+
if isSystemPath(tok) {
522514
// Check if it's a redirect target (token follows > or >>)
523515
for i, t := range tokens {
524516
if (t == ">" || t == ">>") && i+1 < len(tokens) && tokens[i+1] == tok {
@@ -679,10 +671,19 @@ func hasSystemRedirectTarget(tokens []string) bool {
679671
if tok == ">" || tok == ">>" {
680672
continue
681673
}
682-
if strings.HasPrefix(tok, "/etc/") || strings.HasPrefix(tok, "/usr/") ||
683-
strings.HasPrefix(tok, "/bin/") || strings.HasPrefix(tok, "/lib/") ||
684-
strings.HasPrefix(tok, "/var/") || strings.HasPrefix(tok, "/opt/") ||
685-
strings.HasPrefix(tok, "/boot/") || strings.HasPrefix(tok, "/sbin/") {
674+
if isSystemPath(tok) {
675+
return true
676+
}
677+
}
678+
return false
679+
}
680+
681+
// isSystemPath returns true if the path targets a system directory.
682+
var systemPathPrefixes = []string{"/etc/", "/usr/", "/bin/", "/lib/", "/var/", "/opt/", "/boot/", "/sbin/"}
683+
684+
func isSystemPath(path string) bool {
685+
for _, p := range systemPathPrefixes {
686+
if strings.HasPrefix(path, p) {
686687
return true
687688
}
688689
}

‎internal/danger/classifier_test.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,3 +517,78 @@ func TestClassify_Tokenize(t *testing.T) {
517517
}
518518

519519
func strPtr(s string) *string { return &s }
520+
521+
func TestIsSystemPath(t *testing.T) {
522+
// Regression: verify the extracted isSystemPath helper works correctly.
523+
tests := []struct {
524+
path string
525+
want bool
526+
}{
527+
{"/etc/hosts", true},
528+
{"/usr/local/bin/go", true},
529+
{"/bin/sh", true},
530+
{"/lib/x86_64-linux-gnu/libc.so", true},
531+
{"/var/log/syslog", true},
532+
{"/opt/app/config", true},
533+
{"/boot/vmlinuz", true},
534+
{"/sbin/init", true},
535+
{"/home/user/file", false},
536+
{"/tmp/scratch", false},
537+
{"/workspace/src", false},
538+
{"./local/file", false},
539+
{"file.go", false},
540+
{"/root/.bashrc", false},
541+
{"/usr", false}, // no trailing slash — must be a directory prefix
542+
}
543+
for _, tt := range tests {
544+
t.Run(tt.path, func(t *testing.T) {
545+
got := isSystemPath(tt.path)
546+
if got != tt.want {
547+
t.Errorf("isSystemPath(%q) = %v, want %v", tt.path, got, tt.want)
548+
}
549+
})
550+
}
551+
}
552+
553+
func TestClassify_ForkBomb_StillDetected(t *testing.T) {
554+
// Regression: fork bomb detection is now centralized in isRawBlocked.
555+
// Ensure it still works.
556+
tests := []struct {
557+
cmd string
558+
cls RiskClass
559+
}{
560+
{":(){ :|:& };:", Blocked},
561+
// Variant with spaces in different places
562+
{" :(){ :|:& };: ", Blocked},
563+
}
564+
for _, tt := range tests {
565+
t.Run(tt.cmd, func(t *testing.T) {
566+
got := Classify(tt.cmd)
567+
if got != tt.cls {
568+
t.Errorf("Classify(%q) = %s, want %s", tt.cmd, got, tt.cls)
569+
}
570+
})
571+
}
572+
}
573+
574+
func TestClassify_SystemRedirectTarget(t *testing.T) {
575+
// Regression: isSystemWrite and hasSystemRedirectTarget now share
576+
// isSystemPath. Verify system path redirect detection works.
577+
tests := []struct {
578+
cmd string
579+
cls RiskClass
580+
}{
581+
{"echo bad > /etc/hosts", SystemWrite},
582+
{"cat data >> /usr/local/etc/app.conf", SystemWrite},
583+
{"echo ok > /tmp/safe.txt", LocalWrite},
584+
{"echo ok > ./local.conf", LocalWrite},
585+
}
586+
for _, tt := range tests {
587+
t.Run(tt.cmd, func(t *testing.T) {
588+
got := Classify(tt.cmd)
589+
if got != tt.cls {
590+
t.Errorf("Classify(%q) = %s, want %s", tt.cmd, got, tt.cls)
591+
}
592+
})
593+
}
594+
}

‎internal/loop/loop.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ type Engine struct {
2727
system string
2828
maxContext int // max context tokens (0 = no limit)
2929
skillLoader SkillLoader // optional: loads matching skills
30+
lastSkillMsg string // last user message that triggered skill loading (dedup)
3031
}
3132

3233
// New creates a new loop Engine.
@@ -201,10 +202,11 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
201202
// Trim context to stay within model's context window
202203
messages = e.trimContext(messages, tools)
203204

204-
// Load relevant skills based on latest user input
205+
// Load relevant skills based on latest user input (once per message)
205206
if e.skillLoader != nil {
206-
if userMsg := lastUserMessage(messages); userMsg != "" {
207+
if userMsg := lastUserMessage(messages); userMsg != "" && userMsg != e.lastSkillMsg {
207208
if skillContext := e.skillLoader(userMsg); skillContext != "" {
209+
e.lastSkillMsg = userMsg
208210
// Inject skill context as a system message right before the user message
209211
insertIdx := len(messages)
210212
for j := len(messages) - 1; j >= 0; j-- {

‎internal/loop/loop_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,3 +571,68 @@ func TestTrimContext_IncludesToolDefTokens(t *testing.T) {
571571
t.Errorf("trimContext with tool defs should trim, got %d >= %d", len(result), len(msgs))
572572
}
573573
}
574+
575+
func TestEngine_SkillLoader_CalledOncePerInput(t *testing.T) {
576+
// Regression: SkillLoader must fire only once per unique user message,
577+
// not once per iteration. Verifies the skill injection leak fix.
578+
skillLoadCount := 0
579+
var loadedInput string
580+
581+
skillLoader := func(userInput string) string {
582+
skillLoadCount++
583+
loadedInput = userInput
584+
return "injected skill content"
585+
}
586+
587+
callCount := 0
588+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
589+
callCount++
590+
if callCount == 1 {
591+
// First iteration: request a tool call
592+
fmt.Fprint(w, `{
593+
"choices":[{
594+
"message":{
595+
"content":"Let me think.",
596+
"tool_calls":[{
597+
"id":"call_1",
598+
"function":{
599+
"name":"echo",
600+
"arguments":"{}"
601+
}
602+
}]
603+
}
604+
}]
605+
}`)
606+
} else {
607+
// Second iteration: final answer
608+
fmt.Fprint(w, `{"choices":[{"message":{"content":"done"}}]}`)
609+
}
610+
}))
611+
defer server.Close()
612+
613+
echoTool := &fakeTool{name: "echo", description: "echo", output: "ok"}
614+
registry := tool.NewRegistry([]tool.Tool{echoTool})
615+
client := llm.New(server.URL, "sk-test", "test-model", "", 0)
616+
engine := New(client, registry, 10, "", nil, 0)
617+
engine.SetSkillLoader(skillLoader)
618+
619+
result, err := engine.Run(context.Background(), "do the task")
620+
if err != nil {
621+
t.Fatalf("Run() error: %v", err)
622+
}
623+
if result != "done" {
624+
t.Errorf("result = %q, want %q", result, "done")
625+
}
626+
627+
// SkillLoader should have been called exactly once,
628+
// not once per iteration (which would be 2+)
629+
if skillLoadCount != 1 {
630+
t.Errorf("SkillLoader called %d times, want 1 (should dedup per input)", skillLoadCount)
631+
}
632+
if loadedInput != "do the task" {
633+
t.Errorf("loadedInput = %q, want %q", loadedInput, "do the task")
634+
}
635+
if callCount != 2 {
636+
t.Errorf("LLM called %d times, want 2", callCount)
637+
}
638+
}

‎internal/skills/loader.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"path/filepath"
77
"strconv"
88
"strings"
9-
"time"
109
)
1110

1211
// ── Frontmatter Parsing ───────────────────────────────────────────────
@@ -291,7 +290,6 @@ func scanDir(dir string) []Skill {
291290
Dir: dir,
292291
Path: skillPath,
293292
}
294-
s.LastUsed = time.Now().UTC()
295293
skills = append(skills, *s)
296294
}
297295
return skills

0 commit comments

Comments
 (0)