diff --git a/contracts/agents-api/environment-templates.md b/contracts/agents-api/environment-templates.md index fc72d7c2b..a61accbd5 100644 --- a/contracts/agents-api/environment-templates.md +++ b/contracts/agents-api/environment-templates.md @@ -906,5 +906,6 @@ published protocol maxima. [File resource qualification](file-resource-semantics records default-selected unversioned content and descriptive metadata, default deletion rejection with multiple versions, and nondefault latest pointer fallback. Core updates the default pointer and top-level name/description atomically, while -concrete version bytes and previously frozen Sessions remain immutable. Last-version -deletion, version-number reuse and complete errors/visibility timing remain gaps. +concrete version bytes and previously frozen Sessions remain immutable. Deleting the +last version deletes the Skill; version numbers are intentionally never reused. +Complete errors and visibility timing remain gaps. diff --git a/contracts/agents-api/file-resource-semantics.md b/contracts/agents-api/file-resource-semantics.md index 9683d9175..550dc7384 100644 --- a/contracts/agents-api/file-resource-semantics.md +++ b/contracts/agents-api/file-resource-semantics.md @@ -44,7 +44,8 @@ The Skill probe observed an acknowledged version deletion followed about two seconds later by a list and exact GET that still exposed that version, while the parent latest pointer had already changed. It stopped and cleaned up. Core does not emulate this inconsistent visibility. Fresh/reduced sole-version deletion was -not reached; the guide's default-versus-last-version precedence remains unresolved. +not reached in this probe; the later [sole-version deletion](#sole-version-deletion--september-23-2026) +batch records that observation. Upload `default:true` was not separately probed: updating descriptive metadata there is the same pointer-consistency rule, covered by Core tests rather than a new official wire claim. No newer schema or integer selector form was adopted. @@ -100,3 +101,47 @@ the neighboring test's initial-focus wait corrected that test synchronization; the final full Web gate passed without weakening assertions or changing credential business behavior. Private logs and original artifacts remain under the evidence root above. These results do not close the remaining protocol gaps. + +## Sole-version deletion — September 23, 2026 + +Evidence: campaign scan 1, `~/.parsar/remediation/20260923/campaign-scan-1/skills-files-templates/findings.json` +SFT-01 to SFT-04, with raw records in the adjacent `official-ledger.jsonl` (labels +`s1-*`, `s2-*`, `s3-*`). The scan owned three Skills and created no Sessions or +model calls. + +| # | Case | Official observation | Core rule | +| --- | --- | --- | --- | +| V1 | Delete the only remaining version, which is also the default | 200 `{"id": "skillver_…", "object": "skill.version.deleted", "deleted": true, "version": "1"}`; retrieve and versions.list then return 404 (SFT-01) | Same body; the Skill is deleted in the same transaction. The reduced case (delete v2, then v1 is the only version) applies the same rule; official evidence covers only a fresh single-version Skill | +| V2 | Delete the default while another version is visible | 400 invalid_request_error, invalid_value, param version (SFT-04) | Unchanged | +| V3 | Delete a nondefault or latest version | 200; latest falls back | Unchanged | +| V4 | Foreign or missing Skill or version | 404 | Unchanged, indistinguishable | + +`DeleteSkillVersion` keeps the owning Skill row lock. When the target is the +default, it deletes the Skill only if no other version row exists, through the +same cascade as `skills.delete`, so every encrypted version row is removed in the +same commit; otherwise the 400 remains. Uploads take the same lock: an upload +committed first makes the default undeletable, and a deletion committed first +makes the later upload return 404. Frozen Session installations keep their own +snapshot, and Templates keep their stored reference intent, exactly as after +`skills.delete`. No schema, query or numbering change is involved. + +Recorded decisions: + +- **SFT-02, number reuse: intentional difference.** After the latest nondefault + version 2 was deleted, the next official upload was numbered "2" again (one + sample, so max+1 and latest+1 are indistinguishable). Core keeps immutable, + monotonically increasing numbers because exact Template and Session selectors + reference numbers; reusing one could re-point a stored exact selector to other + bytes and make a frozen Session's concrete version ambiguous. A sole-version + deletion removes the Skill, so numbering never restarts within a Skill. +- **SFT-03, upstream anomaly: never emulated.** Deleting default version 1 about + four seconds after an acknowledged version 2 upload returned 200 and removed the + whole Skill, including version 2. The same request with version 2 visible + returned 400 (SFT-04). Core serializes both operations on the Skill row, so a + version deletion never removes an acknowledged upload. + +Core acceptance: real-PostgreSQL store tests prove atomic Skill removal without +orphaned version rows, unchanged frozen Session contents, creation retry and +Template intent, and both lock orders of a concurrent upload; a real HTTP test +covers V1 to V4 across two tenants, and `official_skills.py` checks V1 with the +pinned SDK and raw HTTP. None of these run a model. diff --git a/contracts/agents-api/list-query-semantics.md b/contracts/agents-api/list-query-semantics.md index 9ecde20ce..bf93e6351 100644 --- a/contracts/agents-api/list-query-semantics.md +++ b/contracts/agents-api/list-query-semantics.md @@ -167,7 +167,9 @@ unsampled inputs: These remain registered differences and are not changed here: repeated Files `purpose` values (SFT-18); unsampled overflowing limits; Skill sole-version -deletion and number reuse (SFT-01/02); Session deletion lifecycle (SES-29/30); +deletion and number reuse (SFT-01/02, since resolved or recorded in +[file resource semantics](file-resource-semantics.md#sole-version-deletion--september-23-2026)); +Session deletion lifecycle (SES-29/30); whitespace input (SES-01..04); Template network forms (SFT-21/22); and response defaults (VA-11, SES-23/25). Malformed path IDs (SES-28), metadata and name error fields (VA-07/08/09), U+0000 (VA-10) and Template network codes (SFT-20) are diff --git a/contracts/agents-api/official-semantics-alignment.md b/contracts/agents-api/official-semantics-alignment.md index 9e477bbbe..97f5bf551 100644 --- a/contracts/agents-api/official-semantics-alignment.md +++ b/contracts/agents-api/official-semantics-alignment.md @@ -189,7 +189,9 @@ Deferred and unchanged: accepting and storing U+0000; hostname forms accepted officially (SFT-21) and `disabled` with domains, which the official service accepts (SFT-22); non-canonical UUID spellings such as uppercase, braces or `urn:uuid:` still resolve to the same resource; Skill sole-version deletion and -number reuse; Session deletion lifecycle; whitespace input; response defaults; +number reuse (since resolved or recorded in +[file resource semantics](file-resource-semantics.md#sole-version-deletion--september-23-2026)); +Session deletion lifecycle; whitespace input; response defaults; and the Files `limit=abc` code. The Environment Files list query parser is aligned for unknown and repeated keys by the [Environment Files wire batch](environment-files.md#wire-alignment--september-23-2026); it still rejects malformed query encoding locally. diff --git a/contracts/agents-api/openapi.yaml b/contracts/agents-api/openapi.yaml index 914bf7b12..9d9590d6a 100644 --- a/contracts/agents-api/openapi.yaml +++ b/contracts/agents-api/openapi.yaml @@ -5569,8 +5569,9 @@ paths: - Skills /skills/{skill_id}/versions/{version}: delete: - description: Rejects deletion of the current default version. Exact hosted last-version/default - deletion precedence is not verified. + description: Deleting the only remaining version also deletes the Skill; existing + Session installation snapshots remain independent. The default version cannot + be deleted while other versions remain. Version numbers are never reused. parameters: - description: Skill ID in: path diff --git a/contracts/agents-api/operation-evidence.md b/contracts/agents-api/operation-evidence.md index eb31f96f5..abf293713 100644 --- a/contracts/agents-api/operation-evidence.md +++ b/contracts/agents-api/operation-evidence.md @@ -99,19 +99,19 @@ Paths in the appendix include `/v1`. SDK names here omit `client.`. `P` means pa | 49 | skills.retrieve | P: safe metadata follows default version | E missing-ID404; W distinct names/descriptions follow default1→2→1 | Skill store and joint resource acceptance | Other error/visibility behavior remains unqualified | | 50 | skills.update | P: atomic default pointer and descriptive metadata update | V default mutation/frozen Sessions; W default1→2→1 name/description changes | Skill store and joint resource acceptance; retained frozen Session regression | Complete errors and concurrent official behavior | | 51 | skills.list | P: scoped resource list; limit 0 empty page with has_more, 0–100 with observed range and duplicate codes | L SFT-08/09/10/11 | K recorded DB resources; L DB tenant A/B | Non-integer and overflowing limits unsampled | -| 52 | skills.delete | P: remove owned source, retain committed Session content | None located | K recorded DB and Live source deletion/continuation | Exact hosted deletion/idempotence/default/latest semantics | +| 52 | skills.delete | P: remove owned source and every encrypted version, retain committed Session content; sole-version deletion uses the same cascade | [sole-version deletion](file-resource-semantics.md#sole-version-deletion--september-23-2026) SFT-01 sole-version deletion then Skill 404; SFT-06 repeat delete 404; SFT-07 retrieve/version reads 404 | K recorded DB and Live source deletion/continuation; sole-version store, HTTP tenant A/B and pinned-SDK DB tests | Official 404 message names the Skill, Core keeps a generic message; physical erasure and concurrent hosted deletion unobserved | | 53 | skills.content.retrieve | P: unversioned content selects default | W distinct default1/latest2 bytes and default2 transition | K recorded DB plus joint resource acceptance | Headers/errors and source deletion/read races | | 54 | skills.versions.create | P: immutable increasing version; optional default change | V second version default=false preserves default1/latest2 | K recorded DB resources | Broader numbering/default/top-level metadata/error/null semantics; upload limits | | 55 | skills.versions.retrieve | P: owned immutable version metadata; malformed version path equals missing | V immediate version1 read404, bounded delayed read200 | K recorded DB resources and Live concrete Session freeze | Visibility timing is observational; full selector/metadata/error parity unqualified | | 56 | skills.versions.list | P: scoped version cursor list; limit 0 empty page with has_more | V delayed owned list contains created versions; L SFT-08/09 | K recorded DB resource checks; L DB zero page | Exact ordering/cursors/default and concurrent version mutation | -| 57 | skills.versions.delete | P: nondefault deletion; default rejects invalid_value/version | W two-version default400; latest200 with parent pointer fallback | Skill store and joint resource acceptance | Sole deletion/number reuse unverified; observed stale official version reads are not emulated | +| 57 | skills.versions.delete | P: nondefault deletion; default rejects invalid_value/version while other versions remain; deleting the only version deletes the Skill in the same locked transaction | W two-version default400; latest200 with parent pointer fallback; [sole-version deletion](file-resource-semantics.md#sole-version-deletion--september-23-2026) SFT-01 sole200 then Skill 404, SFT-04 default400 with visible v2 | Skill store (atomic removal, frozen Session, upload lock order), HTTP tenant A/B and joint resource acceptance | SFT-02 number reuse is an intentional difference (numbers stay immutable); SFT-03 whole-Skill removal and stale official version reads are not emulated | | 58 | skills.versions.content.retrieve | P: decrypt/read immutable concrete bundle | W v1/v2 ZIP members and markers | K recorded DB/live consumption; joint resource acceptance | Content headers/errors and source deletion/read races | ## Remaining gaps without task ordering 1. **Public generic semantics:** sampled create/event/envelope/error/no-op corrections are merged. L aligns unknown/repeated list keys, sampled limit bounds and single-resource unknown keys. X gives malformed path IDs on every Beta, Files and Skills route the exact missing-resource response, reports metadata/name field errors with official code and param, maps Template network rejections to `invalid_request_error`, and rejects U+0000 in stored strings as a documented local limit (the official service stores it). G aligns Environment Files list query tolerance and the sampled Files.create/list errors. Other resource-by-resource omissions/null/default/error params, overflowing limits, list caps, concurrent mutation and deletion require separate evidence. 2. **Session differences:** The Session admission batch removes idle `none` creation and empty metadata update. Local durable creation idempotency remains an explicit difference. Whitespace-only input succeeds officially but is rejected by the existing Core message validator; this newly observed difference is queued separately. Session agent updates, newer Environment shapes and root Item turn_id are baseline-upgrade questions. -3. **Template/Skill composition:** shared env/files/setup/packages selection is covered by the composition batch; template-reference null network/capability lists are covered by the null-selection batch. Official derived capability-directory projection remains different. Skill content/default metadata are covered by file-resource-semantics.md; sole-version deletion, visibility and broader numbering/error behavior remain unverified. +3. **Template/Skill composition:** shared env/files/setup/packages selection is covered by the composition batch; template-reference null network/capability lists are covered by the null-selection batch. Official derived capability-directory projection remains different. Skill content/default metadata and sole-version deletion are covered by file-resource-semantics.md; number reuse is an intentional difference; visibility and broader error behavior remain unverified. 4. **Execution coverage:** use T's qualified matrix, not a blanket missing-image/structured-output claim. MiniMax functions/service MCP, optional tool combinations, unsupported images/placements and broader native lifecycle are explicit restrictions. PTC omission retains approved native behavior; Claude/MiniMax public Usage remains null; child settlement cadence/native close limits remain visible. No second executor/model loop or guessed counters are justified. 5. **Workspace and resources:** live Files bounds, recursion and parent creation (G aligns the sampled envelope, empty pages and path errors), artifact capture edges for hard links/special files and cancellation (Y aligns output symlinks, republication and the list envelope), full Environment metadata/lifecycle and Vault archive/in-flight-token semantics remain partial or unknown. Retired Core-managed E2B acceptance cannot qualify current user enrollment. diff --git a/services/agents-api/internal/api/skills.go b/services/agents-api/internal/api/skills.go index a613f7029..3fdcbeea8 100644 --- a/services/agents-api/internal/api/skills.go +++ b/services/agents-api/internal/api/skills.go @@ -140,7 +140,7 @@ func (h *Handler) getSkillVersion(w http.ResponseWriter, r *http.Request) { } // @Summary Delete a Skill version -// @Description Rejects deletion of the current default version. Exact hosted last-version/default deletion precedence is not verified. +// @Description Deleting the only remaining version also deletes the Skill; existing Session installation snapshots remain independent. The default version cannot be deleted while other versions remain. Version numbers are never reused. // @Tags Skills // @Produce json // @Security BearerAuth diff --git a/services/agents-api/internal/store/skill_version_deletion_public_test.go b/services/agents-api/internal/store/skill_version_deletion_public_test.go new file mode 100644 index 000000000..598601c17 --- /dev/null +++ b/services/agents-api/internal/store/skill_version_deletion_public_test.go @@ -0,0 +1,120 @@ +package store_test + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + "github.com/MiniMax-AI-Dev/parsar/internal/agentdaemon/device" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/api" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/credentialcrypto" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/store" + "github.com/google/uuid" +) + +// TestSkillVersionDeletionHTTPPostgres covers skills.versions.delete over real +// HTTP and PostgreSQL: sole-version deletion (V1), default rejection while +// other versions remain (V2), nondefault latest deletion (V3) and tenant +// isolation (V4). +func TestSkillVersionDeletionHTTPPostgres(t *testing.T) { + _, pool := store.NewTestStore(t) + cipher, err := credentialcrypto.New(bytes.Repeat([]byte{64}, 32)) + if err != nil { + t.Fatal(err) + } + s := store.NewWithCredentialCipher(pool, cipher) + owner, foreign, ownerTenant := uuid.NewString(), uuid.NewString(), uuid.NewString() + auth, err := api.NewAuthenticator([]api.APIKey{ + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "skill-owner", TokenSHA256: device.HashCredential(owner), TenantID: ownerTenant}, + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "skill-foreign", TokenSHA256: device.HashCredential(foreign), TenantID: uuid.NewString()}, + }) + if err != nil { + t.Fatal(err) + } + h, err := api.NewHandler(s, auth, "codex", api.WithSkills(s)) + if err != nil { + t.Fatal(err) + } + server := httptest.NewServer(h) + defer server.Close() + client := pathIDClient{t: t, server: server} + object := func(raw string) map[string]any { + t.Helper() + var value map[string]any + if err := json.Unmarshal([]byte(raw), &value); err != nil { + t.Fatal(raw, err) + } + return value + } + expect := func(token, method, path string, status int) string { + t.Helper() + got, raw := client.do(token, method, path, "", nil) + if got != status { + t.Fatalf("%s %s: %d %s", method, path, got, raw) + } + return raw + } + 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")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), ownerTenant, 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. + if raw := expect(foreign, http.MethodDelete, path+"/versions/1", http.StatusNotFound); raw != missing { + t.Fatal("foreign response differs from missing", raw, missing) + } + expect(owner, http.MethodDelete, path+"/versions/2", http.StatusNotFound) + expect(owner, http.MethodGet, path, http.StatusOK) + + // V1: the observed official body, then the Skill is gone from every read. + deleted := object(expect(owner, http.MethodDelete, path+"/versions/1", http.StatusOK)) + if want := map[string]any{"id": version["id"], "object": "skill.version.deleted", "deleted": true, "version": "1"}; !reflect.DeepEqual(deleted, want) { + t.Fatal("sole-version deletion body", deleted) + } + for _, suffix := range []string{"", "/content", "/versions", "/versions/1", "/versions/1/content"} { + expect(owner, http.MethodGet, path+suffix, http.StatusNotFound) + } + if raw := expect(owner, http.MethodGet, "/v1/skills", http.StatusOK); strings.Contains(raw, sole.ID) { + t.Fatal("deleted Skill listed", raw) + } + expect(owner, http.MethodDelete, path+"/versions/1", http.StatusNotFound) + 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")) + 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.Fatal(err) + } + path = "/v1/skills/" + pair.ID + before := expect(owner, http.MethodGet, path+"/versions", http.StatusOK) + rejected := object(expect(owner, http.MethodDelete, path+"/versions/1", http.StatusBadRequest)) + if want := map[string]any{"error": map[string]any{"type": "invalid_request_error", "code": "invalid_value", "param": "version", "message": "Cannot delete the default skill version."}}; !reflect.DeepEqual(rejected, want) { + t.Fatal("default deletion with another version", rejected) + } + expect(foreign, http.MethodDelete, path+"/versions/2", http.StatusNotFound) + if after := expect(owner, http.MethodGet, path+"/versions", http.StatusOK); after != before { + t.Fatal("rejected deletions changed versions", after) + } + if latest := object(expect(owner, http.MethodDelete, path+"/versions/2", http.StatusOK)); latest["version"] != "2" || latest["deleted"] != true { + t.Fatal("latest deletion", latest) + } + if parent := object(expect(owner, http.MethodGet, path, http.StatusOK)); parent["default_version"] != "1" || parent["latest_version"] != "1" { + t.Fatal("latest pointer fallback", parent) + } + if reduced := object(expect(owner, http.MethodDelete, path+"/versions/1", http.StatusOK)); reduced["version"] != "1" || reduced["object"] != "skill.version.deleted" { + t.Fatal("reduced sole-version deletion", reduced) + } + expect(owner, http.MethodGet, path, http.StatusNotFound) +} diff --git a/services/agents-api/internal/store/skill_version_deletion_test.go b/services/agents-api/internal/store/skill_version_deletion_test.go new file mode 100644 index 000000000..c9c16f041 --- /dev/null +++ b/services/agents-api/internal/store/skill_version_deletion_test.go @@ -0,0 +1,198 @@ +package store + +import ( + "bytes" + "encoding/json" + "errors" + "reflect" + "testing" + "time" + + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/credentialcrypto" + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" +) + +func skillVersionDeletionStore(t *testing.T) (*Store, *pgxpool.Pool) { + t.Helper() + _, 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() + 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") + if err != nil { + t.Fatal(err) + } + reference := EnvironmentSetup{Skills: []EnvironmentSkill{{Metadata: EnvironmentSkillMetadata{Type: "skill_reference", SkillID: skill.ID}}}} + template, err := s.CreateEnvironmentTemplate(t.Context(), tenant, EnvironmentTemplateInput{SetSkills: true, Initialization: reference}) + if err != nil { + t.Fatal(err) + } + input := CreateSessionInput{Creator: FixtureCreator(), Engine: "codex", IdempotencyKey: uuid.NewString(), Configuration: json.RawMessage(`{"environment":{"type":"openai_hosted"}}`), Initialization: reference} + session, err := s.CreateSession(t.Context(), tenant, input) + if err != nil { + t.Fatal(err) + } + frozen, err := s.ReadEnvironmentSetup(t.Context(), tenant, session.ID) + 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) + } + + // Committed snapshots and Template intent 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) + } + retry, err := s.CreateSession(t.Context(), tenant, input) + if err != nil || retry.ID != session.ID { + t.Fatal("committed retry read the deleted source", err) + } + kept, err := s.GetEnvironmentTemplate(t.Context(), tenant, template.ID) + if err != nil || !reflect.DeepEqual(kept.Skills, template.Skills) || !kept.UpdatedAt.Equal(template.UpdatedAt) { + t.Fatal("Template reference intent changed", 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/agents-api/internal/store/skill_versions.go b/services/agents-api/internal/store/skill_versions.go index 2a5c6c145..c25555d24 100644 --- a/services/agents-api/internal/store/skill_versions.go +++ b/services/agents-api/internal/store/skill_versions.go @@ -10,6 +10,7 @@ import ( "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) { @@ -100,7 +101,19 @@ func (s *Store) DeleteSkillVersion(ctx context.Context, tenantID, skillID, versi return err } if owner.DefaultVersion == number { - return ErrDefaultSkillVersion + // 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])) + _, err = q.DeleteSkill(ctx, sqlc.DeleteSkillParams{TenantID: tenant, ID: id}) + return err } row, err := q.DeleteSkillVersion(ctx, sqlc.DeleteSkillVersionParams{TenantID: tenant, SkillID: id, Version: number}) if err != nil { diff --git a/services/agents-api/tests/official_skills.py b/services/agents-api/tests/official_skills.py index 157e09fcf..4cfe91c99 100644 --- a/services/agents-api/tests/official_skills.py +++ b/services/agents-api/tests/official_skills.py @@ -91,6 +91,25 @@ def main(): assert [item.id for item in client.skills.list()] == before deleted = client.skills.versions.delete(version="1", skill_id=skill.id) assert deleted.id == first.id and deleted.version == "1" and deleted.object == "skill.version.deleted" and deleted.deleted + # Deleting the only remaining version also deletes the Skill. + sole = client.skills.create(files=[("proof/SKILL.md", manifest, "text/markdown")]) + owned.append(sole.id) + sole_path = base + "/skills/" + sole.id + assert http.delete(sole_path + "/versions/1", headers=foreign).status_code == 404 + sole_version = client.skills.versions.retrieve(version="1", skill_id=sole.id) + removed = client.skills.versions.with_raw_response.delete(version="1", skill_id=sole.id) + assert removed.http_response.json() == {"id": sole_version.id, "object": "skill.version.deleted", "deleted": True, "version": "1"} + assert removed.parse().deleted + owned.remove(sole.id) + for suffix in ("", "/content", "/versions", "/versions/1"): + assert http.get(sole_path + suffix, headers=headers).status_code == 404 + try: + client.skills.retrieve(sole.id) + except openai.NotFoundError: + pass + else: + raise AssertionError("Skill survived deletion of its only version") + assert sole.id not in [item.id for item in client.skills.list()] assert client.skills.delete(directory.id).deleted owned.remove(directory.id) assert http.get(base + "/skills/" + directory.id + "/versions/1/content", headers=headers).status_code == 404