P4b: skill noun + contrib/store (SQLite for budget/persona/skill/audit)

Merges the skill half of the persona/skill pair plus the second nested module.
(Squashed onto main from phase-4b-skill; the audit/budget/persona batteries it
was stacked on already landed via the P4 merge.)

- skill/: clean-redesign Skill noun + LEAN SkillStore (lifecycle/versions/
  schedule only) + ToRunnable + Memory default.
- contrib/store/: separate go.mod carrying modernc.org/sqlite, so the driver
  never enters the core go.sum. db.Budget()/Personas()/Skills()/Audit() back
  all four store seams (JSON-blob + indexed columns; round-trip tested).
  Includes the verified gadfly #5 fixes (AppendVersion tx+UNIQUE+error,
  Mark*ScheduledRun atomic json_set, busy_timeout, NaN guard).
- CI: builds + tests the nested module and asserts it owns the sqlite driver.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

skill/consts.go

@@ -0,0 +1,6 @@
+package skill
+
+// DefaultChatbotInputName is the input-param name a chatbot-exposed skill
+// receives the user's message under when its schema doesn't name one. Moved
+// from mort's chatbot_provider.go (a host concern) as a host-agnostic default.
+const DefaultChatbotInputName = "request"

skill/inputs.go

@@ -0,0 +1,422 @@
+package skill
+
+import (
+	"fmt"
+	"strings"
+)
+
+// This file holds the shared input-parsing primitives used by both the
+// chatbot exposure adapter (chatbot_provider.go) and the .skill Discord
+// command handler (commands.go) to construct a SkillInputs map from
+// caller-supplied raw values. Centralising here avoids the two paths
+// drifting in their type-coercion or required-check semantics.
+//
+// Two layers:
+//
+//   - CoerceInputValue: per-param-type coercion (int/float/bool/string).
+//     Accepts loosely-typed values (LLM-stringified numbers, JSON
+//     float64s for ints) and returns a value in the target Go shape.
+//
+//   - CoerceInputs: per-skill validation. Walks the InputSchema, coerces
+//     each declared param via CoerceInputValue, drops extras silently,
+//     errors on missing required.
+//
+// Why exported (capital): both consumers live in the same package, but
+// the names are also referenced in test files and the symbols are
+// genuinely useful API for any future consumer (webui form handler,
+// scheduler in v2). Keep the surface small.
+
+// CoerceInputValue coerces a single raw value to the target InputParam
+// type. JSON numbers arrive from json.Unmarshal as float64; bools as
+// bool; strings as string. Type-mismatched strings are accepted ("3" →
+// int 3, "true" → bool true) because both LLM tool calls and Discord
+// command args frequently surface scalars as strings.
+//
+// Why: LLM tool-call args come through json.Unmarshal of a plain
+// map[string]any, which forces every JSON number into float64 and every
+// JSON string into string. Without this coerce step, an int parameter
+// would arrive in SkillInputs as a float64, a bool sent as "true" would
+// arrive as a string, etc. — confusing the skill agent's prompt
+// renderer and any tool-side logic that switches on Go type. The
+// .skill command handler benefits identically: arg tokens arrive as
+// strings, but downstream tools may expect typed values.
+//
+// Test: TestCoerceInputValue in inputs_test.go covers each branch.
+func CoerceInputValue(paramType string, v any) (any, error) {
+	switch paramType {
+	case "int":
+		switch x := v.(type) {
+		case float64:
+			return int(x), nil
+		case int:
+			return x, nil
+		case string:
+			var i int
+			if _, err := fmt.Sscanf(x, "%d", &i); err != nil {
+				return nil, fmt.Errorf("not an int: %q", x)
+			}
+			return i, nil
+		default:
+			return nil, fmt.Errorf("not an int: %T", v)
+		}
+	case "float":
+		switch x := v.(type) {
+		case float64:
+			return x, nil
+		case int:
+			return float64(x), nil
+		case string:
+			var f float64
+			if _, err := fmt.Sscanf(x, "%f", &f); err != nil {
+				return nil, fmt.Errorf("not a float: %q", x)
+			}
+			return f, nil
+		default:
+			return nil, fmt.Errorf("not a float: %T", v)
+		}
+	case "bool":
+		switch x := v.(type) {
+		case bool:
+			return x, nil
+		case string:
+			switch x {
+			case "true", "True", "TRUE", "1":
+				return true, nil
+			case "false", "False", "FALSE", "0":
+				return false, nil
+			default:
+				return nil, fmt.Errorf("not a bool: %q", x)
+			}
+		default:
+			return nil, fmt.Errorf("not a bool: %T", v)
+		}
+	default:
+		// "string", "user", "channel", "url", and unknown — coerce to
+		// string. JSON numbers/bools are stringified so the executor's
+		// validateInputs (which strips e.g. <@!123> wrappers) gets a
+		// uniform string input.
+		switch x := v.(type) {
+		case string:
+			return x, nil
+		case float64:
+			return fmt.Sprintf("%v", x), nil
+		case bool:
+			return fmt.Sprintf("%v", x), nil
+		default:
+			return fmt.Sprintf("%v", v), nil
+		}
+	}
+}
+
+// CoerceInputs validates and coerces a map of raw caller-supplied values
+// against the declared parameter set:
+//
+//   - Extra keys (not in params) are dropped silently.
+//   - Missing required keys return an error so the caller can surface
+//     usage information.
+//   - Per-param type coercion handles int/float/bool sent as strings.
+//
+// Returns a fresh map containing only declared params; never mutates the
+// input map.
+//
+// Why: see CoerceInputValue. Both callers (chatbot exposure adapter,
+// .skill command handler) need the same required-check + extra-drop
+// semantics; previously only the chatbot path implemented them, which
+// is exactly why .skill <name> <args> dropped its arguments entirely.
+//
+// Test: TestCoerceInputs in inputs_test.go.
+func CoerceInputs(params []InputParam, raw map[string]any) (map[string]any, error) {
+	out := make(map[string]any, len(params))
+	for _, p := range params {
+		v, present := raw[p.Name]
+		if !present {
+			if p.Required {
+				return nil, fmt.Errorf("missing required parameter %q", p.Name)
+			}
+			continue
+		}
+		typed, err := CoerceInputValue(p.Type, v)
+		if err != nil {
+			return nil, fmt.Errorf("parameter %q: %w", p.Name, err)
+		}
+		out[p.Name] = typed
+	}
+	return out, nil
+}
+
+// ParseCommandInputs parses a free-form command argument string into a
+// raw map[string]any keyed by InputSchema parameter names. Three modes
+// are supported, picked by the shape of `schema`:
+//
+//	CASE A — empty schema:
+//	  The whole string becomes {"request": "<rest>"}. Mirrors the
+//	  chatbot exposure default (DefaultChatbotInputName) so a skill with
+//	  no declared inputs can still receive its trigger text uniformly
+//	  across both surfaces.
+//
+//	CASE B — exactly one required param (with optional non-required
+//	tail):
+//	  If the user passed any --key=value or --key value flags they're
+//	  parsed as flags (Case C). Otherwise the WHOLE rest-of-message
+//	  becomes that single required param's value. This is the
+//	  "single-arg convenience" pattern that lets `.skill weather Boston
+//	  today` work without the user typing --city=.
+//
+//	CASE C — multiple params, OR any --flag style input:
+//	  Tokens are parsed as `--name=value` or `--name value`. Bare
+//	  positional tokens after a flag are collected as that flag's value.
+//	  Trailing positional tokens with no preceding flag are dropped
+//	  (the caller's usage string should mention the flag form).
+//
+// The returned map values are RAW strings (or bool true for
+// presence-only flags); type coercion is the caller's job via
+// CoerceInputs.
+//
+// Why this signature instead of returning the typed map directly: the
+// caller wants to distinguish "missing required" (→ usage reply) from
+// "type coercion failed" (→ explicit error). Splitting parse from
+// coerce keeps the message specific.
+func ParseCommandInputs(schema []InputParam, raw string) map[string]any {
+	out := map[string]any{}
+	raw = strings.TrimSpace(raw)
+	if raw == "" {
+		return out
+	}
+
+	// Detect flag-style input regardless of schema shape — even a single
+	// required-param schema may be invoked via `.skill x --name value`
+	// for forward compat.
+	hasFlag := strings.Contains(raw, "--")
+
+	switch {
+	case len(schema) == 0:
+		// Empty schema: mirror the chatbot exposure adapter's default
+		// "request" pseudo-param so executor.composePrompt can render
+		// it uniformly.
+		out[DefaultChatbotInputName] = raw
+
+	case !hasFlag && countRequired(schema) == 1:
+		// Single-required-param convenience: whole rest-of-message is the
+		// value, regardless of any other (non-required) params declared.
+		// They can be supplied via --flag form if needed.
+		req := firstRequired(schema)
+		out[req.Name] = raw
+
+	default:
+		// Flag-style parse. Walk tokens looking for --name[=value] or
+		// --name <value>.
+		parseFlagStyle(out, schema, raw)
+	}
+	return out
+}
+
+// countRequired returns the number of params marked Required.
+func countRequired(schema []InputParam) int {
+	n := 0
+	for _, p := range schema {
+		if p.Required {
+			n++
+		}
+	}
+	return n
+}
+
+// firstRequired returns the first required param. Caller must have
+// already verified at least one exists.
+func firstRequired(schema []InputParam) *InputParam {
+	for i := range schema {
+		if schema[i].Required {
+			return &schema[i]
+		}
+	}
+	return nil
+}
+
+// parseFlagStyle walks tokens for --name=value and --name value forms.
+// Unknown flags (not in schema) are still accepted into the output map
+// so the caller can detect and warn about them; CoerceInputs will drop
+// extras when constructing the final SkillInputs.
+//
+// Tokens not preceded by a --flag are dropped. v1 is intentionally
+// strict-ish here: we don't try to guess which positional token belongs
+// to which param when there are several. The single-required-param
+// convenience handles the common ambiguity-free case in the caller.
+func parseFlagStyle(out map[string]any, schema []InputParam, raw string) {
+	tokens := tokeniseCommandLine(raw)
+	declared := map[string]bool{}
+	for _, p := range schema {
+		declared[p.Name] = true
+	}
+
+	i := 0
+	for i < len(tokens) {
+		t := tokens[i]
+		if !strings.HasPrefix(t, "--") {
+			// Bare positional token outside a flag context — drop. The
+			// caller's usage string should steer users to flag form.
+			i++
+			continue
+		}
+		key := t[2:]
+		// --name=value form
+		if eq := strings.IndexByte(key, '='); eq >= 0 {
+			out[key[:eq]] = key[eq+1:]
+			i++
+			continue
+		}
+		// --name <value> form: take the next token IF it doesn't itself
+		// start with --. Otherwise treat as a presence-only boolean flag.
+		if i+1 < len(tokens) && !strings.HasPrefix(tokens[i+1], "--") {
+			out[key] = tokens[i+1]
+			i += 2
+			continue
+		}
+		out[key] = "true"
+		i++
+	}
+	_ = declared // reserved for v2 unknown-flag warnings
+}
+
+// tokeniseCommandLine splits a free-form Discord command argument
+// string into tokens. Quoted spans (single or double quotes) are kept
+// as one token so users can pass values with spaces:
+//
+//	.skill weather --city="New York"
+//	.skill summarise --text 'a long sentence here'
+//
+// Mirrors the user's intuition without introducing a full shell
+// parser. Newlines split as whitespace.
+func tokeniseCommandLine(s string) []string {
+	var out []string
+	var cur strings.Builder
+	var quote rune
+	flush := func() {
+		if cur.Len() > 0 {
+			out = append(out, cur.String())
+			cur.Reset()
+		}
+	}
+	for _, r := range s {
+		switch {
+		case quote != 0:
+			if r == quote {
+				quote = 0
+				continue
+			}
+			cur.WriteRune(r)
+		case r == '"' || r == '\'':
+			quote = r
+		case r == ' ' || r == '\t' || r == '\n':
+			flush()
+		default:
+			cur.WriteRune(r)
+		}
+	}
+	flush()
+	return out
+}
+
+// ResolveCommandInputs is the one-call helper a Discord .skill handler
+// uses to turn a free-form rest-of-message into a coerced
+// SkillInputs map ready to hand to the executor. It is the single
+// production entry point for command-side input resolution: every
+// caller must use it (do NOT chain ParseCommandInputs + CoerceInputs
+// directly).
+//
+// Why this exists as a single function: chaining
+// ParseCommandInputs + CoerceInputs at the call site is what broke
+// `.skill echo hello world` in production. ParseCommandInputs Case A
+// (empty schema) writes the user's text into out["request"], but
+// CoerceInputs(emptySchema, …) iterates the DECLARED params and
+// silently drops every key not in the schema — so "request" is
+// dropped before reaching the executor, and the agent's user-prompt
+// renders "(no input provided)". The fix is to mirror the chatbot
+// exposure adapter: derive the EFFECTIVE param set (which inflates
+// an empty schema to a single required "request" param) and coerce
+// against that, not the original empty schema.
+//
+// What:
+//   - Empty input_schema → effective params = [{request, required, string}],
+//     so ParseCommandInputs Case A's "request" key survives Coerce.
+//   - Non-empty input_schema → effective params = the schema as-is, so
+//     Case B / Case C parse-and-coerce semantics are unchanged.
+//
+// Returns the coerced SkillInputs map, or an error suitable for
+// surfacing to the user (e.g. via FormatUsage). Never mutates
+// `schema`.
+//
+// Test: TestResolveCommandInputs_* in inputs_test.go cover the three
+// cases plus the empty-schema regression.
+func ResolveCommandInputs(schema []InputParam, raw string) (map[string]any, error) {
+	rawInputs := ParseCommandInputs(schema, raw)
+	effective := effectiveCommandParams(schema)
+	return CoerceInputs(effective, rawInputs)
+}
+
+// effectiveCommandParams returns the parameter set the .skill command
+// path should use for coercion. Mirrors chatbotToolParams in
+// chatbot_provider.go: an empty input_schema is inflated to a single
+// required "request" string param so the user's free-text trigger
+// survives CoerceInputs's drop-extras semantics.
+//
+// Why a separate helper (vs reusing chatbotToolParams): keeping the
+// helper local to inputs.go avoids dragging chatbot_provider.go into
+// the .skill command path's import surface and makes the intent
+// (Discord-side parameter inflation) explicit at the call site.
+func effectiveCommandParams(schema []InputParam) []InputParam {
+	if len(schema) > 0 {
+		return schema
+	}
+	return []InputParam{{
+		Name:        DefaultChatbotInputName,
+		Description: "The user's free-text trigger.",
+		Type:        "string",
+		Required:    true,
+	}}
+}
+
+// FormatUsage renders a human-readable usage string for the .skill
+// invocation form. Used by command handlers when required params are
+// missing or coercion fails.
+//
+// Why: keep the usage message in one place so both the missing-required
+// and coercion-failed paths produce identical output.
+func FormatUsage(name string, schema []InputParam) string {
+	var sb strings.Builder
+	fmt.Fprintf(&sb, "usage: `.skill %s", name)
+	if len(schema) == 0 {
+		sb.WriteString(" <text>`")
+		return sb.String()
+	}
+	if countRequired(schema) == 1 {
+		req := firstRequired(schema)
+		fmt.Fprintf(&sb, " <%s>`", req.Name)
+		// Show optional flags (if any).
+		var optional []InputParam
+		for _, p := range schema {
+			if !p.Required {
+				optional = append(optional, p)
+			}
+		}
+		if len(optional) > 0 {
+			sb.WriteString("\n  optional:")
+			for _, p := range optional {
+				fmt.Fprintf(&sb, " --%s=<%s>", p.Name, p.Type)
+			}
+		}
+		return sb.String()
+	}
+	// Multi-param: full --flag form.
+	for _, p := range schema {
+		if p.Required {
+			fmt.Fprintf(&sb, " --%s=<%s>", p.Name, p.Type)
+		}
+	}
+	for _, p := range schema {
+		if !p.Required {
+			fmt.Fprintf(&sb, " [--%s=<%s>]", p.Name, p.Type)
+		}
+	}
+	sb.WriteString("`")
+	return sb.String()
+}

skill/memory.go

@@ -0,0 +1,169 @@
+package skill
+
+import (
+	"context"
+	"sort"
+	"sync"
+	"time"
+)
+
+// Memory is a zero-dependency in-process SkillStore — a light host or test gets
+// saved-skill persistence with no DB. Mort backs SkillStore with GORM/MySQL;
+// contrib/store adds durable SQLite.
+type Memory struct {
+	mu       sync.RWMutex
+	skills   map[string]*Skill         // by ID
+	versions map[string][]SkillVersion // by skill ID, append order
+	byVerID  map[string]SkillVersion   // by version ID
+}
+
+// NewMemory returns an empty in-memory SkillStore.
+func NewMemory() *Memory {
+	return &Memory{
+		skills:   map[string]*Skill{},
+		versions: map[string][]SkillVersion{},
+		byVerID:  map[string]SkillVersion{},
+	}
+}
+
+var _ SkillStore = (*Memory)(nil)
+
+func (m *Memory) Initialize(context.Context) error { return nil }
+
+func (m *Memory) Save(_ context.Context, s *Skill) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	cp := *s
+	m.skills[s.ID] = &cp
+	return nil
+}
+
+func (m *Memory) Get(_ context.Context, id string) (*Skill, error) {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	s, ok := m.skills[id]
+	if !ok {
+		return nil, ErrNotFound
+	}
+	cp := *s
+	return &cp, nil
+}
+
+func (m *Memory) GetByName(_ context.Context, ownerID, name string) (*Skill, error) {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	for _, s := range m.skills {
+		if s.OwnerID == ownerID && s.Name == name {
+			cp := *s
+			return &cp, nil
+		}
+	}
+	return nil, ErrNotFound
+}
+
+func (m *Memory) Delete(_ context.Context, id string) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	delete(m.skills, id)
+	return nil
+}
+
+func (m *Memory) listWhere(keep func(*Skill) bool) []Skill {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	out := make([]Skill, 0, len(m.skills))
+	for _, s := range m.skills {
+		if keep == nil || keep(s) {
+			out = append(out, *s)
+		}
+	}
+	sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name })
+	return out
+}
+
+func (m *Memory) ListByOwner(_ context.Context, ownerID string) ([]Skill, error) {
+	return m.listWhere(func(s *Skill) bool { return s.OwnerID == ownerID }), nil
+}
+
+func (m *Memory) ListPublic(context.Context) ([]Skill, error) {
+	return m.listWhere(func(s *Skill) bool { return s.Visibility == VisibilityPublic }), nil
+}
+
+func (m *Memory) ListSharedWith(_ context.Context, memberID string) ([]Skill, error) {
+	return m.listWhere(func(s *Skill) bool {
+		if s.Visibility != VisibilityShared {
+			return false
+		}
+		for _, id := range s.SharedWith {
+			if id == memberID {
+				return true
+			}
+		}
+		return false
+	}), nil
+}
+
+func (m *Memory) ListBuiltinByName(_ context.Context, name string) (*Skill, error) {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	for _, s := range m.skills {
+		if s.Source == SourceBuiltin && s.Name == name {
+			cp := *s
+			return &cp, nil
+		}
+	}
+	return nil, ErrNotFound
+}
+
+func (m *Memory) ListChatbotExposed(context.Context) ([]Skill, error) {
+	return m.listWhere(func(s *Skill) bool { return s.ExposeAsChatbotTool }), nil
+}
+
+func (m *Memory) ListDueScheduled(_ context.Context, now time.Time) ([]Skill, error) {
+	return m.listWhere(func(s *Skill) bool { return s.DueAt(now) }), nil
+}
+
+func (m *Memory) MarkScheduledRun(_ context.Context, skillID string, ranAt, nextAt time.Time) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	s, ok := m.skills[skillID]
+	if !ok {
+		return ErrNotFound
+	}
+	s.LastScheduledRunAt = ranAt
+	s.NextRunAt = nextAt
+	return nil
+}
+
+func (m *Memory) AppendVersion(_ context.Context, sv SkillVersion) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.versions[sv.SkillID] = append(m.versions[sv.SkillID], sv)
+	m.byVerID[sv.ID] = sv
+	return nil
+}
+
+func (m *Memory) ListVersionsBySkill(_ context.Context, skillID string, limit int) ([]SkillVersion, error) {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	all := m.versions[skillID]
+	// newest first
+	out := make([]SkillVersion, 0, len(all))
+	for i := len(all) - 1; i >= 0; i-- {
+		out = append(out, all[i])
+		if limit > 0 && len(out) >= limit {
+			break
+		}
+	}
+	return out, nil
+}
+
+func (m *Memory) GetVersionByID(_ context.Context, versionID string) (*SkillVersion, error) {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	sv, ok := m.byVerID[versionID]
+	if !ok {
+		return nil, ErrNotFound
+	}
+	return &sv, nil
+}

skill/runnable.go

@@ -0,0 +1,35 @@
+package skill
+
+import (
+	"time"
+
+	"gitea.stevedudenhoeffer.com/steve/executus/run"
+)
+
+// ToRunnable lowers a saved Skill into the kernel's run.RunnableAgent DTO, so
+// run.Executor can run a skill WITHOUT importing this battery (the inversion of
+// mort's skillexec running a skills.Skill). Maps the static shape only; the
+// skill's input schema → prompt rendering, palette resolution, audit, etc. are
+// supplied separately (the host renders inputs into the input string and wires
+// run.Ports). A skill exposes a flat tool list (no SkillPalette/SubAgentPalette
+// — composition is a host concern), so those stay empty.
+func (s *Skill) ToRunnable() run.RunnableAgent {
+	return run.RunnableAgent{
+		ID:            s.ID,
+		Name:          s.Name,
+		SystemPrompt:  s.SystemPrompt,
+		ModelTier:     s.ModelTier,
+		MaxIterations: s.MaxIterations,
+		MaxRuntime:    s.MaxRuntime,
+		LowLevelTools: s.Tools,
+	}
+}
+
+// DueAt reports whether a scheduled skill is due at now (cron empty => never).
+// Convenience for a host scheduler that doesn't want to re-parse the cron.
+func (s *Skill) DueAt(now time.Time) bool {
+	if s.Schedule == "" || s.NextRunAt.IsZero() {
+		return false
+	}
+	return !s.NextRunAt.After(now)
+}

skill/schedule.go

@@ -0,0 +1,107 @@
+package skill
+
+import (
+	"fmt"
+	"strings"
+	"time"
+
+	"github.com/robfig/cron/v3"
+)
+
+// scheduleParser is the cron parser shared across the skills package. It
+// accepts the standard 5-field syntax (minute hour dom month dow) plus
+// descriptors such as @daily, @hourly, etc. We do not enable the seconds
+// field — schedule cadence is governed in minutes, and a seconds field
+// would invite specs that fire below the min-interval floor without
+// surfacing as such in the spec text.
+//
+// Why standalone vs. cron.ParseStandard: ParseStandard rejects descriptors
+// (@daily, @hourly). Skills callers may want to write @daily as a
+// shorthand alongside the explicit "daily" / "weekly" forms we translate
+// below.
+var scheduleParser = cron.NewParser(
+	cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow | cron.Descriptor,
+)
+
+// ParseSchedule turns a user-supplied schedule expression into a
+// cron.Schedule. The empty string returns (nil, nil) — callers should
+// treat that as "on-demand only".
+//
+// Why: Skill.Schedule is a string field stored verbatim; the validator,
+// the scheduler runner, and any future tooling all need to round-trip
+// through the same parser. Centralising it here avoids drift.
+//
+// Accepted shorthands:
+//   - "daily"  → "0 0 * * *"   (midnight UTC every day)
+//   - "weekly" → "0 0 * * 0"   (midnight UTC every Sunday)
+//
+// Anything else is fed through robfig/cron/v3's standard parser
+// (descriptors enabled).
+//
+// Test: schedule_test.go covers shorthand expansion and invalid-spec
+// rejection.
+func ParseSchedule(expr string) (cron.Schedule, error) {
+	expr = strings.TrimSpace(expr)
+	if expr == "" {
+		return nil, nil
+	}
+	switch strings.ToLower(expr) {
+	case "daily":
+		expr = "0 0 * * *"
+	case "weekly":
+		expr = "0 0 * * 0"
+	}
+	sched, err := scheduleParser.Parse(expr)
+	if err != nil {
+		return nil, fmt.Errorf("invalid schedule %q: %w", expr, err)
+	}
+	return sched, nil
+}
+
+// ScheduleMinInterval returns an estimate of the smallest gap between
+// consecutive fire times for a parsed schedule. It samples the next two
+// fire times from a couple of starting points and returns the smallest
+// observed gap.
+//
+// Why: cron.Schedule does not expose a "smallest interval" API. The
+// validator needs this to enforce a per-skill min-interval floor (so an
+// admin can't accidentally register "* * * * *" and burn GPU minutes).
+// Two probe points are enough to catch irregular schedules whose tightest
+// gap appears at a particular point in the week (e.g. "0 9 * * 1,5",
+// where Mon→Fri is 4d but Fri→Mon is 3d — both sampled).
+//
+// Returns 0 if sched is nil.
+//
+// Test: schedule_test.go covers a "* * * * *" minute-interval probe and
+// the irregular Mon/Fri case.
+func ScheduleMinInterval(sched cron.Schedule) time.Duration {
+	if sched == nil {
+		return 0
+	}
+	// Probe from a fixed reference and from a midweek offset. Six fire
+	// times across two starts catches weekly irregularities (the worst
+	// case is a schedule that fires once a week — we still get one gap
+	// per probe). Using a wall-clock-independent reference keeps the
+	// test deterministic.
+	starts := []time.Time{
+		time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC),     // Monday 00:00
+		time.Date(2024, 1, 4, 12, 30, 0, 0, time.UTC),   // Thursday 12:30
+		time.Date(2024, 6, 15, 23, 59, 59, 0, time.UTC), // mid-year, late
+	}
+	var min time.Duration
+	for _, t := range starts {
+		// Sample three consecutive fires per start to capture two gaps.
+		f1 := sched.Next(t)
+		f2 := sched.Next(f1)
+		f3 := sched.Next(f2)
+		for _, gap := range []time.Duration{f2.Sub(f1), f3.Sub(f2)} {
+			if gap <= 0 {
+				continue
+			}
+			if min == 0 || gap < min {
+				min = gap
+			}
+		}
+	}
+	return min
+}

skill/skill.go

@@ -0,0 +1,424 @@
+// Package skills implements the agentic skills platform: user-creatable
+// agent definitions (system prompt + tool whitelist + I/O spec) that run
+// in-process via majordomo's agent loop.
+//
+// A Skill is a saved agent definition. It can be invoked from Discord
+// (.skill <name>), exposed to the chatbot as a tool (via the
+// SkillsToolProvider), and (in v2) scheduled. Skills compose tools from
+// the skilltools registry, gated by a three-stage permission model:
+// save-time AuthoringRequirement, share-time SafeForShare, execute-time
+// SkillNameGate.
+//
+// This file declares the domain types only. Storage lives in storage.go;
+// validation lives in validate.go. The grand storage pattern documented in
+// pkg/logic/storage/CLAUDE.md applies — when adding a field to Skill, you
+// MUST also update pkg/logic/skills/gorm_model.go (gormSkill, fromStorage,
+// toStorage) or persistence will silently break.
+package skill
+
+import "time"
+
+// Skill is the domain definition of an agentic skill.
+//
+// Why: a skill is a saved agent definition reusable across invocations
+// (Discord, chatbot tool, scheduled run in v2). The struct is intentionally
+// flat — every field lives on its own column on the skills table; there is
+// no JSON-blob spec column. This keeps queries (e.g. "list all skills with
+// chatbot exposure") indexable and avoids opaque migration headaches.
+//
+// What: identity + authoring + agent spec + visibility + chatbot exposure
+// fields, all on one struct.
+//
+// Test: see validate_test.go and integration_test.go for round-trip and
+// validation coverage.
+type Skill struct {
+	// Identity
+	ID          string // UUID
+	OwnerID     string // Discord member ID; empty for builtin
+	Name        string // unique per (owner, builtin namespace)
+	Description string
+	Source      Source // SourceBuiltin | SourceManual
+	CreatedAt   time.Time
+	UpdatedAt   time.Time
+
+	// Authoring (copied at save time from the user)
+	AuthoredBy string // member ID at time of last edit (audit; may differ from owner over time)
+
+	// Versioning (for builtins; user skills typically stay at 1.0.0)
+	Version string // semver; used by builtin loader to decide re-seed
+
+	// Spec — agent definition
+	SystemPrompt   string
+	Tools          []string // registry tool names
+	ModelTier      string   // "fast" | "standard" | "thinking" | explicit "provider/model"
+	InputSchema    []InputParam
+	OutputTarget   OutputTarget
+	Schedule       string        // cron; empty = on-demand only; rejected in v1 (ships in v2)
+	Visibility     Visibility    // VisibilityPrivate | VisibilityShared | VisibilityPublic
+	SharedWith     []string      // member IDs for visibility=shared
+	MaxIterations  int           // 0 → use convar default
+	MaxToolCalls   int           // 0 → use convar default
+	MaxRuntime     time.Duration // 0 → use convar default
+	InitialMessage string
+
+	// Chatbot exposure (v1 — proves out the platform via mortventure)
+	ExposeAsChatbotTool    bool
+	ChatbotToolName        string
+	ChatbotToolDescription string
+	ChatbotChannelFilter   string // named filter from the channel-filter registry
+
+	// Admin gating (v2 — public scheduled channel skills require approval).
+	// DEPRECATED in v3: PinnedVersionID subsumes this flag for non-owner
+	// invocation gating. CanInvoke no longer references this column.
+	// Drop in v4.
+	PendingApproval bool
+
+	// Pinned version (v3 — admin-curated invocation gate).
+	//
+	// Why: in v3, non-owner invocation requires that an admin explicitly
+	// pin a known snapshot. This replaces v2's PendingApproval flag —
+	// pinning is the explicit "approved for general use" signal, and the
+	// pinned snapshot is what executes for non-owner callers (so an owner
+	// editing a public skill never accidentally exposes work-in-progress
+	// to other users).
+	//
+	// PinnedVersionID is the SkillVersion.ID (UUID) of the snapshot that
+	// non-owner invocations resolve to. Empty means "no pin yet" — only
+	// the owner and admins can invoke.
+	//
+	// Schema column is `pinned_version` per the design spec but the field
+	// name in the domain struct is explicit about the kind of value it
+	// holds (a snapshot row's UUID, NOT a semver string), which avoids
+	// the spec ambiguity around "pin to v1.0.5" potentially mapping to
+	// multiple snapshot rows over time.
+	PinnedVersionID string
+
+	// PinnedAt is the wall-clock time the pin was set. Zero means
+	// PinnedVersionID is empty (never pinned).
+	PinnedAt time.Time
+
+	// PinnedBy is the admin member ID who set the current pin. Empty
+	// when PinnedVersionID is empty.
+	PinnedBy string
+
+	// Scheduler bookkeeping (v2). Updated by the scheduler runner after
+	// a successful (or failed-but-counted) scheduled execution.
+	//
+	// LastScheduledRunAt records the wall-clock time of the most recent
+	// scheduled invocation; zero means "never run on schedule".
+	//
+	// NextRunAt is the precomputed wake-up time the scheduler polls for
+	// (`WHERE next_run_at <= NOW()`). It is recomputed by feeding
+	// LastScheduledRunAt (or NOW() on first scheduling) through
+	// ParseSchedule(Schedule).Next(...). Manual / on-demand invocations
+	// MUST NOT touch these fields.
+	LastScheduledRunAt time.Time
+	NextRunAt          time.Time
+
+	// ExtendedBounds, when true, lets a non-admin author save the skill
+	// with bounds (MaxIterations / MaxToolCalls / MaxRuntime) above the
+	// default tier (12/30/60s) up to the extended tier (50/150/600s).
+	// Set by an admin via `.skill admin grant-extended <name>`. Cleared
+	// by `.skill admin revoke-extended <name>`. Builtins and admin-
+	// authored skills bypass the cap entirely (the tier resolution in
+	// Validate treats AuthorIsAdmin and ExtendedBounds equivalently).
+	//
+	// Why a per-skill flag vs a per-user grant: governance is per-skill
+	// — an admin reviews a specific skill's bounds and decides those
+	// resource limits are justified for THAT skill. A user grant would
+	// blanket-allow expensive bounds on every skill they author.
+	ExtendedBounds bool
+
+	// ParallelCompositionAllowed gates whether this skill may use the
+	// skill_invoke_parallel tool. Default false.
+	//
+	// Why a per-skill admin gate: parallel fan-out multiplies blast
+	// radius (one bad skill spawns N concurrent runs). Admins approve
+	// each skill that's allowed to use parallel composition; granting
+	// is per-skill via `.skill admin grant-parallel <name>`. Builtins
+	// may set this directly in skill.yml (the loader bypasses
+	// save-time gates by design).
+	//
+	// Checked AT INVOCATION TIME (every skill_invoke_parallel call), so
+	// admins can grant or revoke without redeploying. The check lives
+	// in the tool handler (pkg/skilltools/tools/skill_invoke_parallel.go)
+	// via the SkillInvokerProvider.IsParallelAllowed extension.
+	ParallelCompositionAllowed bool
+
+	// ExecutionLane is the named lane the skill's runs are submitted to
+	// when the executor routes through pkg/lane (v6). Default
+	// "skill-default"; admin overrides per-skill via
+	// `.skill admin set-lane <name> <lane>`.
+	//
+	// Why per-skill (vs a single global skill lane): different skills
+	// have different concurrency profiles. A long-running web-research
+	// skill might warrant a dedicated 1-slot lane to avoid starving
+	// quick chatbot-exposed skills; an admin should be able to isolate
+	// it without a code change.
+	//
+	// Empty string falls through to "skill-default" at executor time
+	// — keeping the field nullable lets a future schema change
+	// distinguish "explicit skill-default" from "never set".
+	ExecutionLane string
+
+	// WebhookSecret enables inbound webhooks (v7). Empty = disabled
+	// (the default). Non-empty = the random secret URL path segment
+	// for POST /webhooks/<secret>. Generated by EnableWebhook;
+	// rotated by RegenerateWebhookSecret. Storage is varchar(64) and
+	// the secret is 32 random bytes (64 hex chars), so the column
+	// holds a fully unique secret per skill.
+	//
+	// Why store the secret directly (not a hash): the webhook handler
+	// must look up the skill by the secret on every POST, which would
+	// require comparing every stored hash against the supplied secret
+	// — a per-call O(n_skills) operation. The secret is treated as a
+	// long random URL key (like a paste UUID); compromise is mitigated
+	// via RegenerateWebhookSecret rotation, not via storage hashing.
+	WebhookSecret string
+
+	// WebhookSignatureRequired controls whether the inbound webhook
+	// handler verifies HMAC against the X-Mort-Signature header. Default
+	// true (the storage column default). Toggling to false skips HMAC
+	// verification — useful for low-stakes integrations behind an IP
+	// allowlist where the caller can't easily compute HMAC. Owners
+	// flip this on the management page; admins can also force it
+	// back on if a leaked allowlist becomes a concern.
+	WebhookSignatureRequired bool
+
+	// WebhookIPAllowlist is a newline-separated list of CIDR blocks
+	// (or bare IPs). Empty string = no allowlist (accept any source
+	// IP). The handler parses the list at request time so updates take
+	// effect immediately without a redeploy. Invalid CIDR entries
+	// are silently dropped at parse time (the management page form
+	// shows a parse-error preview before save).
+	WebhookIPAllowlist string
+
+	// EncryptionEnabled (v8) opts the skill into per-skill envelope
+	// encryption for KV values and file blob content. Default false
+	// (plaintext storage; matches the legacy default). When true, new
+	// writes go through the AES-256-GCM helpers in pkg/skilltools and
+	// the corresponding skill_kv / skill_file_blobs row stamps
+	// encryption_key_version=1; reads transparently decrypt rows whose
+	// version > 0 and pass through rows whose version == 0 (mixed
+	// storage is supported indefinitely).
+	//
+	// !!!!! OPERATIONAL WARNING !!!!! This flag is a write-side switch
+	// only. Disabling encryption for an already-encrypted skill does
+	// NOT decrypt existing rows — they remain reachable as long as
+	// the master key is intact. Losing SKILLS_ENCRYPTION_MASTER_KEY
+	// renders every encrypted row unreadable; back the master key up
+	// separately from database backups. See pkg/skilltools/encryption.go
+	// for the full operational rules.
+	EncryptionEnabled bool
+
+	// Preemptible (v9) opts the skill into preemption: when a higher-
+	// priority job arrives at a full lane, this skill's running job may
+	// be cancelled mid-flight to free a slot. Default false.
+	//
+	// !!!!! OPERATIONAL WARNING !!!!! Preemption means the skill's
+	// scaddy.Agent context is cancelled mid-step; any partial side
+	// effects (file writes, KV updates, sent emails, etc.) remain
+	// committed. Only mark a skill preemptible when it is idempotent
+	// or read-only — otherwise the user-visible state may be
+	// inconsistent with the run's "preempted" terminal status.
+	//
+	// The lane scheduler will not preempt jobs younger than
+	// `skills.lane.preemption_min_runtime_seconds` (default 30s) to
+	// prevent thrashing. The preempted run is recorded with
+	// status="preempted".
+	Preemptible bool
+
+	// DefaultPriority (v9) is the per-skill default priority used by
+	// the lane scheduler's fair-share queue ordering. Higher numbers
+	// run first within a single user's sub-queue. Default 0.
+	//
+	// Per-invocation overrides (skill_invoke priority arg, webhook
+	// X-Mort-Priority header) win over this default. Owners may set
+	// values in the range [-`skills.priority_max_per_user`,
+	// +`skills.priority_max_per_user`] (default cap 5); admins may
+	// exceed the cap.
+	DefaultPriority int
+
+	// Tags is a free-form set of short labels owners attach to a skill
+	// for organisation + discovery. The list page renders each tag as a
+	// chip and offers a dropdown filter populated from all visible
+	// skills' tags.
+	//
+	// Why a separate field (vs reusing Description / Tools): tags are a
+	// curatorial signal, not part of the agent spec — they only matter
+	// to humans browsing the list. Storing them on the skill row (vs a
+	// side table) keeps lookups index-only and matches how the rest of
+	// the skill's flat fields are persisted.
+	//
+	// Validate enforces: each tag is trimmed + lowercased; max 32 chars
+	// per tag; max 16 tags per skill; duplicates within a single skill
+	// are deduped.
+	Tags []string
+
+	// DeprecatedByAgentID is the Phase 7 soft-retire pointer: when
+	// non-empty, the Skill is "soft retired" — hidden from default
+	// listings (`.skill list`, the webui index, chatbot tool exposure)
+	// but STILL invokable via `.skill <name>` and via `skill_invoke`
+	// tool calls. The string is the agents.Agent.ID of the replacement
+	// Agent that supersedes this Skill.
+	//
+	// Why a pointer (not a bool): a future audit / migration tool needs
+	// to follow the soft-retire link back to the replacement. An admin
+	// browsing the deprecated-skills page wants to see "what should I
+	// use instead?" without a separate lookup table.
+	//
+	// Why keep the Skill row (not drop it): existing skill_invoke calls
+	// in user-authored skills, scheduled jobs, and webhook integrations
+	// would break if the row vanished. Soft-retire preserves the
+	// callable surface while signalling "this is the old name; the
+	// replacement Agent is the curated version."
+	//
+	// Set by the Phase 7 boot migration (pkg/logic/agents/migrate_phase7.go);
+	// admins may also flip it manually via storage tooling. Listing
+	// methods filter on this field by default but explicit GetByName /
+	// GetForInvocation lookups bypass the filter so direct invocation
+	// continues to work.
+	DeprecatedByAgentID string
+
+	// DefaultEmoji is an optional identity emoji for the skill, shown
+	// as the __start__ fallback when StateReactEmoji has no __start__
+	// entry. Also forwarded to the invoking Discord message when a
+	// parent agent calls this skill via skill_invoke, so the user sees
+	// the child skill's identity emoji during execution.
+	DefaultEmoji string
+
+	// StateReactEmoji maps tool names (and reserved keys "__start__",
+	// "__end__", "__error__") to Discord emoji that the bot reacts to
+	// the invoking message with as the skill progresses. Empty map
+	// (the default) disables state-react reactions for this skill.
+	//
+	// Why: the legacy `.query` agent surfaced live progress via emoji
+	// reactions on the invoking message (magnifying glass on search,
+	// page on read, …). Skills inherit the same UX without each
+	// author having to wire `update_status` for trivial signalling —
+	// the emoji map is declarative and the executor calls inv.OnEvent
+	// at the relevant boundaries. update_status remains for richer
+	// interim text; emoji reactions are an additive lightweight signal.
+	//
+	// Reserved keys:
+	//   - __start__: reacted right before agent.Run starts
+	//   - __end__:   reacted on successful completion
+	//   - __error__: reacted on terminal error
+	//
+	// Tool keys: react fires on each tool dispatch. Repeated reactions
+	// of the same emoji are no-ops at Discord (idempotent), so a skill
+	// that calls web_search 5x just leaves one 🔍.
+	//
+	// Map values are arbitrary Discord emoji strings (unicode emoji,
+	// custom emoji `<:name:id>`, animated `<a:name:id>`). Validate does
+	// not enforce a format — Discord rejects invalid emoji at react
+	// time and the executor swallows that with a log line.
+	StateReactEmoji map[string]string
+}
+
+// ThreadIDInputKey is the magic key under skilltools.Invocation.SkillInputs
+// that the v2 .skill new / .skill edit wizard handlers use to thread a
+// pre-created thread channel ID through to delivery. When
+// OutputTarget.Kind == "thread" and this key is present in
+// inv.SkillInputs, delivery posts directly to that thread channel;
+// otherwise it falls back to OutputTarget.Target / inv.ChannelID.
+//
+// Why a magic input key vs an OutputTarget override field: keeps the
+// wire shape (Skill struct) unchanged and keeps the override scoped
+// to a single invocation. Wizard commands set this immediately after
+// MessageThreadStartComplex; nothing else writes it.
+//
+// Why defined here vs in skillexec: wizard command handlers in this
+// package need to write the key, and skillexec imports skills (so
+// the reverse import would cycle). Skillexec aliases this constant.
+const ThreadIDInputKey = "__thread_id__"
+
+// Source distinguishes builtins (loaded from skills/<name>/skill.yml on
+// boot) from user-authored manual skills.
+//
+// Why: builtin skills bypass save-time authoring and share-time safety
+// checks because the loader is trusted infrastructure.
+type Source string
+
+const (
+	SourceBuiltin Source = "builtin"
+	SourceManual  Source = "manual"
+)
+
+// InputParam declares a typed input slot on a skill, populated at
+// invocation time from positional/flag args (Discord) or form fields
+// (webui).
+//
+// Why: skills are invoked from heterogeneous surfaces and need a uniform
+// schema for input collection and validation. The Type drives string→typed
+// coercion in skillexec.validateInputs; Choices restricts to an enum set.
+type InputParam struct {
+	Name        string
+	Description string
+	Type        string // "string"|"int"|"float"|"bool"|"user"|"channel"|"url"
+	Required    bool
+	Default     string // string-encoded; parsed per Type at invocation
+	Choices     []string
+}
+
+// OutputTarget controls where the executor delivers a skill's output.
+//
+// Why: skills run in many contexts and the user shouldn't have to think
+// about delivery — the spec encodes it once. The Discord delivery
+// implementation in pkg/logic/skillexec/delivery.go reads this struct.
+type OutputTarget struct {
+	Kind   string // "channel"|"dm"|"thread"|"webui_only"|"channel_with_summary"
+	Target string // channel/member/thread ID, or empty for caller-context
+}
+
+// Visibility controls who may invoke a skill.
+//
+// Why: separates *invocation* gating (this struct) from *tool authoring*
+// gating (skilltools.Permission) — they are orthogonal. A non-admin can
+// invoke an admin-authored public skill that uses db_select; the permission
+// model for the underlying tool only fires at save time, not invocation.
+type Visibility string
+
+const (
+	VisibilityPrivate Visibility = "private"
+	VisibilityShared  Visibility = "shared"
+	VisibilityPublic  Visibility = "public"
+)
+
+// IsKnownVisibility reports whether v is a recognised visibility value.
+// Used by Validate.
+func IsKnownVisibility(v Visibility) bool {
+	switch v {
+	case VisibilityPrivate, VisibilityShared, VisibilityPublic:
+		return true
+	}
+	return false
+}
+
+// IsKnownOutputKind reports whether kind is a recognised OutputTarget.Kind.
+// Used by Validate and by the Discord delivery switch.
+//
+// "channel_with_summary" is the v-research delivery kind: full output
+// posts to a configured spam channel (skills.research.spam_channel_id)
+// while a generated summary posts in the original channel as a reply
+// linking back. Falls through to plain "channel" behaviour when the
+// spam channel convar is unset or matches the invocation channel.
+// Validate accepts this kind here; the Discord delivery switch in
+// pkg/logic/skillexec/delivery_discord.go is the consumer side.
+func IsKnownOutputKind(kind string) bool {
+	switch kind {
+	case "channel", "dm", "thread", "webui_only", "channel_with_summary":
+		return true
+	}
+	return false
+}
+
+// IsKnownInputType reports whether t is a recognised InputParam.Type.
+// Used by Validate and by skillexec.validateInputs for coercion dispatch.
+func IsKnownInputType(t string) bool {
+	switch t {
+	case "string", "int", "float", "bool", "user", "channel", "url":
+		return true
+	}
+	return false
+}

skill/skill_test.go

@@ -0,0 +1,57 @@
+package skill
+
+import (
+	"context"
+	"testing"
+	"time"
+)
+
+func TestSkillToRunnable(t *testing.T) {
+	s := &Skill{
+		ID: "s1", Name: "summarizer", SystemPrompt: "summarize well", ModelTier: "fast",
+		MaxIterations: 4, MaxRuntime: 20 * time.Second, Tools: []string{"summarize", "now"},
+	}
+	r := s.ToRunnable()
+	if r.ID != "s1" || r.ModelTier != "fast" || r.MaxIterations != 4 || len(r.LowLevelTools) != 2 {
+		t.Fatalf("ToRunnable mapping wrong: %+v", r)
+	}
+	// A skill exposes a flat tool list, not a palette.
+	if len(r.SkillPalette) != 0 || len(r.SubAgentPalette) != 0 {
+		t.Errorf("skill should have empty palettes, got %+v", r)
+	}
+}
+
+func TestMemoryStoreVisibilityAndVersions(t *testing.T) {
+	ctx := context.Background()
+	m := NewMemory()
+	pub := &Skill{ID: "a", Name: "pub", OwnerID: "o1", Visibility: VisibilityPublic}
+	shared := &Skill{ID: "b", Name: "shr", OwnerID: "o1", Visibility: VisibilityShared, SharedWith: []string{"bob"}}
+	priv := &Skill{ID: "c", Name: "prv", OwnerID: "o1", Visibility: VisibilityPrivate}
+	for _, s := range []*Skill{pub, shared, priv} {
+		if err := m.Save(ctx, s); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if ps, _ := m.ListPublic(ctx); len(ps) != 1 || ps[0].ID != "a" {
+		t.Errorf("ListPublic = %+v", ps)
+	}
+	if ss, _ := m.ListSharedWith(ctx, "bob"); len(ss) != 1 || ss[0].ID != "b" {
+		t.Errorf("ListSharedWith(bob) = %+v", ss)
+	}
+	if ss, _ := m.ListSharedWith(ctx, "carol"); len(ss) != 0 {
+		t.Errorf("ListSharedWith(carol) should be empty, got %+v", ss)
+	}
+	if all, _ := m.ListByOwner(ctx, "o1"); len(all) != 3 {
+		t.Errorf("ListByOwner = %d, want 3", len(all))
+	}
+	// Versions: newest-first, fetchable by id.
+	m.AppendVersion(ctx, SkillVersion{ID: "v1", SkillID: "a", Version: "1.0.0"})
+	m.AppendVersion(ctx, SkillVersion{ID: "v2", SkillID: "a", Version: "1.1.0"})
+	vs, _ := m.ListVersionsBySkill(ctx, "a", 10)
+	if len(vs) != 2 || vs[0].ID != "v2" {
+		t.Errorf("versions newest-first wrong: %+v", vs)
+	}
+	if got, err := m.GetVersionByID(ctx, "v1"); err != nil || got.Version != "1.0.0" {
+		t.Errorf("GetVersionByID: %v %+v", err, got)
+	}
+}

skill/skill_version.go

@@ -0,0 +1,28 @@
+package skill
+
+import "time"
+
+// SkillVersion is one immutable snapshot of a Skill at the moment it
+// was saved. The skill_versions table is append-only; pruning is by
+// retention policy in PruneOldVersions.
+//
+// Why: edit history with rollback (v3) and the admin pin gate (v3 Phase 4)
+// both need a stable snapshot of the skill at a known version. The Snapshot
+// field carries the FULL Skill struct so a later restore or pin produces
+// the exact agent definition that was saved — system_prompt, tools,
+// schedule, every field — not a synthesized partial snapshot.
+//
+// What: identity (UUID per snapshot) + skill ref + version-string copy +
+// the full Skill payload + audit fields (saved_by, saved_at, edit_summary).
+//
+// Test: see skill_version_test.go for round-trip, list ordering, prune
+// retention, and version-by-number disambiguation coverage.
+type SkillVersion struct {
+	ID          string    // UUID per snapshot (NOT the skill's ID)
+	SkillID     string    // FK to skills.id (conceptually; not enforced by GORM)
+	Version     string    // Skill.Version at save time (semver)
+	Snapshot    Skill     // full Skill struct embedded; serialised as JSON
+	SavedBy     string    // caller member ID (or "" for builtin loader / pre-v3)
+	SavedAt     time.Time // wall-clock save time
+	EditSummary string    // optional human-readable note ("changed model tier", "...")
+}

skill/store.go

@@ -0,0 +1,44 @@
+package skill
+
+import (
+	"context"
+	"errors"
+	"time"
+)
+
+// ErrNotFound is returned when a skill (or version) lookup misses.
+var ErrNotFound = errors.New("skill not found")
+
+// SkillStore is the persistence seam for saved skills. This is the DELIBERATELY
+// LEAN redesign of mort's 60-method skills.Storage: it carries only skill
+// lifecycle (CRUD + visibility), versioning, and scheduling. The KV/file/quota
+// sub-stores that were fused into mort's interface are NOT here — they are the
+// tools/ store seams (KVStorage / FileStorage / QuotaProvider); email recipients
+// and channel grants stay host concerns. A host backs this with its DB; Memory()
+// is the zero-dependency default; contrib/store adds durable SQLite.
+type SkillStore interface {
+	// Initialize prepares storage (idempotent).
+	Initialize(ctx context.Context) error
+
+	// --- lifecycle ---
+	Save(ctx context.Context, s *Skill) error
+	Get(ctx context.Context, id string) (*Skill, error)
+	GetByName(ctx context.Context, ownerID, name string) (*Skill, error)
+	Delete(ctx context.Context, id string) error
+
+	// --- listing / visibility ---
+	ListByOwner(ctx context.Context, ownerID string) ([]Skill, error)
+	ListPublic(ctx context.Context) ([]Skill, error)
+	ListSharedWith(ctx context.Context, memberID string) ([]Skill, error)
+	ListBuiltinByName(ctx context.Context, name string) (*Skill, error)
+	ListChatbotExposed(ctx context.Context) ([]Skill, error)
+
+	// --- scheduling ---
+	ListDueScheduled(ctx context.Context, now time.Time) ([]Skill, error)
+	MarkScheduledRun(ctx context.Context, skillID string, ranAt, nextAt time.Time) error
+
+	// --- versioning ---
+	AppendVersion(ctx context.Context, sv SkillVersion) error
+	ListVersionsBySkill(ctx context.Context, skillID string, limit int) ([]SkillVersion, error)
+	GetVersionByID(ctx context.Context, versionID string) (*SkillVersion, error)
+}

skill/validate.go

@@ -0,0 +1,374 @@
+package skill
+
+import (
+	"fmt"
+	"strings"
+	"time"
+
+	"gitea.stevedudenhoeffer.com/steve/executus/model"
+)
+
+// ChannelFilterChecker is the subset of ChannelFilterRegistry used by
+// Validate to check that a skill references a registered channel filter.
+//
+// Why: kept narrow so tests can pass a tiny stub; full registry is
+// declared in channel_filters.go.
+type ChannelFilterChecker interface {
+	Has(name string) bool
+}
+
+// ModelTierChecker reports whether the given model tier or
+// "provider/model" spec is recognised. Validate uses this to reject
+// typos at save time.
+//
+// Why: tiers come from llms.tier.* convars (fast/standard/thinking by
+// default) but admins may add custom tiers; explicit "provider/model"
+// is also valid. Validate accepts anything non-empty matching either
+// pattern — finer correctness is the LLM call's job.
+type ModelTierChecker interface {
+	IsValid(spec string) bool
+}
+
+// defaultModelTierChecker accepts all registered tier names (via
+// model.IsTierName) plus any "provider/model" form (string contains "/").
+// Tests can substitute a strict checker via ValidateOpts.ModelTierChecker.
+type defaultModelTierChecker struct{}
+
+func (defaultModelTierChecker) IsValid(spec string) bool {
+	if spec == "" {
+		return false
+	}
+	if model.IsTierName(spec) {
+		return true
+	}
+	// Accept tier-with-reasoning (e.g. "thinking:high")
+	if i := strings.IndexByte(spec, ':'); i > 0 {
+		if model.IsTierName(spec[:i]) {
+			return true
+		}
+	}
+	// Accept explicit "provider/model" or "provider/model:reasoning"
+	return strings.ContainsRune(spec, '/')
+}
+
+// ValidateOpts customises what Validate accepts. All fields are optional;
+// nil checkers fall back to permissive defaults.
+//
+// Why: Validate is called from save paths (which know the registries) and
+// from tests (which want to control acceptance). Bundling the deps here
+// keeps the Skill API stable.
+type ValidateOpts struct {
+	// Filters is consulted when the skill declares a chatbot channel
+	// filter. nil → channel-filter validity is not checked (use only in
+	// tests).
+	Filters ChannelFilterChecker
+	// ModelTier checks the ModelTier spec. nil → defaultModelTierChecker.
+	ModelTier ModelTierChecker
+	// MinIntervalMinutes is the floor on the smallest gap between
+	// consecutive fires of a skill's cron schedule. Zero → use the
+	// package default (defaultMinScheduleIntervalMinutes). Tests pass an
+	// explicit value to exercise the boundary.
+	MinIntervalMinutes int
+
+	// AuthorIsAdmin tells Validate the author has admin privileges and
+	// may save with extended-tier bounds without ExtendedBounds=true.
+	// SaveUserSkill passes this from s.admin.IsAdmin(sk.AuthoredBy).
+	// Builtin loader sets this true to bypass the per-skill flag check
+	// (builtins are trusted infrastructure).
+	AuthorIsAdmin bool
+
+	// DefaultMaxIterations / DefaultMaxToolCalls / DefaultMaxRuntimeSecs
+	// override the package-default tier-1 caps. Zero → fall back to the
+	// constants below. Production wiring populates these from convars
+	// (skills.default_max_iterations etc.) so admins can adjust the
+	// default tier without a redeploy.
+	DefaultMaxIterations  int
+	DefaultMaxToolCalls   int
+	DefaultMaxRuntimeSecs int
+
+	// ExtendedMaxIterations / ExtendedMaxToolCalls / ExtendedMaxRuntimeSecs
+	// override the package-default tier-2 caps (the ceilings allowed when
+	// ExtendedBounds=true OR AuthorIsAdmin=true). Zero → fall back to the
+	// constants below.
+	ExtendedMaxIterations  int
+	ExtendedMaxToolCalls   int
+	ExtendedMaxRuntimeSecs int
+}
+
+// Tiered cap defaults. The DEFAULT tier is what a non-admin author sees
+// without an explicit grant; the EXTENDED tier is what admin authors and
+// admin-granted skills may use. Values are tuned in the v3 spec
+// "Governance: tiered resource caps" section.
+//
+// The package's existing absolute ceilings (maxIterationsLimit=50 and
+// maxRuntime=10m) act as outer floors / sanity bounds; the tier caps
+// are the active gate at save time. Extended caps respect the absolute
+// ceilings naturally (50 iter, 600s = 10min runtime).
+const (
+	// Default tier — non-admin authors of skills without ExtendedBounds.
+	DefaultMaxIterations  = 12
+	DefaultMaxToolCalls   = 30
+	DefaultMaxRuntimeSecs = 60
+
+	// Extended tier — admin authors OR ExtendedBounds=true.
+	ExtendedMaxIterations  = 50
+	ExtendedMaxToolCalls   = 150
+	ExtendedMaxRuntimeSecs = 600 // 10m
+
+	maxIterationsLimit                = 50
+	minRuntime                        = time.Second
+	maxRuntime                        = 10 * time.Minute
+	defaultMinScheduleIntervalMinutes = 30
+
+	// MaxTagsPerSkill caps the number of organisation tags any single
+	// skill may carry. Generous compared to typical taxonomies (GitHub
+	// allows ~10 topics/repo). The cap exists to prevent the list
+	// page's chip rendering from becoming unmanageable.
+	MaxTagsPerSkill = 16
+
+	// MaxTagLength is the per-tag character ceiling. Long enough for
+	// hyphenated phrases ("retro-gaming") but short enough that the
+	// list-page tag dropdown stays readable.
+	MaxTagLength = 32
+)
+
+// Validate enforces the skill spec invariants documented in the design
+// spec ("Skill domain model" section). It is called at save time; the
+// builtin loader skips authoring/share-safety checks but still runs
+// Validate, so all callers can rely on a saved skill being well-formed.
+//
+// Why: spec rules are easy to violate by hand and silently break
+// downstream (e.g. an unknown channel filter never exposes the skill to
+// the chatbot). Every rule fails loudly here.
+//
+// What: returns the first error found; callers may surface it directly to
+// users. opts may be the zero value, in which case channel-filter
+// validation is skipped (tests).
+//
+// Test: each rejection branch has a dedicated unit test in
+// validate_test.go.
+func (s *Skill) Validate(opts ValidateOpts) error {
+	if s == nil {
+		return fmt.Errorf("skill is nil")
+	}
+	if strings.TrimSpace(s.Name) == "" {
+		return fmt.Errorf("skill name is required")
+	}
+	if strings.TrimSpace(s.SystemPrompt) == "" {
+		return fmt.Errorf("skill system prompt is required")
+	}
+
+	// ModelTier
+	tierCheck := opts.ModelTier
+	if tierCheck == nil {
+		tierCheck = defaultModelTierChecker{}
+	}
+	if !tierCheck.IsValid(s.ModelTier) {
+		return fmt.Errorf("unknown model tier %q (expected a tier alias or provider/model)", s.ModelTier)
+	}
+
+	// Schedule — empty means on-demand only. A non-empty value must be
+	// a valid cron expression (or one of the "daily" / "weekly"
+	// shorthands) AND have a smallest fire-gap >= the configured
+	// min-interval floor. Both checks share the package-level
+	// ParseSchedule helper so the scheduler runner uses the same parser.
+	if expr := strings.TrimSpace(s.Schedule); expr != "" {
+		sched, err := ParseSchedule(expr)
+		if err != nil {
+			return fmt.Errorf("schedule: %w", err)
+		}
+		minMinutes := opts.MinIntervalMinutes
+		if minMinutes == 0 {
+			minMinutes = defaultMinScheduleIntervalMinutes
+		}
+		floor := time.Duration(minMinutes) * time.Minute
+		if interval := ScheduleMinInterval(sched); interval < floor {
+			return fmt.Errorf(
+				"schedule %q runs more often than the minimum (every %s, floor is %s)",
+				expr, interval.Round(time.Second), floor)
+		}
+	}
+
+	// Iteration / call / runtime budgets. Zero is allowed — the executor
+	// substitutes a convar-backed default. Negative is always wrong.
+	// The absolute ceilings (maxIterationsLimit=50, maxRuntime=10m) are
+	// outer sanity bounds; the tier caps below are the active gate.
+	//
+	// Why admin bypass on the outer ceilings: builtins are trusted
+	// infrastructure (per the v2 "Builtin loader must bypass save-time
+	// gates" lesson). The builtin loader passes AuthorIsAdmin=true so
+	// trusted skills like `deepresearch` (max_iterations=100,
+	// max_runtime=45m) and `research` (max_runtime=15m) can validate
+	// without re-tuning the package-wide outer floor for everyone.
+	// Non-admin authors still hit the original ceilings AND the
+	// tier-based cap (default 12 iter / 60s runtime, extended 50 iter /
+	// 600s runtime) — both layers stay intact for the untrusted path.
+	if s.MaxIterations < 0 {
+		return fmt.Errorf("max_iterations must be >= 0, got %d", s.MaxIterations)
+	}
+	if !opts.AuthorIsAdmin && s.MaxIterations > maxIterationsLimit {
+		return fmt.Errorf("max_iterations must be 0..%d, got %d", maxIterationsLimit, s.MaxIterations)
+	}
+	if s.MaxToolCalls < 0 {
+		return fmt.Errorf("max_tool_calls must be >= 0, got %d", s.MaxToolCalls)
+	}
+	if s.MaxRuntime < 0 {
+		return fmt.Errorf("max_runtime must be 0 or positive, got %s", s.MaxRuntime)
+	}
+	if s.MaxRuntime > 0 && s.MaxRuntime < minRuntime {
+		return fmt.Errorf("max_runtime must be 0 or >= %s, got %s", minRuntime, s.MaxRuntime)
+	}
+	if !opts.AuthorIsAdmin && s.MaxRuntime > maxRuntime {
+		return fmt.Errorf("max_runtime must be 0 or in [%s..%s], got %s", minRuntime, maxRuntime, s.MaxRuntime)
+	}
+
+	// Tiered cap resolution: a skill saved by an admin OR a skill with
+	// ExtendedBounds=true (admin-granted) may use the extended tier;
+	// everything else saturates at the default tier. Builtins go through
+	// the loader's bypass path (AuthorIsAdmin=true).
+	defIter := opts.DefaultMaxIterations
+	if defIter == 0 {
+		defIter = DefaultMaxIterations
+	}
+	defCalls := opts.DefaultMaxToolCalls
+	if defCalls == 0 {
+		defCalls = DefaultMaxToolCalls
+	}
+	defRuntime := opts.DefaultMaxRuntimeSecs
+	if defRuntime == 0 {
+		defRuntime = DefaultMaxRuntimeSecs
+	}
+	extIter := opts.ExtendedMaxIterations
+	if extIter == 0 {
+		extIter = ExtendedMaxIterations
+	}
+	extCalls := opts.ExtendedMaxToolCalls
+	if extCalls == 0 {
+		extCalls = ExtendedMaxToolCalls
+	}
+	extRuntime := opts.ExtendedMaxRuntimeSecs
+	if extRuntime == 0 {
+		extRuntime = ExtendedMaxRuntimeSecs
+	}
+	maxIter := defIter
+	maxCalls := defCalls
+	maxRuntimeSecs := defRuntime
+	tier := "default"
+	hint := "; ask an admin to grant extended_bounds for higher"
+	if s.ExtendedBounds || opts.AuthorIsAdmin {
+		maxIter = extIter
+		maxCalls = extCalls
+		maxRuntimeSecs = extRuntime
+		tier = "extended"
+		hint = "" // already at the highest tier — no upgrade path
+	}
+	// Admin bypass on the tier cap: trusted infrastructure (builtins,
+	// admin-authored skills) may exceed the extended tier. The
+	// non-admin author still hits the tier cap above. See the
+	// "trusted infrastructure" rationale on the outer-ceiling block.
+	if !opts.AuthorIsAdmin {
+		if s.MaxIterations > maxIter {
+			return fmt.Errorf("max_iterations %d exceeds %s cap (%d)%s",
+				s.MaxIterations, tier, maxIter, hint)
+		}
+		if s.MaxToolCalls > maxCalls {
+			return fmt.Errorf("max_tool_calls %d exceeds %s cap (%d)%s",
+				s.MaxToolCalls, tier, maxCalls, hint)
+		}
+		if s.MaxRuntime > 0 && s.MaxRuntime > time.Duration(maxRuntimeSecs)*time.Second {
+			return fmt.Errorf("max_runtime %s exceeds %s cap (%ds)%s",
+				s.MaxRuntime, tier, maxRuntimeSecs, hint)
+		}
+	}
+
+	// Output target
+	if !IsKnownOutputKind(s.OutputTarget.Kind) {
+		return fmt.Errorf("unknown output_target.kind %q", s.OutputTarget.Kind)
+	}
+
+	// Input schema
+	seenInput := map[string]struct{}{}
+	for i, p := range s.InputSchema {
+		if strings.TrimSpace(p.Name) == "" {
+			return fmt.Errorf("input_schema[%d]: Name is required", i)
+		}
+		if !IsKnownInputType(p.Type) {
+			return fmt.Errorf("input_schema[%d] (%q): unknown type %q", i, p.Name, p.Type)
+		}
+		if _, dup := seenInput[p.Name]; dup {
+			return fmt.Errorf("input_schema: duplicate parameter name %q", p.Name)
+		}
+		seenInput[p.Name] = struct{}{}
+	}
+
+	// Tools
+	seenTool := map[string]struct{}{}
+	for _, t := range s.Tools {
+		if strings.TrimSpace(t) == "" {
+			return fmt.Errorf("tools: empty tool name")
+		}
+		if _, dup := seenTool[t]; dup {
+			return fmt.Errorf("tools: duplicate tool name %q", t)
+		}
+		seenTool[t] = struct{}{}
+	}
+
+	// Tags — normalise + bounds-check. The caller may pass user input
+	// directly; we trim, lowercase, dedup, and bound count + per-tag
+	// length. Mutating the slice in place is intentional so callers
+	// don't need a separate normalise pass.
+	//
+	// Why caps (16 tags / 32 chars): both are generous for human-
+	// curated organisation labels (compare to GitHub's 10 topics/repo
+	// + ~50 chars). The aim is rejecting accidental data dumps and
+	// keeping the list-page chip rendering manageable, not strict
+	// taxonomy enforcement.
+	if len(s.Tags) > MaxTagsPerSkill {
+		return fmt.Errorf("tags: too many (max %d, got %d)", MaxTagsPerSkill, len(s.Tags))
+	}
+	if len(s.Tags) > 0 {
+		seenTag := map[string]struct{}{}
+		out := make([]string, 0, len(s.Tags))
+		for _, raw := range s.Tags {
+			t := strings.ToLower(strings.TrimSpace(raw))
+			if t == "" {
+				continue
+			}
+			if len(t) > MaxTagLength {
+				return fmt.Errorf("tags: %q exceeds %d chars", t, MaxTagLength)
+			}
+			if _, dup := seenTag[t]; dup {
+				continue
+			}
+			seenTag[t] = struct{}{}
+			out = append(out, t)
+		}
+		s.Tags = out
+	}
+
+	// Visibility
+	if !IsKnownVisibility(s.Visibility) {
+		return fmt.Errorf("unknown visibility %q", s.Visibility)
+	}
+	if s.Visibility == VisibilityShared && len(s.SharedWith) == 0 {
+		return fmt.Errorf("visibility=shared requires non-empty shared_with")
+	}
+
+	// Chatbot exposure
+	if s.ExposeAsChatbotTool {
+		if strings.TrimSpace(s.ChatbotToolName) == "" {
+			return fmt.Errorf("expose_as_chatbot_tool=true requires chatbot_tool_name")
+		}
+		if strings.TrimSpace(s.ChatbotToolDescription) == "" {
+			return fmt.Errorf("expose_as_chatbot_tool=true requires chatbot_tool_description")
+		}
+		if strings.TrimSpace(s.ChatbotChannelFilter) == "" {
+			return fmt.Errorf("expose_as_chatbot_tool=true requires chatbot_channel_filter")
+		}
+		if opts.Filters != nil && !opts.Filters.Has(s.ChatbotChannelFilter) {
+			return fmt.Errorf("unknown chatbot_channel_filter %q (not registered)", s.ChatbotChannelFilter)
+		}
+	}
+
+	return nil
+}