Fix (review): seat picker must surface the skill the run resolves

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 <noreply@anthropic.com>
This commit is contained in:
soroush.asadi
2026-06-13 18:30:12 +03:30
parent 2ebe2808be
commit 428eae9643
2 changed files with 20 additions and 4 deletions
@@ -95,10 +95,16 @@ internal static class SkillsEndpoints
query = query.Where(s => s.Visibility == vis); query = query.Where(s => s.Visibility == vis);
} }
var skills = await query // Order so the FIRST row per key is exactly the one the run-time catalog resolves
.OrderBy(s => s.SkillKey) // (SkillCatalog.GetByKeysAsync): a Published row beats a Draft, the org's own beats the
.ThenByDescending(s => s.Version) // shared builtin, then the latest version — using the same Ordinal version comparison.
.ToListAsync(ct); // 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()); return Results.Ok(skills.Select(ToSummary).ToList());
} }
@@ -32,6 +32,8 @@ public sealed class SkillRunScopingTests(PostgresFixture postgres) : IClassFixtu
private sealed record SyncResult(int Indexed); private sealed record SyncResult(int Indexed);
private sealed record SkillSummary(string SkillKey, string Name, string Version, string Status);
private sealed record RunResponse( private sealed record RunResponse(
Guid Id, Guid SeatId, Guid WorkItemId, Guid? AgentId, string Status, Guid Id, Guid SeatId, Guid WorkItemId, Guid? AgentId, string Status,
string? ActionType, string? ActionRisk, string? Prompt, string? Output, string? Error); 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); Assert.Contains(OrgABody, done.Prompt);
// …and another org's same-key skill never leaked across the tenant boundary. // …and another org's same-key skill never leaked across the tenant boundary.
Assert.DoesNotContain(OrgBPoison, done.Prompt); 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<List<SkillSummary>>(
$"/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) private static async Task SeedSkillAsync(TeamUpWebFactory factory, Guid orgId, string key, string body)