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.
This commit is contained in:
1 parent
c50f17190c
commit
04093c4869
8 files changed
+156
-12
No files matched your search
+2
-2
@@ -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}`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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 }>;
|
||||
|
||||
@@ -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.");
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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" });
|
||||
});
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
Reference in new issue
Block a user