Batch 19: Skills framework

docs/USER-SKILLS-SPEC.md

@@ -146,14 +146,20 @@ the single source of truth (portable: export = zip of the directory).
 
 ```
 GET    /console/api/skills/mine                 list my skills
-POST   /console/api/skills/mine/import          zip/URL, preview→confirm (reuses admin flow + SafeTransport)
-PUT    /console/api/skills/mine/{name}          create/update authored skill
+PUT    /console/api/skills/mine/{name}          create/update authored skill (?create=1: 409 if the name exists)
 POST   /console/api/skills/mine/{name}/enable   / disable (library-level)
 POST   /console/api/skills/mine/{name}/chat     {enabled: bool} — trusted-path opt-in
 DELETE /console/api/skills/mine/{name}
 GET    /console/api/skills/mine/{name}/export
 ```
 
+Users don't import packages: the personal import route (zip upload or URL
+fetch into the caller's library) was removed, as no UI used it and it
+exposed the archive parser and the URL fetcher to every user. Admins import
+into the instance library (preview → confirm; the confirm carries the
+previewed digest and is refused if the package changed). A user customizes a
+built-in by authoring a skill of the same name.
+
 Admin routes unchanged. Shard binding UI gains the owner's user skills in the
 `#shard-skillpacks` checklist (a shard may bind its owner's skills, not other
 users').
@@ -163,8 +169,8 @@ users').
 - **Desktop Skills panel**: new "My skills" section above the instance
   library — cards with origin badge (`authored` / `imported from …`),
   enabled toggle, "Use in chat" toggle (warning dialog for imported), Edit /
-  Export / Delete. Import button available to every user (writes to their
-  library).
+  Export / Delete. New skill (authoring) for every user; import is
+  admin-only (instance library).
 - **Mobile**: read-only list + enable/off toggles in Account (same pattern as
   shards); authoring is desktop-only, same note ("Create and edit skills on
   the desktop console").

familiar-gateway/cmd/gateway/main.go

@@ -622,6 +622,13 @@ func main() {
 			if len(pkgs) == 0 {
 				return nil
 			}
+			// Capped like the chat block (it wasn't: a shard bound to
+			// many skills got an unbudgeted prompt block that could
+			// overflow a small model's window).
+			pkgs, dropped := skillpkg.CapPromptSkills(pkgs, skillpkg.MaxPromptSkills)
+			if dropped > 0 {
+				log.Printf("[skills] shard %s binds %d skills; advertising %d", ov.ShardID, len(pkgs)+dropped, len(pkgs))
+			}
 			ps := make([]skillpkg.PromptSkill, 0, len(pkgs))
 			for _, p := range pkgs {
 				ps = append(ps, skillpkg.PromptSkill{Name: p.Name, Description: p.Description})
@@ -648,10 +655,9 @@ func main() {
 			if len(pkgs) == 0 {
 				return ""
 			}
-			const maxChatSkills = 20
-			if len(pkgs) > maxChatSkills {
-				log.Printf("[skills] user %s has %d chat-enabled skills; advertising first %d", userID, len(pkgs), maxChatSkills)
-				pkgs = pkgs[:maxChatSkills]
+			pkgs, dropped := skillpkg.CapPromptSkills(pkgs, skillpkg.MaxPromptSkills)
+			if dropped > 0 {
+				log.Printf("[skills] user %s has %d chat-enabled skills; advertising %d (built-ins kept)", userID, len(pkgs)+dropped, len(pkgs))
 			}
 			ps := make([]skillpkg.PromptSkill, 0, len(pkgs))
 			for _, p := range pkgs {

familiar-gateway/internal/admin/handler.go

@@ -537,7 +537,6 @@ func (h *Handler) Mux(authed http.Handler) http.Handler {
 	authedMux.HandleFunc("POST /console/api/research/runs/{id}/cancel", h.cancelResearchRun)
 	// owner's shards in Phase A; trusted-path exposure is Phase B.
 	authedMux.HandleFunc("GET /console/api/skills/mine", h.listMySkills)
-	authedMux.HandleFunc("POST /console/api/skills/mine/import", h.importMySkill)
 	authedMux.HandleFunc("POST /console/api/skills/mine/{id}/enable", h.setMySkillDisabled(false))
 	authedMux.HandleFunc("POST /console/api/skills/mine/{id}/disable", h.setMySkillDisabled(true))
 	authedMux.HandleFunc("POST /console/api/skills/mine/{id}/chat", h.setMySkillChat)

familiar-gateway/internal/admin/instance_settings.go

@@ -80,6 +80,28 @@ func (s *InstanceSettingsStore) GetAll(ctx context.Context) (map[string]string,
 	return out, rows.Err()
 }
 
+// SetAll upserts several settings in one transaction: all land or none.
+func (s *InstanceSettingsStore) SetAll(ctx context.Context, kv map[string]string, updatedBy string) error {
+	tx, err := s.db.BeginTx(ctx, nil)
+	if err != nil {
+		return err
+	}
+	defer tx.Rollback()
+	for k, v := range kv {
+		if _, err := tx.ExecContext(ctx, `
+			INSERT INTO instance_settings (key, value, updated_at, updated_by)
+			VALUES ($1, $2, NOW(), $3)
+			ON CONFLICT (key) DO UPDATE
+			   SET value = EXCLUDED.value,
+			       updated_at = NOW(),
+			       updated_by = EXCLUDED.updated_by`,
+			k, v, updatedBy); err != nil {
+			return err
+		}
+	}
+	return tx.Commit()
+}
+
 // Set upserts one setting.
 func (s *InstanceSettingsStore) Set(ctx context.Context, key, value, updatedBy string) error {
 	_, err := s.db.ExecContext(ctx, `
@@ -160,38 +182,51 @@ func (h *Handler) getSystemPrompt(w http.ResponseWriter, r *http.Request) {
 //	{"base": "<prompt text>", "user_visible": true|false}
 //
 // An empty base clears the override — the pipeline falls back to the
-// file-loaded base.md. Writes the DB, then refreshes the in-memory
-// PromptStore so the change takes effect on the next turn without a
-// gateway restart.
+// file-loaded base.md — and so does a base equal to that file: the
+// editor shows base.md when there's no override and sends it back on
+// every save, so ticking "visible to users" froze today's base.md as
+// an override, and later base.md upgrades were silently ignored. An
+// absent base leaves the override as it is. Both settings are written
+// in one transaction, then the in-memory PromptStore is refreshed so
+// the change takes effect on the next turn without a restart (a
+// failure between two separate writes left the DB and the running
+// prompt disagreeing until the next restart applied it).
 func (h *Handler) putSystemPrompt(w http.ResponseWriter, r *http.Request) {
 	if h.instanceSettings == nil || h.promptStore == nil {
 		writeJSONError(w, http.StatusServiceUnavailable, "system prompt settings not configured")
 		return
 	}
 	au, _ := AuthUserFrom(r.Context())
+	r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
 	var body struct {
-		Base        string `json:"base"`
-		UserVisible bool   `json:"user_visible"`
+		Base        *string `json:"base"`
+		UserVisible bool    `json:"user_visible"`
 	}
 	if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
 		writeJSONError(w, http.StatusBadRequest, "invalid body: "+err.Error())
 		return
 	}
-	base := strings.TrimSpace(body.Base)
-	if err := h.instanceSettings.Set(r.Context(), SettingSystemPromptBase, base, au.UserID); err != nil {
-		writeJSONError(w, http.StatusInternalServerError, err.Error())
-		return
-	}
 	visibleStr := "false"
 	if body.UserVisible {
 		visibleStr = "true"
 	}
-	if err := h.instanceSettings.Set(r.Context(), SettingSystemPromptUserVisible, visibleStr, au.UserID); err != nil {
+	settings := map[string]string{SettingSystemPromptUserVisible: visibleStr}
+	var base string
+	if body.Base != nil {
+		base = strings.TrimSpace(*body.Base)
+		if base == strings.TrimSpace(h.promptStore.FileBase()) {
+			base = "" // the file itself: no override
+		}
+		settings[SettingSystemPromptBase] = base
+	}
+	if err := h.instanceSettings.SetAll(r.Context(), settings, au.UserID); err != nil {
 		writeJSONError(w, http.StatusInternalServerError, err.Error())
 		return
 	}
 	// Refresh the in-memory cache the pipeline reads.
-	h.promptStore.SetBaseOverride(base)
+	if body.Base != nil {
+		h.promptStore.SetBaseOverride(base)
+	}
 
 	writeJSON(w, http.StatusOK, systemPromptResponse{
 		Base:        h.promptStore.EffectiveBase(),

familiar-gateway/internal/admin/skillpkg_handlers.go

@@ -57,44 +57,67 @@ func (h *Handler) listSkillPackages(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, map[string]any{"items": items})
 }
 
-// readImportPayload extracts the archive bytes + confirm flag +
-// source URL from an import request: multipart/form-data with a
-// "file" zip part (and optional "confirm" field), or JSON
-// {"url": ..., "confirm": bool} for a direct-zip fetch. Shared by the
-// admin (instance) and user (/skills/mine) import endpoints.
-func readImportPayload(r *http.Request) (data []byte, confirm bool, sourceURL string, err error) {
+// Request body caps. Nothing bounded these bodies: a multipart upload
+// spilled to temp disk for as long as it streamed, and a JSON body was
+// decoded whole into memory, from any signed-in user.
+const (
+	maxImportRequestBytes = 22 << 20  // a 21MB archive plus form overhead
+	maxSkillRequestBytes  = 512 << 10 // authored skills (256KB body) and small JSON
+)
+
+// limitBody caps r's body at n bytes; reading past it fails.
+func limitBody(w http.ResponseWriter, r *http.Request, n int64) {
+	r.Body = http.MaxBytesReader(w, r.Body, n)
+}
+
+// importPayload is an import request: the archive, whether it's the
+// confirm step, the source URL, and the digest the preview showed.
+type importPayload struct {
+	data      []byte
+	confirm   bool
+	sourceURL string
+	digest    string
+}
+
+// readImportPayload extracts an import request: multipart/form-data
+// with a "file" zip part (and optional "confirm" and "digest" fields),
+// or JSON {"url": ..., "confirm": bool, "digest": ...} for a direct-zip
+// fetch.
+func readImportPayload(w http.ResponseWriter, r *http.Request) (importPayload, error) {
+	limitBody(w, r, maxImportRequestBytes)
 	ct := r.Header.Get("Content-Type")
 	switch {
 	case strings.HasPrefix(ct, "multipart/form-data"):
 		if err := r.ParseMultipartForm(25 << 20); err != nil {
-			return nil, false, "", fmt.Errorf("invalid upload: %w", err)
+			return importPayload{}, fmt.Errorf("invalid upload: %w", err)
 		}
 		f, _, err := r.FormFile("file")
 		if err != nil {
-			return nil, false, "", fmt.Errorf("missing file part")
+			return importPayload{}, fmt.Errorf("missing file part")
 		}
 		defer f.Close()
-		data, err = io.ReadAll(io.LimitReader(f, 21<<20))
+		data, err := io.ReadAll(io.LimitReader(f, 21<<20))
 		if err != nil {
-			return nil, false, "", fmt.Errorf("read upload: %w", err)
+			return importPayload{}, fmt.Errorf("read upload: %w", err)
 		}
-		return data, r.FormValue("confirm") == "true", "", nil
+		return importPayload{data: data, confirm: r.FormValue("confirm") == "true", digest: r.FormValue("digest")}, nil
 	default:
 		var body struct {
 			URL     string `json:"url"`
 			Confirm bool   `json:"confirm"`
+			Digest  string `json:"digest"`
 		}
 		if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
-			return nil, false, "", fmt.Errorf("invalid body: %w", err)
+			return importPayload{}, fmt.Errorf("invalid body: %w", err)
 		}
 		if body.URL == "" {
-			return nil, false, "", fmt.Errorf("url required (or upload a zip via multipart)")
+			return importPayload{}, fmt.Errorf("url required (or upload a zip via multipart)")
 		}
 		fetched, err := fetchSkillArchive(r.Context(), body.URL)
 		if err != nil {
-			return nil, false, "", err
+			return importPayload{}, err
 		}
-		return fetched, body.Confirm, body.URL, nil
+		return importPayload{data: fetched, confirm: body.Confirm, sourceURL: body.URL, digest: body.Digest}, nil
 	}
 }
 
@@ -129,16 +152,28 @@ func (h *Handler) importSkillPackage(w http.ResponseWriter, r *http.Request) {
 		writeJSONError(w, http.StatusUnauthorized, "missing authenticated user")
 		return
 	}
-	data, confirm, sourceURL, err := readImportPayload(r)
+	in, err := readImportPayload(w, r)
+	if err != nil {
+		writeJSONError(w, http.StatusBadRequest, err.Error())
+		return
+	}
+	if !in.confirm {
+		h.writeImportPreview(w, in.data)
+		return
+	}
+	// Admit only what the admin previewed. Confirm re-uploads or
+	// re-fetches the archive, and a URL (or anything on the path of a
+	// plain http one) could serve a different package the second time.
+	loaded, _, _, err := h.skillPkgs.PreviewZip(in.data, h.skillPkgKnownTools)
 	if err != nil {
 		writeJSONError(w, http.StatusBadRequest, err.Error())
 		return
 	}
-	if !confirm {
-		h.writeImportPreview(w, data)
+	if in.digest == "" || loaded.Digest != in.digest {
+		writeJSONError(w, http.StatusConflict, "the package isn't the one previewed (its digest changed) — preview it again")
 		return
 	}
-	pkg, err := h.skillPkgs.ImportZip(r.Context(), data, au.UserID, sourceURL, h.skillPkgKnownTools)
+	pkg, err := h.skillPkgs.ImportZip(r.Context(), in.data, au.UserID, in.sourceURL, h.skillPkgKnownTools)
 	if err != nil {
 		writeJSONError(w, http.StatusBadRequest, err.Error())
 		return
@@ -231,6 +266,7 @@ func (h *Handler) setSkillPackageChat(w http.ResponseWriter, r *http.Request) {
 	if !h.requireSkillPkgs(w) {
 		return
 	}
+	limitBody(w, r, maxSkillRequestBytes)
 	var body struct {
 		Enabled bool `json:"enabled"`
 	}
@@ -299,34 +335,6 @@ func (h *Handler) listMySkills(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, map[string]any{"items": items})
 }
 
-// importMySkill mirrors the admin import flow (preview → confirm,
-// zip upload or SSRF-guarded URL fetch) into the caller's private
-// library.
-func (h *Handler) importMySkill(w http.ResponseWriter, r *http.Request) {
-	if !h.requireSkillPkgs(w) {
-		return
-	}
-	uid, ok := h.requireUserSession(w, r)
-	if !ok {
-		return
-	}
-	data, confirm, sourceURL, err := readImportPayload(r)
-	if err != nil {
-		writeJSONError(w, http.StatusBadRequest, err.Error())
-		return
-	}
-	if !confirm {
-		h.writeImportPreview(w, data)
-		return
-	}
-	pkg, err := h.skillPkgs.ImportZipForUser(r.Context(), uid, data, sourceURL, h.skillPkgKnownTools)
-	if err != nil {
-		writeJSONError(w, http.StatusBadRequest, err.Error())
-		return
-	}
-	writeJSON(w, http.StatusCreated, pkg)
-}
-
 func (h *Handler) setMySkillDisabled(disabled bool) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		if !h.requireSkillPkgs(w) {
@@ -371,6 +379,7 @@ func (h *Handler) setMySkillChat(w http.ResponseWriter, r *http.Request) {
 	if !ok {
 		return
 	}
+	limitBody(w, r, maxSkillRequestBytes)
 	var body struct {
 		Enabled bool `json:"enabled"`
 	}
@@ -415,11 +424,11 @@ func (h *Handler) putMySkill(w http.ResponseWriter, r *http.Request) {
 	}
 	name := r.PathValue("name")
 	if name == "import" {
-		// Route-shape collision guard: POST /skills/mine/import is the
-		// import endpoint; a skill by that name would shadow it.
+		// Reserved: the path of the (removed) personal import endpoint.
 		writeJSONError(w, http.StatusBadRequest, `"import" is a reserved name`)
 		return
 	}
+	limitBody(w, r, maxSkillRequestBytes)
 	var body struct {
 		Description string `json:"description"`
 		Body        string `json:"body"`
@@ -428,7 +437,17 @@ func (h *Handler) putMySkill(w http.ResponseWriter, r *http.Request) {
 		writeJSONError(w, http.StatusBadRequest, "invalid body: "+err.Error())
 		return
 	}
-	pkg, err := h.skillPkgs.SaveAuthored(r.Context(), uid, name, body.Description, body.Body, h.skillPkgKnownTools)
+	// ?create=1 (the New skill editor) refuses an existing name instead
+	// of overwriting that skill.
+	save := h.skillPkgs.SaveAuthored
+	if r.URL.Query().Get("create") == "1" {
+		save = h.skillPkgs.CreateAuthored
+	}
+	pkg, err := save(r.Context(), uid, name, body.Description, body.Body, h.skillPkgKnownTools)
+	if errors.Is(err, skillpkg.ErrExists) {
+		writeJSONError(w, http.StatusConflict, "you already have a skill named "+name+" — open it to edit, or pick another name")
+		return
+	}
 	if err != nil {
 		writeJSONError(w, http.StatusBadRequest, err.Error())
 		return
@@ -476,6 +495,7 @@ func (h *Handler) duplicateMySkill(w http.ResponseWriter, r *http.Request) {
 	if !ok {
 		return
 	}
+	limitBody(w, r, maxSkillRequestBytes)
 	var body struct {
 		Name string `json:"name"`
 	}
@@ -579,6 +599,7 @@ func (h *Handler) putShardSkills(w http.ResponseWriter, r *http.Request) {
 		writeJSONError(w, http.StatusNotFound, "shard not found")
 		return
 	}
+	limitBody(w, r, maxSkillRequestBytes)
 	var body struct {
 		SkillIDs []string `json:"skill_ids"`
 	}