From 04093c4869595545bcc683d14ff926dd3fddca13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Wed, 8 Jul 2026 23:41:51 -0400 Subject: [PATCH] Harden the seams a project sweep surfaced A systematic review of the whole codebase (main process, renderer, IPC/migration/packaging seams) surfaced four real defects; each is fixed at its root: - Legacy catalogue drift: the operator's profile still carried 89 active skills from the removed ~/.agents import era next to the 228 built-ins. Catalogue sync now mirrors the bundle - actives whose slug the bundle does not ship retire (bindings keep resolving via their pinned revisions; retired skills leave list_skills and cannot be granted anew) - Broken-bundle guard: staging now refuses a bundle without the humanizer skill, so an empty or quarantined resources folder can never trigger the mirror pass into retiring a healthy catalogue - Duplicate agent names: @mentions, DM routing, and task assignment all resolve by name, so createAgent and updateAgent renames now reject a name already held by an active agent (case-insensitive); the mention resolver still refuses to guess between legacy duplicates - Stale DM titles: agent-to-agent conversation rows titled themselves from the frozen creation-time space name, so renames never reached the sidebar; direct titles now derive from members' current names Verified clean in the same sweep: IPC surface (82 channels, handler + preload + api aligned), per-migration transactions with rollback, quota-parking month rollover, retry-budget counting, meeting spot reuse across floor-plan rebuilds. --- src/main/index.ts | 4 +-- src/main/services/builtin-skills.ts | 19 +++++++++-- src/main/services/organization-service.ts | 19 ++++++++++- src/main/services/skill-service.ts | 28 +++++++++++++++ src/renderer/src/pages/chat/chat-helpers.ts | 26 +++++++++++--- tests/unit/builtin-skills.test.ts | 38 +++++++++++++++++++-- tests/unit/chat-governance.test.ts | 8 ++++- tests/unit/profile-identity.test.ts | 26 ++++++++++++++ 8 files changed, 156 insertions(+), 12 deletions(-) diff --git a/src/main/index.ts b/src/main/index.ts index f33f3d6b..ece4cd9b 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -100,8 +100,8 @@ function syncAllOrganizationSkills(connection: DatabaseConnection, artifactRoot: const orgs = connection.db.prepare("select id from organizations").all() as Array<{ id: string }>; for (const org of orgs) { const report = connection.transaction(() => importStagedSkills(skills, org.id, staged)); - if (report.imported > 0 || report.failures.length > 0) { - console.info(`[skills] org ${org.id}: imported ${report.imported}, skipped ${report.skipped}, failures ${report.failures.length}`); + if (report.imported > 0 || report.retired > 0 || report.failures.length > 0) { + console.info(`[skills] org ${org.id}: imported ${report.imported}, skipped ${report.skipped}, retired ${report.retired}, failures ${report.failures.length}`); for (const failure of report.failures.slice(0, 5)) console.warn(`[skills] ${failure}`); } } diff --git a/src/main/services/builtin-skills.ts b/src/main/services/builtin-skills.ts index 39647345..9f5bd51a 100644 --- a/src/main/services/builtin-skills.ts +++ b/src/main/services/builtin-skills.ts @@ -29,6 +29,8 @@ export interface StagedBuiltinSkill { export interface BuiltinSkillSyncReport { imported: number; skipped: number; + /** Active skills retired because the bundle no longer carries their slug. */ + retired: number; failures: string[]; } @@ -57,12 +59,24 @@ export function stageBuiltinSkillsLibrary(sourceRoot: string, libraryRoot: strin } staged.push(...stageCategorySkills(sourceDir, libraryRoot, seen)); } + // A bundle without the forced house-style skill is a broken install, not a + // small catalogue. Failing here keeps the import (and the mirror-retire + // pass) from wiping a healthy org catalogue against an empty directory. + if (!seen.has("humanizer")) { + throw new Error(`Built-in skills bundle at ${sourceRoot} is missing the humanizer skill - refusing to sync a broken bundle.`); + } return staged; } -/** Publishes every staged skill the org does not already carry. */ +/** + * Makes the org catalogue mirror the bundle: publishes every staged skill + * the org does not carry yet, then retires active skills whose slug the + * bundle no longer ships (legacy ~/.agents imports, removed skills). + * Existing bindings pin revisions directly and keep working; retired skills + * simply leave list_skills and stop being grantable. + */ export function importStagedSkills(skills: SkillService, orgId: string, staged: StagedBuiltinSkill[]): BuiltinSkillSyncReport { - const report: BuiltinSkillSyncReport = { imported: 0, skipped: 0, failures: [] }; + const report: BuiltinSkillSyncReport = { imported: 0, skipped: 0, retired: 0, failures: [] }; for (const unit of staged) { try { if (skills.hasSkill(orgId, unit.slug)) { @@ -75,6 +89,7 @@ export function importStagedSkills(skills: SkillService, orgId: string, staged: report.failures.push(`${unit.slug}: ${error instanceof Error ? error.message : String(error)}`); } } + report.retired = skills.retireSkillsNotIn(orgId, staged.map((unit) => unit.slug)).length; return report; } diff --git a/src/main/services/organization-service.ts b/src/main/services/organization-service.ts index 9ef8ff05..ad353c94 100644 --- a/src/main/services/organization-service.ts +++ b/src/main/services/organization-service.ts @@ -76,6 +76,7 @@ export class OrganizationService { createAgent(input: CreateAgentInput): string { assertUsableMentionName(input.name); + this.assertAgentNameAvailable(input.orgId, input.name, null); const hiringPolicy = this.governance.enforceHiringPolicy(input); const id = createId("agt"); const now = new Date().toISOString(); @@ -154,7 +155,10 @@ export class OrganizationService { if (!role) throw new Error("Agent role is required."); const name = input.name === undefined ? agent.name : input.name.trim(); if (!name) throw new Error("Agent name is required."); - if (name !== agent.name) assertUsableMentionName(name); + if (name !== agent.name) { + assertUsableMentionName(name); + this.assertAgentNameAvailable(input.orgId, name, input.agentId); + } if (input.modelProviderId) { const provider = this.db.prepare("select id from providers where id = ? and org_id = ?") .get(input.modelProviderId, input.orgId) as { id: string } | undefined; @@ -330,6 +334,19 @@ export class OrganizationService { if (goalId && project.goal_id !== goalId) throw new Error("Project is linked to a different goal."); } + /** + * Agent names are identities: @mentions, DM routing, and task assignment + * all resolve by name, so two active agents must never share one. + */ + private assertAgentNameAvailable(orgId: string, name: string, exceptAgentId: string | null): void { + const clash = this.db.prepare(` + select id from agents + where org_id = ? and lifecycle = 'active' and lower(name) = lower(?) and (? is null or id <> ?) + limit 1 + `).get(orgId, name.trim(), exceptAgentId, exceptAgentId) as { id: string } | undefined; + if (clash) throw new Error(`An active agent named "${name.trim()}" already exists. Pick a different name.`); + } + private addAgentToWorkspaceSpaces(orgId: string, agentId: string): void { const spaces = this.db.prepare("select id from chat_spaces where org_id = ? and visibility = ? and space_type <> ?") .all(orgId, "workspace", "direct") as Array<{ id: string }>; diff --git a/src/main/services/skill-service.ts b/src/main/services/skill-service.ts index 53442370..4165bb7e 100644 --- a/src/main/services/skill-service.ts +++ b/src/main/services/skill-service.ts @@ -48,6 +48,34 @@ export class SkillService { return Boolean(this.db.prepare("select id from skill_definitions where org_id = ? and slug = ?").get(orgId, slug)); } + /** + * Retires every active skill whose slug is not in `keepSlugs`, so the org + * catalogue mirrors the shipped bundle. Bindings pin revisions directly + * and keep resolving; retired skills leave list_skills and cannot be + * granted anew. Returns the retired slugs. + */ + retireSkillsNotIn(orgId: string, keepSlugs: string[]): string[] { + const keep = new Set(keepSlugs); + const active = this.db.prepare("select id, slug from skill_definitions where org_id = ? and status = 'active'") + .all(orgId) as Array<{ id: string; slug: string }>; + const retired: string[] = []; + for (const skill of active) { + if (keep.has(skill.slug)) continue; + this.db.prepare("update skill_definitions set status = 'retired' where id = ?").run(skill.id); + this.audit.record({ + orgId, + entityType: "skill_definition", + entityId: skill.id, + actorType: "system", + actorId: null, + action: "retired", + payload: { slug: skill.slug, reason: "not_in_builtin_bundle" }, + }); + retired.push(skill.slug); + } + return retired; + } + importManifest(orgId: string, directoryPath: string, manifest: SkillManifest, explicitSlug?: string): string { const slug = explicitSlug ?? slugify(manifest.name); if (!slug) throw new Error("Skill name could not be derived."); diff --git a/src/renderer/src/pages/chat/chat-helpers.ts b/src/renderer/src/pages/chat/chat-helpers.ts index fd7f650e..0bf8cf50 100644 --- a/src/renderer/src/pages/chat/chat-helpers.ts +++ b/src/renderer/src/pages/chat/chat-helpers.ts @@ -127,12 +127,16 @@ export function buildConversations(input: { const items = input.spaces.map((space) => { const last = lastBySpace.get(space.id) ?? null; const isDirect = space.spaceType === "direct"; - const counterpart = isDirect - ? resolveCounterpart(membersBySpace.get(space.id) ?? [], input) - : null; + const members = membersBySpace.get(space.id) ?? []; + const counterpart = isDirect ? resolveCounterpart(members, input) : null; + // Direct titles derive from the members' CURRENT names — the space name + // froze whatever they were called when the conversation was created, so + // renames would otherwise never reach the sidebar. + const liveNames = isDirect ? resolveMemberNames(members, input) : []; + const directTitle = counterpart?.name ?? (liveNames.length > 0 ? liveNames.join(" · ") : space.name); return { space, - title: isDirect && counterpart ? counterpart.name : space.name, + title: isDirect ? directTitle : space.name, isDirect, counterpart, preview: last ? `${senderName(last, context)}: ${previewText(last)}` : null, @@ -143,6 +147,20 @@ export function buildConversations(input: { return items.sort((a, b) => b.lastActivityAt.localeCompare(a.lastActivityAt)); } +/** Current display names of a space's members, in membership order. */ +function resolveMemberNames( + members: ChatMembershipRecord[], + input: { employees: EmployeeRecord[]; agents: AgentRecord[] }, +): string[] { + return members + .map((member) => { + if (member.principalType === "agent") return input.agents.find((item) => item.id === member.principalId)?.name ?? null; + if (member.principalType === "employee") return input.employees.find((item) => item.id === member.principalId)?.displayName ?? null; + return null; + }) + .filter((name): name is string => name !== null); +} + /** * The single other party of a direct conversation, from the viewer's seat. * Conversations between two agents have no single counterpart for the diff --git a/tests/unit/builtin-skills.test.ts b/tests/unit/builtin-skills.test.ts index 0959306a..70fc3f52 100644 --- a/tests/unit/builtin-skills.test.ts +++ b/tests/unit/builtin-skills.test.ts @@ -86,15 +86,49 @@ describe("built-in skills", () => { expect(humanizer?.manifest.description).toContain("AI-generated writing"); const first = h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, staged)); - expect(first).toMatchObject({ imported: 2, skipped: 0, failures: [] }); + expect(first).toMatchObject({ imported: 2, skipped: 0, retired: 0, failures: [] }); const second = h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, staged)); - expect(second).toMatchObject({ imported: 0, skipped: 2 }); + expect(second).toMatchObject({ imported: 0, skipped: 2, retired: 0 }); const slugs = (h.connection.db.prepare("select slug from skill_definitions where org_id = ? order by slug").all(h.orgId) as Array<{ slug: string }>) .map((row) => row.slug); expect(slugs).toEqual(["engineering-backend-architect", "humanizer"]); h.connection.close(); }); + test("legacy catalogue entries retire so every install mirrors the bundle, while their bindings keep working", () => { + const h = harness(); + const staged = stageBuiltinSkillsLibrary(h.sourceRoot, h.libraryRoot); + h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, staged)); + + // A relic of the old ~/.agents import: active, not in the bundle, bound. + let agentId = ""; + h.connection.transaction(() => { + const now = new Date().toISOString(); + h.connection.db.prepare(` + insert into skill_definitions(id, org_id, slug, name, owner_type, status, created_at) + values ('skl_legacy', ?, 'legacy-tool', 'Legacy Tool', 'organization', 'active', ?) + `).run(h.orgId, now); + h.connection.db.prepare(` + insert into skill_revisions(id, skill_id, version_semver, manifest_json, source_type, source_ref, published_at, immutable_checksum) + values ('skr_legacy', 'skl_legacy', '1.0.0', '{"name":"Legacy Tool","requiredTools":[]}', 'local_directory', '/old', ?, 'sum') + `).run(now); + agentId = h.organization.createAgent({ + orgId: h.orgId, name: "Ada", role: "Engineer", department: "Engineering", + modelProviderId: null, modelId: null, budgetCents: 1000, managerAgentId: null, + }); + h.skills.bind({ orgId: h.orgId, agentId, skillId: "skl_legacy", revisionId: "skr_legacy" }); + }); + + const report = h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, staged)); + expect(report.retired).toBe(1); + const legacy = h.connection.db.prepare("select status from skill_definitions where id = 'skl_legacy'").get() as { status: string }; + expect(legacy.status).toBe("retired"); + const builtinStatuses = (h.connection.db.prepare("select status from skill_definitions where org_id = ? and id <> 'skl_legacy'").all(h.orgId) as Array<{ status: string }>) + .map((row) => row.status); + expect(new Set(builtinStatuses)).toEqual(new Set(["active"])); + h.connection.close(); + }); + test("every agent's run carries humanizer; hire-time skills join it and oversized grants are named, not overflowed", () => { const h = harness(); // A third, oversized flat skill to prove the compiled-prompt cap. diff --git a/tests/unit/chat-governance.test.ts b/tests/unit/chat-governance.test.ts index d648b29a..3684c4f5 100644 --- a/tests/unit/chat-governance.test.ts +++ b/tests/unit/chat-governance.test.ts @@ -754,7 +754,13 @@ describe("chat governance", () => { }); employeeId = (connection.db.prepare("select id from employees where org_id = ?").get(orgId) as { id: string }).id; organization.createAgent({ orgId, name: "Ada", role: "Reviewer", department: "Engineering", modelProviderId: null, modelId: null, budgetCents: 10000, managerAgentId: null }); - organization.createAgent({ orgId, name: "Ada", role: "Release lead", department: "Engineering", modelProviderId: null, modelId: null, budgetCents: 10000, managerAgentId: null }); + // The front door refuses duplicate names outright... + expect(() => organization.createAgent({ orgId, name: "ada", role: "Release lead", department: "Engineering", modelProviderId: null, modelId: null, budgetCents: 10000, managerAgentId: null })) + .toThrow("already exists"); + // ...but a legacy database may already carry duplicates; mentions must + // still refuse to guess between them. + const secondId = organization.createAgent({ orgId, name: "Ada Backup", role: "Release lead", department: "Engineering", modelProviderId: null, modelId: null, budgetCents: 10000, managerAgentId: null }); + connection.db.prepare("update agents set name = 'Ada' where id = ?").run(secondId); spaceId = chat.createSpace({ orgId, spaceType: "project", name: "release", topic: "Release coordination", visibility: "workspace" }); }); diff --git a/tests/unit/profile-identity.test.ts b/tests/unit/profile-identity.test.ts index 229ecb1e..48a825fa 100644 --- a/tests/unit/profile-identity.test.ts +++ b/tests/unit/profile-identity.test.ts @@ -62,6 +62,32 @@ describe("identity editing", () => { h.connection.close(); }); + test("rejects renaming or hiring into an existing active agent name", () => { + const h = harness(); + h.connection.transaction(() => { + h.organization.createAgent({ + orgId: h.orgId, name: "Dana Lee", role: "Engineer", department: "Engineering", + modelProviderId: null, modelId: null, budgetCents: 1000, managerAgentId: h.agentId, + }); + }); + // Hiring a second "Dana Lee" (any case) is ambiguous for mentions and DMs. + expect(() => h.organization.createAgent({ + orgId: h.orgId, name: "dana lee", role: "Designer", department: "Design", + modelProviderId: null, modelId: null, budgetCents: 1000, managerAgentId: h.agentId, + })).toThrow("already exists"); + // Renaming the CEO onto that name is equally ambiguous. + expect(() => h.organization.updateAgent({ + orgId: h.orgId, agentId: h.agentId, name: "Dana Lee", role: "CEO", title: "CEO", + roleLevel: "c_suite", department: "Executive", budgetCents: 25000, modelProviderId: null, modelId: null, + })).toThrow("already exists"); + // Saving without changing the name stays legal. + h.connection.transaction(() => h.organization.updateAgent({ + orgId: h.orgId, agentId: h.agentId, name: "CEO", role: "CEO", title: "CEO", + roleLevel: "c_suite", department: "Executive", budgetCents: 25000, modelProviderId: null, modelId: null, + })); + h.connection.close(); + }); + test("rejects blank names and malformed avatar colors", () => { const h = harness(); const base = {