From 0f83d43d381b7f25c3e4f0f732bd5760464beb9a Mon Sep 17 00:00:00 2001 From: Ivan Diatchenko <2518263+inhuman@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:45:14 +0300 Subject: [PATCH 1/3] =?UTF-8?q?path:=20=D0=BF=D1=83=D1=82=D1=8C=20=D0=BB?= =?UTF-8?q?=D1=8E=D0=B1=D0=BE=D0=B9=20=D0=B3=D0=BB=D1=83=D0=B1=D0=B8=D0=BD?= =?UTF-8?q?=D1=8B=20=D0=B8=20=D1=8D=D0=BB=D0=B5=D0=BC=D0=B5=D0=BD=D1=82=20?= =?UTF-8?q?=D1=81=D0=BF=D0=B8=D1=81=D0=BA=D0=B0,=20=D0=B8=20=D0=BF=D1=80?= =?UTF-8?q?=D0=BE=D0=BC=D0=B0=D1=85=20=D0=B3=D1=80=D0=BE=D0=BC=D0=BA=D0=B8?= =?UTF-8?q?=D0=B9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Подстановка читала один уровень, рядом стояло «наблюдаемые случаи плоские». Наблюдение было верным и перестало быть: ответ инструмента настолько глубокий, насколько его сделал инструмент. На 16 живых генерациях описания шага формат отверг шесть, и ПЯТЬ из шести — один случай: число на третьем уровне ответа kubectl. Список допустимых полей в промпте (4.3 КБ) долю отказов не сдвинул — не хватало формы, а не документации. Теперь `var.a.b.c` и `var.items[0].name` — и в подстановке, и слева от условия. Индекс пишется `[0]`, а не `.0`: так его пишут авторы, и так путь остаётся однозначным (`.0` — это ещё и поле с именем «0», законный ключ JSON). Форма ссылки теперь описана ОДИН раз (RefPattern) и вшита во все регулярки, которые её читают: подстановка, четыре формы условий, линтер. Раньше каждая писала её сама, и написания уже разошлись — `{{a.b}}` умело один уровень, условие принимало сколько угодно точек и точку в конце. Промах пути — ошибка шага, а не пустая строка: `a.b.c` при отсутствующем `b` неотличимо от «значение пустое», а по нему ветвятся. В отказе сказано, где путь оборвался и какие поля у объекта есть на самом деле. Граница проведена по тому, что старая грамматика умела выразить: голое `var` и одиночное `var.field` сохраняют молчание — на нём написаны чужие скиллы, лежащие в пользовательском хранилище, и апгрейд движка не имеет права их ронять. Молчащую половину сторожит W14. `[*]` отвергается отдельным сообщением, называющим цикл: автор не опечатался в синтаксисе, он попросил язык запросов, а нужен ему for_each. Побочно: lookup+objectOf заменены одним resolve, guard-тест резолвера переведён на него; expand/payload/callArgs возвращают ошибку — путь умеет не разрешиться. --- CHANGELOG.md | 54 +++++++++ README.md | 13 +++ README.ru.md | 12 ++ cond.go | 47 +++++--- expand.go | 175 +++++++++++----------------- expand_test.go | 40 +++++-- lint/rules_vars.go | 23 ++-- lint/rules_vars_test.go | 34 ++++++ path.go | 237 ++++++++++++++++++++++++++++++++++++++ path_test.go | 245 ++++++++++++++++++++++++++++++++++++++++ resolver_test.go | 8 +- skill.schema.ru.yaml | 10 +- skill.schema.yaml | 12 +- steps.go | 78 ++++++++++--- version.go | 2 +- 15 files changed, 831 insertions(+), 159 deletions(-) create mode 100644 path.go create mode 100644 path_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 22b4dd3..3b3a61e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,60 @@ wrap skills in something of your own — front matter, a markdown body, several documents in one file — unwrap before calling and wrap the result back; anything else is refused rather than guessed at. +## 2.4.0 + +A reference may now be a PATH. A skill that does not use one behaves exactly as +it did; a skill that uses one must declare `skill_engine_version: 2.4.0`. + +- **Added**: a path of any depth, and an index into a list — in a substitution + and on the left of a condition alike: + + ```yaml + instruction: "{{pod.metadata.name}} restarted {{pod.status.containerStatuses[0].restartCount}} times" + cond: "pod.status.containerStatuses[0].restartCount > 0" + ``` + + Substitution used to stop at one field, and the comment beside it said the + observed cases were flat. They were, and then they were not: a tool's answer + is as deep as the tool made it. Measured on sixteen live generations of a step + description, six were refused by the format, and FIVE of the six were the same + case — a number three levels inside a `kubectl get` answer. Not a model + failing to learn the format either: the same measurement with the list of + allowed fields in the prompt (4.3 KB of it) held the same share of refusals. + + An index is written `[0]`, not `.0`. That is what authors write, and it keeps + a path unambiguous: `.0` would be the field named "0" — a legal JSON key — and + an index at the same time. + + Still deliberately absent: `[*]`, filters, arithmetic, functions. That is + where a format turns into a query language. A condition written with `[*]` now + gets a refusal of its own, naming the loop that does what was asked. + +- **Added, and a behaviour change**: **a path that does not resolve is an + error.** The step fails, and the message says where the walk broke and what + the object did have (`` `pod.status` has no field `restarts` (it has: + containerStatuses, phase) ``). + + An unknown name still expands to an empty string in silence — a deliberate + decision, because a marker reaching the model reads as part of the + instruction. For a path that silence is worse: `a.b.c` with `b` missing is + indistinguishable from "the value is empty", and branches are taken on it. + Same call the numeric operands make. + + **The line is drawn at what the old grammar could express.** A bare `var` and + a single `var.field` keep their silence, because skills written under that + promise live in other people's storage and must not start failing on an + upgrade. Anything deeper, and any index, is new syntax that owes nothing to + the old contract. The silent half is watched statically by the linter's W14 — + the only place it can be watched at all. + +- **Changed (Go API)**: the substitution and resolution helpers now return an + error, since a path can fail: `expand`, `expandForArgs`, `expandArgs`, + `callArgs`, `payload` and `expandWhole` are internal, but `RefPattern` is new + and exported — the shape of a reference, so that a linter, an editor or a + visualiser finds one the way the engine does instead of spelling out a + narrower grammar and silently skipping what it cannot parse. + ## 2.3.0 Four new condition forms. A skill that does not use them behaves exactly as it diff --git a/README.md b/README.md index d85aba6..9929603 100644 --- a/README.md +++ b/README.md @@ -565,6 +565,19 @@ it is the stored value that flows on. It works on `call` steps too, except - `save_as` puts a step's result into a variable; **a step without `save_as` writes into `answer`** — that is where the application takes the turn's answer from. An empty `answer` = the program produced no answer. +- **A value inside a structured result is reached by a path**, in a + substitution and in a condition alike: `{{pod.metadata.name}}`, + `{{pod.status.containerStatuses[0].restartCount}}`, and the same written + without braces on the left of a condition. An index is `[0]`; `[*]` and + filters are refused, because a path resolves to ONE value and picking many is + what `for_each` is for. + + **A path that does not resolve is an error, not an empty string.** Silence + there is worse than useless: `a.b.c` with `b` missing is indistinguishable + from "the value is empty", and branches are taken on it. The refusal says + where the walk broke and what the object did have. A bare name and a single + `var.field` keep their old silence — that promise is what skills already + written were built on, and the linter's W14 is what watches it. - `.mem` — the working-memory handle of a result, ALWAYS, not only for large ones: `args: {stdin: {from: "{{tickets.mem}}"}}` sends the data past the model's context. It is read from the value's LAST line, where the host diff --git a/README.ru.md b/README.ru.md index dea11d7..eaad003 100644 --- a/README.ru.md +++ b/README.ru.md @@ -544,6 +544,18 @@ steps: - `save_as` кладёт результат шага в переменную; **шаг без `save_as` пишет в `answer`** — оттуда приложение берёт ответ хода. Пустой `answer` = программа ответа не дала. +- **До значения внутри структурного результата ведёт путь** — одинаково в + подстановке и в условии: `{{pod.metadata.name}}`, + `{{pod.status.containerStatuses[0].restartCount}}` и то же самое без скобок + слева от условия. Индекс пишется `[0]`; `[*]` и фильтры отвергаются: путь + разрешается в ОДНО значение, а выбирать многое — это `for_each`. + + **Путь, не разрешившийся до значения, — ошибка, а не пустая строка.** Молчание + здесь хуже, чем бесполезно: `a.b.c` при отсутствующем `b` неотличимо от + «значение пустое», а по нему ветвятся. В отказе сказано, где путь оборвался и + что у объекта было на самом деле. Голое имя и одиночное `var.field` сохраняют прежнее + молчание: на этом обещании написаны уже существующие скиллы, а сторожит его + правило линтера W14. - `<имя>.mem` — хендл рабочей памяти результата, ВСЕГДА, не только у крупных: `args: {stdin: {from: "{{tickets.mem}}"}}` шлёт данные мимо контекста модели. Читается в ПОСЛЕДНЕЙ строке значения, там, где его пишет хост: `[mem:…]`, diff --git a/cond.go b/cond.go index 0ac5a59..5ab98fd 100644 --- a/cond.go +++ b/cond.go @@ -13,7 +13,12 @@ import ( "unicode/utf8" ) -var condRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*(==|!=)\s*(.*?)\s*$`) +// The left operand of every condition is a REFERENCE — a name or a path into a +// value (`pod.status.containers[0].image`). Its shape is defined once, in +// path.go, and built into each of these: two spellings of one thing had already +// drifted apart here (substitution stopped at one field while a condition took +// any number of dots and a trailing one). +var condRe = regexp.MustCompile(`^\s*(` + RefPattern + `)\s*(==|!=)\s*(.*?)\s*$`) // emptyCondRe — the "step produced nothing" condition: `var is empty` / // `var is not empty`. @@ -24,7 +29,7 @@ var condRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*(==|!=)\s*(.*?) // they mean the same. This pattern showed up three times in a single live // skill, and a meaning repeated in three places belongs to the engine rather // than to the skill (same reasoning as ErrorPolicy). -var emptyCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s+is\s+(not\s+)?empty\s*$`) +var emptyCondRe = regexp.MustCompile(`^\s*(` + RefPattern + `)\s+is\s+(not\s+)?empty\s*$`) // containsCondRe — the "the text names one of these" condition: // `var contains a | b | c` / `var not contains a | b`. @@ -45,7 +50,7 @@ var emptyCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s+is\s+(not\ // The list may come out empty here on purpose: `input contains` with nothing // after it is recognised as this form so the error can say what is missing, // rather than falling through to "the condition does not parse". -var containsCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s+(not\s+)?contains\b\s*(.*?)\s*$`) +var containsCondRe = regexp.MustCompile(`^\s*(` + RefPattern + `)\s+(not\s+)?contains\b\s*(.*?)\s*$`) // numCondRe — the numeric comparisons: `var > 5`, `var >= req.limit`, // `var < 0.5`, `var <= days`. @@ -72,7 +77,7 @@ var containsCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s+(not\s+ // Deliberately NOT here: arithmetic (`a + b > c`, `len(x) > 0`). Those are // expressions, and expressions are the door to skills that cannot be read from // top to bottom. -var numCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*(>=|<=|>|<)\s*(\S.*?)\s*$`) +var numCondRe = regexp.MustCompile(`^\s*(` + RefPattern + `)\s*(>=|<=|>|<)\s*(\S.*?)\s*$`) // numberRe — what counts as a number on either side of a comparison. // @@ -82,10 +87,6 @@ var numCondRe = regexp.MustCompile(`^\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*(>=|<=|>|<)\ // the silence these conditions are written to avoid. var numberRe = regexp.MustCompile(`^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$`) -// nameRe — a variable name: the shape the left side of every condition takes, -// and the shape the right side of a comparison takes when it is not a literal. -var nameRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_.]*$`) - // bracedLeftRe / bracedNameRe — a condition written the way substitution is // written everywhere else: `{{pod.restartCount}} > 5`. // @@ -100,10 +101,18 @@ var nameRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_.]*$`) // them by exactly two pairs of braces; now the error names the braces and // prints the condition without them. var ( - bracedLeftRe = regexp.MustCompile(`^\s*\{\{\s*[a-zA-Z_][a-zA-Z0-9_.]*\s*\}\}`) - bracedNameRe = regexp.MustCompile(`\{\{\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*\}\}`) + bracedLeftRe = regexp.MustCompile(`^\s*\{\{\s*` + RefPattern + `\s*\}\}`) + bracedNameRe = regexp.MustCompile(`\{\{\s*(` + RefPattern + `)\s*\}\}`) ) +// selectorRe — the form a path deliberately does not have: `containers[*]`, and +// filters after it. It appeared in the same measurement as the paths themselves +// (`containerStatuses[*].restartCount > 0`), which is why it gets a refusal of +// its own instead of falling into the list of allowed shapes: the author did +// not mistype the syntax, they asked for something the format declines to be — +// a query language. What they want is a loop, and the message says so. +var selectorRe = regexp.MustCompile(`\[\s*[*?]`) + // isBlank — "the step produced nothing useful": empty or a failure marker. func isBlank(v string) bool { t := strings.TrimSpace(v) @@ -118,6 +127,11 @@ func isBlank(v string) bool { // with nothing to look for can never fire, and a branch that can never run is // not a branch, it is a hole the author cannot see. func parseCond(cond string) (name, op, want string, err error) { + if selectorRe.MatchString(cond) { + return "", "", "", fmt.Errorf("condition %q: `[*]` picks MANY elements and a condition compares one — "+ + "walk the list with `for_each` and put the condition in its body. "+ + "A single element is `list[0]`", cond) + } if bracedLeftRe.MatchString(cond) { bare := strings.TrimSpace(bracedNameRe.ReplaceAllString(cond, "$1")) // The suggestion is checked before it is offered: `{{a}} > пять` is two @@ -151,7 +165,7 @@ func parseCond(cond string) (name, op, want string, err error) { // Static, because it can never work: `count > пять` is wrong in the // file, not at the moment the branch is reached. Validate calls this // parser, so the skill is refused at load instead of mid-turn. - if _, ok := parseNumber(m[3]); !ok && !nameRe.MatchString(m[3]) { + if _, ok := parseNumber(m[3]); !ok && !refRe.MatchString(m[3]) { return "", "", "", fmt.Errorf("condition %q: the right side of `%s` must be a number or the name of "+ "a variable holding one, and %q is neither", cond, m[2], m[3]) } @@ -372,7 +386,10 @@ func (s *state) eval(cond string) (bool, error) { // A large result lives in a variable as a preview plus a handle, and a // condition reading the preview would answer about the first few hundred // bytes while looking exactly as if it had answered about the value. - got := s.payload(name) + got, err := s.payload(name) + if err != nil { + return false, err + } switch op { case "is empty": return isBlank(got), nil @@ -407,7 +424,11 @@ func (s *state) operand(cond, ref string) (number, error) { if n, ok := parseNumber(ref); ok { return n, nil } - got := strings.TrimSpace(s.payload(ref)) + raw, err := s.payload(ref) + if err != nil { + return number{}, err + } + got := strings.TrimSpace(raw) if isBlank(got) { return number{}, fmt.Errorf("condition %q: `%s` is empty (or a marked failure) — there is nothing to "+ "compare. An empty variable is NOT zero: answering the comparison would make "+ diff --git a/expand.go b/expand.go index 1c932eb..18e967d 100644 --- a/expand.go +++ b/expand.go @@ -3,17 +3,11 @@ package skillengine // {{var}} substitution and normalisation of step values. import ( - "encoding/json" "regexp" "strings" "unicode" ) -// varRe catches {{var}} and {{var.field}} — one level of nesting. -// -// Deeper is deliberately absent: the observed cases are flat, and nesting -// drags in indexes, filters and the rest of a template engine the format -// avoids. // assetRe catches {{asset:name}} — substituting a payload's content into the // TEXT of an instruction. // @@ -26,14 +20,23 @@ import ( // way, into a tool argument past the model. var assetRe = regexp.MustCompile(`\{\{\s*asset:([a-zA-Z_][a-zA-Z0-9_-]*)\s*\}\}`) -var varRe = regexp.MustCompile(`\{\{\s*([a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*)?)\s*\}\}`) +// varRe catches a reference in braces: {{var}}, {{var.field}}, {{var.a.b}}, +// {{var.items[0].name}}. The shape of the reference itself lives in one place +// (RefPattern) — see path.go. +var varRe = regexp.MustCompile(`\{\{\s*(` + RefPattern + `)\s*\}\}`) // expand substitutes {{var}}. An unknown variable becomes an empty string // rather than staying as text: a marker that reaches the model reads to it as // part of the instruction and produces questions about "the variable var". -func (s *state) expand(text string) string { - out, _ := s.expandWith(text, func(name string) (string, bool) { return s.lookup(name), true }) - return out +// +// A PATH that does not resolve is an error instead — see resolve: the silence +// is a promise to the shapes that existed before paths did, not to the paths. +func (s *state) expand(text string) (string, error) { + out, _, err := s.expandWith(text, func(name string) (string, bool, error) { + v, err := s.resolve(name) + return v, true, err + }) + return out, err } // expandWhole — substitution for a step that CANNOT fetch the rest of a value. @@ -61,38 +64,48 @@ func (s *state) expand(text string) string { // Returns the name of the first variable that could NOT be made whole — the // value carries a handle and there is no reader for it, or the reader no longer // has it. The step still runs, on the fragment, and the caller is told. -func (s *state) expandWhole(text string) (string, string) { - return s.expandWith(text, func(name string) (string, bool) { - raw := s.lookup(name) +func (s *state) expandWhole(text string) (string, string, error) { + return s.expandWith(text, func(name string) (string, bool, error) { + raw, err := s.resolve(name) + if err != nil { + return "", true, err + } id := memHandle(raw) if id == "" { - return trimHostNote(raw, s.vocab.TruncationNotes), true + return trimHostNote(raw, s.vocab.TruncationNotes), true, nil } if s.memory != nil { if full, ok := s.memory.Get(id); ok { - return trimHostNote(full, s.vocab.TruncationNotes), true + return trimHostNote(full, s.vocab.TruncationNotes), true, nil } } // The note goes even so: an instruction to make a call this step cannot // make is worse than a fragment, and the fragment is reported. - return trimHostNote(raw, s.vocab.TruncationNotes), false + return trimHostNote(raw, s.vocab.TruncationNotes), false, nil }) } -func (s *state) expandWith(text string, value func(name string) (string, bool)) (string, string) { +func (s *state) expandWith(text string, value func(name string) (string, bool, error)) (string, string, error) { text = assetRe.ReplaceAllStringFunc(text, func(m string) string { return s.asset(assetRe.FindStringSubmatch(m)[1]) }) unresolved := "" + var failed error out := varRe.ReplaceAllStringFunc(text, func(m string) string { name := varRe.FindStringSubmatch(m)[1] - v, ok := value(name) + v, ok, err := value(name) + // The FIRST failure is the one reported: a text with three broken paths + // is one mistake made three times, and the walk cannot be stopped + // halfway through a replacement anyway. + if err != nil && failed == nil { + failed = err + } if !ok && unresolved == "" { unresolved = name } return v }) - return out, unresolved + return out, unresolved, failed } // expandForArgs — substitution into call ARGUMENTS, not into an instruction. @@ -111,13 +124,12 @@ func (s *state) expandWith(text string, value func(name string) (string, bool)) // field access and for_each. // // So values go into arguments WHOLE and WITHOUT the host's note. -func (s *state) expandForArgs(text string) string { - text = assetRe.ReplaceAllStringFunc(text, func(m string) string { - return s.asset(assetRe.FindStringSubmatch(m)[1]) - }) - return varRe.ReplaceAllStringFunc(text, func(m string) string { - return s.payload(varRe.FindStringSubmatch(m)[1]) +func (s *state) expandForArgs(text string) (string, error) { + out, _, err := s.expandWith(text, func(name string) (string, bool, error) { + v, err := s.payload(name) + return v, true, err }) + return out, err } // payload — a variable as a consumer that is NOT the model must see it: whole, @@ -135,8 +147,12 @@ func (s *state) expandForArgs(text string) string { // So there is exactly one way to ask for the data form, and any new consumer // has to say which of the two it wants — `expand` for the model, this for // everyone else. A guard test keeps it that way. -func (s *state) payload(name string) string { - return trimHostNote(s.fullValue(s.lookup(name)), s.vocab.TruncationNotes) +func (s *state) payload(name string) (string, error) { + v, err := s.resolve(name) + if err != nil { + return "", err + } + return trimHostNote(s.fullValue(v), s.vocab.TruncationNotes), nil } // asset returns a payload's content, fetching it on first use. @@ -169,72 +185,6 @@ func (s *state) asset(name string) string { return v } -// lookup returns the value of a variable or of its field. -// -// A field is looked up in the JSON object a step stored in the variable (a -// structured answer). A missing one yields an empty string, same as a missing -// variable: a marker that reached the model would read as part of the -// instruction. -func (s *state) lookup(name string) string { - if v, ok := s.vars[name]; ok { - return v - } - base, field, hasField := strings.Cut(name, ".") - if !hasField { - return "" - } - raw, ok := s.vars[base] - if !ok { - return "" - } - obj, ok := s.objectOf(raw) - if !ok { - return "" - } - v, ok := obj[field] - if !ok { - return "" - } - if str, isStr := v.(string); isStr { - return str - } - out, err := json.Marshal(v) - if err != nil { - return "" - } - return string(out) -} - -// objectOf parses a variable's value as a JSON object. -// -// A variable's value is what the host WOULD show the model, not the raw tool -// output: a working-memory handle ("[mem:id]") is always appended, and a large -// one is truncated to a preview on top of that. Both break parsing, which is -// why the field silently went empty for ANY `call:` result — {{var.field}} -// substitution never worked on such variables. -// -// Order: strip the host's note and try; if that failed (truncated), take the -// whole thing from working memory by the handle — that is what it is appended -// for. -func (s *state) objectOf(raw string) (map[string]any, bool) { - var obj map[string]any - if err := json.Unmarshal([]byte(trimHostNote(raw, s.vocab.TruncationNotes)), &obj); err == nil { - return obj, true - } - id := memHandle(raw) - if id == "" || s.memory == nil { - return nil, false - } - full, ok := s.memory.Get(id) - if !ok { - return nil, false - } - if err := json.Unmarshal([]byte(full), &obj); err != nil { - return nil, false - } - return obj, true -} - // fullValue returns a variable's value IN FULL. // // A variable holds what the host would show the model: a large result is @@ -285,15 +235,19 @@ func trimHostNote(s string, notes []string) string { // expandArgs substitutes {{var}} into STRING argument values, walking nested // structures. Numbers, flags and object shape stay as written: substitution is // about values, not about the call's schema. -func (s *state) expandArgs(args map[string]any) map[string]any { +func (s *state) expandArgs(args map[string]any) (map[string]any, error) { if len(args) == 0 { - return nil + return nil, nil } out := make(map[string]any, len(args)) for k, v := range args { - out[k] = s.expandAny(v) + e, err := s.expandAny(v) + if err != nil { + return nil, err + } + out[k] = e } - return out + return out, nil } // callArgs prepares the arguments of a `call:` step: value substitution plus @@ -310,20 +264,23 @@ func (s *state) expandArgs(args map[string]any) map[string]any { // An explicit `_deliver` in the step's arguments wins: it is written for that // specific call, whereas the asset's declaration is a default for all of its // consumers. -func (s *state) callArgs(args map[string]any) map[string]any { - out := s.expandArgs(args) +func (s *state) callArgs(args map[string]any) (map[string]any, error) { + out, err := s.expandArgs(args) + if err != nil { + return nil, err + } if _, explicit := out["_deliver"]; explicit { - return out + return out, nil } to := s.assetDeliver(args) if to == "" { - return out + return out, nil } if out == nil { out = make(map[string]any, 1) } out["_deliver"] = map[string]any{"to": to} - return out + return out, nil } // assetDeliver returns the delivery route declared by the first asset that @@ -346,7 +303,7 @@ func (s *state) assetDeliver(args map[string]any) string { return "" } -func (s *state) expandAny(v any) any { +func (s *state) expandAny(v any) (any, error) { switch t := v.(type) { case string: return s.expandForArgs(t) @@ -357,18 +314,22 @@ func (s *state) expandAny(v any) any { // exactly what assets exist for. if from, ok := t["from"].(string); ok && len(t) == 1 { if name, isAsset := strings.CutPrefix(from, "asset:"); isAsset { - return s.asset(name) + return s.asset(name), nil } } return s.expandArgs(t) case []any: out := make([]any, len(t)) for i, e := range t { - out[i] = s.expandAny(e) + x, err := s.expandAny(e) + if err != nil { + return nil, err + } + out[i] = x } - return out + return out, nil default: - return v + return v, nil } } diff --git a/expand_test.go b/expand_test.go index a992cdd..75c9a99 100644 --- a/expand_test.go +++ b/expand_test.go @@ -132,8 +132,8 @@ func TestFieldLookupIgnoresHostMemNote(t *testing.T) { s := &state{vars: map[string]string{ "ctx": `{"head_sha":"abc123","delta_scope":"go"}` + "\n[mem:res-1]", }} - assert.Equal(t, "abc123", s.lookup("ctx.head_sha")) - assert.Equal(t, "go", s.lookup("ctx.delta_scope")) + assert.Equal(t, "abc123", resolved(t, s, "ctx.head_sha")) + assert.Equal(t, "go", resolved(t, s, "ctx.delta_scope")) } // In a truncated preview the JSON is incomplete and will never parse. The whole @@ -149,14 +149,15 @@ func TestFieldLookupFallsBackToWorkingMemory(t *testing.T) { vars: map[string]string{"ctx": `{"head_sha":"dead` + "…\n[mem:res-9 — this is a PREVIEW, 42kb in total]"}, memory: fakeMemory{"res-9": full}, } - assert.Equal(t, "deadbeef", s.lookup("ctx.head_sha")) - assert.Equal(t, "^(a|b)$", s.lookup("ctx.delta_regex")) + assert.Equal(t, "deadbeef", resolved(t, s, "ctx.head_sha")) + assert.Equal(t, "^(a|b)$", resolved(t, s, "ctx.delta_regex")) } // No memory — the field is empty, but that does not bring the turn down. func TestFieldLookupWithoutMemoryStaysEmpty(t *testing.T) { s := &state{vars: map[string]string{"ctx": `{"head_sha":"dead` + "…\n[mem:res-9]"}} - assert.Equal(t, "", s.lookup("ctx.head_sha")) + assert.Equal(t, "", resolved(t, s, "ctx.head_sha"), + "one level deep keeps the silence it was promised") } // Into ARGUMENTS a value goes whole and without the host's note: there it is @@ -170,7 +171,7 @@ func TestArgsGetCleanFullValue(t *testing.T) { vars: map[string]string{"findings": `{"a":1` + "…\n[mem:res-3 — this is a PREVIEW, 42kb in total]"}, memory: fakeMemory{"res-3": `{"a":1,"b":2}`}, } - args := s.expandArgs(map[string]any{"stdin": "{{findings}}"}) + args := expandedArgs(t, s, map[string]any{"stdin": "{{findings}}"}) assert.Equal(t, `{"a":1,"b":2}`, args["stdin"], "whole, from memory, without the note") } @@ -183,7 +184,7 @@ func TestArgsTrimDeclaredHostNote(t *testing.T) { vars: map[string]string{"findings": `{"a":1}` + "\n" + note}, vocab: Vocabulary{TruncationNotes: []string{"обрезано:", "gekürzt:", "shortened:"}}, } - args := s.expandArgs(map[string]any{"stdin": "{{findings}}"}) + args := expandedArgs(t, s, map[string]any{"stdin": "{{findings}}"}) assert.Equal(t, `{"a":1}`, args["stdin"], note) } } @@ -194,7 +195,7 @@ func TestArgsTrimDeclaredHostNote(t *testing.T) { // host whose results legitimately end in a bracketed line. func TestArgsKeepAnUndeclaredNote(t *testing.T) { s := &state{vars: map[string]string{"findings": `{"a":1}` + "\n[gekürzt: 42kb]"}} - args := s.expandArgs(map[string]any{"stdin": "{{findings}}"}) + args := expandedArgs(t, s, map[string]any{"stdin": "{{findings}}"}) assert.Equal(t, `{"a":1}`+"\n[gekürzt: 42kb]", args["stdin"]) } @@ -202,7 +203,7 @@ func TestArgsKeepAnUndeclaredNote(t *testing.T) { // resolves it, so it can recognise it. func TestArgsTrimTheFormatsOwnHandle(t *testing.T) { s := &state{vars: map[string]string{"findings": `{"a":1}` + "\n[mem:res-3]"}} - args := s.expandArgs(map[string]any{"stdin": "{{findings}}"}) + args := expandedArgs(t, s, map[string]any{"stdin": "{{findings}}"}) assert.Equal(t, `{"a":1}`, args["stdin"]) } @@ -210,5 +211,24 @@ func TestArgsTrimTheFormatsOwnHandle(t *testing.T) { // truncated and how to read the rest. func TestInstructionKeepsMemHandle(t *testing.T) { s := &state{vars: map[string]string{"ctx": "data\n[mem:res-9]"}} - assert.Contains(t, s.expand("here is {{ctx}}"), "[mem:res-9]") + out, err := s.expand("here is {{ctx}}") + require.NoError(t, err) + assert.Contains(t, out, "[mem:res-9]") +} + +// resolved — a reference the test expects to resolve. A path that broke is an +// error, and a test that swallowed it would be asserting about the empty string +// the failure left behind. +func resolved(t *testing.T, s *state, ref string) string { + t.Helper() + v, err := s.resolve(ref) + require.NoError(t, err) + return v +} + +func expandedArgs(t *testing.T, s *state, args map[string]any) map[string]any { + t.Helper() + out, err := s.expandArgs(args) + require.NoError(t, err) + return out } diff --git a/lint/rules_vars.go b/lint/rules_vars.go index 4bdb914..b5bc458 100644 --- a/lint/rules_vars.go +++ b/lint/rules_vars.go @@ -20,9 +20,14 @@ import ( // search through the file finds it, and suspicion falls on anything except the // order of the steps. -// varRefRE — a `{{name}}` reference. Dots are allowed inside (a field of an -// object) along with the engine's suffixes; the engine trims the spaces itself. -var varRefRE = regexp.MustCompile(`\{\{\s*([a-zA-Z_][a-zA-Z0-9_.]*)\s*\}\}`) +// varRefRE — a `{{name}}` reference: a name, a path into its value +// (`ctx.status.pods[0].name`), or the engine's suffixes. The engine trims the +// spaces itself. +// +// The shape is taken from the engine rather than spelled out again: a reference +// this rule cannot parse is a reference it silently does not check, and the +// half of the format it stopped seeing would be the newest half. +var varRefRE = regexp.MustCompile(`\{\{\s*(` + skillengine.RefPattern + `)\s*\}\}`) func (r *run) workflowVarRefs(flow *skillengine.Flow) { known := set(r.opts.HostVars) @@ -265,15 +270,19 @@ func producedBy(s *skillengine.Step) []string { // knownVarBase checks a reference and returns the base name when it is unknown. // -// `x.field` is legitimate when `x` exists: the value is parsed as an object and -// the field is taken from it. The engine's suffixes (the memory handle, the -// skipped marker) are references to the base name too. +// `x.field` and `x.a.b[0]` are legitimate when `x` exists: the value is parsed +// as JSON and the path walked into it. The engine's suffixes (the memory +// handle, the skipped marker) are references to the base name too. +// +// Where the path itself leads is not this rule's business — that is runtime +// shape, and the engine refuses a path that does not resolve. What a linter can +// see is whether the variable it starts from exists at all. func knownVarBase(ref string, scope map[string]bool) (string, bool) { if scope[ref] { return "", true } base := ref - if i := strings.Index(ref, "."); i > 0 { + if i := strings.IndexAny(ref, ".["); i > 0 { base = ref[:i] } if scope[base] { diff --git a/lint/rules_vars_test.go b/lint/rules_vars_test.go index 2190645..4868c0f 100644 --- a/lint/rules_vars_test.go +++ b/lint/rules_vars_test.go @@ -63,6 +63,40 @@ func TestW14_ForwardReference(t *testing.T) { requireFinding(t, rep, "W14", lint.SeverityError) } +// A reference may be a PATH into a value, and the rule reads it the way the +// engine does: the variable it starts from is what a linter can check, while +// where the path leads is runtime shape. Reading it with a narrower grammar is +// worse than not reading it at all — the reference goes unchecked in silence, +// and the unchecked half is the newest half of the format. +func TestW14_PathsAreReadAsFarAsTheirVariable(t *testing.T) { + rep := lintSkill(t, wf(` tools: ["docs"] + steps: + - name: fetch + instruction: fetch the pod + tools: [] + save_as: pod + - name: report + when: "pod.status.containerStatuses[0].restartCount > 0" + instruction: "{{pod.metadata.name}} in {{pod.status.containerStatuses[0].name}}" + tools: [] +`)) + requireQuiet(t, rep, "W14") + + // The same paths, one letter wrong in the variable they start from. + rep = lintSkill(t, wf(` tools: ["docs"] + steps: + - name: fetch + instruction: fetch the pod + tools: [] + save_as: pod + - name: report + instruction: "{{pood.status.containerStatuses[0].name}}" + tools: [] +`)) + f := requireFinding(t, rep, "W14", lint.SeverityError) + assert.Contains(t, f.Message, "pood") +} + // A numeric comparison names a variable on BOTH sides, and a typo in the // THRESHOLD is the same silence as a typo in the value: the condition looks // right, and the branch it guards never fires the way it reads. diff --git a/path.go b/path.go new file mode 100644 index 0000000..cc82854 --- /dev/null +++ b/path.go @@ -0,0 +1,237 @@ +package skillengine + +// References to a value: `var`, `var.field`, `var.a.b.c`, `var.items[0].name`. + +import ( + "encoding/json" + "fmt" + "regexp" + "slices" + "strconv" + "strings" +) + +// RefPattern — the shape of a reference: a name, then any number of `.field` +// steps and `[0]` indexes. +// +// Exported for the same reason as CondVars: whoever reads a description — a +// linter, an editor, a visualiser — must find a reference the way the engine +// finds one. A reference a reader cannot parse is a reference it silently does +// not check, and the half of the format it stops seeing is always the newest. +// +// ONE pattern, built into every regex that reads a reference — substitution, +// the branch conditions, the linter. They used to spell it out apiece, and the +// spellings had already drifted (`{{a.b}}` was one level deep while a condition +// accepted any number of dots and a trailing one). A reference is one thing. +// +// Depth used to stop at one field, and the comment beside it said the observed +// cases were flat. That was true and stopped being true: a tool's answer is as +// deep as the tool made it. Measured on sixteen live generations of a step +// description, six were refused by the format and FIVE of the six were one +// case — a number lying three levels inside a `kubectl get` answer, written the +// way such paths are written everywhere: +// +// pod_details.status.containerStatuses[0].restartCount > 0 +// +// Not a model failing to learn the format either: the same measurement with the +// list of allowed fields in the prompt (4.3 KB of it) held the same share of +// refusals. The format was short of a form, not the author short of the docs. +// +// An INDEX is written `[0]` rather than `.0`, and both halves of that matter. +// `[0]` is what authors write, and refusing the notation everyone uses would +// keep producing the refusals this exists to remove. It also keeps a path +// unambiguous: `.0` would be the field named "0" — a legal JSON key — and an +// index at the same time, so a reader could not tell which was meant. +// +// Deliberately NOT here: `[*]`, filters, arithmetic, functions. That is where a +// format turns into a template language and drags in precedence, escaping and +// runtime errors. A path either resolves to one value or it does not. +const RefPattern = `[a-zA-Z_][a-zA-Z0-9_]*(?:\.[a-zA-Z_][a-zA-Z0-9_]*|\[[0-9]+\])*` + +var refRe = regexp.MustCompile(`^` + RefPattern + `$`) + +// refStepRe splits a reference into its steps: `.field` or `[0]`. +var refStepRe = regexp.MustCompile(`\.([a-zA-Z_][a-zA-Z0-9_]*)|\[([0-9]+)\]`) + +// pathStep — one step of a walk: a field of an object, or an index of a list. +type pathStep struct { + field string + index int + byIdx bool +} + +func (p pathStep) String() string { + if p.byIdx { + return "[" + strconv.Itoa(p.index) + "]" + } + return "." + p.field +} + +// splitRef splits a reference into the variable it starts from and the steps +// that walk into its value. +func splitRef(ref string) (base string, steps []pathStep, ok bool) { + if !refRe.MatchString(ref) { + return "", nil, false + } + base = ref + if i := strings.IndexAny(ref, ".["); i >= 0 { + base = ref[:i] + } + for _, m := range refStepRe.FindAllStringSubmatch(ref[len(base):], -1) { + if m[1] != "" { + steps = append(steps, pathStep{field: m[1]}) + continue + } + // The pattern has already restricted this to digits; a number too long + // for an int is out of range for any list anyway. + n, err := strconv.Atoi(m[2]) + if err != nil { + return "", nil, false + } + steps = append(steps, pathStep{index: n, byIdx: true}) + } + return base, steps, true +} + +// resolve returns the value a reference points at. +// +// WHY A MISS IS AN ERROR, and why only for part of the grammar. An unknown name +// expands to an empty string in silence — a deliberate decision, because a +// marker reaching the model reads to it as part of the instruction. For a path +// that silence is worse than useless: `a.b.c` with `b` missing is +// indistinguishable from "the value is empty", and branches are taken on it. +// +// So a path that does not resolve is an error — the same call the numeric +// operands make ("both ways of not being a number are LOUD"). But ONLY for what +// the old grammar could not express: `var` and `var.field` keep their silence, +// because skills written under that promise are in other people's storage and +// must not start failing on an engine upgrade. Anything deeper, and any index, +// is new syntax that owes nothing to the old contract. +// +// The silent half is not left unwatched: the linter's W14 finds an unknown name +// statically, which is the only place it can be found at all. +func (s *state) resolve(ref string) (string, error) { + // An exact name wins over a walk: the engine makes variables whose names + // contain a dot itself (`pods.mem`, `findings.skipped`), and they are names, + // not paths into a value. + if v, ok := s.vars[ref]; ok { + return v, nil + } + base, steps, ok := splitRef(ref) + if !ok || len(steps) == 0 { + return "", nil + } + // `var.field` is the shape that existed before paths did, so a miss inside + // it stays as quiet as it has always been. + quiet := len(steps) == 1 && !steps[0].byIdx + + miss := func(format string, args ...any) (string, error) { + if quiet { + return "", nil + } + return "", fmt.Errorf("reference `%s`: %s", ref, fmt.Sprintf(format, args...)) + } + + raw, ok := s.vars[base] + if !ok { + return miss("there is no variable `%s`", base) + } + cur, ok := s.valueOf(raw) + if !ok { + return miss("`%s` is not JSON, and a path can only be read into a structured value — it holds %q", + base, clipValue(raw)) + } + at := base + for _, st := range steps { + switch { + case st.byIdx: + list, isList := cur.([]any) + if !isList { + return miss("`%s` is %s, not a list", at, kindOf(cur)) + } + if st.index >= len(list) { + return miss("the list `%s` has %d element(s), and there is no [%d]", at, len(list), st.index) + } + cur = list[st.index] + default: + obj, isObj := cur.(map[string]any) + if !isObj { + return miss("`%s` is %s, not an object", at, kindOf(cur)) + } + v, has := obj[st.field] + if !has { + return miss("`%s` has no field `%s` (it has: %s)", at, st.field, fieldNames(obj)) + } + cur = v + } + at += st.String() + } + return itemText(cur), nil +} + +// valueOf parses a variable's value as JSON — an object or a list. +// +// A variable's value is what the host WOULD show the model, not the raw tool +// output: a working-memory handle ("[mem:id]") is always appended, and a large +// one is truncated to a preview on top of that. Both break parsing, which is +// why the field silently went empty for ANY `call:` result — {{var.field}} +// substitution never worked on such variables. +// +// Order: strip the host's note and try; if that failed (truncated), take the +// whole thing from working memory by the handle — that is what it is appended +// for. +func (s *state) valueOf(raw string) (any, bool) { + var v any + if err := json.Unmarshal([]byte(trimHostNote(raw, s.vocab.TruncationNotes)), &v); err == nil { + return v, true + } + id := memHandle(raw) + if id == "" || s.memory == nil { + return nil, false + } + full, ok := s.memory.Get(id) + if !ok { + return nil, false + } + if err := json.Unmarshal([]byte(full), &v); err != nil { + return nil, false + } + return v, true +} + +// kindOf names what a value turned out to be, so that a refusal says why the +// step could not be taken rather than only that it could not. +func kindOf(v any) string { + switch v.(type) { + case map[string]any: + return "an object" + case []any: + return "a list" + case string: + return "a string" + case float64: + return "a number" + case bool: + return "a boolean" + case nil: + return "null" + } + return "not a structure" +} + +// fieldNames lists what the object DOES have — the half of a refusal that turns +// it into a fix. Sorted, because a map's order would make the same failure read +// differently on every run, and clipped, because a tool's answer can carry +// dozens of keys and the sentence saying what is wrong must survive them. +func fieldNames(obj map[string]any) string { + names := make([]string, 0, len(obj)) + for k := range obj { + names = append(names, k) + } + slices.Sort(names) + const max = 12 + if len(names) > max { + return strings.Join(names[:max], ", ") + fmt.Sprintf(", … (%d more)", len(names)-max) + } + return strings.Join(names, ", ") +} diff --git a/path_test.go b/path_test.go new file mode 100644 index 0000000..6a1a44e --- /dev/null +++ b/path_test.go @@ -0,0 +1,245 @@ +package skillengine + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The answer of a real tool, in the shape a real tool returns it. The measured +// failure was a number three levels down in exactly this, and the author of the +// step wrote the path to it the way such paths are written everywhere. +const podDetails = `{ + "metadata": {"name": "api-7f9", "namespace": "prod"}, + "status": { + "phase": "Running", + "containerStatuses": [ + {"name": "api", "restartCount": 12, "ready": false}, + {"name": "sidecar", "restartCount": 0, "ready": true} + ] + } +}` + +func TestSplitRef(t *testing.T) { + for _, c := range []struct { + ref string + base string + steps []pathStep + }{ + {"pods", "pods", nil}, + {"pods.name", "pods", []pathStep{{field: "name"}}}, + {"a.b.c", "a", []pathStep{{field: "b"}, {field: "c"}}}, + {"pods[0]", "pods", []pathStep{{index: 0, byIdx: true}}}, + {"a.items[2].name", "a", []pathStep{{field: "items"}, {index: 2, byIdx: true}, {field: "name"}}}, + } { + t.Run(c.ref, func(t *testing.T) { + base, steps, ok := splitRef(c.ref) + require.True(t, ok) + assert.Equal(t, c.base, base) + assert.Equal(t, c.steps, steps) + }) + } + + for _, bad := range []string{"a.[0]", "a..b", "a.b.", "1a", "a[*]", "a[-1]", "a-b"} { + _, _, ok := splitRef(bad) + assert.Falsef(t, ok, "%q was accepted as a reference", bad) + } +} + +// The live case end to end: a number three levels inside a tool's answer, +// substituted into an instruction. +func TestDeepPathReachesTheInstruction(t *testing.T) { + r := &fakeRunner{} + f := parseFlow(t, ` +steps: + - name: report + instruction: "{{pod.metadata.name}} restarted {{pod.status.containerStatuses[0].restartCount}} times" + tools: [] +`) + _, _, err := ExecuteWith(context.Background(), f, Deps{Runner: r}, + map[string]string{"pod": podDetails}) + require.NoError(t, err) + assert.Equal(t, "api-7f9 restarted 12 times", r.seen[0].Instruction) +} + +// The same path in a condition — the form five of six refusals took. +func TestDeepPathInACondition(t *testing.T) { + f := parseFlow(t, ` +steps: + - if: + cond: "pod.status.containerStatuses[0].restartCount > 0" + then: + - set: {var: verdict, value: "restarting"} + else: + - set: {var: verdict, value: "calm"} + - name: second + when: "pod.status.containerStatuses[1].ready == true" + set: {var: sidecar, value: "ok"} +`) + require.NoError(t, f.Validate()) + vars, _, err := ExecuteWith(context.Background(), f, Deps{}, map[string]string{"pod": podDetails}) + require.NoError(t, err) + assert.Equal(t, "restarting", vars["verdict"]) + assert.Equal(t, "ok", vars["sidecar"], "a boolean compares as the text it is written in") +} + +// A value that is not a string comes back as the JSON it is: an object, a list, +// a number. +func TestPathYieldsJSONForStructures(t *testing.T) { + s := &state{vars: map[string]string{"pod": podDetails}} + assert.Equal(t, "prod", resolved(t, s, "pod.metadata.namespace")) + assert.Equal(t, "12", resolved(t, s, "pod.status.containerStatuses[0].restartCount")) + assert.Equal(t, `{"name":"api-7f9","namespace":"prod"}`, resolved(t, s, "pod.metadata")) + assert.Contains(t, resolved(t, s, "pod.status.containerStatuses"), `"sidecar"`) +} + +// A path that does not resolve is an ERROR, not an empty string. Silence here is +// worse than useless: `a.b.c` with `b` missing is indistinguishable from "the +// value is empty", and branches are taken on it. +func TestABrokenPathStopsTheTurn(t *testing.T) { + f := parseFlow(t, ` +steps: + - name: report + instruction: "{{pod.status.restarts}} times" + tools: [] +`) + _, _, err := ExecuteWith(context.Background(), f, Deps{Runner: &fakeRunner{}}, + map[string]string{"pod": podDetails}) + require.Error(t, err) + assert.Contains(t, err.Error(), "`pod.status` has no field `restarts`") + assert.Contains(t, err.Error(), "containerStatuses", "the refusal must list what the object DOES have") +} + +// Where exactly the walk broke, and why — the half of a refusal that turns it +// into a fix. +func TestTheRefusalSaysWhereTheWalkBroke(t *testing.T) { + s := &state{vars: map[string]string{"pod": podDetails, "plain": "just text"}} + for _, c := range []struct{ ref, want string }{ + {"pod.status.containerStatuses[9].name", "has 2 element(s), and there is no [9]"}, + {"pod.metadata[0].name", "`pod.metadata` is an object, not a list"}, + {"pod.metadata.name.x", "`pod.metadata.name` is a string, not an object"}, + {"pod.status.phase.deeper", "is a string, not an object"}, + {"plain.a.b", "`plain` is not JSON"}, + {"nowhere.a.b", "there is no variable `nowhere`"}, + } { + t.Run(c.ref, func(t *testing.T) { + _, err := s.resolve(c.ref) + require.Error(t, err) + assert.Contains(t, err.Error(), c.want) + assert.Contains(t, err.Error(), c.ref, "the refusal must quote the reference as written") + }) + } +} + +// The silence one level deep is a PROMISE, not an oversight: skills written +// under it live in other people's storage and must not start failing on an +// engine upgrade. Anything deeper is new syntax and owes it nothing. +func TestOneLevelKeepsItsSilence(t *testing.T) { + s := &state{vars: map[string]string{"pod": podDetails, "plain": "just text"}} + for _, ref := range []string{"pod.nope", "plain.nope", "nowhere", "nowhere.nope"} { + v, err := s.resolve(ref) + require.NoErrorf(t, err, "%q became loud, and skills already written depend on it not being", ref) + assert.Empty(t, v) + } +} + +// An exact name wins over a walk: the engine makes variables whose names contain +// a dot itself, and they are names, not paths. +func TestAnExactNameBeatsAPath(t *testing.T) { + s := &state{vars: map[string]string{ + "pods": `{"mem": "not the handle"}`, + "pods" + MemSuffix: "res-7", + "found": `{"skipped": "not this either"}`, + "found" + SkippedSuffix: "in_docs", + }} + assert.Equal(t, "res-7", resolved(t, s, "pods.mem")) + assert.Equal(t, "in_docs", resolved(t, s, "found.skipped")) +} + +// A large result lives in a variable as a preview plus a handle, and the preview +// is truncated JSON that will never parse. A path reads the whole thing from +// working memory — the same fallback one-level access has always had. +func TestAPathReadsThroughWorkingMemory(t *testing.T) { + s := &state{ + vars: map[string]string{"pod": `{"status": {"contai` + "…\n[mem:res-9 — this is a PREVIEW, 42kb in total]"}, + memory: fakeMemory{"res-9": podDetails}, + } + assert.Equal(t, "12", resolved(t, s, "pod.status.containerStatuses[0].restartCount")) +} + +// Into ARGUMENTS a path goes the same way, and a broken one fails the step +// rather than handing a tool an empty argument. +func TestPathsInCallArguments(t *testing.T) { + c := &recordingCaller{out: "done"} + f := parseFlow(t, ` +tools: ["k8s"] +steps: + - call: + tool: k8s:restart + args: {name: "{{pod.metadata.name}}", ns: "{{pod.metadata.namespace}}"} + save_as: out +`) + _, _, err := ExecuteWith(context.Background(), f, Deps{Caller: c}, map[string]string{"pod": podDetails}) + require.NoError(t, err) + assert.Equal(t, map[string]any{"name": "api-7f9", "ns": "prod"}, c.args) + + broken := parseFlow(t, ` +tools: ["k8s"] +steps: + - call: + tool: k8s:restart + args: {name: "{{pod.metadata.nome}}"} + save_as: out +`) + _, _, err = ExecuteWith(context.Background(), broken, Deps{Caller: c}, map[string]string{"pod": podDetails}) + require.Error(t, err) + assert.Contains(t, err.Error(), "has no field `nome`") +} + +// A loop reads its collection through the same resolver, so the list may sit at +// the end of a path. +func TestForEachOverAPath(t *testing.T) { + f := parseFlow(t, ` +steps: + - for_each: + in: pod.status.containerStatuses + as: c + collect: names + steps: + - set: {var: names, value: "{{c.name}}"} +`) + vars, _, err := ExecuteWith(context.Background(), f, Deps{}, map[string]string{"pod": podDetails}) + require.NoError(t, err) + assert.Equal(t, "api\n\nsidecar", vars["names"]) +} + +// `[*]` was written in the same measurement as the paths. It is refused — a +// path resolves to ONE value — but the refusal names the loop that does what +// was asked, instead of listing the shapes that are allowed. +func TestASelectorIsRefusedWithAWayOut(t *testing.T) { + _, _, _, err := parseCond("pod.status.containerStatuses[*].restartCount > 0") + require.Error(t, err) + assert.Contains(t, err.Error(), "for_each") + assert.Contains(t, err.Error(), "list[0]") +} + +// The form a model reaches for first: the path in braces, inside a condition. +// Braces are still refused, and the message still prints the condition without +// them — now for a path of any depth. +func TestBracedDeepPathNamesTheBraces(t *testing.T) { + _, _, _, err := parseCond("{{pod.status.containerStatuses[0].restartCount}} > 0") + require.Error(t, err) + assert.Contains(t, err.Error(), "`pod.status.containerStatuses[0].restartCount > 0`") +} + +// A value large enough to be a whole tool result is clipped in the refusal: the +// sentence saying what is wrong has to survive it. +func TestTheNotJSONRefusalDoesNotPrintEverything(t *testing.T) { + s := &state{vars: map[string]string{"log": strings.Repeat("line of a log ", 200)}} + _, err := s.resolve("log.a.b") + require.Error(t, err) + assert.Less(t, len(err.Error()), 200) +} diff --git a/resolver_test.go b/resolver_test.go index 42c8f51..1b076b5 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -17,12 +17,12 @@ import ( // Deliberately short, and each entry is here because it IS the mechanism rather // than a consumer of it: // -// lookup — the resolver itself: the one place that reads a variable, and -// the only one that knows a name may address a field; -// set — the writer, the counterpart of lookup; +// resolve — the resolver itself: the one place that reads a variable, and +// the only one that knows a name may be a path into its value; +// set — the writer, the counterpart of resolve; // newState — seeds the variables that came in, before any step runs. var allowedVarsAccess = map[string]bool{ - "lookup": true, + "resolve": true, "set": true, "newState": true, } diff --git a/skill.schema.ru.yaml b/skill.schema.ru.yaml index 45e5d02..6faf64c 100644 --- a/skill.schema.ru.yaml +++ b/skill.schema.ru.yaml @@ -462,7 +462,8 @@ $defs: type: object description: | Схема структурного ответа шага. Результат кладётся в переменную - объектом, поля доступны как `{{var.field}}` — ОДИН уровень вложенности. + объектом, а значение внутри достаётся ПУТЁМ: `{{var.field}}`, + `{{var.a.b.c}}`, `{{var.items[0].name}}`. Зачем: живой шаг разбора извлекает кластер, namespace и под ОДНИМ шагом вместо трёх, то есть −2 генерации на ход в скилле, где их всего 3–4. @@ -920,6 +921,13 @@ $defs: Переменная называется ЗДЕСЬ БЕЗ {{ }} — скобки принадлежат подстановке, условие берёт само имя. + Слева стоит ССЫЛКА: имя либо путь внутрь значения — `ctx.mode`, + `pod.status.containerStatuses[0].restartCount`. Та же форма, что и в + подстановке. Путь, не разрешившийся до значения, — ошибка, а не false: + иначе `a.b.c` при отсутствующем `b` неотличимо от «значение пустое», а + по нему ветвятся. `[*]` и фильтры отвергаются намеренно: путь + разрешается в ОДНО значение, а выбирать многое — это `for_each`. + `is empty` существует потому, что ветке «не нашлось, пробуем иначе» пустота и отказ означают одно и то же, хотя политике — разное. diff --git a/skill.schema.yaml b/skill.schema.yaml index a2a37af..b1bee16 100644 --- a/skill.schema.yaml +++ b/skill.schema.yaml @@ -474,8 +474,8 @@ $defs: type: object description: | The schema of the step's structured answer. The result is stored in - the variable as an object, fields reachable as `{{var.field}}` — ONE - level of nesting. + the variable as an object, and a value inside it is reached by a PATH: + `{{var.field}}`, `{{var.a.b.c}}`, `{{var.items[0].name}}`. Why: a live parsing step extracts the cluster, the namespace and the pod in ONE step instead of three, i.e. −2 generations per turn in a @@ -942,6 +942,14 @@ $defs: A variable is named WITHOUT {{ }} here — the braces belong to substitution, and a condition takes the name itself. + On the left stands a REFERENCE: a name, or a path into the value — + `ctx.mode`, `pod.status.containerStatuses[0].restartCount`. The same + shape substitution takes. A path that does not resolve is an error, + not a false: `a.b.c` with `b` missing would otherwise be + indistinguishable from "the value is empty", and branches are taken on + it. `[*]` and filters are refused on purpose — a path resolves to ONE + value, and picking many is what `for_each` is for. + `is empty` exists because to a "nothing found, try another way" branch emptiness and a failure mean the same thing, even though to the policy they do not. diff --git a/steps.go b/steps.go index a4fda94..19fe10b 100644 --- a/steps.go +++ b/steps.go @@ -89,7 +89,11 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { } switch { case step.Set != nil: - s.set(step.Set.Var, s.expand(step.Set.Value)) + v, err := s.expand(step.Set.Value) + if err != nil { + return false, err + } + s.set(step.Set.Var, v) s.trace(step, "ok", "", 0, started) return false, nil @@ -98,7 +102,11 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { // would take the whole remainder of the skill with it — for a full stop // there is abort. case step.Switch != nil: - key := strings.TrimSpace(s.lookup(step.Switch.Var)) + v, err := s.resolve(step.Switch.Var) + if err != nil { + return false, err + } + key := strings.TrimSpace(v) branch, ok := step.Switch.Cases[key] chosen, outcome := key, "ok" if !ok { @@ -119,7 +127,7 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { } } s.trace(step, outcome, chosen, 0, started) - _, err := s.run(ctx, branch) + _, err = s.run(ctx, branch) return false, err case step.If != nil: @@ -146,7 +154,10 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { return s.parallelStep(ctx, step) case step.Exit != nil: - reason := s.expand(step.Exit.Reason) + reason, err := s.expand(step.Exit.Reason) + if err != nil { + return false, err + } s.trace(step, "exit", reason, 0, started) return false, &ExitError{Reason: reason} @@ -167,7 +178,10 @@ func (s *state) runStep(ctx context.Context, step Step) (bool, error) { // five clusters hands the model the tools of ALL five, and it can call // the wrong one. Narrowing makes the mistake impossible rather than // unlikely. - only := s.expand(step.OnServer) + only, err := s.expand(step.OnServer) + if err != nil { + return s.onError(step, err) + } if err := s.allowServer(only); err != nil { return s.onError(step, err) } @@ -177,9 +191,15 @@ func (s *state) runStep(ctx context.Context, step Step) (bool, error) { // the rest of a value, so it is given the whole thing instead (see // expandWhole). `on_server` above counts as having tools: it narrows the // radius to one server rather than removing it. - instruction, unreachable := s.expand(run.Instruction), "" + var instruction, unreachable string + var err error if len(tools) == 0 { - instruction, unreachable = s.expandWhole(run.Instruction) + instruction, unreachable, err = s.expandWhole(run.Instruction) + } else { + instruction, err = s.expand(run.Instruction) + } + if err != nil { + return s.onError(step, err) } req := StepRequest{ Name: step.Name, @@ -250,7 +270,10 @@ func (s *state) runStep(ctx context.Context, step Step) (bool, error) { if isBlankResult(value) { switch policy { case EmptyUse: - value = s.expand(replacement) + value, err = s.expand(replacement) + if err != nil { + return s.onError(step, err) + } s.traceCalls(step, "ok", "on_empty: used the declared value", calls, failed, started) // EmptyRetry lands here with its retries spent, and is treated as // EmptyFail: the author asked to retry because empty was not @@ -330,7 +353,11 @@ func (s *state) callStep(ctx context.Context, step Step) (bool, error) { if step.OnServer != "" { // The server is named by the step — the tool name in call.tool may come // without a prefix. The computed name goes through the same set check. - server = s.expand(step.OnServer) + expanded, err := s.expand(step.OnServer) + if err != nil { + return s.onError(step, err) + } + server = expanded if _, bare, ok := SplitToolRef(call.Tool); ok { tool = bare } else { @@ -354,7 +381,12 @@ func (s *state) callStep(ctx context.Context, step Step) (bool, error) { return s.onError(step, err) } - out, err := s.caller.CallTool(ctx, server, tool, s.callArgs(call.Args)) + args, err := s.callArgs(call.Args) + if err != nil { + s.trace(step, outcomeFor(err), err.Error(), 0, started) + return s.onError(step, err) + } + out, err := s.caller.CallTool(ctx, server, tool, args) if err != nil { s.trace(step, outcomeFor(err), err.Error(), 1, started) return s.onError(step, err) @@ -367,7 +399,10 @@ func (s *state) callStep(ctx context.Context, step Step) (bool, error) { if policy, replacement := emptyPolicyOf(step); isBlankResult(out) { switch policy { case EmptyUse: - out = s.expand(replacement) + out, err = s.expand(replacement) + if err != nil { + return s.onError(step, err) + } s.trace(step, "ok", "on_empty: used the declared value", 1, started) case EmptyFail: s.trace(step, "degraded", errEmptyResult.Error(), 1, started) @@ -406,7 +441,12 @@ func (s *state) delegateStep(ctx context.Context, step Step) (bool, error) { s.trace(step, outcomeFor(err), err.Error(), 0, started) return s.onError(step, err) } - out, err := s.delegate.Delegate(ctx, d.Skill, s.expand(d.Task)) + task, err := s.expand(d.Task) + if err != nil { + s.trace(step, outcomeFor(err), err.Error(), 0, started) + return s.onError(step, err) + } + out, err := s.delegate.Delegate(ctx, d.Skill, task) if err != nil { s.trace(step, outcomeFor(err), err.Error(), 0, started) return s.onError(step, err) @@ -439,7 +479,12 @@ func (s *state) forEachStep(ctx context.Context, step Step) (bool, error) { // // The collection is taken WHOLE: the variable holds a preview, and a // truncated list would give a partial walk that looks complete. - items := splitCollection(s.fullValue(s.lookup(fe.In))) + in, err := s.resolve(fe.In) + if err != nil { + s.trace(step, outcomeFor(err), err.Error(), 0, started) + return s.onError(step, err) + } + items := splitCollection(s.fullValue(in)) total := len(items) limit := fe.MaxIterations if limit <= 0 { @@ -490,7 +535,12 @@ func (s *state) forEachStep(ctx context.Context, step Step) (bool, error) { // cuts the last line) and a single handle can no longer stand for N // results. Resolving before the join is the only moment this is // still fixable. - collected = append(collected, s.payload(fe.Collect)) + got, perr := s.payload(fe.Collect) + if perr != nil { + s.trace(step, outcomeFor(perr), perr.Error(), len(items), started) + return s.onError(step, perr) + } + collected = append(collected, got) } } if fe.Collect != "" { diff --git a/version.go b/version.go index 5922df8..2cfbd0a 100644 --- a/version.go +++ b/version.go @@ -28,7 +28,7 @@ import ( // minor — an optional field was added (a skill using it requires an engine // no older than that minor); // patch — engine fixes, the format did not change. -const EngineVersion = "2.3.0" +const EngineVersion = "2.4.0" // LegacyEngineVersion — what counts as the declared version when the field is // absent (skills written before it was introduced). From af1ae34b6221a9cdabb613e5c30b2b5c5f7be869 Mon Sep 17 00:00:00 2001 From: Ivan Diatchenko <2518263+inhuman@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:03:58 +0300 Subject: [PATCH 2/3] =?UTF-8?q?path:=20=D0=BF=D1=80=D0=B0=D0=B2=D0=BA?= =?UTF-8?q?=D0=B8=20=D0=BF=D0=BE=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20?= =?UTF-8?q?=E2=80=94=20=D0=BB=D0=BE=D0=B6=D0=BD=D0=B0=D1=8F=20=D0=B8=D1=81?= =?UTF-8?q?=D1=82=D0=BE=D1=80=D0=B8=D1=8F,=20exit=20=D0=B8=20=D0=BF=D0=BE?= =?UTF-8?q?=D0=BB=D0=B8=D1=82=D0=B8=D0=BA=D0=B0=20=D1=88=D0=B0=D0=B3=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Комментарий про valueOf утверждал, что подстановка в поле «никогда не работала» на результатах call. Неправда: фолбэк в память стоит в objectOf с первого коммита (afe751d), я перенёс фразу дословно, не проверив. Переписано на «почему обработка здесь нужна», без истории. Та же фраза была в комментарии теста — убрана и там; в репозитории копий не осталось. 2. exit больше не отменяется собственной подписью. Подпись — это caption, а exit — способ скилла сказать «не мой случай, верните ход обычным путём»; потребитель отличает выход от отказа специально (скилл, запущенный поимённо, на отказе ход останавливает, на выходе нет). Непроходящий путь в подписи превращал одно в другое молча. Теперь подстановка там best-effort: что не разрешилось, остаётся как написано, скобками наружу, где это видно читателю. Закрыто двумя тестами — раньше поведение не было закреплено вовсе. 3. set, switch и if при отказе оставляют трассу и слушают on_error. До появления путей эти три рода шага не умели отказывать вовсе, поэтому обе половины у них не были подключены, — при том что on_error лежит в inline Run, откуда его читают все остальные роды. Молча игнорировать разобранное поле — ровно тот класс «объявлено и не действует», который формат и вылавливает. Заодно Flow.Validate проверяет значение on_error у любого рода шага, а не только рядом с инструкцией: неизвестное значение теперь означало бы тихий abort. --- CHANGELOG.md | 15 +++++++++ expand_test.go | 4 +-- flow.go | 11 +++++-- path.go | 16 +++++----- path_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++++ skill.schema.ru.yaml | 5 +++ skill.schema.yaml | 5 +++ steps.go | 35 +++++++++++++++++--- 8 files changed, 151 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b3a61e..e341a02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,21 @@ it did; a skill that uses one must declare `skill_engine_version: 2.4.0`. the old contract. The silent half is watched statically by the linter's W14 — the only place it can be watched at all. +- **Changed**: `set`, `switch` and `if` now leave a TRACE when they fail and + obey the step's `on_error`. Until a reference became a path these three could + not fail at all, so neither half was ever wired up for them — while + `on_error` is a step-level key that lands in the same place the other kinds + read it from. An unknown value in it is now refused by `Flow.Validate` for + every kind, not only beside an instruction. + +- **Changed**: a path that does not resolve inside an `exit` REASON no longer + cancels the exit. The reason is a caption; `exit` is how a skill hands the + turn back ("not my case"), and a consumer tells that apart from a failure on + purpose — a skill run by name stops the turn when it fails and does not when + it leaves. Substitution there is best-effort: whatever did not resolve stays + as the author wrote it, braces and all, where the reader of the reason can + see it. + - **Changed (Go API)**: the substitution and resolution helpers now return an error, since a path can fail: `expand`, `expandForArgs`, `expandArgs`, `callArgs`, `payload` and `expandWhole` are internal, but `RefPattern` is new diff --git a/expand_test.go b/expand_test.go index 75c9a99..d165bc9 100644 --- a/expand_test.go +++ b/expand_test.go @@ -126,8 +126,8 @@ type fakeMemory map[string]string func (m fakeMemory) Get(id string) (string, bool) { v, ok := m[id]; return v, ok } // The host appends a handle to ANY tool result, and the variable stops being -// valid JSON. Without stripping the note, `{{var.field}}` did not work ONCE on -// any `call:` result — the field silently went empty. +// valid JSON. Strip the note or no reference into a `call:` result parses at +// all — the field would go silently empty on every one of them. func TestFieldLookupIgnoresHostMemNote(t *testing.T) { s := &state{vars: map[string]string{ "ctx": `{"head_sha":"abc123","delta_scope":"go"}` + "\n[mem:res-1]", diff --git a/flow.go b/flow.go index ce9f6c3..0fc8a06 100644 --- a/flow.go +++ b/flow.go @@ -738,13 +738,20 @@ func validateSteps(steps []Step, path string) error { at = fmt.Sprintf("%s (%s)", at, s.Name) } n := 0 - if s.Run != nil && strings.TrimSpace(s.Run.Instruction) != "" { - n++ + // `on_error` is a STEP-level key and lands in the inline Run whatever the + // step does — so it is checked here rather than beside the instruction. + // Since a reference became a path, `set`, `switch` and `if` can fail too + // and their policy is read from this same field; an unknown value there + // would quietly mean abort, which is a declaration without an effect. + if s.Run != nil { switch s.Run.OnError { case "", PolicyAbort, PolicyContinue, PolicySkip: default: return fmt.Errorf("%s: unknown on_error %q", at, s.Run.OnError) } + } + if s.Run != nil && strings.TrimSpace(s.Run.Instruction) != "" { + n++ if s.Run.MaxCalls < 0 { return fmt.Errorf("%s: max_calls is negative", at) } diff --git a/path.go b/path.go index cc82854..66bf5e4 100644 --- a/path.go +++ b/path.go @@ -171,15 +171,15 @@ func (s *state) resolve(ref string) (string, error) { // valueOf parses a variable's value as JSON — an object or a list. // -// A variable's value is what the host WOULD show the model, not the raw tool -// output: a working-memory handle ("[mem:id]") is always appended, and a large -// one is truncated to a preview on top of that. Both break parsing, which is -// why the field silently went empty for ANY `call:` result — {{var.field}} -// substitution never worked on such variables. +// Two things stand between the value and the parser, and both are the host's +// doing: a variable holds what the host WOULD show the model, so a +// working-memory handle ("[mem:id]") is appended to any result and a large one +// is truncated to a preview on top of that. Either of them alone makes the text +// not JSON, so a path into a `call:` result has to deal with both. // -// Order: strip the host's note and try; if that failed (truncated), take the -// whole thing from working memory by the handle — that is what it is appended -// for. +// Order: strip the host's note and try; if that failed — the value is a +// truncated preview and no longer parses at all — take the whole thing from +// working memory by the handle, which is what the handle is appended for. func (s *state) valueOf(raw string) (any, bool) { var v any if err := json.Unmarshal([]byte(trimHostNote(raw, s.vocab.TruncationNotes)), &v); err == nil { diff --git a/path_test.go b/path_test.go index 6a1a44e..2d50dba 100644 --- a/path_test.go +++ b/path_test.go @@ -235,6 +235,82 @@ func TestBracedDeepPathNamesTheBraces(t *testing.T) { assert.Contains(t, err.Error(), "`pod.status.containerStatuses[0].restartCount > 0`") } +// A caption must not cancel the exit it captions. `exit` is how a skill hands +// the turn back — "not my case" — and the consumer tells that apart from a +// failure: a skill run by name stops the turn on a failure and does not on an +// exit. A path that would not resolve in the reason must not silently turn one +// into the other, so the substitution there is best-effort and what did not +// resolve stays visible as written. +func TestABrokenPathInAnExitReasonStillExits(t *testing.T) { + f := parseFlow(t, ` +steps: + - name: not_mine + exit: {reason: "not my case: {{req.data.cluster}}"} + - name: never + set: {var: reached, value: "yes"} +`) + vars, outcome, err := ExecuteWith(context.Background(), f, Deps{}, + map[string]string{"req": `{"kind": "other"}`}) + require.Error(t, err) + require.ErrorIs(t, err, ErrExit, "a caption that would not interpolate cancelled the exit") + + var exit *ExitError + require.ErrorAs(t, err, &exit) + assert.Equal(t, "not my case: {{req.data.cluster}}", exit.Reason, + "what did not resolve stays as written, where the reader can see it") + assert.Empty(t, vars["reached"], "the flow stopped, as an exit does") + assert.Equal(t, "exit", outcome.Steps[0].Outcome) +} + +// A caption that DOES resolve is substituted as always — best-effort is the +// fallback, not the behaviour. +func TestAnExitReasonIsStillInterpolated(t *testing.T) { + f := parseFlow(t, ` +steps: + - exit: {reason: "not my case: {{req.kind}}"} +`) + _, _, err := ExecuteWith(context.Background(), f, Deps{}, + map[string]string{"req": `{"kind": "other"}`}) + var exit *ExitError + require.ErrorAs(t, err, &exit) + assert.Equal(t, "not my case: other", exit.Reason) +} + +// `set`, `switch` and `if` could not fail at all until a reference became a +// path. Now that they can, they do what every other kind has always done: +// leave a trace and consult the policy the step already declares. +func TestABrokenPathInSetSwitchAndIfObeysThePolicy(t *testing.T) { + for _, kind := range []string{ + `set: {var: name, value: "{{pod.status.nope.deeper}}"}`, + `switch: {var: pod.status.nope.deeper, cases: {a: [{set: {var: x, value: y}}]}}`, + `if: {cond: "pod.status.nope.deeper == 1", then: [{set: {var: x, value: y}}]}`, + } { + t.Run(kind[:3], func(t *testing.T) { + src := ` +steps: + - name: risky + ` + kind + ` + on_error: continue + - name: after + set: {var: reached, value: "yes"} +` + vars, outcome, err := ExecuteWith(context.Background(), parseFlow(t, src), Deps{}, + map[string]string{"pod": podDetails}) + require.NoError(t, err, "the step declared `continue` and the flow stopped anyway") + assert.Equal(t, "yes", vars["reached"]) + require.NotEmpty(t, outcome.Steps) + assert.Equal(t, "error", outcome.Steps[0].Outcome, "a tolerated failure still leaves a trace") + assert.Contains(t, outcome.Steps[0].Reason, "has no field `nope`") + + // Without a policy the same failure stops the turn. + strict := strings.Replace(src, "\n on_error: continue", "", 1) + _, _, err = ExecuteWith(context.Background(), parseFlow(t, strict), Deps{}, + map[string]string{"pod": podDetails}) + require.Error(t, err) + }) + } +} + // A value large enough to be a whole tool result is clipped in the refusal: the // sentence saying what is wrong has to survive it. func TestTheNotJSONRefusalDoesNotPrintEverything(t *testing.T) { diff --git a/skill.schema.ru.yaml b/skill.schema.ru.yaml index 6faf64c..34867e1 100644 --- a/skill.schema.ru.yaml +++ b/skill.schema.ru.yaml @@ -568,6 +568,11 @@ $defs: abort — прекратить поток (умолчание); continue — записать отказ в переменную и идти дальше; skip — пропустить остаток ТЕКУЩЕЙ ветки. + + Действует на любой род шага, который умеет отказать, а с 2.4.0 к ним + относятся `set`, `switch` и `if`: путь, не разрешившийся до значения, + — это отказ, а эти три и есть места, где путь читается без модели и + без инструмента. Отказ по правам помечается DENIED:, прочий сбой — ERROR:; политика их различает, ветвление `is empty` — нет. diff --git a/skill.schema.yaml b/skill.schema.yaml index b1bee16..dce25dc 100644 --- a/skill.schema.yaml +++ b/skill.schema.yaml @@ -583,6 +583,11 @@ $defs: abort — stop the flow (default); continue — record the failure into a variable and move on; skip — skip the rest of the CURRENT branch. + + It applies to every kind of step that can fail, which since 2.4.0 + includes `set`, `switch` and `if`: a path that does not resolve is a + failure, and these three are where one is read without a model or a + tool involved. A permission refusal is marked DENIED:, any other breakage ERROR:; the policy tells them apart, an `is empty` branch does not. diff --git a/steps.go b/steps.go index 19fe10b..929a304 100644 --- a/steps.go +++ b/steps.go @@ -91,7 +91,7 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { case step.Set != nil: v, err := s.expand(step.Set.Value) if err != nil { - return false, err + return s.failed(step, err, started) } s.set(step.Set.Var, v) s.trace(step, "ok", "", 0, started) @@ -104,7 +104,7 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { case step.Switch != nil: v, err := s.resolve(step.Switch.Var) if err != nil { - return false, err + return s.failed(step, err, started) } key := strings.TrimSpace(v) branch, ok := step.Switch.Cases[key] @@ -133,7 +133,7 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { case step.If != nil: ok, err := s.eval(step.If.Cond) if err != nil { - return false, err + return s.failed(step, err, started) } if ok { s.trace(step, "ok", "then", 0, started) @@ -154,9 +154,20 @@ func (s *state) one(ctx context.Context, step Step) (bool, error) { return s.parallelStep(ctx, step) case step.Exit != nil: + // The reason is a CAPTION, and a caption that would not interpolate must + // not cancel the exit. `exit` is how a skill hands the turn back — "not + // my case" — and a consumer tells that apart from a failure on purpose: + // a skill run by name stops the turn when it fails and does not when it + // leaves. A broken path in the caption would silently turn one into the + // other. + // + // So the substitution here is best-effort: whatever did not resolve + // stays as the author wrote it, braces and all, where the reader of the + // reason can see it. Better than a hole in the sentence, and better than + // an English remark inside a caption written in another language. reason, err := s.expand(step.Exit.Reason) if err != nil { - return false, err + reason = step.Exit.Reason } s.trace(step, "exit", reason, 0, started) return false, &ExitError{Reason: reason} @@ -826,6 +837,22 @@ func (s *state) toolsFor(run *Run) []string { return out } +// failed — a step that could not do its work: leave a trace, then let the +// step's own policy decide. +// +// The kinds that reach it from `one` — `set`, `switch`, `if` — could not fail +// at all until a reference became a PATH, so neither half was ever wired up for +// them. Both matter. A failure with no trace vanishes: under a tolerant policy +// the flow moves on and the events hold neither the step nor a reason, which is +// how two live steps once dropped out of a turn and read as absent from the +// skill. And `on_error` is a field these steps already PARSE — it lands in the +// inline Run — so ignoring it would leave the author with a declaration that +// has no effect, the class this format keeps hunting down. +func (s *state) failed(step Step, err error, started time.Time) (bool, error) { + s.trace(step, outcomeFor(err), err.Error(), 0, started) + return s.onError(step, err) +} + // onError applies the step's failure policy. func (s *state) onError(step Step, err error) (bool, error) { // A permission refusal is the most common class in live skills, and the From 83569228d16441f35277089f6ba7f48517e9613f Mon Sep 17 00:00:00 2001 From: Ivan Diatchenko <2518263+inhuman@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:59:19 +0300 Subject: [PATCH 3/3] =?UTF-8?q?handbook:=20=D1=81=D0=BF=D1=80=D0=B0=D0=B2?= =?UTF-8?q?=D0=BE=D1=87=D0=BD=D0=B8=D0=BA=20=D1=83=D0=B5=D0=B7=D0=B6=D0=B0?= =?UTF-8?q?=D0=B5=D1=82=20=D0=B2=20=D0=BC=D0=BE=D0=B4=D1=83=D0=BB=D1=8C=20?= =?UTF-8?q?=D0=B8=20=D0=B4=D0=BE=D1=81=D1=82=D0=B0=D1=91=D1=82=D1=81=D1=8F?= =?UTF-8?q?=20=D0=B8=D0=B7=20=D0=BA=D0=BE=D0=B4=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Справочник был написан 3 августа и лежал в specs/, то есть в .gitignore: его не было ни в репозитории, ни в модуле, ни в vendor/ у встраивающего. Он существовал на одной машине, при том что его собственный README адресует его в том числе инструменту, который пишет скиллы. Теперь handbook/ с go:embed и доступом как у схемы: skillengine.HandbookIndex() // ~1.4 КБ, кладётся в промпт целиком skillengine.Handbook("flow-shape") Разделами, а не целиком: 70 КБ в шаг не влезают и не должны. Проверено не изнутри репы, а из отдельного модуля через replace — то же, что vendor/. Почему указатель обязан вести туда, куда адресат может пойти: за три дня сборки скиллов моделью одиннадцать коммитов подряд — один класс, форма, которой в формате нет. Перечень полей в промпте не помогает (1 из 8 против 0 из 8, в пределах шума), проза не помогает («сделай отбор отдельным шагом» — 0 из 16), а прежний указатель «позови schema-тул» на пути программы мёртв дважды: тула нет в радиусе, а у шагов сборки tools: []. Поэтому раздел теперь начинается с готовой ФОРМЫ, и её можно достать вызовом. Правило линтера, покрытое разделом, несёт его id: Rule.Handbook и Finding.Handbook. Полем, а не приклеенной фразой — отказ собирает встраивающий: человеку «см. также», инструменту id, по которому он сходит. Справочник не становится вторым источником правды: имена полей живут в схеме, и тест отвергает раздел, назвавший поле, которого в схеме нет (отличая поля формата от полей пользовательской response_schema по типу справа). Из содержания вычищен словарь чужой установки — протокол вызова, поля телеметрии, кластеры, имена скиллов и стадии конвейера. За это уже ретрачено пять версий, поэтому заведены два сторожа: публичный файл не имеет права называть чужую установку (имена берутся из .githooks/private-names, в самом тесте их нет — он публичный), а справочник не имеет права звучать написанным внутри неё. P2 переписан: «условие читает один уровень» стало ложью через сутки после написания — пути и числовые сравнения уже в движке. --- CHANGELOG.md | 42 ++++- README.md | 1 + README.ru.md | 1 + asset_test.go | 4 +- call_test.go | 2 +- handbook.go | 148 +++++++++++++++ handbook/01-response-schema.md | 315 ++++++++++++++++++++++++++++++++ handbook/02-failures.md | 237 ++++++++++++++++++++++++ handbook/03-instruction-text.md | 225 +++++++++++++++++++++++ handbook/04-context-and-cost.md | 132 +++++++++++++ handbook/05-verification.md | 138 ++++++++++++++ handbook/06-flow-shape.md | 115 ++++++++++++ handbook/README.md | 74 ++++++++ handbook_test.go | 281 ++++++++++++++++++++++++++++ lint/README.md | 14 ++ lint/catalogue.go | 67 ++++--- lint/doc_test.go | 35 ++++ lint/lint.go | 19 +- 18 files changed, 1818 insertions(+), 32 deletions(-) create mode 100644 handbook.go create mode 100644 handbook/01-response-schema.md create mode 100644 handbook/02-failures.md create mode 100644 handbook/03-instruction-text.md create mode 100644 handbook/04-context-and-cost.md create mode 100644 handbook/05-verification.md create mode 100644 handbook/06-flow-shape.md create mode 100644 handbook/README.md create mode 100644 handbook_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e341a02..20dc33a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,8 +23,46 @@ anything else is refused rather than guessed at. ## 2.4.0 -A reference may now be a PATH. A skill that does not use one behaves exactly as -it did; a skill that uses one must declare `skill_engine_version: 2.4.0`. +A reference may now be a PATH, and the skill author's handbook travels with the +module. + +- **Added**: `handbook/` — the failure classes of the format and the forms that + avoid them, embedded and reachable from code: + + ```go + for _, s := range skillengine.HandbookIndex() { … } // ~1.4 KB, fits in a prompt + text := skillengine.Handbook("flow-shape") // a section, on demand + ``` + + It existed before this release and could not be reached: it lived in the + ignored spec tree, so it was in no repository, no module and no `vendor/`. + Now it cannot drift from the engine either — updating the module updates the + handbook. + + Why it has to be reachable rather than merely written. Over three days of a + model writing skills, eleven commits in a row were one class — a form the + format does not have, a different one each time. Three measurements from those + days: the LIST OF FIELDS in the prompt does not help (invented keys 1 of 8 + against 0 of 8, inside the noise); PROSE does not (the hint "make it a separate + step" worked 0 times out of 16, while a ready form gets copied); and a POINTER + works only where the addressee can go — the refusal used to end with "call the + schema tool", which no skill had in its radius. So: whole forms, fetched by a + tool. Every section now opens with a piece to copy. + + Each rule of the linter that covers a handbook class carries its section id — + `Rule.Handbook`, and `Finding.Handbook` on every finding it makes. + + Two things the handbook deliberately is not. It is **not a second source of + truth about the format**: names and types of fields live in the schema, and a + test refuses a section that names a field the schema does not have. And it + carries **no installation's vocabulary** — the tool names, telemetry fields, + clusters and skill names of the deployment it was written in are gone, and a + test keeps them out. It is in Russian, like the failure reports it was written + from; the schema and the READMEs stay bilingual. + +A skill that uses no path behaves exactly as it did; one that uses a path must +declare `skill_engine_version: 2.4.0`. The handbook needs no declaration — it is +documentation, not format. - **Added**: a path of any depth, and an index into a list — in a substitution and on the left of a condition alike: diff --git a/README.md b/README.md index 9929603..0469008 100644 --- a/README.md +++ b/README.md @@ -326,6 +326,7 @@ reading when you are writing a skill, not before. | [`examples/skills/`](examples/skills/) | the format itself — thirteen skills, each a commented example | | [`examples/`](examples/) | two applications that embed the engine, both runnable offline | | [skill.schema.yaml](skill.schema.yaml) | the source of truth for the format (`SchemaRU` is the same in Russian) | +| [handbook/](handbook/) | the failure classes and the forms that avoid them — in Russian, and reachable from code (`HandbookIndex`, `Handbook`) | | [lint/README.md](lint/README.md) | the rule table, and what deliberately stays with the embedder | | [CHANGELOG.md](CHANGELOG.md) | what changed in each format version, and what a migration does | diff --git a/README.ru.md b/README.ru.md index eaad003..6e7fe2b 100644 --- a/README.ru.md +++ b/README.ru.md @@ -309,6 +309,7 @@ react-цикл: каждое решение (какой инструмент, х | [`examples/skills/`](examples/skills/) | сам формат — тринадцать скиллов, каждый с комментариями | | [`examples/`](examples/) | два приложения, встраивающих движок, оба гоняются без сети | | [skill.schema.yaml](skill.schema.yaml) | источник истины про формат (`SchemaRU` — то же по-русски) | +| [handbook/](handbook/) | классы отказов и формы, которые их обходят; доступен из кода (`HandbookIndex`, `Handbook`) | | [lint/README.md](lint/README.md) | таблица правил и то, что намеренно осталось у встраивающего | | [CHANGELOG.md](CHANGELOG.md) | что менялось в каждой версии формата и что делает миграция | diff --git a/asset_test.go b/asset_test.go index 9138c3c..4e9dee6 100644 --- a/asset_test.go +++ b/asset_test.go @@ -278,7 +278,7 @@ func TestAssetRefInsideListIsResolved(t *testing.T) { c := &recordingCaller{out: "ok"} a := &fakeAssets{content: map[string]string{"battery": "echo hello"}} f := parseFlow(t, ` -tools: ["k8s-job"] +tools: ["sandbox"] assets: battery: kind: code @@ -288,7 +288,7 @@ assets: steps: - name: run call: - tool: k8s-job:run_job + tool: sandbox:run_job args: image: go-review command: ["sh", "-c", {from: "asset:battery"}] diff --git a/call_test.go b/call_test.go index 83b35b0..4348320 100644 --- a/call_test.go +++ b/call_test.go @@ -243,7 +243,7 @@ func TestCallStepDeniedWhenFlowHasNoServers(t *testing.T) { steps: - name: sneaky call: - tool: gitlab-write-prod:create_merge_request + tool: tracker-write:create_ticket save_as: out `) _, _, err := ExecuteWith(context.Background(), f, Deps{Caller: c}, nil) diff --git a/handbook.go b/handbook.go new file mode 100644 index 0000000..1fdceb0 --- /dev/null +++ b/handbook.go @@ -0,0 +1,148 @@ +package skillengine + +// The skill author's handbook: failure classes and the forms that avoid them. + +import ( + "embed" + "path" + "sort" + "strings" +) + +// handbookFS — the handbook, embedded the way the schema is. +// +// Files in the repository are not access. An embedder gets this package through +// `go mod vendor`, which copies only what the build references: a directory of +// markdown nobody imports simply does not travel, and a handbook that does not +// travel exists on one machine. +// +// The reason it has to travel at all is measured. Over three days of a model +// writing skills, eleven commits in a row were one class — a form the format +// does not have, a different one each time. Three measurements from those days +// say what does not fix it and what does: +// +// - the LIST OF FIELDS in the prompt does not: invented keys 1 of 8 against +// 0 of 8, inside the noise, with refusals holding in both arms. The author +// is not short of field NAMES; +// - PROSE does not: "if you need a subset, make it a separate step" worked 0 +// times out of 16. A ready form gets copied; an explanation does not; +// - a POINTER to knowledge works only where the addressee can go. The refusal +// ended with "call the schema tool" — dead twice over on the program path, +// since no skill had that tool in its radius and the steps that write +// skills run with `tools: []`. +// +// Hence: whole forms, reachable by a tool. The index is a few hundred bytes and +// travels in every prompt; a section is fetched when it is needed. +// +//go:embed handbook/*.md +var handbookFS embed.FS + +// HandbookSection — one section of the handbook in the index. +type HandbookSection struct { + // ID — the name a refusal can point at: "response-schema", "failures". + ID string + // Title — the section's heading, as the section itself writes it. + Title string + // Summary — one line: what this section is about. + Summary string +} + +// HandbookIndex returns the sections, in reading order. +// +// Cheap on purpose — a few hundred bytes all together, so it can sit in a +// prompt whole without pushing the task out of it. The text of a section is +// fetched by ID with Handbook. +func HandbookIndex() []HandbookSection { + files, err := handbookFS.ReadDir("handbook") + if err != nil { + return nil + } + names := make([]string, 0, len(files)) + for _, f := range files { + if f.IsDir() || f.Name() == "README.md" { + // README is the human's way in — the table of contents, not a + // section. It says the same things the index does. + continue + } + names = append(names, f.Name()) + } + // By file name: the numeric prefix is the reading order, and it is the only + // place that order is written down. A second list in Go would drift from + // the directory on the first section added. + sort.Strings(names) + + out := make([]HandbookSection, 0, len(names)) + for _, name := range names { + text, err := handbookFS.ReadFile(path.Join("handbook", name)) + if err != nil { + continue + } + title, summary := handbookHead(string(text)) + out = append(out, HandbookSection{ID: handbookID(name), Title: title, Summary: summary}) + } + return out +} + +// Handbook returns the text of one section. An unknown id yields an empty +// string — the caller asked for something that is not there, and inventing a +// nearest match would answer a question nobody asked. +func Handbook(id string) string { + for _, f := range handbookFiles() { + if handbookID(f) == id { + text, err := handbookFS.ReadFile(path.Join("handbook", f)) + if err != nil { + return "" + } + return string(text) + } + } + return "" +} + +func handbookFiles() []string { + files, err := handbookFS.ReadDir("handbook") + if err != nil { + return nil + } + out := make([]string, 0, len(files)) + for _, f := range files { + if !f.IsDir() && f.Name() != "README.md" { + out = append(out, f.Name()) + } + } + sort.Strings(out) + return out +} + +// handbookID turns a file name into the id a refusal can print: +// "01-response-schema.md" → "response-schema". The number orders the files and +// is not part of the name — an id that changes when a section is inserted +// before it would break every refusal that quotes it. +func handbookID(file string) string { + name := strings.TrimSuffix(file, ".md") + if i := strings.Index(name, "-"); i > 0 && strings.Trim(name[:i], "0123456789") == "" { + name = name[i+1:] + } + return name +} + +// handbookHead reads the section's own first line and its one-line summary. +// +// The summary is the blockquote right under the heading, not the first +// paragraph: a lede is two or three sentences and belongs to the section, while +// the index needs one line per section and needs it to stay one line. A test +// keeps every section carrying both. +func handbookHead(text string) (title, summary string) { + for line := range strings.SplitSeq(text, "\n") { + line = strings.TrimSpace(line) + switch { + case title == "" && strings.HasPrefix(line, "# "): + title = strings.TrimSpace(strings.TrimPrefix(line, "# ")) + case title != "" && strings.HasPrefix(line, "> "): + return title, strings.TrimSpace(strings.TrimPrefix(line, "> ")) + case title != "" && strings.HasPrefix(line, "## "): + return title, "" // the summary was missing; the test says so + } + } + return title, "" +} diff --git a/handbook/01-response-schema.md b/handbook/01-response-schema.md new file mode 100644 index 0000000..27e0789 --- /dev/null +++ b/handbook/01-response-schema.md @@ -0,0 +1,315 @@ +# Структурный ответ шага + +> Схема шага — грамматика декодирования: чего модель выдать не может, но обязана, кончается зависанием. + +## Форма + +```yaml +# Шаг разбора запроса — то, с чего начинается почти любая программа. +- name: understand + instruction: |- + Запрос: {{input}} + Разбери его по полям. Пришли ВСЕ поля; чего в запросе нет — оставь пустым. + tools: [] + model: small/model # схема работает только там, где есть грамматика + sampling: {max_tokens: 512} # ответу разбора больше не нужно + response_schema: + type: object + required: [intent, has_target] # только то, на что ответ есть ВСЕГДА + properties: + intent: {type: string, enum: [list, show, compare]} + has_target: {type: boolean} + target: {type: string, maxLength: 200} # значение из запроса — НЕ required + save_as: req +``` + +--- + +Схема шага — это не описание того, что хочется получить. Это **грамматика +декодирования**: модель физически не может выдать текст, который ей не +соответствует. Отсюда все правила ниже: то, чего модель выдать не может, но +обязана, кончается не ошибкой, а зависанием. + +--- + +## S1. Обязательным делай РЕШЕНИЕ, а не ЗНАЧЕНИЕ + +**Правило.** В `required` ставь только те поля, на которые модель может ответить +**всегда** — при любом входе, даже пустом. Значение, которое приходит из запроса +пользователя, обязательным быть не может: в запросе его может не быть. + +**Почему.** Обязательное поле без источника разрешается моделью одним из двух +способов, и оба ломают ход. + +*Разгон в пробелы.* Закрыть `}` грамматика не даёт, пока обязательное поле не +выдано, а пробел между токенами JSON легален всегда — модель уходит в пробел до +потолка токенов. + +``` +{"cluster": "east-1", "mode": "list" + ← и 512 токенов чистых пробелов +``` + +Прогон 02.08: скилл про поды с `namespace` в `required` падал на **4 вопросах из +4**, где namespace не назван («покажи поды в кластере dev»). Каждый — дважды: +ретрай на обрыв есть, но шаг стоит на `temperature: 0`, вход тот же — значит и +выход тот же. **Ретрай детерминированного шага не лечит ничего.** + +*Проза-заглушка.* Там, где модель всё же пишет, она пишет объяснение: + +```json +{"key": "ABC-1, ABC-2, ... (список ключей тикетов, если данные доступны)"} +``` + +Ветка `cond: req.key is empty` такую строку пропускает — строка не пуста, — и +дальше вызов уходит с мусором. В живом случае 02.08 это кончилось таблицей +несуществующих тикетов с именами людей, и проверяющий шаг её пропустил. + +*Выдуманное число.* У числового поля запасного варианта нет вовсе: +Скилл разбора пайплайна с `id: integer` в `required` на вопрос без ссылки выдал +`{"id": 12345678}` и трижды сходил за несуществующим пайплайном. + +**Как правильно.** + +```yaml +# Было: значение, которого в запросе может не быть +required: [cluster, namespace, mode] + +# Стало: остаются те, у кого ответ есть всегда — +# cluster (enum с умолчанием), mode (enum) +required: [cluster, mode] +``` + +Если ветка обязана знать, назвали значение или нет, спрашивай **признак**, а не +значение: `deep: boolean`, `has_key: boolean`, `channel: enum [..., unknown]`. +На такой вопрос ответ есть всегда. + +**Умолчание — законный выход.** «Не названо — `prod`» обязательным быть разрешает: +модели есть что написать. Пустота — не разрешает. + +> Статика: **W16** ловит случай, когда противоречие ОБЪЯВЛЕНО («не назван — +> пустая строка» при поле в `required`). Когда описание про отсутствие молчит, +> находит только прогон шага на живой модели (см. [05](05-verification.md)). + +--- + +## S1a. Схема требует — а модель всё равно не присылает: попроси в тексте + +**Правило.** Если обязательных полей несколько и часть из них к вопросу +отношения не имеет, добавь в инструкцию явную просьбу прислать их все. +Обязательности в схеме НЕ достаточно. + +**Почему.** Модель пишет поля, которых касается вопрос, а на первом ненужном +останавливается — и, поскольку закрыть `}` грамматика не даёт, уходит в пробелы. +Тот же разгон, что в S1, но по другой причине: значение есть (`false`), просто +модель считает себя закончившей. + +Замер 03.08, шаг выбора источников (шесть булевых полей, все обязательные), +вопрос «что сломалось за сутки, посмотри логи, метрики и тикеты»: + +``` +{"docs": false, "logs": true, "metrics": true, "repo": false, "tracker": true + ← и 700 символов пробелов +``` + +Не хватает `wiki` — последнего по алфавиту и единственного, которого вопрос не +касается. Воспроизводится 3 прогона из 3 при `temperature: 0`. + +Одна фраза в инструкции — «пришли ВСЕ шесть полей, у невыбранного `false`» — +убирает обрыв: 5 вопросов из 5. + +**Что НЕ помогает** (проверено на том же шаге): + +| попытка | результат | +|---|---| +| убрать часть полей из `required` | молча не приходят вовсе — 5 из 5 | +| `additionalProperties: false` | без изменений | +| `maxLength` соседнему полю | без изменений | +| модель посильнее | обрывы те же, просто на других вопросах | + +**Заодно правило про свободный текст рядом с флагами.** В том же замере поле +свободного текста (`topic`) в одном `required` с булевыми давало обрыв на 3 +вопросах из 3 при ЛЮБОМ числе флагов, а без него в `required` — не приходило ни +разу из 5. Вывод: не смешивай в одном шаге «сочини текст» и «прими N решений». +Если текст нужен дальше по потоку, посмотри, нельзя ли обойтись исходным +вопросом (`{{input}}`) — здесь он оказался лишним: каждый делегируемый скилл +всё равно разбирал вопрос заново. + +--- + +## S2. У объекта должно быть хоть одно обязательное поле — кроме случая, когда пустоту разбирает поток + +**Правило.** Схема без `required` разрешает пустой `{}`, и потребитель получает +молчание вместо данных. + +Исключение — когда пустой ответ **и есть** законный ответ, а поток его +разбирает: + +```yaml + response_schema: + type: object + properties: + id: {type: string} # ← в схеме одно поле, и его может не быть + save_as: rel + + - if: + cond: rel.id is empty # ← ветка ждёт именно этого + then: [...] +``` + +> Статика: **W9**, с послаблением ровно на этот случай — молчит, если в потоке +> есть `cond: <та же переменная> is empty`. Послабление не распространяется на +> вложенные объекты: у записи внутри массива своя пустота, её никакая ветка не +> разбирает. + +--- + +## S3. У свободной строки должен быть потолок + +**Правило.** Каждому строковому полю, куда пишется текст, а не значение из +списка, ставь `maxLength`. Массиву — ещё и `maxItems`, а элементу массива — +свой `maxLength` (у него нет `properties`, и проверка полей его не видит). + +**Почему.** Поле без потолка модель заполняет столько, сколько дадут. Живой случай 02.08, скилл создания MR: ссылки в запросе не было, и модель +дописывала выдуманный URL до упора — + +``` +…_requests_reviewers=1&allowed_merge_requests_can_merge=true& +``` + +— шаг оборвался по потолку токенов, ход потрачен впустую. Второй случай: находки +ревью `strengths: items: {type: string}` без потолка — обрыв посреди записи +уносил ВЕСЬ документ, а не одну запись. + +**Порог бери из данных** (p95 длины поля), а не на глаз. + +> Статика: **W13**. + +--- + +## S4. Конечный выбор — это `enum`, а не строка + +Кластер, канал записи, вид графика, режим — везде, где вариантов конечное число, +пиши `enum` (или `one_of` для ответа-значения). Тогда неверное значение +становится невозможным, а не маловероятным: грамматика не даст его выдать. + +Строкой такие поля дают классический дрейф — `staging`, `staging-1`, `стейджинг`, +`Staging 1` в одном и том же скилле. + +--- + +## S5. Схема требует модели с грамматикой — назови её в шаге + +`response_schema` без грамматики декодирования — обман: модель и без неё часто +отдаёт валидный JSON, то есть «структурный ответ» выглядит работающим, ничего не +гарантируя, и разбор ломается через раз без следа в логах. + +Поэтому движок отвергает шаг со схемой без явной `model` (`response_schema` +без `model` — отказ на разборе), а приложение вправе отвергнуть и модель, у +которой грамматики нет. Называй модель прямо в шаге или в профиле: +`model: small/model`. + +--- + +## S6. Потолок шага со схемой узкий — и это нарочно + +Шаг разбора запроса раскладывает вопрос по полям, и ответ у него короткий: замер +за 7 дней прода по 386 вызовам — максимум 98 токенов. Поэтому потолок такого +шага разумно держать узким (**512**), а не отдавать ему весь конверт модели. + +Не поднимай его, чтобы «вылечить» обрыв: обрыв в таком шаге почти всегда +означает S1 или S3, а больший потолок только удорожает срыв (замер: 6000 → 37 с, +12000 → 75 с, и во втором случае ход упирается уже в бюджет приложения). + +Если ответу шага действительно нужно больше — скажи явно: + +```yaml + sampling: + max_tokens: 6000 +``` + +**Помни, что бюджет держит и рассуждение.** У моделей с видимым reasoning +`reasoning_content` идёт в тот же `max_tokens`. Признак: текст ответа короткий (400 +символов), рассуждение длинное (7000), генерация оборвана по длине. +Это не «ответ слишком длинный», это «до ответа не дошло». + +--- + +## S7. Обязательное поле не должно идти ПОСЛЕ необязательного + +**Правило.** Называй поля так, чтобы по алфавиту все обязательные шли раньше +всех необязательных. Порядок в схеме роли не играет — модель эмитит по алфавиту. + +**Почему.** Модель пишет поля по алфавиту и останавливается там, где считает +себя закончившей. Необязательное поле, вставшее раньше обязательного, успевает +СКАЗАТЬ то, после чего обязательное написать уже нечего. + +Замер 14.08, шаг разбора просьбы собрать скилл. Поле `missing` («чего не +хватает») стояло прямо перед обязательным `name`: + +``` +{"description": "…", "form": "workflow", "intent": "build", + "missing": "задача не названа" + ← и пробелы до потолка +``` + +Объяснив в `missing`, что задача не ясна, модель не могла назвать скилл — а +закрыть объект без `name` грамматика не даёт. Три входа из пяти. + +Лечится ПЕРЕИМЕНОВАНИЕМ, а не просьбой: `missing` → `what_missing` встаёт после +`name`, и всё обязательное успевает выйти. Восемь входов из восьми, и прогон схем по +всему каталогу (234 входа) зелёный. + +Это дешевле S1a: там на каждый шаг нужна фраза в инструкции, здесь — одна буква +в имени поля. + +--- + +## S8. Два обязательных списка подряд — обрыв + +**Правило.** Если решений два и оба выражаются списком, сведи их в ОДИН список, +а разложить по местам пусть код: он знает, что чем является. + +**Почему.** Закрыв первый список пустым, модель второй уже не начинает. + +Замер 14.08, тот же шаг. Требовались `from_kinds` (виды источников) и +`from_servers` (отдельные серверы): + +``` +{"description": "…", "form": "workflow", "from_kinds": [] + ← и пробелы до потолка +``` + +| редакция | обрывов | источник назван | +|---|---|---| +| два обязательных списка | **2 из 9** | 7 из 9 | +| один список `from_where` | **0 из 15** | 13 из 15 | + +Разложить смешанный список по видам и именам — работа встраивающего приложения, +и она однозначна: его реестр знает, что вид, а что имя сервера. + +--- + +## S9. Поле схемы не хранит текст со СВОИМ синтаксисом + +**Правило.** YAML, DOT, markdown-список, любой текст, где значимы переводы строк +и кавычки, — отдельным шагом БЕЗ схемы. В поле структурного ответа его класть +нельзя. + +**Почему.** Перевод строки внутри JSON-строки пишется escape-ом `\n`, кавычка — +`\"`. Под грамматикой декодирования модель ставит вместо них пробел. Просьба «не +забудь экранировать» — ровно та просьба, которую эта книга и заменяет проверкой: +текстовый шаг просто не имеет, чему ломаться. + +Три случая одного класса за два дня, в одном и том же скилле: + +| что клали в поле схемы | что приехало | +|---|---| +| описание шагов (YAML) | одна строка без единого перевода: `steps: - name: get_pods - call: …` | +| схема на DOT | шапка без рёбер: `digraph G { rankdir=LR; node [shape=box]; `, 3 обрыва из 5 | +| нумерованный список блоков | стена: ни одного `\n` в поле, шесть пунктов в строку | + +Все три ушли, как только текст стал отдельным шагом без `response_schema`. + +Признак, по которому это узнают в записях: содержимое поля выглядит +осмысленным, но не разбирается ничем — и в нём нет ни одного перевода строки. diff --git a/handbook/02-failures.md b/handbook/02-failures.md new file mode 100644 index 0000000..c011a02 --- /dev/null +++ b/handbook/02-failures.md @@ -0,0 +1,237 @@ +# Отказы: как скилл должен вести себя, когда данных нет + +> Самый дорогой класс дефектов — не «упало», а «поехало дальше и ответило». + +## Форма + +```yaml +# Вызов, который может не удаться, и ветка на случай, когда данных нет. +- name: fetch + call: + tool: tracker:issue_get + args: {key: "{{req.key}}"} + on_error: continue # отказ ЗАПИШЕТСЯ в переменную, а не пропадёт + save_as: ticket + +- if: + cond: ticket is empty # покрывает и пустоту, и ERROR:/DENIED: + then: + - name: not_found + instruction: "Данные получить не удалось. Скажи это прямо. НЕ придумывай." + tools: [] + max_calls: 0 + - exit: {reason: тикет не получен} +``` + +--- + +Самый дорогой класс дефектов — не «упало», а «поехало дальше и ответило». Ход, +который честно сказал «не смог», стоит один вызов. Ход, который выдумал ответ, +стоит доверия ко всем остальным ответам. + +--- + +## F1. После каждого шага с `on_error: continue` должна быть ветка пустоты + +**Правило.** `on_error: continue` не «пропускает» отказ — он **записывает** его в +переменную и идёт дальше. Значит следующий шаг получит текст отказа вместо +данных и честно отработает `ok`. + +```yaml + - name: fetch_brief + call: + tool: tracker:issue_get + args: {issue_key: "{{req.key}}"} + on_error: continue + save_as: ticket + + # ОБЯЗАТЕЛЬНО: иначе ticket с текстом ошибки доедет до отчёта + - if: + cond: ticket is empty + then: + - name: not_found + instruction: |- + Что вернул трекер по ключу {{req.key}}: {{ticket}} + Тикет получить не удалось. Скажи это прямо… Данные НЕ придумывай. + tools: [] + max_calls: 0 + - exit: + reason: тикет не получен +``` + +**Почему.** Живой случай 02.08, скилл про тикет: такой ветки в нём не было. +Вызов отклонён валидацией ключа → `on_error: continue` → следующий шаг прочитал +текст ошибки как тикет → отчёт составил план → шаг ответа развернул план в +таблицу несуществующих тикетов с именами людей, и проверяющий шаг её пропустил. +Пользователь получил уверенную таблицу выдумки — при том что в той же сессии лежали 31 настоящий +тикет, добытые другим вызовом. + +**`is empty` — правильная проверка, специально для этого.** Она означает «шаг не +дал ничего полезного» и покрывает как пустоту, так и помеченный отказ +(`ERROR:` / `DENIED:`), который пишет политика. Так и написано в схеме формата: + +> `continue` — записать отказ в переменную и идти дальше… Отказ по правам +> помечается `DENIED:`, прочий сбой — `ERROR:`; политика их различает, +> ветвление `is empty` — нет. + +> ⚠️ Отдельная зарубка: **читай схему формата, а не исходники движка.** Автор +> этого справочника посмотрел на код, увидел «в переменную кладётся текст +> ошибки», достроил вывод «значит `is empty` не сработает» и объявил дефектом +> семь исправных мест в шести скиллах. Контракт был описан в схеме прямым +> текстом. + +--- + +## F2. Где может решать код — не должна решать модель + +Проверку «данные приехали или нет» делает `cond`, а не шаг-классификатор. Это +дешевле (ноль вызовов), быстрее и, главное, детерминированно. + +Замер, ради которого стоит это помнить: первая версия починки того же скилла +учила шаг-классификатор отвечать третьим исходом `failed`. **Модель не выбрала +его ни разу** — четыре формы отказа из четырёх получили `robot`, потому что +вопрос шага начинался с «похоже ли имя reporter на робота», то есть предполагал, +что тикет есть. Ветка `cond: ticket is empty` не имеет такой болезни в принципе. + +Модель нужна там, где нужно **суждение** (это робот или человек? это рантайм-баг +или логический?). Для «пусто ли» суждение не нужно. + +--- + +## F3. Отказ обязан быть громким + +Движок сам ставит `degraded` шагу без текста, развилке без сработавших веток, +`switch` без совпадения при пустом `default` и циклу с отказавшими итерациями. +Не гаси эту громкость собственными руками: + +- не заводи `default`-ветку, которая «на всякий случай» делает что-то + осмысленное при неразобранном входе — тогда отказ станет успехом; +- не пиши в `answer` служебную переменную. Пустой `answer` означает «программа + ответа не дала», и ход пойдёт обычным путём; служебный JSON в `answer` + означает «вот ответ», и приложение дорисует его до правдоподобного текста; +- при частичных данных (`parallel` с `on_error: continue`) в шаге ответа прямо + скажи, чего не хватает: у `.skipped` для этого есть список веток, + пропущенных по `when` — без него шаг ответа не отличает «источник ответил + пусто» от «в источник не ходили». + +--- + +## F4. `on_error` и `on_empty` — про разное + +| поле | про что | когда нужно | +|---|---|---| +| `on_error` | вызов/шаг **упал** (нет прав, 404, отказ инструмента) | у любого шага, чей отказ не должен рвать поток | +| `on_empty` | шаг **отработал**, но не дал текста | у шага, чья пустота — это сбой, а не ответ | + +`on_empty: fail` ставь шагу, чей пустой ответ бессмыслен (разбор запроса, +рендер отчёта). `on_empty: continue` — когда пустота законна. + +--- + +## F4a. Пустая строка проходит `required`, а до инструмента не доезжает + +**Правило.** Обязательность поля НЕ гарантирует, что в нём что-то есть: пустая +строка схему удовлетворяет. Если значение уходит аргументом в вызов, проверяй +его веткой, а не надейся на `required`. + +**Почему.** Пустые аргументы отбрасываются при нормализации перед отправкой — +и инструмент отвечает не «пустой запрос», а «обязательный аргумент отсутствует». +Ошибка выглядит так, будто скилл забыл передать поле, хотя поле объявлено +обязательным и модель его прислала. + +Живой случай 02.08: вопрос «какие страницы обновляли за последнюю неделю в +пространстве X» — темы для поиска в нём нет, он про недавние изменения. Шаг +разбора вернул `{"query": "", "space": "X"}`, схема довольна, а поиск ответил +`query Missing required argument`. + +```yaml + - if: + cond: req.query is empty # ← пустую строку ловит именно это + then: + - name: ask_topic + instruction: |- + Поиск лексический, ему нужно слово или название, а в запросе темы нет. + Скажи это прямо и попроси тему. + tools: [] + max_calls: 0 + - exit: + reason: тема поиска не названа +``` + +**Обобщение.** `required` защищает от «поле не пришло», а не от «поле пустое». +Между схемой и инструментом есть ещё одна граница, и на ней пустота +превращается в отсутствие. + +--- + +## F5. `exit` — законный конец ветки + +Не тяни поток до конца ради формальности. Если ветка выяснила, что дальше идти +некуда (репозиторий не назван, тикет не получен, прав нет), заканчивай `exit` с +причиной — она попадёт в трассу хода. + +```yaml + - exit: + reason: репозиторий не назван или в этот инстанс нет write-канала +``` + +--- + +## F6. Права проверяются ДО первого шага — объявляй только то, что нужно + +Программа объявляет инструменты статически, поэтому нехватка прав видна заранее +и отказ приходит до первой генерации. Отказ **полный**: не хватило одного +инструмента — не запускается вся программа. + +Практическое следствие: не объявляй в `tools`/`builtin_tools` серверы «про +запас». Каждый лишний сервер — это ещё одно условие, при котором скилл целиком +недоступен пользователю с узкими правами. В прогоне 02.08 из-за этого шесть +скиллов не дали ни одного поведенческого замера: у учётной записи, под которой +шёл прогон, не было прав на один из объявленных серверов. + +--- + +## F7. Ветвись по МАРКЕРУ, а не по формулировке + +**Правило.** Если поток читает ответ инструмента и решает по нему, пусть +инструмент вернёт отдельное слово-маркер (`NO_PLAN`, `NO_TOOLS`, `ASKED_ONCE`), и +ветвись по нему. Условие на человеческую фразу — отложенный отказ. + +**Почему.** Формулировку правят, и правка молча меняет поведение потока. К тому +же сравнение `contains` свободно с конца слова, но не с середины. + +Живой промах 13.08: ветка ловила `contains Ошибка`, а инструмент ответил +«валидация нашла **ошибки**». С заглавной «Ошибка» это не совпадает, ветка не +сработала, цикл починки крутился на успешном результате. + +Маркер отдельным словом ни с чем не пересекается и переживает любую редактуру +текста вокруг. + +**И обратное — про признак УСПЕХА.** Отказов у инструмента полтора десятка +формулировок, а успех один и печатается одной строкой. Значит условие +продолжения пишут так: + +```yaml +when: written not contains Предложение сохранено # ← отсутствие успеха +``` + +а не «наличие слова про ошибку»: перечислять формулировки отказа — гонка, +которую не выиграть. + +--- + +## F8. Диагностика называет СЛУЧИВШЕЕСЯ, а не частый случай + +**Правило.** Текст отказа, который читает автор (или модель, чинящая описание), +обязан цитировать место. Подсказка про частый случай ставится ПОСЛЕ цитаты и не +имеет права выглядеть диагнозом. + +**Почему.** Подсказка написана по прошлому отказу, а чинить будут этот. + +Живой промах 14.08: разбор дал точный адрес — «строка 58, отображение вместо +строки», — а рецепт добавил «чаще всего это `on_server`». В том файле `on_server` +не было вовсе: отображение стояло в `set.value`. Три захода починки исправляли +то, чего нет, и вернули байт-в-байт тот же отказ. + +Цитата обязана начинаться с начала ШАГА, а не с фиксированного отступа назад: +чинить будут шаг, и правка по имени шага требует это имя видеть. diff --git a/handbook/03-instruction-text.md b/handbook/03-instruction-text.md new file mode 100644 index 0000000..f32369a --- /dev/null +++ b/handbook/03-instruction-text.md @@ -0,0 +1,225 @@ +# Текст шага + +> Инструкция шага — это промпт: что написано, уходит в модель; чего не написано, модель додумает. + +## Форма + +```yaml +# Порядок вопросов проверен замером и менять его нельзя (I1): пока шаг начинался +# с вопроса о свойствах данных, он на любой отказ отвечал так, будто данные есть. +- name: check_reporter + instruction: |- + Данные: {{ticket}} + 1. Есть ли данные вообще? Если нет — ответь `failed`. + 2. Если есть: автор — человек или технический аккаунт? + Человек: ФИО, личное имя. Технический: robot, svc, noreply. + Значения бери БУКВАЛЬНО из данных; имена из этой инструкции значениями не являются. + tools: [] + one_of: [human, robot, failed] + save_as: reporter_kind +``` + +--- + +Инструкция шага — это промпт. Всё, что в ней написано, уходит в модель и на +что-то влияет; всё, что не написано, модель додумает. + +--- + +## I1. Сначала спроси, ЕСТЬ ли данные, потом — какие они + +**Правило.** Если шаг может получить на вход отказ или пустоту, первый вопрос +инструкции — про наличие данных. Вопрос о свойствах данных **предполагает**, что +данные есть, и модель это предположение принимает. + +Замер 02.08, один и тот же шаг, одна и та же модель, одни и те же варианты +ответа — меняется только порядок вопроса: + +| формулировка | тикет-человек | тикет-робот | 4 формы отказа | +|---|---|---|---| +| «похоже ли имя reporter на робота? … `failed` — тикета нет» | human ✅ | robot ✅ | **robot ❌ 4/4** | +| «сначала реши, ЕСТЬ ли данные тикета … потом кто reporter» | human ✅ | robot ✅ | **failed ✅ 4/4** | + +Порядок вариантов в `one_of` при этом ни на что не влияет — проверено +перестановкой. Влияет порядок **вопросов**. + +--- + +## I2. Перечисление примеров тянет ответ к себе + +Подсказка вида «признаки в имени: robot, bot, integration, svc, service, system, +noreply, auto» на тонком входе смещает маленькую модель к ответу `robot` +независимо от данных. На полном тикете смещения не видно, на обрывке — видно. + +Это не запрет на примеры. Это причина: **давать примеры симметрично** («имя +человека: ФИО, личное имя; технический аккаунт: robot, svc, noreply») или +уводить перечисление в описание поля схемы, а не в основной вопрос. + +--- + +## I2a. Конкретное имя в инструкции становится ответом + +**Правило.** В описании поля не пиши примеров, которые выглядят как готовое +ЗНАЧЕНИЕ этого поля. Нужен образец формы — делай его заведомо ненастоящим: +`<группа>/<проект>`, а не путь существующего репозитория. + +**Почему.** Модель ищет в промпте, чем заполнить поле, и конкретное имя рядом с +описанием — лучший кандидат, чем слово из запроса. + +Живой отказ 03.08. Шаг разбора запроса про поиск в коде: + +``` +project — путь репозитория, если он дан ссылкой или путём + (группа/подгруппа/имя-сервиса). … +needle — ЧТО ищем: имя функции, класса, текст ошибки. +``` + +На запрос «найди в коде parseConfig» разбор вернул `needle` = **имя сервиса из +примера выше**. Дальше поиск честно искал репозиторий с таким именем, не нашёл — +а шаг отчёта выдал функцию с телом, комментариями и объяснением, которой не +существует. Ни один шаг не упал: все `ok`. + +Две поправки закрыли это: пример стал безличным (`<группа>/<проект>`), а к полю +добавлено «БУКВАЛЬНО из запроса; значения бери только оттуда — имена из этой +инструкции значениями не являются». + +**Симптом, по которому узнаётся класс:** в ответе фигурирует что-то, чего в +запросе не было, но что есть в тексте самого скилла. + +--- + +## I2b. Служебная ветка не должна выглядеть источником + +**Правило.** Имя шага попадает в служебные списки (`.skipped`) и +читается моделью как содержание. Ветка-механика, названная как ветка-работа, +искажает вывод шага-сводки. + +Живой отказ 03.08. Скилл поиска: ветки `probe_docs`, `probe_tracker` — +источники, плюс добавленные `probe_docs_default`, `probe_metrics_default` — +умолчание, когда источник не назван. Пропущенные ветки перечисляются в `.skipped`. + +На запросе, где документация и трекер **отработали**, список выглядел так: + +``` +не запускались: probe_docs_default, probe_metrics_default, probe_metrics, probe_logs, … +``` + +Шаг сводки прочитал `probe_docs_default` как «документацию не опрашивали» и ответил +«ни один из зондов не запустился» — при двух отработавших с находками. + +Лечится двумя вещами: имя механики не притворяется источником (`fallback_docs`), +и шаг-сводка судит **по разделу находок**, а не по списку пропущенных — тот +непуст почти всегда. + +--- + +## I3. Пояснения для людей — в YAML-комментарии, не в инструкцию + +```yaml + # Порядок вопроса проверен замером и менять его нельзя: пока шаг начинался + # с «похоже ли имя reporter на робота», он на любой отказ отвечал robot. + - name: check_reporter + instruction: |- + Тикет: {{ticket}} + … +``` + +Всё, что внутри `instruction:`, уедет в промпт — включая объяснение, почему код +написан так, ссылки на задачи и заметки «не менять». Модели это шум, а в шаге со +схемой ещё и лишние токены в узком потолке. + +Комментарий YAML не уходит никуда и виден ровно тому, кто правит скилл. + +--- + +## I4. Не проси того, что можно выразить структурой + +Каждая просьба в тексте («НЕ вызывай retract без подтверждения», «РОВНО ОДИН +запуск прогона», «не тяни логи на чистом код-баге») исполняется настолько, +насколько модель дочитала до неё, прежде чем начать действовать. + +| просьба в прозе | как это же выражается программой | +|---|---| +| «не делай X без подтверждения» | ветки `if`: в неподтверждённой ветке шага с X просто нет | +| «сделай ровно один раз» | один шаг `call` — повторить его модель не может | +| «не ходи в источник» | `tools: []` у шага | +| «верни ровно эти поля» | `response_schema` с `required` | +| «выбери одно из трёх» | `one_of` | + +Оставляй в тексте только то, что требует суждения. + +--- + +## I4a. Словарь «слово → значение» исполняет условие, а не модель + +**Правило.** Если весь шаг сводится к «какие из этих слов назвал запрос», +замени его на `contains` в условиях веток. Модель там не нужна: словарь синонимов +и так написан в тексте шага, то есть решение уже детерминированное. + +**Замер** (десять живых запросов, словарь писался до того, как запросы были +извлечены — проверка на подгонку): + +| исполнитель | точных совпадений | +|---|---:| +| модель, `temperature: 0` | 5 / 10 | +| тот же словарь условием | **10 / 10** | + +Три редакции инструкции потолок не подняли. Промахи модели однотипны: +сваливается в умолчание, теряя НАЗВАННОЕ — «поищи про X в вики и в жире» → +умолчание вместо двух названных источников. + +```yaml +- - name: probe_docs + when: input contains вики | wiki | confluence | документац + delegate: {skill: docs-find, task: "{{input}}", on_error: continue} +``` + +**Корни в словаре делай ДЛИННЫМИ.** `contains` требует начала слова, но не +конца: «заказ» находит «заказы», «заказа», «заказу» — ради этого и сделано, и +поэтому формату не нужен стемминг. Цена — слишком короткий корень цепляет +постороннее: «под» найдётся в «подготовь», «код» в «кодировке». Лечится длиной +корня, а не настройкой. + +**Умолчание — отдельная ветка.** «Ни один источник не назван» выражается +`not contains` по ВСЕМУ словарю сразу, а не по словам одного источника: + +```yaml +- - name: probe_docs_default + when: input not contains <весь словарь всех источников> +``` + +--- + +## I5. Имя инструмента в тексте требует того, ЧЕМ его звать + +Если в инструкции шага назван инструмент, шаг обязан объяснить, ЧЕМ его звать: +на пути программы модель видит не описания инструментов, а тот вызывающий +инструмент, который дало приложение (линтер знает его имя как +`Options.CallProtocol`). Иначе модель зовёт инструмент по имени и получает «tool +not found» — живой случай: `search` вместо `tracker_search`. + +> Статика: **W12**. + +--- + +## I6. Ссылайся только на существующие переменные + +`{{имя}}`, которого в этом месте потока нет, движок молча разрешает в пустую +строку. Ход при этом не падает — он отвечает так, будто вопрос был другим. + +Область видимости **плоская**: ветка исполняется в том же состоянии, объявленное +внутри ветки видно после неё. Не строй скилл на предположении, что переменные +ветки изолированы. + +> Статика: **W14**. На первом же прогоне поймала живой баг: шаг писал в `lang`, +> инструкция ниже звала `{{detect_lang}}`. + +--- + +## I7. Имя шага — часть трассы + +Имена шагов попадают в трассу хода (`Outcome.Steps`), по ним читают, где ход +развалился, и по ним же ищут закономерности на месяце данных. Называй шаги +глаголом по существу (`understand`, `fetch_brief`, `resolve_pod`, +`report`), одинаковые роли — одинаково во всех скиллах: сравнимость важнее +изобретательности. diff --git a/handbook/04-context-and-cost.md b/handbook/04-context-and-cost.md new file mode 100644 index 0000000..62c7aa8 --- /dev/null +++ b/handbook/04-context-and-cost.md @@ -0,0 +1,132 @@ +# Что едет в контекст модели и сколько это стоит + +> Расход хода — не число шагов, а объём, проходящий через контекст модели, помноженный на число генераций. + +## Форма + +```yaml +# Знание — в шаг БЕЗ инструментов, данные — мимо контекста, ссылкой на хендл. +- name: decide + instruction: "{{asset:index_map}} … выбери индекс и период" + tools: [] # знание едет один раз, а не в каждую генерацию + save_as: plan + +- name: fetch + call: {tool: logs:search, args: {index: "{{plan.index}}"}, save_as: hits} + +- name: crunch + call: + tool: runner:exec + args: + stdin: {from: "{{hits.mem}}"} # данные идут МИМО модели + code: "{{asset:count_by_namespace}}" # код ассетом, не в промпте +``` + +--- + +Главный расход скилла — не число шагов, а объём, который проходит **через +контекст модели**, помноженный на число генераций. + +--- + +## C1. Знание дорого в шаге С ИНСТРУМЕНТАМИ + +**Правило.** Большой кусок знания (справка по индексам, правила формата запроса, +таблица соответствий) кладут в шаг **без** инструментов. В шаге с инструментами +он едет в **каждую** генерацию react-цикла. + +Живой замер: скилл поиска по логам со знанием в шаге с инструментами проиграл +прозе — 36k → 43k токенов. Это единственный скилл каталога, который после +перевода на программу стал дороже, и причина ровно эта. + +**Как правильно** — разделить «решить» и «выполнить»: + +```yaml + - name: decide # знание, tools: [] — одна генерация + instruction: "{{asset:index_map}} … выбери индекс и период" + tools: [] + response_schema: {...} + save_as: plan + + - name: fetch # инструменты, без знания + call: {...} +``` + +--- + +## C2. Ассет по ссылке не проходит через контекст, ассет в тексте — проходит + +```yaml +args: + code: "{{asset:versions_parser}}" # ← ТЕКСТ: уедет в промпт целиком +args: + stdin: {from: "asset:big_table"} # ← ССЫЛКА: подставится хост-сайдом +``` + +Код парсера, который едет ссылкой, модель не переписывает и не «улучшает». Пока +он жил в теле промпта, скилл был вынужден просить «возьми код ДОСЛОВНО, НЕ +сжимай» — просьбу, которую модель нарушала тем чаще, чем длиннее код. + +--- + +## C3. Данные между шагами передавай хендлом `.mem` + +У результата любого шага есть хендл рабочей памяти — **всегда**, не только у +крупного: + +```yaml + - name: crunch + call: + tool: runner:exec + args: + stdin: {from: "{{pods.mem}}"} # данные идут мимо модели + code: "{{asset:count_by_namespace}}" +``` + +Без `.mem` в промпт уедет весь ответ инструмента — выборка подов по всем +namespace в контекст просто не влезет, а проходить через него ей и не нужно. + +> Статика: **W10** — `from:` обязан ссылаться на хендл. + +--- + +## C4. На пути программ модель не видит схем инструментов + +Шаг `instruction` с непустым `tools` видит вызывающий инструмент приложения, а +не полные описания инструментов с их JSON-схемами и `enum` (в отличие от пути, +на котором модель получает описания целиком). + +Следствие: **то, чего нет в схеме перед глазами, модель придумывает**. Живые +промахи прогона 02.08: + +| что вызвала модель | чем кончилось | +|---|---| +| `search_code {"scope": "all"}` | 400 «scope does not have a valid value» ×3 → шаг degraded | +| `get_pipeline {"project_id": "YOUR_PROJECT_ID", "pipeline_id": 12345678}` | буквальный плейсхолдер и выдуманный id | +| `search` на сервере трекера | «tool not found» (правильное имя — `tracker_search`) | + +**Как правильно.** В шаге с инструментами называй словами: сервер, точное имя +инструмента и обязательные аргументы с допустимыми значениями. А лучше — унеси +вызов в шаг `call`, где аргументы записаны в YAML и придумывать нечего. + +--- + +## C5. Один шаг — одно действие + +`instruction` (генерация), `call` (вызов без генерации), `set`, `switch`, `if`, +`for_each`, `parallel`, `delegate`, `exit`. Шаг, который «сходит и заодно +разберёт», превращается в react-цикл с непредсказуемым числом генераций. + +Практическое: детерминированный вызов, аргументы которого известны заранее, — +это `call`, а не «попроси модель позвать». Скилл про логи берёт список подов +`call`-шагом ВСЕГДА, потому что имя из запроса почти никогда не полное, а сам +вызов ничего не решает. + +--- + +## C6. Параллель — для независимых веток, и она обязана считать пропуски + +`parallel` с `collect:` даёт шагу ответа не только результаты веток, но и +`.skipped` — список веток, пропущенных по `when`. Без него шаг ответа +не различает «источник ответил пусто» и «в источник не ходили», а это разные +ответы пользователю. diff --git a/handbook/05-verification.md b/handbook/05-verification.md new file mode 100644 index 0000000..2d4f33a --- /dev/null +++ b/handbook/05-verification.md @@ -0,0 +1,138 @@ +# Как проверять скилл + +> Порядок от секунд к часам: смысл не в том, чтобы ускорить полный прогон, а в том, чтобы он реже был нужен. + +## Форма + +``` +1. Flow.Validate + линтер — секунды, без модели +2. шаги со схемой на живой модели — минуты, входы НЕПОЛНЫЕ (V2) +3. сквозной прогон скилла — минуты, с инструментами +4. набор живых запросов — то, что реально спрашивают, а не примеры +``` + +Каждый слой ловит своё, и первые два ловят большую часть — без них четвёртый +превращается в способ узнавать про опечатки за полтора часа. + +--- + +Порядок от секунд к часам. Смысл — не ускорять полный прогон, а **уменьшать +нужду** в нём: почти всё, что ломается, ловится задолго до него. + +| слой | время | что ловит | +|---|---|---| +| `Flow.Validate` + линтер (`W1`–`W16`) | секунды, без модели | объявленные противоречия: схема против инструкции, ссылки на несуществующее, конверты, потолки | +| прогон отдельных шагов на модели | минуты, только модель | то, что видно лишь на живой модели: обрыв, неразбираемый ответ, выдуманное значение | +| сквозной прогон скилла | минуты | ход целиком, с инструментами | +| набор живых запросов | ~10 минут | поведение на том, что реально спрашивают | +| весь каталог | часы | общий срез перед релизом | + +--- + +## V1. Линтер — первым, всегда + +Он бесплатный и ловит целые классы: `W9`/`W13`/`W16` (схемы), `W10` (хендлы), +`W12` (имена инструментов), `W14` (переменные), `W7`/`W15` (встроенные тулы). +Красный линтер — это не «придирка», это отказ, который иначе доедет до +пользователя. + +--- + +## V2. Прогон шагов на модели: подавай входы БЕЗ слотов + +Шаг разбора запроса — это один вызов модели с фиксированным промптом. Ни +инструментов, ни инфраструктуры, ни деплоя для его проверки не нужно: берём +инструкцию шага, подставляем вопрос, зовём модель с той же схемой и смотрим, +доехал ли ответ (генерация не оборвана по длине, JSON разбирается). + +Каталог из 28 скиллов проверяется так весь — **208 входов за 50 секунд**. + +**Главное правило такой проверки: вход должен быть НЕПОЛНЫМ.** Первая версия брала по +два первых `trigger_examples` и давала зелёный, ни разу не задев проверяемый +путь — потому что первые примеры самые полные: + +``` +покажи поды в неймспейсе billing ← слот есть, ход исправен +покажи поды в кубах биллинга ← слот есть +какие поды в проде ← слота НЕТ, вот здесь и ломается (3-й!) +``` + +**Примеры скилла — это его идеальные входы, а ломается он на краях.** Настоящие +края лежат в живых запросах: возьми вопросы, которые реально сроутились в скилл +(в том числе продолжения разговора — «короче», «а какие там деплойменты», +«вернись к задачам»), и подай их шагу. Таких формулировок в примерах не +будет никогда. + +--- + +## V3. Негативный контроль обязателен + +Зелёный тест ничего не стоит, пока не показано, что он умеет краснеть. Верни +дефект (поставь поле обратно в `required`), прогони, убедись, что тест падает +именно на том входе, — и только потом верни починку. + +На этом же держится доверие к самой проверке: с восстановленным +`namespace: required` она упала ровно на «какие поды в проде», воспроизведя за 3 +секунды то, что полный прогон показывал полтора часа и в виде «упал инструмент». + +**Мало «покраснел» — важно, ОТ ЧЕГО.** Проверка, целящаяся в подстроку, краснеет +не от того, что проверяет. Замер 14.08: тест цикла сборки прошёл четыре мутации +из четырёх — но первая его редакция ОДНУ пережила. Она искала слово где угодно в +результате и была довольна текстом ответа человека, пока поле источников молча +не сохранялось. Целься в поле, а входные данные подбирай так, +чтобы искомое не могло прийти из соседней строки. + +--- + +## V4. Перепроверяй по СКИЛЛУ, а не по упавшему кейсу + +Правка схемы меняет скилл для всех кейсов, где он участвует, а не только для +тех, что упали. Гоняй все кейсы затронутого скилла — иначе регрессия в +сегодня-зелёных пройдёт незамеченной. + +--- + +## V5. Проверяй измеритель раньше измеряемого + +Ошибка в измерителе не выглядит ошибкой — она выглядит результатом, и тем +убедительнее, чем лучше совпадает с ожиданием. За один вечер разбора я намерил +ерунду трижды: + +| что «нашлось» | чем оказалось | +|---|---| +| правило линтера поймало 6-й скилл | подстрока «пуст» внутри слова «переза**пуст**ить» (в Go `\b` ASCII-only и на кириллице не работает) | +| «шаг всегда отвечает robot» | в тестовом тикете человеку был вписан адрес вида `noreply@` | +| «на всё отвечает robot» | кавычки shell в цикле прогона — в запрос уходил не тот текст | + +Что помогает: +1. открыть сырые данные тех 2–3 случаев, на которых держится вывод; +2. спросить, могла ли метрика сработать по другой причине; +3. **брать проверяемый текст из самого скилла, а не копией** — копия молча + разойдётся с боевой, и проба подтвердит формулировку, которой в бою нет; +3a. **брать ВХОД из живого отказа целиком, а не писать по памяти** — замер + 14.08: починка формы прошла свой тест и не сработала в бою, потому что в + фикстуре, написанной по памяти, не было `save_as`, а он есть почти у каждого + шага. Отказ у нас записан — бери его дословно; +4. сверять вывод с живыми событиями: гипотеза «шаг всегда отвечает robot» + умерла об один запрос к ним — ветка робота срабатывала в 7 сессиях из 27. + +--- + +## V6. Что читать в трассе после прогона + +Движок возвращает `Outcome` — трассу каждого шага, а не текст, который надо +вычитывать глазами: + +| сигнал | значение | +|---|---| +| `Outcome.Steps[i].Outcome ∈ {error, degraded}` | шаг не отработал | +| `denied` | шаг не пошёл: не хватило прав | +| `skipped` вместе с `Outcome.Skipped` | шаг не подошёл по `when` — задача совпала со скиллом частично | +| генерация оборвана по длине | потолок токенов; ищи S1/S3 | +| хвост ответа из одних пробелов | тот самый разгон (см. [01](01-response-schema.md), S1) | + +Последние два приложение видит в собственных событиях вокруг вызова модели — +движок про них не знает. + +Отдельно стоит смотреть ходы, где все шаги `ok`, а ответ не опирается на +добытые данные: трасса такое не ловит, ловит только проверка самого ответа. diff --git a/handbook/06-flow-shape.md b/handbook/06-flow-shape.md new file mode 100644 index 0000000..d194c17 --- /dev/null +++ b/handbook/06-flow-shape.md @@ -0,0 +1,115 @@ +# Форма программы + +> Где в описании стоит шаг, что читает условие и что видно после ветки. + +## Форма + +```yaml +workflow: + tools: [tracker] + steps: + - name: understand # разбор запроса — до развилок + instruction: … + tools: [] + save_as: req + + - name: record # то, что обязано случиться ВСЕГДА, — тоже до развилок + call: {tool: tracker:note, args: {text: "{{input}}"}} + + - name: route # и только теперь ветвление + if: + cond: "req.status.retries > 0" + then: [ … ] + else: [ … ] +``` + +--- + +Правила про то, ГДЕ в описании стоит шаг и что условие может прочитать. Сюда же +примыкает C5 («один шаг — одно действие») из +[04-context-and-cost.md](04-context-and-cost.md) — там она оказалась по +соседству с ценой контекста, но по смыслу про то же. + +--- + +## P1. Действие, обязательное при каждом входе, стоит ДО развилок + +**Правило.** Если шаг обязан случиться всегда, он не может жить внутри ветки. +Ставь его до первого `if`/`switch`, а решать, есть ли работа, пусть инструмент. + +**Почему.** Ветка исполняется не всегда — в этом её смысл. Шаг, который обязан +случиться независимо от исхода, попав внутрь ветки, не случается ровно в тех +случаях, когда ветка не сработала, а это обычно и есть интересные случаи. + +Живой круг 14.08, многоходовая сборка. Ветка «чего-то не хватает» записывала +ЗАДАННЫЙ ВОПРОС, а записать ОТВЕТ человека должен был шаг ниже по потоку — до +которого эта ветка не доходит никогда: + +``` +ход 1: спросили → в артефакт лёг вопрос без ответа +ход 2: ответили → записать некому; разбор видит «спросили и не ответили» +ход 3: спросили снова, другими словами → и так далее +``` + +Реплика человека терялась целиком. Лечится переносом записи ДО развилок: шаг +зовётся всегда, а есть ли что закрывать — решает инструмент (незакрытого вопроса +нет → вызов ничего не меняет). + +**Как узнать заранее.** Спроси про каждый шаг: «если сюда не дойдут, что +сломается на СЛЕДУЮЩЕМ входе?» Если ответ «потеряется то, что человек сказал», — +шаг стоит не там. + +--- + +## P2. Условие читает ПУТЬ, но выбирает ОДНО значение + +**Правило.** `cond` читает переменную и путь внутрь её значения любой глубины, +включая элемент списка по индексу. Сравнивать можно на равенство, на пустоту, на +слова (`contains`) и на число (`>`, `>=`, `<`, `<=`). + +```yaml +- name: maybe_logs + if: + cond: "pod.status.containerStatuses[0].restartCount > 5" + then: + - name: logs + call: {tool: k8s:pod_logs, args: {name: "{{pod.metadata.name}}"}} +``` + +Чего нет: выбора многих элементов (`[*]`, фильтры) и арифметики. Путь однозначен +— он либо разрешается в одно значение, либо нет; выражения принесли бы +приоритеты, экранирование и ошибки времени исполнения, то есть формат перестал +бы читаться сверху вниз. `[*]` отвергается отдельным сообщением: пройди список +`for_each` и положи условие в тело. + +**Откуда правило.** Замер 14.08, 16 генераций описания шагов: шесть отказов +формата, **пять из шести — один и тот же случай**, и ни один не про арифметику: + +``` +pod_details.status.containerStatuses[0].restartCount > 0 +``` + +Модель не ошибалась в том, ГДЕ лежит число: столько уровней и отдаёт источник. +Столько не читал сам формат — до 2.4.0 условие видело один уровень, и каждое +вложенное поле стоило лишнего шага со структурным ответом. Если правишь скилл +старше — этот обход можно убрать, объявив `skill_engine_version: 2.4.0`. + +**Промах пути — отказ шага, а не пустота.** `a.b.c` при отсутствующем `b` было +бы неотличимо от «значение пустое», и ветка молча ушла бы не туда. Шаг падает, а +в отказе сказано, где путь оборвался и какие поля у объекта есть на самом деле. +Исключение — голое имя и одиночное `var.field`: они остались тихими, потому что +на этом обещании написаны скиллы старше 2.4.0. + +--- + +## P3. Область видимости переменных ПЛОСКАЯ + +**Правило.** Ветка исполняется в том же состоянии, что и остальная программа: +переменная, объявленная внутри `then`, видна после всей развилки. Не рассчитывай +на изоляцию и не заводи одинаковые имена в разных ветках, надеясь, что они не +столкнутся. + +**Почему.** Так устроен движок, и всякая проверка, предположившая обратное, врёт. + +Замер: правило линтера, написанное в догадке об изоляции ветвей, дало 42 находки +на живом каталоге — настоящей среди них была одна. diff --git a/handbook/README.md b/handbook/README.md new file mode 100644 index 0000000..9b4da77 --- /dev/null +++ b/handbook/README.md @@ -0,0 +1,74 @@ +# Как писать скиллы-программы + +Справочник по живым отказам. Каждое правило здесь оплачено сломанным ходом в +проде или в прогоне — ни одного «из общих соображений». + +## Кому + +Человеку, который пишет или правит скилл, и **инструменту, который делает то же +самое**: если приложение даёт агенту собирать и чинить скиллы, ошибаются они в +одних и тех же местах, и места эти перечислены ниже. + +Поэтому справочник лежит В МОДУЛЕ и доступен из кода — иначе указатель на знание +ведёт туда, куда адресат не может пойти: + +```go +for _, s := range skillengine.HandbookIndex() { + fmt.Println(s.ID, "—", s.Summary) // индекс дешёвый, его кладут в промпт целиком +} +text := skillengine.Handbook("flow-shape") // раздел — по запросу +``` + +Разделами, а не целиком: справочник — 70 КБ, в промпт шага он не влезает и не +должен. Смысл в том, чтобы посмотреть нужный кусок. + +## Как устроено + +| id | файл | про что | +|---|---|---| +| `response-schema` | [01-response-schema.md](01-response-schema.md) | структурный ответ шага: `required`, потолки, enum, грамматика | +| `failures` | [02-failures.md](02-failures.md) | отказы: `on_error`, ветка пустоты, громкость, выход | +| `instruction-text` | [03-instruction-text.md](03-instruction-text.md) | текст шага: порядок вопросов, подсказки, что НЕ пишут в инструкцию | +| `context-and-cost` | [04-context-and-cost.md](04-context-and-cost.md) | что едет в контекст модели и сколько это стоит | +| `verification` | [05-verification.md](05-verification.md) | как убедиться, что скилл работает, не тратя полтора часа | +| `flow-shape` | [06-flow-shape.md](06-flow-shape.md) | форма программы: где стоит шаг, что читает условие, видимость переменных | + +Каждый раздел начинается с **готовой формы**: куском, который копируют целиком, +а не объяснением, как надо. Так пришлось сделать по замеру — прозаическая +подсказка «сделай отбор отдельным шагом» не сработала ни разу из шестнадцати, а +пример из каталога копируется. + +Дальше правила, и каждое подано так: **что делать → почему (какой отказ это +стоило) → как выглядит правильно**. Где отказ ловится статикой, назван номер +правила линтера (`W…`) — тогда достаточно прогнать линтер, а не думать. + +## Одно правило поверх всех + +**Скилл-программа отличается от прозы тем, что невозможное в ней НЕВОЗМОЖНО, а +не нежелательно.** Проза просит («не вызывай retract без подтверждения»), и +просьба исполняется настолько, насколько модель дочитала до неё, прежде чем +начать действовать. Программа не просит: в неподтверждённой ветке вызова просто +нет. + +Отсюда способ проверять свой скилл: найди в нём каждое место, где ты **просишь** +модель что-то сделать или не сделать, и спроси, нельзя ли это выразить +ветвлением, схемой или отсутствием инструмента. Обычно можно, и это всегда +надёжнее. + +## Что считать источником истины + +- контракт формата — [`skill.schema.yaml`](../skill.schema.yaml) (и русский + [`skill.schema.ru.yaml`](../skill.schema.ru.yaml)); **читать надо его, а не + исходники движка**: поведение бывает описано там прямо, а по коду + достраивается неверно (см. F1 — на этом обжигались); +- версия формата объявляется скиллом: `skill_engine_version`, и чужой мажор + движок отвергает в обе стороны; +- **имена и типы полей живут в схеме, и только там.** Справочник про + УПОТРЕБЛЕНИЕ и классы отказов — то, чего в схеме нет и быть не может. Поле, + названное здесь, обязано существовать в схеме; это проверяется тестом. + +## На каком языке + +Справочник написан по-русски, как и его источник — разборы живых отказов. Схема +формата двуязычна (`skill.schema.yaml` и `skill.schema.ru.yaml`), README и +QUICKSTART тоже; справочник пока нет. diff --git a/handbook_test.go b/handbook_test.go new file mode 100644 index 0000000..603b480 --- /dev/null +++ b/handbook_test.go @@ -0,0 +1,281 @@ +package skillengine_test + +import ( + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + se "github.com/inhuman/skill-engine" + "gopkg.in/yaml.v3" +) + +// The handbook must ARRIVE, which is the whole reason it moved out of the +// ignored spec tree: an embedder gets this package through `go mod vendor`, +// and vendor copies only what the build references. +func TestHandbookIsEmbedded(t *testing.T) { + index := se.HandbookIndex() + require.NotEmpty(t, index, "the handbook did not travel with the package") + + onDisk, err := filepath.Glob("handbook/[0-9]*.md") + require.NoError(t, err) + assert.Len(t, index, len(onDisk), "a section on disk is missing from the index, or the other way round") + + for _, s := range index { + assert.NotEmpty(t, se.Handbook(s.ID), "section %q is indexed and empty", s.ID) + } + assert.Empty(t, se.Handbook("no-such-section"), + "an unknown id must yield nothing rather than the nearest match") + assert.Empty(t, se.Handbook("README"), "the table of contents is not a section") +} + +// The index is what travels in EVERY prompt, so it has to stay cheap: the point +// of splitting the handbook by section is that the whole of it (70 KB) does not +// go into a step. An index that grows into a document defeats that. +func TestHandbookIndexIsCheap(t *testing.T) { + total := 0 + for _, s := range se.HandbookIndex() { + assert.NotEmpty(t, s.Title, "section %q has no heading", s.ID) + require.NotEmptyf(t, s.Summary, "section %q has no `> summary` line under its heading", s.ID) + assert.LessOrEqualf(t, len([]rune(s.Summary)), 120, + "the summary of %q is a paragraph; the index needs one line", s.ID) + total += len(s.ID) + len(s.Title) + len(s.Summary) + } + assert.Less(t, total, 3000, "the index stopped being something you can put in a prompt whole") +} + +// An id is what a refusal prints, so it must not move when a section is +// inserted before another: the number in the file name orders the files and is +// not part of the name. +func TestHandbookIDsAreStableAndUnique(t *testing.T) { + seen := map[string]bool{} + for _, s := range se.HandbookIndex() { + assert.NotContains(t, s.ID, ".md") + assert.Regexp(t, `^[a-z][a-z0-9-]*$`, s.ID) + assert.Falsef(t, seen[s.ID], "two sections share the id %q", s.ID) + seen[s.ID] = true + } +} + +// Every section opens with a FORM — a piece to copy, not an explanation of how +// to write one. Paid for by measurement: the prose hint "if you need a subset, +// make it a separate step" worked 0 times out of 16, while an example from the +// catalogue gets copied. +func TestEverySectionOpensWithAForm(t *testing.T) { + for _, s := range se.HandbookIndex() { + text := se.Handbook(s.ID) + head, _, _ := strings.Cut(text, "\n---\n") + assert.Containsf(t, head, "## Форма", "section %q does not open with a form", s.ID) + assert.Containsf(t, head, "```", "the form of %q has nothing to copy", s.ID) + } +} + +// schemaKeyword — what may appear as `name:` in the handbook without being a +// field of the skill format: the JSON-Schema vocabulary a response_schema is +// written in, and the two argument conventions the format documents in prose +// rather than as properties. +var schemaKeyword = map[string]bool{ + "type": true, "properties": true, "required": true, "enum": true, + "items": true, "maxlength": true, "maxitems": true, "minitems": true, + "additionalproperties": true, "format": true, "default": true, + "description": true, "from": true, "stdin": true, "code": true, +} + +// fieldRe — a claimed field of the format: `on_error: continue` inside +// backticks, taken up to the first colon, with what follows it. +var fieldRe = regexp.MustCompile("`([a-z_][a-z0-9_]*):\\s*([^`]*)") + +// declaresAType — `id: integer`, `strengths: items: {type: string}`. A name +// followed by a JSON-Schema type is a field of the SKILL AUTHOR's own +// response_schema, invented for one example, and the format knows nothing about +// it. A name followed by a value (`on_error: continue`, `tools: []`) is a field +// of the format being used, and that is what this guard is about. +var declaresAType = regexp.MustCompile(`^(integer|boolean|string|number|array|object|enum|items|required|\{type)\b`) + +// The handbook must not become a SECOND source of truth about the format. +// +// Names and types of fields live in the schema and only there: a handbook that +// lists fields diverges from it on the first change, and the reader cannot tell +// which of the two is lying. Its subject is USE and failure classes — exactly +// what the schema has no room for. +// +// So: a section naming a field must take the name from the schema, or not name +// it. This is the test that keeps that true. +func TestHandbookNamesOnlyFieldsTheSchemaHas(t *testing.T) { + known := schemaFieldNames(t) + for _, s := range se.HandbookIndex() { + for _, m := range fieldRe.FindAllStringSubmatch(se.Handbook(s.ID), -1) { + name := m[1] + if schemaKeyword[strings.ToLower(name)] || declaresAType.MatchString(strings.TrimSpace(m[2])) { + continue + } + assert.Truef(t, known[name], + "section %q writes `%s:` and the schema has no such field — "+ + "either the name is wrong or the schema is the one that changed", s.ID, name) + } + } +} + +// schemaFieldNames — every property name the schema defines, at any level. +func schemaFieldNames(t *testing.T) map[string]bool { + t.Helper() + var doc any + require.NoError(t, yaml.Unmarshal([]byte(se.SchemaYAML), &doc)) + + out := map[string]bool{} + var walk func(node any, underProperties bool) + walk = func(node any, underProperties bool) { + switch v := node.(type) { + case map[string]any: + for k, nested := range v { + if underProperties { + out[k] = true + } + walk(nested, k == "properties") + } + case []any: + for _, nested := range v { + walk(nested, false) + } + } + } + walk(doc, false) + require.NotEmpty(t, out) + return out +} + +// privateNames — the installation's own names, read from the list the git hooks +// share. +// +// NOT spelled out here, and that is the whole point: this file is public, so a +// list of private names inside it would publish exactly what it exists to keep +// out. The repository had already decided that — `.githooks/private-names` says +// so in its own header — and the pre-commit hook refuses any tracked file that +// writes such a name, this one included. It fired on the first version of this +// test, which is how the list got here in the first place. +// +// So there is one copy, in a directory that is gitignored, and this reads it. +// Only the NAMES, not the shape heuristics beside them: those are tuned for the +// added lines of a commit, and over whole files they fire on an e-mail in a +// licence or a hostname in prose — a check that always fires is one people +// switch off. +// +// Without the file (a fresh clone, CI) there is nothing to check against, and +// the test says so rather than passing quietly. The authoritative check is the +// hook: `git config core.hooksPath .githooks`. +func privateNames(t *testing.T) *regexp.Regexp { + t.Helper() + raw, err := os.ReadFile(filepath.Join(".githooks", "private-names")) + if err != nil { + t.Skip("нет .githooks/private-names — список приватных имён живёт только там; " + + "включается через git config core.hooksPath .githooks") + } + var parts []string + for line := range strings.SplitSeq(string(raw), "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "PRIVATE_NAMES=") { + continue + } + body := strings.Trim(strings.TrimPrefix(line, "PRIVATE_NAMES="), `'"`) + body = strings.TrimPrefix(body, "$PRIVATE_NAMES|") + parts = append(parts, body) + } + require.NotEmpty(t, parts, "список приватных имён пуст — сторож охранял бы пустоту") + re, err := regexp.Compile("(?i)" + strings.Join(parts, "|")) + require.NoError(t, err) + return re +} + +// publicFiles — the files that will actually SHIP: what git tracks, plus what is +// untracked and not ignored. +// +// Asking git rather than walking the tree is not convenience, it is the +// definition. The first version of this guard walked everything and reported +// `specs/` — a directory that is in .gitignore and goes nowhere, holding +// working copies of the consuming application's sources. A guard that names +// files nobody publishes is a guard that gets switched off. +func publicFiles(t *testing.T) []string { + t.Helper() + out, err := exec.Command("git", "ls-files", "--cached", "--others", "--exclude-standard").Output() + if err != nil { + t.Skip("git недоступен — списка публикуемых файлов взять неоткуда") + } + var files []string + for _, path := range strings.Split(strings.TrimSpace(string(out)), "\n") { + switch filepath.Ext(path) { + case ".go", ".md", ".yaml", ".yml": + // This file carries both lists as data; it cannot be its own subject. + if filepath.Base(path) != "handbook_test.go" { + files = append(files, path) + } + } + } + require.NotEmpty(t, files) + return files +} + +// Публичный файл не имеет права называть чужую установку. +// +// Шире хука по охвату и уже по времени: хук смотрит ДОБАВЛЕННЫЕ строки коммита, +// этот тест — целые файлы, то есть находит и то, что доехало раньше, чем список +// пополнился. Так и нашлись `gitlab-write-prod` и `k8s-job` в фикстурах. +func TestNoPublicFileNamesAnInstallation(t *testing.T) { + private := privateNames(t) + for _, path := range publicFiles(t) { + text, err := os.ReadFile(path) + if err != nil { + continue // удалён между листингом и чтением — не наше дело + } + for _, hit := range private.FindAllIndex(text, -1) { + // Своя печать вместо assert.Contains: тот при промахе вываливает + // ВЕСЬ файл, и одна находка залила бы вывод так, что остальных в + // нём не разглядеть. Имя не печатается — оно приватное; печатается + // адрес, по которому его видно. + t.Errorf("%s:%d называет имя из .githooks/private-names — это существует ровно в одной установке", + path, strings.Count(string(text[:hit[0]]), "\n")+1) + } + } +} + +// hostVocabulary — words that belong to ONE installation and must not ride into +// the module with the handbook. +// +// Five published versions of this library were retracted for exactly this: they +// carried the vocabulary of the installation the format grew in — word lists +// baked into the engine and the linter, an example built from one application's +// dictionary. The handbook arrived from that same installation, and it named +// that application's tools, telemetry fields, clusters and skills as though +// they were the format's. +// +// The list is short and specific on purpose. It is not a filter for prose about +// products (a live case is allowed to say it happened in a tracker), and it +// holds no private NAMES — those come from the hooks' own list, see +// privateNames above. What is left is STYLE: words that read as written inside +// one deployment, and that reappear when the handbook is re-synced from there. +var hostVocabulary = []string{ + "mcp_call", "mcp_list_servers", "mcp-exec", + "skill_step", "skill_program", "subagent_llm_call", "content_excerpt", "content_tail", + "GRAMMAR_CAPABLE_MODELS", "vllm/", + "шейкдаун", "батаре", "принципал", "субагент", "оркестратор", +} + +func TestHandbookCarriesNoInstallationVocabulary(t *testing.T) { + files, err := filepath.Glob("handbook/*.md") + require.NoError(t, err) + require.NotEmpty(t, files) + + for _, path := range files { + text, err := os.ReadFile(path) + require.NoError(t, err) + low := strings.ToLower(string(text)) + for _, word := range hostVocabulary { + assert.NotContainsf(t, low, strings.ToLower(word), + "%s carries %q — a name that means something in one installation only", path, word) + } + } +} diff --git a/lint/README.md b/lint/README.md index 6779d56..ef59519 100644 --- a/lint/README.md +++ b/lint/README.md @@ -125,6 +125,20 @@ package refuses to produce. package: while rules were being added on both sides of the boundary they collided, and a number meaning two things in two places is worse than no number. +## Where a finding sends the reader + +A rule that catches a class the handbook covers carries its section id — on the +rule (`Rule.Handbook`) and on every finding it makes (`Finding.Handbook`). The +text comes from the engine: `skillengine.Handbook(id)`, with +`skillengine.HandbookIndex()` listing what there is. + +It is a field rather than a sentence glued onto the message, because whoever +assembles the refusal decides what to do with it: a person reads "see also", a +tool fetches the section and puts the form in front of the model that is writing +the skill. That second half is the point — the previous version of this pointer +said "call the schema tool", and on the program path that tool was in no skill's +radius, while the steps that write skills run with `tools: []`. + ## What it does not check, and why - **Judgement by a model** — do two skills claim the same requests, is the diff --git a/lint/catalogue.go b/lint/catalogue.go index ad4dd99..e6eb697 100644 --- a/lint/catalogue.go +++ b/lint/catalogue.go @@ -32,6 +32,15 @@ type Rule struct { // missing would switch off checks that never needed it — which is exactly // the failure that split W2 into two passes. Needs []string + // Handbook — the id of the handbook section that covers this class of + // failure, empty where none does. `skillengine.Handbook(id)` returns its + // text, and every finding of the rule carries the same id. + // + // The mapping lives here rather than beside each call site for the reason + // the catalogue exists at all: a rule that points at a section nobody can + // name, or at one that has been renamed, is worse than a rule that points + // nowhere. A test checks every id against the handbook the module ships. + Handbook string } // Rules returns the catalogue, ordered by id. @@ -41,64 +50,78 @@ func Rules() []Rule { Title: "the file parses, the header is legal, and the format version is one this engine speaks"}, {ID: "S3", Emits: []Severity{SeverityError}, Needs: []string{"Options.StaleAPIs"}, Title: "the playbook uses a construct the embedder has removed"}, - {ID: "S5", Emits: []Severity{SeverityWarn}, + {ID: "S5", Handbook: "context-and-cost", Emits: []Severity{SeverityWarn}, Title: "the playbook's size against the budget — it is context weight on every run"}, {ID: "S6", Emits: []Severity{SeverityInfo}, Title: "no trigger_examples: the skill is only reachable by being named outright"}, {ID: "W1", Emits: []Severity{SeverityError}, Title: "the description does not pass the engine's own validation"}, - {ID: "W2", Emits: []Severity{SeverityError}, Needs: []string{"Facts.ServerNames"}, + {ID: "W2", Handbook: "failures", Emits: []Severity{SeverityError}, Needs: []string{"Facts.ServerNames"}, Title: "a server the program names is declared by the skill (needs nothing) and registered (needs the registry)"}, {ID: "W3", Emits: []Severity{SeverityError}, Needs: []string{"Facts.AllTools"}, Title: "a call step's tool exists on its server"}, - {ID: "W4", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.Assets"}, + {ID: "W4", Handbook: "context-and-cost", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.Assets"}, Title: "an asset is passed the way its kind implies — through the model's context or past it"}, {ID: "W5", Emits: []Severity{SeverityError}, Needs: []string{"Facts.ToolSchemas"}, Title: "a call step carries the arguments its tool requires"}, - {ID: "W6", Emits: []Severity{SeverityError}, + {ID: "W6", Handbook: "flow-shape", Emits: []Severity{SeverityError}, Title: "somebody writes into the variable a loop collects"}, - {ID: "W7", Emits: []Severity{SeverityError}, + {ID: "W7", Handbook: "failures", Emits: []Severity{SeverityError}, Title: "a built-in tool called by a step is declared in builtin_tools"}, - {ID: "W8", Emits: []Severity{SeverityError, SeverityWarn}, Needs: []string{"Options.Envelopes"}, + {ID: "W8", Handbook: "context-and-cost", Emits: []Severity{SeverityError, SeverityWarn}, Needs: []string{"Options.Envelopes"}, Title: "a wrapped call result is substituted whole where a field was meant"}, - {ID: "W9", Emits: []Severity{SeverityError}, + {ID: "W9", Handbook: "response-schema", Emits: []Severity{SeverityError}, Title: "an object in a response schema has at least one required field"}, - {ID: "W10", Emits: []Severity{SeverityError}, + {ID: "W10", Handbook: "context-and-cost", Emits: []Severity{SeverityError}, Title: "`from:` in a call's arguments receives a handle, not the value's text"}, - {ID: "W11", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.Assets"}, + {ID: "W11", Handbook: "context-and-cost", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.Assets"}, Title: "an asset's params are keys the resolver actually reads"}, - {ID: "W12", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.CallProtocol", "Facts.AllTools"}, + {ID: "W12", Handbook: "instruction-text", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.CallProtocol", "Facts.AllTools"}, Title: "an instruction names a tool without saying how tools are called — ENTIRELY dependent on a live tool listing, so it does not run offline"}, - {ID: "W13", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.FreeTextFields"}, + {ID: "W13", Handbook: "response-schema", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.FreeTextFields"}, Title: "a free-text field of a response schema has a length ceiling (fields inside arrays need no vocabulary)"}, - {ID: "W14", Emits: []Severity{SeverityError}, + {ID: "W14", Handbook: "instruction-text", Emits: []Severity{SeverityError}, Title: "every reference — a {{template}} or a bare name in a condition — names a variable that exists at that point"}, - {ID: "W15", Emits: []Severity{SeverityError}, Needs: []string{"Facts.BuiltinTools"}, + {ID: "W15", Handbook: "failures", Emits: []Severity{SeverityError}, Needs: []string{"Facts.BuiltinTools"}, Title: "a declared built-in tool exists in the application's registry"}, - {ID: "W16", Emits: []Severity{SeverityError}, Needs: []string{"Options.EmptyWords"}, + {ID: "W16", Handbook: "response-schema", Emits: []Severity{SeverityError}, Needs: []string{"Options.EmptyWords"}, Title: "a required field is not one the description beside it allows to be empty"}, - {ID: "W17", Emits: []Severity{SeverityError}, + {ID: "W17", Handbook: "flow-shape", Emits: []Severity{SeverityError}, Title: "`switch.var` is given a variable's name, not a {{template}}"}, - {ID: "W18", Emits: []Severity{SeverityWarn}, + {ID: "W18", Handbook: "instruction-text", Emits: []Severity{SeverityWarn}, Title: "no alternative of a `contains` is already covered by a shorter one"}, - {ID: "W19", Emits: []Severity{SeverityError}, + {ID: "W19", Handbook: "context-and-cost", Emits: []Severity{SeverityError}, Title: "every asset a step references is declared by the skill"}, - {ID: "W20", Emits: []Severity{SeverityWarn}, + {ID: "W20", Handbook: "context-and-cost", Emits: []Severity{SeverityWarn}, Title: "every asset the skill declares is referenced by a step"}, - {ID: "E1", Emits: []Severity{SeverityError}, Needs: []string{"Facts.ServerNames"}, + {ID: "E1", Handbook: "failures", Emits: []Severity{SeverityError}, Needs: []string{"Facts.ServerNames"}, Title: "every server the skill declares is registered"}, {ID: "E2", Emits: []Severity{SeverityError}, Needs: []string{"Options.CallProtocol", "Facts.AllTools"}, Title: "a tool the playbook calls exists on the server it names"}, - {ID: "E3", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.ReadOnlyRoles", "Facts.WriteServers"}, + {ID: "E3", Handbook: "failures", Emits: []Severity{SeverityWarn}, Needs: []string{"Options.ReadOnlyRoles", "Facts.WriteServers"}, Title: "a skill that calls itself read-only does not reach for a server that writes"}, {ID: "E4", Emits: []Severity{SeverityWarn}, Needs: []string{"Facts.SkillNames"}, Title: "a delegate step names a skill that exists"}, - {ID: "E5", Emits: []Severity{SeverityError}, Needs: []string{"Facts.BuiltinTools"}, + {ID: "E5", Handbook: "failures", Emits: []Severity{SeverityError}, Needs: []string{"Facts.BuiltinTools"}, Title: "a built-in tool the playbook says to call is declared in builtin_tools"}, - {ID: SkipRule, Emits: []Severity{SeverityInfo}, + {ID: SkipRule, Handbook: "verification", Emits: []Severity{SeverityInfo}, Title: "a rule did not run, and why — so a partial check is not read as a clean one"}, } } + +// handbookOf returns the handbook section a rule belongs to, or "". +// +// Looked up from the catalogue rather than kept in a second map: two lists of +// the same thing drift, and the one that drifts here would send a reader to a +// section about something else. +func handbookOf(rule string) string { + for _, r := range Rules() { + if r.ID == rule { + return r.Handbook + } + } + return "" +} diff --git a/lint/doc_test.go b/lint/doc_test.go index 6061262..eafb3b5 100644 --- a/lint/doc_test.go +++ b/lint/doc_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + skillengine "github.com/inhuman/skill-engine" "github.com/inhuman/skill-engine/lint" ) @@ -24,6 +25,40 @@ func TestReadmeListsEveryRule(t *testing.T) { } } +// A rule pointing at a handbook section that does not exist is worse than a +// rule pointing nowhere: the refusal names a place, the reader goes, and finds +// nothing. Sections get renamed by whoever edits the handbook, and this is what +// tells them a rule was left behind. +func TestEveryRulePointsAtASectionThatExists(t *testing.T) { + sections := map[string]bool{} + for _, s := range skillengine.HandbookIndex() { + sections[s.ID] = true + } + require.NotEmpty(t, sections) + + for _, rule := range lint.Rules() { + if rule.Handbook == "" { + continue + } + assert.Truef(t, sections[rule.Handbook], + "rule %s points at handbook section %q, which the module does not ship", rule.ID, rule.Handbook) + assert.NotEmpty(t, skillengine.Handbook(rule.Handbook)) + } +} + +// And the pointer has to reach the finding: a mapping invisible from a report +// is a mapping that does nothing. +func TestAFindingCarriesItsHandbookSection(t *testing.T) { + rep := lintSkill(t, wf(` tools: ["docs"] + steps: + - name: tell + instruction: "retell {{nope}}" + tools: [] +`)) + f := requireFinding(t, rep, "W14", lint.SeverityError) + assert.Equal(t, "instruction-text", f.Handbook) +} + // The same in the other direction for the fixtures: an unlisted file is one // nobody notices has stopped working. func TestFixturesReadmeListsEveryFile(t *testing.T) { diff --git a/lint/lint.go b/lint/lint.go index bbc072c..27e2049 100644 --- a/lint/lint.go +++ b/lint/lint.go @@ -52,6 +52,17 @@ type Finding struct { Path string Line int // 1-based; 0 = the finding is about the whole file Message string + // Handbook — the id of the handbook section that covers this class, empty + // when none does. Resolve it with skillengine.Handbook(id). + // + // A separate field rather than a sentence glued onto Message: the pointer is + // for whoever ASSEMBLES the refusal — a person reads it as "see also", a + // tool follows it and fetches the section. Measured on the other side of + // that: a refusal ending in "call the schema tool" was dead twice over, + // because no skill had that tool in its radius and the steps that write + // skills run with `tools: []`. A pointer is only worth printing where the + // addressee can go, which is what putting the handbook in the module fixed. + Handbook string } // Report — the findings of one run plus a summary of what was skipped. @@ -353,17 +364,15 @@ func compileStale(in []StaleAPI) ([]staleAPI, error) { // add records a finding on the skill currently being checked. func (r *run) add(rule string, sev Severity, format string, args ...any) { - r.findings = append(r.findings, Finding{ - Rule: rule, Severity: sev, Skill: r.skill, Path: r.path, - Message: fmt.Sprintf(format, args...), - }) + r.addAt(rule, sev, 0, format, args...) } // addAt is add with a line number. func (r *run) addAt(rule string, sev Severity, line int, format string, args ...any) { r.findings = append(r.findings, Finding{ Rule: rule, Severity: sev, Skill: r.skill, Path: r.path, Line: line, - Message: fmt.Sprintf(format, args...), + Message: fmt.Sprintf(format, args...), + Handbook: handbookOf(rule), }) }