From 428eae9643e34a0c92638113814fbcf23fee17fd Mon Sep 17 00:00:00 2001 From: "soroush.asadi" Date: Sat, 13 Jun 2026 18:30:12 +0330 Subject: [PATCH] Fix (review): seat picker must surface the skill the run resolves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found a display-vs-run mismatch: the seat picker collapses the library to the first row per key, but ListSkills ordered by version only — so for a key the org authored alongside a higher-versioned builtin, the picker showed/flagged the builtin while the run injected the org's own skill. ListSkills now orders the same way the run-time catalog resolves (Published-first, org-owned-over-builtin, then latest version with the same Ordinal comparison), computed in-memory so the version tiebreak can't diverge from SkillCatalog. The run itself was already correct; this aligns what the operator sees with what executes. No client change needed. SkillRunScopingTests now also asserts the library's first row for a key the org authored is the org-owned Published row, not the builtin. Verified: skills test subset 4/4 (full suite green pre-merge). Co-Authored-By: Claude Opus 4.8 --- .../Endpoints/SkillsEndpoints.cs | 14 ++++++++++---- .../SkillRunScopingTests.cs | 10 ++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Modules/TeamUp.Modules.Skills/Endpoints/SkillsEndpoints.cs b/src/Modules/TeamUp.Modules.Skills/Endpoints/SkillsEndpoints.cs index 18e2c7a..74fde84 100644 --- a/src/Modules/TeamUp.Modules.Skills/Endpoints/SkillsEndpoints.cs +++ b/src/Modules/TeamUp.Modules.Skills/Endpoints/SkillsEndpoints.cs @@ -95,10 +95,16 @@ internal static class SkillsEndpoints query = query.Where(s => s.Visibility == vis); } - var skills = await query - .OrderBy(s => s.SkillKey) - .ThenByDescending(s => s.Version) - .ToListAsync(ct); + // Order so the FIRST row per key is exactly the one the run-time catalog resolves + // (SkillCatalog.GetByKeysAsync): a Published row beats a Draft, the org's own beats the + // shared builtin, then the latest version — using the same Ordinal version comparison. + // The seat picker collapses to the first row per key, so display matches what will run. + var skills = (await query.ToListAsync(ct)) + .OrderBy(s => s.SkillKey, StringComparer.Ordinal) + .ThenByDescending(s => s.Status == SkillStatus.Published) + .ThenByDescending(s => s.OrganizationId == organizationId) + .ThenByDescending(s => s.Version, StringComparer.Ordinal) + .ToList(); return Results.Ok(skills.Select(ToSummary).ToList()); } diff --git a/tests/TeamUp.IntegrationTests/SkillRunScopingTests.cs b/tests/TeamUp.IntegrationTests/SkillRunScopingTests.cs index 8d75cbf..aa9cfe1 100644 --- a/tests/TeamUp.IntegrationTests/SkillRunScopingTests.cs +++ b/tests/TeamUp.IntegrationTests/SkillRunScopingTests.cs @@ -32,6 +32,8 @@ public sealed class SkillRunScopingTests(PostgresFixture postgres) : IClassFixtu private sealed record SyncResult(int Indexed); + private sealed record SkillSummary(string SkillKey, string Name, string Version, string Status); + private sealed record RunResponse( Guid Id, Guid SeatId, Guid WorkItemId, Guid? AgentId, string Status, string? ActionType, string? ActionRisk, string? Prompt, string? Output, string? Error); @@ -125,6 +127,14 @@ public sealed class SkillRunScopingTests(PostgresFixture postgres) : IClassFixtu Assert.Contains(OrgABody, done.Prompt); // …and another org's same-key skill never leaked across the tenant boundary. Assert.DoesNotContain(OrgBPoison, done.Prompt); + + // The seat picker collapses to the first row per key — that row must be the one the run + // resolved (the org's own, not the builtin), so seat configuration matches execution. + var library = await client.GetFromJsonAsync>( + $"/api/skills/?organizationId={owner.OrganizationId}"); + var firstImpl = library!.First(s => s.SkillKey == "code-implementation"); + Assert.Equal("ACME Code Impl", firstImpl.Name); + Assert.Equal("Published", firstImpl.Status); } private static async Task SeedSkillAsync(TeamUpWebFactory factory, Guid orgId, string key, string body)