From db7bc4f9bae549cfe83c99cd9b0dcd767d6e4dd3 Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Wed, 30 Sep 2026 16:37:37 +0000 Subject: [PATCH] Move Skills and skill versions into the skills domain and skillpg The skills package owns the Skill vocabulary, identifiers, version selection, the version-deletion decision, content checks and the use cases; skillpg stores Skills and their sealed versions in PostgreSQL and records their write audit through auditpg. The /v1 and /core/v1 project Skill routes call skills.Service for use cases and skills.Reader for plain reads, and errors_skills.go maps the domain errors to the existing responses. environmentconfig checks Skill version selectors with skills.ValidateSelector. store keeps Session creation's frozen Skill snapshot, which now uses the skills selection, binding and content rules. Store Skill operations, their cursor errors and their tests are deleted or moved to the domain and adapter packages. Without a credential key, Skill uploads and content reads fail with credentialcrypto.ErrUnavailable and answer 503. --- services/core/IMPLEMENTATION.md | 5 +- services/core/cmd/server/http_routes_test.go | 3 +- services/core/cmd/server/main.go | 9 +- services/core/internal/api/dependencies.go | 3 +- .../core/internal/api/dependencies_test.go | 6 +- services/core/internal/api/errors.go | 19 +- services/core/internal/api/errors_skills.go | 38 ++ .../core/internal/api/errors_skills_test.go | 57 ++ services/core/internal/api/errors_test.go | 8 +- services/core/internal/api/fakes_test.go | 112 ++-- .../core/internal/api/resource_query_test.go | 87 ++- .../core/internal/api/session_artifacts.go | 5 +- services/core/internal/api/skills.go | 58 +- services/core/internal/api/skills_list.go | 9 +- services/core/internal/api/skills_transfer.go | 27 +- services/core/internal/api/skills_upload.go | 28 +- .../core/internal/api/source_files_content.go | 14 +- .../internal/api/source_files_errors_test.go | 1 - .../core/internal/credentialcrypto/skill.go | 9 + .../core/internal/environmentconfig/skills.go | 6 +- .../postgres/skillpg/audit_test.go | 226 ++++++++ .../persistence/postgres/skillpg/skillpg.go | 360 ++++++++++++ .../postgres/skillpg/skillpg_test.go | 526 ++++++++++++++++++ .../providers/configuration_flow_test.go | 3 +- services/core/internal/skills/content.go | 13 + services/core/internal/skills/deletion.go | 32 ++ services/core/internal/skills/doc.go | 5 +- services/core/internal/skills/errors.go | 39 ++ services/core/internal/skills/rules_test.go | 153 +++++ services/core/internal/skills/service.go | 223 ++++++++ services/core/internal/skills/service_test.go | 394 +++++++++++++ services/core/internal/skills/skill.go | 115 ++++ services/core/internal/skills/storage.go | 96 ++++ .../internal/store/admin_delete_audit_test.go | 41 +- .../store/environment_skill_references.go | 43 +- .../environment_skill_references_test.go | 13 +- .../internal/store/list_cursor_public_test.go | 23 +- services/core/internal/store/list_cursors.go | 19 +- .../store/path_id_semantics_public_test.go | 3 +- .../store/public_handler_fixture_test.go | 10 +- .../store/skill_default_metadata_test.go | 105 ---- .../core/internal/store/skill_fixture_test.go | 44 ++ .../skill_version_deletion_public_test.go | 16 +- .../store/skill_version_deletion_test.go | 158 +----- .../core/internal/store/skill_versions.go | 158 ------ services/core/internal/store/skills.go | 189 ------- services/core/internal/store/skills_list.go | 107 ---- .../core/internal/store/skills_list_test.go | 66 --- services/core/internal/store/skills_test.go | 166 ------ .../store/write_audit_resources_test.go | 150 ----- 50 files changed, 2632 insertions(+), 1368 deletions(-) create mode 100644 services/core/internal/api/errors_skills.go create mode 100644 services/core/internal/api/errors_skills_test.go create mode 100644 services/core/internal/persistence/postgres/skillpg/audit_test.go create mode 100644 services/core/internal/persistence/postgres/skillpg/skillpg.go create mode 100644 services/core/internal/persistence/postgres/skillpg/skillpg_test.go create mode 100644 services/core/internal/skills/content.go create mode 100644 services/core/internal/skills/deletion.go create mode 100644 services/core/internal/skills/errors.go create mode 100644 services/core/internal/skills/rules_test.go create mode 100644 services/core/internal/skills/service.go create mode 100644 services/core/internal/skills/service_test.go create mode 100644 services/core/internal/skills/skill.go create mode 100644 services/core/internal/skills/storage.go delete mode 100644 services/core/internal/store/skill_default_metadata_test.go create mode 100644 services/core/internal/store/skill_fixture_test.go delete mode 100644 services/core/internal/store/skill_versions.go delete mode 100644 services/core/internal/store/skills.go delete mode 100644 services/core/internal/store/skills_list.go delete mode 100644 services/core/internal/store/skills_list_test.go delete mode 100644 services/core/internal/store/skills_test.go delete mode 100644 services/core/internal/store/write_audit_resources_test.go diff --git a/services/core/IMPLEMENTATION.md b/services/core/IMPLEMENTATION.md index f43806063..2266ab8be 100644 --- a/services/core/IMPLEMENTATION.md +++ b/services/core/IMPLEMENTATION.md @@ -27,6 +27,7 @@ Domain owners, each with its PostgreSQL adapter under `internal/persistence/post - `vaults` (`vaultpg`): Vaults and Credentials, the encryption of Credential secrets, OAuth access-token refresh, and the MCP credential selection that Session creation freezes and the Dispatcher's `Credentials` resolves into a bearer token. - `environmenttemplates` (`templatepg`): Environment Templates, their validation and default network, their sealed setup, initial files, Skills and Plugins, and the resolved Template that Session creation composes into its Environment. - `modelconfiguration` (`modelconfigurationpg`): each Harness's deployment default model configuration and its last-use observations. +- `skills` (`skillpg`): Skills and their immutable versions: archive checks, the default and latest pointers, version selection and deletion, and each version's sealed archive. Session creation freezes selected versions inside its `store` transaction with the `skills` rules. ## Request handling @@ -42,7 +43,7 @@ On the Beta group, the OpenAI-Beta check (exactly one `agents=v1` value) runs be - Stored strings other than metadata rely on PostgreSQL rejecting U+0000 and invalid UTF-8: `pgunit.IsUnstorableText` detects SQLSTATE `22021` (text parameter) and `22P05` (`\u0000` in jsonb), and the adapter returns `textvalue.ErrUnstorable`, the 400 unstorable-text error, including for query filters such as `agent_id`. The failing statement aborts its transaction, so keep each request's writes in one transaction. - An `after` cursor that cannot name a resource on a lookup list (Agents, Sessions, Turns, Templates, Vaults, Credentials) resolves through `pgunit.LookupCursor` to the never-assigned maximum UUID and runs the normal lookup, so storage failures and missing rows behave as for a well-formed cursor. Resolve every cursor only inside its already resolved parent and tenant. -- Lists whose parent and cursor lookups are separate statements (Artifacts, Skill versions) re-check the parent before reporting a cursor 400, so a parent deleted in between still returns its 404. Item and Subagent lists read both inside one locked Session transaction. The Skill version cursor lookup is tenant-wide so another Skill's version can be told apart from a missing one; another tenant's version stays missing. +- Lists whose parent and cursor lookups are separate statements (Artifacts, and Skill versions in `skills`) re-check the parent before reporting a cursor 400, so a parent deleted in between still returns its 404. Item and Subagent lists read both inside one locked Session transaction. `skillpg` looks up a Skill version cursor tenant-wide so `skills` can tell another Skill's version from a missing one; another tenant's version stays missing. ## Source Files and Artifacts @@ -60,7 +61,7 @@ Capture bytes into private large objects without a Session admission lock. Befor - The directory helper checks each requested path component with `Root.Lstat` below `os.OpenRoot(workspace)` and opens the final directory with `O_NOFOLLOW`. A missing component, a regular file or a symbolic link maps to the distinct `not_directory` result, which the daemon and gateway carry only for directory reads; Core turns it into an empty page. `not_found` (Claude SDK adapter reader), permission, transport and uncertain results keep their errors. - A Files.create write intent stores a digest of the path, size and content, not a path ledger, so Core cannot tell a file an earlier Files.create wrote from any other file; an existing regular file therefore gets the untracked-file message. Reserve the intent under the Session lock before dispatch. The daemon verifies the complete body's SHA-256 before calling the writer, and the writer creates parents with `Root.MkdirAll(0700)`, writes `.oac-write-` in the workspace root and publishes it with `Root.Link`, which never replaces an existing entry. Known refusals return `write_rejected` with `reason` `destination_directory` or `unsafe_destination`; Core settles the intent as `rejected`, which leaves no committed receipt and releases the mutation owner. Only an exact committed or rejected receipt settles an intent; nothing settles an unknown one automatically. - Check the 5 MiB inline bound after path validation and Base64 decoding, and before the pending-hosted check, the source File lookup and execution. The JSON body limit still admits the Base64 form of 50 MiB so that oversized inline bodies up to that size get the official message. -- Serialize Skill version uploads, default changes and version deletion on the owning Skill row lock. Deleting the default version deletes the Skill only when no other version row exists, through the same cascade as Skill deletion, so every encrypted version row goes in the same commit. `next_version` only increases. +- `skillpg` serializes Skill version uploads, default changes and version deletion on the owning Skill row lock, and `skills` decides a version deletion over the locked Skill. Deleting the default version deletes the Skill only when no other version row exists, through the same cascade as Skill deletion, so every encrypted version row goes in the same commit. `next_version` only increases. ## Scheduling, preparation and pending input diff --git a/services/core/cmd/server/http_routes_test.go b/services/core/cmd/server/http_routes_test.go index 7dfa926d0..a56fe9407 100644 --- a/services/core/cmd/server/http_routes_test.go +++ b/services/core/cmd/server/http_routes_test.go @@ -90,7 +90,8 @@ func daemonComposition(t testing.TB) http.Handler { Projects: trapProjects{keys: keys}, ModelProviders: struct{ api.ModelProviders }{}, ModelProvidersReader: struct{ api.ModelProvidersReader }{}, Vaults: struct{ api.Vaults }{}, VaultsReader: struct{ api.VaultsReader }{}, - Files: struct{ api.Files }{}, FilesReader: struct{ api.FilesReader }{}, Skills: struct{ api.Skills }{}, + Files: struct{ api.Files }{}, FilesReader: struct{ api.FilesReader }{}, + Skills: struct{ api.Skills }{}, SkillsReader: struct{ api.SkillsReader }{}, Agents: struct{ api.Agents }{}, AgentsReader: struct{ api.AgentsReader }{}, Sessions: struct{ api.Sessions }{}, SessionEvents: struct{ api.SessionEvents }{}, EnvironmentTemplates: struct{ api.EnvironmentTemplates }{}, EnvironmentTemplatesReader: struct{ api.EnvironmentTemplatesReader }{}, SessionHistory: struct{ api.SessionHistory }{}, Subagents: struct{ api.Subagents }{}, Artifacts: struct{ api.Artifacts }{}, diff --git a/services/core/cmd/server/main.go b/services/core/cmd/server/main.go index 49d64358b..df2ea7cb8 100644 --- a/services/core/cmd/server/main.go +++ b/services/core/cmd/server/main.go @@ -45,6 +45,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/filepg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/modelconfigurationpg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgunit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/skillpg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/templatepg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/vaultpg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtime" @@ -54,6 +55,7 @@ import ( historystoreresolver "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimehistory/storeresolver" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimeobs" observationstoreresolver "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimeobs/storeresolver" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/vaults" "github.com/jackc/pgx/v5/pgxpool" @@ -139,6 +141,11 @@ func run() error { if err != nil { return err } + skillStore := skillpg.New(units, credentialKey) + skillService, err := skills.NewService(skillStore, skillStore) + if err != nil { + return err + } installation, err := installationFacts(public) if err != nil { return err @@ -334,7 +341,7 @@ func run() error { Projects: executionStore, ModelProviders: modelConfigurationService, ModelProvidersReader: modelConfigurationStore, Vaults: vaultService, VaultsReader: vaultStore, - Skills: executionStore, + Skills: skillService, SkillsReader: skillStore, EnvironmentTemplates: environmentTemplates, EnvironmentTemplatesReader: templateStore, Files: fileService, FilesReader: fileStore, Agents: agentService, AgentsReader: agentStore, diff --git a/services/core/internal/api/dependencies.go b/services/core/internal/api/dependencies.go index 2b3e18efd..7a67cedcd 100644 --- a/services/core/internal/api/dependencies.go +++ b/services/core/internal/api/dependencies.go @@ -37,6 +37,7 @@ type Dependencies struct { Files Files FilesReader FilesReader Skills Skills + SkillsReader SkillsReader EnvironmentTemplates EnvironmentTemplates Agents Agents AgentsReader AgentsReader @@ -118,9 +119,9 @@ func (d Dependencies) validate() error { field{"InstallationBindings", d.InstallationBindings}, field{"Projects", d.Projects}, field{"Vaults", d.Vaults}, field{"VaultsReader", d.VaultsReader}, field{"ModelProviders", d.ModelProviders}, field{"ModelProvidersReader", d.ModelProvidersReader}, - field{"Skills", d.Skills}, field{"Files", d.Files}, field{"FilesReader", d.FilesReader}, field{"EnvironmentTemplates", d.EnvironmentTemplates}, field{"EnvironmentTemplatesReader", d.EnvironmentTemplatesReader}, + field{"Skills", d.Skills}, field{"SkillsReader", d.SkillsReader}, field{"Agents", d.Agents}, field{"AgentsReader", d.AgentsReader}, field{"Sessions", d.Sessions}, field{"SessionEvents", d.SessionEvents}, field{"SessionHistory", d.SessionHistory}, field{"Subagents", d.Subagents}, diff --git a/services/core/internal/api/dependencies_test.go b/services/core/internal/api/dependencies_test.go index 476f3a633..1903fdce3 100644 --- a/services/core/internal/api/dependencies_test.go +++ b/services/core/internal/api/dependencies_test.go @@ -23,6 +23,7 @@ type testFakes struct { files *fakeFiles filesReader *fakeFilesReader skills *fakeSkills + skillsReader *fakeSkillsReader environmentTemplates *fakeEnvironmentTemplates agents *fakeAgents agentsReader *fakeAgentsReader @@ -62,9 +63,9 @@ func testDependencies(t testing.TB) (Dependencies, *testFakes) { projects: &fakeProjects{t: t}, modelProviders: &fakeModelProviders{t: t}, modelProvidersReader: &fakeModelProvidersReader{t: t}, vaults: &fakeVaults{t: t}, vaultsReader: &fakeVaultsReader{t: t}, - skills: &fakeSkills{t: t}, environmentTemplates: &fakeEnvironmentTemplates{t: t}, environmentTemplatesReader: &fakeEnvironmentTemplatesReader{t: t}, files: &fakeFiles{t: t}, filesReader: &fakeFilesReader{t: t}, + skills: &fakeSkills{t: t}, skillsReader: &fakeSkillsReader{t: t}, agents: &fakeAgents{t: t}, agentsReader: &fakeAgentsReader{t: t}, sessions: &fakeSessions{t: t}, sessionEvents: &fakeSessionEvents{t: t}, sessionHistory: &fakeSessionHistory{t: t}, subagents: &fakeSubagents{t: t}, artifacts: &fakeArtifacts{t: t}, @@ -76,11 +77,12 @@ func testDependencies(t testing.TB) (Dependencies, *testFakes) { } return Dependencies{ Engine: "codex", CoreKeys: coreKeys(t, "admin"), InstallationBindings: f.installationBindings, - Projects: f.projects, Skills: f.skills, + Projects: f.projects, ModelProviders: f.modelProviders, ModelProvidersReader: f.modelProvidersReader, Vaults: f.vaults, VaultsReader: f.vaultsReader, Files: f.files, FilesReader: f.filesReader, EnvironmentTemplates: f.environmentTemplates, EnvironmentTemplatesReader: f.environmentTemplatesReader, + Skills: f.skills, SkillsReader: f.skillsReader, Agents: f.agents, AgentsReader: f.agentsReader, Sessions: f.sessions, SessionEvents: f.sessionEvents, SessionHistory: f.sessionHistory, Subagents: f.subagents, Artifacts: f.artifacts, SessionAdmin: f.sessionAdmin, diff --git a/services/core/internal/api/errors.go b/services/core/internal/api/errors.go index 198695f7c..259fcf01a 100644 --- a/services/core/internal/api/errors.go +++ b/services/core/internal/api/errors.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "net/http" - "strings" v1 "github.com/MiniMax-AI/OpenAgentCore/contracts/agents-api/v1" "github.com/MiniMax-AI/OpenAgentCore/internal/obs/log" @@ -188,8 +187,6 @@ func writeStoreError(w http.ResponseWriter, r *http.Request, err error, notFound case errors.Is(err, store.ErrRuntimeNodeUnavailable): writeError(w, http.StatusServiceUnavailable, "runtime_node_unavailable", "The selected sandbox node is unavailable or has no capacity.") - case errors.Is(err, store.ErrDefaultSkillVersion): - writeError(w, http.StatusBadRequest, "invalid_value", "Cannot delete the default skill version.", "version") case errors.Is(err, execution.ErrModelProviderRequired): writeError(w, http.StatusBadRequest, "model_provider_required", "This Session was created without a model provider and cannot run. Create a new Session with x_agents_core.model_provider or an Agent that has one saved.") case errors.Is(err, store.ErrHostedEnvironmentFailed): @@ -206,20 +203,10 @@ func writeStoreError(w http.ResponseWriter, r *http.Request, err error, notFound case errors.Is(err, execution.ErrExecutionUnavailable): writeError(w, http.StatusServiceUnavailable, "execution_unavailable", "Execution is not available on this service.") case errors.As(err, &cursor): - // Observed official fields for an unresolved list cursor: Skill versions - // use invalid_value on after, Beta lists invalid_request_error with a null param. - if listFamilyOf(r) == skillsList { - writeError(w, http.StatusBadRequest, "invalid_value", cursor.Message, "after") - } else { - writeError(w, http.StatusBadRequest, "invalid_request_error", cursor.Message) - } + // Observed official fields for an unresolved Beta list cursor, with a null param. + writeError(w, http.StatusBadRequest, "invalid_request_error", cursor.Message) case errors.Is(err, store.ErrNotFound): - code := "not_found_error" - // Skills retain their non-beta error envelope. - if strings.HasPrefix(r.URL.Path, "/v1/skills/") || r.URL.Path == "/v1/skills" { - code = "" - } - writeError(w, http.StatusNotFound, code, "Resource not found.", notFoundParam...) + writeError(w, http.StatusNotFound, "not_found_error", "Resource not found.", notFoundParam...) case errors.Is(err, store.ErrSessionNotIdle): // Observed official status, type, code, null param and message. writeError(w, http.StatusConflict, "conflict_error", "session must be durably idle or failed without required actions before deletion") diff --git a/services/core/internal/api/errors_skills.go b/services/core/internal/api/errors_skills.go new file mode 100644 index 000000000..7877587a3 --- /dev/null +++ b/services/core/internal/api/errors_skills.go @@ -0,0 +1,38 @@ +package api + +import ( + "errors" + "net/http" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" +) + +// writeSkillsError maps the skills domain's errors to the Skills responses. +// The /v1 Skills routes keep their own observed error fields; the /core/v1 +// Project routes use the Beta ones. +func writeSkillsError(w http.ResponseWriter, r *http.Request, err error) { + skillsRoute := listFamilyOf(r) == skillsList + var cursor *skills.CursorError + switch { + case errors.Is(err, skills.ErrDefaultVersion): + writeError(w, http.StatusBadRequest, "invalid_value", "Cannot delete the default skill version.", "version") + case errors.As(err, &cursor): + // Observed official fields for an unresolved version cursor. + if skillsRoute { + writeError(w, http.StatusBadRequest, "invalid_value", cursor.Message, "after") + } else { + writeError(w, http.StatusBadRequest, "invalid_request_error", cursor.Message) + } + case errors.Is(err, skills.ErrNotFound): + code := "not_found_error" + if skillsRoute { + code = "" + } + writeError(w, http.StatusNotFound, code, "Resource not found.") + case errors.Is(err, skills.ErrInvalidInput): + writeError(w, http.StatusBadRequest, "invalid_request", invalidInputMessage) + case writeAuditSourceError(w, r, err) || writeTextValueError(w, r, err) || writeCredentialUnavailableError(w, r, err): + default: + writeInternalError(w, r) + } +} diff --git a/services/core/internal/api/errors_skills_test.go b/services/core/internal/api/errors_skills_test.go new file mode 100644 index 000000000..7ff09785b --- /dev/null +++ b/services/core/internal/api/errors_skills_test.go @@ -0,0 +1,57 @@ +package api + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/textvalue" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" +) + +// The /v1 Skills routes keep their observed error fields; the /core/v1 Project +// Skills routes use the Beta ones. +func TestWriteSkillsError(t *testing.T) { + const project = "/core/v1/projects/project/skills/skill_missing" + for _, test := range []struct { + name, path string + err error + status int + body string + }{ + {"default version", "/v1/skills/skill_missing/versions/1", skills.ErrDefaultVersion, http.StatusBadRequest, + `{"error":{"message":"Cannot delete the default skill version.","type":"invalid_request_error","code":"invalid_value","param":"version"}}`}, + {"cursor", "/v1/skills/skill_missing/versions", &skills.CursorError{Message: "Skill version cursor does not match this skill."}, http.StatusBadRequest, + `{"error":{"message":"Skill version cursor does not match this skill.","type":"invalid_request_error","code":"invalid_value","param":"after"}}`}, + {"project cursor", project + "/versions", &skills.CursorError{Message: "Skill version cursor does not match this skill."}, http.StatusBadRequest, + `{"error":{"message":"Skill version cursor does not match this skill.","type":"invalid_request_error","code":"invalid_request_error","param":null}}`}, + {"list not found", "/v1/skills", skills.ErrNotFound, http.StatusNotFound, + `{"error":{"message":"Resource not found.","type":"invalid_request_error","code":null,"param":null}}`}, + {"version not found", "/v1/skills/skill_missing/versions/1", fmt.Errorf("lookup: %w", skills.ErrNotFound), http.StatusNotFound, + `{"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}}`}, + {"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, + `{"error":{"message":"` + unstorableTextMessage + `","type":"invalid_request_error","code":"invalid_request_error","param":null}}`}, + {"audit source", "/v1/skills", fmt.Errorf("record: %w", writeaudit.ErrInvalidSource), http.StatusBadRequest, + `{"error":{"message":"` + invalidInputMessage + `","type":"invalid_request_error","code":"invalid_request","param":null}}`}, + {"credential key missing", "/v1/skills", credentialcrypto.ErrUnavailable, http.StatusServiceUnavailable, + `{"error":{"message":"Credential encryption is not configured on this service.","type":"server_error","code":"credential_storage_unavailable","param":null}}`}, + {"unknown", "/v1/skills", errors.New("connection reset"), http.StatusInternalServerError, + `{"error":{"message":"The operation could not be completed.","type":"server_error","code":"internal_error","param":null}}`}, + } { + t.Run(test.name, func(t *testing.T) { + response := httptest.NewRecorder() + writeSkillsError(response, httptest.NewRequest(http.MethodGet, test.path, nil), test.err) + if response.Code != test.status || response.Body.String() != test.body+"\n" { + t.Fatalf("%d %s", response.Code, response.Body) + } + }) + } +} diff --git a/services/core/internal/api/errors_test.go b/services/core/internal/api/errors_test.go index 02618526e..ef6fefee7 100644 --- a/services/core/internal/api/errors_test.go +++ b/services/core/internal/api/errors_test.go @@ -23,7 +23,7 @@ func TestResourceNotFoundErrorSurfaces(t *testing.T) { for _, path := range []string{ "/v1/agents/missing", "/v1/vaults/missing", "/v1/agents/sessions/missing/items", "/v1/agents/environments/missing/files", - "/v1/files", "/v1/files/missing/content", "/v1/skills", "/v1/skills/missing/versions/1", + "/v1/files", "/v1/files/missing/content", } { t.Run(path, func(t *testing.T) { response := httptest.NewRecorder() @@ -56,14 +56,12 @@ func TestResourceNotFoundErrorSurfaces(t *testing.T) { } } -// An unresolved list cursor keeps its store message; Skill versions use the -// observed invalid_value code on after, Beta lists invalid_request_error with a -// null param. +// An unresolved Beta list cursor keeps its store message with the observed +// invalid_request_error code and a null param. func TestInvalidCursorErrorFields(t *testing.T) { for path, want := range map[string]string{ "/v1/agents/sessions/session/items": `{"error":{"message":"Invalid session item ID in ` + "`after`" + `","type":"invalid_request_error","code":"invalid_request_error","param":null}}`, "/v1/agents/sessions/session/subagents": `{"error":{"message":"Invalid session item ID in ` + "`after`" + `","type":"invalid_request_error","code":"invalid_request_error","param":null}}`, - "/v1/skills/skill_missing/versions": `{"error":{"message":"Invalid session item ID in ` + "`after`" + `","type":"invalid_request_error","code":"invalid_value","param":"after"}}`, "/v1/agents/sessions/session/artifacts?x=1": `{"error":{"message":"Invalid session item ID in ` + "`after`" + `","type":"invalid_request_error","code":"invalid_request_error","param":null}}`, } { response := httptest.NewRecorder() diff --git a/services/core/internal/api/fakes_test.go b/services/core/internal/api/fakes_test.go index 16fb637f4..f673bf6f3 100644 --- a/services/core/internal/api/fakes_test.go +++ b/services/core/internal/api/fakes_test.go @@ -19,9 +19,11 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimeobs" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/sandbox" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/sessions" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/vaults" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" + "github.com/google/uuid" ) // Strict fakes: one per Dependencies area, with a func field per method. A @@ -877,95 +879,99 @@ func (f *fakeSessions) AuditSessionOperation(a0 context.Context, a1 string, a2 s } type fakeSkills struct { - t testing.TB - createSkill func(context.Context, string, []byte) (store.Skill, error) - getSkill func(context.Context, string, string) (store.Skill, error) - updateSkillDefault func(context.Context, string, string, string) (store.Skill, error) - deleteSkill func(context.Context, string, string) error - listSkills func(context.Context, string, string, int, bool) (store.SkillPage, error) - createSkillVersion func(context.Context, string, string, []byte, bool) (store.SkillVersion, error) - getSkillVersion func(context.Context, string, string, string) (store.SkillVersion, error) - readSkillVersion func(context.Context, string, string, string) (store.SkillVersion, []byte, error) - readDefaultSkillVersion func(context.Context, string, string) (store.SkillVersion, []byte, error) - deleteSkillVersion func(context.Context, string, string, string) (store.SkillVersion, error) - listSkillVersions func(context.Context, string, string, string, int, bool) (store.SkillVersionPage, error) -} - -func (f *fakeSkills) CreateSkill(a0 context.Context, a1 string, a2 []byte) (store.Skill, error) { + t testing.TB + createSkill func(context.Context, skills.CreateSkill) (skills.Skill, error) + createVersion func(context.Context, skills.CreateVersion) (skills.Version, error) + setDefaultVersion func(context.Context, skills.SetDefaultVersion) (skills.Skill, error) + deleteSkill func(context.Context, skills.DeleteSkill) error + deleteVersion func(context.Context, skills.DeleteVersion) (skills.Version, error) + listSkills func(context.Context, skills.ListSkills) (skills.Page, error) + listVersions func(context.Context, skills.ListVersions) (skills.VersionPage, error) + readVersion func(context.Context, skills.ReadVersion) (skills.Content, error) + readDefaultVersion func(context.Context, skills.ReadDefaultVersion) (skills.Content, error) +} + +func (f *fakeSkills) CreateSkill(a0 context.Context, a1 skills.CreateSkill) (skills.Skill, error) { if f.createSkill == nil { unexpectedCall(f.t, "CreateSkill") } - return f.createSkill(a0, a1, a2) + return f.createSkill(a0, a1) } -func (f *fakeSkills) GetSkill(a0 context.Context, a1 string, a2 string) (store.Skill, error) { - if f.getSkill == nil { - unexpectedCall(f.t, "GetSkill") +func (f *fakeSkills) CreateVersion(a0 context.Context, a1 skills.CreateVersion) (skills.Version, error) { + if f.createVersion == nil { + unexpectedCall(f.t, "CreateVersion") } - return f.getSkill(a0, a1, a2) + return f.createVersion(a0, a1) } -func (f *fakeSkills) UpdateSkillDefault(a0 context.Context, a1 string, a2 string, a3 string) (store.Skill, error) { - if f.updateSkillDefault == nil { - unexpectedCall(f.t, "UpdateSkillDefault") +func (f *fakeSkills) SetDefaultVersion(a0 context.Context, a1 skills.SetDefaultVersion) (skills.Skill, error) { + if f.setDefaultVersion == nil { + unexpectedCall(f.t, "SetDefaultVersion") } - return f.updateSkillDefault(a0, a1, a2, a3) + return f.setDefaultVersion(a0, a1) } -func (f *fakeSkills) DeleteSkill(a0 context.Context, a1 string, a2 string) error { +func (f *fakeSkills) DeleteSkill(a0 context.Context, a1 skills.DeleteSkill) error { if f.deleteSkill == nil { unexpectedCall(f.t, "DeleteSkill") } - return f.deleteSkill(a0, a1, a2) + return f.deleteSkill(a0, a1) +} + +func (f *fakeSkills) DeleteVersion(a0 context.Context, a1 skills.DeleteVersion) (skills.Version, error) { + if f.deleteVersion == nil { + unexpectedCall(f.t, "DeleteVersion") + } + return f.deleteVersion(a0, a1) } -func (f *fakeSkills) ListSkills(a0 context.Context, a1 string, a2 string, a3 int, a4 bool) (store.SkillPage, error) { +func (f *fakeSkills) ListSkills(a0 context.Context, a1 skills.ListSkills) (skills.Page, error) { if f.listSkills == nil { unexpectedCall(f.t, "ListSkills") } - return f.listSkills(a0, a1, a2, a3, a4) + return f.listSkills(a0, a1) } -func (f *fakeSkills) CreateSkillVersion(a0 context.Context, a1 string, a2 string, a3 []byte, a4 bool) (store.SkillVersion, error) { - if f.createSkillVersion == nil { - unexpectedCall(f.t, "CreateSkillVersion") +func (f *fakeSkills) ListVersions(a0 context.Context, a1 skills.ListVersions) (skills.VersionPage, error) { + if f.listVersions == nil { + unexpectedCall(f.t, "ListVersions") } - return f.createSkillVersion(a0, a1, a2, a3, a4) + return f.listVersions(a0, a1) } -func (f *fakeSkills) GetSkillVersion(a0 context.Context, a1 string, a2 string, a3 string) (store.SkillVersion, error) { - if f.getSkillVersion == nil { - unexpectedCall(f.t, "GetSkillVersion") +func (f *fakeSkills) ReadVersion(a0 context.Context, a1 skills.ReadVersion) (skills.Content, error) { + if f.readVersion == nil { + unexpectedCall(f.t, "ReadVersion") } - return f.getSkillVersion(a0, a1, a2, a3) + return f.readVersion(a0, a1) } -func (f *fakeSkills) ReadSkillVersion(a0 context.Context, a1 string, a2 string, a3 string) (store.SkillVersion, []byte, error) { - if f.readSkillVersion == nil { - unexpectedCall(f.t, "ReadSkillVersion") +func (f *fakeSkills) ReadDefaultVersion(a0 context.Context, a1 skills.ReadDefaultVersion) (skills.Content, error) { + if f.readDefaultVersion == nil { + unexpectedCall(f.t, "ReadDefaultVersion") } - return f.readSkillVersion(a0, a1, a2, a3) + return f.readDefaultVersion(a0, a1) } -func (f *fakeSkills) ReadDefaultSkillVersion(a0 context.Context, a1 string, a2 string) (store.SkillVersion, []byte, error) { - if f.readDefaultSkillVersion == nil { - unexpectedCall(f.t, "ReadDefaultSkillVersion") - } - return f.readDefaultSkillVersion(a0, a1, a2) +type fakeSkillsReader struct { + t testing.TB + skill func(context.Context, string, uuid.UUID) (skills.Skill, error) + version func(context.Context, string, uuid.UUID, int64) (skills.Version, error) } -func (f *fakeSkills) DeleteSkillVersion(a0 context.Context, a1 string, a2 string, a3 string) (store.SkillVersion, error) { - if f.deleteSkillVersion == nil { - unexpectedCall(f.t, "DeleteSkillVersion") +func (f *fakeSkillsReader) Skill(a0 context.Context, a1 string, a2 uuid.UUID) (skills.Skill, error) { + if f.skill == nil { + unexpectedCall(f.t, "Skill") } - return f.deleteSkillVersion(a0, a1, a2, a3) + return f.skill(a0, a1, a2) } -func (f *fakeSkills) ListSkillVersions(a0 context.Context, a1 string, a2 string, a3 string, a4 int, a5 bool) (store.SkillVersionPage, error) { - if f.listSkillVersions == nil { - unexpectedCall(f.t, "ListSkillVersions") +func (f *fakeSkillsReader) Version(a0 context.Context, a1 string, a2 uuid.UUID, a3 int64) (skills.Version, error) { + if f.version == nil { + unexpectedCall(f.t, "Version") } - return f.listSkillVersions(a0, a1, a2, a3, a4, a5) + return f.version(a0, a1, a2, a3) } type fakeSubagents struct { diff --git a/services/core/internal/api/resource_query_test.go b/services/core/internal/api/resource_query_test.go index b502081b9..bacc96848 100644 --- a/services/core/internal/api/resource_query_test.go +++ b/services/core/internal/api/resource_query_test.go @@ -16,6 +16,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/agents" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/environmenttemplates" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimedevice" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/google/uuid" ) @@ -141,43 +142,36 @@ func TestSingleResourceRoutesIgnoreUnknownQueryKeys(t *testing.T) { } } -// missingSkillStore reports every Skill as missing and records list parameters. -type missingSkillStore struct { +// missingSkills reports every Skill as missing and records list parameters. +type missingSkills struct { tenants []string limit int hasMore bool } -func (s *missingSkillStore) GetSkill(_ context.Context, tenant, _ string) (store.Skill, error) { - s.tenants = append(s.tenants, tenant) - return store.Skill{}, store.ErrNotFound -} - -func (s *missingSkillStore) DeleteSkill(_ context.Context, tenant, _ string) error { - s.tenants = append(s.tenants, tenant) - return store.ErrNotFound -} - -func (s *missingSkillStore) GetSkillVersion(_ context.Context, tenant, _, _ string) (store.SkillVersion, error) { - s.tenants = append(s.tenants, tenant) - return store.SkillVersion{}, store.ErrNotFound -} - -func (s *missingSkillStore) ListSkills(_ context.Context, tenant, _ string, limit int, _ bool) (store.SkillPage, error) { - s.tenants, s.limit = append(s.tenants, tenant), limit - return store.SkillPage{HasMore: s.hasMore}, nil -} - -func (s *missingSkillStore) ListSkillVersions(_ context.Context, tenant, _, _ string, limit int, _ bool) (store.SkillVersionPage, error) { - s.tenants, s.limit = append(s.tenants, tenant), limit - return store.SkillVersionPage{HasMore: s.hasMore}, nil -} - -func skillQueryHandler(t *testing.T, s *missingSkillStore) (http.Handler, string) { +func skillQueryHandler(t *testing.T, s *missingSkills) (http.Handler, string) { t.Helper() h, tenant, _ := twoTenantHandler(t, func(_ *Dependencies, f *testFakes) { - f.skills.getSkill, f.skills.deleteSkill, f.skills.getSkillVersion = s.GetSkill, s.DeleteSkill, s.GetSkillVersion - f.skills.listSkills, f.skills.listSkillVersions = s.ListSkills, s.ListSkillVersions + f.skillsReader.skill = func(_ context.Context, tenant string, _ uuid.UUID) (skills.Skill, error) { + s.tenants = append(s.tenants, tenant) + return skills.Skill{}, skills.ErrNotFound + } + f.skillsReader.version = func(_ context.Context, tenant string, _ uuid.UUID, _ int64) (skills.Version, error) { + s.tenants = append(s.tenants, tenant) + return skills.Version{}, skills.ErrNotFound + } + f.skills.deleteSkill = func(_ context.Context, c skills.DeleteSkill) error { + s.tenants = append(s.tenants, c.TenantID) + return skills.ErrNotFound + } + f.skills.listSkills = func(_ context.Context, c skills.ListSkills) (skills.Page, error) { + s.tenants, s.limit = append(s.tenants, c.TenantID), c.Limit + return skills.Page{HasMore: s.hasMore}, nil + } + f.skills.listVersions = func(_ context.Context, c skills.ListVersions) (skills.VersionPage, error) { + s.tenants, s.limit = append(s.tenants, c.TenantID), c.Limit + return skills.VersionPage{HasMore: s.hasMore}, nil + } }) return h, tenant } @@ -197,7 +191,7 @@ func TestSkillResourceRoutesIgnoreUnknownQueryKeys(t *testing.T) { {http.MethodGet, "/v1/skills/skill_missing/versions/1"}, } { t.Run(route.method+" "+route.path, func(t *testing.T) { - s := &missingSkillStore{} + s := &missingSkills{} h, tenant := skillQueryHandler(t, s) plain := skillQueryRequest(h, route.method, route.path) query := skillQueryRequest(h, route.method, route.path+"?tenant_id=foreign&limit=5&unknown=1") @@ -211,7 +205,7 @@ func TestSkillResourceRoutesIgnoreUnknownQueryKeys(t *testing.T) { func TestSkillListLimitZeroReturnsEmptyPage(t *testing.T) { for _, path := range []string{"/v1/skills", "/v1/skills/skill_example/versions"} { for _, hasMore := range []bool{true, false} { - s := &missingSkillStore{hasMore: hasMore} + s := &missingSkills{hasMore: hasMore} h, tenant := skillQueryHandler(t, s) w := skillQueryRequest(h, http.MethodGet, path+"?limit=0&unknown=1") want := fmt.Sprintf(`{"object":"list","data":[],"first_id":null,"last_id":null,"has_more":%t}`, hasMore) @@ -317,26 +311,27 @@ func TestSourceFileUploadIgnoresUnknownQueryKeys(t *testing.T) { } } -// ownedSkillStore accepts uploads and knows one owned Skill. -type ownedSkillStore struct { +// ownedSkills accepts uploads and knows one owned Skill. +type ownedSkills struct { owner string + ownedID uuid.UUID tenants []string defaults []bool created int } -func (s *ownedSkillStore) CreateSkill(_ context.Context, tenant string, _ []byte) (store.Skill, error) { - s.tenants, s.created = append(s.tenants, tenant), s.created+1 - return store.Skill{ID: "skill_created", Name: "proof", DefaultVersion: 1, LatestVersion: 1}, nil +func (s *ownedSkills) CreateSkill(_ context.Context, c skills.CreateSkill) (skills.Skill, error) { + s.tenants, s.created = append(s.tenants, c.TenantID), s.created+1 + return skills.Skill{ID: "skill_created", Name: "proof", DefaultVersion: 1, LatestVersion: 1}, nil } -func (s *ownedSkillStore) CreateSkillVersion(_ context.Context, tenant, skill string, _ []byte, makeDefault bool) (store.SkillVersion, error) { - s.tenants, s.defaults = append(s.tenants, tenant), append(s.defaults, makeDefault) - if tenant != s.owner || skill != "skill_owned" { - return store.SkillVersion{}, store.ErrNotFound +func (s *ownedSkills) CreateVersion(_ context.Context, c skills.CreateVersion) (skills.Version, error) { + s.tenants, s.defaults = append(s.tenants, c.TenantID), append(s.defaults, c.MakeDefault) + if c.TenantID != s.owner || c.SkillID != s.ownedID { + return skills.Version{}, skills.ErrNotFound } s.created++ - return store.SkillVersion{ID: "skillver_created", SkillID: skill, Name: "proof", Version: 2}, nil + return skills.Version{ID: "skillver_created", SkillID: skills.FormatID(c.SkillID), Name: "proof", Version: 2}, nil } func skillUpload(t *testing.T, include bool) ([]byte, string) { @@ -359,9 +354,9 @@ func skillUpload(t *testing.T, include bool) ([]byte, string) { } func TestSkillUploadsIgnoreUnknownQueryKeys(t *testing.T) { - s := &ownedSkillStore{} + s := &ownedSkills{ownedID: uuid.New()} h, owner, foreign := twoTenantHandler(t, func(_ *Dependencies, f *testFakes) { - f.skills.createSkill, f.skills.createSkillVersion = s.CreateSkill, s.CreateSkillVersion + f.skills.createSkill, f.skills.createVersion = s.CreateSkill, s.CreateVersion }) s.owner = owner server := newSourceFileServer(t, h) @@ -376,12 +371,12 @@ func TestSkillUploadsIgnoreUnknownQueryKeys(t *testing.T) { t.Fatalf("create: %d %s %v", status, raw, s.tenants) } // Version creation: a foreign Skill equals a missing one and writes nothing. - _, denied := sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_owned/versions"+query, "foreign-key", contentType, body) + _, denied := sourceRequest(t, server, http.MethodPost, "/v1/skills/"+skills.FormatID(s.ownedID)+"/versions"+query, "foreign-key", contentType, body) _, missing := sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_missing/versions"+query, "test-api-key", contentType, body) if string(denied) != string(missing) || !strings.Contains(string(denied), "Resource not found.") || s.created != 1 || s.tenants[1] != foreign { t.Fatalf("foreign version: %s / %s tenants=%v", denied, missing, s.tenants) } - status, raw = sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_owned/versions"+query, "test-api-key", contentType, body) + status, raw = sourceRequest(t, server, http.MethodPost, "/v1/skills/"+skills.FormatID(s.ownedID)+"/versions"+query, "test-api-key", contentType, body) // The default query key is not the multipart default field. if status != http.StatusOK || !strings.Contains(string(raw), `"version":"2"`) || s.created != 2 || s.defaults[len(s.defaults)-1] { t.Fatalf("version: %d %s defaults=%v", status, raw, s.defaults) diff --git a/services/core/internal/api/session_artifacts.go b/services/core/internal/api/session_artifacts.go index c0befbfe5..1a835b169 100644 --- a/services/core/internal/api/session_artifacts.go +++ b/services/core/internal/api/session_artifacts.go @@ -102,11 +102,14 @@ func (h *Handler) deleteSessionArtifact(w http.ResponseWriter, r *http.Request) // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/artifacts/{artifact_id}/content [get] func (h *Handler) sessionArtifactContent(w http.ResponseWriter, r *http.Request) { - serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { + err := serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { return h.Artifacts.ReadSessionArtifact(ctx, tenantID(r), chi.URLParam(r, "session_id"), chi.URLParam(r, "artifact_id"), func(a store.SessionArtifact, body io.Reader) error { return consume(path.Base(a.Path), a.SizeBytes, body) }) }) + if err != nil { + writeStoreError(w, r, err) + } } func artifactResponse(a store.SessionArtifact) v1.SessionArtifact { diff --git a/services/core/internal/api/skills.go b/services/core/internal/api/skills.go index 6cb0ea051..948103faa 100644 --- a/services/core/internal/api/skills.go +++ b/services/core/internal/api/skills.go @@ -6,23 +6,29 @@ import ( "strconv" v1 "github.com/MiniMax-AI/OpenAgentCore/contracts/agents-api/v1" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/go-chi/chi/v5" + "github.com/google/uuid" ) -// Skills manages Skills and their immutable versions. +// Skills runs the Skill use cases: uploads, the default pointer, deletion, +// lists and content reads. type Skills interface { - CreateSkill(context.Context, string, []byte) (store.Skill, error) - GetSkill(context.Context, string, string) (store.Skill, error) - UpdateSkillDefault(context.Context, string, string, string) (store.Skill, error) - DeleteSkill(context.Context, string, string) error - ListSkills(context.Context, string, string, int, bool) (store.SkillPage, error) - CreateSkillVersion(context.Context, string, string, []byte, bool) (store.SkillVersion, error) - GetSkillVersion(context.Context, string, string, string) (store.SkillVersion, error) - ReadSkillVersion(context.Context, string, string, string) (store.SkillVersion, []byte, error) - ReadDefaultSkillVersion(context.Context, string, string) (store.SkillVersion, []byte, error) - DeleteSkillVersion(context.Context, string, string, string) (store.SkillVersion, error) - ListSkillVersions(context.Context, string, string, string, int, bool) (store.SkillVersionPage, error) + CreateSkill(context.Context, skills.CreateSkill) (skills.Skill, error) + CreateVersion(context.Context, skills.CreateVersion) (skills.Version, error) + SetDefaultVersion(context.Context, skills.SetDefaultVersion) (skills.Skill, error) + DeleteSkill(context.Context, skills.DeleteSkill) error + DeleteVersion(context.Context, skills.DeleteVersion) (skills.Version, error) + ListSkills(context.Context, skills.ListSkills) (skills.Page, error) + ListVersions(context.Context, skills.ListVersions) (skills.VersionPage, error) + ReadVersion(context.Context, skills.ReadVersion) (skills.Content, error) + ReadDefaultVersion(context.Context, skills.ReadDefaultVersion) (skills.Content, error) +} + +// SkillsReader reads Skill and version metadata. +type SkillsReader interface { + Skill(ctx context.Context, tenantID string, id uuid.UUID) (skills.Skill, error) + Version(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (skills.Version, error) } func (h *Handler) registerSkillRoutes(r chi.Router) { @@ -50,9 +56,9 @@ func (h *Handler) registerSkillRoutes(r chi.Router) { // @Success 200 {object} v1.Skill // @Router /skills/{skill_id} [get] func (h *Handler) getSkill(w http.ResponseWriter, r *http.Request) { - value, err := h.Skills.GetSkill(r.Context(), tenantID(r), chi.URLParam(r, "skill_id")) + value, err := h.SkillsReader.Skill(r.Context(), tenantID(r), skills.PathID(chi.URLParam(r, "skill_id"))) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, skillResponseResource(value)) @@ -75,12 +81,12 @@ func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { } var input v1.SkillUpdateRequest if decodeInputObject(body, &input, "default_version") != nil || input.DefaultVersion == "" { - writeStoreError(w, r, store.ErrInvalidInput) + writeSkillsError(w, r, skills.ErrInvalidInput) return } - value, err := h.Skills.UpdateSkillDefault(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), input.DefaultVersion) + 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 { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, skillResponseResource(value)) @@ -96,8 +102,8 @@ func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { // @Router /skills/{skill_id} [delete] func (h *Handler) deleteSkill(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "skill_id") - if err := h.Skills.DeleteSkill(r.Context(), tenantID(r), id); err != nil { - writeStoreError(w, r, err) + if err := h.Skills.DeleteSkill(r.Context(), skills.DeleteSkill{TenantID: tenantID(r), SkillID: skills.PathID(id)}); err != nil { + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, v1.SkillDeleted{ID: id, Object: "skill.deleted", Deleted: true}) @@ -112,9 +118,9 @@ func (h *Handler) deleteSkill(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.SkillVersion // @Router /skills/{skill_id}/versions/{version} [get] func (h *Handler) getSkillVersion(w http.ResponseWriter, r *http.Request) { - value, err := h.Skills.GetSkillVersion(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), chi.URLParam(r, "version")) + value, err := h.SkillsReader.Version(r.Context(), tenantID(r), skills.PathID(chi.URLParam(r, "skill_id")), skills.PathVersion(chi.URLParam(r, "version"))) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, skillVersionResponse(value)) @@ -130,17 +136,17 @@ func (h *Handler) getSkillVersion(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.SkillVersionDeleted // @Router /skills/{skill_id}/versions/{version} [delete] func (h *Handler) deleteSkillVersion(w http.ResponseWriter, r *http.Request) { - value, err := h.Skills.DeleteSkillVersion(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), chi.URLParam(r, "version")) + value, err := h.Skills.DeleteVersion(r.Context(), skills.DeleteVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Version: skills.PathVersion(chi.URLParam(r, "version"))}) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, v1.SkillVersionDeleted{ID: value.ID, Object: "skill.version.deleted", Version: strconv.FormatInt(value.Version, 10), Deleted: true}) } -func skillResponseResource(s store.Skill) v1.Skill { +func skillResponseResource(s skills.Skill) v1.Skill { return v1.Skill{ID: s.ID, Object: "skill", CreatedAt: s.CreatedAt.Unix(), Name: s.Name, Description: s.Description, DefaultVersion: strconv.FormatInt(s.DefaultVersion, 10), LatestVersion: strconv.FormatInt(s.LatestVersion, 10)} } -func skillVersionResponse(s store.SkillVersion) v1.SkillVersion { +func skillVersionResponse(s skills.Version) v1.SkillVersion { return v1.SkillVersion{ID: s.ID, Object: "skill.version", SkillID: s.SkillID, CreatedAt: s.CreatedAt.Unix(), Name: s.Name, Description: s.Description, Version: strconv.FormatInt(s.Version, 10)} } diff --git a/services/core/internal/api/skills_list.go b/services/core/internal/api/skills_list.go index a54a6a9e1..bab40c047 100644 --- a/services/core/internal/api/skills_list.go +++ b/services/core/internal/api/skills_list.go @@ -4,6 +4,7 @@ import ( "net/http" v1 "github.com/MiniMax-AI/OpenAgentCore/contracts/agents-api/v1" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/go-chi/chi/v5" ) @@ -22,9 +23,9 @@ func (h *Handler) listSkills(w http.ResponseWriter, r *http.Request) { if !ok { return } - page, err := h.Skills.ListSkills(r.Context(), tenantID(r), options.after, options.limit, options.ascending) + page, err := h.Skills.ListSkills(r.Context(), skills.ListSkills{TenantID: tenantID(r), After: options.after, Limit: options.limit, Ascending: options.ascending}) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } result := v1.SkillList{Object: "list", Data: make([]v1.Skill, 0, len(page.Skills)), HasMore: page.HasMore} @@ -54,9 +55,9 @@ func (h *Handler) listSkillVersions(w http.ResponseWriter, r *http.Request) { if !ok { return } - page, err := h.Skills.ListSkillVersions(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), options.after, options.limit, options.ascending) + page, err := h.Skills.ListVersions(r.Context(), skills.ListVersions{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), After: options.after, Limit: options.limit, Ascending: options.ascending}) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } result := v1.SkillVersionList{Object: "list", Data: make([]v1.SkillVersion, 0, len(page.Versions)), HasMore: page.HasMore} diff --git a/services/core/internal/api/skills_transfer.go b/services/core/internal/api/skills_transfer.go index fbd9f1621..4696bd6bf 100644 --- a/services/core/internal/api/skills_transfer.go +++ b/services/core/internal/api/skills_transfer.go @@ -9,7 +9,7 @@ import ( "time" "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/go-chi/chi/v5" ) @@ -54,21 +54,21 @@ func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bo if errors.As(err, &limit) { writeContentTooLarge(w) } else { - writeStoreError(w, r, store.ErrInvalidInput) + writeSkillsError(w, r, skills.ErrInvalidInput) } return } if version { - result, err := h.Skills.CreateSkillVersion(ctx, tenantID(r), chi.URLParam(r, "skill_id"), archive, makeDefault) + result, err := h.Skills.CreateVersion(ctx, skills.CreateVersion{TenantID: tenantID(r), SkillID: skills.PathID(chi.URLParam(r, "skill_id")), Archive: archive, MakeDefault: makeDefault}) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, skillVersionResponse(result)) } else { - result, err := h.Skills.CreateSkill(ctx, tenantID(r), archive) + result, err := h.Skills.CreateSkill(ctx, skills.CreateSkill{TenantID: tenantID(r), Archive: archive}) if err != nil { - writeStoreError(w, r, err) + writeSkillsError(w, r, err) return } writeJSON(w, http.StatusOK, skillResponseResource(result)) @@ -84,20 +84,23 @@ func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bo // @Success 200 {file} binary // @Router /skills/{skill_id}/content [get] func (h *Handler) skillContent(w http.ResponseWriter, r *http.Request) { - serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { - var value store.SkillVersion - var body []byte + err := serveStoredContent(w, r, 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 if version := chi.URLParam(r, "version"); version != "" { - value, body, err = h.Skills.ReadSkillVersion(ctx, tenantID(r), chi.URLParam(r, "skill_id"), version) + content, err = h.Skills.ReadVersion(ctx, skills.ReadVersion{TenantID: tenantID(r), SkillID: skill, Version: skills.PathVersion(version)}) } else { - value, body, err = h.Skills.ReadDefaultSkillVersion(ctx, tenantID(r), chi.URLParam(r, "skill_id")) + content, err = h.Skills.ReadDefaultVersion(ctx, skills.ReadDefaultVersion{TenantID: tenantID(r), SkillID: skill}) } if err != nil { return err } - return consume(value.Name+".zip", int64(len(body)), bytes.NewReader(body)) + return consume(content.Version.Name+".zip", int64(len(content.Archive)), bytes.NewReader(content.Archive)) }) + if err != nil { + writeSkillsError(w, r, err) + } } // @Summary Download immutable Skill version content diff --git a/services/core/internal/api/skills_upload.go b/services/core/internal/api/skills_upload.go index 2c1870b97..6a98b3e5e 100644 --- a/services/core/internal/api/skills_upload.go +++ b/services/core/internal/api/skills_upload.go @@ -12,18 +12,18 @@ import ( "unicode/utf8" "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" + "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, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } reader, err := r.MultipartReader() if err != nil { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } var archive bytes.Buffer writer := zip.NewWriter(&archive) @@ -42,41 +42,41 @@ 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, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } if field == "default" { if !versionUpload || seenDefault { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } if _, ok := attrs["filename"]; ok { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } value, err := io.ReadAll(io.LimitReader(part, 6)) if err != nil || (string(value) != "true" && string(value) != "false") { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } seenDefault = true makeDefault = string(value) == "true" } else { name := attrs["filename"] if (field != "files" && field != "files[]") || name == "" || files >= 500 || zipUpload { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } if field == "files" { if files != 0 { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } 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, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } 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, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } paths[name] = true body, err := io.ReadAll(io.LimitReader(part, int64(agentskill.MaxExpandedBytes-expanded)+1)) @@ -85,7 +85,7 @@ func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) } expanded += len(body) if expanded > agentskill.MaxExpandedBytes { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } header := &zip.FileHeader{Name: name, Method: zip.Deflate} header.SetMode(0644) @@ -110,13 +110,13 @@ func readSkillUpload(r *http.Request, versionUpload bool) ([]byte, bool, error) return nil, false, err } if files == 0 { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } if !zipUpload { uploaded = archive.Bytes() } if _, err := agentskill.Inspect(uploaded); err != nil { - return nil, false, store.ErrInvalidInput + return nil, false, skills.ErrInvalidInput } return uploaded, makeDefault, nil } diff --git a/services/core/internal/api/source_files_content.go b/services/core/internal/api/source_files_content.go index 5e804e83a..e03198ba3 100644 --- a/services/core/internal/api/source_files_content.go +++ b/services/core/internal/api/source_files_content.go @@ -30,11 +30,13 @@ func (h *Handler) sourceFileContent(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "", "Not allowed to download files of purpose: user_data") } -func serveStoredContent(w http.ResponseWriter, r *http.Request, read func(context.Context, func(string, int64, io.Reader) error) error, notFoundParam ...string) { +// 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 { deadline := time.Now().Add(sourceTransferTimeout) if http.NewResponseController(w).SetWriteDeadline(deadline) != nil { writeError(w, http.StatusServiceUnavailable, "file_transfer_unavailable", "Bounded file transfer is unavailable.") - return + return nil } ctx, cancel := context.WithDeadline(r.Context(), deadline) defer cancel() @@ -53,10 +55,8 @@ func serveStoredContent(w http.ResponseWriter, r *http.Request, read func(contex } return err }) - if err != nil { - if started { - panic(http.ErrAbortHandler) - } - writeStoreError(w, r, err, notFoundParam...) + if err != nil && started { + panic(http.ErrAbortHandler) } + return err } diff --git a/services/core/internal/api/source_files_errors_test.go b/services/core/internal/api/source_files_errors_test.go index eee42750f..3dc7d8eed 100644 --- a/services/core/internal/api/source_files_errors_test.go +++ b/services/core/internal/api/source_files_errors_test.go @@ -48,7 +48,6 @@ func TestStoreErrorOptionalParameterPreservesOtherErrors(t *testing.T) { code any param []string }{ - {"/v1/skills/skill_missing", store.ErrNotFound, 404, nil, nil}, {"/v1/agents/agent_missing", store.ErrNotFound, 404, "not_found_error", nil}, } { w := httptest.NewRecorder() diff --git a/services/core/internal/credentialcrypto/skill.go b/services/core/internal/credentialcrypto/skill.go index 2522e1ae4..e827e24c9 100644 --- a/services/core/internal/credentialcrypto/skill.go +++ b/services/core/internal/credentialcrypto/skill.go @@ -2,7 +2,10 @@ package credentialcrypto import ( "encoding/json" + "strconv" "unicode/utf8" + + "github.com/google/uuid" ) // SkillBinding prevents encrypted bundles from being moved across owners or versions. @@ -13,6 +16,12 @@ type SkillBinding struct { Version string `json:"version"` } +// NewSkillBinding binds a version's archive to its tenant, Skill, version ID +// and version number. +func NewSkillBinding(tenant, skill, versionID uuid.UUID, version int64) SkillBinding { + return SkillBinding{TenantID: tenant.String(), SkillID: skill.String(), VersionID: versionID.String(), Version: strconv.FormatInt(version, 10)} +} + func (c *Cipher) SealSkill(body []byte, binding SkillBinding) ([]byte, error) { aad, err := skillData(binding) if err != nil { diff --git a/services/core/internal/environmentconfig/skills.go b/services/core/internal/environmentconfig/skills.go index 4b26dbcc5..4e4c9d6de 100644 --- a/services/core/internal/environmentconfig/skills.go +++ b/services/core/internal/environmentconfig/skills.go @@ -72,10 +72,8 @@ func validateSkills(requested []Skill, installed bool) error { if m.SkillID == "" || len(m.SkillID) > 256 || m.Name != "" || m.Description != "" || len(skill.Archive) != 0 { return ErrInvalid } - if m.Version != "" && m.Version != "latest" { - if _, err := skills.ParseVersion(m.Version); err != nil { - return ErrInvalid - } + if skills.ValidateSelector(m.Version) != nil { + return ErrInvalid } continue } diff --git a/services/core/internal/persistence/postgres/skillpg/audit_test.go b/services/core/internal/persistence/postgres/skillpg/audit_test.go new file mode 100644 index 000000000..12fca7ed9 --- /dev/null +++ b/services/core/internal/persistence/postgres/skillpg/audit_test.go @@ -0,0 +1,226 @@ +package skillpg_test + +import ( + "context" + "reflect" + "strings" + "testing" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/adminaudit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgtest" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// auditMutation is one Skill write and the audit record it must leave. +type auditMutation struct { + action, kind, parent string + owners int + run func(context.Context) (string, error) +} + +var auditMutations = []string{"skill_create", "skill_upload_version", "skill_update_default", "skill_delete", "version_delete", "version_delete_last"} + +func prepareAuditMutation(t *testing.T, f fixture, tenant, name string, bundle []byte) auditMutation { + t.Helper() + ctx := t.Context() + if name == "skill_create" { + return auditMutation{action: "create", kind: "skill", owners: 2, run: func(ctx context.Context) (string, error) { + v, e := f.service.CreateSkill(ctx, skills.CreateSkill{TenantID: tenant, Archive: bundle}) + return v.ID, e + }} + } + created, err := f.service.CreateSkill(ctx, skills.CreateSkill{TenantID: tenant, Archive: bundle}) + if err != nil { + t.Fatal(err) + } + id := key(t, created.ID) + switch name { + case "skill_upload_version": + return auditMutation{action: "upload_version", kind: "skill_version", parent: created.ID, owners: 1, run: func(ctx context.Context) (string, error) { + v, e := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: bundle, MakeDefault: true}) + return v.ID, e + }} + case "skill_delete": + return auditMutation{action: "delete", kind: "skill", run: func(ctx context.Context) (string, error) { + return created.ID, f.service.DeleteSkill(ctx, skills.DeleteSkill{TenantID: tenant, SkillID: id}) + }} + case "version_delete_last": + return auditMutation{action: "delete", kind: "skill_version", parent: created.ID, run: func(ctx context.Context) (string, error) { + v, e := f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 1}) + return v.ID, e + }} + } + if _, err = f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: bundle}); err != nil { + t.Fatal(err) + } + if name == "skill_update_default" { + return auditMutation{action: "update_default_version", kind: "skill", run: func(ctx context.Context) (string, error) { + v, e := f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "2"}) + return v.ID, e + }} + } + return auditMutation{action: "delete", kind: "skill_version", parent: created.ID, run: func(ctx context.Context) (string, error) { + v, e := f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 2}) + return v.ID, e + }} +} + +func writeAuditContext(ctx context.Context, tenant, request string) context.Context { + return writeaudit.WithSource(ctx, writeaudit.Source{ + KeyID: "static:" + strings.Repeat("a", 64), Name: "resource audit fixture", Prefix: "aaaaaaaa", + Kind: "static", TenantID: tenant, RequestID: request, TraceID: "resource-audit-trace", + }) +} + +// snapshot returns every row of tables, or only the tenant's rows when tenant +// is set, so a comparison covers ciphertext, counters and timestamps. +func snapshot(t *testing.T, pool *pgxpool.Pool, tenant string, tables ...string) map[string]string { + t.Helper() + result := make(map[string]string, len(tables)) + for _, table := range tables { + query := "SELECT COALESCE(jsonb_agg(to_jsonb(r) ORDER BY to_jsonb(r)::text)::text, '[]') FROM " + pgx.Identifier{table}.Sanitize() + " r" + var args []any + if tenant != "" { + query, args = query+" WHERE tenant_id=$1", []any{tenant} + } + var rows string + if err := pool.QueryRow(t.Context(), query, args...).Scan(&rows); err != nil { + t.Fatalf("snapshot %s: %v", table, err) + } + result[table] = rows + } + return result +} + +// A database trigger fails the final audit insertion after each real Skill +// write. Comparing complete tenant rows proves the write rolled back. +func TestWriteAuditTransactions(t *testing.T) { + pool := pgtest.OpenIsolated(t, nil) + f := newFixture(t, pool, testCipher(t, 91)) + ctx := t.Context() + if _, err := pool.Exec(ctx, `CREATE FUNCTION reject_resource_audit_fixture() RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN IF NEW.request_id = 'reject-resource-audit' THEN RAISE EXCEPTION 'forced audit insertion failure'; END IF; RETURN NEW; END $$; + CREATE TRIGGER reject_resource_audit_fixture BEFORE INSERT ON write_audit_operations FOR EACH ROW EXECUTE FUNCTION reject_resource_audit_fixture()`); err != nil { + t.Fatal(err) + } + bundle := proofArchive(t, "audit-private-archive") + tables := []string{"skills", "skill_versions", "write_audit_operations", "write_audit_owners"} + for _, name := range auditMutations { + t.Run(name, func(t *testing.T) { + tenant := uuid.NewString() + mutation := prepareAuditMutation(t, f, tenant, name, bundle) + before := snapshot(t, pool, tenant, tables...) + if _, err := mutation.run(writeAuditContext(ctx, tenant, "reject-resource-audit")); err == nil { + t.Fatal("audit failure was accepted") + } + if !reflect.DeepEqual(before, snapshot(t, pool, tenant, tables...)) { + t.Fatal("audit failure left business or audit changes") + } + request := uuid.NewString() + id, err := mutation.run(writeAuditContext(ctx, tenant, request)) + if err != nil { + t.Fatal(err) + } + var action, kind, gotID string + var parent *string + if err := pool.QueryRow(ctx, `SELECT action,resource_type,resource_id,parent_id FROM write_audit_operations WHERE tenant_id=$1 AND request_id=$2`, tenant, request).Scan(&action, &kind, &gotID, &parent); err != nil { + t.Fatal(err) + } + gotParent := "" + if parent != nil { + gotParent = *parent + } + if action != mutation.action || kind != mutation.kind || gotID != id || gotParent != mutation.parent { + t.Fatalf("wrong operation identity: %s %s %s %s", action, kind, gotID, gotParent) + } + var owners int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM write_audit_owners WHERE tenant_id=$1`, tenant).Scan(&owners); err != nil || owners != mutation.owners { + t.Fatalf("ownership count %d, want %d: %v", owners, mutation.owners, err) + } + rows := snapshot(t, pool, tenant, "write_audit_operations", "write_audit_owners") + for _, table := range rows { + if strings.Contains(table, "audit-private-archive") { + t.Fatal("audit contains the archive") + } + } + }) + } +} + +const rejectedAdminRequest = "reject-admin-mutation-fixture" + +// Administrator provenance takes precedence over an inherited public source: +// each deletion records one administrator audit row and no public-key +// operation, and a failed administrator audit rolls the deletion back. +func TestAdminDeleteAuditTransactions(t *testing.T) { + pool := pgtest.OpenIsolated(t, nil) + f := newFixture(t, pool, testCipher(t, 94)) + ctx := t.Context() + if _, err := pool.Exec(ctx, `CREATE FUNCTION reject_admin_mutation_fixture() RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN IF NEW.request_id = 'reject-admin-mutation-fixture' THEN RAISE EXCEPTION 'forced administrator audit failure'; END IF; RETURN NEW; END $$; + CREATE TRIGGER reject_admin_mutation_fixture BEFORE INSERT ON admin_audit_log FOR EACH ROW EXECUTE FUNCTION reject_admin_mutation_fixture()`); err != nil { + t.Fatal(err) + } + bundle := proofArchive(t, "admin-private-archive") + tables := []string{"skills", "skill_versions", "admin_audit_log", "write_audit_operations", "write_audit_owners"} + adminContext := func(tenant, request string) context.Context { + return adminaudit.WithSource(writeAuditContext(ctx, tenant, request), adminaudit.Source{ + CredentialID: "87654321", ActorLabel: "administrator fixture", ProjectID: tenant, RequestID: request, TraceID: "admin-mutation-trace", + }) + } + for _, name := range []string{"skill_delete", "version_delete", "version_delete_last"} { + t.Run(name, func(t *testing.T) { + tenant := uuid.NewString() + mutation := prepareAuditMutation(t, f, tenant, name, bundle) + if _, err := pool.Exec(ctx, "INSERT INTO execution_project_scopes(tenant_id,organization_id,project_id) VALUES($1,'admin-delete',$2)", tenant, tenant); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, "INSERT INTO projects(id,name,tenant_id,subject_kind,subject_id) VALUES($1,'Delete fixture',$1,'service_account',$2)", tenant, "project:"+tenant); err != nil { + t.Fatal(err) + } + before := snapshot(t, pool, "", tables...) + if _, err := mutation.run(adminContext(tenant, rejectedAdminRequest)); err == nil { + t.Fatal("administrator audit failure was accepted") + } + if !reflect.DeepEqual(before, snapshot(t, pool, "", tables...)) { + t.Fatal("failed administrator audit changed Skill or audit rows") + } + request := uuid.NewString() + id, err := mutation.run(adminContext(tenant, request)) + if err != nil { + t.Fatal(err) + } + var credential, actor, project, trace, action, kind, gotID, mappings, raw string + if err := pool.QueryRow(ctx, `SELECT admin_credential_id,actor_label,project_id,trace_id,action,resource_type,resource_id,result_ids::text,to_jsonb(a)::text + FROM admin_audit_log a WHERE tenant_id=$1 AND request_id=$2`, tenant, request).Scan(&credential, &actor, &project, &trace, &action, &kind, &gotID, &mappings, &raw); err != nil { + t.Fatal(err) + } + if credential != "87654321" || actor != "administrator fixture" || project != tenant || trace != "admin-mutation-trace" || action != "delete" || kind != mutation.kind || gotID != id || mappings != "[]" { + t.Fatal("administrator audit identity differs") + } + if strings.Contains(raw, "admin-private-archive") { + t.Fatal("private content entered administrator audit") + } + var audits, operations, owners int + if err := pool.QueryRow(ctx, `SELECT (SELECT count(*) FROM admin_audit_log WHERE tenant_id=$1 AND request_id=$2),(SELECT count(*) FROM write_audit_operations WHERE tenant_id=$1),(SELECT count(*) FROM write_audit_owners WHERE tenant_id=$1)`, tenant, request).Scan(&audits, &operations, &owners); err != nil { + t.Fatal(err) + } + if audits != 1 || operations != 0 || owners != 0 { + t.Fatal("administrator impersonated public-key provenance") + } + var remaining int + if err := pool.QueryRow(ctx, "SELECT count(*) FROM skill_versions WHERE tenant_id=$1 AND 'skillver_' || id::text = $2", tenant, id).Scan(&remaining); err != nil || remaining != 0 { + t.Fatal("deleted version survived", remaining, err) + } + if kind == "skill" { + if _, err := f.store.Skill(ctx, tenant, key(t, id)); err == nil { + t.Fatal("deleted Skill survived") + } + } + }) + } +} diff --git a/services/core/internal/persistence/postgres/skillpg/skillpg.go b/services/core/internal/persistence/postgres/skillpg/skillpg.go new file mode 100644 index 000000000..c05225094 --- /dev/null +++ b/services/core/internal/persistence/postgres/skillpg/skillpg.go @@ -0,0 +1,360 @@ +// Package skillpg stores Skills and their versions in PostgreSQL. Version +// archives are sealed with the credential key and bound to their tenant, +// Skill, version ID and number. +package skillpg + +import ( + "context" + "errors" + "fmt" + "math" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/db/sqlc" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/auditpg" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgunit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/textvalue" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" +) + +// Store is the Skills storage and Reader. +type Store struct { + pool *pgunit.Pool + cipher *credentialcrypto.Cipher +} + +var ( + _ skills.Storage = (*Store)(nil) + _ skills.Reader = (*Store)(nil) +) + +// New builds the Skill store. Without a cipher, uploads and content reads +// fail with credentialcrypto.ErrUnavailable and metadata reads still work. +func New(pool *pgunit.Pool, cipher *credentialcrypto.Cipher) *Store { + return &Store{pool: pool, cipher: cipher} +} + +func (s *Store) CreateSkill(ctx context.Context, in skills.NewSkill) (skills.Skill, error) { + tenant, err := parseTenant(in.TenantID) + if err != nil { + return skills.Skill{}, err + } + var result skills.Skill + err = s.pool.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { + q := sqlc.New(tx) + row, err := q.CreateSkill(ctx, sqlc.CreateSkillParams{ID: newID(), TenantID: tenant, Name: in.Name, Description: in.Description}) + if err != nil { + return err + } + initial, err := s.insertVersion(ctx, q, row.TenantID, row.ID, 1, in.Name, in.Description, in.Archive) + if err != nil { + return err + } + result = skillFromRow(row) + return auditpg.RecordWriteAudit(ctx, q, in.TenantID, "create", "skill", result.ID, "", + writeaudit.Resource{Type: "skill", ID: result.ID}, + writeaudit.Resource{Type: "skill_version", ID: initial.ID, ParentID: result.ID}) + }) + if err != nil { + return skills.Skill{}, translate(err) + } + return result, nil +} + +func (s *Store) CreateVersion(ctx context.Context, in skills.NewVersion) (skills.Version, error) { + tenant, err := parseTenant(in.TenantID) + if err != nil { + return skills.Version{}, err + } + var result skills.Version + err = s.pool.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { + q := sqlc.New(tx) + owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: pgID(in.SkillID)}) + if err != nil { + return err + } + if owner.NextVersion == math.MaxInt64 { + return fmt.Errorf("%w: skill version numbers exhausted", skills.ErrInvalidInput) + } + result, err = s.insertVersion(ctx, q, owner.TenantID, owner.ID, owner.NextVersion, in.Name, in.Description, in.Archive) + if err != nil { + return err + } + if err := q.AdvanceSkillVersion(ctx, sqlc.AdvanceSkillVersionParams{TenantID: owner.TenantID, ID: owner.ID, MakeDefault: in.MakeDefault, Name: in.Name, Description: in.Description}); err != nil { + return err + } + return auditpg.RecordWriteAudit(ctx, q, in.TenantID, "upload_version", "skill_version", result.ID, result.SkillID, + writeaudit.Resource{Type: "skill_version", ID: result.ID, ParentID: result.SkillID}) + }) + if err != nil { + return skills.Version{}, translate(err) + } + return result, nil +} + +func (s *Store) SetDefaultVersion(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (skills.Skill, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Skill{}, err + } + var result skills.Skill + err = s.pool.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { + q := sqlc.New(tx) + if _, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: pgID(skillID)}); err != nil { + return err + } + target, err := q.GetSkillVersion(ctx, sqlc.GetSkillVersionParams{TenantID: tenant, SkillID: pgID(skillID), Version: version}) + if err != nil { + return err + } + row, err := q.SetDefaultSkillVersion(ctx, sqlc.SetDefaultSkillVersionParams{TenantID: tenant, ID: pgID(skillID), DefaultVersion: version, Name: target.Name, Description: target.Description}) + if err != nil { + return err + } + result = skillFromRow(row) + return auditpg.RecordWriteAudit(ctx, q, tenantID, "update_default_version", "skill", result.ID, "") + }) + if err != nil { + return skills.Skill{}, translate(err) + } + return result, nil +} + +func (s *Store) DeleteSkill(ctx context.Context, tenantID string, skillID uuid.UUID) error { + tenant, err := parseTenant(tenantID) + if err != nil { + return err + } + err = s.pool.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { + q := sqlc.New(tx) + if _, err := q.DeleteSkill(ctx, sqlc.DeleteSkillParams{TenantID: tenant, ID: pgID(skillID)}); err != nil { + return err + } + return auditpg.RecordWriteAudit(ctx, q, tenantID, "delete", "skill", skills.FormatID(skillID), "") + }) + return translate(err) +} + +func (s *Store) WithVersionDeletion(ctx context.Context, tenantID string, skillID uuid.UUID, apply func(skills.VersionDeletionTx) error) error { + tenant, err := parseTenant(tenantID) + if err != nil { + return err + } + err = s.pool.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { + q := sqlc.New(tx) + owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: pgID(skillID)}) + if err != nil { + return err + } + return apply(&versionDeletion{ctx: ctx, q: q, tenantID: tenantID, skill: owner}) + }) + return translate(err) +} + +// versionDeletion is one Skill locked by WithVersionDeletion. ctx is the +// transaction's. +type versionDeletion struct { + ctx context.Context + q *sqlc.Queries + tenantID string + skill sqlc.Skill +} + +func (d *versionDeletion) LoadVersionDeletion(version int64) (skills.VersionDeletionFacts, error) { + target, err := d.q.GetSkillVersion(d.ctx, sqlc.GetSkillVersionParams{TenantID: d.skill.TenantID, SkillID: d.skill.ID, Version: version}) + if err != nil { + return skills.VersionDeletionFacts{}, translate(err) + } + // Two rows tell whether any version besides the target remains. + rows, err := d.q.ListSkillVersions(d.ctx, sqlc.ListSkillVersionsParams{TenantID: d.skill.TenantID, SkillID: d.skill.ID, PageLimit: 2}) + if err != nil { + return skills.VersionDeletionFacts{}, translate(err) + } + return skills.VersionDeletionFacts{Skill: skillFromRow(d.skill), Target: versionFromRow(target), OthersRemain: len(rows) > 1}, nil +} + +func (d *versionDeletion) ApplyVersionDeletion(decision skills.VersionDeletion) error { + number := decision.Target.Version + if decision.DeleteSkill { + // The versions go with the Skill through the foreign key cascade. + if _, err := d.q.DeleteSkill(d.ctx, sqlc.DeleteSkillParams{TenantID: d.skill.TenantID, ID: d.skill.ID}); err != nil { + return translate(err) + } + } else { + if _, err := d.q.DeleteSkillVersion(d.ctx, sqlc.DeleteSkillVersionParams{TenantID: d.skill.TenantID, SkillID: d.skill.ID, Version: number}); err != nil { + return translate(err) + } + if decision.RefreshLatest { + if err := d.q.RefreshLatestSkillVersion(d.ctx, sqlc.RefreshLatestSkillVersionParams{TenantID: d.skill.TenantID, ID: d.skill.ID}); err != nil { + return translate(err) + } + } + } + return translate(auditpg.RecordWriteAudit(d.ctx, d.q, d.tenantID, "delete", "skill_version", decision.Target.ID, decision.Target.SkillID)) +} + +func (s *Store) Skill(ctx context.Context, tenantID string, id uuid.UUID) (skills.Skill, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Skill{}, err + } + row, err := s.pool.Queries().GetSkill(ctx, sqlc.GetSkillParams{TenantID: tenant, ID: pgID(id)}) + if err != nil { + return skills.Skill{}, translate(err) + } + return skillFromRow(row), nil +} + +func (s *Store) Skills(ctx context.Context, tenantID string, page skills.SkillPageQuery) (skills.Page, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Page{}, err + } + // The extra row reports whether another page follows. + params := sqlc.ListSkillsParams{TenantID: tenant, PageLimit: int32(page.Limit + 1), Ascending: page.Ascending, AfterID: pgtype.UUID{Valid: true}} + if page.After != nil { + params.AfterCreated = pgtype.Timestamptz{Time: page.After.CreatedAt, Valid: true} + params.AfterID = pgID(page.After.ID) + } + rows, err := s.pool.Queries().ListSkills(ctx, params) + if err != nil { + return skills.Page{}, translate(err) + } + result := skills.Page{Skills: make([]skills.Skill, 0, min(page.Limit, len(rows))), HasMore: len(rows) > page.Limit} + for _, row := range rows[:min(page.Limit, len(rows))] { + result.Skills = append(result.Skills, skillFromRow(row)) + } + return result, nil +} + +func (s *Store) Version(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (skills.Version, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Version{}, err + } + row, err := s.pool.Queries().GetSkillVersion(ctx, sqlc.GetSkillVersionParams{TenantID: tenant, SkillID: pgID(skillID), Version: version}) + if err != nil { + return skills.Version{}, translate(err) + } + return versionFromRow(row), nil +} + +func (s *Store) VersionByID(ctx context.Context, tenantID string, id uuid.UUID) (skills.Version, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Version{}, err + } + row, err := s.pool.Queries().GetSkillVersionByID(ctx, sqlc.GetSkillVersionByIDParams{TenantID: tenant, ID: pgID(id)}) + if err != nil { + return skills.Version{}, translate(err) + } + return versionFromRow(sqlc.GetSkillVersionRow(row)), nil +} + +func (s *Store) Versions(ctx context.Context, tenantID string, skillID uuid.UUID, page skills.VersionPageQuery) (skills.VersionPage, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.VersionPage{}, err + } + rows, err := s.pool.Queries().ListSkillVersions(ctx, sqlc.ListSkillVersionsParams{ + TenantID: tenant, SkillID: pgID(skillID), PageLimit: int32(page.Limit + 1), Ascending: page.Ascending, + AfterVersion: pgtype.Int8{Int64: page.AfterVersion, Valid: page.AfterVersion > 0}, + }) + if err != nil { + return skills.VersionPage{}, translate(err) + } + result := skills.VersionPage{Versions: make([]skills.Version, 0, min(page.Limit, len(rows))), HasMore: len(rows) > page.Limit} + for _, row := range rows[:min(page.Limit, len(rows))] { + result.Versions = append(result.Versions, versionFromRow(sqlc.GetSkillVersionRow(row))) + } + return result, nil +} + +func (s *Store) VersionContent(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (skills.Content, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Content{}, err + } + row, err := s.pool.Queries().ReadSkillVersion(ctx, sqlc.ReadSkillVersionParams{TenantID: tenant, SkillID: pgID(skillID), Version: version}) + if err != nil { + return skills.Content{}, translate(err) + } + return s.open(row) +} + +func (s *Store) DefaultVersionContent(ctx context.Context, tenantID string, skillID uuid.UUID) (skills.Content, error) { + tenant, err := parseTenant(tenantID) + if err != nil { + return skills.Content{}, err + } + row, err := s.pool.Queries().ReadDefaultSkillVersion(ctx, sqlc.ReadDefaultSkillVersionParams{TenantID: tenant, ID: pgID(skillID)}) + if err != nil { + return skills.Content{}, translate(err) + } + return s.open(row) +} + +// insertVersion seals the archive under a new version ID and stores it. +func (s *Store) insertVersion(ctx context.Context, q *sqlc.Queries, tenant, skill pgtype.UUID, version int64, name, description string, archive []byte) (skills.Version, error) { + if s.cipher == nil { + return skills.Version{}, credentialcrypto.ErrUnavailable + } + id := newID() + body, err := s.cipher.SealSkill(archive, credentialcrypto.NewSkillBinding(tenant.Bytes, skill.Bytes, id.Bytes, version)) + if err != nil { + return skills.Version{}, err + } + row, err := q.CreateSkillVersion(ctx, sqlc.CreateSkillVersionParams{ID: id, TenantID: tenant, SkillID: skill, Version: version, Name: name, Description: description, Contents: body}) + if err != nil { + return skills.Version{}, err + } + return versionFromRow(sqlc.GetSkillVersionRow(row)), nil +} + +func (s *Store) open(row sqlc.SkillVersion) (skills.Content, error) { + if s.cipher == nil { + return skills.Content{}, credentialcrypto.ErrUnavailable + } + archive, err := s.cipher.OpenSkill(row.Contents, credentialcrypto.NewSkillBinding(row.TenantID.Bytes, row.SkillID.Bytes, row.ID.Bytes, row.Version)) + if err != nil { + return skills.Content{}, err + } + version := versionFromRow(sqlc.GetSkillVersionRow{ID: row.ID, TenantID: row.TenantID, SkillID: row.SkillID, Version: row.Version, Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt}) + return skills.Content{Version: version, Archive: archive}, nil +} + +// translate turns the database outcomes callers act on into the domain's +// errors and returns every other error as it is. +func translate(err error) error { + switch { + case errors.Is(err, pgx.ErrNoRows): + return skills.ErrNotFound + case pgunit.IsUnstorableText(err): + return textvalue.ErrUnstorable + } + return err +} + +func parseTenant(value string) (pgtype.UUID, error) { + tenant, err := pgunit.ParseID(value) + if err != nil { + return pgtype.UUID{}, fmt.Errorf("%w: tenant ID", skills.ErrInvalidInput) + } + return tenant, nil +} + +func pgID(id uuid.UUID) pgtype.UUID { return pgtype.UUID{Bytes: id, Valid: true} } + +func newID() pgtype.UUID { return pgID(uuid.New()) } + +func skillFromRow(row sqlc.Skill) skills.Skill { + return skills.Skill{ID: skills.FormatID(row.ID.Bytes), Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt.Time, DefaultVersion: row.DefaultVersion, LatestVersion: row.LatestVersion} +} + +func versionFromRow(row sqlc.GetSkillVersionRow) skills.Version { + return skills.Version{ID: skills.FormatVersionID(row.ID.Bytes), SkillID: skills.FormatID(row.SkillID.Bytes), Version: row.Version, Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt.Time} +} diff --git a/services/core/internal/persistence/postgres/skillpg/skillpg_test.go b/services/core/internal/persistence/postgres/skillpg/skillpg_test.go new file mode 100644 index 000000000..b7ebc9580 --- /dev/null +++ b/services/core/internal/persistence/postgres/skillpg/skillpg_test.go @@ -0,0 +1,526 @@ +package skillpg_test + +import ( + "archive/zip" + "bytes" + "errors" + "fmt" + "sort" + "sync" + "testing" + "time" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgtest" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgunit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/skillpg" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/textvalue" + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" +) + +// fixture is one Skill store and the service over it. +type fixture struct { + pool *pgxpool.Pool + store *skillpg.Store + service *skills.Service +} + +func newFixture(t *testing.T, pool *pgxpool.Pool, cipher *credentialcrypto.Cipher) fixture { + t.Helper() + store := skillpg.New(pgunit.NewPool(pool), cipher) + service, err := skills.NewService(store, store) + if err != nil { + t.Fatal(err) + } + return fixture{pool: pool, store: store, service: service} +} + +func testCipher(t *testing.T, seed byte) *credentialcrypto.Cipher { + t.Helper() + cipher, err := credentialcrypto.New(bytes.Repeat([]byte{seed}, 32)) + if err != nil { + t.Fatal(err) + } + return cipher +} + +func archive(t *testing.T, name, description, marker string) []byte { + t.Helper() + var buffer bytes.Buffer + writer := zip.NewWriter(&buffer) + file, err := writer.CreateHeader(&zip.FileHeader{Name: name + "/SKILL.md", Method: zip.Store}) + if err != nil { + t.Fatal(err) + } + if _, err = fmt.Fprintf(file, "---\nname: %s\ndescription: %s\n---\n%s\n", name, description, marker); err != nil { + t.Fatal(err) + } + if err = writer.Close(); err != nil { + t.Fatal(err) + } + return buffer.Bytes() +} + +func proofArchive(t *testing.T, marker string) []byte { + return archive(t, "proof", "Verify a versioned Skill.", marker) +} + +func key(t *testing.T, id string) uuid.UUID { + t.Helper() + value, err := skills.ParseID(id) + if err != nil { + t.Fatal(err) + } + return value +} + +func (f fixture) create(t *testing.T, tenant string, archive []byte) skills.Skill { + t.Helper() + created, err := f.service.CreateSkill(t.Context(), skills.CreateSkill{TenantID: tenant, Archive: archive}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = f.store.DeleteSkill(t.Context(), tenant, key(t, created.ID)) }) + return created +} + +func (f fixture) rowCounts(t *testing.T, skill uuid.UUID) (skillRows, versionRows int) { + t.Helper() + if err := f.pool.QueryRow(t.Context(), "SELECT (SELECT count(*) FROM skills WHERE id=$1), (SELECT count(*) FROM skill_versions WHERE skill_id=$1)", skill).Scan(&skillRows, &versionRows); err != nil { + t.Fatal(err) + } + return skillRows, versionRows +} + +func TestOwnershipEncryptionAndVersions(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 41)) + ctx := t.Context() + tenant, foreign := uuid.NewString(), uuid.NewString() + bundle := proofArchive(t, "confidential-skill-canary") + created := f.create(t, tenant, bundle) + id := key(t, created.ID) + if created.DefaultVersion != 1 || created.LatestVersion != 1 { + t.Fatal("initial pointers", created) + } + metadata, err := skillpg.New(pgunit.NewPool(f.pool), nil).Skill(ctx, tenant, id) + if err != nil || metadata.Name != "proof" { + t.Fatal("metadata requires no content key", err) + } + var contents []byte + if err = f.pool.QueryRow(ctx, "SELECT contents FROM skill_versions WHERE tenant_id=$1", tenant).Scan(&contents); err != nil { + t.Fatal(err) + } + if bytes.Contains(contents, []byte("confidential-skill-canary")) { + t.Fatal("plaintext bundle persisted") + } + content, err := f.service.ReadVersion(ctx, skills.ReadVersion{TenantID: tenant, SkillID: id, Version: 1}) + if err != nil || !bytes.Equal(content.Archive, bundle) || content.Version.Version != 1 { + t.Fatal("content round trip", err) + } + + // Another tenant's Skill is a missing one for every operation. + if _, err = f.store.Skill(ctx, foreign, id); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign metadata", err) + } + if _, err = f.service.ReadVersion(ctx, skills.ReadVersion{TenantID: foreign, SkillID: id, Version: 1}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign content", err) + } + if _, err = f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: foreign, SkillID: id, Archive: bundle, MakeDefault: true}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign version", err) + } + if _, err = f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: foreign, SkillID: id, Version: "1"}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign pointer", err) + } + if err = f.service.DeleteSkill(ctx, skills.DeleteSkill{TenantID: foreign, SkillID: id}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign delete", err) + } + if _, err = f.service.ListSkills(ctx, skills.ListSkills{TenantID: foreign, After: created.ID, Limit: 20}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign cursor", err) + } + if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: foreign, SkillID: id, Limit: 20}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign versions", err) + } + + // Concurrent uploads allocate consecutive version numbers. + const count = 8 + results := make(chan skills.Version, count) + failures := make(chan error, count) + var group sync.WaitGroup + for range count { + group.Go(func() { + version, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: bundle}) + if err != nil { + failures <- err + } else { + results <- version + } + }) + } + group.Wait() + close(results) + close(failures) + for err := range failures { + t.Fatal(err) + } + numbers := []int{} + for version := range results { + numbers = append(numbers, int(version.Version)) + } + sort.Ints(numbers) + for i, number := range numbers { + if number != i+2 { + t.Fatal("concurrent version allocation", numbers) + } + } + if len(numbers) != count { + t.Fatal("missing versions", numbers) + } + current, err := f.store.Skill(ctx, tenant, id) + if err != nil || current.DefaultVersion != 1 || current.LatestVersion != count+1 { + t.Fatal("concurrent pointers", current, err) + } + first, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, Limit: 3, Ascending: true}) + if err != nil || !first.HasMore || len(first.Versions) != 3 || first.Versions[0].Version != 1 { + t.Fatal("first page", first, err) + } + next, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, After: first.Versions[2].ID, Limit: 20, Ascending: true}) + if err != nil || next.HasMore || len(next.Versions) != 6 || next.Versions[0].Version != 4 { + t.Fatal("version resource cursor", next, err) + } + var cursorErr *skills.CursorError + 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) { + t.Fatal("missing default", err) + } + updated, err := f.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "3"}) + if err != nil || updated.DefaultVersion != 3 { + t.Fatal("default update", err) + } + if _, err = f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 3}); !errors.Is(err, skills.ErrDefaultVersion) { + t.Fatal("default deletion", err) + } + if _, err = f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: count + 1}); err != nil { + t.Fatal(err) + } + if current, err = f.store.Skill(ctx, tenant, id); err != nil || current.LatestVersion != count { + t.Fatal("latest pointer after deleting the latest version", current, err) + } + added, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: bundle, MakeDefault: true}) + if err != nil || added.Version != count+2 { + t.Fatal("deleted version number reused", added, err) + } + if err = f.service.DeleteSkill(ctx, skills.DeleteSkill{TenantID: tenant, SkillID: id}); err != nil { + t.Fatal(err) + } + if _, err = f.service.ReadVersion(ctx, skills.ReadVersion{TenantID: tenant, SkillID: id, Version: 1}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("cascaded content", err) + } + var remaining int + if err = f.pool.QueryRow(ctx, "SELECT count(*) FROM skill_versions WHERE tenant_id=$1", tenant).Scan(&remaining); err != nil || remaining != 0 { + t.Fatal("orphan content", remaining, err) + } +} + +// A zero page is empty; HasMore reports whether a resource follows the cursor. +func TestListsAcceptLimitZero(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 43)) + ctx := t.Context() + tenant, foreign := uuid.NewString(), uuid.NewString() + bundle := proofArchive(t, "limit-zero") + created := f.create(t, tenant, bundle) + id := key(t, created.ID) + if _, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: bundle}); err != nil { + t.Fatal(err) + } + for _, test := range []struct { + tenant, after string + hasMore bool + }{{tenant, "", true}, {tenant, created.ID, false}, {foreign, "", false}} { + page, err := f.service.ListSkills(ctx, skills.ListSkills{TenantID: test.tenant, After: test.after, Limit: 0, Ascending: true}) + if err != nil || len(page.Skills) != 0 || page.HasMore != test.hasMore { + t.Fatalf("zero Skill page after %q: %+v %v", test.after, page, err) + } + } + versions, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, Limit: 100, Ascending: true}) + if err != nil || len(versions.Versions) != 2 { + t.Fatal("versions", versions, err) + } + for after, hasMore := range map[string]bool{"": true, versions.Versions[0].ID: true, versions.Versions[1].ID: false} { + page, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, After: after, Limit: 0, Ascending: true}) + if err != nil || len(page.Versions) != 0 || page.HasMore != hasMore { + t.Fatalf("zero version page after %q: %+v %v", after, page, err) + } + } + // Foreign cursors and parents stay indistinguishable from missing ones. + if _, err = f.service.ListSkills(ctx, skills.ListSkills{TenantID: foreign, After: created.ID, Ascending: true}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign cursor", err) + } + if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: foreign, SkillID: id, Ascending: true}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign versions", err) + } + // A version cursor of another Skill in the tenant is a cursor error; one in + // another tenant is missing. + other := f.create(t, tenant, bundle) + otherPage, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: key(t, other.ID), Limit: 1}) + if err != nil || len(otherPage.Versions) != 1 { + t.Fatal("other Skill versions", otherPage, err) + } + var cursorErr *skills.CursorError + if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, After: otherPage.Versions[0].ID, Limit: 1}); !errors.As(err, &cursorErr) || cursorErr.Message != "Skill version cursor does not match this skill." { + t.Fatal("other Skill's cursor", err) + } + foreignSkill := f.create(t, foreign, bundle) + foreignPage, err := f.service.ListVersions(ctx, skills.ListVersions{TenantID: foreign, SkillID: key(t, foreignSkill.ID), Limit: 1}) + if err != nil || len(foreignPage.Versions) != 1 { + t.Fatal("foreign Skill versions", foreignPage, err) + } + if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, After: foreignPage.Versions[0].ID, Limit: 1}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign version cursor", err) + } +} + +func TestMetadataTracksDefaultVersion(t *testing.T) { + pool := pgtest.Open(t) + f := newFixture(t, pool, testCipher(t, 74)) + // Metadata reads and default changes need no content key. + metadataOnly := newFixture(t, pool, nil) + ctx := t.Context() + tenant, foreign := uuid.NewString(), uuid.NewString() + names := []string{"first-proof", "second-proof", "third-proof"} + descriptions := []string{"First immutable version.", "Second immutable version.", "Third immutable version."} + archives := make([][]byte, len(names)) + for i := range names { + archives[i] = archive(t, names[i], descriptions[i], "Private marker for "+names[i]+".") + } + created := f.create(t, tenant, archives[0]) + id := key(t, created.ID) + assertMetadata := func(value skills.Skill, version, latest int64) { + t.Helper() + if value.ID != created.ID || !value.CreatedAt.Equal(created.CreatedAt) || value.DefaultVersion != version || value.LatestVersion != latest || value.Name != names[version-1] || value.Description != descriptions[version-1] { + t.Fatalf("metadata does not track default %d/latest %d: %+v", version, latest, value) + } + } + assertStored := func(version, latest int64) { + t.Helper() + value, err := metadataOnly.store.Skill(ctx, tenant, id) + if err != nil { + t.Fatal("metadata read without content key", err) + } + assertMetadata(value, version, latest) + page, err := metadataOnly.service.ListSkills(ctx, skills.ListSkills{TenantID: tenant, Limit: 10, Ascending: true}) + if err != nil || len(page.Skills) != 1 { + t.Fatal("metadata list without content key", page, err) + } + assertMetadata(page.Skills[0], version, latest) + selected, err := f.service.ReadDefaultVersion(ctx, skills.ReadDefaultVersion{TenantID: tenant, SkillID: id}) + if err != nil || selected.Version.Version != version || selected.Version.Name != names[version-1] || selected.Version.Description != descriptions[version-1] || !bytes.Equal(selected.Archive, archives[version-1]) { + t.Fatal("default content differs from public metadata", selected.Version, err) + } + } + assertMetadata(created, 1, 1) + if _, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: archives[1]}); err != nil { + t.Fatal(err) + } + assertStored(1, 2) + for _, version := range []string{"2", "1"} { + updated, err := metadataOnly.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: version}) + if err != nil { + t.Fatal("default update without content key", err) + } + assertMetadata(updated, updated.DefaultVersion, 2) + assertStored(updated.DefaultVersion, 2) + } + if _, err := metadataOnly.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: foreign, SkillID: id, Version: "2"}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign default update", err) + } + if _, err := metadataOnly.service.SetDefaultVersion(ctx, skills.SetDefaultVersion{TenantID: tenant, SkillID: id, Version: "999"}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("missing default update", err) + } + assertStored(1, 2) + // Uploads and content reads need the key; the failed upload stores nothing. + if _, err := metadataOnly.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: archives[2]}); !errors.Is(err, credentialcrypto.ErrUnavailable) { + t.Fatal("upload without content key", err) + } + if _, err := metadataOnly.service.ReadDefaultVersion(ctx, skills.ReadDefaultVersion{TenantID: tenant, SkillID: id}); !errors.Is(err, credentialcrypto.ErrUnavailable) { + t.Fatal("content read without content key", err) + } + assertStored(1, 2) + if _, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: archives[2], MakeDefault: true}); err != nil { + t.Fatal(err) + } + assertStored(3, 3) + for i := range archives { + content, err := f.service.ReadVersion(ctx, skills.ReadVersion{TenantID: tenant, SkillID: id, Version: int64(i + 1)}) + if err != nil || content.Version.Name != names[i] || content.Version.Description != descriptions[i] || !bytes.Equal(content.Archive, archives[i]) { + t.Fatal("default changes modified immutable version", i+1, content.Version, err) + } + } +} + +// Deleting the sole version deletes the Skill and all its rows in one commit. +func TestSoleVersionDeletionRemovesSkill(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 63)) + ctx := t.Context() + tenant, foreign := uuid.NewString(), uuid.NewString() + skill := f.create(t, tenant, proofArchive(t, "sole-version")) + id := key(t, skill.ID) + version, err := f.store.Version(ctx, tenant, id, 1) + if err != nil { + t.Fatal(err) + } + // Foreign and missing targets fail before any mutation. + if _, err = f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: foreign, SkillID: id, Version: 1}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("foreign sole-version deletion", err) + } + if _, err = f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 2}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("missing version deletion", err) + } + if skillRows, versionRows := f.rowCounts(t, id); skillRows != 1 || versionRows != 1 { + t.Fatal("rejected deletion changed rows", skillRows, versionRows) + } + deleted, err := f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 1}) + if err != nil || deleted.ID != version.ID || deleted.SkillID != skill.ID || deleted.Version != 1 { + t.Fatal("sole-version deletion", deleted, err) + } + if skillRows, versionRows := f.rowCounts(t, id); skillRows != 0 || versionRows != 0 { + t.Fatal("orphaned Skill rows", skillRows, versionRows) + } + if _, err = f.store.Skill(ctx, tenant, id); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("deleted Skill is readable", err) + } + if _, err = f.service.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: id, Limit: 20}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("deleted Skill versions are listable", err) + } + if _, err = f.service.ReadDefaultVersion(ctx, skills.ReadDefaultVersion{TenantID: tenant, SkillID: id}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("deleted Skill content is readable", err) + } + if page, err := f.service.ListSkills(ctx, skills.ListSkills{TenantID: tenant, Limit: 20}); err != nil || len(page.Skills) != 0 { + t.Fatal("deleted Skill is listed", page, err) + } + if _, err = f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 1}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("repeated sole-version deletion", err) + } + if err = f.service.DeleteSkill(ctx, skills.DeleteSkill{TenantID: tenant, SkillID: id}); !errors.Is(err, skills.ErrNotFound) { + t.Fatal("Skill deletion after sole-version deletion", err) + } +} + +// Upload and sole-version deletion serialize on the Skill row: an upload that +// commits first makes the default undeletable, and a deletion that commits +// first makes the later upload miss the Skill. Neither loses acknowledged data. +func TestSoleVersionDeletionSerializesWithUpload(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 63)) + ctx := t.Context() + tenant := uuid.NewString() + for _, uploadFirst := range []bool{true, false} { + skill := f.create(t, tenant, proofArchive(t, "race-first")) + id := key(t, skill.ID) + // Hold the owner lock so both operations queue in a known order. + holder, err := f.pool.Begin(ctx) + if err != nil { + t.Fatal(err) + } + var holderPID int32 + if err = holder.QueryRow(ctx, "SELECT pg_backend_pid() FROM skills WHERE id=$1 FOR UPDATE", id).Scan(&holderPID); err != nil { + t.Fatal(err) + } + type outcome struct { + version skills.Version + err error + } + uploaded, deleted := make(chan outcome, 1), make(chan outcome, 1) + second := proofArchive(t, "race-second") + upload := func() { + version, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: id, Archive: second}) + uploaded <- outcome{version, err} + } + remove := func() { + version, err := f.service.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: id, Version: 1}) + deleted <- outcome{version, err} + } + early, late := upload, remove + if !uploadFirst { + early, late = remove, upload + } + go early() + waitForLockWaiters(t, f.pool, holderPID, 1) + go late() + waitForLockWaiters(t, f.pool, holderPID, 2) + if err = holder.Rollback(ctx); err != nil { + t.Fatal(err) + } + up, del := <-uploaded, <-deleted + skillRows, versionRows := f.rowCounts(t, id) + if uploadFirst { + if up.err != nil || up.version.Version != 2 || !errors.Is(del.err, skills.ErrDefaultVersion) || skillRows != 1 || versionRows != 2 { + t.Fatal("upload before deletion", up, del, skillRows, versionRows) + } + current, err := f.store.Skill(ctx, tenant, id) + if err != nil || current.DefaultVersion != 1 || current.LatestVersion != 2 { + t.Fatal("pointers after serialized upload", current, err) + } + } else if del.err != nil || del.version.Version != 1 || !errors.Is(up.err, skills.ErrNotFound) || skillRows != 0 || versionRows != 0 { + t.Fatal("deletion before upload", up, del, skillRows, versionRows) + } + } +} + +// waitForLockWaiters waits until count sessions queue behind holder. +func waitForLockWaiters(t *testing.T, pool *pgxpool.Pool, holder int32, count int) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + var waiting int + err := pool.QueryRow(t.Context(), `WITH RECURSIVE queued(pid) AS ( + SELECT $1::int UNION SELECT a.pid FROM pg_stat_activity a JOIN queued q ON q.pid = ANY(pg_blocking_pids(a.pid)) +) SELECT count(*) - 1 FROM queued`, holder).Scan(&waiting) + if err != nil { + t.Fatal(err) + } + if waiting >= count { + return + } + if time.Now().After(deadline) { + t.Fatal("lock waiters", waiting, count) + } + time.Sleep(10 * time.Millisecond) + } +} + +// Text PostgreSQL cannot store, here a YAML-escaped U+0000 in the manifest +// description, is the shared unstorable-text error and stores nothing. +func TestUnstorableTextStoresNothing(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 45)) + ctx := t.Context() + tenant := uuid.NewString() + bad := archive(t, "proof", `"before\0after"`, "unstorable") + if _, err := f.service.CreateSkill(ctx, skills.CreateSkill{TenantID: tenant, Archive: bad}); !errors.Is(err, textvalue.ErrUnstorable) { + t.Fatal("create", err) + } + created := f.create(t, tenant, proofArchive(t, "storable")) + if _, err := f.service.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: key(t, created.ID), Archive: bad, MakeDefault: true}); !errors.Is(err, textvalue.ErrUnstorable) { + t.Fatal("version", err) + } + var skillRows, versionRows int + if err := f.pool.QueryRow(ctx, "SELECT (SELECT count(*) FROM skills WHERE tenant_id=$1), (SELECT count(*) FROM skill_versions WHERE tenant_id=$1)", tenant).Scan(&skillRows, &versionRows); err != nil || skillRows != 1 || versionRows != 1 { + t.Fatal("rejected text left rows", skillRows, versionRows, err) + } + if current, err := f.store.Skill(ctx, tenant, key(t, created.ID)); err != nil || current.LatestVersion != 1 || current.DefaultVersion != 1 { + t.Fatal("rejected version moved the pointers", current, err) + } +} + +// A malformed tenant is invalid input, not a database error. +func TestMalformedTenantIsInvalidInput(t *testing.T) { + f := newFixture(t, pgtest.Open(t), testCipher(t, 46)) + if _, err := f.service.CreateSkill(t.Context(), skills.CreateSkill{TenantID: "not-a-tenant", Archive: proofArchive(t, "tenant")}); !errors.Is(err, skills.ErrInvalidInput) { + t.Fatal("create", err) + } + if _, err := f.store.Skill(t.Context(), "not-a-tenant", uuid.New()); !errors.Is(err, skills.ErrInvalidInput) { + t.Fatal("read", err) + } +} diff --git a/services/core/internal/sandbox/providers/configuration_flow_test.go b/services/core/internal/sandbox/providers/configuration_flow_test.go index 97a05cf41..deddc4598 100644 --- a/services/core/internal/sandbox/providers/configuration_flow_test.go +++ b/services/core/internal/sandbox/providers/configuration_flow_test.go @@ -103,11 +103,12 @@ func TestAdditionalConfigurationProviderUsesCommonAPIAndStore(t *testing.T) { // The flow reaches only the store areas and the deployment setup; every // other dependency panics if called. h, err := api.NewHandler(api.Dependencies{ - Engine: "codex", CoreKeys: auth, InstallationBindings: s, Projects: s, Skills: s, + Engine: "codex", CoreKeys: auth, InstallationBindings: s, Projects: s, ModelProviders: struct{ api.ModelProviders }{}, ModelProvidersReader: struct{ api.ModelProvidersReader }{}, Vaults: struct{ api.Vaults }{}, VaultsReader: struct{ api.VaultsReader }{}, Files: struct{ api.Files }{}, FilesReader: struct{ api.FilesReader }{}, EnvironmentTemplates: struct{ api.EnvironmentTemplates }{}, EnvironmentTemplatesReader: struct{ api.EnvironmentTemplatesReader }{}, + Skills: struct{ api.Skills }{}, SkillsReader: struct{ api.SkillsReader }{}, Agents: struct{ api.Agents }{}, AgentsReader: struct{ api.AgentsReader }{}, Sessions: s, SessionEvents: s, SessionHistory: s, Subagents: s, Artifacts: s, SessionAdmin: s, Environments: s, Admin: s, AdminAudit: struct{ api.AdminAudit }{}, WriteAudit: struct{ api.WriteAudit }{}, diff --git a/services/core/internal/skills/content.go b/services/core/internal/skills/content.go new file mode 100644 index 000000000..0010bea40 --- /dev/null +++ b/services/core/internal/skills/content.go @@ -0,0 +1,13 @@ +package skills + +import "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" + +// VerifyContent checks that a decrypted archive is still the complete bundle +// its version records, before anything reads or installs it. +func VerifyContent(content Content) error { + metadata := agentskill.Metadata{Type: "inline", Name: content.Version.Name, Description: content.Version.Description} + if _, err := agentskill.Read(content.Archive, metadata); err != nil { + return ErrInvalidInput + } + return nil +} diff --git a/services/core/internal/skills/deletion.go b/services/core/internal/skills/deletion.go new file mode 100644 index 000000000..c30083152 --- /dev/null +++ b/services/core/internal/skills/deletion.go @@ -0,0 +1,32 @@ +package skills + +// VersionDeletionFacts is what a version deletion decides from, loaded under +// the Skill's lock. +type VersionDeletionFacts struct { + Skill Skill + Target Version + // OthersRemain reports whether the Skill has a version besides Target. + OthersRemain bool +} + +// VersionDeletion is a decided version deletion. DeleteSkill deletes the +// whole Skill, whose only version Target is; otherwise only Target is deleted +// and RefreshLatest moves the latest pointer to the highest remaining version. +type VersionDeletion struct { + Target Version + DeleteSkill bool + RefreshLatest bool +} + +// DecideVersionDeletion applies the deletion rules. The default version can +// be deleted only as the Skill's sole version, and that deletes the Skill +// itself; Session installations frozen from it are independent copies. +func DecideVersionDeletion(facts VersionDeletionFacts) (VersionDeletion, error) { + if facts.Target.Version == facts.Skill.DefaultVersion { + if facts.OthersRemain { + return VersionDeletion{}, ErrDefaultVersion + } + return VersionDeletion{Target: facts.Target, DeleteSkill: true}, nil + } + return VersionDeletion{Target: facts.Target, RefreshLatest: facts.Target.Version == facts.Skill.LatestVersion}, nil +} diff --git a/services/core/internal/skills/doc.go b/services/core/internal/skills/doc.go index 402a329d3..eb3975b85 100644 --- a/services/core/internal/skills/doc.go +++ b/services/core/internal/skills/doc.go @@ -1,2 +1,5 @@ -// Package skills owns the Skill resource vocabulary and its rules. +// Package skills owns Skills: tenant-owned bundles with immutable, numbered +// versions and a default version. It holds their vocabulary, the identifier, +// version and selector rules, the version deletion rules, and the use cases +// behind the Skills API. persistence/postgres/skillpg implements its storage. package skills diff --git a/services/core/internal/skills/errors.go b/services/core/internal/skills/errors.go new file mode 100644 index 000000000..5d63bfcf0 --- /dev/null +++ b/services/core/internal/skills/errors.go @@ -0,0 +1,39 @@ +package skills + +import ( + "errors" + "fmt" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/echotext" +) + +var ( + // ErrNotFound reports a Skill or version that does not exist in the tenant. + // Malformed identifiers and other tenants' resources are indistinguishable + // from missing ones. + ErrNotFound = errors.New("skill 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 + // versions remain. + ErrDefaultVersion = errors.New("cannot delete the default skill version") +) + +// CursorError reports a version list cursor that does not name a version of +// the listed Skill. Message is the official message for the case. +type CursorError struct{ Message string } + +func (e *CursorError) Error() string { return e.Message } + +// errCursorParent is a version of another Skill in the same tenant. +var errCursorParent = &CursorError{Message: "Skill version cursor does not match this skill."} + +// cursorPrefixError reports a cursor that is not a version ID at all. The +// official message echoes the value; a long or unprintable value is left out +// so the error stays bounded. +func cursorPrefixError(value string) error { + if !echotext.Allowed(value) { + return &CursorError{Message: "Invalid 'after'. Expected an ID that begins with 'skillver'."} + } + return &CursorError{Message: fmt.Sprintf("Invalid 'after': '%s'. Expected an ID that begins with 'skillver'.", value)} +} diff --git a/services/core/internal/skills/rules_test.go b/services/core/internal/skills/rules_test.go new file mode 100644 index 000000000..103877289 --- /dev/null +++ b/services/core/internal/skills/rules_test.go @@ -0,0 +1,153 @@ +package skills + +import ( + "archive/zip" + "bytes" + "errors" + "strings" + "testing" + + "github.com/google/uuid" +) + +func TestParseID(t *testing.T) { + id := uuid.New() + for _, test := range []struct { + name, value string + parse func(string) (uuid.UUID, error) + want uuid.UUID + }{ + {"skill", FormatID(id), ParseID, id}, + {"version", FormatVersionID(id), ParseVersionID, id}, + {"bare UUID", id.String(), ParseID, uuid.Nil}, + {"version prefix for a Skill", FormatVersionID(id), ParseID, uuid.Nil}, + {"Skill prefix for a version", FormatID(id), ParseVersionID, uuid.Nil}, + {"upper case", "skill_" + strings.ToUpper(id.String()), ParseID, uuid.Nil}, + {"braced", "skill_{" + id.String() + "}", ParseID, uuid.Nil}, + {"nil UUID", FormatID(uuid.Nil), ParseID, uuid.Nil}, + {"empty", "", ParseID, uuid.Nil}, + } { + t.Run(test.name, func(t *testing.T) { + got, err := test.parse(test.value) + if test.want == uuid.Nil { + if !errors.Is(err, ErrNotFound) || got != uuid.Nil { + t.Fatalf("got %s, %v", got, err) + } + return + } + if err != nil || got != test.want { + t.Fatalf("got %s, %v", got, err) + } + }) + } +} + +// Malformed path segments resolve to values Core never assigns, so they take +// the missing-resource path. +func TestPathValues(t *testing.T) { + id := uuid.New() + for value, want := range map[string]uuid.UUID{FormatID(id): id, "skill_missing": uuid.Max, id.String(): uuid.Max, "": uuid.Max} { + if got := PathID(value); got != want { + t.Errorf("PathID(%q) = %s", value, got) + } + } + for value, want := range map[string]int64{"1": 1, "42": 42, "0": 0, "01": 0, "-1": 0, "latest": 0, "": 0, "9223372036854775808": 0} { + if got := PathVersion(value); got != want { + t.Errorf("PathVersion(%q) = %d", value, got) + } + } +} + +func TestSelectVersion(t *testing.T) { + for _, test := range []struct { + selector string + want int64 + valid bool + }{ + {"", 2, true}, + {"latest", 5, true}, + {"3", 3, true}, + {"0", 0, false}, + {"03", 0, false}, + {"Latest", 0, false}, + {"default", 0, false}, + } { + got, err := SelectVersion(test.selector, 2, 5) + if test.valid != (err == nil) || got != test.want { + t.Errorf("SelectVersion(%q) = %d, %v", test.selector, got, err) + } + if test.valid != (ValidateSelector(test.selector) == nil) { + t.Errorf("ValidateSelector(%q) disagrees", test.selector) + } + } +} + +func TestDecideVersionDeletion(t *testing.T) { + skill := Skill{DefaultVersion: 2, LatestVersion: 4} + for _, test := range []struct { + name string + target int64 + others bool + want VersionDeletion + err error + }{ + {"default with others", 2, true, VersionDeletion{}, ErrDefaultVersion}, + {"sole default", 2, false, VersionDeletion{Target: Version{Version: 2}, DeleteSkill: true}, nil}, + {"latest", 4, true, VersionDeletion{Target: Version{Version: 4}, RefreshLatest: true}, nil}, + {"middle", 3, true, VersionDeletion{Target: Version{Version: 3}}, nil}, + } { + t.Run(test.name, func(t *testing.T) { + got, err := DecideVersionDeletion(VersionDeletionFacts{Skill: skill, Target: Version{Version: test.target}, OthersRemain: test.others}) + if !errors.Is(err, test.err) || got != test.want { + t.Fatalf("got %+v, %v", got, err) + } + }) + } +} + +func TestCursorPrefixError(t *testing.T) { + var cursor *CursorError + if err := cursorPrefixError("skill_x"); !errors.As(err, &cursor) || cursor.Message != "Invalid 'after': 'skill_x'. Expected an ID that begins with 'skillver'." { + t.Fatalf("echoed: %v", err) + } + if err := cursorPrefixError(strings.Repeat("x", 1000)); !errors.As(err, &cursor) || cursor.Message != "Invalid 'after'. Expected an ID that begins with 'skillver'." { + t.Fatalf("long value: %v", err) + } + if err := cursorPrefixError("bad\x00"); !errors.As(err, &cursor) || cursor.Message != "Invalid 'after'. Expected an ID that begins with 'skillver'." { + t.Fatalf("unprintable value: %v", err) + } +} + +func TestVerifyContent(t *testing.T) { + archive := testArchive(t, "proof", "Verify a Skill.") + version := Version{Name: "proof", Description: "Verify a Skill."} + if err := VerifyContent(Content{Version: version, Archive: archive}); err != nil { + t.Fatal(err) + } + for name, content := range map[string]Content{ + "other name": {Version: Version{Name: "other", Description: version.Description}, Archive: archive}, + "other description": {Version: Version{Name: version.Name, Description: "Other."}, Archive: archive}, + "corrupt archive": {Version: version, Archive: archive[:len(archive)/2]}, + } { + if err := VerifyContent(content); !errors.Is(err, ErrInvalidInput) { + t.Errorf("%s: %v", name, err) + } + } +} + +func testArchive(t *testing.T, name, description string) []byte { + t.Helper() + var buffer bytes.Buffer + writer := zip.NewWriter(&buffer) + file, err := writer.CreateHeader(&zip.FileHeader{Name: name + "/SKILL.md", Method: zip.Store}) + if err != nil { + t.Fatal(err) + } + if _, err = file.Write([]byte("---\nname: " + name + "\ndescription: " + description + "\n---\nProof.")); err != nil { + t.Fatal(err) + } + if err = writer.Close(); err != nil { + t.Fatal(err) + } + return buffer.Bytes() +} diff --git a/services/core/internal/skills/service.go b/services/core/internal/skills/service.go new file mode 100644 index 000000000..d1c089577 --- /dev/null +++ b/services/core/internal/skills/service.go @@ -0,0 +1,223 @@ +package skills + +import ( + "context" + "errors" + "strings" + + "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" + "github.com/google/uuid" +) + +// MaxPageLimit bounds every Skill and version page. A limit of 0 returns an +// empty page whose HasMore reports whether anything follows the cursor. +const MaxPageLimit = 100 + +// Service runs the Skill use cases. +type Service struct { + storage Storage + reader Reader +} + +func NewService(storage Storage, reader Reader) (*Service, error) { + if storage == nil || reader == nil { + return nil, errors.New("skills: storage and reader are required") + } + return &Service{storage: storage, reader: reader}, nil +} + +// CreateSkill uploads a new Skill. +type CreateSkill struct { + TenantID string + Archive []byte +} + +// CreateSkill validates the archive and stores it as the new Skill's +// version 1. +func (s *Service) CreateSkill(ctx context.Context, c CreateSkill) (Skill, error) { + metadata, err := agentskill.Inspect(c.Archive) + if err != nil { + return Skill{}, ErrInvalidInput + } + return s.storage.CreateSkill(ctx, NewSkill{TenantID: c.TenantID, Name: metadata.Name, Description: metadata.Description, Archive: c.Archive}) +} + +// CreateVersion uploads a new version of a Skill. +type CreateVersion struct { + TenantID string + SkillID uuid.UUID + Archive []byte + MakeDefault bool +} + +// CreateVersion validates the archive before looking up the Skill, then +// stores it under the Skill's next version number. +func (s *Service) CreateVersion(ctx context.Context, c CreateVersion) (Version, error) { + metadata, err := agentskill.Inspect(c.Archive) + if err != nil { + return Version{}, ErrInvalidInput + } + return s.storage.CreateVersion(ctx, NewVersion{TenantID: c.TenantID, SkillID: c.SkillID, Name: metadata.Name, Description: metadata.Description, Archive: c.Archive, MakeDefault: c.MakeDefault}) +} + +// SetDefaultVersion points a Skill's default at one of its versions. Version +// is the request's selector and must be a concrete version number. +type SetDefaultVersion struct { + TenantID string + SkillID uuid.UUID + Version string +} + +func (s *Service) SetDefaultVersion(ctx context.Context, c SetDefaultVersion) (Skill, error) { + number, err := ParseVersion(c.Version) + if err != nil { + return Skill{}, ErrInvalidInput + } + return s.storage.SetDefaultVersion(ctx, c.TenantID, c.SkillID, number) +} + +// DeleteSkill deletes a Skill and its versions. Session installations frozen +// from them are independent copies and remain. +type DeleteSkill struct { + TenantID string + SkillID uuid.UUID +} + +func (s *Service) DeleteSkill(ctx context.Context, c DeleteSkill) error { + return s.storage.DeleteSkill(ctx, c.TenantID, c.SkillID) +} + +// DeleteVersion deletes one version of a Skill. +type DeleteVersion struct { + TenantID string + SkillID uuid.UUID + Version int64 +} + +// DeleteVersion decides under the Skill's lock, so it serializes with uploads +// and pointer changes, and returns the deleted version. +func (s *Service) DeleteVersion(ctx context.Context, c DeleteVersion) (Version, error) { + var deleted Version + err := s.storage.WithVersionDeletion(ctx, c.TenantID, c.SkillID, func(tx VersionDeletionTx) error { + facts, err := tx.LoadVersionDeletion(c.Version) + if err != nil { + return err + } + decision, err := DecideVersionDeletion(facts) + if err != nil { + return err + } + if err := tx.ApplyVersionDeletion(decision); err != nil { + return err + } + deleted = decision.Target + return nil + }) + if err != nil { + return Version{}, err + } + return deleted, nil +} + +// ListSkills lists the tenant's Skills. After is a Skill ID; a malformed or +// missing one is ErrNotFound. +type ListSkills struct { + TenantID string + After string + Limit int + Ascending bool +} + +func (s *Service) ListSkills(ctx context.Context, c ListSkills) (Page, error) { + if c.Limit < 0 || c.Limit > MaxPageLimit { + return Page{}, ErrInvalidInput + } + query := SkillPageQuery{Limit: c.Limit, Ascending: c.Ascending} + if c.After != "" { + id := PathID(c.After) + cursor, err := s.reader.Skill(ctx, c.TenantID, id) + if err != nil { + return Page{}, err + } + query.After = &SkillCursor{CreatedAt: cursor.CreatedAt, ID: id} + } + return s.reader.Skills(ctx, c.TenantID, query) +} + +// ListVersions lists a Skill's versions. After is a version ID, not a +// version number. +type ListVersions struct { + TenantID string + SkillID uuid.UUID + After string + Limit int + Ascending bool +} + +// ListVersions resolves the Skill before the cursor. A cursor that is not a +// version ID is a CursorError; a missing version, including another +// tenant's, is ErrNotFound; another Skill's version in the tenant is a +// CursorError. The Skill is looked up again before that last error, so a +// Skill deleted in between stays not found. +func (s *Service) ListVersions(ctx context.Context, c ListVersions) (VersionPage, error) { + if c.Limit < 0 || c.Limit > MaxPageLimit { + return VersionPage{}, ErrInvalidInput + } + if _, err := s.reader.Skill(ctx, c.TenantID, c.SkillID); err != nil { + return VersionPage{}, err + } + query := VersionPageQuery{Limit: c.Limit, Ascending: c.Ascending} + if c.After != "" { + if !strings.HasPrefix(c.After, "skillver") { + return VersionPage{}, cursorPrefixError(c.After) + } + id, err := ParseVersionID(c.After) + if err != nil { + return VersionPage{}, err + } + cursor, err := s.reader.VersionByID(ctx, c.TenantID, id) + if err != nil { + return VersionPage{}, err + } + if cursor.SkillID != FormatID(c.SkillID) { + if _, err := s.reader.Skill(ctx, c.TenantID, c.SkillID); err != nil { + return VersionPage{}, err + } + return VersionPage{}, errCursorParent + } + query.AfterVersion = cursor.Version + } + return s.reader.Versions(ctx, c.TenantID, c.SkillID, query) +} + +// ReadVersion reads one version's archive. +type ReadVersion struct { + TenantID string + SkillID uuid.UUID + Version int64 +} + +func (s *Service) ReadVersion(ctx context.Context, c ReadVersion) (Content, error) { + return verified(s.reader.VersionContent(ctx, c.TenantID, c.SkillID, c.Version)) +} + +// ReadDefaultVersion reads the archive of a Skill's default version, selecting +// the pointer and the content in one read. +type ReadDefaultVersion struct { + TenantID string + SkillID uuid.UUID +} + +func (s *Service) ReadDefaultVersion(ctx context.Context, c ReadDefaultVersion) (Content, error) { + return verified(s.reader.DefaultVersionContent(ctx, c.TenantID, c.SkillID)) +} + +func verified(content Content, err error) (Content, error) { + if err == nil { + err = VerifyContent(content) + } + if err != nil { + return Content{}, err + } + return content, nil +} diff --git a/services/core/internal/skills/service_test.go b/services/core/internal/skills/service_test.go new file mode 100644 index 000000000..def5bbba9 --- /dev/null +++ b/services/core/internal/skills/service_test.go @@ -0,0 +1,394 @@ +package skills + +import ( + "context" + "errors" + "reflect" + "testing" + "time" + + "github.com/google/uuid" +) + +type fakeStorage struct { + t *testing.T + createSkill func(context.Context, NewSkill) (Skill, error) + createVersion func(context.Context, NewVersion) (Version, error) + setDefaultVersion func(context.Context, string, uuid.UUID, int64) (Skill, error) + deleteSkill func(context.Context, string, uuid.UUID) error + withVersionDeletion func(context.Context, string, uuid.UUID, func(VersionDeletionTx) error) error +} + +func unexpectedCall(t *testing.T, method string) { + t.Helper() + t.Fatalf("unexpected call to %s", method) +} + +func (f *fakeStorage) CreateSkill(ctx context.Context, skill NewSkill) (Skill, error) { + if f.createSkill == nil { + unexpectedCall(f.t, "CreateSkill") + } + return f.createSkill(ctx, skill) +} + +func (f *fakeStorage) CreateVersion(ctx context.Context, version NewVersion) (Version, error) { + if f.createVersion == nil { + unexpectedCall(f.t, "CreateVersion") + } + return f.createVersion(ctx, version) +} + +func (f *fakeStorage) SetDefaultVersion(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Skill, error) { + if f.setDefaultVersion == nil { + unexpectedCall(f.t, "SetDefaultVersion") + } + return f.setDefaultVersion(ctx, tenantID, skillID, version) +} + +func (f *fakeStorage) DeleteSkill(ctx context.Context, tenantID string, skillID uuid.UUID) error { + if f.deleteSkill == nil { + unexpectedCall(f.t, "DeleteSkill") + } + return f.deleteSkill(ctx, tenantID, skillID) +} + +func (f *fakeStorage) WithVersionDeletion(ctx context.Context, tenantID string, skillID uuid.UUID, apply func(VersionDeletionTx) error) error { + if f.withVersionDeletion == nil { + unexpectedCall(f.t, "WithVersionDeletion") + } + return f.withVersionDeletion(ctx, tenantID, skillID, apply) +} + +type fakeVersionDeletionTx struct { + t *testing.T + load func(int64) (VersionDeletionFacts, error) + apply func(VersionDeletion) error +} + +func (f *fakeVersionDeletionTx) LoadVersionDeletion(version int64) (VersionDeletionFacts, error) { + if f.load == nil { + unexpectedCall(f.t, "LoadVersionDeletion") + } + return f.load(version) +} + +func (f *fakeVersionDeletionTx) ApplyVersionDeletion(decision VersionDeletion) error { + if f.apply == nil { + unexpectedCall(f.t, "ApplyVersionDeletion") + } + return f.apply(decision) +} + +type fakeReader struct { + t *testing.T + skill func(context.Context, string, uuid.UUID) (Skill, error) + skills func(context.Context, string, SkillPageQuery) (Page, error) + version func(context.Context, string, uuid.UUID, int64) (Version, error) + versionByID func(context.Context, string, uuid.UUID) (Version, error) + versions func(context.Context, string, uuid.UUID, VersionPageQuery) (VersionPage, error) + versionContent func(context.Context, string, uuid.UUID, int64) (Content, error) + defaultVersionContent func(context.Context, string, uuid.UUID) (Content, error) +} + +func (f *fakeReader) Skill(ctx context.Context, tenantID string, id uuid.UUID) (Skill, error) { + if f.skill == nil { + unexpectedCall(f.t, "Skill") + } + return f.skill(ctx, tenantID, id) +} + +func (f *fakeReader) Skills(ctx context.Context, tenantID string, page SkillPageQuery) (Page, error) { + if f.skills == nil { + unexpectedCall(f.t, "Skills") + } + return f.skills(ctx, tenantID, page) +} + +func (f *fakeReader) Version(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Version, error) { + if f.version == nil { + unexpectedCall(f.t, "Version") + } + return f.version(ctx, tenantID, skillID, version) +} + +func (f *fakeReader) VersionByID(ctx context.Context, tenantID string, id uuid.UUID) (Version, error) { + if f.versionByID == nil { + unexpectedCall(f.t, "VersionByID") + } + return f.versionByID(ctx, tenantID, id) +} + +func (f *fakeReader) Versions(ctx context.Context, tenantID string, skillID uuid.UUID, page VersionPageQuery) (VersionPage, error) { + if f.versions == nil { + unexpectedCall(f.t, "Versions") + } + return f.versions(ctx, tenantID, skillID, page) +} + +func (f *fakeReader) VersionContent(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Content, error) { + if f.versionContent == nil { + unexpectedCall(f.t, "VersionContent") + } + return f.versionContent(ctx, tenantID, skillID, version) +} + +func (f *fakeReader) DefaultVersionContent(ctx context.Context, tenantID string, skillID uuid.UUID) (Content, error) { + if f.defaultVersionContent == nil { + unexpectedCall(f.t, "DefaultVersionContent") + } + return f.defaultVersionContent(ctx, tenantID, skillID) +} + +func newTestService(t *testing.T) (*Service, *fakeStorage, *fakeReader) { + t.Helper() + storage, reader := &fakeStorage{t: t}, &fakeReader{t: t} + service, err := NewService(storage, reader) + if err != nil { + t.Fatal(err) + } + return service, storage, reader +} + +func TestNewServiceRequiresDependencies(t *testing.T) { + if _, err := NewService(nil, &fakeReader{t: t}); err == nil { + t.Fatal("nil storage accepted") + } + if _, err := NewService(&fakeStorage{t: t}, nil); err == nil { + t.Fatal("nil reader accepted") + } +} + +// Uploads take their name and description from the archive, and an invalid +// archive is rejected before storage sees it, even for a missing Skill. +func TestUploadsValidateTheArchiveFirst(t *testing.T) { + service, storage, _ := newTestService(t) + archive, skill := testArchive(t, "proof", "Verify a Skill."), uuid.New() + for name, upload := range map[string]func() error{ + "skill": func() error { + _, err := service.CreateSkill(t.Context(), CreateSkill{TenantID: "tenant", Archive: []byte("not a zip")}) + return err + }, + "version": func() error { + _, err := service.CreateVersion(t.Context(), CreateVersion{TenantID: "tenant", SkillID: skill, Archive: nil}) + return err + }, + } { + if err := upload(); !errors.Is(err, ErrInvalidInput) { + t.Fatalf("%s: %v", name, err) + } + } + storage.createSkill = func(_ context.Context, got NewSkill) (Skill, error) { + if !reflect.DeepEqual(got, NewSkill{TenantID: "tenant", Name: "proof", Description: "Verify a Skill.", Archive: archive}) { + t.Fatalf("new skill = %+v", got) + } + return Skill{ID: "skill_created"}, nil + } + if created, err := service.CreateSkill(t.Context(), CreateSkill{TenantID: "tenant", Archive: archive}); err != nil || created.ID != "skill_created" { + t.Fatalf("create = %+v, %v", created, err) + } + storage.createVersion = func(_ context.Context, got NewVersion) (Version, error) { + if !reflect.DeepEqual(got, NewVersion{TenantID: "tenant", SkillID: skill, Name: "proof", Description: "Verify a Skill.", Archive: archive, MakeDefault: true}) { + t.Fatalf("new version = %+v", got) + } + return Version{}, ErrNotFound + } + if _, err := service.CreateVersion(t.Context(), CreateVersion{TenantID: "tenant", SkillID: skill, Archive: archive, MakeDefault: true}); !errors.Is(err, ErrNotFound) { + t.Fatalf("version = %v", err) + } +} + +// The default version must be a concrete version number; selectors are +// rejected before storage. +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) { + t.Fatalf("%q: %v", value, err) + } + } + storage.setDefaultVersion = func(_ context.Context, tenant string, id uuid.UUID, version int64) (Skill, error) { + if tenant != "tenant" || id != skill || version != 3 { + t.Fatalf("set default = %s %s %d", tenant, id, version) + } + return Skill{DefaultVersion: 3}, nil + } + if updated, err := service.SetDefaultVersion(t.Context(), SetDefaultVersion{TenantID: "tenant", SkillID: skill, Version: "3"}); err != nil || updated.DefaultVersion != 3 { + t.Fatalf("set default = %+v, %v", updated, err) + } +} + +func TestDeleteVersionDecidesUnderTheLock(t *testing.T) { + skill := uuid.New() + for _, test := range []struct { + name string + facts VersionDeletionFacts + loadErr error + applied *VersionDeletion + err error + }{ + {"missing version", VersionDeletionFacts{}, ErrNotFound, nil, ErrNotFound}, + {"default with others", VersionDeletionFacts{Skill: Skill{DefaultVersion: 2, LatestVersion: 3}, Target: Version{Version: 2}, OthersRemain: true}, nil, nil, ErrDefaultVersion}, + {"latest", VersionDeletionFacts{Skill: Skill{DefaultVersion: 1, LatestVersion: 2}, Target: Version{ID: "skillver_x", Version: 2}, OthersRemain: true}, nil, &VersionDeletion{Target: Version{ID: "skillver_x", Version: 2}, RefreshLatest: true}, nil}, + } { + t.Run(test.name, func(t *testing.T) { + service, storage, _ := newTestService(t) + var applied *VersionDeletion + storage.withVersionDeletion = func(_ context.Context, tenant string, id uuid.UUID, apply func(VersionDeletionTx) error) error { + if tenant != "tenant" || id != skill { + t.Fatalf("lock = %s %s", tenant, id) + } + tx := &fakeVersionDeletionTx{t: t, load: func(version int64) (VersionDeletionFacts, error) { + if version != 2 { + t.Fatalf("load version %d", version) + } + return test.facts, test.loadErr + }} + if test.applied != nil { + tx.apply = func(decision VersionDeletion) error { applied = &decision; return nil } + } + return apply(tx) + } + deleted, err := service.DeleteVersion(t.Context(), DeleteVersion{TenantID: "tenant", SkillID: skill, Version: 2}) + if !errors.Is(err, test.err) || !reflect.DeepEqual(applied, test.applied) { + t.Fatalf("deleted = %+v, %v; applied %+v", deleted, err, applied) + } + if test.applied != nil && deleted != test.applied.Target { + t.Fatalf("deleted = %+v", deleted) + } + }) + } +} + +func TestListSkills(t *testing.T) { + service, _, reader := newTestService(t) + for _, limit := range []int{-1, MaxPageLimit + 1} { + if _, err := service.ListSkills(t.Context(), ListSkills{TenantID: "tenant", Limit: limit}); !errors.Is(err, ErrInvalidInput) { + t.Fatalf("limit %d: %v", limit, err) + } + } + // A malformed cursor resolves like a missing Skill. + reader.skill = func(_ context.Context, _ string, id uuid.UUID) (Skill, error) { + if id != uuid.Max { + t.Fatalf("cursor lookup %s", id) + } + return Skill{}, ErrNotFound + } + if _, err := service.ListSkills(t.Context(), ListSkills{TenantID: "tenant", After: "skill_bad", Limit: 1}); !errors.Is(err, ErrNotFound) { + t.Fatalf("malformed cursor: %v", err) + } + cursor, created := uuid.New(), time.Unix(100, 0) + reader.skill = func(context.Context, string, uuid.UUID) (Skill, error) { return Skill{CreatedAt: created}, nil } + reader.skills = func(_ context.Context, tenant string, page SkillPageQuery) (Page, error) { + want := SkillPageQuery{After: &SkillCursor{CreatedAt: created, ID: cursor}, Limit: 0, Ascending: true} + if tenant != "tenant" || !reflect.DeepEqual(page, want) { + t.Fatalf("page = %+v", page) + } + return Page{HasMore: true}, nil + } + if page, err := service.ListSkills(t.Context(), ListSkills{TenantID: "tenant", After: FormatID(cursor), Limit: 0, Ascending: true}); err != nil || !page.HasMore { + t.Fatalf("page = %+v, %v", page, err) + } +} + +func TestListVersionsResolvesTheCursor(t *testing.T) { + skill, other, cursor := uuid.New(), uuid.New(), uuid.New() + for _, test := range []struct { + name string + after string + skillErrs []error + cursorSkill uuid.UUID + cursorErr error + err error + message string + }{ + {name: "missing skill", after: "anything", skillErrs: []error{ErrNotFound}, err: ErrNotFound}, + {name: "not a version ID", after: "skill_x", skillErrs: []error{nil}, message: "Invalid 'after': 'skill_x'. Expected an ID that begins with 'skillver'."}, + {name: "malformed version ID", after: "skillver_x", skillErrs: []error{nil}, err: ErrNotFound}, + {name: "missing version", after: FormatVersionID(cursor), skillErrs: []error{nil}, cursorErr: ErrNotFound, err: ErrNotFound}, + {name: "other skill", after: FormatVersionID(cursor), skillErrs: []error{nil, nil}, cursorSkill: other, message: "Skill version cursor does not match this skill."}, + {name: "skill deleted meanwhile", after: FormatVersionID(cursor), skillErrs: []error{nil, ErrNotFound}, cursorSkill: other, err: ErrNotFound}, + } { + t.Run(test.name, func(t *testing.T) { + service, _, reader := newTestService(t) + reader.skill = func(_ context.Context, _ string, id uuid.UUID) (Skill, error) { + if id != skill || len(test.skillErrs) == 0 { + t.Fatalf("unexpected Skill lookup %s", id) + } + err := test.skillErrs[0] + test.skillErrs = test.skillErrs[1:] + return Skill{}, err + } + if test.cursorSkill != uuid.Nil || test.cursorErr != nil { + reader.versionByID = func(_ context.Context, tenant string, id uuid.UUID) (Version, error) { + if tenant != "tenant" || id != cursor { + t.Fatalf("cursor lookup %s %s", tenant, id) + } + return Version{SkillID: FormatID(test.cursorSkill)}, test.cursorErr + } + } + _, err := service.ListVersions(t.Context(), ListVersions{TenantID: "tenant", SkillID: skill, After: test.after, Limit: 5}) + var cursorErr *CursorError + if test.message != "" { + if !errors.As(err, &cursorErr) || cursorErr.Message != test.message { + t.Fatalf("err = %v", err) + } + } else if !errors.Is(err, test.err) { + t.Fatalf("err = %v", err) + } + if len(test.skillErrs) != 0 { + t.Fatalf("Skill lookups left: %v", test.skillErrs) + } + }) + } +} + +func TestListVersionsPagesAfterTheCursorVersion(t *testing.T) { + service, _, reader := newTestService(t) + for _, limit := range []int{-1, MaxPageLimit + 1} { + if _, err := service.ListVersions(t.Context(), ListVersions{TenantID: "tenant", SkillID: uuid.New(), Limit: limit}); !errors.Is(err, ErrInvalidInput) { + t.Fatalf("limit %d: %v", limit, err) + } + } + skill, cursor := uuid.New(), uuid.New() + reader.skill = func(context.Context, string, uuid.UUID) (Skill, error) { return Skill{}, nil } + reader.versionByID = func(context.Context, string, uuid.UUID) (Version, error) { + return Version{SkillID: FormatID(skill), Version: 4}, nil + } + reader.versions = func(_ context.Context, _ string, id uuid.UUID, page VersionPageQuery) (VersionPage, error) { + if id != skill || page != (VersionPageQuery{AfterVersion: 4, Limit: 2}) { + t.Fatalf("page = %s %+v", id, page) + } + return VersionPage{HasMore: true}, nil + } + if page, err := service.ListVersions(t.Context(), ListVersions{TenantID: "tenant", SkillID: skill, After: FormatVersionID(cursor), Limit: 2}); err != nil || !page.HasMore { + t.Fatalf("page = %+v, %v", page, err) + } +} + +// Content reads verify the decrypted archive against the version record. +func TestReadsVerifyContent(t *testing.T) { + service, _, reader := newTestService(t) + skill := uuid.New() + good := Content{Version: Version{Name: "proof", Description: "Verify a Skill."}, Archive: testArchive(t, "proof", "Verify a Skill.")} + tampered := Content{Version: Version{Name: "other", Description: "Verify a Skill."}, Archive: good.Archive} + reader.versionContent = func(_ context.Context, _ string, id uuid.UUID, version int64) (Content, error) { + if id != skill { + t.Fatalf("read %s", id) + } + if version == 1 { + return good, nil + } + return tampered, nil + } + if content, err := service.ReadVersion(t.Context(), ReadVersion{TenantID: "tenant", SkillID: skill, Version: 1}); err != nil || !reflect.DeepEqual(content, good) { + t.Fatalf("read = %v", err) + } + if content, err := service.ReadVersion(t.Context(), ReadVersion{TenantID: "tenant", SkillID: skill, Version: 2}); !errors.Is(err, ErrInvalidInput) || content.Archive != nil { + t.Fatalf("tampered read = %v", err) + } + reader.defaultVersionContent = func(context.Context, string, uuid.UUID) (Content, error) { return Content{}, ErrNotFound } + if _, err := service.ReadDefaultVersion(t.Context(), ReadDefaultVersion{TenantID: "tenant", SkillID: skill}); !errors.Is(err, ErrNotFound) { + t.Fatalf("default read = %v", err) + } +} diff --git a/services/core/internal/skills/skill.go b/services/core/internal/skills/skill.go new file mode 100644 index 000000000..37c6d730d --- /dev/null +++ b/services/core/internal/skills/skill.go @@ -0,0 +1,115 @@ +package skills + +import ( + "strings" + "time" + + "github.com/google/uuid" +) + +// Skill is a tenant-owned Skill. Its name and description are those of its +// default version. +type Skill struct { + ID string + Name string + Description string + CreatedAt time.Time + DefaultVersion int64 + LatestVersion int64 +} + +// Version is one immutable version of a Skill. Version numbers start at 1 and +// are never reused. +type Version struct { + ID string + SkillID string + Version int64 + Name string + Description string + CreatedAt time.Time +} + +// Content is a version and its archive. +type Content struct { + Version Version + Archive []byte +} + +// Page is one page of Skills; HasMore reports whether any Skill follows it. +type Page struct { + Skills []Skill + HasMore bool +} + +// VersionPage is one page of a Skill's versions. +type VersionPage struct { + Versions []Version + HasMore bool +} + +const ( + idPrefix = "skill_" + versionIDPrefix = "skillver_" +) + +// FormatID returns the public ID of the Skill whose key is id. +func FormatID(id uuid.UUID) string { return idPrefix + id.String() } + +// FormatVersionID returns the public ID of the version whose key is id. +func FormatVersionID(id uuid.UUID) string { return versionIDPrefix + id.String() } + +// ParseID returns the key of a public Skill ID: "skill_" followed by a +// canonical, nonzero UUID. Any other value names no Skill and is ErrNotFound. +func ParseID(value string) (uuid.UUID, error) { return parseID(value, idPrefix) } + +// ParseVersionID returns the key of a public version ID ("skillver_"). +func ParseVersionID(value string) (uuid.UUID, error) { return parseID(value, versionIDPrefix) } + +func parseID(value, prefix string) (uuid.UUID, error) { + id, err := uuid.Parse(strings.TrimPrefix(value, prefix)) + if err != nil || id == uuid.Nil || value != prefix+id.String() { + return uuid.Nil, ErrNotFound + } + return id, nil +} + +// PathID resolves a Skill ID from a request path. A malformed value resolves +// to the maximum UUID, which Core never assigns, so the request follows +// exactly the missing-Skill path. Request-body references use ParseID. +func PathID(value string) uuid.UUID { + id, err := ParseID(value) + if err != nil { + return uuid.Max + } + return id +} + +// PathVersion resolves a version number from a request path. Versions start +// at 1, so a malformed value resolves to the never-assigned version 0 and +// follows the missing-version path. Request-body selectors use ParseVersion. +func PathVersion(value string) int64 { + number, err := ParseVersion(value) + if err != nil { + return 0 + } + return number +} + +// SelectVersion resolves a Skill reference's version selector against the +// Skill's pointers: empty selects the default version, "latest" the latest, +// and any other value must be a concrete version. +func SelectVersion(selector string, defaultVersion, latestVersion int64) (int64, error) { + switch selector { + case "": + return defaultVersion, nil + case "latest": + return latestVersion, nil + } + return ParseVersion(selector) +} + +// ValidateSelector checks a version selector without resolving it. +func ValidateSelector(selector string) error { + _, err := SelectVersion(selector, 1, 1) + return err +} diff --git a/services/core/internal/skills/storage.go b/services/core/internal/skills/storage.go new file mode 100644 index 000000000..065fb0a7c --- /dev/null +++ b/services/core/internal/skills/storage.go @@ -0,0 +1,96 @@ +package skills + +import ( + "context" + "time" + + "github.com/google/uuid" +) + +// Storage persists Skill writes. Each method, and each WithVersionDeletion +// callback, runs in one transaction that also records the write's audit. +// Tenant IDs are UUIDs; a malformed one is ErrInvalidInput. +type Storage interface { + // CreateSkill stores a new Skill with the archive as its version 1, which + // is both its default and latest version. + CreateSkill(context.Context, NewSkill) (Skill, error) + // CreateVersion locks the Skill and stores the archive under the Skill's + // next version number. It is ErrNotFound for a missing Skill and + // ErrInvalidInput once the version numbers are exhausted. + CreateVersion(context.Context, NewVersion) (Version, error) + // SetDefaultVersion locks the Skill and points its default at an existing + // version, whose name and description the Skill then takes. + SetDefaultVersion(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Skill, error) + // DeleteSkill deletes the Skill and all its versions. + DeleteSkill(ctx context.Context, tenantID string, skillID uuid.UUID) error + // WithVersionDeletion locks the Skill, or is ErrNotFound, and runs apply + // in that transaction. It commits only when apply returns nil. + WithVersionDeletion(ctx context.Context, tenantID string, skillID uuid.UUID, apply func(VersionDeletionTx) error) error +} + +// NewSkill is a validated upload for a new Skill. +type NewSkill struct { + TenantID string + Name string + Description string + Archive []byte +} + +// NewVersion is a validated upload for an existing Skill. MakeDefault also +// points the Skill's default at the new version. +type NewVersion struct { + TenantID string + SkillID uuid.UUID + Name string + Description string + Archive []byte + MakeDefault bool +} + +// VersionDeletionTx is one locked Skill during a version deletion. +type VersionDeletionTx interface { + // LoadVersionDeletion loads the facts DecideVersionDeletion needs. It is + // ErrNotFound when the Skill has no such version. + LoadVersionDeletion(version int64) (VersionDeletionFacts, error) + // ApplyVersionDeletion carries out a decided deletion. + ApplyVersionDeletion(VersionDeletion) error +} + +// Reader reads Skills. Reads never lock and never decrypt metadata-only +// results. +type Reader interface { + Skill(ctx context.Context, tenantID string, id uuid.UUID) (Skill, error) + // Skills lists one page of the tenant's Skills in creation order, then by + // ID; a nil After starts at the first Skill. + Skills(ctx context.Context, tenantID string, page SkillPageQuery) (Page, error) + Version(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Version, error) + // VersionByID finds a version anywhere in the tenant, so a list cursor can + // tell another Skill's version from a missing one. + VersionByID(ctx context.Context, tenantID string, id uuid.UUID) (Version, error) + // Versions lists one page of a Skill's versions in version order. + Versions(ctx context.Context, tenantID string, skillID uuid.UUID, page VersionPageQuery) (VersionPage, error) + // VersionContent and DefaultVersionContent decrypt one version's archive. + VersionContent(ctx context.Context, tenantID string, skillID uuid.UUID, version int64) (Content, error) + DefaultVersionContent(ctx context.Context, tenantID string, skillID uuid.UUID) (Content, error) +} + +// SkillPageQuery selects one page of Skills. Limit is at most MaxPageLimit. +type SkillPageQuery struct { + After *SkillCursor + Limit int + Ascending bool +} + +// SkillCursor is the position of the Skill a page starts after. +type SkillCursor struct { + CreatedAt time.Time + ID uuid.UUID +} + +// VersionPageQuery selects one page of versions after the version number +// AfterVersion, or from the first when it is 0. +type VersionPageQuery struct { + AfterVersion int64 + Limit int + Ascending bool +} diff --git a/services/core/internal/store/admin_delete_audit_test.go b/services/core/internal/store/admin_delete_audit_test.go index 61f930028..6eeb2f6c1 100644 --- a/services/core/internal/store/admin_delete_audit_test.go +++ b/services/core/internal/store/admin_delete_audit_test.go @@ -12,6 +12,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/adminaudit" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/sessions" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" "github.com/google/uuid" "github.com/jackc/pgx/v5" ) @@ -47,10 +48,19 @@ func requireAdminAuditFailure(t *testing.T, s *Store, err error, before int64) { } } +type resourceAuditMutation struct { + action, kind, parent string + run func(context.Context) (string, error) +} + func adminDeleteContext(ctx context.Context, tenant, request string) context.Context { // Even an inherited public provenance context must not turn an administrator // operation into a user-key operation. - return adminaudit.WithSource(resourceAuditContext(ctx, tenant, request), adminaudit.Source{ + public := writeaudit.WithSource(ctx, writeaudit.Source{ + KeyID: "static:" + strings.Repeat("a", 64), Name: "resource audit fixture", Prefix: "aaaaaaaa", + Kind: "static", TenantID: tenant, RequestID: request, TraceID: "resource-audit-trace", + }) + return adminaudit.WithSource(public, adminaudit.Source{ CredentialID: "87654321", ActorLabel: "administrator fixture", ProjectID: tenant, RequestID: request, TraceID: "admin-mutation-trace", }) } @@ -81,7 +91,7 @@ func assertAdminMutationAudit(t *testing.T, s *Store, tenant, request, action, k if credential != "87654321" || actor != "administrator fixture" || key != expectedKey || trace != "admin-mutation-trace" || gotAction != action || gotKind != kind || gotID != id || mappings != "[]" { t.Fatal("administrator audit identity differs") } - for _, secret := range []string{"admin-private-archive", "admin-private-body", "private-agent-canary", "audit-private-token"} { + for _, secret := range []string{"admin-private-body", "private-agent-canary", "audit-private-token"} { if strings.Contains(raw, secret) { t.Fatal("private content entered administrator audit") } @@ -103,19 +113,10 @@ func TestAdminDeleteResourceAuditTransactions(t *testing.T) { } s := NewWithCredentialCipher(pool, cipher) rejectAdminAuditInsert(t, s) - archive := skillArchive(t, "admin-private-archive") - tables := []string{"agents", "agent_model_execution", "skills", "skill_versions", "sessions", "turns", "environments", "session_artifacts", "admin_audit_log", "write_audit_operations", "write_audit_owners", "pg_largeobject_metadata", "pg_largeobject"} - for _, name := range []string{"skill_delete", "version_delete", "version_delete_last", "session_delete", "artifact_delete"} { + tables := []string{"agents", "agent_model_execution", "sessions", "turns", "environments", "session_artifacts", "admin_audit_log", "write_audit_operations", "write_audit_owners", "pg_largeobject_metadata", "pg_largeobject"} + for _, name := range []string{"session_delete", "artifact_delete"} { t.Run(name, func(t *testing.T) { - tenant := uuid.NewString() - var mutation resourceAuditMutation - var verifyRestored func() - removedObjects := 0 - if name == "session_delete" || name == "artifact_delete" { - tenant, mutation, verifyRestored, removedObjects = prepareAdminHistoryDelete(t, s, name) - } else { - mutation = prepareResourceAuditMutation(t, s, tenant, name, archive) - } + tenant, mutation, verifyRestored, removedObjects := prepareAdminHistoryDelete(t, s, name) if _, err := pool.Exec(t.Context(), "INSERT INTO execution_project_scopes(tenant_id,organization_id,project_id) VALUES($1,'admin-delete',$2)", tenant, tenant); err != nil { t.Fatal(err) } @@ -192,18 +193,6 @@ func assertAdminDeletedResource(t *testing.T, s *Store, tenant string, mutation t.Helper() var err error switch mutation.kind { - case "skill": - _, err = s.GetSkill(t.Context(), tenant, id) - case "skill_version": - versionID, parseErr := skillResourceID(id, "skillver_") - if parseErr != nil { - t.Fatal(parseErr) - } - var count int - if queryErr := s.pool.QueryRow(t.Context(), "SELECT count(*) FROM skill_versions WHERE tenant_id=$1 AND id=$2", tenant, versionID).Scan(&count); queryErr != nil || count != 0 { - t.Fatal("skill version survived deletion", queryErr) - } - return case "session": _, err = s.GetSession(t.Context(), tenant, id) case "artifact": diff --git a/services/core/internal/store/environment_skill_references.go b/services/core/internal/store/environment_skill_references.go index 37ac91428..27c355381 100644 --- a/services/core/internal/store/environment_skill_references.go +++ b/services/core/internal/store/environment_skill_references.go @@ -6,10 +6,12 @@ import ( "sort" "strconv" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/db/sqlc" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/environmentconfig" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" ) // freezeEnvironmentSkills runs only for a newly inserted Session, in its transaction. @@ -32,11 +34,15 @@ func (s *Store) freezeEnvironmentSkills(ctx context.Context, q *sqlc.Queries, te } sort.Strings(ids) for _, id := range ids { - tenant, skill, err := skillIDs(tenantID, id) + tenant, err := parseID(tenantID) if err != nil { return environmentconfig.Setup{}, err } - owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: skill}) + skill, err := skills.ParseID(id) + if err != nil { + return environmentconfig.Setup{}, ErrNotFound + } + owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: pgtype.UUID{Bytes: skill, Valid: true}}) if errors.Is(err, pgx.ErrNoRows) { err = ErrNotFound } @@ -52,17 +58,9 @@ func (s *Store) freezeEnvironmentSkills(ctx context.Context, q *sqlc.Queries, te continue } owner := owners[skill.Metadata.SkillID] - number := owner.DefaultVersion - switch skill.Metadata.Version { - case "": - case "latest": - number = owner.LatestVersion - default: - var err error - number, err = skills.ParseVersion(skill.Metadata.Version) - if err != nil { - return environmentconfig.Setup{}, ErrInvalidInput - } + number, err := skills.SelectVersion(skill.Metadata.Version, owner.DefaultVersion, owner.LatestVersion) + if err != nil { + return environmentconfig.Setup{}, ErrInvalidInput } row, err := q.ReadSkillVersion(ctx, sqlc.ReadSkillVersionParams{TenantID: owner.TenantID, SkillID: owner.ID, Version: number}) if errors.Is(err, pgx.ErrNoRows) { @@ -71,14 +69,29 @@ func (s *Store) freezeEnvironmentSkills(ctx context.Context, q *sqlc.Queries, te if err != nil { return environmentconfig.Setup{}, err } - version, archive, err := s.openSkillVersion(row) + content, err := s.openFrozenSkill(row) if err != nil { return environmentconfig.Setup{}, err } - result.Skills[i] = environmentconfig.Skill{Metadata: environmentconfig.SkillMetadata{Type: "skill_reference", SkillID: version.SkillID, Version: strconv.FormatInt(version.Version, 10), Name: version.Name, Description: version.Description}, Archive: archive} + version := content.Version + result.Skills[i] = environmentconfig.Skill{Metadata: environmentconfig.SkillMetadata{Type: "skill_reference", SkillID: version.SkillID, Version: strconv.FormatInt(version.Version, 10), Name: version.Name, Description: version.Description}, Archive: content.Archive} } if result.ValidateInstalled() != nil { return environmentconfig.Setup{}, ErrInvalidInput } return result, nil } + +// openFrozenSkill decrypts a version the Session transaction selected and +// checks it is still the bundle its version records. +func (s *Store) openFrozenSkill(row sqlc.SkillVersion) (skills.Content, error) { + archive, err := s.credentialCipher.OpenSkill(row.Contents, credentialcrypto.NewSkillBinding(row.TenantID.Bytes, row.SkillID.Bytes, row.ID.Bytes, row.Version)) + if err != nil { + return skills.Content{}, err + } + content := skills.Content{Version: skills.Version{ID: skills.FormatVersionID(row.ID.Bytes), SkillID: skills.FormatID(row.SkillID.Bytes), Version: row.Version, Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt.Time}, Archive: archive} + if skills.VerifyContent(content) != nil { + return skills.Content{}, ErrInvalidInput + } + return content, nil +} diff --git a/services/core/internal/store/environment_skill_references_test.go b/services/core/internal/store/environment_skill_references_test.go index a6c94c622..96dc98c48 100644 --- a/services/core/internal/store/environment_skill_references_test.go +++ b/services/core/internal/store/environment_skill_references_test.go @@ -9,6 +9,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/environmentconfig" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/google/uuid" ) @@ -19,13 +20,15 @@ func TestSkillReferencesFreezeWithinSessionCreation(t *testing.T) { t.Fatal(err) } s := NewWithCredentialCipher(pool, cipher) + skillService := SkillService(t, pool, cipher) tenant := uuid.NewString() first, second := skillArchive(t, "frozen-first"), skillArchive(t, "frozen-second") - skill, err := s.CreateSkill(t.Context(), tenant, first) + skill, err := skillService.CreateSkill(t.Context(), skills.CreateSkill{TenantID: tenant, Archive: first}) if err != nil { t.Fatal(err) } - if _, err = s.CreateSkillVersion(t.Context(), tenant, skill.ID, second, false); err != nil { + skillID := skills.PathID(skill.ID) + if _, err = skillService.CreateVersion(t.Context(), skills.CreateVersion{TenantID: tenant, SkillID: skillID, Archive: second}); err != nil { t.Fatal(err) } intent := environmentconfig.Setup{Skills: []environmentconfig.Skill{{Metadata: environmentconfig.SkillMetadata{Type: "skill_reference", SkillID: skill.ID}}}} @@ -90,7 +93,7 @@ func TestSkillReferencesFreezeWithinSessionCreation(t *testing.T) { if input.Initialization.Skills[0].Metadata.Version != "" || len(input.Initialization.Skills[0].Archive) != 0 { t.Fatal("creation mutated caller intent") } - if _, err = s.UpdateSkillDefault(t.Context(), tenant, skill.ID, "2"); err != nil { + if _, err = skillService.SetDefaultVersion(t.Context(), skills.SetDefaultVersion{TenantID: tenant, SkillID: skillID, Version: "2"}); err != nil { t.Fatal(err) } for _, selector := range []string{"", "latest", "1"} { @@ -107,7 +110,7 @@ func TestSkillReferencesFreezeWithinSessionCreation(t *testing.T) { assertFrozen(created.ID, "2", second) } } - if err = s.DeleteSkill(t.Context(), tenant, skill.ID); err != nil { + if err = skillService.DeleteSkill(t.Context(), skills.DeleteSkill{TenantID: tenant, SkillID: skillID}); err != nil { t.Fatal(err) } retry, err := s.CreateSession(t.Context(), tenant, input) @@ -128,7 +131,7 @@ func TestSkillReferenceAuthorizationRollsBackSession(t *testing.T) { } s := NewWithCredentialCipher(pool, cipher) tenant, foreign := uuid.NewString(), uuid.NewString() - skill, err := s.CreateSkill(t.Context(), tenant, skillArchive(t, "private-owner")) + skill, err := SkillService(t, pool, cipher).CreateSkill(t.Context(), skills.CreateSkill{TenantID: tenant, Archive: skillArchive(t, "private-owner")}) if err != nil { t.Fatal(err) } diff --git a/services/core/internal/store/list_cursor_public_test.go b/services/core/internal/store/list_cursor_public_test.go index 977e457c3..79981e39b 100644 --- a/services/core/internal/store/list_cursor_public_test.go +++ b/services/core/internal/store/list_cursor_public_test.go @@ -16,6 +16,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimedevice" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/sessions" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/google/uuid" ) @@ -33,7 +34,7 @@ type cursorFixture struct { file string } -func seedCursorFixture(t *testing.T, s *store.Store, writer *store.Store, client pathIDClient, token, tenant, label string) cursorFixture { +func seedCursorFixture(t *testing.T, s *store.Store, writer *store.Store, skillService *skills.Service, client pathIDClient, token, tenant, label string) cursorFixture { t.Helper() ctx := t.Context() var f cursorFixture @@ -149,34 +150,35 @@ func seedCursorFixture(t *testing.T, s *store.Store, writer *store.Store, client f.otherSubagent = subagent(otherSubSession, "child") f.otherChildTurn = first("/v1/agents/sessions/" + otherSubSession + "/subagents/" + f.otherSubagent + "/turns") - skill, err := s.CreateSkill(ctx, tenant, store.SkillArchive(t, label+"-cursor-skill")) + skill, err := skillService.CreateSkill(ctx, skills.CreateSkill{TenantID: tenant, Archive: store.SkillArchive(t, label+"-cursor-skill")}) if err != nil { t.Fatal(err) } f.skill = skill.ID - versions, err := s.ListSkillVersions(ctx, tenant, skill.ID, "", 10, true) + skillID := skills.PathID(skill.ID) + versions, err := skillService.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: skillID, Limit: 10, Ascending: true}) if err != nil || len(versions.Versions) != 1 { t.Fatal("fixture Skill version", versions, err) } f.version = versions.Versions[0].ID - later, err := s.CreateSkillVersion(ctx, tenant, skill.ID, store.SkillArchive(t, label+"-cursor-skill-v2"), false) + later, err := skillService.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: skillID, Archive: store.SkillArchive(t, label+"-cursor-skill-v2")}) if err != nil { t.Fatal(err) } f.laterVersion = later.ID - deleted, err := s.CreateSkillVersion(ctx, tenant, skill.ID, store.SkillArchive(t, label+"-cursor-skill-v3"), false) + deleted, err := skillService.CreateVersion(ctx, skills.CreateVersion{TenantID: tenant, SkillID: skillID, Archive: store.SkillArchive(t, label+"-cursor-skill-v3")}) if err != nil { t.Fatal(err) } - if _, err = s.DeleteSkillVersion(ctx, tenant, skill.ID, "3"); err != nil { + if _, err = skillService.DeleteVersion(ctx, skills.DeleteVersion{TenantID: tenant, SkillID: skillID, Version: 3}); err != nil { t.Fatal(err) } f.deletedVersion = deleted.ID - otherSkill, err := s.CreateSkill(ctx, tenant, store.SkillArchive(t, label+"-cursor-other-skill")) + otherSkill, err := skillService.CreateSkill(ctx, skills.CreateSkill{TenantID: tenant, Archive: store.SkillArchive(t, label+"-cursor-other-skill")}) if err != nil { t.Fatal(err) } - otherVersions, err := s.ListSkillVersions(ctx, tenant, otherSkill.ID, "", 10, true) + otherVersions, err := skillService.ListVersions(ctx, skills.ListVersions{TenantID: tenant, SkillID: skills.PathID(otherSkill.ID), Limit: 10, Ascending: true}) if err != nil || len(otherVersions.Versions) != 1 { t.Fatal("fixture other Skill version", otherVersions, err) } @@ -237,8 +239,9 @@ func TestListCursorErrorsPostgres(t *testing.T) { defer server.Close() client := pathIDClient{t: t, server: server} writer := executionOwner(t, db, s).Store - a := seedCursorFixture(t, s, writer, client, owner, ownerTenant, "a") - b := seedCursorFixture(t, s, writer, client, foreign, foreignTenant, "b") + skillService := store.SkillService(t, db.pool, db.cipher) + a := seedCursorFixture(t, s, writer, skillService, client, owner, ownerTenant, "a") + b := seedCursorFixture(t, s, writer, skillService, client, foreign, foreignTenant, "b") text := func(value string) *string { return &value } var ( diff --git a/services/core/internal/store/list_cursors.go b/services/core/internal/store/list_cursors.go index f3821fe34..697573ca5 100644 --- a/services/core/internal/store/list_cursors.go +++ b/services/core/internal/store/list_cursors.go @@ -1,11 +1,6 @@ package store -import ( - "errors" - "fmt" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/echotext" -) +import "errors" // InvalidCursorError reports a list `after` cursor that does not name a // resource of that list once its parents have resolved. Message is the list @@ -25,20 +20,8 @@ var ( errResourceCursor = &InvalidCursorError{Message: "Invalid resource ID in `after`"} // Session Artifacts. errArtifactCursor = &InvalidCursorError{Message: "after is not a valid artifact ID"} - // A Skill version of another Skill in the same tenant. - errSkillVersionCursorParent = &InvalidCursorError{Message: "Skill version cursor does not match this skill."} ) -// skillVersionCursorPrefix reports a Skill version cursor that is not a -// version resource ID at all. The official message echoes the value; a long or -// unprintable value is left out so the error stays bounded. -func skillVersionCursorPrefix(value string) error { - if !echotext.Allowed(value) { - return &InvalidCursorError{Message: "Invalid 'after'. Expected an ID that begins with 'skillver'."} - } - return &InvalidCursorError{Message: fmt.Sprintf("Invalid 'after': '%s'. Expected an ID that begins with 'skillver'.", value)} -} - // unresolvedCursor replaces a missing cursor resource with the list's cursor // error and keeps every other failure. func unresolvedCursor(err, cursor error) error { diff --git a/services/core/internal/store/path_id_semantics_public_test.go b/services/core/internal/store/path_id_semantics_public_test.go index e831cedba..a3e9dca85 100644 --- a/services/core/internal/store/path_id_semantics_public_test.go +++ b/services/core/internal/store/path_id_semantics_public_test.go @@ -12,6 +12,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimedevice" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/google/uuid" "github.com/jackc/pgx/v5/pgxpool" @@ -122,7 +123,7 @@ func TestMalformedPathIDsMatchMissingPostgres(t *testing.T) { t.Fatal("fixture Environment", err) } environment := hosted.Environment.ID - skill, err := s.CreateSkill(t.Context(), ownerTenant, store.SkillArchive(t, "path-skill")) + skill, err := store.SkillService(t, pool, cipher).CreateSkill(t.Context(), skills.CreateSkill{TenantID: ownerTenant, Archive: store.SkillArchive(t, "path-skill")}) if err != nil { t.Fatal(err) } diff --git a/services/core/internal/store/public_handler_fixture_test.go b/services/core/internal/store/public_handler_fixture_test.go index d24d71535..35143f4a0 100644 --- a/services/core/internal/store/public_handler_fixture_test.go +++ b/services/core/internal/store/public_handler_fixture_test.go @@ -19,11 +19,13 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/filepg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/modelconfigurationpg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgunit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/skillpg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/templatepg" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimedevice" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimehistory" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimeobs" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/sandbox" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" ) @@ -61,13 +63,19 @@ func publicHandler(t testing.TB, s *store.Store, db fixtureDB, keys fixtureKeyRe if err != nil { return nil, err } + skillStore := skillpg.New(pgunit.NewPool(db.pool), db.cipher) + skillService, err := skills.NewService(skillStore, skillStore) + if err != nil { + return nil, err + } deps := api.Dependencies{ Engine: engine, CoreKeys: admin, InstallationBindings: s, - Projects: fixtureProjects{Store: s, keys: keys}, Skills: s, + Projects: fixtureProjects{Store: s, keys: keys}, ModelProviders: modelConfigurationService, ModelProvidersReader: modelConfigurationStore, Vaults: vaultService, VaultsReader: vaultStore, Files: fileService, FilesReader: fileStore, EnvironmentTemplates: environmentTemplates, EnvironmentTemplatesReader: templates, + Skills: skillService, SkillsReader: skillStore, Agents: agentService, AgentsReader: agentStore, Sessions: s, SessionEvents: s, SessionHistory: s, Subagents: s, Artifacts: s, SessionAdmin: s, Environments: s, Admin: s, AdminAudit: audit, WriteAudit: audit, diff --git a/services/core/internal/store/skill_default_metadata_test.go b/services/core/internal/store/skill_default_metadata_test.go deleted file mode 100644 index 005ed426a..000000000 --- a/services/core/internal/store/skill_default_metadata_test.go +++ /dev/null @@ -1,105 +0,0 @@ -package store - -import ( - "archive/zip" - "bytes" - "errors" - "fmt" - "strconv" - "testing" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" - "github.com/google/uuid" -) - -func skillMetadataArchive(t *testing.T, name, description string) []byte { - t.Helper() - var buffer bytes.Buffer - writer := zip.NewWriter(&buffer) - file, err := writer.CreateHeader(&zip.FileHeader{Name: name + "/SKILL.md", Method: zip.Store}) - if err != nil { - t.Fatal(err) - } - if _, err = fmt.Fprintf(file, "---\nname: %s\ndescription: %s\n---\nPrivate marker for %s.\n", name, description, name); err != nil { - t.Fatal(err) - } - if err = writer.Close(); err != nil { - t.Fatal(err) - } - return buffer.Bytes() -} - -func TestSkillMetadataTracksDefaultVersion(t *testing.T) { - _, pool := testStore(t) - cipher, err := credentialcrypto.New(bytes.Repeat([]byte{74}, 32)) - if err != nil { - t.Fatal(err) - } - s := NewWithCredentialCipher(pool, cipher) - metadataOnly := New(pool) - tenant, foreign := uuid.NewString(), uuid.NewString() - names := []string{"first-proof", "second-proof", "third-proof"} - descriptions := []string{"First immutable version.", "Second immutable version.", "Third immutable version."} - archives := make([][]byte, len(names)) - for i := range names { - archives[i] = skillMetadataArchive(t, names[i], descriptions[i]) - } - created, err := s.CreateSkill(t.Context(), tenant, archives[0]) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, created.ID) }) - assertMetadata := func(value Skill, version, latest int64) { - t.Helper() - if value.ID != created.ID || !value.CreatedAt.Equal(created.CreatedAt) || value.DefaultVersion != version || value.LatestVersion != latest || value.Name != names[version-1] || value.Description != descriptions[version-1] { - t.Fatalf("metadata does not track default %d/latest %d: %+v", version, latest, value) - } - } - assertStored := func(version, latest int64) { - t.Helper() - value, err := metadataOnly.GetSkill(t.Context(), tenant, created.ID) - if err != nil { - t.Fatal("metadata read without content key", err) - } - assertMetadata(value, version, latest) - page, err := metadataOnly.ListSkills(t.Context(), tenant, "", 10, true) - if err != nil || len(page.Skills) != 1 { - t.Fatal("metadata list without content key", page, err) - } - assertMetadata(page.Skills[0], version, latest) - selected, body, err := s.ReadDefaultSkillVersion(t.Context(), tenant, created.ID) - if err != nil || selected.Version != version || selected.Name != names[version-1] || selected.Description != descriptions[version-1] || !bytes.Equal(body, archives[version-1]) { - t.Fatal("default content differs from public metadata", selected, err) - } - } - assertMetadata(created, 1, 1) - if _, err = s.CreateSkillVersion(t.Context(), tenant, created.ID, archives[1], false); err != nil { - t.Fatal(err) - } - assertStored(1, 2) - for _, version := range []int64{2, 1} { - updated, err := metadataOnly.UpdateSkillDefault(t.Context(), tenant, created.ID, strconv.FormatInt(version, 10)) - if err != nil { - t.Fatal("default update without content key", err) - } - assertMetadata(updated, version, 2) - assertStored(version, 2) - } - if _, err = metadataOnly.UpdateSkillDefault(t.Context(), foreign, created.ID, "2"); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign default update", err) - } - if _, err = metadataOnly.UpdateSkillDefault(t.Context(), tenant, created.ID, "999"); !errors.Is(err, ErrNotFound) { - t.Fatal("missing default update", err) - } - assertStored(1, 2) - if _, err = s.CreateSkillVersion(t.Context(), tenant, created.ID, archives[2], true); err != nil { - t.Fatal(err) - } - assertStored(3, 3) - for i := range archives { - version, body, err := s.ReadSkillVersion(t.Context(), tenant, created.ID, strconv.Itoa(i+1)) - if err != nil || version.Name != names[i] || version.Description != descriptions[i] || !bytes.Equal(body, archives[i]) { - t.Fatal("default changes modified immutable version", i+1, version, err) - } - } -} diff --git a/services/core/internal/store/skill_fixture_test.go b/services/core/internal/store/skill_fixture_test.go new file mode 100644 index 000000000..8f0c141bc --- /dev/null +++ b/services/core/internal/store/skill_fixture_test.go @@ -0,0 +1,44 @@ +package store + +import ( + "archive/zip" + "bytes" + "fmt" + "testing" + + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/pgunit" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/skillpg" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" + "github.com/jackc/pgx/v5/pgxpool" +) + +// skillArchive builds a minimal valid Skill archive. +func skillArchive(t *testing.T, marker string) []byte { + t.Helper() + var buffer bytes.Buffer + writer := zip.NewWriter(&buffer) + file, err := writer.CreateHeader(&zip.FileHeader{Name: "proof/SKILL.md", Method: zip.Store}) + if err != nil { + t.Fatal(err) + } + if _, err = fmt.Fprintf(file, "---\nname: proof\ndescription: Verify a versioned Skill.\n---\n%s", marker); err != nil { + t.Fatal(err) + } + if err = writer.Close(); err != nil { + t.Fatal(err) + } + return buffer.Bytes() +} + +// SkillService manages Skills in the database of a Store test. cipher must be +// the Store's, so Session creation can open the versions it freezes. +func SkillService(t testing.TB, pool *pgxpool.Pool, cipher *credentialcrypto.Cipher) *skills.Service { + t.Helper() + skillStore := skillpg.New(pgunit.NewPool(pool), cipher) + service, err := skills.NewService(skillStore, skillStore) + if err != nil { + t.Fatal(err) + } + return service +} diff --git a/services/core/internal/store/skill_version_deletion_public_test.go b/services/core/internal/store/skill_version_deletion_public_test.go index d63940cde..54eb4ce7f 100644 --- a/services/core/internal/store/skill_version_deletion_public_test.go +++ b/services/core/internal/store/skill_version_deletion_public_test.go @@ -11,6 +11,7 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/runtimedevice" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/store" "github.com/google/uuid" ) @@ -56,11 +57,14 @@ func TestSkillVersionDeletionHTTPPostgres(t *testing.T) { } missing := expect(owner, http.MethodDelete, "/v1/skills/skill_"+uuid.NewString()+"/versions/1", http.StatusNotFound) - sole, err := s.CreateSkill(t.Context(), ownerTenant, store.SkillArchive(t, "sole-http")) + skillService := store.SkillService(t, pool, cipher) + sole, err := skillService.CreateSkill(t.Context(), skills.CreateSkill{TenantID: ownerTenant, Archive: store.SkillArchive(t, "sole-http")}) if err != nil { t.Fatal(err) } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), ownerTenant, sole.ID) }) + t.Cleanup(func() { + _ = skillService.DeleteSkill(t.Context(), skills.DeleteSkill{TenantID: ownerTenant, SkillID: skills.PathID(sole.ID)}) + }) version := object(expect(owner, http.MethodGet, "/v1/skills/"+sole.ID+"/versions/1", http.StatusOK)) path := "/v1/skills/" + sole.ID // V4: a foreign tenant receives the missing-Skill response and changes nothing. @@ -85,12 +89,14 @@ func TestSkillVersionDeletionHTTPPostgres(t *testing.T) { expect(owner, http.MethodDelete, path, http.StatusNotFound) // V2 and V3 on a Skill with two versions, then V1 on the reduced Skill. - pair, err := s.CreateSkill(t.Context(), ownerTenant, store.SkillArchive(t, "pair-one")) + pair, err := skillService.CreateSkill(t.Context(), skills.CreateSkill{TenantID: ownerTenant, Archive: store.SkillArchive(t, "pair-one")}) if err != nil { t.Fatal(err) } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), ownerTenant, pair.ID) }) - if _, err = s.CreateSkillVersion(t.Context(), ownerTenant, pair.ID, store.SkillArchive(t, "pair-two"), false); err != nil { + t.Cleanup(func() { + _ = skillService.DeleteSkill(t.Context(), skills.DeleteSkill{TenantID: ownerTenant, SkillID: skills.PathID(pair.ID)}) + }) + if _, err = skillService.CreateVersion(t.Context(), skills.CreateVersion{TenantID: ownerTenant, SkillID: skills.PathID(pair.ID), Archive: store.SkillArchive(t, "pair-two")}); err != nil { t.Fatal(err) } path = "/v1/skills/" + pair.ID diff --git a/services/core/internal/store/skill_version_deletion_test.go b/services/core/internal/store/skill_version_deletion_test.go index 2f1d7c1b7..948b458d6 100644 --- a/services/core/internal/store/skill_version_deletion_test.go +++ b/services/core/internal/store/skill_version_deletion_test.go @@ -3,46 +3,28 @@ package store import ( "bytes" "encoding/json" - "errors" "reflect" "testing" - "time" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/environmentconfig" + "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" "github.com/google/uuid" - "github.com/jackc/pgx/v5/pgxpool" ) -func skillVersionDeletionStore(t *testing.T) (*Store, *pgxpool.Pool) { - t.Helper() +// Deleting a Skill's sole version deletes the Skill, but committed Session +// snapshots and their idempotent retries are unchanged. +func TestSoleSkillVersionDeletionKeepsFrozenSetup(t *testing.T) { _, pool := testStore(t) cipher, err := credentialcrypto.New(bytes.Repeat([]byte{63}, 32)) if err != nil { t.Fatal(err) } - return NewWithCredentialCipher(pool, cipher), pool -} - -func skillRowCounts(t *testing.T, pool *pgxpool.Pool, skillID string) (skills, versions int) { - t.Helper() - id := uuid.MustParse(skillID[len("skill_"):]) - if err := pool.QueryRow(t.Context(), "SELECT (SELECT count(*) FROM skills WHERE id=$1), (SELECT count(*) FROM skill_versions WHERE skill_id=$1)", id).Scan(&skills, &versions); err != nil { - t.Fatal(err) - } - return skills, versions -} - -func TestSoleSkillVersionDeletionRemovesSkill(t *testing.T) { - s, pool := skillVersionDeletionStore(t) - tenant, foreign := uuid.NewString(), uuid.NewString() + s := NewWithCredentialCipher(pool, cipher) + skillService := SkillService(t, pool, cipher) + tenant := uuid.NewString() archive := skillArchive(t, "sole-version-frozen") - skill, err := s.CreateSkill(t.Context(), tenant, archive) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, skill.ID) }) - version, err := s.GetSkillVersion(t.Context(), tenant, skill.ID, "1") + skill, err := skillService.CreateSkill(t.Context(), skills.CreateSkill{TenantID: tenant, Archive: archive}) if err != nil { t.Fatal(err) } @@ -56,46 +38,10 @@ func TestSoleSkillVersionDeletionRemovesSkill(t *testing.T) { if err != nil || len(frozen.Skills) != 1 || frozen.Skills[0].Metadata.Version != "1" || !bytes.Equal(frozen.Skills[0].Archive, archive) { t.Fatal("fixture Session did not freeze the sole version", err) } - - // Foreign and missing targets fail before any mutation. - if _, err = s.DeleteSkillVersion(t.Context(), foreign, skill.ID, "1"); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign sole-version deletion", err) - } - if _, err = s.DeleteSkillVersion(t.Context(), tenant, skill.ID, "2"); !errors.Is(err, ErrNotFound) { - t.Fatal("missing version deletion", err) - } - if skills, versions := skillRowCounts(t, pool, skill.ID); skills != 1 || versions != 1 { - t.Fatal("rejected deletion changed rows", skills, versions) - } - - deleted, err := s.DeleteSkillVersion(t.Context(), tenant, skill.ID, "1") - if err != nil || deleted.ID != version.ID || deleted.SkillID != skill.ID || deleted.Version != 1 { - t.Fatal("sole-version deletion", deleted, err) - } - // The Skill and every encrypted version row are gone in the same commit. - if skills, versions := skillRowCounts(t, pool, skill.ID); skills != 0 || versions != 0 { - t.Fatal("orphaned Skill rows", skills, versions) - } - if _, err = s.GetSkill(t.Context(), tenant, skill.ID); !errors.Is(err, ErrNotFound) { - t.Fatal("deleted Skill is readable", err) - } - if _, err = s.ListSkillVersions(t.Context(), tenant, skill.ID, "", 20, false); !errors.Is(err, ErrNotFound) { - t.Fatal("deleted Skill versions are listable", err) - } - if _, _, err = s.ReadDefaultSkillVersion(t.Context(), tenant, skill.ID); !errors.Is(err, ErrNotFound) { - t.Fatal("deleted Skill content is readable", err) - } - if page, err := s.ListSkills(t.Context(), tenant, "", 20, false); err != nil || len(page.Skills) != 0 { - t.Fatal("deleted Skill is listed", page, err) - } - if _, err = s.DeleteSkillVersion(t.Context(), tenant, skill.ID, "1"); !errors.Is(err, ErrNotFound) { - t.Fatal("repeated sole-version deletion", err) - } - if err = s.DeleteSkill(t.Context(), tenant, skill.ID); !errors.Is(err, ErrNotFound) { - t.Fatal("Skill deletion after sole-version deletion", err) + if _, err = skillService.DeleteVersion(t.Context(), skills.DeleteVersion{TenantID: tenant, SkillID: skills.PathID(skill.ID), Version: 1}); err != nil { + t.Fatal(err) } - // Committed snapshots are unchanged, as with DeleteSkill. after, err := s.ReadEnvironmentSetup(t.Context(), tenant, session.ID) if err != nil || !reflect.DeepEqual(after.Skills, frozen.Skills) { t.Fatal("frozen Session installation changed", err) @@ -105,87 +51,3 @@ func TestSoleSkillVersionDeletionRemovesSkill(t *testing.T) { t.Fatal("committed retry read the deleted source", err) } } - -// Upload and sole-version deletion serialize on the Skill row: an upload that -// commits first makes the default undeletable, and a deletion that commits -// first makes the later upload miss the Skill. Neither loses acknowledged data. -func TestSoleSkillVersionDeletionSerializesWithUpload(t *testing.T) { - s, pool := skillVersionDeletionStore(t) - tenant := uuid.NewString() - for _, uploadFirst := range []bool{true, false} { - skill, err := s.CreateSkill(t.Context(), tenant, skillArchive(t, "race-first")) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, skill.ID) }) - // Hold the owner lock so both operations queue in a known order. - holder, err := pool.Begin(t.Context()) - if err != nil { - t.Fatal(err) - } - var holderPID int32 - if err = holder.QueryRow(t.Context(), "SELECT pg_backend_pid() FROM skills WHERE id=$1 FOR UPDATE", uuid.MustParse(skill.ID[len("skill_"):])).Scan(&holderPID); err != nil { - t.Fatal(err) - } - type outcome struct { - version SkillVersion - err error - } - uploaded, deleted := make(chan outcome, 1), make(chan outcome, 1) - second := skillArchive(t, "race-second") - upload := func() { - version, err := s.CreateSkillVersion(t.Context(), tenant, skill.ID, second, false) - uploaded <- outcome{version, err} - } - remove := func() { - version, err := s.DeleteSkillVersion(t.Context(), tenant, skill.ID, "1") - deleted <- outcome{version, err} - } - early, late := upload, remove - if !uploadFirst { - early, late = remove, upload - } - go early() - waitForSkillLockWaiters(t, pool, holderPID, 1) - go late() - waitForSkillLockWaiters(t, pool, holderPID, 2) - if err = holder.Rollback(t.Context()); err != nil { - t.Fatal(err) - } - up, del := <-uploaded, <-deleted - skills, versions := skillRowCounts(t, pool, skill.ID) - if uploadFirst { - if up.err != nil || up.version.Version != 2 || !errors.Is(del.err, ErrDefaultSkillVersion) || skills != 1 || versions != 2 { - t.Fatal("upload before deletion", up, del, skills, versions) - } - current, err := s.GetSkill(t.Context(), tenant, skill.ID) - if err != nil || current.DefaultVersion != 1 || current.LatestVersion != 2 { - t.Fatal("pointers after serialized upload", current, err) - } - } else if del.err != nil || del.version.Version != 1 || !errors.Is(up.err, ErrNotFound) || skills != 0 || versions != 0 { - t.Fatal("deletion before upload", up, del, skills, versions) - } - } -} - -// waitForSkillLockWaiters waits until count sessions queue behind holder. -func waitForSkillLockWaiters(t *testing.T, pool *pgxpool.Pool, holder int32, count int) { - t.Helper() - deadline := time.Now().Add(10 * time.Second) - for { - var waiting int - err := pool.QueryRow(t.Context(), `WITH RECURSIVE queued(pid) AS ( - SELECT $1::int UNION SELECT a.pid FROM pg_stat_activity a JOIN queued q ON q.pid = ANY(pg_blocking_pids(a.pid)) -) SELECT count(*) - 1 FROM queued`, holder).Scan(&waiting) - if err != nil { - t.Fatal(err) - } - if waiting >= count { - return - } - if time.Now().After(deadline) { - t.Fatal("lock waiters", waiting, count) - } - time.Sleep(10 * time.Millisecond) - } -} diff --git a/services/core/internal/store/skill_versions.go b/services/core/internal/store/skill_versions.go deleted file mode 100644 index a8a4c3ebc..000000000 --- a/services/core/internal/store/skill_versions.go +++ /dev/null @@ -1,158 +0,0 @@ -package store - -import ( - "context" - "errors" - "math" - - "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/db/sqlc" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/auditpg" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" - "github.com/jackc/pgx/v5" -) - -// ErrDefaultSkillVersion rejects deleting the default while other versions remain. -var ErrDefaultSkillVersion = errors.New("cannot delete the default skill version") - -func (s *Store) CreateSkillVersion(ctx context.Context, tenantID, skillID string, archive []byte, makeDefault bool) (SkillVersion, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersion{}, err - } - metadata, err := agentskill.Inspect(archive) - if err != nil { - return SkillVersion{}, ErrInvalidInput - } - var result SkillVersion - err = s.pooled.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { - q := s.queries.WithTx(tx) - owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: id}) - if err != nil { - return err - } - if owner.NextVersion == math.MaxInt64 { - return ErrInvalidInput - } - result, err = s.saveSkillVersion(ctx, q, tenant, id, owner.NextVersion, metadata, archive) - if err != nil { - return err - } - if err := q.AdvanceSkillVersion(ctx, sqlc.AdvanceSkillVersionParams{TenantID: tenant, ID: id, MakeDefault: makeDefault, Name: metadata.Name, Description: metadata.Description}); err != nil { - return err - } - return auditpg.RecordWriteAudit(ctx, q, tenantID, "upload_version", "skill_version", result.ID, result.SkillID, - writeaudit.Resource{Type: "skill_version", ID: result.ID, ParentID: result.SkillID}) - }) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - return result, err -} - -func (s *Store) GetSkillVersion(ctx context.Context, tenantID, skillID, version string) (SkillVersion, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersion{}, err - } - number := skillPathVersion(version) - row, err := s.queries.GetSkillVersion(ctx, sqlc.GetSkillVersionParams{TenantID: tenant, SkillID: id, Version: number}) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - return skillVersionFromRow(row), err -} - -// ReadSkillVersion reads metadata and encrypted bytes from one authorized row. -func (s *Store) ReadSkillVersion(ctx context.Context, tenantID, skillID, version string) (SkillVersion, []byte, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersion{}, nil, err - } - number := skillPathVersion(version) - row, err := s.queries.ReadSkillVersion(ctx, sqlc.ReadSkillVersionParams{TenantID: tenant, SkillID: id, Version: number}) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - if err != nil { - return SkillVersion{}, nil, err - } - return s.openSkillVersion(row) -} - -func (s *Store) openSkillVersion(row sqlc.SkillVersion) (SkillVersion, []byte, error) { - body, err := s.credentialCipher.OpenSkill(row.Contents, skillBinding(row.TenantID, row.SkillID, row.ID, row.Version)) - if err != nil { - return SkillVersion{}, nil, err - } - metadata := agentskill.Metadata{Type: "inline", Name: row.Name, Description: row.Description} - if _, err := agentskill.Read(body, metadata); err != nil { - return SkillVersion{}, nil, ErrInvalidInput - } - result := skillVersionFromRow(sqlc.GetSkillVersionRow{ID: row.ID, TenantID: row.TenantID, SkillID: row.SkillID, Version: row.Version, Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt}) - return result, body, nil -} - -func (s *Store) DeleteSkillVersion(ctx context.Context, tenantID, skillID, version string) (SkillVersion, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersion{}, err - } - number := skillPathVersion(version) - var result SkillVersion - err = s.pooled.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { - q := s.queries.WithTx(tx) - owner, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: id}) - if err != nil { - return err - } - if owner.DefaultVersion == number { - // The default is deletable only as the sole remaining version. As on the - // hosted service, that deletes the Skill itself under this lock, through - // the DeleteSkill cascade; frozen Session installations are independent. - rows, err := q.ListSkillVersions(ctx, sqlc.ListSkillVersionsParams{TenantID: tenant, SkillID: id, PageLimit: 2}) - if err != nil { - return err - } - if len(rows) != 1 { - return ErrDefaultSkillVersion - } - result = skillVersionFromRow(sqlc.GetSkillVersionRow(rows[0])) - if _, err = q.DeleteSkill(ctx, sqlc.DeleteSkillParams{TenantID: tenant, ID: id}); err != nil { - return err - } - return auditpg.RecordWriteAudit(ctx, q, tenantID, "delete", "skill_version", result.ID, result.SkillID) - } - row, err := q.DeleteSkillVersion(ctx, sqlc.DeleteSkillVersionParams{TenantID: tenant, SkillID: id, Version: number}) - if err != nil { - return err - } - result = skillVersionFromRow(sqlc.GetSkillVersionRow(row)) - if owner.LatestVersion == number { - if err := q.RefreshLatestSkillVersion(ctx, sqlc.RefreshLatestSkillVersionParams{TenantID: tenant, ID: id}); err != nil { - return err - } - } - return auditpg.RecordWriteAudit(ctx, q, tenantID, "delete", "skill_version", result.ID, result.SkillID) - }) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - return result, err -} - -// ReadDefaultSkillVersion selects the pointer and immutable content in one read. -func (s *Store) ReadDefaultSkillVersion(ctx context.Context, tenantID, skillID string) (SkillVersion, []byte, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersion{}, nil, err - } - row, err := s.queries.ReadDefaultSkillVersion(ctx, sqlc.ReadDefaultSkillVersionParams{TenantID: tenant, ID: id}) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - if err != nil { - return SkillVersion{}, nil, err - } - return s.openSkillVersion(row) -} diff --git a/services/core/internal/store/skills.go b/services/core/internal/store/skills.go deleted file mode 100644 index 35ba7de4c..000000000 --- a/services/core/internal/store/skills.go +++ /dev/null @@ -1,189 +0,0 @@ -package store - -import ( - "context" - "errors" - "strconv" - "strings" - "time" - - "github.com/MiniMax-AI/OpenAgentCore/internal/agentskill" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/db/sqlc" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/persistence/postgres/auditpg" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/skills" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" - "github.com/google/uuid" - "github.com/jackc/pgx/v5" - "github.com/jackc/pgx/v5/pgtype" -) - -type Skill struct { - ID string - Name string - Description string - CreatedAt time.Time - DefaultVersion int64 - LatestVersion int64 -} - -type SkillVersion struct { - ID string - SkillID string - Version int64 - Name string - Description string - CreatedAt time.Time -} - -func (s *Store) CreateSkill(ctx context.Context, tenantID string, archive []byte) (Skill, error) { - tenant, err := parseID(tenantID) - if err != nil { - return Skill{}, err - } - metadata, err := agentskill.Inspect(archive) - if err != nil { - return Skill{}, ErrInvalidInput - } - var result Skill - err = s.pooled.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { - q := s.queries.WithTx(tx) - id := pgtype.UUID{Bytes: uuid.New(), Valid: true} - row, err := q.CreateSkill(ctx, sqlc.CreateSkillParams{ID: id, TenantID: tenant, Name: metadata.Name, Description: metadata.Description}) - if err != nil { - return err - } - initial, err := s.saveSkillVersion(ctx, q, tenant, id, 1, metadata, archive) - if err != nil { - return err - } - result = skillFromRow(row) - return auditpg.RecordWriteAudit(ctx, q, tenantID, "create", "skill", result.ID, "", - writeaudit.Resource{Type: "skill", ID: result.ID}, - writeaudit.Resource{Type: "skill_version", ID: initial.ID, ParentID: result.ID}) - }) - return result, err -} - -func (s *Store) GetSkill(ctx context.Context, tenantID, skillID string) (Skill, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return Skill{}, err - } - row, err := s.queries.GetSkill(ctx, sqlc.GetSkillParams{TenantID: tenant, ID: id}) - if errors.Is(err, pgx.ErrNoRows) { - return Skill{}, ErrNotFound - } - return skillFromRow(row), err -} - -func (s *Store) UpdateSkillDefault(ctx context.Context, tenantID, skillID, version string) (Skill, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return Skill{}, err - } - number, err := skills.ParseVersion(version) - if err != nil { - return Skill{}, ErrInvalidInput - } - var result Skill - err = s.pooled.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { - q := s.queries.WithTx(tx) - if _, err := q.LockSkill(ctx, sqlc.LockSkillParams{TenantID: tenant, ID: id}); err != nil { - return err - } - version, err := q.GetSkillVersion(ctx, sqlc.GetSkillVersionParams{TenantID: tenant, SkillID: id, Version: number}) - if err != nil { - return err - } - row, err := q.SetDefaultSkillVersion(ctx, sqlc.SetDefaultSkillVersionParams{TenantID: tenant, ID: id, DefaultVersion: number, Name: version.Name, Description: version.Description}) - if err != nil { - return err - } - result = skillFromRow(row) - return auditpg.RecordWriteAudit(ctx, q, tenantID, "update_default_version", "skill", result.ID, "") - }) - if errors.Is(err, pgx.ErrNoRows) { - err = ErrNotFound - } - return result, err -} - -func (s *Store) DeleteSkill(ctx context.Context, tenantID, skillID string) error { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return err - } - err = s.pooled.Transaction(ctx, func(ctx context.Context, tx pgx.Tx) error { - q := s.queries.WithTx(tx) - if _, err := q.DeleteSkill(ctx, sqlc.DeleteSkillParams{TenantID: tenant, ID: id}); err != nil { - return err - } - return auditpg.RecordWriteAudit(ctx, q, tenantID, "delete", "skill", skillID, "") - }) - if errors.Is(err, pgx.ErrNoRows) { - return ErrNotFound - } - return err -} - -func (s *Store) saveSkillVersion(ctx context.Context, q *sqlc.Queries, tenant, skill pgtype.UUID, version int64, metadata agentskill.Metadata, archive []byte) (SkillVersion, error) { - id := pgtype.UUID{Bytes: uuid.New(), Valid: true} - body, err := s.credentialCipher.SealSkill(archive, skillBinding(tenant, skill, id, version)) - if err != nil { - return SkillVersion{}, err - } - row, err := q.CreateSkillVersion(ctx, sqlc.CreateSkillVersionParams{ID: id, TenantID: tenant, SkillID: skill, Version: version, Name: metadata.Name, Description: metadata.Description, Contents: body}) - return skillVersionFromRow(sqlc.GetSkillVersionRow(row)), err -} - -func skillBinding(tenant, skill, versionID pgtype.UUID, version int64) credentialcrypto.SkillBinding { - return credentialcrypto.SkillBinding{TenantID: uuid.UUID(tenant.Bytes).String(), SkillID: uuid.UUID(skill.Bytes).String(), VersionID: uuid.UUID(versionID.Bytes).String(), Version: strconv.FormatInt(version, 10)} -} - -func skillIDs(tenantID, skillID string) (pgtype.UUID, pgtype.UUID, error) { - tenant, err := parseID(tenantID) - if err != nil { - return tenant, pgtype.UUID{}, err - } - id, err := skillResourceID(skillID, "skill_") - return tenant, id, err -} - -func skillResourceID(value, prefix string) (pgtype.UUID, error) { - id, err := uuid.Parse(strings.TrimPrefix(value, prefix)) - if err != nil || id == uuid.Nil || value != prefix+id.String() { - return pgtype.UUID{}, ErrNotFound - } - return pgtype.UUID{Bytes: id, Valid: true}, nil -} - -// skillPathIDs resolves a Skill path identifier. Like pgunit.PathID, a malformed -// value resolves to an identifier that never exists, so the request follows the -// missing-Skill path. Request-body references keep skillIDs. -func skillPathIDs(tenantID, skillID string) (pgtype.UUID, pgtype.UUID, error) { - tenant, id, err := skillIDs(tenantID, skillID) - if errors.Is(err, ErrNotFound) { - return tenant, pgtype.UUID{Bytes: uuid.Max, Valid: true}, nil - } - return tenant, id, err -} - -// skillPathVersion resolves a version path segment. Versions start at 1, so a -// malformed segment resolves to the never-assigned version 0 and follows the -// missing-version path. Request-body selectors keep skills.ParseVersion. -func skillPathVersion(value string) int64 { - number, err := skills.ParseVersion(value) - if err != nil { - return 0 - } - return number -} - -func skillFromRow(row sqlc.Skill) Skill { - return Skill{ID: "skill_" + uuid.UUID(row.ID.Bytes).String(), Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt.Time, DefaultVersion: row.DefaultVersion, LatestVersion: row.LatestVersion} -} - -func skillVersionFromRow(row sqlc.GetSkillVersionRow) SkillVersion { - return SkillVersion{ID: "skillver_" + uuid.UUID(row.ID.Bytes).String(), SkillID: "skill_" + uuid.UUID(row.SkillID.Bytes).String(), Version: row.Version, Name: row.Name, Description: row.Description, CreatedAt: row.CreatedAt.Time} -} diff --git a/services/core/internal/store/skills_list.go b/services/core/internal/store/skills_list.go deleted file mode 100644 index 54a7cc11a..000000000 --- a/services/core/internal/store/skills_list.go +++ /dev/null @@ -1,107 +0,0 @@ -package store - -import ( - "context" - "errors" - "strings" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/db/sqlc" - "github.com/jackc/pgx/v5" - "github.com/jackc/pgx/v5/pgtype" -) - -type SkillPage struct { - Skills []Skill - HasMore bool -} - -type SkillVersionPage struct { - Versions []SkillVersion - HasMore bool -} - -// ListSkills and ListSkillVersions accept limit 0: the page is empty and HasMore -// reports whether any resource follows the cursor. -func (s *Store) ListSkills(ctx context.Context, tenantID, after string, limit int, ascending bool) (SkillPage, error) { - tenant, err := parseID(tenantID) - if err != nil { - return SkillPage{}, err - } - if limit < 0 || limit > 100 { - return SkillPage{}, ErrInvalidInput - } - params := sqlc.ListSkillsParams{TenantID: tenant, PageLimit: int32(limit + 1), Ascending: ascending, AfterID: pgtype.UUID{Valid: true}} - if after != "" { - cursor, err := s.GetSkill(ctx, tenantID, after) - if err != nil { - return SkillPage{}, err - } - params.AfterCreated = pgtype.Timestamptz{Time: cursor.CreatedAt, Valid: true} - params.AfterID, _ = skillResourceID(cursor.ID, "skill_") - } - rows, err := s.queries.ListSkills(ctx, params) - if err != nil { - return SkillPage{}, err - } - page := SkillPage{Skills: make([]Skill, 0, min(limit, len(rows))), HasMore: len(rows) > limit} - if page.HasMore { - rows = rows[:limit] - } - for _, row := range rows { - page.Skills = append(page.Skills, skillFromRow(row)) - } - return page, nil -} - -func (s *Store) ListSkillVersions(ctx context.Context, tenantID, skillID, after string, limit int, ascending bool) (SkillVersionPage, error) { - tenant, id, err := skillPathIDs(tenantID, skillID) - if err != nil { - return SkillVersionPage{}, err - } - if limit < 0 || limit > 100 { - return SkillVersionPage{}, ErrInvalidInput - } - if _, err := s.GetSkill(ctx, tenantID, skillID); err != nil { - return SkillVersionPage{}, err - } - params := sqlc.ListSkillVersionsParams{TenantID: tenant, SkillID: id, PageLimit: int32(limit + 1), Ascending: ascending} - if after != "" { - // A value that is not a version resource ID is invalid; a well-formed - // version missing from this tenant, including another tenant's, is not - // found; another Skill's version in this tenant does not match. - if !strings.HasPrefix(after, "skillver") { - return SkillVersionPage{}, skillVersionCursorPrefix(after) - } - cursorID, err := skillResourceID(after, "skillver_") - if err != nil { - return SkillVersionPage{}, err - } - cursor, err := s.queries.GetSkillVersionByID(ctx, sqlc.GetSkillVersionByIDParams{TenantID: tenant, ID: cursorID}) - if errors.Is(err, pgx.ErrNoRows) { - return SkillVersionPage{}, ErrNotFound - } - if err != nil { - return SkillVersionPage{}, err - } - if cursor.SkillID != id { - // As for Artifacts, a Skill deleted since its lookup stays not found. - if _, err := s.GetSkill(ctx, tenantID, skillID); err != nil { - return SkillVersionPage{}, err - } - return SkillVersionPage{}, errSkillVersionCursorParent - } - params.AfterVersion = pgtype.Int8{Int64: cursor.Version, Valid: true} - } - rows, err := s.queries.ListSkillVersions(ctx, params) - if err != nil { - return SkillVersionPage{}, err - } - page := SkillVersionPage{Versions: make([]SkillVersion, 0, min(limit, len(rows))), HasMore: len(rows) > limit} - if page.HasMore { - rows = rows[:limit] - } - for _, row := range rows { - page.Versions = append(page.Versions, skillVersionFromRow(sqlc.GetSkillVersionRow(row))) - } - return page, nil -} diff --git a/services/core/internal/store/skills_list_test.go b/services/core/internal/store/skills_list_test.go deleted file mode 100644 index 311c4e739..000000000 --- a/services/core/internal/store/skills_list_test.go +++ /dev/null @@ -1,66 +0,0 @@ -package store - -import ( - "bytes" - "errors" - "testing" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" - "github.com/google/uuid" -) - -func TestSkillListsAcceptLimitZero(t *testing.T) { - _, pool := testStore(t) - cipher, err := credentialcrypto.New(bytes.Repeat([]byte{43}, 32)) - if err != nil { - t.Fatal(err) - } - s := NewWithCredentialCipher(pool, cipher) - tenant, foreign := uuid.NewString(), uuid.NewString() - archive := skillArchive(t, "limit-zero") - created, err := s.CreateSkill(t.Context(), tenant, archive) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, created.ID) }) - if _, err = s.CreateSkillVersion(t.Context(), tenant, created.ID, archive, false); err != nil { - t.Fatal(err) - } - - // A zero page is empty; HasMore reports whether a resource follows the cursor. - for _, test := range []struct { - tenant, after string - hasMore bool - }{{tenant, "", true}, {tenant, created.ID, false}, {foreign, "", false}} { - page, err := s.ListSkills(t.Context(), test.tenant, test.after, 0, true) - if err != nil || len(page.Skills) != 0 || page.HasMore != test.hasMore { - t.Fatalf("zero Skill page after %q: %+v %v", test.after, page, err) - } - } - versions, err := s.ListSkillVersions(t.Context(), tenant, created.ID, "", 100, true) - if err != nil || len(versions.Versions) != 2 { - t.Fatal("versions", versions, err) - } - for after, hasMore := range map[string]bool{"": true, versions.Versions[0].ID: true, versions.Versions[1].ID: false} { - page, err := s.ListSkillVersions(t.Context(), tenant, created.ID, after, 0, true) - if err != nil || len(page.Versions) != 0 || page.HasMore != hasMore { - t.Fatalf("zero version page after %q: %+v %v", after, page, err) - } - } - - // Foreign cursors and parents stay indistinguishable from missing ones. - if _, err = s.ListSkills(t.Context(), foreign, created.ID, 0, true); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign cursor", err) - } - if _, err = s.ListSkillVersions(t.Context(), foreign, created.ID, "", 0, true); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign versions", err) - } - for _, limit := range []int{-1, 101} { - if _, err = s.ListSkills(t.Context(), tenant, "", limit, true); !errors.Is(err, ErrInvalidInput) { - t.Fatal("Skill limit", limit, err) - } - if _, err = s.ListSkillVersions(t.Context(), tenant, created.ID, "", limit, true); !errors.Is(err, ErrInvalidInput) { - t.Fatal("version limit", limit, err) - } - } -} diff --git a/services/core/internal/store/skills_test.go b/services/core/internal/store/skills_test.go deleted file mode 100644 index 58b520e2c..000000000 --- a/services/core/internal/store/skills_test.go +++ /dev/null @@ -1,166 +0,0 @@ -package store - -import ( - "archive/zip" - "bytes" - "errors" - "fmt" - "sort" - "strconv" - "sync" - "testing" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" - "github.com/google/uuid" -) - -func skillArchive(t *testing.T, marker string) []byte { - t.Helper() - var buffer bytes.Buffer - writer := zip.NewWriter(&buffer) - file, err := writer.CreateHeader(&zip.FileHeader{Name: "proof/SKILL.md", Method: zip.Store}) - if err != nil { - t.Fatal(err) - } - if _, err = fmt.Fprintf(file, "---\nname: proof\ndescription: Verify a versioned Skill.\n---\n%s", marker); err != nil { - t.Fatal(err) - } - if err = writer.Close(); err != nil { - t.Fatal(err) - } - return buffer.Bytes() -} - -func TestSkillsOwnershipEncryptionAndVersions(t *testing.T) { - _, pool := testStore(t) - cipher, err := credentialcrypto.New(bytes.Repeat([]byte{41}, 32)) - if err != nil { - t.Fatal(err) - } - s := NewWithCredentialCipher(pool, cipher) - tenant, foreign := uuid.NewString(), uuid.NewString() - archive := skillArchive(t, "confidential-skill-canary") - created, err := s.CreateSkill(t.Context(), tenant, archive) - if err != nil { - t.Fatal(err) - } - if created.DefaultVersion != 1 || created.LatestVersion != 1 { - t.Fatal("initial pointers", created) - } - t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, created.ID) }) - metadata, err := New(pool).GetSkill(t.Context(), tenant, created.ID) - if err != nil || metadata.Name != "proof" { - t.Fatal("metadata requires no content key", err) - } - var contents []byte - if err = pool.QueryRow(t.Context(), "SELECT contents FROM skill_versions WHERE tenant_id=$1", tenant).Scan(&contents); err != nil { - t.Fatal(err) - } - if bytes.Contains(contents, []byte("confidential-skill-canary")) { - t.Fatal("plaintext bundle persisted") - } - version, body, err := s.ReadSkillVersion(t.Context(), tenant, created.ID, "1") - if err != nil || !bytes.Equal(body, archive) || version.Version != 1 { - t.Fatal("content round trip", err) - } - if _, err = s.GetSkill(t.Context(), foreign, created.ID); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign metadata", err) - } - if _, _, err = s.ReadSkillVersion(t.Context(), foreign, created.ID, "1"); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign content", err) - } - if _, err = s.CreateSkillVersion(t.Context(), foreign, created.ID, archive, true); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign version", err) - } - if _, err = s.UpdateSkillDefault(t.Context(), foreign, created.ID, "1"); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign pointer", err) - } - if err = s.DeleteSkill(t.Context(), foreign, created.ID); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign delete", err) - } - if _, err = s.ListSkills(t.Context(), foreign, created.ID, 20, false); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign cursor", err) - } - if _, err = s.ListSkillVersions(t.Context(), foreign, created.ID, "", 20, false); !errors.Is(err, ErrNotFound) { - t.Fatal("foreign versions", err) - } - - const count = 8 - results := make(chan SkillVersion, count) - failures := make(chan error, count) - var group sync.WaitGroup - for range count { - group.Add(1) - go func() { - defer group.Done() - version, err := s.CreateSkillVersion(t.Context(), tenant, created.ID, archive, false) - if err != nil { - failures <- err - } else { - results <- version - } - }() - } - group.Wait() - close(results) - close(failures) - for err := range failures { - t.Fatal(err) - } - numbers := []int{} - for version := range results { - numbers = append(numbers, int(version.Version)) - } - sort.Ints(numbers) - for i, number := range numbers { - if number != i+2 { - t.Fatal("concurrent version allocation", numbers) - } - } - if len(numbers) != count { - t.Fatal("missing versions", numbers) - } - current, err := s.GetSkill(t.Context(), tenant, created.ID) - if err != nil || current.DefaultVersion != 1 || current.LatestVersion != count+1 { - t.Fatal("concurrent pointers", current, err) - } - first, err := s.ListSkillVersions(t.Context(), tenant, created.ID, "", 3, true) - if err != nil || !first.HasMore || len(first.Versions) != 3 || first.Versions[0].Version != 1 { - t.Fatal("first page", first, err) - } - next, err := s.ListSkillVersions(t.Context(), tenant, created.ID, first.Versions[2].ID, 20, true) - if err != nil || next.HasMore || len(next.Versions) != 6 || next.Versions[0].Version != 4 { - t.Fatal("version resource cursor", next, err) - } - var cursorErr *InvalidCursorError - if _, err = s.ListSkillVersions(t.Context(), tenant, created.ID, "3", 20, 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 = s.UpdateSkillDefault(t.Context(), tenant, created.ID, "999"); !errors.Is(err, ErrNotFound) { - t.Fatal("missing default", err) - } - updated, err := s.UpdateSkillDefault(t.Context(), tenant, created.ID, "3") - if err != nil || updated.DefaultVersion != 3 { - t.Fatal("default update", err) - } - if _, err = s.DeleteSkillVersion(t.Context(), tenant, created.ID, "3"); !errors.Is(err, ErrDefaultSkillVersion) { - t.Fatal("default deletion", err) - } - if _, err = s.DeleteSkillVersion(t.Context(), tenant, created.ID, strconv.Itoa(count+1)); err != nil { - t.Fatal(err) - } - added, err := s.CreateSkillVersion(t.Context(), tenant, created.ID, archive, true) - if err != nil || added.Version != count+2 { - t.Fatal("deleted version number reused", added, err) - } - if err = s.DeleteSkill(t.Context(), tenant, created.ID); err != nil { - t.Fatal(err) - } - if _, _, err = s.ReadSkillVersion(t.Context(), tenant, created.ID, "1"); !errors.Is(err, ErrNotFound) { - t.Fatal("cascaded content", err) - } - var remaining int - if err = pool.QueryRow(t.Context(), "SELECT count(*) FROM skill_versions WHERE tenant_id=$1", tenant).Scan(&remaining); err != nil || remaining != 0 { - t.Fatal("orphan content", remaining, err) - } -} diff --git a/services/core/internal/store/write_audit_resources_test.go b/services/core/internal/store/write_audit_resources_test.go deleted file mode 100644 index 7469c6212..000000000 --- a/services/core/internal/store/write_audit_resources_test.go +++ /dev/null @@ -1,150 +0,0 @@ -package store - -import ( - "bytes" - "context" - "reflect" - "strings" - "testing" - - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/writeaudit" - "github.com/google/uuid" - "github.com/jackc/pgx/v5" -) - -type resourceAuditMutation struct { - action, kind, parent string - owners int - run func(context.Context) (string, error) -} - -func resourceAuditContext(ctx context.Context, tenant, request string) context.Context { - return writeaudit.WithSource(ctx, writeaudit.Source{ - KeyID: "static:" + strings.Repeat("a", 64), Name: "resource audit fixture", Prefix: "aaaaaaaa", - Kind: "static", TenantID: tenant, RequestID: request, TraceID: "resource-audit-trace", - }) -} - -// A database trigger fails the final audit insertion after each real business -// mutation. Comparing complete tenant rows proves rollback of secret ciphertext, -// version counters, timestamps, and cascades. -func TestWriteAuditStandaloneResourceTransactions(t *testing.T) { - _, pool := newManagedTestStore(t) - cipher, err := credentialcrypto.New(bytes.Repeat([]byte{91}, 32)) - if err != nil { - t.Fatal(err) - } - s := NewWithCredentialCipher(pool, cipher) - ctx := t.Context() - _, err = pool.Exec(ctx, `CREATE FUNCTION reject_resource_audit_fixture() RETURNS trigger LANGUAGE plpgsql AS $$ - BEGIN IF NEW.request_id = 'reject-resource-audit' THEN RAISE EXCEPTION 'forced audit insertion failure'; END IF; RETURN NEW; END $$; - CREATE TRIGGER reject_resource_audit_fixture BEFORE INSERT ON write_audit_operations FOR EACH ROW EXECUTE FUNCTION reject_resource_audit_fixture()`) - if err != nil { - t.Fatal(err) - } - archive := skillArchive(t, "audit-private-archive") - for _, name := range []string{ - "skill_create", "skill_upload_version", "skill_update_default", "skill_delete", "version_delete", "version_delete_last", - } { - t.Run(name, func(t *testing.T) { - tenant := uuid.NewString() - mutation := prepareResourceAuditMutation(t, s, tenant, name, archive) - snapshot := func() map[string]string { - result := make(map[string]string) - for _, table := range []string{"agents", "skills", "skill_versions", "write_audit_operations", "write_audit_owners"} { - query := "SELECT COALESCE(jsonb_agg(to_jsonb(r) ORDER BY to_jsonb(r)::text)::text, '[]') FROM " + pgx.Identifier{table}.Sanitize() + " r WHERE tenant_id=$1" - var value string - if err := pool.QueryRow(ctx, query, tenant).Scan(&value); err != nil { - t.Fatalf("snapshot %s: %v", table, err) - } - result[table] = value - } - return result - } - before := snapshot() - if _, err := mutation.run(resourceAuditContext(ctx, tenant, "reject-resource-audit")); err == nil { - t.Fatal("audit failure was accepted") - } - if !reflect.DeepEqual(before, snapshot()) { - t.Fatal("audit failure left business or audit changes") - } - request := uuid.NewString() - id, err := mutation.run(resourceAuditContext(ctx, tenant, request)) - if err != nil { - t.Fatal(err) - } - var action, kind, gotID string - var parent *string - if err := pool.QueryRow(ctx, `SELECT action,resource_type,resource_id,parent_id FROM write_audit_operations WHERE tenant_id=$1 AND request_id=$2`, tenant, request).Scan(&action, &kind, &gotID, &parent); err != nil { - t.Fatal(err) - } - gotParent := "" - if parent != nil { - gotParent = *parent - } - if action != mutation.action || kind != mutation.kind || gotID != id || gotParent != mutation.parent { - t.Fatalf("wrong operation identity: %s %s %s %s", action, kind, gotID, gotParent) - } - var owners int - if err := pool.QueryRow(ctx, `SELECT count(*) FROM write_audit_owners WHERE tenant_id=$1`, tenant).Scan(&owners); err != nil || owners != mutation.owners { - t.Fatalf("ownership count %d, want %d: %v", owners, mutation.owners, err) - } - rows := snapshot() - for _, table := range []string{"write_audit_operations", "write_audit_owners"} { - for _, secret := range []string{"audit-private-archive", "audit-private-token", "audit-private-replacement"} { - if strings.Contains(rows[table], secret) { - t.Fatal("audit contains secret") - } - } - } - }) - } -} - -func prepareResourceAuditMutation(t *testing.T, s *Store, tenant, name string, archive []byte) resourceAuditMutation { - t.Helper() - ctx := t.Context() - must := func(err error) { - t.Helper() - if err != nil { - t.Fatal(err) - } - } - if strings.HasPrefix(name, "skill_") || strings.HasPrefix(name, "version_") { - if name == "skill_create" { - return resourceAuditMutation{action: "create", kind: "skill", owners: 2, run: func(ctx context.Context) (string, error) { v, e := s.CreateSkill(ctx, tenant, archive); return v.ID, e }} - } - v, err := s.CreateSkill(ctx, tenant, archive) - must(err) - if name == "skill_upload_version" { - return resourceAuditMutation{action: "upload_version", kind: "skill_version", parent: v.ID, owners: 1, run: func(ctx context.Context) (string, error) { - v, e := s.CreateSkillVersion(ctx, tenant, v.ID, archive, true) - return v.ID, e - }} - } - if name == "skill_delete" { - return resourceAuditMutation{action: "delete", kind: "skill", run: func(ctx context.Context) (string, error) { return v.ID, s.DeleteSkill(ctx, tenant, v.ID) }} - } - if name == "version_delete_last" { - return resourceAuditMutation{action: "delete", kind: "skill_version", parent: v.ID, run: func(ctx context.Context) (string, error) { - v, e := s.DeleteSkillVersion(ctx, tenant, v.ID, "1") - return v.ID, e - }} - } - _, err = s.CreateSkillVersion(ctx, tenant, v.ID, archive, false) - must(err) - if name == "skill_update_default" { - return resourceAuditMutation{action: "update_default_version", kind: "skill", run: func(ctx context.Context) (string, error) { - v, e := s.UpdateSkillDefault(ctx, tenant, v.ID, "2") - return v.ID, e - }} - } - return resourceAuditMutation{action: "delete", kind: "skill_version", parent: v.ID, run: func(ctx context.Context) (string, error) { - v, e := s.DeleteSkillVersion(ctx, tenant, v.ID, "2") - return v.ID, e - }} - } - t.Fatal("unknown resource audit mutation", name) - return resourceAuditMutation{} -}