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/cmd/codeaf/chat.go b/cmd/codeaf/chat.go index ec201ac6e..39e9109e3 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, 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 — +// 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 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. 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/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..375fc6e7f 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,42 @@ func (l *Linear) brief(task Task) string { return block.String() } +// 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 +// 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..df05c26cc 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/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/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/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 894dc2a94..84c7080a0 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. @@ -381,3 +383,258 @@ 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 +} + +// 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 + } + // Tokenize the goal into whole words so a skill named "lint" is never + // pinned by "splinter" or "test" by "latest". + 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 { + 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 +// 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 ba95e4324..2ee910b2e 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 { @@ -406,3 +408,191 @@ 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) + } +} + +// 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 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"}, + {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/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/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/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/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..6173482ab --- /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" + + "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 +// 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..ab32a54a6 --- /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 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") + + 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 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") { + 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..a9f1efadd --- /dev/null +++ b/internal/session/tools_skill.go @@ -0,0 +1,147 @@ +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, 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 +// 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 + } + 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/session/tools_skill_test.go b/internal/session/tools_skill_test.go new file mode 100644 index 000000000..de1e6d7dd --- /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) + } +} 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..c4127b17e 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" @@ -21,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 @@ -236,6 +241,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 +283,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 +312,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 +368,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 +498,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 +512,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 +538,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 +549,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 +582,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, sources[0].Trust, 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 +677,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 +706,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 +724,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) } @@ -725,6 +762,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) { @@ -740,6 +791,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 +1102,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 +1135,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 +1412,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 +1630,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 +1645,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 +1662,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 +1684,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 +1741,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 +1772,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 +2085,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 +2112,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 +2139,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 +2158,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/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 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..8ace46da9 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,194 @@ 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) + } +} + +// 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")) + 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)