From 581de4019a9fa3afa18663ee20a7911b810538c0 Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Sat, 24 Oct 2026 16:39:19 +0000 Subject: [PATCH 1/3] Report Skill upload and update field errors --- contracts/agents-api/source-files.md | 8 +- contracts/agents-api/zh/source-files.md | 10 +- services/core/internal/api/errors.go | 13 +- services/core/internal/api/errors_skills.go | 2 + .../core/internal/api/errors_skills_test.go | 2 + .../core/internal/api/session_artifacts.go | 2 +- services/core/internal/api/skills.go | 31 +++- .../internal/api/skills_field_errors_test.go | 154 ++++++++++++++++++ services/core/internal/api/skills_transfer.go | 7 +- services/core/internal/api/skills_upload.go | 63 ++++--- .../core/internal/api/source_files_content.go | 4 +- .../core/internal/api/source_files_test.go | 2 +- .../persistence/postgres/skillpg/skillpg.go | 3 + .../postgres/skillpg/skillpg_test.go | 4 +- services/core/internal/skills/errors.go | 3 + services/core/internal/skills/service.go | 2 +- services/core/internal/skills/service_test.go | 2 +- 17 files changed, 266 insertions(+), 46 deletions(-) create mode 100644 services/core/internal/api/skills_field_errors_test.go diff --git a/contracts/agents-api/source-files.md b/contracts/agents-api/source-files.md index 6bfe8e65f..bf29d2c78 100644 --- a/contracts/agents-api/source-files.md +++ b/contracts/agents-api/source-files.md @@ -62,11 +62,11 @@ A copy into a workspace reads a consistent snapshot of the File and can finish a | Operation | Behavior | | --- | --- | | `POST /skills` | Uploads a new Skill. Its first version is both default and latest | -| `POST /skills/{skill_id}/versions` | Uploads a new version. A `default` form field of `true` makes it the default; `false` or omitted leaves the default unchanged | +| `POST /skills/{skill_id}/versions` | Uploads a new version. A `default` form field of `true` or `false`, in any letter case, makes it the default or leaves the default unchanged; omitted leaves it unchanged | | `GET /skills`, `GET /skills/{skill_id}` | Skill metadata, without decrypting any bundle | -| `POST /skills/{skill_id}` | `{"default_version": ""}` changes the default version | +| `POST /skills/{skill_id}` | `{"default_version": ""}` changes the default version. Body errors are field errors with the member as `param`; a version the Skill does not have returns 404 with `param: "default_version"` | | `DELETE /skills/{skill_id}` | Deletes the Skill and every version | -| `GET /skills/{skill_id}/content` | ZIP of the default version | +| `GET /skills/{skill_id}/content` | ZIP of the default version, `Content-Type: application/zip`, filename `-.zip` | | `GET /skills/{skill_id}/versions`, `GET /skills/{skill_id}/versions/{version}` | Version metadata. The list orders by version number, and `after` is a version ID (`skillver_…`), not a number | | `GET /skills/{skill_id}/versions/{version}/content` | ZIP of that version | | `DELETE /skills/{skill_id}/versions/{version}` | See [Delete a version](#delete-a-version) | @@ -87,6 +87,8 @@ A bundle has one top-level folder containing `SKILL.md` and any supporting files - Entries are regular files or directories with clean relative paths. Links, special files, absolute paths, `..` components and duplicates are rejected. - Limits: 5 MiB compressed, 20 MiB expanded, 500 files, and 1,000 ZIP entries including directories. +A rejected upload returns 400 `invalid_request_error` with `param: "files"` and code `invalid_value`, and a message that states the cause. A missing `files` part, an unknown part and `default` on create are `Missing required parameter` and `Unknown parameter` errors for that member; an invalid `default` names `param: "default"`. The upload routes also reject a `name` that contains `_` and a `description` longer than 1024 characters. Skills already stored, plugin Skills and Environment installation keep accepting `_` in the name and any non-empty description. + Core encrypts each version's bundle bound to its Project, Skill and version. ZIP uploads keep executable bits; directory uploads store files with mode 0644. ### Versions and metadata diff --git a/contracts/agents-api/zh/source-files.md b/contracts/agents-api/zh/source-files.md index 697b11ede..f0edfd5de 100644 --- a/contracts/agents-api/zh/source-files.md +++ b/contracts/agents-api/zh/source-files.md @@ -1,7 +1,7 @@ --- title: "文件与 Skill" source: contracts/agents-api/source-files.md -source_hash: 32041f334a2bf3141731fab69901fc97665da225e4e03328df0db7300faa18ff +source_hash: a87f5745f8d96e7399726fa99cb8c69fc083ee03faf122bce7c4fc33ad95b5d0 --- Files（`/v1/files`）和 Skills（`/v1/skills`）是具有独立生命周期的项目资源，不依赖 Session。File 保存上传字节，Environment 通过 ID 复制它们。Skill 保存不可变、版本化的包，由模板和 Session 引用。项目的所有 API 密钥共享这些资源。 @@ -64,11 +64,11 @@ Core 在自身数据库中以 PostgreSQL 大对象存储 File 字节。上传和 | 操作 | 行为 | | --- | --- | | `POST /skills` | 上传新 Skill。首版同时为默认和最新版本 | -| `POST /skills/{skill_id}/versions` | 上传新版本。表单 `default` 为 `true` 时设为默认；`false` 或省略时默认不变 | +| `POST /skills/{skill_id}/versions` | 上传新版本。表单 `default` 取 `true` 或 `false`（不区分大小写），分别设为默认或保持默认不变；省略时默认不变 | | `GET /skills`, `GET /skills/{skill_id}` | Skill 元数据，不解密任何包 | -| `POST /skills/{skill_id}` | `{"default_version": ""}` 修改默认版本 | +| `POST /skills/{skill_id}` | `{"default_version": ""}` 修改默认版本。请求体错误是以该成员为 `param` 的字段错误；Skill 没有的版本返回 404，`param: "default_version"` | | `DELETE /skills/{skill_id}` | 删除 Skill 与所有版本 | -| `GET /skills/{skill_id}/content` | 默认版本 ZIP | +| `GET /skills/{skill_id}/content` | 默认版本 ZIP，`Content-Type: application/zip`，文件名 `-.zip` | | `GET /skills/{skill_id}/versions`, `GET /skills/{skill_id}/versions/{version}` | 版本元数据。列表按版本号排序，`after` 是版本 ID（`skillver_…`），不是数字 | | `GET /skills/{skill_id}/versions/{version}/content` | 指定版本 ZIP | | `DELETE /skills/{skill_id}/versions/{version}` | 见[删除版本](#delete-a-version) | @@ -89,6 +89,8 @@ curl "$OPENAI_BASE_URL/skills" -H "Authorization: Bearer $OPENAI_API_KEY" -F fil - 条目为普通文件或目录，使用规范相对路径。链接、特殊文件、绝对路径、`..` 组件和重复项被拒绝。 - 限制：压缩后 5 MiB，展开后 20 MiB，500 个文件，以及含目录在内的 1,000 个 ZIP 条目。 +被拒绝的上传返回 400 `invalid_request_error`，`param: "files"`，code 为 `invalid_value`，message 说明原因。缺少 `files` 部分、未知部分以及创建时携带 `default` 分别返回针对该成员的 `Missing required parameter` 与 `Unknown parameter` 错误；无效的 `default` 的 `param: "default"`。上传路由还会拒绝包含 `_` 的 `name` 以及超过 1024 个字符的 `description`。已存储的 Skill、插件 Skill 和 Environment 安装仍接受名称中的 `_` 和任何非空 `description`。 + Core 将每个版本的包加密并绑定到项目、Skill 与版本。ZIP 上传保留可执行位；目录上传以 mode 0644 存储文件。 ### 版本与元数据 {#versions-and-metadata} diff --git a/services/core/internal/api/errors.go b/services/core/internal/api/errors.go index af339ab6d..05d2ad2bb 100644 --- a/services/core/internal/api/errors.go +++ b/services/core/internal/api/errors.go @@ -61,9 +61,10 @@ func writeContentTooLarge(w http.ResponseWriter) { const unstorableTextMessage = "Request text contains characters this service cannot store or compare, such as U+0000 or invalid UTF-8." // fieldError is a request validation failure reported with the official -// invalid_request_error code. An empty param serializes as null. +// invalid_request_error code unless it names another. An empty param +// serializes as null. type fieldError struct { - param, message string + param, message, code string } func (e *fieldError) Error() string { return e.message } @@ -75,10 +76,14 @@ func writeFieldError(w http.ResponseWriter, err error) bool { if !errors.As(err, &field) { return false } + code := field.code + if code == "" { + code = "invalid_request_error" + } if field.param == "" { - writeError(w, http.StatusBadRequest, "invalid_request_error", field.message) + writeError(w, http.StatusBadRequest, code, field.message) } else { - writeError(w, http.StatusBadRequest, "invalid_request_error", field.message, field.param) + writeError(w, http.StatusBadRequest, code, field.message, field.param) } return true } diff --git a/services/core/internal/api/errors_skills.go b/services/core/internal/api/errors_skills.go index 89109e2af..fe59db136 100644 --- a/services/core/internal/api/errors_skills.go +++ b/services/core/internal/api/errors_skills.go @@ -23,6 +23,8 @@ func writeSkillsError(w http.ResponseWriter, r *http.Request, err error) { } else { writeError(w, http.StatusBadRequest, "invalid_request_error", cursor.Message) } + case errors.Is(err, skills.ErrVersionNotFound): + writeError(w, http.StatusNotFound, "", "Skill version not found.", "default_version") case errors.Is(err, skills.ErrNotFound): code := "not_found_error" if skillsRoute { diff --git a/services/core/internal/api/errors_skills_test.go b/services/core/internal/api/errors_skills_test.go index f08aaa25d..2730864bd 100644 --- a/services/core/internal/api/errors_skills_test.go +++ b/services/core/internal/api/errors_skills_test.go @@ -34,6 +34,8 @@ func TestWriteSkillsError(t *testing.T) { `{"error":{"message":"Resource not found.","type":"invalid_request_error","code":null,"param":null}}`}, {"project not found", project, skills.ErrNotFound, http.StatusNotFound, `{"error":{"message":"Resource not found.","type":"not_found_error","code":"not_found_error","param":null}}`}, + {"default version missing", "/v1/skills/skill_missing", skills.ErrVersionNotFound, http.StatusNotFound, + `{"error":{"message":"Skill version not found.","type":"invalid_request_error","code":null,"param":"default_version"}}`}, {"invalid input", "/v1/skills", skills.ErrInvalidInput, http.StatusBadRequest, `{"error":{"message":"Invalid resource identifier or request limits.","type":"invalid_request_error","code":"invalid_request","param":null}}`}, {"unstorable text", "/v1/skills", fmt.Errorf("create: %w", textvalue.ErrUnstorable), http.StatusBadRequest, diff --git a/services/core/internal/api/session_artifacts.go b/services/core/internal/api/session_artifacts.go index 79275ca82..5e0a41f9b 100644 --- a/services/core/internal/api/session_artifacts.go +++ b/services/core/internal/api/session_artifacts.go @@ -60,7 +60,7 @@ func (h *Handler) deleteSessionArtifact(w http.ResponseWriter, r *http.Request) } func (h *Handler) sessionArtifactContent(w http.ResponseWriter, r *http.Request) { - err := serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { + err := serveStoredContent(w, r, "application/octet-stream", func(ctx context.Context, consume func(string, int64, io.Reader) error) error { return h.ArtifactsReader.ReadSessionArtifact(ctx, tenantID(r), chi.URLParam(r, "session_id"), chi.URLParam(r, "artifact_id"), func(a sessions.Artifact, body io.Reader) error { return consume(path.Base(a.Path), a.SizeBytes, body) }) diff --git a/services/core/internal/api/skills.go b/services/core/internal/api/skills.go index 392c3c8d4..a69b11683 100644 --- a/services/core/internal/api/skills.go +++ b/services/core/internal/api/skills.go @@ -2,6 +2,7 @@ package api import ( "context" + "encoding/json" "net/http" "strconv" @@ -61,12 +62,34 @@ func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { if !ok { return } - var input v1.SkillUpdateRequest - if decodeInputObject(body, &input, "default_version") != nil || input.DefaultVersion == "" { - writeSkillsError(w, r, skills.ErrInvalidInput) + raw, err := jsonObjectBody(body) + if err != nil { + writeFieldError(w, err) + return + } + if field, found := unknownBodyField(raw, "default_version"); found { + if !echoableField(field) { + writeFieldError(w, errUnknownParameter) + return + } + writeFieldError(w, &fieldError{param: field, message: "Unknown parameter: '" + field + "'."}) + return + } + var input struct { + DefaultVersion json.RawMessage `json:"default_version"` + } + _ = json.Unmarshal(raw, &input) + var version string + if len(input.DefaultVersion) == 0 { + writeFieldError(w, missingParameter("default_version")) + return + } + if err := checkValue("default_version", input.DefaultVersion, shape{kind: stringValue}); err != nil { + writeFieldError(w, err) return } - value, err := h.Skills.SetDefaultVersion(r.Context(), skills.SetDefaultVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Version: input.DefaultVersion}) + _ = json.Unmarshal(input.DefaultVersion, &version) + value, err := h.Skills.SetDefaultVersion(r.Context(), skills.SetDefaultVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Version: version}) if err != nil { writeSkillsError(w, r, err) return diff --git a/services/core/internal/api/skills_field_errors_test.go b/services/core/internal/api/skills_field_errors_test.go new file mode 100644 index 000000000..aa9ae7754 --- /dev/null +++ b/services/core/internal/api/skills_field_errors_test.go @@ -0,0 +1,154 @@ +package api + +import ( + "archive/zip" + "bytes" + "context" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + "net/textproto" + "strings" + "testing" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/go-chi/chi/v5" +) + +func skillZip(t *testing.T, name, description string) []byte { + t.Helper() + var buffer bytes.Buffer + writer := zip.NewWriter(&buffer) + file, err := writer.Create("proof/SKILL.md") + if err != nil { + t.Fatal(err) + } + if _, err = file.Write([]byte("---\nname: " + name + "\ndescription: " + description + "\n---\nbody")); err != nil { + t.Fatal(err) + } + if err = writer.Close(); err != nil { + t.Fatal(err) + } + return buffer.Bytes() +} + +// skillServer serves the Skill routes over HTTP, which the bounded transfers need. +func skillServer(t *testing.T, fake *fakeSkills) *httptest.Server { + t.Helper() + router := chi.NewRouter() + router.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), adminTenantContextKey{}, "tenant"))) + }) + }) + (&Handler{Dependencies: Dependencies{Skills: fake}}).registerSkillRoutes(router) + server := httptest.NewServer(router) + t.Cleanup(server.Close) + return server +} + +func TestSkillUploadFieldErrors(t *testing.T) { + type part struct { + field, filename string + body []byte + } + good := skillZip(t, "proof", "Verify an upload.") + for _, test := range []struct { + name string + version bool + parts []part + want string + }{ + {"no files", false, nil, `"message":"Missing required parameter: 'files'.","type":"invalid_request_error","code":"invalid_request_error","param":"files"`}, + {"default on create", false, []part{{"files", "p.zip", good}, {"default", "", []byte("true")}}, `"message":"Unknown parameter: 'default'.","type":"invalid_request_error","code":"invalid_request_error","param":"default"`}, + {"bad default", true, []part{{"files", "p.zip", good}, {"default", "", []byte("yes")}}, `"code":"invalid_request_error","param":"default"`}, + {"bad bundle", false, []part{{"files", "p.zip", []byte("not a zip")}}, `"type":"invalid_request_error","code":"invalid_value","param":"files"`}, + {"underscore name", false, []part{{"files", "p.zip", skillZip(t, "my_skill", "Verify an upload.")}}, `"message":"The Skill name must not contain '_'.","type":"invalid_request_error","code":"invalid_value","param":"files"`}, + {"long description", false, []part{{"files", "p.zip", skillZip(t, "proof", strings.Repeat("d", 1025))}}, `"message":"The Skill description must be at most 1024 characters.","type":"invalid_request_error","code":"invalid_value","param":"files"`}, + } { + t.Run(test.name, func(t *testing.T) { + var buffer bytes.Buffer + writer := multipart.NewWriter(&buffer) + for _, entry := range test.parts { + header := textproto.MIMEHeader{} + disposition := `form-data; name="` + entry.field + `"` + if entry.filename != "" { + disposition += `; filename="` + entry.filename + `"` + } + header.Set("Content-Disposition", disposition) + p, err := writer.CreatePart(header) + if err != nil { + t.Fatal(err) + } + _, _ = p.Write(entry.body) + } + _ = writer.Close() + path := "/v1/skills" + if test.version { + path += "/skill_x/versions" + } + response, err := http.Post(skillServer(t, &fakeSkills{t: t}).URL+path, writer.FormDataContentType(), &buffer) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + body, _ := io.ReadAll(response.Body) + if response.StatusCode != http.StatusBadRequest || !strings.Contains(string(body), test.want) { + t.Fatalf("%d %s", response.StatusCode, body) + } + }) + } +} + +func TestSkillUploadAcceptsCapitalizedDefault(t *testing.T) { + var buffer bytes.Buffer + writer := multipart.NewWriter(&buffer) + file, _ := writer.CreateFormFile("files", "p.zip") + _, _ = file.Write(skillZip(t, "proof", "Verify an upload.")) + _ = writer.WriteField("default", "True") + _ = writer.Close() + request := httptest.NewRequest(http.MethodPost, "/v1/skills/skill_x/versions", &buffer) + request.Header.Set("Content-Type", writer.FormDataContentType()) + if _, makeDefault, err := readSkillUpload(request, true); err != nil || !makeDefault { + t.Fatal(makeDefault, err) + } +} + +func TestSkillUpdateErrors(t *testing.T) { + fake := &fakeSkills{t: t, setDefaultVersion: func(context.Context, skills.SetDefaultVersion) (skills.Skill, error) { + return skills.Skill{}, skills.ErrVersionNotFound + }} + router := chi.NewRouter() + router.Post("/v1/skills/{skill_id}", (&Handler{Dependencies: Dependencies{Skills: fake}}).updateSkill) + for _, test := range []struct{ name, body, want string }{ + {"unknown key", `{"default_version":"1","x":1}`, `"message":"Unknown parameter: 'x'.","type":"invalid_request_error","code":"invalid_request_error","param":"x"`}, + {"missing", `{}`, `"message":"Missing required parameter: 'default_version'.","type":"invalid_request_error","code":"invalid_request_error","param":"default_version"`}, + {"wrong type", `{"default_version":1}`, `"message":"Invalid type for 'default_version': expected a string, but got an integer instead.","type":"invalid_request_error","code":"invalid_request_error","param":"default_version"`}, + {"missing version", `{"default_version":"9"}`, `"message":"Skill version not found.","type":"invalid_request_error","code":null,"param":"default_version"`}, + } { + t.Run(test.name, func(t *testing.T) { + request := httptest.NewRequest(http.MethodPost, "/v1/skills/skill_x", strings.NewReader(test.body)) + request = request.WithContext(context.WithValue(request.Context(), adminTenantContextKey{}, "tenant")) + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + if !strings.Contains(response.Body.String(), test.want) || (test.name == "missing version") != (response.Code == http.StatusNotFound) { + t.Fatalf("%d %s", response.Code, response.Body) + } + }) + } +} + +func TestSkillContentDownloadHeaders(t *testing.T) { + fake := &fakeSkills{t: t, readDefaultVersion: func(context.Context, skills.ReadDefaultVersion) (skills.Content, error) { + return skills.Content{Version: skills.Version{Name: "proof", Version: 3}, Archive: []byte("zip")}, nil + }} + response, err := http.Get(skillServer(t, fake).URL + "/v1/skills/skill_x/content") + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + if response.Header.Get("Content-Type") != "application/zip" || response.Header.Get("Content-Disposition") != `attachment; filename=proof-3.zip` { + t.Fatal(response.Header) + } +} diff --git a/services/core/internal/api/skills_transfer.go b/services/core/internal/api/skills_transfer.go index 975017902..365692783 100644 --- a/services/core/internal/api/skills_transfer.go +++ b/services/core/internal/api/skills_transfer.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "io" "net/http" "time" @@ -34,7 +35,7 @@ func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bo var limit *http.MaxBytesError if errors.As(err, &limit) { writeContentTooLarge(w) - } else { + } else if !writeFieldError(w, err) { writeSkillsError(w, r, skills.ErrInvalidInput) } return @@ -57,7 +58,7 @@ func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bo } func (h *Handler) skillContent(w http.ResponseWriter, r *http.Request) { - err := serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { + err := serveStoredContent(w, r, "application/zip", func(ctx context.Context, consume func(string, int64, io.Reader) error) error { skill := skills.PathID(chi.URLParam(r, "skill_id")) var content skills.Content var err error @@ -69,7 +70,7 @@ func (h *Handler) skillContent(w http.ResponseWriter, r *http.Request) { if err != nil { return err } - return consume(content.Version.Name+".zip", int64(len(content.Archive)), bytes.NewReader(content.Archive)) + return consume(fmt.Sprintf("%s-%d.zip", content.Version.Name, content.Version.Version), int64(len(content.Archive)), bytes.NewReader(content.Archive)) }) if err != nil { writeSkillsError(w, r, err) diff --git a/services/core/internal/api/skills_upload.go b/services/core/internal/api/skills_upload.go index 6a98b3e5e..208a226bb 100644 --- a/services/core/internal/api/skills_upload.go +++ b/services/core/internal/api/skills_upload.go @@ -12,18 +12,17 @@ import ( "unicode/utf8" "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" ) // readSkillUpload preserves directory paths from Content-Disposition. The // multipart Part.FileName helper would discard their parent components. func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) { if r.Header.Get("Content-Encoding") != "" { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("Skill uploads do not support Content-Encoding.") } reader, err := r.MultipartReader() if err != nil { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("Skill uploads must be multipart/form-data.") } var archive bytes.Buffer writer := zip.NewWriter(&archive) @@ -42,41 +41,53 @@ func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) kind, attrs, err := mime.ParseMediaType(part.Header.Get("Content-Disposition")) field := attrs["name"] if err != nil || kind != "form-data" || part.Header.Get("Content-Transfer-Encoding") != "" { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("Each part must be form-data without a Content-Transfer-Encoding.") } if field == "default" { - if !versionUpload || seenDefault { - return nil, false, skills.ErrInvalidInput + if !versionUpload { + return nil, false, &fieldError{param: "default", message: "Unknown parameter: 'default'."} } - if _, ok := attrs["filename"]; ok { - return nil, false, skills.ErrInvalidInput + if _, ok := attrs["filename"]; ok || seenDefault { + return nil, false, &fieldError{param: "default", message: "The 'default' field must be one plain value."} } value, err := io.ReadAll(io.LimitReader(part, 6)) - if err != nil || (string(value) != "true" && string(value) != "false") { - return nil, false, skills.ErrInvalidInput + if err != nil { + return nil, false, err + } + makeDefault = strings.EqualFold(string(value), "true") + if !makeDefault && !strings.EqualFold(string(value), "false") { + return nil, false, invalidValue("default", string(value), []string{"true", "false"}) } seenDefault = true - makeDefault = string(value) == "true" } else { name := attrs["filename"] - if (field != "files" && field != "files[]") || name == "" || files >= 500 || zipUpload { - return nil, false, skills.ErrInvalidInput + if field != "files" && field != "files[]" { + if !echoableField(field) { + return nil, false, errUnknownParameter + } + return nil, false, &fieldError{param: field, message: "Unknown parameter: '" + field + "'."} + } + if name == "" { + return nil, false, invalidFiles("Each 'files' part must be a file with a filename.") + } + if files >= 500 || zipUpload { + return nil, false, invalidFiles("Upload either one ZIP file or at most 500 files, not both.") } if field == "files" { if files != 0 { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("Upload either one ZIP file or at most 500 files, not both.") } uploaded, err = io.ReadAll(io.LimitReader(part, agentskill.MaxArchiveBytes+1)) if err != nil { return nil, false, err } if len(uploaded) > agentskill.MaxArchiveBytes { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("The ZIP file exceeds 5 MiB.") } zipUpload = true } else { if !utf8.ValidString(name) || len(name) > 4096 || path.IsAbs(name) || path.Clean(name) != name || strings.ContainsAny(name, "\\\x00\r\n") || !strings.Contains(name, "/") || paths[name] { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("Each file name must be a unique, clean relative path inside a top-level folder.") } paths[name] = true body, err := io.ReadAll(io.LimitReader(part, int64(agentskill.MaxExpandedBytes-expanded)+1)) @@ -85,7 +96,7 @@ func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) } expanded += len(body) if expanded > agentskill.MaxExpandedBytes { - return nil, false, skills.ErrInvalidInput + return nil, false, invalidFiles("The files exceed 20 MiB expanded.") } header := &zip.FileHeader{Name: name, Method: zip.Deflate} header.SetMode(0644) @@ -110,13 +121,25 @@ func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) return nil, false, err } if files == 0 { - return nil, false, skills.ErrInvalidInput + return nil, false, missingParameter("files") } if !zipUpload { uploaded = archive.Bytes() } - if _, err := agentskill.Inspect(uploaded); err != nil { - return nil, false, skills.ErrInvalidInput + metadata, err := agentskill.Inspect(uploaded) + if err != nil { + return nil, false, invalidFiles("The bundle must hold one top-level folder with a valid SKILL.md and files within the limits.") + } + // Installed and plugin Skills keep the wider native rules; only this route is stricter. + if strings.Contains(metadata.Name, "_") { + return nil, false, invalidFiles("The Skill name must not contain '_'.") + } + if utf8.RuneCountInString(metadata.Description) > 1024 { + return nil, false, invalidFiles("The Skill description must be at most 1024 characters.") } return uploaded, makeDefault, nil } + +func invalidFiles(message string) error { + return &fieldError{param: "files", code: "invalid_value", message: message} +} diff --git a/services/core/internal/api/source_files_content.go b/services/core/internal/api/source_files_content.go index 7c2812f0d..50156655a 100644 --- a/services/core/internal/api/source_files_content.go +++ b/services/core/internal/api/source_files_content.go @@ -24,7 +24,7 @@ func (h *Handler) sourceFileContent(w http.ResponseWriter, r *http.Request) { // serveStoredContent streams the body read supplies. It returns read's error // while no response has started; after that, a failure aborts the response. -func serveStoredContent(w http.ResponseWriter, r *http.Request, read func(context.Context, func(string, int64, io.Reader) error) error) error { +func serveStoredContent(w http.ResponseWriter, r *http.Request, contentType string, read func(context.Context, func(string, int64, io.Reader) error) error) error { deadline := time.Now().Add(sourceTransferTimeout) if http.NewResponseController(w).SetWriteDeadline(deadline) != nil { writeError(w, http.StatusServiceUnavailable, "file_transfer_unavailable", "Bounded file transfer is unavailable.") @@ -34,7 +34,7 @@ func serveStoredContent(w http.ResponseWriter, r *http.Request, read func(contex defer cancel() started := false err := read(ctx, func(filename string, size int64, body io.Reader) error { - w.Header().Set("Content-Type", "application/octet-stream") + w.Header().Set("Content-Type", contentType) w.Header().Set("Content-Disposition", mime.FormatMediaType("attachment", map[string]string{"filename": filename})) w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) w.Header().Set("Cache-Control", "no-store") diff --git a/services/core/internal/api/source_files_test.go b/services/core/internal/api/source_files_test.go index 9adcd126a..a45392fdd 100644 --- a/services/core/internal/api/source_files_test.go +++ b/services/core/internal/api/source_files_test.go @@ -265,7 +265,7 @@ func TestEnvironmentSourceCopyEnforcesScopeUnionAndSize(t *testing.T) { func TestStoredContentDoesNotCompleteTruncatedResponse(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - serveStoredContent(w, r, func(_ context.Context, consume func(string, int64, io.Reader) error) error { + serveStoredContent(w, r, "application/octet-stream", func(_ context.Context, consume func(string, int64, io.Reader) error) error { return consume("source.bin", 3, strings.NewReader("x")) }) })) diff --git a/services/core/internal/persistence/postgres/skillpg/skillpg.go b/services/core/internal/persistence/postgres/skillpg/skillpg.go index cc3089737..37a8285f7 100644 --- a/services/core/internal/persistence/postgres/skillpg/skillpg.go +++ b/services/core/internal/persistence/postgres/skillpg/skillpg.go @@ -108,6 +108,9 @@ func (s *Store) SetDefaultVersion(ctx context.Context, tenantID string, skillID return err } target, err := q.GetSkillVersion(ctx, sqlc.GetSkillVersionParams{TenantID: tenant, SkillID: pgID(skillID), Version: version}) + if errors.Is(err, pgx.ErrNoRows) { + return skills.ErrVersionNotFound + } if err != nil { return err } diff --git a/services/core/internal/persistence/postgres/skillpg/skillpg_test.go b/services/core/internal/persistence/postgres/skillpg/skillpg_test.go index d843546e5..2a0ab10a5 100644 --- a/services/core/internal/persistence/postgres/skillpg/skillpg_test.go +++ b/services/core/internal/persistence/postgres/skillpg/skillpg_test.go @@ -197,7 +197,7 @@ func TestOwnershipEncryptionAndVersions(t *testing.T) { if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, After: "3", Limit: 20, Ascending: true}); !errors.As(err, &cursorErr) || cursorErr.Message != "Invalid 'after': '3'. Expected an ID that begins with 'skillver'." { t.Fatal("numeric version is not a cursor", err) } - if _, err = f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "999"}); !errors.Is(err, skills.ErrNotFound) { + if _, err = f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "999"}); !errors.Is(err, skills.ErrVersionNotFound) { t.Fatal("missing default", err) } updated, err := f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "3"}) @@ -341,7 +341,7 @@ func TestMetadataTracksDefaultVersion(t *testing.T) { if _, err := replaced.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: foreign, SkillID: id, Version: "2"}); !errors.Is(err, skills.ErrNotFound) { t.Fatal("foreign default update", err) } - if _, err := replaced.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "999"}); !errors.Is(err, skills.ErrNotFound) { + if _, err := replaced.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "999"}); !errors.Is(err, skills.ErrVersionNotFound) { t.Fatal("missing default update", err) } assertStored(1, 2) diff --git a/services/core/internal/skills/errors.go b/services/core/internal/skills/errors.go index 5d63bfcf0..28683951f 100644 --- a/services/core/internal/skills/errors.go +++ b/services/core/internal/skills/errors.go @@ -12,6 +12,9 @@ var ( // Malformed identifiers and other tenants' resources are indistinguishable // from missing ones. ErrNotFound = errors.New("skill not found") + // ErrVersionNotFound reports a version number that names no version of an + // existing Skill. + ErrVersionNotFound = errors.New("skill version not found") // ErrInvalidInput reports an invalid archive, version, page limit or tenant. ErrInvalidInput = errors.New("invalid skill request") // ErrDefaultVersion rejects deleting the default version while other diff --git a/services/core/internal/skills/service.go b/services/core/internal/skills/service.go index d1c089577..6e43d4069 100644 --- a/services/core/internal/skills/service.go +++ b/services/core/internal/skills/service.go @@ -71,7 +71,7 @@ type SetDefaultVersion struct { func (s *Service) SetDefaultVersion(ctx context.Context, c SetDefaultVersion) (Skill, error) { number, err := ParseVersion(c.Version) if err != nil { - return Skill{}, ErrInvalidInput + return Skill{}, ErrVersionNotFound } return s.storage.SetDefaultVersion(ctx, c.TenantID, c.SkillID, number) } diff --git a/services/core/internal/skills/service_test.go b/services/core/internal/skills/service_test.go index def5bbba9..7c0447614 100644 --- a/services/core/internal/skills/service_test.go +++ b/services/core/internal/skills/service_test.go @@ -203,7 +203,7 @@ func TestSetDefaultVersionRequiresAConcreteVersion(t *testing.T) { service, storage, _ := newTestService(t) skill := uuid.New() for _, value := range []string{"", "latest", "0", "01", "-1", "x"} { - if _, err := service.SetDefaultVersion(t.Context(), SetDefaultVersion{TenantID: "tenant", SkillID: skill, Version: value}); !errors.Is(err, ErrInvalidInput) { + if _, err := service.SetDefaultVersion(t.Context(), SetDefaultVersion{TenantID: "tenant", SkillID: skill, Version: value}); !errors.Is(err, ErrVersionNotFound) { t.Fatalf("%q: %v", value, err) } } From f21a570d73beb62652f4a746e95850bbde1f8496 Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Sat, 24 Oct 2026 16:52:47 +0000 Subject: [PATCH 2/3] Keep the Skills update body rules outside the shared gate --- services/core/internal/api/skills.go | 34 +++++++++------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/services/core/internal/api/skills.go b/services/core/internal/api/skills.go index a69b11683..b62e2ca3a 100644 --- a/services/core/internal/api/skills.go +++ b/services/core/internal/api/skills.go @@ -57,39 +57,27 @@ func (h *Handler) getSkill(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, skillResponseResource(value)) } +var skillUpdateShape = shape{kind: objectValue, members: []member{{"default_version", requiredString}}} + func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { body, ok := readJSONBodyLimit(w, r, 64<<10, "Request exceeds 64 KiB.") if !ok { return } - raw, err := jsonObjectBody(body) - if err != nil { - writeFieldError(w, err) - return - } - if field, found := unknownBodyField(raw, "default_version"); found { - if !echoableField(field) { - writeFieldError(w, errUnknownParameter) + // The Skills routes keep their own body rules; a well-formed object reports + // its field errors first. + if json.Valid(body) && jsonValueKind(body) == "an object" { + if err := checkValue("", body, skillUpdateShape); err != nil { + writeFieldError(w, err) return } - writeFieldError(w, &fieldError{param: field, message: "Unknown parameter: '" + field + "'."}) - return - } - var input struct { - DefaultVersion json.RawMessage `json:"default_version"` - } - _ = json.Unmarshal(raw, &input) - var version string - if len(input.DefaultVersion) == 0 { - writeFieldError(w, missingParameter("default_version")) - return } - if err := checkValue("default_version", input.DefaultVersion, shape{kind: stringValue}); err != nil { - writeFieldError(w, err) + var input v1.SkillUpdateRequest + if decodeInputObject(body, &input, "default_version") != nil { + writeSkillsError(w, r, skills.ErrInvalidInput) return } - _ = json.Unmarshal(input.DefaultVersion, &version) - value, err := h.Skills.SetDefaultVersion(r.Context(), skills.SetDefaultVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Version: version}) + value, err := h.Skills.SetDefaultVersion(r.Context(), skills.SetDefaultVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Version: input.DefaultVersion}) if err != nil { writeSkillsError(w, r, err) return From 13cb8f5ca92b14aa8a76634a0bd534394b795e9c Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Sun, 25 Oct 2026 02:39:05 +0000 Subject: [PATCH 3/3] Keep 400 for malformed Skill versions and the generic not-found message --- contracts/agents-api/core.openapi.yaml | 4 ++-- services/core/internal/api/admin_skills_routes.go | 4 ++-- services/core/internal/api/errors_skills.go | 11 ++++++----- services/core/internal/api/errors_skills_test.go | 2 +- .../core/internal/api/skills_field_errors_test.go | 2 +- services/core/internal/skills/service.go | 2 +- services/core/internal/skills/service_test.go | 2 +- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/contracts/agents-api/core.openapi.yaml b/contracts/agents-api/core.openapi.yaml index 0a95caad4..a0448560b 100644 --- a/contracts/agents-api/core.openapi.yaml +++ b/contracts/agents-api/core.openapi.yaml @@ -6311,7 +6311,7 @@ paths: required: true type: string produces: - - application/octet-stream + - application/zip responses: "200": description: OK @@ -6447,7 +6447,7 @@ paths: required: true type: string produces: - - application/octet-stream + - application/zip responses: "200": description: OK diff --git a/services/core/internal/api/admin_skills_routes.go b/services/core/internal/api/admin_skills_routes.go index 583968e5b..bd0a77c05 100644 --- a/services/core/internal/api/admin_skills_routes.go +++ b/services/core/internal/api/admin_skills_routes.go @@ -46,7 +46,7 @@ func (h *Handler) adminDeleteSkill(w http.ResponseWriter, r *http.Request) { // @Summary Download Skill content in a Project // @Description Core key only. Reuses the public resource projection and operation rules; the Project ID selects the target space and does not authenticate. // @Tags Skills -// @Produce octet-stream +// @Produce application/zip // @Security DeploymentAdminAuth // @Param skill_id path string true "Skill ID" // @Success 200 {file} binary @@ -103,7 +103,7 @@ func (h *Handler) adminDeleteSkillVersion(w http.ResponseWriter, r *http.Request // @Summary Download immutable Skill version content in a Project // @Description Core key only. Reuses the public resource projection and operation rules; the Project ID selects the target space and does not authenticate. // @Tags Skills -// @Produce octet-stream +// @Produce application/zip // @Security DeploymentAdminAuth // @Param skill_id path string true "Skill ID" // @Param version path string true "Concrete version number" diff --git a/services/core/internal/api/errors_skills.go b/services/core/internal/api/errors_skills.go index fe59db136..736e2e4f9 100644 --- a/services/core/internal/api/errors_skills.go +++ b/services/core/internal/api/errors_skills.go @@ -23,14 +23,15 @@ func writeSkillsError(w http.ResponseWriter, r *http.Request, err error) { } else { writeError(w, http.StatusBadRequest, "invalid_request_error", cursor.Message) } - case errors.Is(err, skills.ErrVersionNotFound): - writeError(w, http.StatusNotFound, "", "Skill version not found.", "default_version") - case errors.Is(err, skills.ErrNotFound): - code := "not_found_error" + case errors.Is(err, skills.ErrNotFound), errors.Is(err, skills.ErrVersionNotFound): + code, param := "not_found_error", []string(nil) if skillsRoute { code = "" } - writeError(w, http.StatusNotFound, code, "Resource not found.") + if errors.Is(err, skills.ErrVersionNotFound) { + param = []string{"default_version"} + } + writeError(w, http.StatusNotFound, code, "Resource not found.", param...) case errors.Is(err, skills.ErrInvalidInput): writeError(w, http.StatusBadRequest, "invalid_request", invalidInputMessage) case writeAuditSourceError(w, r, err) || writeTextValueError(w, r, err): diff --git a/services/core/internal/api/errors_skills_test.go b/services/core/internal/api/errors_skills_test.go index 2730864bd..c7431eb29 100644 --- a/services/core/internal/api/errors_skills_test.go +++ b/services/core/internal/api/errors_skills_test.go @@ -35,7 +35,7 @@ func TestWriteSkillsError(t *testing.T) { {"project not found", project, skills.ErrNotFound, http.StatusNotFound, `{"error":{"message":"Resource not found.","type":"not_found_error","code":"not_found_error","param":null}}`}, {"default version missing", "/v1/skills/skill_missing", skills.ErrVersionNotFound, http.StatusNotFound, - `{"error":{"message":"Skill version not found.","type":"invalid_request_error","code":null,"param":"default_version"}}`}, + `{"error":{"message":"Resource not found.","type":"invalid_request_error","code":null,"param":"default_version"}}`}, {"invalid input", "/v1/skills", skills.ErrInvalidInput, http.StatusBadRequest, `{"error":{"message":"Invalid resource identifier or request limits.","type":"invalid_request_error","code":"invalid_request","param":null}}`}, {"unstorable text", "/v1/skills", fmt.Errorf("create: %w", textvalue.ErrUnstorable), http.StatusBadRequest, diff --git a/services/core/internal/api/skills_field_errors_test.go b/services/core/internal/api/skills_field_errors_test.go index aa9ae7754..a1be754f4 100644 --- a/services/core/internal/api/skills_field_errors_test.go +++ b/services/core/internal/api/skills_field_errors_test.go @@ -125,7 +125,7 @@ func TestSkillUpdateErrors(t *testing.T) { {"unknown key", `{"default_version":"1","x":1}`, `"message":"Unknown parameter: 'x'.","type":"invalid_request_error","code":"invalid_request_error","param":"x"`}, {"missing", `{}`, `"message":"Missing required parameter: 'default_version'.","type":"invalid_request_error","code":"invalid_request_error","param":"default_version"`}, {"wrong type", `{"default_version":1}`, `"message":"Invalid type for 'default_version': expected a string, but got an integer instead.","type":"invalid_request_error","code":"invalid_request_error","param":"default_version"`}, - {"missing version", `{"default_version":"9"}`, `"message":"Skill version not found.","type":"invalid_request_error","code":null,"param":"default_version"`}, + {"missing version", `{"default_version":"9"}`, `"message":"Resource not found.","type":"invalid_request_error","code":null,"param":"default_version"`}, } { t.Run(test.name, func(t *testing.T) { request := httptest.NewRequest(http.MethodPost, "/v1/skills/skill_x", strings.NewReader(test.body)) diff --git a/services/core/internal/skills/service.go b/services/core/internal/skills/service.go index 6e43d4069..d1c089577 100644 --- a/services/core/internal/skills/service.go +++ b/services/core/internal/skills/service.go @@ -71,7 +71,7 @@ type SetDefaultVersion struct { func (s *Service) SetDefaultVersion(ctx context.Context, c SetDefaultVersion) (Skill, error) { number, err := ParseVersion(c.Version) if err != nil { - return Skill{}, ErrVersionNotFound + return Skill{}, ErrInvalidInput } return s.storage.SetDefaultVersion(ctx, c.TenantID, c.SkillID, number) } diff --git a/services/core/internal/skills/service_test.go b/services/core/internal/skills/service_test.go index 7c0447614..def5bbba9 100644 --- a/services/core/internal/skills/service_test.go +++ b/services/core/internal/skills/service_test.go @@ -203,7 +203,7 @@ func TestSetDefaultVersionRequiresAConcreteVersion(t *testing.T) { service, storage, _ := newTestService(t) skill := uuid.New() for _, value := range []string{"", "latest", "0", "01", "-1", "x"} { - if _, err := service.SetDefaultVersion(t.Context(), SetDefaultVersion{TenantID: "tenant", SkillID: skill, Version: value}); !errors.Is(err, ErrVersionNotFound) { + if _, err := service.SetDefaultVersion(t.Context(), SetDefaultVersion{TenantID: "tenant", SkillID: skill, Version: value}); !errors.Is(err, ErrInvalidInput) { t.Fatalf("%q: %v", value, err) } }