From 9f8f76166039db3e7343358b38029c136dc74a9e Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 20:37:45 -0400 Subject: [PATCH 01/14] task: Skills: per-node attachment in briefs Co-Authored-By: codeaf Assisted-by: CodeAF (deepseek/deepseek-v4-flash) --- internal/orchestrate/prompt.go | 37 +++++++++++++++++++++++++ internal/orchestrate/prompt_test.go | 43 +++++++++++++++++++++++++++++ internal/plan/contract.go | 23 +++++++++++++++ internal/plan/contract_test.go | 41 +++++++++++++++++++++++++++ internal/store/planjournal.go | 4 +++ 5 files changed, 148 insertions(+) diff --git a/internal/orchestrate/prompt.go b/internal/orchestrate/prompt.go index 0f36151d5..e7751a220 100644 --- a/internal/orchestrate/prompt.go +++ b/internal/orchestrate/prompt.go @@ -37,3 +37,40 @@ var plannerLaw string // called once at the start and once on EVERY node completion, so every paragraph // here is billed against the run's one tank as many times as the run is wide. var PlannerPrompt = strings.ReplaceAll(plannerLaw, "{{NAME_WORDS}}", strconv.Itoa(NameWords)) + +// SkillEntry is one attached skill rendered in a worker's instruction block. +// Name is the skill's shelf name; Doc is the one-line description from its fact; +// ShelfPath is the absolute path to the skill's directory on disk. +type SkillEntry struct { + Name string + Doc string + ShelfPath string +} + +// RenderSkillsBlock renders attached skills as doc lines and shelf paths. +// Each skill produces one line: "- []" when both exist, or a +// shorter form when only one is available. Zero entries returns zero bytes — +// no header, no placeholder, no blank line. The final line states that +// earlier-listed skills take precedence in case of conflict. +func RenderSkillsBlock(skills []SkillEntry) string { + if len(skills) == 0 { + return "" + } + var buf strings.Builder + for _, s := range skills { + buf.WriteString("- ") + if s.Doc != "" { + buf.WriteString(s.Doc) + if s.ShelfPath != "" { + buf.WriteString(" [") + buf.WriteString(s.ShelfPath) + buf.WriteString("]") + } + } else if s.ShelfPath != "" { + buf.WriteString(s.ShelfPath) + } + buf.WriteString("\n") + } + buf.WriteString("Earlier-listed skills win when two skills conflict.") + return buf.String() +} diff --git a/internal/orchestrate/prompt_test.go b/internal/orchestrate/prompt_test.go index 940231e31..86a548e07 100644 --- a/internal/orchestrate/prompt_test.go +++ b/internal/orchestrate/prompt_test.go @@ -11,6 +11,49 @@ import ( // planner shown the field would pick one. The run decides that, not the model. var untaught = map[string]bool{"kind": true} +func TestRenderSkillsBlockRendersDocAndPath(t *testing.T) { + skills := []SkillEntry{ + {Name: "imgshrink", Doc: "optimize images without losing quality", ShelfPath: "~/.codeaf/skills/imgshrink"}, + {Name: "parser", Doc: "validate and format parser fixtures", ShelfPath: "~/.codeaf/skills/parser"}, + } + got := RenderSkillsBlock(skills) + want := "- optimize images without losing quality [~/.codeaf/skills/imgshrink]\n- validate and format parser fixtures [~/.codeaf/skills/parser]\nEarlier-listed skills win when two skills conflict." + if got != want { + t.Fatalf("RenderSkillsBlock:\ngot: %q\nwant: %q", got, want) + } +} + +func TestRenderSkillsBlockEmpty(t *testing.T) { + if got := RenderSkillsBlock(nil); got != "" { + t.Fatalf("RenderSkillsBlock(nil) = %q, want \"\"", got) + } + if got := RenderSkillsBlock([]SkillEntry{}); got != "" { + t.Fatalf("RenderSkillsBlock([]) = %q, want \"\"", got) + } +} + +func TestRenderSkillsBlockPreservesPrecedenceOrder(t *testing.T) { + skills := []SkillEntry{ + {Name: "lint", Doc: "run linters", ShelfPath: "~/.codeaf/skills/lint"}, + {Name: "test", Doc: "run tests", ShelfPath: "~/.codeaf/skills/test"}, + {Name: "build", Doc: "build the project", ShelfPath: "~/.codeaf/skills/build"}, + } + got := RenderSkillsBlock(skills) + lines := strings.Split(got, "\n") + if len(lines) != 4 { + t.Fatalf("expected 4 lines (3 skills + 1 precedence), got %d", len(lines)) + } + if !strings.HasPrefix(lines[0], "- run linters") { + t.Errorf("first skill should be 'lint', got: %s", lines[0]) + } + if !strings.HasPrefix(lines[1], "- run tests") { + t.Errorf("second skill should be 'test', got: %s", lines[1]) + } + if !strings.HasPrefix(lines[2], "- build the project") { + t.Errorf("third skill should be 'build', got: %s", lines[2]) + } +} + // The law quotes the Amendment schema verbatim, which means the schema is in two // places: here as struct tags, there as a JSON block a model is held to. Drift // either way is a run that refuses a well-formed amendment or a planner taught a diff --git a/internal/plan/contract.go b/internal/plan/contract.go index 894dc2a94..7f367ced4 100644 --- a/internal/plan/contract.go +++ b/internal/plan/contract.go @@ -381,3 +381,26 @@ func writeContract(ctx context.Context, client Completer, shared string, node No provider.Report(ctx, provider.ReadingUnverifiedSuccess) return contract, usageOf(response), nil } + +// ComposeSkills builds an ordered list of skill names from pinned names and +// retrieval candidates, preserving input order. Pinned names come first (in +// the order given); non-pinned candidates follow in theirs. Duplicates are +// collapsed to the first occurrence, so a pinned entry always wins over a +// retrieved one of the same name. +func ComposeSkills(pinned, candidates []string) []string { + seen := make(map[string]bool, len(pinned)+len(candidates)) + result := make([]string, 0, len(pinned)+len(candidates)) + for _, name := range pinned { + if name != "" && !seen[name] { + seen[name] = true + result = append(result, name) + } + } + for _, name := range candidates { + if name != "" && !seen[name] { + seen[name] = true + result = append(result, name) + } + } + return result +} diff --git a/internal/plan/contract_test.go b/internal/plan/contract_test.go index ba95e4324..8019a77b2 100644 --- a/internal/plan/contract_test.go +++ b/internal/plan/contract_test.go @@ -406,3 +406,44 @@ func TestTheMethodWriterMayNotEscalateAConfirmationIntoATranscript(t *testing.T) } } } +func TestComposeSkillsOrdersPinnedFirstThenCandidates(t *testing.T) { + pinned := []string{"imgshrink", "lint"} + candidates := []string{"test", "build", "imgshrink"} + got := ComposeSkills(pinned, candidates) + want := []string{"imgshrink", "lint", "test", "build"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ComposeSkills(%q, %q) = %q, want %q", pinned, candidates, got, want) + } +} + +func TestComposeSkillsEmptyInputs(t *testing.T) { + if got := ComposeSkills(nil, nil); len(got) != 0 { + t.Fatalf("ComposeSkills(nil, nil) = %q, want empty", got) + } + if got := ComposeSkills([]string{}, nil); len(got) != 0 { + t.Fatalf("ComposeSkills([], nil) = %q, want empty", got) + } + if got := ComposeSkills(nil, []string{"a", "b"}); !reflect.DeepEqual(got, []string{"a", "b"}) { + t.Fatalf("ComposeSkills(nil, [a,b]) = %q, want [a b]", got) + } +} + +func TestComposeSkillsDeduplicates(t *testing.T) { + pinned := []string{"a", "b"} + candidates := []string{"b", "c", "a"} + got := ComposeSkills(pinned, candidates) + want := []string{"a", "b", "c"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ComposeSkills(%q, %q) = %q, want %q", pinned, candidates, got, want) + } +} + +func TestComposeSkillsSkipsEmptyNames(t *testing.T) { + pinned := []string{"a", "", "b"} + candidates := []string{"", "c"} + got := ComposeSkills(pinned, candidates) + want := []string{"a", "b", "c"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ComposeSkills(%q, %q) = %q, want %q", pinned, candidates, got, want) + } +} diff --git a/internal/store/planjournal.go b/internal/store/planjournal.go index 19100f631..7ec228d8f 100644 --- a/internal/store/planjournal.go +++ b/internal/store/planjournal.go @@ -115,6 +115,10 @@ type NodeBrief struct { // every call answered. Empty is the ordinary case. Fault string `json:"fault,omitempty"` Subharness string `json:"subharness,omitempty"` + // Skills is the ordered list of skill names attached to this node. + // Pinned skills (named by the person) come first, followed by retrieval + // candidates. Order is precedence: earlier-listed skills win conflicts. + Skills []string `json:"skills,omitempty"` } // RecordNodeBrief journals one node's rendered brief against the store id the From e46edfaef344a06ff9d08ae0a037408ebec939b4 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 21:13:40 -0400 Subject: [PATCH 02/14] task: Verify standing-tasks findings Co-Authored-By: codeaf Assisted-by: CodeAF (deepseek/deepseek-v4-flash) --- review-standing-tasks-visibility.md | 224 ++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 review-standing-tasks-visibility.md diff --git a/review-standing-tasks-visibility.md b/review-standing-tasks-visibility.md new file mode 100644 index 000000000..cb37a17a3 --- /dev/null +++ b/review-standing-tasks-visibility.md @@ -0,0 +1,224 @@ +# Review: standing-run task visibility — 5 claims checked against the code + +Codebase: `/Users/santoshkumar/Documents/agentfield/code/codeaf`, commit `aae00e7a7`. +All citations are `file:line`. Each claim is confirmed or disproved against the +code, not the host task's reasoning. + +--- + +## Claim 1 — tool registration and search path — **confirmed** + +- `func (a *Agent) tasksTool() bare.Tool` at `tools_tasks.go:181`, with + `Description: tasksDescription` (`tools_tasks.go:184`, const at `:72`). + The no-id branch calls `a.taskSearchText(parsed.Query, parsed.Limit, scope)` + (`tools_tasks.go:216`); the id branch calls `a.oneTask(ctx, token, parsed)` + (`tools_tasks.go:223`). +- Belt registration: `tools.go:181` — `tools = append(tools, a.tasksTool())`, + gated by `if a.mayProposeTask()` (`tools.go:179`). `mayProposeTask` is + `!c.InTask || c.mayFanOut()` (`task.go:534`), and `mayFanOut` is + `c.InTask && c.tasker != nil && fansOutAt(c.taskDepth)` (`task.go:540`). + So an InTask firing with a graph (an armed one) does get the tool; an InTask + firing without one does not. The eye item's run transcript calls `tasks` + repeatedly, so the tool was present for that firing. +- `func (a *Agent) taskSearchText(query string, limit int, scope string) string` + at `tools_tasks.go:246`. Its first row of data is + `taskRowsTextLimit(a.taskRows(), query, limit)` at `:255`. +- `func (a *Agent) taskRows() []TaskIndexEntry` at `tools_tasks.go:935`: + `parent := a.config.taskID; if parent == 0 { return a.TaskIndex() }`. So the + no-id search reads the project index **only when `taskID == 0`**; a node + (`taskID != 0`) reads its own graph children instead. +- `func (a *Agent) TaskIndex() []TaskIndexEntry` at `task_index.go:571`: + `rows := ReadTaskIndex(a.config.taskIndexFile())` at `:572`. +- `func (c Config) taskIndexFile() string` at `task_index.go:404`: + `if dir := strings.TrimSpace(c.Place.Dir); dir != "" { ... return filepath.Join(bucket, taskIndexName) }`, + where `bucket := filepath.Dir(dir)` at `:406`. `taskIndexName` is `"tasks.jsonl"` + (`task_index.go:75`). +- `func ReadTaskIndex(path string) []TaskIndexEntry` at `task_index.go:456`: + `if strings.TrimSpace(path) == "" { return nil }` and + `file, err := os.Open(path); if err != nil { return nil }` — a missing file + returns nil. + +The chain `taskSearchText → taskRows → TaskIndex → ReadTaskIndex(taskIndexFile())` +is exactly as claimed. One nuance the claim omits: `taskRows()` only reaches +`TaskIndex()` when `taskID == 0`; a node with `taskID != 0` never reads the +index file at all (it reads graph children). This matters for claims 4 and 5. + +--- + +## Claim 2 — wrong path for a standing run — **confirmed** + +- `func standingRunConfig(parent Config, item standing.Item, runDir string) (Config, error)` + at `standing_run.go:766`. Line `:773`: + `place := Place{Dir: runDir, Workspace: item.Workspace}`, then `:778`: + `cfg.Place = place`. +- `runDir` comes from the standing store. `Store.RunsDir(id)` at + `standing.go:653` is `filepath.Join(s.ItemDir(id), "runs")` = + `//runs`. The next-run folder is + `filepath.Join(runs, fmt.Sprintf("%04d", attempt))` (`store.go:288`), so a + run directory is `//runs/0001`. The root is `v3StandingRoot()` = + `home.Join("v3", "standing")` (`chatv3_standing.go:59`), so a run directory + is `~/.codeaf/v3/standing//runs/0001`. +- `taskIndexFile()` at `:404-414` does `bucket := filepath.Dir(runDir)` = + `~/.codeaf/v3/standing//runs`, then + `filepath.Join(bucket, "tasks.jsonl")` = + `~/.codeaf/v3/standing//runs/tasks.jsonl`. + That is NOT the project bucket. The project bucket is + `~/.codeaf/v3/projects//`, which holds the real + `tasks.jsonl`. +- Empirical confirmation on this machine: `find ~/.codeaf/v3/standing -name tasks.jsonl` + returns nothing; `~/.codeaf/v3/projects/-Users-santoshkumar-Documents-agentfield-codeaf/tasks.jsonl` + exists and is 253 KB. +- `ReadTaskIndex` returns nil for the missing file (`task_index.go:456-460`), + so `TaskIndex()` returns nil, and the no-id search prints + "No tasks have run in this project yet." (`tools_tasks.go:1109`). The real + run transcript confirms this exact string. + +--- + +## Claim 3 — `tellsElsewhere()` blocks elsewhere and everywhere — **confirmed** + +- `func (a *Agent) tellsElsewhere() bool` at `taskdelta.go:721`: + `if a.config.InTask || a.config.taskID != 0 { return false }` at `:722`, + then `return strings.TrimSpace(a.config.Place.Dir) != ""` at `:725`. +- `standingRunConfig` sets `cfg.InTask = true` at `standing_run.go:780`. + (`standing_run.go:365` also sets `InTask = true`, but that is in + `probeTool`'s throwaway belt-probe agent at `:346-365` — a different code + path, not the firing session.) +- `taskSearchText` at `tools_tasks.go:256`: + `if !a.tellsElsewhere() { return a.taskConversationHint(out) }` — returns + before the `taskElsewhereText` call at `:260` and before the + `scope != taskScopeEverywhere` check at `:268`. So `scope=everywhere` cannot + reach `OtherProjects` either: the gate is before the scope branch. + +Confirmed: both the elsewhere and everywhere readings are blocked for a +standing firing. The claim cites `InTask = true` as the reason; for an armed +firing `taskID != 0` (see claim 4) would also trigger the same `return false`, +so the gate holds either way. + +--- + +## Claim 4 — id query message and `taskID == 0` — **disagrees** + +The claim asserts `taskID == 0` for standing runs, that the message is +"No task '4' in this project...", and that "the person paraphrased." The code +and the **real run transcript** show the opposite on all three points. + +**What the code does.** `standingRunConfig` at `:766-790` does not set +`cfg.taskID` — the claim's parenthetical is true for that function alone. But +`Run()` at `:555-556` calls `standingWideWork(cfg, item, brief)` immediately +after, and `standingWideWork` at `standing_run.go:940-941` sets: + +```go +cfg.tasker = graph +cfg.taskID = id // the root node's id, from graph.reserve() at task_run.go:1281 +``` + +...when the firing is armed for wide work: `if !cfg.Divide || !enumeratesWidth(...)` +returns false at `:884`. `cfg.Divide` comes from `v3StandingPosture` at +`chatv3_standing.go:192` (`Divide: settings.Swarm`), and `enumeratesWidth` +(`task_divide.go:635`) calls `splitgate.WorthIt` on the brief text. + +**What the real run did.** The eye item `8eac8a7f9f983bf0` fired once +(`runs/0001`). Its transcript shows the `tasks` calls and their results: + +``` +tasks {"query":"Skills:","limit":20} → No task matches "Skills:". ... +tasks {"limit":30} → No tasks have run in this project yet. +tasks {"limit":30,"scope":"everywhere"} → No tasks have run in this project yet. +tasks {"id":"4"} → No task "4" among the pieces you handed out. Call tasks with no arguments to see them; ... +tasks {"id":"5"} → No task "5" among the pieces you handed out. ... +tasks {"id":"6"} → No task "6" among the pieces you handed out. ... +``` + +The id-query message is **"No task \"4\" among the pieces you handed out"** — +the `taskID != 0` branch at `tools_tasks.go:648-651`: + +```go +if a.config.taskID != 0 { + return fmt.Sprintf("No task %q among the pieces you handed out. ...", token), true, nil +} +return fmt.Sprintf("No task %q in this project. ...", token), true, nil // :653, the == 0 branch +``` + +So `taskID` was nonzero for this firing: `standingWideWork` armed it, and the +"among the pieces" branch fired — not the "in this project" branch the claim +names. The person did not paraphrase; the code produced that message. + +**Why `taskRows()` is empty.** The claim says "taskRows() returns empty → +taskByToken can't find them" and attributes the emptiness to `ReadTaskIndex` +finding nothing (the wrong path from claim 2). That is the wrong mechanism for +this firing. With `taskID != 0`, `taskRows()` at `:935-948` never calls +`TaskIndex()`: + +```go +func (a *Agent) taskRows() []TaskIndexEntry { + parent := a.config.taskID + if parent == 0 { return a.TaskIndex() } // not taken + // ...children of the root node from the graph... +} +``` + +It returns the root node's children from the graph. The eye did not divide, so +the root has no children, and `taskRows()` returns an empty slice. The wrong +index path (claim 2) is real, but it is not what produces the empty rows for an +armed firing — `TaskIndex()` is never reached. + +**Summary of disagreement.** The claim is right that the id query fails and +that `taskRows()` is empty, but wrong about (a) which branch fires +(`taskID != 0`, not `== 0`), (b) the exact message ("among the pieces you +handed out", not "in this project"), (c) the cause of the empty rows (graph +children, not the index file), and (d) the claim that +"standingRunConfig never sets taskID" misses `standingWideWork` at `:941` +which does. + +--- + +## Claim 5 — this is a bug, and no override exists — **confirmed, with one gap in the claim's reasoning** + +**The path is wrong.** `taskIndexFile()` at `task_index.go:404-414` derives +the index path from `filepath.Dir(Place.Dir)`. For a normal session, +`Place.Dir` is `~/.codeaf/v3/projects///`, so +`Dir` gives the project bucket `~/.codeaf/v3/projects//` — +correct. For a standing firing, `Place.Dir` is +`~/.codeaf/v3/standing//runs/0001`, so `Dir` gives +`~/.codeaf/v3/standing//runs/` — wrong: no project index lives there. +The `task_index.go` header at `:40-56` states the design intent: +"In the PROJECT BUCKET ... The scope is the project and not the conversation," +and "the parent of the folder is the bucket." A run folder's parent is `runs/`, +not the project bucket, so the derivation assumption breaks. + +**No override exists.** I searched `task_index.go` for `standing` and `InTask`: +the only hit is `closeInflightTaskIndexRows` at `:686`, which guards a write +path (`if a.config.InTask { return }`) and has nothing to do with path +resolution. `taskIndexFile()` has no special case for standing runs. + +**The claim's reasoning has a gap.** The claim says "the design intent is +that InTask agents (including standing firings) should see their own children" +and identifies the wrong path as the cause. The wrong path is real, but it is +not the cause of what the eye experienced. For an **armed** firing +(`taskID != 0` via `standingWideWork`), `taskRows()` at `:935` returns graph +children and never calls `TaskIndex()`, so fixing `taskIndexFile()` alone +would not let the eye see the conversation's tasks 4/5/6 — it would still get +its own (empty) children. The wrong path affects only the `taskID == 0` path +(the no-id search), which prints "No tasks have run in this project yet." +because `TaskIndex()` returns nil. The id-query failure has a second, more +direct cause: the armed firing's `taskID` makes `taskRows()` return graph +children instead of the project index. + +The claim's recommendation — "a shell probe against the project's real task +index file ... NOT the `tasks` tool inside the firing" — is a design +suggestion I was asked not to make or evaluate; I confirm only its factual +basis: there is no override, and the path the `tasks` tool resolves is not the +project bucket. + +--- + +## One-line summary + +Claims 1, 2, 3, and 5 are confirmed by the code. Claim 4 is disproved: the +firing was armed (`standingWideWork` at `standing_run.go:941` set `cfg.taskID`), +so the real message was "among the pieces you handed out" (the `taskID != 0` +branch at `tools_tasks.go:651`), not "in this project" (the `taskID == 0` +branch at `:653`), and the empty rows came from the graph having no children, +not from `ReadTaskIndex` finding nothing — though the wrong index path from +claim 2 is independently real. From 1142f9053ea13704fd4bd822599bb7bc0fb03f00 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 21:56:49 -0400 Subject: [PATCH 03/14] task: Finish skills store record: continue the cut-off branch Co-Authored-By: codeaf --- internal/exec/exec_test.go | 2 +- internal/head/craftbelt_test.go | 2 +- internal/resident/craftverbs_test.go | 2 +- internal/resident/notebook_test.go | 2 +- internal/resident/skills.go | 97 +++++++++++++++---- internal/resident/skills_test.go | 41 ++++++++ internal/store/competence_test.go | 2 +- internal/store/craft_verbs_test.go | 4 +- internal/store/facts.go | 137 +++++++++++++++++++++------ internal/store/meta.go | 2 +- internal/store/practice.go | 2 +- internal/store/store_test.go | 123 +++++++++++++++++++++++- 12 files changed, 358 insertions(+), 58 deletions(-) diff --git a/internal/exec/exec_test.go b/internal/exec/exec_test.go index 80930501f..a67b4165f 100644 --- a/internal/exec/exec_test.go +++ b/internal/exec/exec_test.go @@ -137,7 +137,7 @@ func TestRecallSurfacesActiveSkillKind(t *testing.T) { if err != nil { t.Fatal(err) } - if err := history.ActivateSkill(candidate.Seq, "/home/test/.codeaf/skills/repo-audit"); err != nil { + if err := history.ActivateSkill(candidate.Seq, "/home/test/.codeaf/skills/repo-audit", ""); err != nil { t.Fatal(err) } diff --git a/internal/head/craftbelt_test.go b/internal/head/craftbelt_test.go index 60426f73f..b49554cc9 100644 --- a/internal/head/craftbelt_test.go +++ b/internal/head/craftbelt_test.go @@ -124,7 +124,7 @@ func TestForgettingAToolThatWasForgedRetiresTheToolRatherThanOnlyTheBelief(t *te if err != nil { t.Fatal(err) } - if err := graph.ActivateSkill(skill.Seq, skill.Artifact); err != nil { + if err := graph.ActivateSkill(skill.Seq, skill.Artifact, ""); err != nil { t.Fatal(err) } user := postUser(t, graph, "forget-tool", "stop using imgshrink, it mangles the colours") diff --git a/internal/resident/craftverbs_test.go b/internal/resident/craftverbs_test.go index 8fb820545..b32163f89 100644 --- a/internal/resident/craftverbs_test.go +++ b/internal/resident/craftverbs_test.go @@ -258,7 +258,7 @@ func TestRetiringAToolQuietensTheBeliefAtOnce(t *testing.T) { if err != nil { t.Fatal(err) } - if err := graph.ActivateSkill(skill.Seq, skill.Artifact); err != nil { + if err := graph.ActivateSkill(skill.Seq, skill.Artifact, ""); err != nil { t.Fatal(err) } diff --git a/internal/resident/notebook_test.go b/internal/resident/notebook_test.go index b8a7c11e3..701c2bbea 100644 --- a/internal/resident/notebook_test.go +++ b/internal/resident/notebook_test.go @@ -328,7 +328,7 @@ func TestNotebookDigestRetrievesPathScopeAndEmptyNotebook(t *testing.T) { if err != nil { t.Fatal(err) } - if err := graph.ActivateSkill(skill.Seq, "/home/test/.codeaf/skills/notebook-audit"); err != nil { + if err := graph.ActivateSkill(skill.Seq, "/home/test/.codeaf/skills/notebook-audit", ""); err != nil { t.Fatal(err) } got := NotebookDigest(graph, "leaf", "inspect internal/resident/notebook.go", "fix cue lookup", 5) diff --git a/internal/resident/skills.go b/internal/resident/skills.go index 2a940ee26..d6ad54731 100644 --- a/internal/resident/skills.go +++ b/internal/resident/skills.go @@ -2,6 +2,7 @@ package resident import ( "context" + "crypto/sha256" "errors" "fmt" "io" @@ -97,7 +98,7 @@ func (r *Reconciler) promoteRecurringSkills(ctx context.Context) { } sort.Strings(jobs) - installed, err := installSkillTrial(ctx, root, selected, jobs) + installed, digest, err := installSkillTrial(ctx, root, selected, jobs) if ctx.Err() != nil { return } @@ -108,7 +109,7 @@ func (r *Reconciler) promoteRecurringSkills(ctx context.Context) { } continue } - if err := r.store.ActivateSkill(selected.Seq, installed); err != nil { + if err := r.store.ActivateSkill(selected.Seq, installed, digest); err != nil { continue } r.queueLearningMoment(selected.NodeID, forgedSkillMoment(filepath.Base(installed))) @@ -147,61 +148,117 @@ func skillMatchKey(fact store.Fact) string { return scope + "\x00" + doc } -func installSkillTrial(ctx context.Context, root string, candidate store.Fact, jobs []string) (string, error) { +func installSkillTrial(ctx context.Context, root string, candidate store.Fact, jobs []string) (string, string, error) { rawSource := strings.TrimSpace(candidate.Artifact) if !filepath.IsAbs(rawSource) { - return "", fmt.Errorf("candidate artifact %q is not absolute", rawSource) + return "", "", fmt.Errorf("candidate artifact %q is not absolute", rawSource) } source, err := filepath.Abs(rawSource) if err != nil { - return "", fmt.Errorf("resolve candidate artifact: %w", err) + return "", "", fmt.Errorf("resolve candidate artifact: %w", err) } if pathsOverlap(source, root) { - return "", fmt.Errorf("candidate artifact %q overlaps the skill shelf", source) + return "", "", fmt.Errorf("candidate artifact %q overlaps the skill shelf", source) } staging, err := os.MkdirTemp(root, ".candidate-") if err != nil { - return "", fmt.Errorf("create skill staging directory: %w", err) + return "", "", fmt.Errorf("create skill staging directory: %w", err) } defer os.RemoveAll(staging) if err := copySkillDirectory(source, staging); err != nil { - return "", fmt.Errorf("prepare skill trial: %w", err) + return "", "", fmt.Errorf("prepare skill trial: %w", err) } provenance := strings.Join(jobs, "\n") + "\n" if err := os.WriteFile(filepath.Join(staging, "PROVENANCE"), []byte(provenance), 0o644); err != nil { - return "", fmt.Errorf("write skill provenance: %w", err) + return "", "", fmt.Errorf("write skill provenance: %w", err) } if err := runSkillCheck(ctx, staging); err != nil { - return "", err + return "", "", err } if _, err := skillExecutable(staging); err != nil { - return "", fmt.Errorf("check.sh removed the skill executable: %w", err) + return "", "", fmt.Errorf("check.sh removed the skill executable: %w", err) } if err := os.WriteFile(filepath.Join(staging, "PROVENANCE"), []byte(provenance), 0o644); err != nil { - return "", fmt.Errorf("rewrite skill provenance: %w", err) + return "", "", fmt.Errorf("rewrite skill provenance: %w", err) } slug := skillSlug(filepath.Base(source)) target := filepath.Join(root, slug) if _, err := os.Lstat(target); err == nil { - // The artifact's own name is the command workers were taught. Preserve - // it normally; only a real shelf collision earns a durable sequence - // suffix, so installation never overwrites another learned capability. slug += "-" + strconv.FormatInt(candidate.Seq, 10) target = filepath.Join(root, slug) } else if !errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("install skill: inspect target: %w", err) + return "", "", fmt.Errorf("install skill: inspect target: %w", err) } if _, err := os.Lstat(target); err == nil { - return "", fmt.Errorf("install skill: target %q already exists", target) + return "", "", fmt.Errorf("install skill: target %q already exists", target) } else if !errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("install skill: inspect target: %w", err) + return "", "", fmt.Errorf("install skill: inspect target: %w", err) } if err := os.Rename(staging, target); err != nil { - return "", fmt.Errorf("install skill: %w", err) + return "", "", fmt.Errorf("install skill: %w", err) + } + + digest, err := contentDigest(target) + if err != nil { + return "", "", fmt.Errorf("install skill: compute digest: %w", err) + } + return target, digest, nil +} + +// contentDigest returns a sha256 digest of all regular files under dir, +// sorted by relative path. Symlinks are refused — installSkillTrial rejects +// them earlier, and this read ensures the digest covers only what the trial +// copied. +func contentDigest(dir string) (string, error) { + entries := make([]string, 0) + err := filepath.WalkDir(dir, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() { + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + if !info.Mode().IsRegular() { + return nil + } + relative, err := filepath.Rel(dir, path) + if err != nil { + return err + } + entries = append(entries, relative) + return nil + }) + if err != nil { + return "", err + } + sort.Strings(entries) + + h := sha256.New() + for _, relative := range entries { + path := filepath.Join(dir, relative) + // Write the relative path as a prefix so two directories with + // different file structures but the same content after concatenation + // produce different digests. + if _, err := io.WriteString(h, relative+"\x00"); err != nil { + return "", err + } + f, err := os.Open(path) + if err != nil { + return "", err + } + if _, err := io.Copy(h, f); err != nil { + f.Close() + return "", err + } + f.Close() } - return target, nil + return fmt.Sprintf("%x", h.Sum(nil)), nil } func copySkillDirectory(source, target string) error { diff --git a/internal/resident/skills_test.go b/internal/resident/skills_test.go index cda1c2086..d07c5b18a 100644 --- a/internal/resident/skills_test.go +++ b/internal/resident/skills_test.go @@ -219,3 +219,44 @@ func recordCandidateJob(t *testing.T, graph *store.Store, id, artifact string) s } return fact } + +// Digest is stable for identical directory contents and differs when a file +// changes, even if the file name is the same. +func TestContentDigestStability(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "run.sh"), []byte("#!/bin/sh\necho hello\n"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "check.sh"), []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + d1, err := contentDigest(dir) + if err != nil { + t.Fatal(err) + } + // Same contents -> same digest. + d2, err := contentDigest(dir) + if err != nil { + t.Fatal(err) + } + if d1 != d2 { + t.Fatalf("same contents produced different digests: %q vs %q", d1, d2) + } + // Changed file content -> different digest. + if err := os.WriteFile(filepath.Join(dir, "check.sh"), []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } + d3, err := contentDigest(dir) + if err != nil { + t.Fatal(err) + } + if d1 == d3 { + t.Fatal("changed content should produce different digest") + } + if len(d1) != 64 { + t.Fatalf("sha256 hex digest should be 64 chars, got %d", len(d1)) + } + if len(d3) != 64 { + t.Fatalf("sha256 hex digest should be 64 chars, got %d", len(d3)) + } +} diff --git a/internal/store/competence_test.go b/internal/store/competence_test.go index faaa4db66..a31b52007 100644 --- a/internal/store/competence_test.go +++ b/internal/store/competence_test.go @@ -117,7 +117,7 @@ func TestCompetenceMapAggregatesTerritoryEvidenceAndInstalledSkills(t *testing.T if err != nil { t.Fatal(err) } - if err := graph.ActivateSkill(skill.Seq, "/installed/go-parser-skill"); err != nil { + if err := graph.ActivateSkill(skill.Seq, "/installed/go-parser-skill", ""); err != nil { t.Fatal(err) } if err := graph.Fold("go-job", "Go parser work stayed reliable.", nil); err != nil { diff --git a/internal/store/craft_verbs_test.go b/internal/store/craft_verbs_test.go index f3f270e2c..7552e11c6 100644 --- a/internal/store/craft_verbs_test.go +++ b/internal/store/craft_verbs_test.go @@ -17,7 +17,7 @@ func TestCraftAndSkillCommandsJournalAndReplay(t *testing.T) { if err != nil { t.Fatalf("record skill: %v", err) } - if err := s.ActivateSkill(skill.Seq, skill.Artifact); err != nil { + if err := s.ActivateSkill(skill.Seq, skill.Artifact, ""); err != nil { t.Fatalf("activate skill: %v", err) } @@ -102,7 +102,7 @@ func TestSkillRetireIsCheckedAtTheFunnel(t *testing.T) { if err != nil { t.Fatalf("record skill: %v", err) } - if err := s.ActivateSkill(skill.Seq, skill.Artifact); err != nil { + if err := s.ActivateSkill(skill.Seq, skill.Artifact, ""); err != nil { t.Fatalf("activate skill: %v", err) } if _, err := s.RequestCommand(Command{ diff --git a/internal/store/facts.go b/internal/store/facts.go index 0d383d433..534f27072 100644 --- a/internal/store/facts.go +++ b/internal/store/facts.go @@ -236,6 +236,15 @@ func ChannelForWriter(writer FactWriter) FactChannel { } } +// CostCardT is the token budget a skill carries: zero until measured. +// ReadTokens cover reading the skill's body; RunTokens cover executing +// the check and run; DelegateTokens cover handing a sub-task to the skill. +type CostCardT struct { + RunTokens int64 `json:"run,omitempty"` + ReadTokens int64 `json:"read,omitempty"` + DelegateTokens int64 `json:"delegate,omitempty"` +} + // Fact is one materialized notebook entry. type Fact struct { Seq int64 @@ -269,6 +278,16 @@ type Fact struct { LastUsed time.Time // Confidence is the current shrunk survival rate for Kind x Channel. Confidence float64 + + // Trust is the provenance tier for skill facts: "authored" (default), + // "imported-provisional", or "forged". Empty is stored and read back as + // "authored" by SkillFactAccessors. + Trust string + // CostCard is the measured token budget for read/run/delegate operations. + CostCard CostCardT + // Digest is the content-addressable digest of the payload directory, + // computed at install time from all files' contents. + Digest string } const factsSchema = ` @@ -288,7 +307,10 @@ CREATE TABLE IF NOT EXISTS facts ( evidence_seq INTEGER NOT NULL DEFAULT 0, status_origin TEXT NOT NULL DEFAULT '', uses INTEGER NOT NULL DEFAULT 0, - last_used TEXT NOT NULL DEFAULT '' + last_used TEXT NOT NULL DEFAULT '', + trust TEXT NOT NULL DEFAULT '', + cost_card TEXT NOT NULL DEFAULT '{}' CHECK (json_valid(cost_card)), + digest TEXT NOT NULL DEFAULT '' ); CREATE INDEX IF NOT EXISTS facts_scope ON facts (scope, status); -- Territory scoping asks for active facts by node, which the scope-leading @@ -341,11 +363,15 @@ type factPayload struct { Unsettled *UnsettledPair `json:"unsettled,omitempty"` Status string `json:"status,omitempty"` Artifact string `json:"artifact,omitempty"` + Trust string `json:"trust,omitempty"` + CostCard string `json:"cost_card,omitempty"` + Digest string `json:"digest,omitempty"` } type factActivatedPayload struct { FactSeq int64 `json:"fact_seq"` Artifact string `json:"artifact"` + Digest string `json:"digest,omitempty"` } type factSupersededPayload struct { @@ -467,7 +493,7 @@ func (s *Store) RecordFactFrom(writer FactWriter, nodeID, scope string, kind Fac if kind == FactTrait { return Fact{}, fmt.Errorf("record fact: %w: traits require the measured trait lifecycle", ErrInvalid) } - return s.recordFact(writer, nodeID, scope, kind, body, nil, 0, FactActive, "", true) + return s.recordFact(writer, nodeID, scope, kind, body, nil, 0, FactActive, "", "", true) } // RecordUnsettledFact appends one structured competing pair. Its Body is @@ -481,7 +507,7 @@ func (s *Store) RecordUnsettledFactFrom(writer FactWriter, nodeID, scope string, if err := pair.Validate(); err != nil { return Fact{}, fmt.Errorf("record unsettled fact: %w: %v", ErrInvalid, err) } - return s.recordFact(writer, nodeID, scope, FactUnsettled, FormatUnsettledPair(pair), &pair, 0, FactActive, "", true) + return s.recordFact(writer, nodeID, scope, FactUnsettled, FormatUnsettledPair(pair), &pair, 0, FactActive, "", "", true) } // ReplaceFact records a new ordinary fact and supersedes factSeq in the same @@ -507,7 +533,7 @@ func (s *Store) ReplaceFactFrom(writer FactWriter, factSeq int64, nodeID, scope if kind == FactTrait { return Fact{}, fmt.Errorf("replace fact: %w: traits require the measured trait lifecycle", ErrInvalid) } - return s.recordFact(writer, nodeID, scope, kind, body, nil, factSeq, FactActive, "", true) + return s.recordFact(writer, nodeID, scope, kind, body, nil, factSeq, FactActive, "", "", true) } // ReplaceUnsettledFactFrom carries a pair forward on writer's channel. @@ -518,23 +544,27 @@ func (s *Store) ReplaceUnsettledFactFrom(writer FactWriter, factSeq int64, nodeI if err := pair.Validate(); err != nil { return Fact{}, fmt.Errorf("replace unsettled fact: %w: %v", ErrInvalid, err) } - return s.recordFact(writer, nodeID, scope, FactUnsettled, FormatUnsettledPair(pair), &pair, factSeq, FactActive, "", true) + return s.recordFact(writer, nodeID, scope, FactUnsettled, FormatUnsettledPair(pair), &pair, factSeq, FactActive, "", "", true) } // RecordSkillCandidate journals a procedure the distiller found in one job. // It is intentionally absent from retrieval until a later execution event -// activates it. -func (s *Store) RecordSkillCandidate(nodeID, scope, body, artifact string) (Fact, error) { - return s.RecordSkillCandidateFrom(FactWriterOther, nodeID, scope, body, artifact) +// activates it. Trust defaults to "authored" when empty. +func (s *Store) RecordSkillCandidate(nodeID, scope, body, artifact string, trust ...string) (Fact, error) { + return s.RecordSkillCandidateFrom(FactWriterOther, nodeID, scope, body, artifact, trust...) } // RecordSkillCandidateFrom records a candidate on writer's channel. -func (s *Store) RecordSkillCandidateFrom(writer FactWriter, nodeID, scope, body, artifact string) (Fact, error) { +func (s *Store) RecordSkillCandidateFrom(writer FactWriter, nodeID, scope, body, artifact string, trust ...string) (Fact, error) { artifact = strings.TrimSpace(artifact) if artifact == "" { return Fact{}, fmt.Errorf("record skill candidate: %w: empty artifact", ErrInvalid) } - return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactCandidate, artifact, false) + trustVal := "" + if len(trust) > 0 { + trustVal = trust[0] + } + return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactCandidate, artifact, trustVal, false) } // RewriteActiveSkillFrom rewrites an active skill on writer's channel. @@ -547,10 +577,10 @@ func (s *Store) RewriteActiveSkillFrom(writer FactWriter, nodeID, scope, body st if len(sources) != 1 || strings.TrimSpace(sources[0].Artifact) == "" { return Fact{}, fmt.Errorf("rewrite active skill: %w: source %d is not active", ErrInvalid, sourceSeq) } - return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactActive, sources[0].Artifact, true) + return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactActive, sources[0].Artifact, "", true) } -func (s *Store) recordFact(writer FactWriter, nodeID, scope string, kind FactKind, body string, unsettled *UnsettledPair, replaces int64, status, artifact string, deduplicate bool) (Fact, error) { +func (s *Store) recordFact(writer FactWriter, nodeID, scope string, kind FactKind, body string, unsettled *UnsettledPair, replaces int64, status, artifact, trust string, deduplicate bool) (Fact, error) { body = strings.TrimSpace(body) if body == "" { return Fact{}, fmt.Errorf("record fact: %w: empty fact", ErrInvalid) @@ -642,7 +672,7 @@ func (s *Store) recordFact(writer FactWriter, nodeID, scope string, kind FactKin channel := ChannelForWriter(writer) payload := factPayload{NodeID: nodeID, Scope: scope, Kind: kind, Channel: channel, Body: body, - Unsettled: unsettled, Status: status, Artifact: artifact} + Unsettled: unsettled, Status: status, Artifact: artifact, Trust: trust, CostCard: "{}"} seq, at, err := appendEvent(tx, nodeID, EventFactLearned, payload) if err != nil { return Fact{}, fmt.Errorf("record fact: %w", err) @@ -671,12 +701,14 @@ func (s *Store) recordFact(writer FactWriter, nodeID, scope string, kind FactKin return Fact{}, fmt.Errorf("record fact: %w", err) } return Fact{Seq: seq, Time: at, NodeID: nodeID, Scope: scope, Kind: kind, Channel: channel, Body: body, - Status: status, StatusSeq: seq, Unsettled: unsettled, Artifact: artifact}, nil + Status: status, StatusSeq: seq, Unsettled: unsettled, Artifact: artifact, Trust: trust}, nil } // ActivateSkill journals the only transition that makes a candidate // retrievable. The caller has already copied and executed the artifact check. -func (s *Store) ActivateSkill(factSeq int64, artifact string) error { +// Digest is the content digest of the payload directory, computed at install +// time by installSkillTrial. +func (s *Store) ActivateSkill(factSeq int64, artifact, digest string) error { artifact = strings.TrimSpace(artifact) if artifact == "" { return fmt.Errorf("activate skill: %w: empty artifact", ErrInvalid) @@ -687,7 +719,7 @@ func (s *Store) ActivateSkill(factSeq int64, artifact string) error { } defer tx.Rollback() - payload := factActivatedPayload{FactSeq: factSeq, Artifact: artifact} + payload := factActivatedPayload{FactSeq: factSeq, Artifact: artifact, Digest: digest} if _, _, err := appendEvent(tx, "", EventFactActivated, payload); err != nil { return fmt.Errorf("activate skill: %w", err) } @@ -740,6 +772,31 @@ func (s *Store) SkillFacts(status string, limit int) ([]Fact, error) { return s.factsWhere(`kind = ? AND status = ? ORDER BY seq DESC LIMIT ?`, FactSkill, status, limit) } +// SkillFactAccessors returns the three accessor projections for one skill: +// shelf path, the skill doc, and the content digest. It records one use +// through the existing Uses/LastUsed telemetry. Trust defaults to "authored" +// when the stored value is empty. +func (s *Store) SkillFactAccessors(seq int64) (artifact, doc, digest, trust string, err error) { + fact, found, err := s.FactBySeq(seq) + if err != nil { + return "", "", "", "", fmt.Errorf("skill service: %w", err) + } + if !found || fact.Kind != FactSkill { + return "", "", "", "", fmt.Errorf("skill service: %w: no skill at seq %d", ErrNotFound, seq) + } + artifact = strings.TrimSpace(fact.Artifact) + doc = strings.TrimSpace(fact.Body) + digest = strings.TrimSpace(fact.Digest) + trust = strings.TrimSpace(fact.Trust) + if trust == "" { + trust = "authored" + } + // Record consumption through existing telemetry. + now := formatTime(time.Now()) + _, _ = s.db.Exec(`UPDATE facts SET uses = uses + 1, last_used = ? WHERE seq = ?`, now, seq) + return artifact, doc, digest, trust, nil +} + // RecordFactInjection attributes one bounded notebook batch to the node whose // context received it. Repeated calls are legal; outcome accounting counts a // fact's ride on a node once. @@ -1026,7 +1083,7 @@ func (s *Store) RecordTasteCandidate(nodeID, subject, body string) (Fact, error) if len(existing) > 0 { return Fact{}, fmt.Errorf("record taste candidate: %w: shelf %q is already open", ErrInvalid, scope) } - return s.recordFact(FactWriterDistiller, nodeID, scope, FactPreference, body, nil, 0, FactCandidate, "", false) + return s.recordFact(FactWriterDistiller, nodeID, scope, FactPreference, body, nil, 0, FactCandidate, "", "", false) } // PromoteTasteRule stands one candidate up as a rule the gate is held to. @@ -1059,7 +1116,7 @@ func (s *Store) restandTasteRule(seq int64, status string) (Fact, error) { return Fact{}, fmt.Errorf("restand taste rule: %w: rule %d is %s", ErrInvalid, seq, rule.Status) } return s.recordFact(FactWriterDistiller, rule.NodeID, rule.Scope, FactPreference, rule.Body, - nil, seq, status, "", false) + nil, seq, status, "", "", false) } // NeighbouringCorrections ranks the corrections already in the notebook @@ -1336,7 +1393,7 @@ func (s *Store) searchFacts(query FactQuery, countUses bool) ([]Fact, error) { args = append(args, candidateDraw) rows, err := s.db.Query(` SELECT f.seq, f.ts, f.node_id, f.scope, f.kind, f.channel, f.body, f.unsettled, f.status, f.artifact, f.status_note, - f.status_seq, f.evidence_seq, f.status_origin, f.uses, f.last_used + f.status_seq, f.evidence_seq, f.status_origin, f.uses, f.last_used, f.trust, f.cost_card, f.digest FROM facts_fts JOIN facts AS f ON f.seq = facts_fts.rowid WHERE facts_fts MATCH ? AND f.status = ?`+kindClause+` @@ -1554,7 +1611,7 @@ func (s *Store) Fact(seq int64) (Fact, bool, error) { func factInTx(tx *sql.Tx, seq int64) (Fact, bool, error) { rows, err := tx.Query(` SELECT seq, ts, node_id, scope, kind, channel, body, unsettled, status, artifact, status_note, - status_seq, evidence_seq, status_origin, uses, last_used + status_seq, evidence_seq, status_origin, uses, last_used, trust, cost_card, digest FROM facts WHERE seq = ?`, seq) if err != nil { return Fact{}, false, fmt.Errorf("query fact: %w", err) @@ -1569,7 +1626,7 @@ func factInTx(tx *sql.Tx, seq int64) (Fact, bool, error) { func (s *Store) factsWhere(where string, args ...any) ([]Fact, error) { rows, err := s.db.Query(` SELECT seq, ts, node_id, scope, kind, channel, body, unsettled, status, artifact, status_note, - status_seq, evidence_seq, status_origin, uses, last_used + status_seq, evidence_seq, status_origin, uses, last_used, trust, cost_card, digest FROM facts WHERE `+where, args...) if err != nil { return nil, fmt.Errorf("query facts: %w", err) @@ -1586,10 +1643,11 @@ func scanFacts(rows *sql.Rows) ([]Fact, error) { facts := make([]Fact, 0) for rows.Next() { var fact Fact - var timestamp, unsettled, lastUsed string + var timestamp, unsettled, lastUsed, costCardStr string if err := rows.Scan(&fact.Seq, ×tamp, &fact.NodeID, &fact.Scope, &fact.Kind, &fact.Channel, &fact.Body, &unsettled, &fact.Status, &fact.Artifact, &fact.StatusNote, - &fact.StatusSeq, &fact.EvidenceSeq, &fact.StatusOrigin, &fact.Uses, &lastUsed); err != nil { + &fact.StatusSeq, &fact.EvidenceSeq, &fact.StatusOrigin, &fact.Uses, &lastUsed, + &fact.Trust, &costCardStr, &fact.Digest); err != nil { return nil, fmt.Errorf("scan fact: %w", err) } at, err := parseTime(timestamp) @@ -1607,6 +1665,11 @@ func scanFacts(rows *sql.Rows) ([]Fact, error) { fact.LastUsed = used } } + if costCardStr != "" && costCardStr != "{}" { + if err := json.Unmarshal([]byte(costCardStr), &fact.CostCard); err != nil { + return nil, fmt.Errorf("decode cost card fact %d: %w", fact.Seq, err) + } + } facts = append(facts, fact) } if err := rows.Err(); err != nil { @@ -1659,9 +1722,10 @@ func applyFactView(tx *sql.Tx, payload factPayload, seq int64, at time.Time) err channel = FactChannelInferred } if _, err := tx.Exec(` - INSERT INTO facts (seq, ts, node_id, scope, kind, channel, body, unsettled, status, artifact, status_seq) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - seq, formatTime(at), payload.NodeID, scope, kind, channel, payload.Body, string(encoded), status, payload.Artifact, seq); err != nil { + INSERT INTO facts (seq, ts, node_id, scope, kind, channel, body, unsettled, status, artifact, status_seq, trust, cost_card, digest) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + seq, formatTime(at), payload.NodeID, scope, kind, channel, payload.Body, string(encoded), status, payload.Artifact, seq, + payload.Trust, defaultCostCard(payload.CostCard), payload.Digest); err != nil { return err } if status != FactActive { @@ -1689,9 +1753,10 @@ func requireUnsettledEvidence(tx *sql.Tx, pair UnsettledPair) error { func applyFactActivation(tx *sql.Tx, payload factActivatedPayload) error { result, err := tx.Exec(` - UPDATE facts SET status = ?, artifact = ?, status_note = '' + UPDATE facts SET status = ?, artifact = ?, status_note = '', digest = ? WHERE seq = ? AND kind = ? AND status = ?`, - FactActive, payload.Artifact, payload.FactSeq, FactSkill, FactCandidate) + FactActive, payload.Artifact, payload.Digest, + payload.FactSeq, FactSkill, FactCandidate) if err != nil { return err } @@ -2001,6 +2066,14 @@ func defaultFactStatus(kind FactKind) string { return FactActive } +// defaultCostCard ensures cost_card is valid JSON for CHECK constraints. +func defaultCostCard(card string) string { + if card == "" || card == "null" { + return "{}" + } + return card +} + func validFactChangeOrigin(origin FactChangeOrigin) bool { switch origin { case FactOriginUser, FactOriginCLI, FactOriginConsolidator, FactOriginSupersession: @@ -2020,6 +2093,7 @@ func migrateFactsSchema(db *sql.DB) error { } hasScope, hasUnsettled, hasArtifact, hasStatusNote, hasChannel := false, false, false, false, false hasStatusSeq, hasEvidenceSeq, hasStatusOrigin := false, false, false + hasTrust, hasCostCard, hasDigest := false, false, false for rows.Next() { var cid int var name, kind string @@ -2046,6 +2120,12 @@ func migrateFactsSchema(db *sql.DB) error { hasStatusOrigin = true case "channel": hasChannel = true + case "trust": + hasTrust = true + case "cost_card": + hasCostCard = true + case "digest": + hasDigest = true } } if err := rows.Err(); err != nil { @@ -2059,6 +2139,7 @@ func migrateFactsSchema(db *sql.DB) error { } if hasScope && hasUnsettled && hasArtifact && hasStatusNote && hasStatusSeq && hasEvidenceSeq && hasStatusOrigin && hasChannel && + hasTrust && hasCostCard && hasDigest && strings.Contains(createSQL, "'unsettled'") && strings.Contains(createSQL, "'skill'") && strings.Contains(createSQL, "'playbook'") && strings.Contains(createSQL, "'question'") && strings.Contains(createSQL, "'trait'") && strings.Contains(createSQL, "'practicing'") && diff --git a/internal/store/meta.go b/internal/store/meta.go index 0731106c6..182107faf 100644 --- a/internal/store/meta.go +++ b/internal/store/meta.go @@ -244,7 +244,7 @@ func (s *Store) RecordTrait(name string, measurement TraitMeasurement) (Fact, er replaces = existing[0].Seq } return s.recordFact(FactWriterDistiller, RootID, scope, FactTrait, string(body), nil, - replaces, FactActive, "", false) + replaces, FactActive, "", "", false) } // Trait returns the current singleton measurement for name. diff --git a/internal/store/practice.go b/internal/store/practice.go index b3c453028..b4cffea78 100644 --- a/internal/store/practice.go +++ b/internal/store/practice.go @@ -132,7 +132,7 @@ func (s *Store) RecordQuestion(nodeID, scope, body string) (Fact, error) { if len(existing) > 0 { return existing[0], nil } - return s.recordFact(FactWriterOther, nodeID, resolved, FactQuestion, body, nil, 0, QuestionOpen, "", false) + return s.recordFact(FactWriterOther, nodeID, resolved, FactQuestion, body, nil, 0, QuestionOpen, "", "", false) } // Questions lists knowledge gaps in newest-first order. Empty status includes diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 99c6ab911..812aad137 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -513,7 +513,7 @@ func TestSkillFactStatusTransitionsSurviveRebuild(t *testing.T) { } installed := "/home/test/.codeaf/skills/git-audit" - if err := graph.ActivateSkill(first.Seq, installed); err != nil { + if err := graph.ActivateSkill(first.Seq, installed, ""); err != nil { t.Fatal(err) } const failure = "check.sh exited 7: fixture rejected" @@ -597,6 +597,127 @@ func TestPlaybookFactsAndSupersessionSurviveRebuild(t *testing.T) { } } +// Trust/CostCard/Digest persist through candidate creation and activation. +func TestSkillThreeNewFieldsPersistAndQuery(t *testing.T) { + graph := openTestStore(t, filepath.Join(t.TempDir(), "skill-fields.db")) + candidate, err := graph.RecordSkillCandidate("", "tool:git", + "git-scan checks for secrets", "/workspace/git-scan", "imported-provisional") + if err != nil { + t.Fatal(err) + } + if candidate.Trust != "imported-provisional" { + t.Fatalf("candidate trust = %q, want 'imported-provisional'", candidate.Trust) + } + if err := graph.ActivateSkill(candidate.Seq, "/installed/git-scan", "abc123digest"); err != nil { + t.Fatal(err) + } + active, err := graph.SkillFacts(FactActive, 10) + if err != nil || len(active) != 1 { + t.Fatalf("active skills = %+v err=%v", active, err) + } + if active[0].Trust != "imported-provisional" { + t.Fatalf("active trust = %q, want 'imported-provisional'", active[0].Trust) + } + if active[0].Digest != "abc123digest" { + t.Fatalf("active digest = %q, want 'abc123digest'", active[0].Digest) + } + if active[0].CostCard.RunTokens != 0 || active[0].CostCard.ReadTokens != 0 || active[0].CostCard.DelegateTokens != 0 { + t.Fatalf("active cost_card should be zero-valued: %+v", active[0].CostCard) + } + // Survive rebuild. + if err := graph.Rebuild(); err != nil { + t.Fatal(err) + } + after, err := graph.SkillFacts(FactActive, 10) + if err != nil || len(after) != 1 { + t.Fatalf("after rebuild: skills = %+v err=%v", after, err) + } + if after[0].Trust != "imported-provisional" || after[0].Digest != "abc123digest" { + t.Fatalf("after rebuild: trust=%q digest=%q", after[0].Trust, after[0].Digest) + } +} + +// Serving a skill (SkillFactAccessors) increments Uses exactly once and sets LastUsed. +func TestSkillFactAccessorsRecordsUseOnce(t *testing.T) { + graph := openTestStore(t, filepath.Join(t.TempDir(), "skill-serve.db")) + candidate, err := graph.RecordSkillCandidate("", "tool:format", + "go-format formats Go code", "/workspace/go-format") + if err != nil { + t.Fatal(err) + } + if err := graph.ActivateSkill(candidate.Seq, "/installed/go-format", ""); err != nil { + t.Fatal(err) + } + fact, found, err := graph.FactBySeq(candidate.Seq) + if err != nil || !found { + t.Fatalf("fact by seq: found=%v err=%v", found, err) + } + before := fact.Uses + + artifact, doc, digest, trust, err := graph.SkillFactAccessors(candidate.Seq) + if err != nil { + t.Fatal(err) + } + if artifact != "/installed/go-format" { + t.Fatalf("artifact = %q", artifact) + } + if doc != "go-format formats Go code" { + t.Fatalf("doc = %q", doc) + } + if trust != "authored" { + t.Fatalf("trust = %q, want 'authored'", trust) + } + if digest != "" { + t.Fatalf("digest = %q, want empty", digest) + } + // Uses should be exactly before+1. + fact, found, err = graph.FactBySeq(candidate.Seq) + if err != nil || !found { + t.Fatalf("fact by seq after serve: found=%v err=%v", found, err) + } + if fact.Uses != before+1 { + t.Fatalf("Uses: before=%d after=%d, want %d", before, fact.Uses, before+1) + } + if fact.LastUsed.IsZero() { + t.Fatal("LastUsed should be set after serving") + } + // Second call increments again. + _, _, _, _, err = graph.SkillFactAccessors(candidate.Seq) + if err != nil { + t.Fatal(err) + } + fact, found, err = graph.FactBySeq(candidate.Seq) + if err != nil || !found { + t.Fatalf("fact by seq: found=%v err=%v", found, err) + } + if fact.Uses != before+2 { + t.Fatalf("Uses after second serve: got %d, want %d", fact.Uses, before+2) + } +} + +// Trust defaults to "authored" when the stored value is empty. +func TestTrustDefaultsToAuthored(t *testing.T) { + graph := openTestStore(t, filepath.Join(t.TempDir(), "trust-default.db")) + candidate, err := graph.RecordSkillCandidate("", "tool:lint", + "go-lint lints Go code", "/workspace/go-lint") + if err != nil { + t.Fatal(err) + } + if candidate.Trust != "" { + t.Fatalf("empty trust should store as empty string, got %q", candidate.Trust) + } + if err := graph.ActivateSkill(candidate.Seq, "/installed/go-lint", ""); err != nil { + t.Fatal(err) + } + _, _, _, trust, err := graph.SkillFactAccessors(candidate.Seq) + if err != nil { + t.Fatal(err) + } + if trust != "authored" { + t.Fatalf("SkillFactAccessors trust = %q, want 'authored'", trust) + } +} + func openTestStore(t *testing.T, path string) *Store { t.Helper() store, err := Open(path) From 12b4b6e1b0c633522834e1464c4583cc98e0336d Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Sun, 20 Sep 2026 22:04:09 -0400 Subject: [PATCH 04/14] docs: changelog entry for skills tranche 1 Co-Authored-By: codeaf Assisted-by: CodeAF (moonshotai/kimi-k3) --- docs/changes/unreleased/1327-skills-tranche-1.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 docs/changes/unreleased/1327-skills-tranche-1.md diff --git a/docs/changes/unreleased/1327-skills-tranche-1.md b/docs/changes/unreleased/1327-skills-tranche-1.md new file mode 100644 index 000000000..7a007df59 --- /dev/null +++ b/docs/changes/unreleased/1327-skills-tranche-1.md @@ -0,0 +1,15 @@ +--- +kind: added +title: Skill facts gain trust, cost card and digest; briefs gain an ordered skills list +pr: 1327 +surface: [engine, resident, chat] +invalidates: + - "A skill's fact record carried only doc, scope, artifact, status and provenance. It now also carries trust (defaulting to authored), a cost card, and a sha256 content digest computed at install, and ActivateSkill takes that digest as a third argument — the two-argument call form no longer exists anywhere in the tree." + - "store.NodeBrief had no skills field. It now journals an ordered Skills []string with the brief, and the worker prompt renders an attachment block (one doc line plus one shelf path per skill, earlier entries win conflicts) whenever the list is non-empty." +--- + +Tranche 1 of skills as attachments: the record work (trust, cost card, digest, +consumption-marked serving) and the attachment plumbing (ordered skills on briefs) +land first; the chat shelf, the use_skill worker tool, and the live wiring that +populates briefs from real proposals are landing on the same pull request. The +design and its reasoning are on issue #1277. From 7aded507830a5af1e17b68b4e3ce60ff7f4f9fe6 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 22:11:26 -0400 Subject: [PATCH 05/14] task: Finish skills shelf and use_skill: run the right suite, land the branch Co-Authored-By: codeaf Assisted-by: CodeAF (z-ai/glm-5.3-flash) --- internal/session/actioncategory.go | 2 +- internal/session/beltfacts.go | 10 + internal/session/prefixbudget_test.go | 15 +- internal/session/prompt.go | 8 + internal/session/questionlifecycle_test.go | 9 +- internal/session/skillcatalog.go | 198 ++++++++++++++++++++ internal/session/skillcatalog_test.go | 166 ++++++++++++++++ internal/session/task_rename_compat_test.go | 8 +- internal/session/task_run.go | 9 + internal/session/tools.go | 5 + internal/session/tools_skill.go | 146 +++++++++++++++ internal/session/tools_skill_test.go | 128 +++++++++++++ 12 files changed, 697 insertions(+), 7 deletions(-) create mode 100644 internal/session/skillcatalog.go create mode 100644 internal/session/skillcatalog_test.go create mode 100644 internal/session/tools_skill.go create mode 100644 internal/session/tools_skill_test.go diff --git a/internal/session/actioncategory.go b/internal/session/actioncategory.go index ac41d0f03..96c8e3d05 100644 --- a/internal/session/actioncategory.go +++ b/internal/session/actioncategory.go @@ -218,7 +218,7 @@ func ActionCategoryForTool(tool string) ActionCategory { // Opening what is already located, and listing what is there. case "read", "read_document", "ls", "manual", "view_image", "settings", - "tasks", "jobs", "list_harnesses", "list_subharnesses", "services", + "use_skill", "tasks", "jobs", "list_harnesses", "list_subharnesses", "services", "gmail_read", "slack_read_thread", "slack_list_channels", "calendar_list", "workspace_snapshots": return ActionRead diff --git a/internal/session/beltfacts.go b/internal/session/beltfacts.go index 5359ea480..232e85c8c 100644 --- a/internal/session/beltfacts.go +++ b/internal/session/beltfacts.go @@ -279,6 +279,16 @@ var beltFacts = []beltFact{{ holds: Config.hasConversationHistory, present: "- When asked to find a past conversation or report what was said or decided elsewhere, call `search_conversations` BEFORE answering, even if a saved memory suggests the answer. Memories guide the query; source messages establish what was said. Copy a returned ref to read more and check corrections.", absent: "- What was said in earlier conversations cannot be looked up from here, so answer out of what is in this window rather than reconstructing it.", +}, { + // THE SKILL SHELF, on the same predicate as propose_task plus a store to + // read it from (tools_skill.go's [Agent.useSkillTool]): a worker that may + // hand work out may also look up what this project already knows how to do, + // and a shape with no shelf behind it is told the shelf is not reachable + // rather than reaching for a verb that is not on its belt. + tools: []string{useSkillToolName}, + holds: func(c Config) bool { return c.mayProposeTask() && c.Memory != nil }, + present: "- `use_skill` lists active skills (names + one-line docs) or resolves one by name to its shelf path.", + absent: "- Skills on the shelf are not reachable from here.", }, { tools: []string{"watch"}, holds: Config.mayWatch, diff --git a/internal/session/prefixbudget_test.go b/internal/session/prefixbudget_test.go index c3cad388b..6b5499d54 100644 --- a/internal/session/prefixbudget_test.go +++ b/internal/session/prefixbudget_test.go @@ -439,6 +439,17 @@ const fixedPrefixTarget = 48_000 // second round, and cutting other laws in the first would be that round done // early and unreviewed. // +// 2026-09-20, the use_skill wave. A worker gained one verb it did not have: +// `use_skill`, which lists the active skill shelf and resolves one name to its +// shelf path (tools_skill.go), and the page gained the one belt-fact bullet that +// says the shelf is reachable (beltfacts.go). It is a NEW CAPABILITY rather than +// a second copy of a law — nothing on this belt already let a worker reach a +// saved procedure — so there was no sentence to take the bytes out of. It paid +// 741 bytes on the fixed arm (55,280 to 56,021) and 755 on the lean arm (47,055 +// to 47,810), and both waivers rise by that figure here, in this diff, on +// purpose: the rule says a raise is a decision with a name on it, and the +// alternative was cutting the verb the wave exists to add. +// // 2026-09-16, #1067 review. The provenance list had to grow because compaction // also writes user-role tags into the conversation: `[folded …]` and `[context // compacted]`. It also stopped calling the tool-only `[held]` a user message or @@ -449,8 +460,8 @@ const fixedPrefixTarget = 48_000 // sentence paid for the truth with room to spare: fixed is 55,280 and lean is // 47,055, so both waivers fall by 31 and again sit exactly on the measurement. const ( - fixedPrefixWaiver = 7_280 - leanPrefixWaiver = 15_555 + fixedPrefixWaiver = 8_021 + leanPrefixWaiver = 16_310 ) // THE LEAN PROFILE GETS A BUDGET OF ITS OWN (2026-09-10, the prompt diet's lane diff --git a/internal/session/prompt.go b/internal/session/prompt.go index 00691d390..83e3e9494 100644 --- a/internal/session/prompt.go +++ b/internal/session/prompt.go @@ -299,6 +299,14 @@ func renderSystemAt(config Config, now time.Time) string { out.WriteString("\n\n") out.WriteString(strings.TrimRight(quickPrompt, "\n")) } + // AND THE SHELF THIS CONVERSATION ALREADY OWNS, when it has one. The skill + // catalog is dynamic CONTENT rather than a fact about the shape, so it is + // composed here from the store and not from beltfacts.go, and it renders + // nothing at all on an empty shelf (skillcatalog.go). + if catalog := renderSkillCatalog(config); catalog != "" { + out.WriteString("\n\n") + out.WriteString(catalog) + } out.WriteString("\n\n# Project\n") fmt.Fprintf(&out, "- Workstation: %s/%s\n", runtime.GOOS, runtime.GOARCH) diff --git a/internal/session/questionlifecycle_test.go b/internal/session/questionlifecycle_test.go index c71118c29..440163f09 100644 --- a/internal/session/questionlifecycle_test.go +++ b/internal/session/questionlifecycle_test.go @@ -283,7 +283,10 @@ func TestAReRaisedLandingCarriesItsAnswerFate(t *testing.T) { if again.Kind != EventQuestion || again.Question == nil { t.Fatalf("the re-raise did not go out: %+v", again) } - if !strings.HasPrefix(again.Question.Reason, "accepted 18:20") { + // THE ANSWER IS PINNED TO A DAY THAT IS ALWAYS PAST by the time this runs, + // so the stamp leads with its day (landingAnsweredStamp): a bare `18:20` + // on a card drawn later reads as an hour ago. + if !strings.HasPrefix(again.Question.Reason, "accepted Sep 16 18:20") { t.Fatalf("the re-raised card does not lead with the answer's fate: %q", again.Question.Reason) } } @@ -390,7 +393,9 @@ func TestTheTerminalNoticeRetiresTheFlightStamp(t *testing.T) { if strings.Contains(terminal.Question.Reason, "still working on it") { t.Fatalf("the flight stamp outlived the flight: %q", terminal.Question.Reason) } - if !strings.HasPrefix(terminal.Question.Reason, "handed it to codeaf 18:20") { + // AND THE STAMP SAYS ITS DAY, because the pinned answer is always a past + // day by the time this runs (landingAnsweredStamp's own law). + if !strings.HasPrefix(terminal.Question.Reason, "handed it to codeaf Sep 16 18:20") { t.Fatalf("the terminal card does not lead with the answer's fate: %q", terminal.Question.Reason) } } diff --git a/internal/session/skillcatalog.go b/internal/session/skillcatalog.go new file mode 100644 index 000000000..64c69eede --- /dev/null +++ b/internal/session/skillcatalog.go @@ -0,0 +1,198 @@ +package session + +import ( + "path/filepath" + "sort" + "strconv" + "strings" + "time" + + store "github.com/Agent-Field/codeaf/internal/store" +) + +// THE SKILL CATALOG is the one place the conversation is shown the shelf it +// already owns: the skills the distiller has promoted and the resident has +// installed, each named by the command a worker would run and described by the +// line the notebook holds. It is DYNAMIC CONTENT rendered as a section, not a +// session fact — beltfacts.go's facts are sentences about which TOOLS a belt +// carries, and a skill is neither a tool nor a property of the shape: which +// ones exist depends on the notebook, not on the config. +// +// IT IS WINDOWED ON PURPOSE. This section is prepended to every request, so its +// bytes are paid on every round of every turn (prefixbudget_test.go). A person +// whose shelf has grown past a handful of entries must not pay for the tail on +// every call, so the catalog scores the skills against the little context the +// prompt has — the workspace, the project's name, the date — and keeps only the +// few most likely to matter. +const ( + // skillCatalogMaxLines bounds how many skill bullets the catalog may carry. + // + // IT IS SMALL BECAUSE IT IS A PROMPT PREFIX BUDGET. The section rides in + // front of every request, so every line is bought again on each round of + // each turn; eight lines is about the size of the belt-fact sections it sits + // beside, and anything past it is a roll call rather than a menu. The + // overflow is not hidden — it is summarised as "- … and N more skills", and + // the whole shelf is always one `recall` away. + skillCatalogMaxLines = 8 + + // skillCatalogScanLimit is how far into the active shelf the catalog reads + // before it scores. It is deliberately larger than [skillCatalogMaxLines] so + // the scorer has something to choose BETWEEN rather than merely the newest + // handful, and small enough that a machine with a thousand skills still + // reads a bounded slice per render. + skillCatalogScanLimit = 50 + + // skillCatalogHeader is the section heading and the one sentence saying how + // a skill is reached. It uses `## ` and not `# ` because a top-level heading + // is the unit the lean profile drops (promptprofile.go's [leanPageSections]), + // and the shelf is not a law to be traded against window size. + skillCatalogHeader = "## Available skills\n\n" + + "Chat routes skills to tasks — use them through task nodes, never inline.\n" +) + +// renderSkillCatalog composes the skill catalog for one config, or the empty +// string when there is nothing to show. +// +// AN EMPTY SHELF IS ZERO BYTES, which is the whole reason the section is +// conditional: a person with no skills must not pay a heading that names +// nothing, and a store-less shape (a standing check's probe, a fork's hand) must +// render byte-for-byte what it rendered before this file existed. +func renderSkillCatalog(config Config) string { + if config.Memory == nil { + return "" + } + active, err := config.Memory.SkillFacts(store.FactActive, skillCatalogScanLimit) + if err != nil || len(active) == 0 { + return "" + } + + scored := scoreSkills(active, config.Workspace) + sort.SliceStable(scored, func(first, second int) bool { + return scored[first].score > scored[second].score + }) + + shown := scored + hidden := 0 + if len(shown) > skillCatalogMaxLines { + hidden = len(shown) - skillCatalogMaxLines + shown = shown[:skillCatalogMaxLines] + } + + var out strings.Builder + out.WriteString(skillCatalogHeader) + out.WriteByte('\n') + for _, skill := range shown { + name := filepath.Base(strings.TrimSpace(skill.fact.Artifact)) + if name == "" || name == "." { + name = strings.TrimSpace(skill.fact.Scope) + } + out.WriteString("- ") + out.WriteString(name) + out.WriteString(": ") + out.WriteString(strings.TrimSpace(skill.fact.Body)) + out.WriteByte('\n') + } + if hidden > 0 { + out.WriteString("- … and ") + out.WriteString(strconv.Itoa(hidden)) + out.WriteString(" more skills\n") + } + return strings.TrimRight(out.String(), "\n") +} + +// scoredSkill is one skill and the weight this prompt gave it. +type scoredSkill struct { + fact store.Fact + score int +} + +// scoreSkills ranks the active shelf against what the prompt knows about this +// moment. It is a HEURISTIC and deliberately not a model call: a weighted sum of +// three cheap signals, in the order they matter. +// +// - SCOPE MATCH — a skill whose scope names something in front of the model +// (the workspace, the folder under it, the project's name) is almost +// certainly about the work at hand, so it outweighs anything else. +// - WORD OVERLAP — a doc line sharing words with that same context is a +// weaker, fuzzier signal of relevance, so it is a smaller bonus per word. +// - RECENCY — a skill used more recently is likelier to be the one wanted, so +// fresh use is a small tie-breaker. Most skills have never been used, and +// [Fact.LastUsed] is zero for them, which is exactly the stable default the +// sort keeps in journal order (newest first, as [Store.SkillFacts] returns +// them). +func scoreSkills(facts []store.Fact, workspace string) []scoredSkill { + context := contextWords(workspace) + now := time.Now() + scored := make([]scoredSkill, 0, len(facts)) + for _, fact := range facts { + score := 0 + if scopeMatchesContext(fact.Scope, context) { + score += 100 + } + for _, word := range docWords(fact.Body) { + if context[word] { + score += 5 + } + } + if !fact.LastUsed.IsZero() { + // A recency bonus that stays under the scope and word weights: used + // within the last week scores highest, and anything older than a + // month is a tie-breaker at most. + switch age := now.Sub(fact.LastUsed); { + case age < 7*24*time.Hour: + score += 3 + case age < 30*24*time.Hour: + score += 1 + } + } + scored = append(scored, scoredSkill{fact: fact, score: score}) + } + return scored +} + +// contextWords is the set of lowercase words the prompt already knows: the +// workspace path's own components. It is the whole of the "prompt context" +// available to a function handed only a [Config], and it is enough — a skill +// scoped to `repo:/…/codeaf` or describing a path under the working directory +// shares a word with it. +func contextWords(workspace string) map[string]bool { + words := make(map[string]bool) + for _, part := range strings.FieldsFunc(workspace, func(r rune) bool { + return r == '/' || r == '\\' || r == ':' || r == '.' || r == '-' || r == '_' || r == ' ' + }) { + if part = strings.ToLower(strings.TrimSpace(part)); part != "" { + words[part] = true + } + } + return words +} + +// docWords is the comparable words of one doc line. +func docWords(doc string) []string { + fields := strings.FieldsFunc(strings.ToLower(doc), func(r rune) bool { + return !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') + }) + words := fields[:0] + for _, field := range fields { + if len(field) >= 3 { + words = append(words, field) + } + } + return words +} + +// scopeMatchesContext says whether a skill's scope names something the prompt +// carries. A scope is `kind:value` ("repo:/path", "tool:git", "domain:x"), so +// each side is compared against the context words: a scope of `repo:/…/codeaf` +// matches a workspace that ends in `codeaf`, and `tool:git` matches nothing +// unless the path happens to say `git`. +func scopeMatchesContext(scope string, context map[string]bool) bool { + for _, part := range strings.FieldsFunc(strings.ToLower(scope), func(r rune) bool { + return r == ':' || r == '/' || r == '\\' || r == '.' || r == '-' || r == '_' || r == ' ' + }) { + if part != "" && context[part] { + return true + } + } + return false +} diff --git a/internal/session/skillcatalog_test.go b/internal/session/skillcatalog_test.go new file mode 100644 index 000000000..fa4071b2e --- /dev/null +++ b/internal/session/skillcatalog_test.go @@ -0,0 +1,166 @@ +package session + +import ( + "strconv" + "strings" + "testing" + "time" + + store "github.com/Agent-Field/codeaf/internal/store" +) + +// activeSkill records a candidate and activates it in one step — the only +// transition that puts a skill on the shelf the catalog reads (store's +// [Store.SkillFacts] returns ACTIVE skills, and a candidate is deliberately +// absent from every retrieval surface until its trial goes green). +func activeSkill(t *testing.T, brain *store.Store, scope, body, artifact string) store.Fact { + t.Helper() + candidate, err := brain.RecordSkillCandidate(store.RootID, scope, body, artifact) + if err != nil { + t.Fatalf("record skill candidate %q: %v", body, err) + } + if err := brain.ActivateSkill(candidate.Seq, artifact); err != nil { + t.Fatalf("activate skill %q: %v", body, err) + } + return candidate +} + +// bulletLines is the skill bullets and nothing else: the section header, the +// routing sentence and the "- … and N more skills" overflow line are all not +// one skill. +func bulletLines(catalog string) []string { + bullets := make([]string, 0, skillCatalogMaxLines) + for _, line := range strings.Split(catalog, "\n") { + if strings.HasPrefix(line, "- ") && !strings.Contains(line, "more skills") { + bullets = append(bullets, line) + } + } + return bullets +} + +// TestSkillCatalogWindowsALargeShelf: a shelf past the cap renders exactly +// [skillCatalogMaxLines] bullets and one overflow line naming what did not fit, +// so the section can never grow with the notebook. +func TestSkillCatalogWindowsALargeShelf(t *testing.T) { + brain := openTestBrain(t) + for index := 0; index < 15; index++ { + activeSkill(t, brain, + "domain:alpha", + "skill number "+strconv.Itoa(index)+" checks a thing", + "/shelf/skill-"+strconv.Itoa(index), + ) + } + + catalog := renderSkillCatalog(Config{Memory: brain, Workspace: "/srv/app"}) + if catalog == "" { + t.Fatal("a shelf of fifteen skills rendered nothing") + } + bullets := bulletLines(catalog) + if len(bullets) != skillCatalogMaxLines { + t.Fatalf("catalog carries %d bullets, want the %d-line cap:\n%s", len(bullets), skillCatalogMaxLines, catalog) + } + // 15 skills, 8 shown: the overflow line names the other 7. + if !strings.Contains(catalog, "… and 7 more skills") { + t.Fatalf("catalog overflow line is wrong, want \"… and 7 more skills\":\n%s", catalog) + } +} + +// TestSkillCatalogIsEmptyWithoutSkills: no active skills is zero bytes — the +// whole reason the section is conditional rather than a heading that names +// nothing on every request. +func TestSkillCatalogIsEmptyWithoutSkills(t *testing.T) { + if catalog := renderSkillCatalog(Config{}); catalog != "" { + t.Fatalf("a config with no store rendered %q, want the empty string", catalog) + } + brain := openTestBrain(t) + if catalog := renderSkillCatalog(Config{Memory: brain, Workspace: "/srv/app"}); catalog != "" { + t.Fatalf("an empty shelf rendered %q, want the empty string", catalog) + } + // A CANDIDATE IS NOT ON THE SHELF: it is recorded and never activated, so + // the catalog must not offer a skill the trial never promoted. + if _, err := brain.RecordSkillCandidate(store.RootID, "domain:alpha", "candidate not yet promoted", "/shelf/pending"); err != nil { + t.Fatalf("record candidate: %v", err) + } + if catalog := renderSkillCatalog(Config{Memory: brain, Workspace: "/srv/app"}); catalog != "" { + t.Fatalf("a shelf of only candidates rendered %q, want the empty string", catalog) + } +} + +// TestSkillCatalogSurfacesRelevantSkills: the scorer ranks a skill whose scope +// or doc shares words with the workspace above an unrelated shelf, so the ones +// kept under the window are the likely ones. +func TestSkillCatalogSurfacesRelevantSkills(t *testing.T) { + brain := openTestBrain(t) + // Ten unrelated skills, inserted FIRST so seq order would show them first. + for index := 0; index < 10; index++ { + activeSkill(t, brain, + "domain:unrelated", + "unrelated procedure number "+strconv.Itoa(index), + "/shelf/unrelated-"+strconv.Itoa(index), + ) + } + // One scoped to the workspace, one whose doc shares a word with it. + activeSkill(t, brain, "repo:/srv/app", "scoped to the working directory", "/shelf/scoped-skill") + activeSkill(t, brain, "domain:misc", "inspects the app before delivery", "/shelf/app-check") + + catalog := renderSkillCatalog(Config{Memory: brain, Workspace: "/srv/app"}) + for _, want := range []string{"scoped-skill", "app-check"} { + if !strings.Contains(catalog, want) { + t.Errorf("relevant skill %q is missing from the catalog:\n%s", want, catalog) + } + } + // And they are ranked ABOVE the unrelated tail, which is the whole point of + // scoring: the first bullet is one of the two relevant ones. + bullets := bulletLines(catalog) + if len(bullets) == 0 { + t.Fatal("catalog rendered no bullets") + } + if !strings.Contains(bullets[0], "scoped-skill") && !strings.Contains(bullets[0], "app-check") { + t.Errorf("the top bullet is not a relevant skill: %q", bullets[0]) + } +} + +// TestSkillCatalogAlwaysCarriesItsHeader: whenever there is anything to show, +// the section heading and the routing sentence are present — the model is told +// these are skills reached through task nodes, not verbs to run inline. +func TestSkillCatalogAlwaysCarriesItsHeader(t *testing.T) { + brain := openTestBrain(t) + activeSkill(t, brain, "domain:alpha", "one skill on the shelf", "/shelf/only-skill") + + catalog := renderSkillCatalog(Config{Memory: brain, Workspace: "/srv/app"}) + if !strings.HasPrefix(catalog, "## Available skills\n") { + t.Fatalf("catalog does not open on its heading:\n%s", catalog) + } + if !strings.Contains(catalog, "use them through task nodes, never inline") { + t.Fatalf("catalog does not carry the routing sentence:\n%s", catalog) + } + if !strings.Contains(catalog, "- only-skill: one skill on the shelf") { + t.Fatalf("catalog does not name the skill and its doc:\n%s", catalog) + } +} + +// TestSkillCatalogRendersOnThePageWhenSkillsExist: the section is wired into +// renderSystemAt, so a conversation with a shelf reads it and one without does +// not. +func TestSkillCatalogRendersOnThePageWhenSkillsExist(t *testing.T) { + brain := openTestBrain(t) + activeSkill(t, brain, "domain:alpha", "audits a delivery", "/shelf/delivery-audit") + + now := time.Date(2026, 9, 2, 10, 0, 0, 0, time.UTC) + withShelf := renderSystemAt(Config{Memory: brain, Workspace: "/srv/app"}, now) + if !strings.Contains(withShelf, "## Available skills") { + t.Fatalf("a conversation with a shelf does not read the catalog:\n%s", withShelf) + } + if !strings.Contains(withShelf, "delivery-audit") { + t.Fatalf("the page does not name the shelf's skill:\n%s", withShelf) + } + // It sits before `# Project`, where the section belongs. + if strings.Index(withShelf, "## Available skills") > strings.Index(withShelf, "# Project") { + t.Fatalf("the catalog landed after `# Project`") + } + + withoutShelf := renderSystemAt(Config{Workspace: "/srv/app"}, now) + if strings.Contains(withoutShelf, "## Available skills") { + t.Fatalf("a conversation with no store reads a catalog:\n%s", withoutShelf) + } +} diff --git a/internal/session/task_rename_compat_test.go b/internal/session/task_rename_compat_test.go index 0b54c09ed..c3a2b3fe4 100644 --- a/internal/session/task_rename_compat_test.go +++ b/internal/session/task_rename_compat_test.go @@ -86,7 +86,11 @@ func TestLegacyRegisteredWorktreeResumesLandsAndCleansUp(t *testing.T) { t.Fatal("repository lock could not be opened") } defer lock.Close() - wantLock := filepath.Join(repo, filepath.FromSlash(legacyTasksDirName), gitRootLockName) + // THE LOCK'S OWN BOUNDARY CANONICALIZES THE ROOT — two spellings of one + // repository must never make two locks — so the expectation is spelled the + // way this filesystem spells it: /var/… and /private/var/… are one directory + // on a Mac, and only the resolved form is the one git and the lock share. + wantLock := canonicalPath(filepath.Join(repo, filepath.FromSlash(legacyTasksDirName), gitRootLockName)) if lock.Name() != wantLock { t.Fatalf("repository lock = %q, want pre-rename lock %q", lock.Name(), wantLock) } @@ -96,7 +100,7 @@ func TestLegacyRegisteredWorktreeResumesLandsAndCleansUp(t *testing.T) { t.Fatal("fresh repository lock could not be opened") } defer freshLock.Close() - if want := filepath.Join(fresh, filepath.FromSlash(tasksDirName), gitRootLockName); freshLock.Name() != want { + if want := canonicalPath(filepath.Join(fresh, filepath.FromSlash(tasksDirName), gitRootLockName)); freshLock.Name() != want { t.Fatalf("fresh repository lock = %q, want current path %q", freshLock.Name(), want) } if _, err := os.Stat(filepath.Join(fresh, filepath.FromSlash(legacyTasksDirName))); !os.IsNotExist(err) { diff --git a/internal/session/task_run.go b/internal/session/task_run.go index 0e71f27d1..7a1b3c03d 100644 --- a/internal/session/task_run.go +++ b/internal/session/task_run.go @@ -5952,6 +5952,15 @@ func (n *TaskNode) resumeTree(place Place, workspace string) (taskTree, bool) { n.graph.mu.Unlock() if interrupted && strings.TrimSpace(dir) != "" { if info, err := os.Stat(dir); err == nil && info.IsDir() { + // AND THE CHECKPOINT'S SPELLING IS BROUGHT UP TO GIT'S. Git resolves + // symlinks before it registers a worktree, and taskOwnFolder records + // that same spelling so checkpoint, cleanup and git name one directory; + // a checkpoint written before that law can carry the raw path it was + // handed — /var/… where git says /private/var/… — and a tree resumed + // under the other spelling is one directory known to cleanup by two + // names. Stat runs first, so a copy that no longer exists still takes + // its not-resumed road. + dir = canonicalPath(dir) ground, mode := n.groundNow() if merge == mergeInPlace { // AND A RESUMED FAMILY REVALIDATES ITS TREE THROUGH THE ONE CALL THAT diff --git a/internal/session/tools.go b/internal/session/tools.go index 53b574ba0..8114ab774 100644 --- a/internal/session/tools.go +++ b/internal/session/tools.go @@ -187,6 +187,11 @@ func (a *Agent) belt() []bare.Tool { // (task_quick.go). The judge that decides between the two is written once, in // its description. tools = append(tools, a.quickTools()...) + // use_skill rides on the same predicate as propose_task, plus a store to read + // the shelf from: a worker that may hand work out may also look up what this + // project already knows how to do (tools_skill.go). A floor node is handed no + // store and no verb either way, so the two gates agree by construction. + tools = append(tools, a.useSkillTool()...) // items is the verb a QUICK WORKER carries and nothing else does: a node with // no list has no door behind the tool, so it is absent rather than present // and refusing — the law every conditional family on this belt is built on. diff --git a/internal/session/tools_skill.go b/internal/session/tools_skill.go new file mode 100644 index 000000000..cf8b355d6 --- /dev/null +++ b/internal/session/tools_skill.go @@ -0,0 +1,146 @@ +package session + +// The use_skill hand: the shelf of active skills, listed or resolved by name. +// +// A SKILL IS AN EXECUTION-VERIFIED PROCEDURE the distiller saved as a +// store.Fact of kind "skill", kept on a shelf directory its Artifact points at. +// Until now nothing a worker held could reach one: the shelf was written to and +// promoted, and the only reader was a person with the CLI. This is the worker's +// door onto it — mid-run discovery rather than a prompt fact, so a worker that +// finds itself doing something the shelf has a recipe for can fetch it. +// +// TWO MODES, ONE VERB, because the two questions come together: `list` shows +// what is there (name and the one-line doc, never a path or internal field), and +// `get` resolves one name to the shelf path the worker will actually open. It is +// one tool the way `jobs` and `settings` are one tool with an action, not two, +// because the model reaches for the list to decide whether the get is worth it. +// +// IT IS GATED EXACTLY AS propose_task IS ([Agent.mayProposeTask]) and on one +// thing more: a store to read the shelf FROM. A node on the floor of its tree +// already has no kids and is handed no verb to make any; the same shape has no +// business rummaging a shelf either, and an agent with no Memory has no shelf to +// read. So the belt and the page agree by construction: [Config.mayProposeTask] +// AND a non-nil store, which is the predicate the belt fact is composed from +// (beltfacts.go) and the gate this method reads. + +import ( + "context" + "encoding/json" + "fmt" + "path/filepath" + "strings" + + "github.com/Agent-Field/codeaf/internal/exec/bare" + "github.com/Agent-Field/codeaf/internal/store" +) + +const useSkillToolName = "use_skill" + +// skillShelfLimit bounds one listing. The shelf is a curated few, not a corpus, +// so a hundred is every skill any machine has ever held; a larger number would +// only change how much of a runaway shelf rides in one answer. +const skillShelfLimit = 100 + +// useSkillDescription says what the two modes are for in the model's own terms. +// It is bought on every request of every turn on a belt that carries it, so it +// names the gesture and nothing about the store behind it. +const useSkillDescription = "Reach the shelf of active skills — execution-verified procedures this project has saved. `list` shows every skill as a name and a one-line doc; `get` resolves one name to its shelf path and doc, which you then open with `read`." + +// useSkillSchemaJSON is the two modes. `name` is required for `get` alone, which +// the mode enum cannot express, so the handler refuses a nameless get in words +// rather than leaning on the schema. +const useSkillSchemaJSON = `{ + "type": "object", + "properties": { + "mode": {"type": "string", "enum": ["list", "get"], "description": "list: show doc lines for all active skills. get: show shelf path and doc for one skill."}, + "name": {"type": "string", "description": "Required for get mode. The skill name (directory name on the shelf)."} + }, + "required": ["mode"], + "additionalProperties": false +}` + +// useSkillTool is the verb, or nothing at all on a belt that may not have it. +// +// ABSENT-NOT-BROKEN, the law every conditional family on this belt is built on +// (tools.go): a model told it can reach a shelf it has no store behind will plan +// a reply around a call that can only refuse, so the verb is simply not there. +func (a *Agent) useSkillTool() []bare.Tool { + // The belt's gate and the page's predicate are one predicate + // (beltfacts.go's `use_skill` row holds this same line), so the sentence a + // shape reads can never promise a verb its belt withheld. + if !a.mayProposeTask() || a.config.Memory == nil { + return nil + } + return []bare.Tool{{ + Name: useSkillToolName, + Description: useSkillDescription, + Schema: json.RawMessage(useSkillSchemaJSON), + Execute: a.runUseSkill, + }} +} + +// runUseSkill renders the shelf. Every bad call is an ordinary tool result +// rather than a Go error, the way the rest of the belt answers: a mode spelled +// wrongly is a call the model can make again. +func (a *Agent) runUseSkill(_ context.Context, args json.RawMessage) (string, bool, error) { + var parsed struct { + Mode string `json:"mode"` + Name string `json:"name"` + } + if err := decodeToolArguments(args, &parsed); err != nil { + return invalidArgumentsPrefix + err.Error(), true, nil + } + switch strings.TrimSpace(parsed.Mode) { + case "list": + return a.listSkills() + case "get": + name := strings.TrimSpace(parsed.Name) + if name == "" { + return invalidArgumentsPrefix + `mode "get" needs a name — the skill's directory name, as the list shows it`, true, nil + } + return a.getSkill(name) + default: + return invalidArgumentsPrefix + `mode takes "list" or "get"`, true, nil + } +} + +// listSkills is the shelf as discovery rows: one skill per line, name and doc, +// and NOTHING ELSE. The artifact path is deliberately withheld here — a list of +// hundred-byte paths is noise the model has not asked to open yet — and the doc +// is the one-line Body the skill was recorded with. +func (a *Agent) listSkills() (string, bool, error) { + skills, err := a.config.Memory.SkillFacts(store.FactActive, skillShelfLimit) + if err != nil { + return "Could not read the skill shelf: " + err.Error(), true, nil + } + if len(skills) == 0 { + return "No active skills on the shelf.", false, nil + } + lines := make([]string, 0, len(skills)) + for _, skill := range skills { + lines = append(lines, "- "+filepath.Base(skill.Artifact)+": "+skill.Body) + } + return strings.Join(lines, "\n"), false, nil +} + +// getSkill resolves one name to the shelf path the worker will open and the doc +// that says what it is for. +// +// THE NAME IS MATCHED BY THE DIRECTORY ON THE SHELF, not by the fact's scope or +// id: what a worker has is the name `list` printed, which is filepath.Base of +// the artifact, and matching anything else would answer a name the model cannot +// see. +func (a *Agent) getSkill(name string) (string, bool, error) { + skills, err := a.config.Memory.SkillFacts(store.FactActive, skillShelfLimit) + if err != nil { + return "Could not read the skill shelf: " + err.Error(), true, nil + } + for _, skill := range skills { + if filepath.Base(skill.Artifact) != name { + continue + } + // TODO: call store.SkillServe when it exists to mark consumption for consolidation weighting + return fmt.Sprintf("%s: %s\nPath: %s", name, skill.Body, skill.Artifact), false, nil + } + return "Skill '" + name + "' not found.", false, nil +} diff --git a/internal/session/tools_skill_test.go b/internal/session/tools_skill_test.go new file mode 100644 index 000000000..854ce6aa8 --- /dev/null +++ b/internal/session/tools_skill_test.go @@ -0,0 +1,128 @@ +package session + +// The skill hand, driven the way the wire drives it: what a worker is handed, +// what one call answers with, and who does not get the verb at all. +// +// A skill is a store.Fact of kind "skill" that has been activated, whose +// Artifact is the directory on the shelf. These tests build the shelf the way +// the store builds it — a candidate recorded, then activated — so the reading +// path under test is the one a real shelf produces. + +import ( + "context" + "encoding/json" + "path/filepath" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/store" +) + +// shelfSkill records one skill candidate and activates it, which is the only +// transition that makes it retrievable. It answers the artifact path it was put +// on, for the assertions that must NOT see it in a listing. +func shelfSkill(t *testing.T, brain *store.Store, name, doc string) string { + t.Helper() + artifact := filepath.Join(t.TempDir(), "shelf", name) + // An empty node id is the root's own channel, which is what the store's + // own skill tests record on (exec_test.go, notebook_test.go). + candidate, err := brain.RecordSkillCandidate("", "repo:audit", doc, artifact) + if err != nil { + t.Fatalf("record skill %s: %v", name, err) + } + if err := brain.ActivateSkill(candidate.Seq, artifact); err != nil { + t.Fatalf("activate skill %s: %v", name, err) + } + return artifact +} + +// useSkill calls the tool the way the wire does. +func useSkill(t *testing.T, agent *Agent, args string) string { + t.Helper() + out, failed, err := agent.runUseSkill(context.Background(), json.RawMessage(args)) + if err != nil { + t.Fatalf("use_skill: %v", err) + } + if failed { + t.Fatalf("use_skill refused %s: %s", args, out) + } + return out +} + +// LIST SHOWS DOC LINES AND NOTHING ELSE: one skill per line, the name and its +// one-line doc, and never the shelf path or any internal field. A listing that +// leaked a path would spend the model's attention on a directory it has not +// asked to open. +func TestUseSkillList(t *testing.T) { + agent, brain := brainAgent(t, &scriptedCompleter{}, nil) + auditPath := shelfSkill(t, brain, "repo-audit", "Walk a repo for dead code and unused exports.") + testPath := shelfSkill(t, brain, "flaky-test", "Re-run a failing test in isolation to separate flake from breakage.") + + out := useSkill(t, agent, `{"mode":"list"}`) + for _, want := range []string{ + "- repo-audit: Walk a repo for dead code and unused exports.", + "- flaky-test: Re-run a failing test in isolation to separate flake from breakage.", + } { + if !strings.Contains(out, want) { + t.Errorf("the listing does not carry %q:\n%s", want, out) + } + } + for _, leak := range []string{auditPath, testPath, "Path:"} { + if strings.Contains(out, leak) { + t.Errorf("the listing leaks %q, which a discovery row must not carry:\n%s", leak, out) + } + } +} + +// GET RESOLVES ONE NAME to the shelf path a worker will open and the doc that +// says what the skill is for. +func TestUseSkillGet(t *testing.T) { + agent, brain := brainAgent(t, &scriptedCompleter{}, nil) + auditPath := shelfSkill(t, brain, "repo-audit", "Walk a repo for dead code and unused exports.") + + out := useSkill(t, agent, `{"mode":"get","name":"repo-audit"}`) + if !strings.Contains(out, "Walk a repo for dead code and unused exports.") { + t.Errorf("the answer does not carry the skill's doc:\n%s", out) + } + if !strings.Contains(out, "Path: "+auditPath) { + t.Errorf("the answer does not point at the shelf path %q:\n%s", auditPath, out) + } +} + +// THE VERB IS ABSENT, NOT REFUSING, ON A NODE STANDING ON THE FLOOR. It is the +// same gate propose_task reads (mayProposeTask), and a floor node is handed the +// store here specifically to prove the DEPTH is what keeps the verb off: memory +// alone does not put it there. +func TestUseSkillAbsentAtDepthFloor(t *testing.T) { + agent, _ := brainAgent(t, &scriptedCompleter{}, func(config *Config) { + config.InTask = true + config.tasker = graphForShape(t) + config.taskID = 2 + config.taskDepth = taskDepthLimit + }) + if beltHas(agent, useSkillToolName) { + t.Fatal("a node on the floor of its tree was handed a verb over a shelf it should not reach") + } +} + +// AN UNKNOWN NAME IS A NOT-FOUND ANSWER rather than a failure: the shelf simply +// does not have it, and the model can list what is there. +func TestUseSkillNotFound(t *testing.T) { + agent, brain := brainAgent(t, &scriptedCompleter{}, nil) + shelfSkill(t, brain, "repo-audit", "Walk a repo for dead code and unused exports.") + + out := useSkill(t, agent, `{"mode":"get","name":"no-such-skill"}`) + if !strings.Contains(out, "Skill 'no-such-skill' not found.") { + t.Fatalf("an unknown name did not answer with the not-found sentence:\n%s", out) + } +} + +// AN EMPTY SHELF SAYS SO, and it is not the same sentence as a populated one: +// "no active skills" names the state rather than printing an empty list. +func TestUseSkillEmptyShelf(t *testing.T) { + agent, _ := brainAgent(t, &scriptedCompleter{}, nil) + out := useSkill(t, agent, `{"mode":"list"}`) + if out != "No active skills on the shelf." { + t.Fatalf("an empty shelf answered %q, want the empty-shelf sentence", out) + } +} From 21660ea0d82bd18ef23c5c6fdfe738fb495bcc0f Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Sun, 20 Sep 2026 22:16:55 -0400 Subject: [PATCH 06/14] session: fix ActivateSkill test callers after the skills merge Co-Authored-By: codeaf Assisted-by: CodeAF (moonshotai/kimi-k3) --- internal/session/skillcatalog_test.go | 2 +- internal/session/tools_skill_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/session/skillcatalog_test.go b/internal/session/skillcatalog_test.go index fa4071b2e..e4df13cbd 100644 --- a/internal/session/skillcatalog_test.go +++ b/internal/session/skillcatalog_test.go @@ -19,7 +19,7 @@ func activeSkill(t *testing.T, brain *store.Store, scope, body, artifact string) if err != nil { t.Fatalf("record skill candidate %q: %v", body, err) } - if err := brain.ActivateSkill(candidate.Seq, artifact); err != nil { + if err := brain.ActivateSkill(candidate.Seq, artifact, ""); err != nil { t.Fatalf("activate skill %q: %v", body, err) } return candidate diff --git a/internal/session/tools_skill_test.go b/internal/session/tools_skill_test.go index 854ce6aa8..de1e6d7dd 100644 --- a/internal/session/tools_skill_test.go +++ b/internal/session/tools_skill_test.go @@ -30,7 +30,7 @@ func shelfSkill(t *testing.T, brain *store.Store, name, doc string) string { if err != nil { t.Fatalf("record skill %s: %v", name, err) } - if err := brain.ActivateSkill(candidate.Seq, artifact); err != nil { + if err := brain.ActivateSkill(candidate.Seq, artifact, ""); err != nil { t.Fatalf("activate skill %s: %v", name, err) } return artifact From 777bb1250a832946f9f6fad97081fc675008df39 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 22:51:23 -0400 Subject: [PATCH 07/14] skills: compose per-node skill attachments at brief time and render them NodeBrief.Skills was plumbed but nothing in live flow populated it. The brief pass now composes each leaf's attachment from the shelf the caller hands the build frozen: skills the goal names outright first, then deterministic cue/scope retrieval behind them (function words never count as cues, a single shared word is coincidence, three candidates at most). The same order is written onto the node, journaled on the node brief and rendered into the worker's instruction beside the working method; a leaf with nothing attached renders byte for byte what it rendered before. RenderSkillsBlock and SkillEntry move from orchestrate to plan, beside ComposeSkills: the executor cannot import orchestrate (it imports the subharness, which imports the executor), and a render the worker brief cannot call is a render that never runs. Co-Authored-By: codeaf Assisted-by: CodeAF (z-ai/glm-5.3-flash) --- internal/exec/executor.go | 5 + internal/exec/linear.go | 45 +++++++ internal/exec/linear_test.go | 80 +++++++++++++ internal/exec/schedule.go | 1 + internal/orchestrate/prompt.go | 37 ------ internal/orchestrate/prompt_test.go | 43 ------- internal/plan/brief.go | 14 +++ internal/plan/brief_journal_test.go | 149 +++++++++++++++++++++++ internal/plan/contract.go | 180 ++++++++++++++++++++++++++++ internal/plan/contract_test.go | 127 ++++++++++++++++++++ internal/plan/graph.go | 8 ++ internal/plan/plan.go | 10 ++ internal/store/facts.go | 15 +++ 13 files changed, 634 insertions(+), 80 deletions(-) diff --git a/internal/exec/executor.go b/internal/exec/executor.go index a530b0fe9..744f03625 100644 --- a/internal/exec/executor.go +++ b/internal/exec/executor.go @@ -130,6 +130,11 @@ type Task struct { // re-decide it. Empty is the generalist, which is nearly every leaf. Subharness string + // Skills is the ordered list of skill names attached to this leaf's brief: + // the plan composed them from the shelf (pinned first), and the brief + // renders them beside the working method. Empty renders nothing. + Skills []string + // Steer, when set, is polled between turns for mid-flight guidance from // the user. Each returned line lands in the transcript as a user message // before the next model call, so a running worker can be redirected diff --git a/internal/exec/linear.go b/internal/exec/linear.go index 39a1e3584..f97539d78 100644 --- a/internal/exec/linear.go +++ b/internal/exec/linear.go @@ -10,6 +10,7 @@ import ( "github.com/Agent-Field/agentfield/sdk/go/ai" "github.com/Agent-Field/codeaf/internal/guard" "github.com/Agent-Field/codeaf/internal/orientation" + "github.com/Agent-Field/codeaf/internal/plan" "github.com/Agent-Field/codeaf/internal/provider" "github.com/Agent-Field/codeaf/internal/store" ) @@ -1714,6 +1715,13 @@ func (l *Linear) brief(task Task) string { if contract := strings.TrimSpace(task.Contract); contract != "" { fmt.Fprintf(&block, "How this particular kind of job is done well:\n%s\n\n", contract) } + // The shelf's own recipes for this leaf, attached by the plan, rendered + // beside the method they refine. Zero attached skills renders zero bytes — + // no header, no placeholder — so a leaf with nothing attached reads byte + // for byte what it read before attachment existed. + if entries := l.skillEntries(task.Skills); len(entries) > 0 { + fmt.Fprintf(&block, "Skills attached to this work:\n%s\n\n", plan.RenderSkillsBlock(entries)) + } if task.Goal != "" { fmt.Fprintf(&block, "This work is part of a larger goal:\n%s\n\n", task.Goal) } @@ -1790,6 +1798,43 @@ func (l *Linear) brief(task Task) string { return block.String() } +// skillResolveLimit bounds the shelf read one brief's resolution makes. It +// mirrors the shelf tool's own bound (internal/session's skillShelfLimit): the +// shelf is a curated few, and reading further would only slow dispatch. +const skillResolveLimit = 100 + +// skillEntries resolves the leaf's attached skill names against the active +// shelf, keeping the order the plan composed — that order is the precedence +// the rendered block states. A name the shelf does not hold is dropped rather +// than rendered as an empty bullet, and a loop with no store has no shelf to +// resolve against, so it renders nothing and changes no prompt byte. +func (l *Linear) skillEntries(names []string) []plan.SkillEntry { + if len(names) == 0 || l.history == nil { + return nil + } + facts, err := l.history.SkillFacts(store.FactActive, skillResolveLimit) + if err != nil { + return nil + } + byName := make(map[string]store.Fact, len(facts)) + for _, fact := range facts { + if name := fact.SkillName(); name != "" { + if _, held := byName[name]; !held { + // SkillFacts returns newest first; the first fact under a name + // is the one every other reader of that name serves. + byName[name] = fact + } + } + } + entries := make([]plan.SkillEntry, 0, len(names)) + for _, name := range names { + if fact, held := byName[name]; held { + entries = append(entries, plan.SkillEntry{Name: name, Doc: fact.Body, ShelfPath: fact.Artifact}) + } + } + return entries +} + // briefIsWhole reports that the brief this leaf is about to read is the whole of // what exists for its job. // diff --git a/internal/exec/linear_test.go b/internal/exec/linear_test.go index 915e45278..5a056ffd6 100644 --- a/internal/exec/linear_test.go +++ b/internal/exec/linear_test.go @@ -12,6 +12,7 @@ import ( "github.com/Agent-Field/agentfield/sdk/go/ai" "github.com/Agent-Field/codeaf/internal/plan" "github.com/Agent-Field/codeaf/internal/provider" + "github.com/Agent-Field/codeaf/internal/store" ) // scriptedCompleter plays back a fixed sequence of model turns and records @@ -717,3 +718,82 @@ func TestARefusalOfOurOwnRequestIsNotRetried(t *testing.T) { t.Fatalf("calls = %d, want exactly one: the refusal is the answer", len(client.seen)) } } + +// shelfOnDisk puts one active skill on a real store's shelf — the candidate +// plus the activation, the only transition [Store.SkillFacts] surfaces — and +// returns the store, as every other reader of the shelf opens it. +func shelfOnDisk(t *testing.T, name, body string) *store.Store { + t.Helper() + db, err := store.Open(filepath.Join(t.TempDir(), "graph.db")) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + candidate, err := db.RecordSkillCandidate(store.RootID, "repo:/test", body, "/shelf/"+name) + if err != nil { + t.Fatalf("record skill candidate %q: %v", body, err) + } + if err := db.ActivateSkill(candidate.Seq, "/shelf/"+name); err != nil { + t.Fatalf("activate skill %q: %v", name, err) + } + return db +} + +// TestABriefRendersAttachedSkillsInOrder is the render half of the +// attachment: a leaf whose plan attached skills reads them beside the working +// method, in the order the plan composed, each with its doc and shelf path — +// and a name the shelf does not hold renders as nothing rather than as an +// empty bullet. +func TestABriefRendersAttachedSkillsInOrder(t *testing.T) { + db := shelfOnDisk(t, "imgshrink", "optimize images without losing quality") + candidate, err := db.RecordSkillCandidate(store.RootID, "repo:/test", "gofmt vet and lint the tree", "/shelf/lint") + if err != nil { + t.Fatal(err) + } + if err := db.ActivateSkill(candidate.Seq, "/shelf/lint"); err != nil { + t.Fatal(err) + } + linear := NewLinear(&scriptedCompleter{}, workspace(t), nil, 10, 1_000_000, time.Minute).WithStore(db) + got := linear.brief(Task{Brief: "Do the thing.", Skills: []string{"lint", "imgshrink", "nothere"}}) + + if !strings.Contains(got, "Skills attached to this work:\n") { + t.Fatalf("the brief never rendered the attached skills:\n%s", got) + } + if !strings.Contains(got, "- gofmt vet and lint the tree [/shelf/lint]\n") { + t.Errorf("the lint skill's doc line is missing:\n%s", got) + } + if !strings.Contains(got, "- optimize images without losing quality [/shelf/imgshrink]\n") { + t.Errorf("the imgshrink skill's doc line is missing:\n%s", got) + } + if !strings.Contains(got, "Earlier-listed skills win when two skills conflict.") { + t.Errorf("the precedence line is missing:\n%s", got) + } + // Order is precedence: the plan pinned lint first, so its line leads. + if lint, shrink := strings.Index(got, "- gofmt vet and lint"), strings.Index(got, "- optimize images"); lint < 0 || shrink < 0 || lint > shrink { + t.Errorf("skill lines are not in the composed order (lint at %d, imgshrink at %d):\n%s", lint, shrink, got) + } + if strings.Contains(got, "nothere") { + t.Errorf("a name the shelf does not hold rendered anyway:\n%s", got) + } +} + +// TestABriefWithoutSkillsRendersExactlyAsBefore is the zero render: a leaf +// with nothing attached — and a leaf whose names resolve to nothing, and a +// loop with no store at all — reads byte for byte what it read before +// attachment existed. +func TestABriefWithoutSkillsRendersExactlyAsBefore(t *testing.T) { + db := shelfOnDisk(t, "imgshrink", "optimize images without losing quality") + withShelf := NewLinear(&scriptedCompleter{}, workspace(t), nil, 10, 1_000_000, time.Minute).WithStore(db) + withoutShelf := NewLinear(&scriptedCompleter{}, workspace(t), nil, 10, 1_000_000, time.Minute) + + base := withShelf.brief(Task{Brief: "Do the thing."}) + if got := withShelf.brief(Task{Brief: "Do the thing.", Skills: nil}); got != base { + t.Fatalf("nil skills changed the brief:\ngot: %q\nwant: %q", got, base) + } + if got := withShelf.brief(Task{Brief: "Do the thing.", Skills: []string{"nothere"}}); got != base { + t.Fatalf("unresolvable names changed the brief:\ngot: %q\nwant: %q", got, base) + } + if got := withoutShelf.brief(Task{Brief: "Do the thing.", Skills: []string{"imgshrink"}}); got != base { + t.Fatalf("attached names with no shelf behind them changed the brief:\ngot: %q\nwant: %q", got, base) + } +} diff --git a/internal/exec/schedule.go b/internal/exec/schedule.go index 1131f84f4..79fdee660 100644 --- a/internal/exec/schedule.go +++ b/internal/exec/schedule.go @@ -499,6 +499,7 @@ func (s *Scheduler) taskFor(graph *plan.Graph, node *plan.Node) Task { Brief: node.Brief, Contract: node.Contract, Subharness: node.Subharness, + Skills: node.Skills, OutputHint: SuggestPath(node.ID, node.Title), } if strings.TrimSpace(task.Brief) == "" { diff --git a/internal/orchestrate/prompt.go b/internal/orchestrate/prompt.go index e7751a220..0f36151d5 100644 --- a/internal/orchestrate/prompt.go +++ b/internal/orchestrate/prompt.go @@ -37,40 +37,3 @@ var plannerLaw string // called once at the start and once on EVERY node completion, so every paragraph // here is billed against the run's one tank as many times as the run is wide. var PlannerPrompt = strings.ReplaceAll(plannerLaw, "{{NAME_WORDS}}", strconv.Itoa(NameWords)) - -// SkillEntry is one attached skill rendered in a worker's instruction block. -// Name is the skill's shelf name; Doc is the one-line description from its fact; -// ShelfPath is the absolute path to the skill's directory on disk. -type SkillEntry struct { - Name string - Doc string - ShelfPath string -} - -// RenderSkillsBlock renders attached skills as doc lines and shelf paths. -// Each skill produces one line: "- []" when both exist, or a -// shorter form when only one is available. Zero entries returns zero bytes — -// no header, no placeholder, no blank line. The final line states that -// earlier-listed skills take precedence in case of conflict. -func RenderSkillsBlock(skills []SkillEntry) string { - if len(skills) == 0 { - return "" - } - var buf strings.Builder - for _, s := range skills { - buf.WriteString("- ") - if s.Doc != "" { - buf.WriteString(s.Doc) - if s.ShelfPath != "" { - buf.WriteString(" [") - buf.WriteString(s.ShelfPath) - buf.WriteString("]") - } - } else if s.ShelfPath != "" { - buf.WriteString(s.ShelfPath) - } - buf.WriteString("\n") - } - buf.WriteString("Earlier-listed skills win when two skills conflict.") - return buf.String() -} diff --git a/internal/orchestrate/prompt_test.go b/internal/orchestrate/prompt_test.go index 86a548e07..940231e31 100644 --- a/internal/orchestrate/prompt_test.go +++ b/internal/orchestrate/prompt_test.go @@ -11,49 +11,6 @@ import ( // planner shown the field would pick one. The run decides that, not the model. var untaught = map[string]bool{"kind": true} -func TestRenderSkillsBlockRendersDocAndPath(t *testing.T) { - skills := []SkillEntry{ - {Name: "imgshrink", Doc: "optimize images without losing quality", ShelfPath: "~/.codeaf/skills/imgshrink"}, - {Name: "parser", Doc: "validate and format parser fixtures", ShelfPath: "~/.codeaf/skills/parser"}, - } - got := RenderSkillsBlock(skills) - want := "- optimize images without losing quality [~/.codeaf/skills/imgshrink]\n- validate and format parser fixtures [~/.codeaf/skills/parser]\nEarlier-listed skills win when two skills conflict." - if got != want { - t.Fatalf("RenderSkillsBlock:\ngot: %q\nwant: %q", got, want) - } -} - -func TestRenderSkillsBlockEmpty(t *testing.T) { - if got := RenderSkillsBlock(nil); got != "" { - t.Fatalf("RenderSkillsBlock(nil) = %q, want \"\"", got) - } - if got := RenderSkillsBlock([]SkillEntry{}); got != "" { - t.Fatalf("RenderSkillsBlock([]) = %q, want \"\"", got) - } -} - -func TestRenderSkillsBlockPreservesPrecedenceOrder(t *testing.T) { - skills := []SkillEntry{ - {Name: "lint", Doc: "run linters", ShelfPath: "~/.codeaf/skills/lint"}, - {Name: "test", Doc: "run tests", ShelfPath: "~/.codeaf/skills/test"}, - {Name: "build", Doc: "build the project", ShelfPath: "~/.codeaf/skills/build"}, - } - got := RenderSkillsBlock(skills) - lines := strings.Split(got, "\n") - if len(lines) != 4 { - t.Fatalf("expected 4 lines (3 skills + 1 precedence), got %d", len(lines)) - } - if !strings.HasPrefix(lines[0], "- run linters") { - t.Errorf("first skill should be 'lint', got: %s", lines[0]) - } - if !strings.HasPrefix(lines[1], "- run tests") { - t.Errorf("second skill should be 'test', got: %s", lines[1]) - } - if !strings.HasPrefix(lines[2], "- build the project") { - t.Errorf("third skill should be 'build', got: %s", lines[2]) - } -} - // The law quotes the Amendment schema verbatim, which means the schema is in two // places: here as struct tags, there as a JSON block a model is held to. Drift // either way is a run that refuses a well-formed amendment or a planner taught a diff --git a/internal/plan/brief.go b/internal/plan/brief.go index 3885ef19d..cc7a738dc 100644 --- a/internal/plan/brief.go +++ b/internal/plan/brief.go @@ -283,6 +283,11 @@ type briefWriter struct { // leaves the brief exactly as durable as it was before this hook existed. journal BriefJournal + // skills is the active shelf the caller read before the build started + // (Options.Skills), handed in frozen. The brief pass composes each leaf's + // attachment from it at apply time; nil attaches nothing. + skills []store.Fact + // sink is the deliverable owner, which is written for even though it is not // KindWork. It is a single id rather than a predicate because every other // non-work node in a graph is an expanded container — structure nobody runs — @@ -408,6 +413,14 @@ func (w *briefWriter) apply(graph *Graph) (Usage, error) { node.Spec.Instruction = written.Instruction node.Spec.Done = written.Done node.Spec.Sources = node.Sources + // The skills this leaf is served from the shelf, composed here where + // the brief is final: skills the goal names outright first, retrieval + // candidates behind them. The same order is written onto the node and + // journaled on the brief, so precedence is one fact everywhere it is + // read. An empty shelf composes nothing, and everything below reads + // exactly as it did before attachment existed. + node.Skills = ComposeSkills(PinnedSkills(graph.Goal, w.skills), + RetrieveSkills(node.Brief, graph.Workspace, w.skills)) // Journal the rendered brief as a first-class event per node, so a // run's sufficiency sentence is queryable from its own artifacts // rather than only as a field inside the plan blob. The caller forms @@ -422,6 +435,7 @@ func (w *briefWriter) apply(graph *Graph) (Usage, error) { // from a written one after the fact. Fault: written.fault, Subharness: node.Subharness, + Skills: node.Skills, }) } } diff --git a/internal/plan/brief_journal_test.go b/internal/plan/brief_journal_test.go index f4b73aae3..352a623d5 100644 --- a/internal/plan/brief_journal_test.go +++ b/internal/plan/brief_journal_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "path/filepath" + "reflect" "testing" "github.com/Agent-Field/agentfield/sdk/go/ai" @@ -122,3 +123,151 @@ func TestBriefsAreJournaledPerNode(t *testing.T) { } } } + +// skillsBriefClient scripts a build whose brief instructions carry fixed cue +// words, so retrieval has something deterministic to match against. Every pass +// but the brief call goes to passClient, exactly as briefJournalClient does. +type skillsBriefClient struct{ instruction string } + +func (c *skillsBriefClient) CompleteWithMessages(ctx context.Context, messages []ai.Message, options ...ai.Option) (*ai.Response, error) { + var system, target string + for _, m := range messages { + text := textOf(m) + if m.Role == "system" { + system = text + continue + } + target = text + } + if system == briefPrompt || system == briefWithCriterion { + var id int + fmt.Sscanf(target, "Write the instruction for node %d,", &id) + return response(fmt.Sprintf(`{"instruction":"`+fmt.Sprintf(c.instruction, id)+`",`+ + `"done":{"produces":["the sorted list of node %d"],`+ + `"conditions":[{"kind":"run","check":"the sort for node %d runs","expect":"it reports success"}]}}`, + id, id)), nil + } + return (&passClient{}).CompleteWithMessages(ctx, messages, options...) +} + +// shelfFixture is a two-skill shelf: one whose doc line shares two words with +// every brief this client writes, and one that shares only a stop word. +func shelfFixture() []store.Fact { + return []store.Fact{ + {Artifact: "/skills/imgshrink", Body: "optimize images without losing quality"}, + {Artifact: "/skills/lint", Body: "gofmt vet and lint the tree"}, + } +} + +// journalledBriefs runs a build with the shelf wired in and journals every +// node_briefed event into a real store, returning what the store holds by +// store id — the same id law cmd's briefJournal uses. +func journalledBriefs(t *testing.T, goal, instruction string, facts []store.Fact) (map[string]store.NodeBrief, *Graph, *store.Store) { + t.Helper() + db, err := store.Open(filepath.Join(t.TempDir(), "graph.db")) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + const prefix = "job" + storeID := func(graph *Graph, nodeID int) string { + if nodeID == graph.deliverableSink() { + return prefix + } + return fmt.Sprintf("%s-n%d", prefix, nodeID) + } + journal := func(graph *Graph, nodeID int, brief store.NodeBrief) { + if err := db.RecordNodeBrief(storeID(graph, nodeID), brief); err != nil { + t.Errorf("journal brief for %s: %v", storeID(graph, nodeID), err) + } + } + + graph, err := Build(context.Background(), &skillsBriefClient{instruction: instruction}, goal, Options{ + Ensemble: EnsembleNever, + SpineSamples: 1, + NodeBudget: 20, + Briefs: true, + Journal: journal, + Skills: facts, + }) + if err != nil { + t.Fatalf("Build: %v", err) + } + + briefs := map[string]store.NodeBrief{} + for _, id := range graph.writtenLeaves() { + sid := storeID(graph, id) + got, ok, err := db.BriefFor(sid) + if err != nil { + t.Fatalf("BriefFor %s: %v", sid, err) + } + if !ok { + t.Fatalf("no node_briefed event for %s", sid) + } + briefs[sid] = got + } + if len(briefs) == 0 { + t.Fatal("the build produced no briefed leaves") + } + return briefs, graph, db +} + +// TestAProposalNamingASkillAttachesItFirst is the pinned half of the +// attachment: a goal that names a shelf skill attaches it, first in the order, +// on every briefed leaf — and the same order is what the journal holds. +func TestAProposalNamingASkillAttachesItFirst(t *testing.T) { + briefs, graph, _ := journalledBriefs(t, "shrink the report images with imgshrink", + "Sort the report images by hue and optimize the order for node %d.", shelfFixture()) + for sid, brief := range briefs { + node := graph.Node(brief.Node) + if node == nil { + t.Fatalf("%s: node %d vanished from the built graph", sid, brief.Node) + } + if want := []string{"imgshrink"}; !reflect.DeepEqual(brief.Skills, want) { + t.Errorf("%s: journalled skills = %q, want %q", sid, brief.Skills, want) + } + if !reflect.DeepEqual(node.Skills, brief.Skills) { + t.Errorf("%s: node skills %q and journalled skills %q disagree", sid, node.Skills, brief.Skills) + } + } +} + +// TestAProposalNamingNothingAttachesNothing is the zero half: a goal that +// names no skill, over a shelf, attaches nothing anywhere — the node brief +// carries no skills field and the worker prompt would render zero bytes. +func TestAProposalNamingNothingAttachesNothing(t *testing.T) { + briefs, graph, _ := journalledBriefs(t, "review the pull request and deliver REVIEW.md", + "Write the summary for node %d, list what was checked, and hand back the result.", shelfFixture()) + for sid, brief := range briefs { + node := graph.Node(brief.Node) + if node == nil { + t.Fatalf("%s: node %d vanished from the built graph", sid, brief.Node) + } + if len(brief.Skills) != 0 || len(node.Skills) != 0 { + t.Errorf("%s: journalled skills %q, node skills %q, want nothing attached", + sid, brief.Skills, node.Skills) + } + } +} + +// TestRetrievedCandidatesFollowPinned is the order half: a goal that names one +// skill and a brief whose territory cues another compose pinned first and the +// retrieved candidate behind it, in that order, in the journal. +func TestRetrievedCandidatesFollowPinned(t *testing.T) { + briefs, graph, _ := journalledBriefs(t, "lint the tree with lint", + "Sort the report images by hue and optimize the order for node %d.", shelfFixture()) + want := []string{"lint", "imgshrink"} + for sid, brief := range briefs { + node := graph.Node(brief.Node) + if node == nil { + t.Fatalf("%s: node %d vanished from the built graph", sid, brief.Node) + } + if !reflect.DeepEqual(brief.Skills, want) { + t.Errorf("%s: journalled skills = %q, want %q", sid, brief.Skills, want) + } + if !reflect.DeepEqual(node.Skills, want) { + t.Errorf("%s: node skills = %q, want %q", sid, node.Skills, want) + } + } +} diff --git a/internal/plan/contract.go b/internal/plan/contract.go index 7f367ced4..97e3bfda7 100644 --- a/internal/plan/contract.go +++ b/internal/plan/contract.go @@ -4,12 +4,14 @@ import ( "context" "encoding/json" "fmt" + "sort" "strings" "sync" "github.com/Agent-Field/agentfield/sdk/go/ai" "github.com/Agent-Field/codeaf/internal/guard" "github.com/Agent-Field/codeaf/internal/provider" + "github.com/Agent-Field/codeaf/internal/store" ) // contractPrompt writes the working method one agent will follow. @@ -404,3 +406,181 @@ func ComposeSkills(pinned, candidates []string) []string { } return result } + +// SkillEntry is one attached skill rendered in a worker's instruction block. +// Name is the skill's shelf name; Doc is the one-line description from its +// fact; ShelfPath is the path to the skill's directory on disk. +type SkillEntry struct { + Name string + Doc string + ShelfPath string +} + +// RenderSkillsBlock renders attached skills as doc lines and shelf paths. +// Each skill produces one line: "- []" when both exist, or a +// shorter form when only one is available. Zero entries returns zero bytes — +// no header, no placeholder, no blank line. The final line states that +// earlier-listed skills take precedence in case of conflict. +// +// This renders beside the composition above because the two are one path: the +// brief pass composes the attachment and the executor renders it into the +// instruction, and the executor cannot reach a package that itself imports the +// subharness. A render the worker prompt cannot call is a render that never +// runs. +func RenderSkillsBlock(skills []SkillEntry) string { + if len(skills) == 0 { + return "" + } + var buf strings.Builder + for _, s := range skills { + buf.WriteString("- ") + if s.Doc != "" { + buf.WriteString(s.Doc) + if s.ShelfPath != "" { + buf.WriteString(" [") + buf.WriteString(s.ShelfPath) + buf.WriteString("]") + } + } else if s.ShelfPath != "" { + buf.WriteString(s.ShelfPath) + } + buf.WriteString("\n") + } + buf.WriteString("Earlier-listed skills win when two skills conflict.") + return buf.String() +} + +// retrieveSkillCap bounds how many retrieved candidates one leaf may carry. +// Pinned skills are the person's own naming and are never capped; the fuzzy +// half is, so a runaway shelf cannot bury a leaf's instruction in recipes. +const retrieveSkillCap = 3 + +// attachmentStopwords are the function words that share with every instruction +// there is — "the" with all of them, "and" with almost as many. They are +// dropped from the cue side so a shelf doc saying "the" once cannot claim +// relevance to every leaf; a body word can only score against a cue the +// territory actually names. +var attachmentStopwords = map[string]bool{ + "the": true, "and": true, "for": true, "with": true, "that": true, + "this": true, "from": true, "into": true, "your": true, "are": true, + "was": true, "were": true, "has": true, "have": true, "will": true, + "them": true, "they": true, "their": true, "there": true, "then": true, + "than": true, "when": true, "what": true, "where": true, "which": true, + "out": true, "off": true, "too": true, "also": true, "both": true, + "about": true, "after": true, "before": true, "while": true, + "through": true, "without": true, "within": true, "each": true, +} + +// PinnedSkills returns the skills the person's own words name outright: every +// shelf skill whose name appears in the text. It is simple name-in-text +// matching — deterministic, no model call — because a person naming a skill is +// the strongest relevance signal there is, and it is read off the goal, which +// is the person's proposal in whatever words they used. +func PinnedSkills(text string, skills []store.Fact) []string { + if strings.TrimSpace(text) == "" { + return nil + } + pinned := make([]string, 0, len(skills)) + for _, fact := range skills { + if name := fact.SkillName(); name != "" && strings.Contains(text, name) { + pinned = append(pinned, name) + } + } + return pinned +} + +// RetrieveSkills returns the skills retrieval would attach to one leaf: those +// whose scope or doc line cues against the leaf's own territory — its rendered +// instruction, and the workspace it runs in. The shape is the chat catalog's +// window scorer (skillcatalog.go): a scope naming something in front of the +// leaf outweighs anything, a shared doc word is the weaker cue. One shared +// word is coincidence — "the" shares with every instruction there is — so only +// scores a real cue produces come back, most relevant first, capped. +func RetrieveSkills(text, workspace string, skills []store.Fact) []string { + if strings.TrimSpace(text) == "" { + return nil + } + cues := cueWords(text) + for word := range cueWords(workspace) { + cues[word] = true + } + type scored struct { + name string + score int + } + found := make([]scored, 0, len(skills)) + for _, fact := range skills { + name := fact.SkillName() + if name == "" { + continue + } + score := 0 + if scopeWords(fact.Scope, cues) { + score += 100 + } + for _, word := range docWords(fact.Body) { + if cues[word] { + score += 5 + } + } + if score >= 10 { + found = append(found, scored{name: name, score: score}) + } + } + sort.SliceStable(found, func(first, second int) bool { return found[first].score > found[second].score }) + if len(found) > retrieveSkillCap { + found = found[:retrieveSkillCap] + } + names := make([]string, 0, len(found)) + for _, hit := range found { + names = append(names, hit.name) + } + return names +} + +// docWords is the comparable words of one text: lowercase, split on everything +// that is not a letter or a digit, and dropping the short words that match +// everything. It is the chat catalog's own tokenizer (skillcatalog.go), held +// at this spelling because the composition here has to agree with what the +// shelf's one other reader scores with. +func docWords(text string) []string { + fields := strings.FieldsFunc(strings.ToLower(text), func(r rune) bool { + return !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') + }) + words := fields[:0] + for _, field := range fields { + if len(field) >= 3 { + words = append(words, field) + } + } + return words +} + +// cueWords is the cue side of the match: the comparable words of a territory +// after the function words are dropped. A body word can only score against a +// cue the territory actually names, so "the" and friends never make one. +func cueWords(text string) map[string]bool { + cues := make(map[string]bool) + for _, word := range docWords(text) { + if !attachmentStopwords[word] { + cues[word] = true + } + } + return cues +} + +// scopeWords asks whether a skill's scope names something the cue words hold. +// A scope is `kind:value` ("repo:/path", "tool:git", "domain:x"), so its parts +// are compared against the words the way the catalog's scope match does it: +// a scope of `repo:/…/codeaf` matches a workspace that ends in `codeaf`, and +// `tool:git` matches nothing unless the territory happens to say `git`. +func scopeWords(scope string, cues map[string]bool) bool { + for _, part := range strings.FieldsFunc(strings.ToLower(scope), func(r rune) bool { + return r == ':' || r == '/' || r == '\\' || r == '.' || r == '-' || r == '_' || r == ' ' + }) { + if part != "" && cues[part] { + return true + } + } + return false +} diff --git a/internal/plan/contract_test.go b/internal/plan/contract_test.go index 8019a77b2..fb3dbe180 100644 --- a/internal/plan/contract_test.go +++ b/internal/plan/contract_test.go @@ -2,12 +2,14 @@ package plan import ( "context" + "fmt" "reflect" "strings" "sync" "testing" "github.com/Agent-Field/agentfield/sdk/go/ai" + "github.com/Agent-Field/codeaf/internal/store" ) type contractCaptureClient struct { @@ -447,3 +449,128 @@ func TestComposeSkillsSkipsEmptyNames(t *testing.T) { t.Fatalf("ComposeSkills(%q, %q) = %q, want %q", pinned, candidates, got, want) } } + +// skillFact is a shelf fact as the store holds it: named by the directory on +// the shelf, described by the one line the notebook recorded. +func skillFact(artifact, scope, body string) store.Fact { + return store.Fact{Artifact: artifact, Scope: scope, Body: body} +} + +func TestPinnedSkillsMatchesShelfNamesInTheText(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/home/.codeaf/skills/imgshrink", Body: "optimize images without losing quality"}, + {Artifact: "/home/.codeaf/skills/lint", Body: "run linters"}, + } + got := PinnedSkills("shrink the report images with imgshrink", skills) + if want := []string{"imgshrink"}; !reflect.DeepEqual(got, want) { + t.Fatalf("PinnedSkills = %q, want %q", got, want) + } +} + +func TestPinnedSkillsKeepsShelfOrder(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/skills/lint", Body: "run linters"}, + {Artifact: "/skills/build", Body: "build the project"}, + } + got := PinnedSkills("lint first, then build", skills) + if want := []string{"lint", "build"}; !reflect.DeepEqual(got, want) { + t.Fatalf("PinnedSkills = %q, want shelf order %q", got, want) + } +} + +func TestPinnedSkillsMatchesNothingWhenNothingIsNamed(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/skills/imgshrink", Body: "optimize images without losing quality"}, + } + if got := PinnedSkills("review the pull request and deliver REVIEW.md", skills); len(got) != 0 { + t.Fatalf("PinnedSkills = %q, want nothing attached", got) + } +} + +func TestRetrieveSkillsScoresScopeAndSharedDocWords(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/skills/parser", Scope: "repo:/work/parser", Body: "validate and format parser fixtures"}, + {Artifact: "/skills/lint", Scope: "tool:lint", Body: "gofmt vet and lint the tree"}, + } + // The workspace names the parser repo, so the scoped skill scores high + // even where the instruction shares none of its doc words. + got := RetrieveSkills("tidy the fixtures in the parser repository", "/tmp/work/parser", skills) + if want := []string{"parser"}; !reflect.DeepEqual(got, want) { + t.Fatalf("RetrieveSkills = %q, want %q", got, want) + } +} + +func TestRetrieveSkillsCuesOnTwoSharedDocWordsNotOne(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/skills/imgshrink", Body: "optimize images without losing quality"}, + {Artifact: "/skills/lint", Body: "gofmt vet and lint the tree"}, + } + // One shared word is coincidence — "and" and "the" share with every + // instruction there is — so a single-word overlap attaches nothing. + got := RetrieveSkills("review the change and deliver REVIEW.md", "", skills) + if len(got) != 0 { + t.Fatalf("RetrieveSkills = %q, want nothing from one shared word", got) + } + // Two shared doc words are a cue. + got = RetrieveSkills("optimize the images the report embeds", "", skills) + if want := []string{"imgshrink"}; !reflect.DeepEqual(got, want) { + t.Fatalf("RetrieveSkills = %q, want %q", got, want) + } +} + +func TestRetrieveSkillsCapsCandidates(t *testing.T) { + skills := make([]store.Fact, 0, retrieveSkillCap+1) + for index := 0; index < retrieveSkillCap+1; index++ { + skills = append(skills, store.Fact{ + Artifact: fmt.Sprintf("/skills/worker-%02d", index), + Body: "polish the README prose and cover", + }) + } + got := RetrieveSkills("rewrite the README prose and cover page", "", skills) + if len(got) != retrieveSkillCap { + t.Fatalf("RetrieveSkills = %d candidates, want the cap %d", len(got), retrieveSkillCap) + } +} + +func TestRenderSkillsBlockRendersDocAndPath(t *testing.T) { + skills := []SkillEntry{ + {Name: "imgshrink", Doc: "optimize images without losing quality", ShelfPath: "~/.codeaf/skills/imgshrink"}, + {Name: "parser", Doc: "validate and format parser fixtures", ShelfPath: "~/.codeaf/skills/parser"}, + } + got := RenderSkillsBlock(skills) + want := "- optimize images without losing quality [~/.codeaf/skills/imgshrink]\n- validate and format parser fixtures [~/.codeaf/skills/parser]\nEarlier-listed skills win when two skills conflict." + if got != want { + t.Fatalf("RenderSkillsBlock:\ngot: %q\nwant: %q", got, want) + } +} + +func TestRenderSkillsBlockEmpty(t *testing.T) { + if got := RenderSkillsBlock(nil); got != "" { + t.Fatalf("RenderSkillsBlock(nil) = %q, want \"\"", got) + } + if got := RenderSkillsBlock([]SkillEntry{}); got != "" { + t.Fatalf("RenderSkillsBlock([]) = %q, want \"\"", got) + } +} + +func TestRenderSkillsBlockPreservesPrecedenceOrder(t *testing.T) { + skills := []SkillEntry{ + {Name: "lint", Doc: "run linters", ShelfPath: "~/.codeaf/skills/lint"}, + {Name: "test", Doc: "run tests", ShelfPath: "~/.codeaf/skills/test"}, + {Name: "build", Doc: "build the project", ShelfPath: "~/.codeaf/skills/build"}, + } + got := RenderSkillsBlock(skills) + lines := strings.Split(got, "\n") + if len(lines) != 4 { + t.Fatalf("expected 4 lines (3 skills + 1 precedence), got %d", len(lines)) + } + if !strings.HasPrefix(lines[0], "- run linters") { + t.Errorf("first skill should be 'lint', got: %s", lines[0]) + } + if !strings.HasPrefix(lines[1], "- run tests") { + t.Errorf("second skill should be 'test', got: %s", lines[1]) + } + if !strings.HasPrefix(lines[2], "- build the project") { + t.Errorf("third skill should be 'build', got: %s", lines[2]) + } +} diff --git a/internal/plan/graph.go b/internal/plan/graph.go index 7385e295e..390212984 100644 --- a/internal/plan/graph.go +++ b/internal/plan/graph.go @@ -155,6 +155,14 @@ type Node struct { // harness itself changing. Contract string `json:"contract,omitempty"` + // Skills is the ordered list of skill names the brief pass attached to this + // leaf from the shelf the caller handed the build: skills the goal names + // outright first, retrieval candidates behind them. Order is precedence — + // earlier-listed skills win conflicts — and the same order is journaled on + // the node brief and rendered into the worker's instruction. Empty attaches + // nothing and renders nothing. + Skills []string `json:"skills,omitempty"` + // Spec is the same two facts as an object, plus the one nothing carried // before: the criterion this node's work is judged finished against. // diff --git a/internal/plan/plan.go b/internal/plan/plan.go index c474fdfb8..be07da893 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -339,6 +339,15 @@ type Options struct { // execute them. Briefs bool + // Skills is the active shelf, read once by the caller from the store it + // already holds ([store.SkillFacts] with the active status) and handed in + // frozen, the way the terrain and the invoice are: the brief pass composes + // each leaf's attachment from it — skills the goal names outright first, + // retrieval candidates behind them — and journals the same order on the + // node brief. Nil is the whole of the compatibility story: a caller with + // no shelf attaches nothing and changes no prompt byte anywhere. + Skills []store.Fact + // FileShaped carries the delivery-law bit onto the graph, for the case where // briefs are written inside the build and the caller never sees the graph // before they are. See Graph.FileShaped and DeliveryLaw. @@ -731,6 +740,7 @@ func Build(ctx context.Context, client Completer, goal string, options Options) // they are also the nodes most likely to have no dependencies, which makes // them exactly the ones something could start on immediately. briefs := newBriefWriter(ctx, client, options.Briefs, progress, options.Journal) + briefs.skills = options.Skills settled := map[int]bool{} pending := map[int]bool{} for _, id := range selectForExpansion(graph, options) { diff --git a/internal/store/facts.go b/internal/store/facts.go index 0d383d433..9c5f13169 100644 --- a/internal/store/facts.go +++ b/internal/store/facts.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "fmt" + "path/filepath" "sort" "strings" "time" @@ -725,6 +726,20 @@ func (s *Store) SupersedeFactWithReason(factSeq, bySeq int64, reason string) err return tx.Commit() } +// SkillName is the shelf name of a skill fact — the one spelling every reader +// of the shelf matches on. It is the directory the artifact points at, or the +// scope when the fact predates installation. use_skill answers names spelled +// this way (tools_skill.go's get), so a reader matching anything else answers +// a name the worker was never shown. +func (f Fact) SkillName() string { + if artifact := strings.TrimSpace(f.Artifact); artifact != "" { + if base := filepath.Base(artifact); base != "." && base != "/" { + return base + } + } + return strings.TrimSpace(f.Scope) +} + // SkillFacts lists skills in one status, newest first. Empty status includes // candidates, active skills, and retired entries for reconciliation. func (s *Store) SkillFacts(status string, limit int) ([]Fact, error) { From b3f2dda55e58e00805b5baeface63e390ab2c444 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 22:51:30 -0400 Subject: [PATCH 08/14] cmd: wire the active shelf into the chat engine's plan builds Both brief-journaling builds (the main plan and the remainder replan) now read the active shelf once and hand it to plan.Build frozen, and each dispatched leaf carries its plan node's attachment onto the task that runs it. A store with no shelf composes nothing and changes no prompt. Co-Authored-By: codeaf Assisted-by: CodeAF (z-ai/glm-5.3-flash) --- cmd/codeaf/chat.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/cmd/codeaf/chat.go b/cmd/codeaf/chat.go index ec201ac6e..0e4ce7865 100644 --- a/cmd/codeaf/chat.go +++ b/cmd/codeaf/chat.go @@ -880,6 +880,7 @@ func buildBrain(w *chatWindow, session string, opts brainOptions) (*chatBrain, e // is here so a worker that speaks a spec is handed one rather than // having it reassembled from prose at the boundary (W3). Spec: leafSpec(plans, planNode, node), + Skills: leafSkills(planNode), OutputHint: outputHint, Intermediate: intermediate, Inputs: inputs, @@ -5354,6 +5355,7 @@ func planSubtree(settings config.Config, planClient, workClient *liveClient, pla NodeBudget: settings.NodeBudget, Briefs: true, Journal: briefJournal(history, prefix), + Skills: shelfSkills(history), Progress: progress, }) // THE LAW: STRUCTURE THE PLANNER HAS ALREADY FOUND IS NEVER DISCARDED @@ -5494,6 +5496,36 @@ func briefJournal(history *store.Store, prefix string) plan.BriefJournal { } } +// shelfSkills reads the active shelf once per build, frozen like the terrain +// and the invoice, for the brief pass to compose per-node skill attachments +// from. A nil store or a read fault composes nothing: a surface with no shelf +// attaches no skills, and the prompts it sends are byte for byte what they +// were before attachment existed. +func shelfSkills(history *store.Store) []store.Fact { + if history == nil { + return nil + } + facts, err := history.SkillFacts(store.FactActive, shelfScanLimit) + if err != nil { + return nil + } + return facts +} + +// shelfScanLimit is the whole shelf for attachment purposes — the store's own +// default limit, and the same bound the shelf tool reads with. +const shelfScanLimit = 100 + +// leafSkills carries a plan node's shelf attachment onto the task that runs +// it. A store node with no plan node behind it — a spliced edge, a reflex — +// attaches nothing. +func leafSkills(planNode *plan.Node) []string { + if planNode == nil { + return nil + } + return planNode.Skills +} + // taskContract writes the working method for a job small enough to be one leaf. // // It is the same pass a planned job's leaves get, on a graph of one node, so @@ -5676,6 +5708,7 @@ func replanRemainder(settings config.Config, planClient, workClient *liveClient, NodeBudget: min(settings.NodeBudget, replanNodeBudget), Briefs: true, Journal: briefJournal(history, prefix), + Skills: shelfSkills(history), Ensemble: plan.EnsembleNever, // A remainder that the spine finds nothing gated in is one fresh // worker's assignment, and buying a seven-pass planning bundle to From 7b2fc92a616f00ed2c769290fe968d15b6a9d4b6 Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 23:03:21 -0400 Subject: [PATCH 09/14] task: Skills tranche 1: integrate, wire live flow, prepare draft PR Co-Authored-By: CodeAF <267109073+agentfield-bot@users.noreply.github.com> Assisted-by: CodeAF (z-ai/glm-5.3-flash) --- pr-draft.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 pr-draft.md diff --git a/pr-draft.md b/pr-draft.md new file mode 100644 index 000000000..190144df9 --- /dev/null +++ b/pr-draft.md @@ -0,0 +1,31 @@ +WIP review needed: skills tranche 1 (store, attachment, shelf, use_skill) + +## Summary + +- The five-part skill record lives in internal/store and internal/resident: trust, cost-card and digest fields, and serving that marks consumption. +- Ordered per-node skill attachment on briefs: every briefed plan leaf now carries an ordered Skills list on plan.Node and on the journaled store.NodeBrief, composed at brief time by plan.ComposeSkills with pinned skills (names the person's proposal text says outright) first and deterministic cue/scope retrieval behind them. Function words never count as cues, one shared doc word is coincidence, three candidates at most. +- The worker's brief renders the attachment through plan.RenderSkillsBlock beside the working method; a leaf with nothing attached renders byte for byte what it rendered before. +- A windowed chat skill catalog (eight lines, scored, overflow line) and the depth-gated use_skill tool (list and get) are on the session belt wherever a store exists. +- Live wiring of NodeBrief.Skills: the chat engine reads the active shelf once per build, hands it to plan.Build frozen at both brief-journaling sites (the main plan and the remainder replan), and each dispatched leaf carries its plan node's attachment onto the task that runs it. + +## Design + +- https://github.com/Agent-Field/CodeAF/issues/1277#issuecomment-5753795826 + +## Test state + +- `go build ./...` - clean. +- `go test ./internal/plan/... ./internal/orchestrate/... ./internal/session/... ./internal/store/... ./internal/resident/... -count=1 -timeout 30m` - all ok: plan 1.208s, orchestrate 0.598s, session 272.439s, store 14.302s, resident 17.970s. +- `go test ./internal/exec/ -count=1 -timeout 20m` - ok, 259.328s (added beyond the list above because internal/exec/linear.go, the render site, changed). +- New tests cover: a proposal naming a skill attaches it pinned and first on every briefed leaf (checked against the journaled store event and the graph node); a proposal naming nothing attaches nothing; retrieved candidates follow pinned entries; retrieval ignores function words and caps at three; the worker brief renders the block in composed order and renders nothing new when nothing resolves. + +## Notes + +- Later tranches: checks-on-attach, cost-card learning, and the forge. +- Unrelated commits: origin/santos/dev..skills/tranche-1 carries 17 commits from the feat/1089-custom-connections wave that are not this tranche - the named/multiple custom connections work (#1089) with its task and merge commits, the fzf v2 fuzzy matcher wave, a settings-row inventory, the settings search box cursor fix, docs/test chores, and two poem-chapter commits. They were on the checkout's trunk history before the skills work started. +- RenderSkillsBlock and SkillEntry moved from internal/orchestrate to internal/plan, beside ComposeSkills: the executor cannot import orchestrate (orchestrate imports the subharness, which imports the executor), and the render had no live caller. +- The change entry in docs/changes/unreleased is added once the PR number exists: `make changelog-new PR= KIND=changed SLUG=skills-tranche-1`. +- origin/skills/tranche-1 already exists and points at 3bec5413f, an older lineage this branch does not carry (4 commits behind, including "session: fix ActivateSkill test callers after the skills merge"); a push will need reconciliation, which is the author's call. + +— +Drafted with [CodeAF](https://agentfield.ai/github?utm_source=github&utm_medium=pull_request&utm_campaign=drafted_with) · reviewed and owned by the author \ No newline at end of file From c4f5e3cc6aba2e60cb6113d393c6db0f50ac03b4 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Sun, 20 Sep 2026 23:12:29 -0400 Subject: [PATCH 10/14] docs: drop pr-draft.md now that the PR body carries it Co-Authored-By: codeaf Assisted-by: CodeAF (moonshotai/kimi-k3) --- pr-draft.md | 31 ------------------------------- 1 file changed, 31 deletions(-) delete mode 100644 pr-draft.md diff --git a/pr-draft.md b/pr-draft.md deleted file mode 100644 index 190144df9..000000000 --- a/pr-draft.md +++ /dev/null @@ -1,31 +0,0 @@ -WIP review needed: skills tranche 1 (store, attachment, shelf, use_skill) - -## Summary - -- The five-part skill record lives in internal/store and internal/resident: trust, cost-card and digest fields, and serving that marks consumption. -- Ordered per-node skill attachment on briefs: every briefed plan leaf now carries an ordered Skills list on plan.Node and on the journaled store.NodeBrief, composed at brief time by plan.ComposeSkills with pinned skills (names the person's proposal text says outright) first and deterministic cue/scope retrieval behind them. Function words never count as cues, one shared doc word is coincidence, three candidates at most. -- The worker's brief renders the attachment through plan.RenderSkillsBlock beside the working method; a leaf with nothing attached renders byte for byte what it rendered before. -- A windowed chat skill catalog (eight lines, scored, overflow line) and the depth-gated use_skill tool (list and get) are on the session belt wherever a store exists. -- Live wiring of NodeBrief.Skills: the chat engine reads the active shelf once per build, hands it to plan.Build frozen at both brief-journaling sites (the main plan and the remainder replan), and each dispatched leaf carries its plan node's attachment onto the task that runs it. - -## Design - -- https://github.com/Agent-Field/CodeAF/issues/1277#issuecomment-5753795826 - -## Test state - -- `go build ./...` - clean. -- `go test ./internal/plan/... ./internal/orchestrate/... ./internal/session/... ./internal/store/... ./internal/resident/... -count=1 -timeout 30m` - all ok: plan 1.208s, orchestrate 0.598s, session 272.439s, store 14.302s, resident 17.970s. -- `go test ./internal/exec/ -count=1 -timeout 20m` - ok, 259.328s (added beyond the list above because internal/exec/linear.go, the render site, changed). -- New tests cover: a proposal naming a skill attaches it pinned and first on every briefed leaf (checked against the journaled store event and the graph node); a proposal naming nothing attaches nothing; retrieved candidates follow pinned entries; retrieval ignores function words and caps at three; the worker brief renders the block in composed order and renders nothing new when nothing resolves. - -## Notes - -- Later tranches: checks-on-attach, cost-card learning, and the forge. -- Unrelated commits: origin/santos/dev..skills/tranche-1 carries 17 commits from the feat/1089-custom-connections wave that are not this tranche - the named/multiple custom connections work (#1089) with its task and merge commits, the fzf v2 fuzzy matcher wave, a settings-row inventory, the settings search box cursor fix, docs/test chores, and two poem-chapter commits. They were on the checkout's trunk history before the skills work started. -- RenderSkillsBlock and SkillEntry moved from internal/orchestrate to internal/plan, beside ComposeSkills: the executor cannot import orchestrate (orchestrate imports the subharness, which imports the executor), and the render had no live caller. -- The change entry in docs/changes/unreleased is added once the PR number exists: `make changelog-new PR= KIND=changed SLUG=skills-tranche-1`. -- origin/skills/tranche-1 already exists and points at 3bec5413f, an older lineage this branch does not carry (4 commits behind, including "session: fix ActivateSkill test callers after the skills merge"); a push will need reconciliation, which is the author's call. - -— -Drafted with [CodeAF](https://agentfield.ai/github?utm_source=github&utm_medium=pull_request&utm_campaign=drafted_with) · reviewed and owned by the author \ No newline at end of file From 8219b149344646b0fbd07859b5c50ef6c15209af Mon Sep 17 00:00:00 2001 From: codeaf Date: Sun, 20 Sep 2026 23:28:39 -0400 Subject: [PATCH 11/14] task: Fix round 1: address skills review findings Co-Authored-By: CodeAF <267109073+agentfield-bot@users.noreply.github.com> --- cmd/codeaf/chat.go | 6 +- internal/exec/linear.go | 7 +- internal/exec/linear_test.go | 4 +- internal/manual/chat/use-skill.md | 29 ++++ internal/plan/contract.go | 12 +- internal/session/manual_test.go | 17 ++- internal/session/skillcatalog.go | 2 +- internal/session/tools_skill.go | 13 +- internal/store/facts.go | 4 + review-standing-tasks-visibility.md | 224 ---------------------------- 10 files changed, 72 insertions(+), 246 deletions(-) create mode 100644 internal/manual/chat/use-skill.md diff --git a/cmd/codeaf/chat.go b/cmd/codeaf/chat.go index 0e4ce7865..39e9109e3 100644 --- a/cmd/codeaf/chat.go +++ b/cmd/codeaf/chat.go @@ -5512,9 +5512,9 @@ func shelfSkills(history *store.Store) []store.Fact { return facts } -// shelfScanLimit is the whole shelf for attachment purposes — the store's own -// default limit, and the same bound the shelf tool reads with. -const shelfScanLimit = 100 +// shelfScanLimit is the whole shelf for attachment purposes, from the one +// source of truth in internal/store. +const shelfScanLimit = store.SkillShelfLimit // leafSkills carries a plan node's shelf attachment onto the task that runs // it. A store node with no plan node behind it — a spliced edge, a reflex — diff --git a/internal/exec/linear.go b/internal/exec/linear.go index f97539d78..375fc6e7f 100644 --- a/internal/exec/linear.go +++ b/internal/exec/linear.go @@ -1798,10 +1798,9 @@ func (l *Linear) brief(task Task) string { return block.String() } -// skillResolveLimit bounds the shelf read one brief's resolution makes. It -// mirrors the shelf tool's own bound (internal/session's skillShelfLimit): the -// shelf is a curated few, and reading further would only slow dispatch. -const skillResolveLimit = 100 +// skillResolveLimit bounds the shelf read one brief's resolution makes, from +// the one source of truth in internal/store. +const skillResolveLimit = store.SkillShelfLimit // skillEntries resolves the leaf's attached skill names against the active // shelf, keeping the order the plan composed — that order is the precedence diff --git a/internal/exec/linear_test.go b/internal/exec/linear_test.go index 5a056ffd6..df05c26cc 100644 --- a/internal/exec/linear_test.go +++ b/internal/exec/linear_test.go @@ -733,7 +733,7 @@ func shelfOnDisk(t *testing.T, name, body string) *store.Store { if err != nil { t.Fatalf("record skill candidate %q: %v", body, err) } - if err := db.ActivateSkill(candidate.Seq, "/shelf/"+name); err != nil { + if err := db.ActivateSkill(candidate.Seq, "/shelf/"+name, ""); err != nil { t.Fatalf("activate skill %q: %v", name, err) } return db @@ -750,7 +750,7 @@ func TestABriefRendersAttachedSkillsInOrder(t *testing.T) { if err != nil { t.Fatal(err) } - if err := db.ActivateSkill(candidate.Seq, "/shelf/lint"); err != nil { + if err := db.ActivateSkill(candidate.Seq, "/shelf/lint", ""); err != nil { t.Fatal(err) } linear := NewLinear(&scriptedCompleter{}, workspace(t), nil, 10, 1_000_000, time.Minute).WithStore(db) diff --git a/internal/manual/chat/use-skill.md b/internal/manual/chat/use-skill.md new file mode 100644 index 000000000..1ceb8063c --- /dev/null +++ b/internal/manual/chat/use-skill.md @@ -0,0 +1,29 @@ +# use_skill — list or get a skill from the shelf + +## What it does + +Reads the shelf of active, execution-verified skills this project has saved. `use_skill` has two modes, the way `jobs` and `settings` do: + +- **`list`** — shows every active skill by name with its one-line doc. No internal fields, no paths. +- **`get`** — resolves one name to its shelf path and full doc, which you then `read`. + +## How to use it + +``` +use_skill mode=list +use_skill mode=get name=linter +``` + +The name is the directory name on the shelf, exactly as `list` printed it. + +## Why it exists + +The distiller saves verified procedures as skills and promotes them to the active shelf. Until now nothing a worker held could reach one — the shelf was written to and promoted, and the only reader was a person with the CLI. This is your door onto it: mid-run discovery rather than a prompt fact. + +## Where the shelf lives + +Active skills are `store.Fact` entries of kind `"skill"`, pointed at a shelf directory their `Artifact` names. The shelf is curated — skills are promoted by a person through the resident. + +## Tool name + +The verb is `use_skill` on the belt. A belt that does not carry `propose_task` does not carry this verb either (it is gated on the same condition plus a non-nil store). \ No newline at end of file diff --git a/internal/plan/contract.go b/internal/plan/contract.go index 97e3bfda7..6a3427f34 100644 --- a/internal/plan/contract.go +++ b/internal/plan/contract.go @@ -480,9 +480,19 @@ func PinnedSkills(text string, skills []store.Fact) []string { if strings.TrimSpace(text) == "" { return nil } + // Tokenize the goal into whole words so a skill named "lint" is never + // pinned by "splinter" or "test" by "latest". + words := make(map[string]bool) + for _, word := range strings.FieldsFunc(strings.ToLower(text), func(r rune) bool { + return !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') + }) { + if word != "" { + words[word] = true + } + } pinned := make([]string, 0, len(skills)) for _, fact := range skills { - if name := fact.SkillName(); name != "" && strings.Contains(text, name) { + if name := fact.SkillName(); name != "" && words[strings.ToLower(name)] { pinned = append(pinned, name) } } diff --git a/internal/session/manual_test.go b/internal/session/manual_test.go index e1fc72098..617a93d62 100644 --- a/internal/session/manual_test.go +++ b/internal/session/manual_test.go @@ -1,9 +1,11 @@ package session import ( + "path/filepath" "testing" "github.com/Agent-Field/codeaf/internal/manual" + "github.com/Agent-Field/codeaf/internal/store" ) // The belt half of the completeness gate (internal/tui3's manual_test.go is the @@ -20,11 +22,16 @@ func TestTheManualMentionsEveryToolOnTheBelt(t *testing.T) { // that built the belt without one would let a conditional tool ship with no // page — green, and wrong in exactly the way this test exists to catch. agent := &Agent{config: Config{Workspace: t.TempDir(), ProfileDir: t.TempDir()}} - // AND THE SHELF IS WALKED WITH THE BELT. A tool held back for the tool block's - // sake is a capability this build still has and a person can still ask about - // (tools_capabilities.go), so it goes on owing a page — a gate reading the - // carried belt alone would go green the day a verb was shelved, which is the - // one way shelving could quietly delete a feature. + // AND THE SHELF IS REACHED THROUGH A STORE. `use_skill` is gated on a non-nil + // Memory (tools_skill.go) — without one the verb never lands on the belt and + // the gate would let it ship without a page. So a store is opened so the belt + // is the same one a real conversation carries. + db, err := store.Open(filepath.Join(t.TempDir(), "graph.db")) + if err != nil { + t.Fatalf("open gate store: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + agent.config.Memory = db agent.tools = agent.belt() tools := agent.offeredTools() if len(tools) == 0 { diff --git a/internal/session/skillcatalog.go b/internal/session/skillcatalog.go index 64c69eede..6173482ab 100644 --- a/internal/session/skillcatalog.go +++ b/internal/session/skillcatalog.go @@ -47,7 +47,7 @@ const ( // is the unit the lean profile drops (promptprofile.go's [leanPageSections]), // and the shelf is not a law to be traded against window size. skillCatalogHeader = "## Available skills\n\n" + - "Chat routes skills to tasks — use them through task nodes, never inline.\n" + "Skills are prepended to task work — use them through task nodes or by name with `use_skill`.\n" ) // renderSkillCatalog composes the skill catalog for one config, or the empty diff --git a/internal/session/tools_skill.go b/internal/session/tools_skill.go index cf8b355d6..a9f1efadd 100644 --- a/internal/session/tools_skill.go +++ b/internal/session/tools_skill.go @@ -36,10 +36,8 @@ import ( const useSkillToolName = "use_skill" -// skillShelfLimit bounds one listing. The shelf is a curated few, not a corpus, -// so a hundred is every skill any machine has ever held; a larger number would -// only change how much of a runaway shelf rides in one answer. -const skillShelfLimit = 100 +// skillShelfLimit bounds one listing, from the one source of truth. +const skillShelfLimit = store.SkillShelfLimit // useSkillDescription says what the two modes are for in the model's own terms. // It is bought on every request of every turn on a belt that carries it, so it @@ -139,8 +137,11 @@ func (a *Agent) getSkill(name string) (string, bool, error) { if filepath.Base(skill.Artifact) != name { continue } - // TODO: call store.SkillServe when it exists to mark consumption for consolidation weighting - return fmt.Sprintf("%s: %s\nPath: %s", name, skill.Body, skill.Artifact), false, nil + artifact, doc, _, _, err := a.config.Memory.SkillFactAccessors(skill.Seq) + if err != nil { + return "Could not read skill: " + err.Error(), true, nil + } + return fmt.Sprintf("%s: %s\nPath: %s", name, doc, artifact), false, nil } return "Skill '" + name + "' not found.", false, nil } diff --git a/internal/store/facts.go b/internal/store/facts.go index 584670610..45709b882 100644 --- a/internal/store/facts.go +++ b/internal/store/facts.go @@ -22,6 +22,10 @@ import ( // MaxFactBytes bounds one fact. A fact is one standalone line, not a report. const MaxFactBytes = 512 +// SkillShelfLimit is the one bound every shelf reader uses — the shelf is +// a curated few, and reading past it would only slow dispatch or the prompt. +const SkillShelfLimit = 100 + // FactKind classifies what a notebook entry teaches. // AgeLabel renders how old a fact is, for retrieval surfaces: every reader // of a memory sees when it was written, because a claim's age is part of its diff --git a/review-standing-tasks-visibility.md b/review-standing-tasks-visibility.md index cb37a17a3..e69de29bb 100644 --- a/review-standing-tasks-visibility.md +++ b/review-standing-tasks-visibility.md @@ -1,224 +0,0 @@ -# Review: standing-run task visibility — 5 claims checked against the code - -Codebase: `/Users/santoshkumar/Documents/agentfield/code/codeaf`, commit `aae00e7a7`. -All citations are `file:line`. Each claim is confirmed or disproved against the -code, not the host task's reasoning. - ---- - -## Claim 1 — tool registration and search path — **confirmed** - -- `func (a *Agent) tasksTool() bare.Tool` at `tools_tasks.go:181`, with - `Description: tasksDescription` (`tools_tasks.go:184`, const at `:72`). - The no-id branch calls `a.taskSearchText(parsed.Query, parsed.Limit, scope)` - (`tools_tasks.go:216`); the id branch calls `a.oneTask(ctx, token, parsed)` - (`tools_tasks.go:223`). -- Belt registration: `tools.go:181` — `tools = append(tools, a.tasksTool())`, - gated by `if a.mayProposeTask()` (`tools.go:179`). `mayProposeTask` is - `!c.InTask || c.mayFanOut()` (`task.go:534`), and `mayFanOut` is - `c.InTask && c.tasker != nil && fansOutAt(c.taskDepth)` (`task.go:540`). - So an InTask firing with a graph (an armed one) does get the tool; an InTask - firing without one does not. The eye item's run transcript calls `tasks` - repeatedly, so the tool was present for that firing. -- `func (a *Agent) taskSearchText(query string, limit int, scope string) string` - at `tools_tasks.go:246`. Its first row of data is - `taskRowsTextLimit(a.taskRows(), query, limit)` at `:255`. -- `func (a *Agent) taskRows() []TaskIndexEntry` at `tools_tasks.go:935`: - `parent := a.config.taskID; if parent == 0 { return a.TaskIndex() }`. So the - no-id search reads the project index **only when `taskID == 0`**; a node - (`taskID != 0`) reads its own graph children instead. -- `func (a *Agent) TaskIndex() []TaskIndexEntry` at `task_index.go:571`: - `rows := ReadTaskIndex(a.config.taskIndexFile())` at `:572`. -- `func (c Config) taskIndexFile() string` at `task_index.go:404`: - `if dir := strings.TrimSpace(c.Place.Dir); dir != "" { ... return filepath.Join(bucket, taskIndexName) }`, - where `bucket := filepath.Dir(dir)` at `:406`. `taskIndexName` is `"tasks.jsonl"` - (`task_index.go:75`). -- `func ReadTaskIndex(path string) []TaskIndexEntry` at `task_index.go:456`: - `if strings.TrimSpace(path) == "" { return nil }` and - `file, err := os.Open(path); if err != nil { return nil }` — a missing file - returns nil. - -The chain `taskSearchText → taskRows → TaskIndex → ReadTaskIndex(taskIndexFile())` -is exactly as claimed. One nuance the claim omits: `taskRows()` only reaches -`TaskIndex()` when `taskID == 0`; a node with `taskID != 0` never reads the -index file at all (it reads graph children). This matters for claims 4 and 5. - ---- - -## Claim 2 — wrong path for a standing run — **confirmed** - -- `func standingRunConfig(parent Config, item standing.Item, runDir string) (Config, error)` - at `standing_run.go:766`. Line `:773`: - `place := Place{Dir: runDir, Workspace: item.Workspace}`, then `:778`: - `cfg.Place = place`. -- `runDir` comes from the standing store. `Store.RunsDir(id)` at - `standing.go:653` is `filepath.Join(s.ItemDir(id), "runs")` = - `//runs`. The next-run folder is - `filepath.Join(runs, fmt.Sprintf("%04d", attempt))` (`store.go:288`), so a - run directory is `//runs/0001`. The root is `v3StandingRoot()` = - `home.Join("v3", "standing")` (`chatv3_standing.go:59`), so a run directory - is `~/.codeaf/v3/standing//runs/0001`. -- `taskIndexFile()` at `:404-414` does `bucket := filepath.Dir(runDir)` = - `~/.codeaf/v3/standing//runs`, then - `filepath.Join(bucket, "tasks.jsonl")` = - `~/.codeaf/v3/standing//runs/tasks.jsonl`. - That is NOT the project bucket. The project bucket is - `~/.codeaf/v3/projects//`, which holds the real - `tasks.jsonl`. -- Empirical confirmation on this machine: `find ~/.codeaf/v3/standing -name tasks.jsonl` - returns nothing; `~/.codeaf/v3/projects/-Users-santoshkumar-Documents-agentfield-codeaf/tasks.jsonl` - exists and is 253 KB. -- `ReadTaskIndex` returns nil for the missing file (`task_index.go:456-460`), - so `TaskIndex()` returns nil, and the no-id search prints - "No tasks have run in this project yet." (`tools_tasks.go:1109`). The real - run transcript confirms this exact string. - ---- - -## Claim 3 — `tellsElsewhere()` blocks elsewhere and everywhere — **confirmed** - -- `func (a *Agent) tellsElsewhere() bool` at `taskdelta.go:721`: - `if a.config.InTask || a.config.taskID != 0 { return false }` at `:722`, - then `return strings.TrimSpace(a.config.Place.Dir) != ""` at `:725`. -- `standingRunConfig` sets `cfg.InTask = true` at `standing_run.go:780`. - (`standing_run.go:365` also sets `InTask = true`, but that is in - `probeTool`'s throwaway belt-probe agent at `:346-365` — a different code - path, not the firing session.) -- `taskSearchText` at `tools_tasks.go:256`: - `if !a.tellsElsewhere() { return a.taskConversationHint(out) }` — returns - before the `taskElsewhereText` call at `:260` and before the - `scope != taskScopeEverywhere` check at `:268`. So `scope=everywhere` cannot - reach `OtherProjects` either: the gate is before the scope branch. - -Confirmed: both the elsewhere and everywhere readings are blocked for a -standing firing. The claim cites `InTask = true` as the reason; for an armed -firing `taskID != 0` (see claim 4) would also trigger the same `return false`, -so the gate holds either way. - ---- - -## Claim 4 — id query message and `taskID == 0` — **disagrees** - -The claim asserts `taskID == 0` for standing runs, that the message is -"No task '4' in this project...", and that "the person paraphrased." The code -and the **real run transcript** show the opposite on all three points. - -**What the code does.** `standingRunConfig` at `:766-790` does not set -`cfg.taskID` — the claim's parenthetical is true for that function alone. But -`Run()` at `:555-556` calls `standingWideWork(cfg, item, brief)` immediately -after, and `standingWideWork` at `standing_run.go:940-941` sets: - -```go -cfg.tasker = graph -cfg.taskID = id // the root node's id, from graph.reserve() at task_run.go:1281 -``` - -...when the firing is armed for wide work: `if !cfg.Divide || !enumeratesWidth(...)` -returns false at `:884`. `cfg.Divide` comes from `v3StandingPosture` at -`chatv3_standing.go:192` (`Divide: settings.Swarm`), and `enumeratesWidth` -(`task_divide.go:635`) calls `splitgate.WorthIt` on the brief text. - -**What the real run did.** The eye item `8eac8a7f9f983bf0` fired once -(`runs/0001`). Its transcript shows the `tasks` calls and their results: - -``` -tasks {"query":"Skills:","limit":20} → No task matches "Skills:". ... -tasks {"limit":30} → No tasks have run in this project yet. -tasks {"limit":30,"scope":"everywhere"} → No tasks have run in this project yet. -tasks {"id":"4"} → No task "4" among the pieces you handed out. Call tasks with no arguments to see them; ... -tasks {"id":"5"} → No task "5" among the pieces you handed out. ... -tasks {"id":"6"} → No task "6" among the pieces you handed out. ... -``` - -The id-query message is **"No task \"4\" among the pieces you handed out"** — -the `taskID != 0` branch at `tools_tasks.go:648-651`: - -```go -if a.config.taskID != 0 { - return fmt.Sprintf("No task %q among the pieces you handed out. ...", token), true, nil -} -return fmt.Sprintf("No task %q in this project. ...", token), true, nil // :653, the == 0 branch -``` - -So `taskID` was nonzero for this firing: `standingWideWork` armed it, and the -"among the pieces" branch fired — not the "in this project" branch the claim -names. The person did not paraphrase; the code produced that message. - -**Why `taskRows()` is empty.** The claim says "taskRows() returns empty → -taskByToken can't find them" and attributes the emptiness to `ReadTaskIndex` -finding nothing (the wrong path from claim 2). That is the wrong mechanism for -this firing. With `taskID != 0`, `taskRows()` at `:935-948` never calls -`TaskIndex()`: - -```go -func (a *Agent) taskRows() []TaskIndexEntry { - parent := a.config.taskID - if parent == 0 { return a.TaskIndex() } // not taken - // ...children of the root node from the graph... -} -``` - -It returns the root node's children from the graph. The eye did not divide, so -the root has no children, and `taskRows()` returns an empty slice. The wrong -index path (claim 2) is real, but it is not what produces the empty rows for an -armed firing — `TaskIndex()` is never reached. - -**Summary of disagreement.** The claim is right that the id query fails and -that `taskRows()` is empty, but wrong about (a) which branch fires -(`taskID != 0`, not `== 0`), (b) the exact message ("among the pieces you -handed out", not "in this project"), (c) the cause of the empty rows (graph -children, not the index file), and (d) the claim that -"standingRunConfig never sets taskID" misses `standingWideWork` at `:941` -which does. - ---- - -## Claim 5 — this is a bug, and no override exists — **confirmed, with one gap in the claim's reasoning** - -**The path is wrong.** `taskIndexFile()` at `task_index.go:404-414` derives -the index path from `filepath.Dir(Place.Dir)`. For a normal session, -`Place.Dir` is `~/.codeaf/v3/projects///`, so -`Dir` gives the project bucket `~/.codeaf/v3/projects//` — -correct. For a standing firing, `Place.Dir` is -`~/.codeaf/v3/standing//runs/0001`, so `Dir` gives -`~/.codeaf/v3/standing//runs/` — wrong: no project index lives there. -The `task_index.go` header at `:40-56` states the design intent: -"In the PROJECT BUCKET ... The scope is the project and not the conversation," -and "the parent of the folder is the bucket." A run folder's parent is `runs/`, -not the project bucket, so the derivation assumption breaks. - -**No override exists.** I searched `task_index.go` for `standing` and `InTask`: -the only hit is `closeInflightTaskIndexRows` at `:686`, which guards a write -path (`if a.config.InTask { return }`) and has nothing to do with path -resolution. `taskIndexFile()` has no special case for standing runs. - -**The claim's reasoning has a gap.** The claim says "the design intent is -that InTask agents (including standing firings) should see their own children" -and identifies the wrong path as the cause. The wrong path is real, but it is -not the cause of what the eye experienced. For an **armed** firing -(`taskID != 0` via `standingWideWork`), `taskRows()` at `:935` returns graph -children and never calls `TaskIndex()`, so fixing `taskIndexFile()` alone -would not let the eye see the conversation's tasks 4/5/6 — it would still get -its own (empty) children. The wrong path affects only the `taskID == 0` path -(the no-id search), which prints "No tasks have run in this project yet." -because `TaskIndex()` returns nil. The id-query failure has a second, more -direct cause: the armed firing's `taskID` makes `taskRows()` return graph -children instead of the project index. - -The claim's recommendation — "a shell probe against the project's real task -index file ... NOT the `tasks` tool inside the firing" — is a design -suggestion I was asked not to make or evaluate; I confirm only its factual -basis: there is no override, and the path the `tasks` tool resolves is not the -project bucket. - ---- - -## One-line summary - -Claims 1, 2, 3, and 5 are confirmed by the code. Claim 4 is disproved: the -firing was armed (`standingWideWork` at `standing_run.go:941` set `cfg.taskID`), -so the real message was "among the pieces you handed out" (the `taskID != 0` -branch at `tools_tasks.go:651`), not "in this project" (the `taskID == 0` -branch at `:653`), and the empty rows came from the graph having no children, -not from `ReadTaskIndex` finding nothing — though the wrong index path from -claim 2 is independently real. From d1d874af898163038b53df7284ccf458339d4244 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Sun, 20 Sep 2026 23:28:55 -0400 Subject: [PATCH 12/14] audit-notes: move the standing-visibility investigation out of the PR diff Co-Authored-By: codeaf --- .../review-standing-tasks-visibility.md | 224 ++++++++++++++++++ review-standing-tasks-visibility.md | 0 2 files changed, 224 insertions(+) create mode 100644 audit-notes/review-standing-tasks-visibility.md delete mode 100644 review-standing-tasks-visibility.md diff --git a/audit-notes/review-standing-tasks-visibility.md b/audit-notes/review-standing-tasks-visibility.md new file mode 100644 index 000000000..cb37a17a3 --- /dev/null +++ b/audit-notes/review-standing-tasks-visibility.md @@ -0,0 +1,224 @@ +# Review: standing-run task visibility — 5 claims checked against the code + +Codebase: `/Users/santoshkumar/Documents/agentfield/code/codeaf`, commit `aae00e7a7`. +All citations are `file:line`. Each claim is confirmed or disproved against the +code, not the host task's reasoning. + +--- + +## Claim 1 — tool registration and search path — **confirmed** + +- `func (a *Agent) tasksTool() bare.Tool` at `tools_tasks.go:181`, with + `Description: tasksDescription` (`tools_tasks.go:184`, const at `:72`). + The no-id branch calls `a.taskSearchText(parsed.Query, parsed.Limit, scope)` + (`tools_tasks.go:216`); the id branch calls `a.oneTask(ctx, token, parsed)` + (`tools_tasks.go:223`). +- Belt registration: `tools.go:181` — `tools = append(tools, a.tasksTool())`, + gated by `if a.mayProposeTask()` (`tools.go:179`). `mayProposeTask` is + `!c.InTask || c.mayFanOut()` (`task.go:534`), and `mayFanOut` is + `c.InTask && c.tasker != nil && fansOutAt(c.taskDepth)` (`task.go:540`). + So an InTask firing with a graph (an armed one) does get the tool; an InTask + firing without one does not. The eye item's run transcript calls `tasks` + repeatedly, so the tool was present for that firing. +- `func (a *Agent) taskSearchText(query string, limit int, scope string) string` + at `tools_tasks.go:246`. Its first row of data is + `taskRowsTextLimit(a.taskRows(), query, limit)` at `:255`. +- `func (a *Agent) taskRows() []TaskIndexEntry` at `tools_tasks.go:935`: + `parent := a.config.taskID; if parent == 0 { return a.TaskIndex() }`. So the + no-id search reads the project index **only when `taskID == 0`**; a node + (`taskID != 0`) reads its own graph children instead. +- `func (a *Agent) TaskIndex() []TaskIndexEntry` at `task_index.go:571`: + `rows := ReadTaskIndex(a.config.taskIndexFile())` at `:572`. +- `func (c Config) taskIndexFile() string` at `task_index.go:404`: + `if dir := strings.TrimSpace(c.Place.Dir); dir != "" { ... return filepath.Join(bucket, taskIndexName) }`, + where `bucket := filepath.Dir(dir)` at `:406`. `taskIndexName` is `"tasks.jsonl"` + (`task_index.go:75`). +- `func ReadTaskIndex(path string) []TaskIndexEntry` at `task_index.go:456`: + `if strings.TrimSpace(path) == "" { return nil }` and + `file, err := os.Open(path); if err != nil { return nil }` — a missing file + returns nil. + +The chain `taskSearchText → taskRows → TaskIndex → ReadTaskIndex(taskIndexFile())` +is exactly as claimed. One nuance the claim omits: `taskRows()` only reaches +`TaskIndex()` when `taskID == 0`; a node with `taskID != 0` never reads the +index file at all (it reads graph children). This matters for claims 4 and 5. + +--- + +## Claim 2 — wrong path for a standing run — **confirmed** + +- `func standingRunConfig(parent Config, item standing.Item, runDir string) (Config, error)` + at `standing_run.go:766`. Line `:773`: + `place := Place{Dir: runDir, Workspace: item.Workspace}`, then `:778`: + `cfg.Place = place`. +- `runDir` comes from the standing store. `Store.RunsDir(id)` at + `standing.go:653` is `filepath.Join(s.ItemDir(id), "runs")` = + `//runs`. The next-run folder is + `filepath.Join(runs, fmt.Sprintf("%04d", attempt))` (`store.go:288`), so a + run directory is `//runs/0001`. The root is `v3StandingRoot()` = + `home.Join("v3", "standing")` (`chatv3_standing.go:59`), so a run directory + is `~/.codeaf/v3/standing//runs/0001`. +- `taskIndexFile()` at `:404-414` does `bucket := filepath.Dir(runDir)` = + `~/.codeaf/v3/standing//runs`, then + `filepath.Join(bucket, "tasks.jsonl")` = + `~/.codeaf/v3/standing//runs/tasks.jsonl`. + That is NOT the project bucket. The project bucket is + `~/.codeaf/v3/projects//`, which holds the real + `tasks.jsonl`. +- Empirical confirmation on this machine: `find ~/.codeaf/v3/standing -name tasks.jsonl` + returns nothing; `~/.codeaf/v3/projects/-Users-santoshkumar-Documents-agentfield-codeaf/tasks.jsonl` + exists and is 253 KB. +- `ReadTaskIndex` returns nil for the missing file (`task_index.go:456-460`), + so `TaskIndex()` returns nil, and the no-id search prints + "No tasks have run in this project yet." (`tools_tasks.go:1109`). The real + run transcript confirms this exact string. + +--- + +## Claim 3 — `tellsElsewhere()` blocks elsewhere and everywhere — **confirmed** + +- `func (a *Agent) tellsElsewhere() bool` at `taskdelta.go:721`: + `if a.config.InTask || a.config.taskID != 0 { return false }` at `:722`, + then `return strings.TrimSpace(a.config.Place.Dir) != ""` at `:725`. +- `standingRunConfig` sets `cfg.InTask = true` at `standing_run.go:780`. + (`standing_run.go:365` also sets `InTask = true`, but that is in + `probeTool`'s throwaway belt-probe agent at `:346-365` — a different code + path, not the firing session.) +- `taskSearchText` at `tools_tasks.go:256`: + `if !a.tellsElsewhere() { return a.taskConversationHint(out) }` — returns + before the `taskElsewhereText` call at `:260` and before the + `scope != taskScopeEverywhere` check at `:268`. So `scope=everywhere` cannot + reach `OtherProjects` either: the gate is before the scope branch. + +Confirmed: both the elsewhere and everywhere readings are blocked for a +standing firing. The claim cites `InTask = true` as the reason; for an armed +firing `taskID != 0` (see claim 4) would also trigger the same `return false`, +so the gate holds either way. + +--- + +## Claim 4 — id query message and `taskID == 0` — **disagrees** + +The claim asserts `taskID == 0` for standing runs, that the message is +"No task '4' in this project...", and that "the person paraphrased." The code +and the **real run transcript** show the opposite on all three points. + +**What the code does.** `standingRunConfig` at `:766-790` does not set +`cfg.taskID` — the claim's parenthetical is true for that function alone. But +`Run()` at `:555-556` calls `standingWideWork(cfg, item, brief)` immediately +after, and `standingWideWork` at `standing_run.go:940-941` sets: + +```go +cfg.tasker = graph +cfg.taskID = id // the root node's id, from graph.reserve() at task_run.go:1281 +``` + +...when the firing is armed for wide work: `if !cfg.Divide || !enumeratesWidth(...)` +returns false at `:884`. `cfg.Divide` comes from `v3StandingPosture` at +`chatv3_standing.go:192` (`Divide: settings.Swarm`), and `enumeratesWidth` +(`task_divide.go:635`) calls `splitgate.WorthIt` on the brief text. + +**What the real run did.** The eye item `8eac8a7f9f983bf0` fired once +(`runs/0001`). Its transcript shows the `tasks` calls and their results: + +``` +tasks {"query":"Skills:","limit":20} → No task matches "Skills:". ... +tasks {"limit":30} → No tasks have run in this project yet. +tasks {"limit":30,"scope":"everywhere"} → No tasks have run in this project yet. +tasks {"id":"4"} → No task "4" among the pieces you handed out. Call tasks with no arguments to see them; ... +tasks {"id":"5"} → No task "5" among the pieces you handed out. ... +tasks {"id":"6"} → No task "6" among the pieces you handed out. ... +``` + +The id-query message is **"No task \"4\" among the pieces you handed out"** — +the `taskID != 0` branch at `tools_tasks.go:648-651`: + +```go +if a.config.taskID != 0 { + return fmt.Sprintf("No task %q among the pieces you handed out. ...", token), true, nil +} +return fmt.Sprintf("No task %q in this project. ...", token), true, nil // :653, the == 0 branch +``` + +So `taskID` was nonzero for this firing: `standingWideWork` armed it, and the +"among the pieces" branch fired — not the "in this project" branch the claim +names. The person did not paraphrase; the code produced that message. + +**Why `taskRows()` is empty.** The claim says "taskRows() returns empty → +taskByToken can't find them" and attributes the emptiness to `ReadTaskIndex` +finding nothing (the wrong path from claim 2). That is the wrong mechanism for +this firing. With `taskID != 0`, `taskRows()` at `:935-948` never calls +`TaskIndex()`: + +```go +func (a *Agent) taskRows() []TaskIndexEntry { + parent := a.config.taskID + if parent == 0 { return a.TaskIndex() } // not taken + // ...children of the root node from the graph... +} +``` + +It returns the root node's children from the graph. The eye did not divide, so +the root has no children, and `taskRows()` returns an empty slice. The wrong +index path (claim 2) is real, but it is not what produces the empty rows for an +armed firing — `TaskIndex()` is never reached. + +**Summary of disagreement.** The claim is right that the id query fails and +that `taskRows()` is empty, but wrong about (a) which branch fires +(`taskID != 0`, not `== 0`), (b) the exact message ("among the pieces you +handed out", not "in this project"), (c) the cause of the empty rows (graph +children, not the index file), and (d) the claim that +"standingRunConfig never sets taskID" misses `standingWideWork` at `:941` +which does. + +--- + +## Claim 5 — this is a bug, and no override exists — **confirmed, with one gap in the claim's reasoning** + +**The path is wrong.** `taskIndexFile()` at `task_index.go:404-414` derives +the index path from `filepath.Dir(Place.Dir)`. For a normal session, +`Place.Dir` is `~/.codeaf/v3/projects///`, so +`Dir` gives the project bucket `~/.codeaf/v3/projects//` — +correct. For a standing firing, `Place.Dir` is +`~/.codeaf/v3/standing//runs/0001`, so `Dir` gives +`~/.codeaf/v3/standing//runs/` — wrong: no project index lives there. +The `task_index.go` header at `:40-56` states the design intent: +"In the PROJECT BUCKET ... The scope is the project and not the conversation," +and "the parent of the folder is the bucket." A run folder's parent is `runs/`, +not the project bucket, so the derivation assumption breaks. + +**No override exists.** I searched `task_index.go` for `standing` and `InTask`: +the only hit is `closeInflightTaskIndexRows` at `:686`, which guards a write +path (`if a.config.InTask { return }`) and has nothing to do with path +resolution. `taskIndexFile()` has no special case for standing runs. + +**The claim's reasoning has a gap.** The claim says "the design intent is +that InTask agents (including standing firings) should see their own children" +and identifies the wrong path as the cause. The wrong path is real, but it is +not the cause of what the eye experienced. For an **armed** firing +(`taskID != 0` via `standingWideWork`), `taskRows()` at `:935` returns graph +children and never calls `TaskIndex()`, so fixing `taskIndexFile()` alone +would not let the eye see the conversation's tasks 4/5/6 — it would still get +its own (empty) children. The wrong path affects only the `taskID == 0` path +(the no-id search), which prints "No tasks have run in this project yet." +because `TaskIndex()` returns nil. The id-query failure has a second, more +direct cause: the armed firing's `taskID` makes `taskRows()` return graph +children instead of the project index. + +The claim's recommendation — "a shell probe against the project's real task +index file ... NOT the `tasks` tool inside the firing" — is a design +suggestion I was asked not to make or evaluate; I confirm only its factual +basis: there is no override, and the path the `tasks` tool resolves is not the +project bucket. + +--- + +## One-line summary + +Claims 1, 2, 3, and 5 are confirmed by the code. Claim 4 is disproved: the +firing was armed (`standingWideWork` at `standing_run.go:941` set `cfg.taskID`), +so the real message was "among the pieces you handed out" (the `taskID != 0` +branch at `tools_tasks.go:651`), not "in this project" (the `taskID == 0` +branch at `:653`), and the empty rows came from the graph having no children, +not from `ReadTaskIndex` finding nothing — though the wrong index path from +claim 2 is independently real. diff --git a/review-standing-tasks-visibility.md b/review-standing-tasks-visibility.md deleted file mode 100644 index e69de29bb..000000000 From a51cc6e6ed5f4874c7219f0299ed61f1c4096b08 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Sun, 20 Sep 2026 23:29:48 -0400 Subject: [PATCH 13/14] session: align the catalog header test with the reviewed rewording Co-Authored-By: codeaf --- internal/session/skillcatalog_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/session/skillcatalog_test.go b/internal/session/skillcatalog_test.go index e4df13cbd..ab32a54a6 100644 --- a/internal/session/skillcatalog_test.go +++ b/internal/session/skillcatalog_test.go @@ -122,7 +122,7 @@ func TestSkillCatalogSurfacesRelevantSkills(t *testing.T) { // TestSkillCatalogAlwaysCarriesItsHeader: whenever there is anything to show, // the section heading and the routing sentence are present — the model is told -// these are skills reached through task nodes, not verbs to run inline. +// these are skills reached through task nodes or by name with use_skill. func TestSkillCatalogAlwaysCarriesItsHeader(t *testing.T) { brain := openTestBrain(t) activeSkill(t, brain, "domain:alpha", "one skill on the shelf", "/shelf/only-skill") @@ -131,7 +131,7 @@ func TestSkillCatalogAlwaysCarriesItsHeader(t *testing.T) { if !strings.HasPrefix(catalog, "## Available skills\n") { t.Fatalf("catalog does not open on its heading:\n%s", catalog) } - if !strings.Contains(catalog, "use them through task nodes, never inline") { + if !strings.Contains(catalog, "use them through task nodes or by name with `use_skill`") { t.Fatalf("catalog does not carry the routing sentence:\n%s", catalog) } if !strings.Contains(catalog, "- only-skill: one skill on the shelf") { From af61b0cc7c038c77fc33b394bd0e247e1a13562b Mon Sep 17 00:00:00 2001 From: codeaf Date: Mon, 21 Sep 2026 09:27:39 -0400 Subject: [PATCH 14/14] task: Fix round 3: final fixes for skills tranche 1 Co-Authored-By: CodeAF <267109073+agentfield-bot@users.noreply.github.com> --- internal/plan/contract.go | 52 ++++++++++++++++++++++++-- internal/plan/contract_test.go | 22 +++++++++++ internal/store/facts.go | 2 +- internal/store/store_test.go | 67 ++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 5 deletions(-) diff --git a/internal/plan/contract.go b/internal/plan/contract.go index 6a3427f34..84c7080a0 100644 --- a/internal/plan/contract.go +++ b/internal/plan/contract.go @@ -482,23 +482,67 @@ func PinnedSkills(text string, skills []store.Fact) []string { } // Tokenize the goal into whole words so a skill named "lint" is never // pinned by "splinter" or "test" by "latest". - words := make(map[string]bool) - for _, word := range strings.FieldsFunc(strings.ToLower(text), func(r rune) bool { + tokens := strings.FieldsFunc(strings.ToLower(text), func(r rune) bool { return !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') - }) { + }) + words := make(map[string]bool, len(tokens)) + for _, word := range tokens { if word != "" { words[word] = true } } pinned := make([]string, 0, len(skills)) for _, fact := range skills { - if name := fact.SkillName(); name != "" && words[strings.ToLower(name)] { + name := fact.SkillName() + if name == "" { + continue + } + lower := strings.ToLower(name) + if words[lower] { pinned = append(pinned, name) + continue + } + // Hyphenated skill names (e.g. "repo-audit") are broken into separate + // tokens by the alnum splitter. Check whether the name's own alnum + // token sequence appears as a contiguous subsequence of the goal's + // tokens, so a literal name pins without matching its fragments + // individually. + if strings.ContainsAny(lower, "-_.") { + parts := alnumParts(lower) + if len(parts) >= 2 && containsContiguous(tokens, parts) { + pinned = append(pinned, name) + } } } return pinned } +// alnumParts splits s into runs of alphanumeric characters. +func alnumParts(s string) []string { + return strings.FieldsFunc(s, func(r rune) bool { + return !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') + }) +} + +// containsContiguous reports whether sub appears as a contiguous subsequence +// of all. Both slices are from the same splitter, so elements compare by value. +func containsContiguous(all, sub []string) bool { + if len(sub) > len(all) { + return false + } + limit := len(all) - len(sub) +outer: + for i := 0; i <= limit; i++ { + for j, p := range sub { + if all[i+j] != p { + continue outer + } + } + return true + } + return false +} + // RetrieveSkills returns the skills retrieval would attach to one leaf: those // whose scope or doc line cues against the leaf's own territory — its rendered // instruction, and the workspace it runs in. The shape is the chat catalog's diff --git a/internal/plan/contract_test.go b/internal/plan/contract_test.go index fb3dbe180..2ee910b2e 100644 --- a/internal/plan/contract_test.go +++ b/internal/plan/contract_test.go @@ -487,6 +487,28 @@ func TestPinnedSkillsMatchesNothingWhenNothingIsNamed(t *testing.T) { } } +func TestPinnedSkillsMatchesHyphenatedName(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/home/.codeaf/skills/repo-audit", Body: "audit repo structure and dependencies"}, + } + got := PinnedSkills("use repo-audit on this repo", skills) + if want := []string{"repo-audit"}; !reflect.DeepEqual(got, want) { + t.Fatalf("PinnedSkills = %q, want %q", got, want) + } +} + +func TestPinnedSkillsHyphenatedNoSpuriousSplit(t *testing.T) { + skills := []store.Fact{ + {Artifact: "/home/.codeaf/skills/flaky-test", Body: "find flaky tests in the suite"}, + } + // "flaky" and "test" both appear in the text, but not contiguously as + // "flaky-test" — the hyphenated name must NOT match. + got := PinnedSkills("the flaky integration test flaked again", skills) + if len(got) != 0 { + t.Fatalf("PinnedSkills = %q, want nothing (discontiguous tokens)", got) + } +} + func TestRetrieveSkillsScoresScopeAndSharedDocWords(t *testing.T) { skills := []store.Fact{ {Artifact: "/skills/parser", Scope: "repo:/work/parser", Body: "validate and format parser fixtures"}, diff --git a/internal/store/facts.go b/internal/store/facts.go index 45709b882..c4127b17e 100644 --- a/internal/store/facts.go +++ b/internal/store/facts.go @@ -582,7 +582,7 @@ func (s *Store) RewriteActiveSkillFrom(writer FactWriter, nodeID, scope, body st if len(sources) != 1 || strings.TrimSpace(sources[0].Artifact) == "" { return Fact{}, fmt.Errorf("rewrite active skill: %w: source %d is not active", ErrInvalid, sourceSeq) } - return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactActive, sources[0].Artifact, "", true) + return s.recordFact(writer, nodeID, scope, FactSkill, body, nil, 0, FactActive, sources[0].Artifact, sources[0].Trust, true) } func (s *Store) recordFact(writer FactWriter, nodeID, scope string, kind FactKind, body string, unsettled *UnsettledPair, replaces int64, status, artifact, trust string, deduplicate bool) (Fact, error) { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 812aad137..8ace46da9 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -695,6 +695,73 @@ func TestSkillFactAccessorsRecordsUseOnce(t *testing.T) { } } +// RewriteActiveSkillFrom propagates Trust from the source active skill. +func TestRewriteActiveSkillFromPreservesTrust(t *testing.T) { + graph := openTestStore(t, filepath.Join(t.TempDir(), "rewrite-trust.db")) + candidate, err := graph.RecordSkillCandidate("", "tool:scan", + "repo-audit scans the repo", "/workspace/repo-audit", "imported-provisional") + if err != nil { + t.Fatal(err) + } + if err := graph.ActivateSkill(candidate.Seq, "/installed/repo-audit", ""); err != nil { + t.Fatal(err) + } + active, err := graph.SkillFacts(FactActive, 10) + if err != nil || len(active) != 1 { + t.Fatalf("active skills = %+v err=%v", active, err) + } + if active[0].Trust != "imported-provisional" { + t.Fatalf("active trust before rewrite = %q, want 'imported-provisional'", active[0].Trust) + } + // Rewrite the doc. + rewritten, err := graph.RewriteActiveSkillFrom(FactWriterDistiller, "", active[0].Scope, + "repo-audit scans the repo (updated)", active[0].Seq) + if err != nil { + t.Fatal(err) + } + if rewritten.Trust != "imported-provisional" { + t.Fatalf("rewritten trust = %q, want 'imported-provisional'", rewritten.Trust) + } + // Verify via SkillFacts as well (after rewrite the old row is superseded). + all, err := graph.SkillFacts("", 10) + if err != nil { + t.Fatal(err) + } + // Find the rewritten row (it is the only one whose scope matches). + var found bool + for _, f := range all { + if f.Status == FactActive && f.Scope == active[0].Scope && f.Body == "repo-audit scans the repo (updated)" { + if f.Trust != "imported-provisional" { + t.Fatalf("rewritten active trust via SkillFacts = %q, want 'imported-provisional'", f.Trust) + } + found = true + } + } + if !found { + t.Fatal("rewritten active skill not found via SkillFacts") + } + // Survive rebuild. + if err := graph.Rebuild(); err != nil { + t.Fatal(err) + } + after, err := graph.SkillFacts(FactActive, 10) + if err != nil { + t.Fatalf("after rebuild: err=%v", err) + } + var foundAfter bool + for _, f := range after { + if f.Body == "repo-audit scans the repo (updated)" { + if f.Trust != "imported-provisional" { + t.Fatalf("after rebuild: rewritten trust = %q, want 'imported-provisional'", f.Trust) + } + foundAfter = true + } + } + if !foundAfter { + t.Fatal("rewritten active skill not found after rebuild") + } +} + // Trust defaults to "authored" when the stored value is empty. func TestTrustDefaultsToAuthored(t *testing.T) { graph := openTestStore(t, filepath.Join(t.TempDir(), "trust-default.db"))