From 8ef3d2d88f4379b7d6f1ece44a3636238bbe2101 Mon Sep 17 00:00:00 2001 From: Fizz <550f2cb2562fc05f87c4839dc3c9b1bfca4d68db4183f8b54d3d477f6836a91d@buzz.block.builderlab.xyz> Date: Mon, 24 Aug 2026 14:23:23 -0600 Subject: [PATCH] test(desktop): repair the share tests and simplify the comments The change to `TeamsSection.tsx` made 3 tests fail in `desktop/tests/e2e/team-snapshot.spec.ts`. CI found the failures. This commit corrects them. The 3 tests share a team. Before, they used the default mock team "Engineering", which has no members (`e2eBridge.ts`). The earlier gate used `hasMissingPersonas`, which is false for a team that has no members, and thus the Share item was enabled. The new gate uses `isUsable`, which is correctly false. The Share item is now disabled, and each test stopped at a disabled menu item. The new gate is correct. The `build_team_export_snapshot` function makes one member for each entry in `team.persona_ids`. A team that has no members thus gives a snapshot that has no members, and `validate_team_snapshot` refuses it. The mock replaces the encode command, and therefore the tests did not show this. The 3 tests now seed their own team, and that team has one member. This commit also writes all the comments of this change again in Simplified Technical English (ASD-STE100): short sentences, active voice, no idioms. The text of the delete dialog is simpler too. No production behavior changes in this commit. Co-authored-by: Storme Briscoe Signed-off-by: Storme Briscoe --- .../src/managed_agents/teams_tests.rs | 20 +++++---- .../features/agents/lib/teamPersonas.test.mjs | 10 ++--- .../features/agents/ui/TeamDeleteDialog.tsx | 2 +- .../src/features/agents/ui/TeamsSection.tsx | 19 +++++--- .../agents/ui/teamDialogSelection.test.mjs | 6 +-- .../features/agents/ui/teamDialogSelection.ts | 26 ++++++----- desktop/tests/e2e/agents.spec.ts | 21 ++++----- desktop/tests/e2e/team-snapshot.spec.ts | 45 ++++++++++++++----- 8 files changed, 92 insertions(+), 57 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/teams_tests.rs b/desktop/src-tauri/src/managed_agents/teams_tests.rs index 282cb532df..da1fe270d3 100644 --- a/desktop/src-tauri/src/managed_agents/teams_tests.rs +++ b/desktop/src-tauri/src/managed_agents/teams_tests.rs @@ -263,14 +263,18 @@ fn agents_referencing_team_empty_when_no_matches() { assert!(agents_referencing_team(&agents, &t).is_empty()); } -/// The escape hatch from the team/agent delete deadlock, asserted end to end at -/// this layer: `delete_team_with_cascade` refuses while any agent references the -/// team, and `validate_persona_deletion` refuses to delete a team's agent — so -/// the only non-circular sequence is "empty the team's roster, then delete the -/// team". Emptying the roster clears `team_id` on the members' instances (see -/// `apply_team_membership_delta` in `commands::teams`), and once it is cleared -/// this guard must report zero referencing agents. If it still matched, the team -/// would stay permanently undeletable and the deadlock would be back. +/// A detached agent must not prevent the deletion of a team. +/// +/// You cannot delete a team and its agents in one step. The +/// `delete_team_with_cascade` function stops if an agent points to the team. +/// The `validate_persona_deletion` function stops if the agent is in a team. +/// Thus only one sequence is possible: first remove all the members from the +/// team, then delete the team. +/// +/// When you remove a member, the `apply_team_membership_delta` function in +/// `commands::teams` clears the `team_id` field of that member. This test makes +/// sure that the guard then finds no agents. If the guard still found the +/// agent, you could not delete the team. #[test] fn detached_agents_no_longer_reference_the_team() { let t = team("json-team-3", "Emptied Team"); diff --git a/desktop/src/features/agents/lib/teamPersonas.test.mjs b/desktop/src/features/agents/lib/teamPersonas.test.mjs index 85163e8f23..109f9164d2 100644 --- a/desktop/src/features/agents/lib/teamPersonas.test.mjs +++ b/desktop/src/features/agents/lib/teamPersonas.test.mjs @@ -93,11 +93,11 @@ test("getUsableTeams keeps only fully-resolved teams with at least one persona", ); }); -// An emptied team is now reachable through the editor (removing every member is -// how you escape the team/agent delete deadlock). It must still be treated as -// unusable: a zero-member snapshot fails the importer's own -// "Team snapshot must have at least one member" check, and deploying it would -// attach no agents at all. +// An emptied team is now possible in the editor. You remove all the members to +// make it possible to delete the team. But the team must still be unusable. A +// snapshot of it has no members, and the import of that snapshot fails with the +// message "Team snapshot must have at least one member". A deployment of it +// would also add no agents. test("resolveTeamPersonas marks a deliberately emptied team complete but unusable", () => { const resolution = resolveTeamPersonas(createTeam("team-empty", []), [ createPersona("persona-1", "Solo"), diff --git a/desktop/src/features/agents/ui/TeamDeleteDialog.tsx b/desktop/src/features/agents/ui/TeamDeleteDialog.tsx index 5477fd9a02..b406211c94 100644 --- a/desktop/src/features/agents/ui/TeamDeleteDialog.tsx +++ b/desktop/src/features/agents/ui/TeamDeleteDialog.tsx @@ -31,7 +31,7 @@ export function TeamDeleteDialog({ Delete team? {team - ? `Delete "${team.name}". This removes the team template only — deployed agents are left in place, but deletion is blocked while any of them still reference this team.` + ? `Delete "${team.name}". This deletes the team template only. Deployed agents stay in place. You cannot delete the team while an agent is still a member of it.` : "Delete this team."} diff --git a/desktop/src/features/agents/ui/TeamsSection.tsx b/desktop/src/features/agents/ui/TeamsSection.tsx index b8508441e4..d424fb6821 100644 --- a/desktop/src/features/agents/ui/TeamsSection.tsx +++ b/desktop/src/features/agents/ui/TeamsSection.tsx @@ -94,13 +94,18 @@ export function TeamsSection({ const missingPersonaCount = resolution.missingPersonaCount; const hasMissingPersonas = resolution.hasMissingPersonas; const isEmptyTeam = team.personaIds.length === 0; - // Deploy/Duplicate/Share need a roster that fully resolves to real - // agents. `isUsable` covers both failure modes: a member that is no - // longer in My Agents, and a deliberately emptied team. Sharing an - // empty team would mint a snapshot that its own importer rejects - // ("Team snapshot must have at least one member"), so gate it here. - // Edit and Delete stay enabled — emptying a team then deleting it is - // the intended way out of the team/agent delete deadlock. + // The Deploy, Duplicate and Share items need a roster that has + // members, and each member must be a known agent. The `isUsable` + // flag shows both conditions. It is false if a member is not in My + // Agents. It is also false if the team has no members. + // + // You must not share a team that has no members. The snapshot then + // has no members, and the import of that snapshot fails with the + // message "Team snapshot must have at least one member". + // + // The Edit and Delete items stay enabled. You use these two items + // to delete a team: first remove all the members, then delete the + // team. const canUseRoster = resolution.isUsable; return ( diff --git a/desktop/src/features/agents/ui/teamDialogSelection.test.mjs b/desktop/src/features/agents/ui/teamDialogSelection.test.mjs index e7d0ede493..eed58a742e 100644 --- a/desktop/src/features/agents/ui/teamDialogSelection.test.mjs +++ b/desktop/src/features/agents/ui/teamDialogSelection.test.mjs @@ -94,9 +94,9 @@ test("orderPersonasByInitiallySelected keeps initially selected personas at top" // ── canSubmitTeamDialog ─────────────────────────────────────────────────── // -// The regression these cover: the submit button used to also require -// `selectedPersonaIds.length > 0`, which made "remove every member" unsavable -// and left the team/agent delete deadlock with no way out. +// These tests show the correction of a defect. Before, the submit button also +// needed `selectedPersonaIds.length > 0`. Thus you could not save a team after +// you removed all the members, and you could not delete the team. test("canSubmitTeamDialog allows saving a team with an empty roster", () => { assert.equal(canSubmitTeamDialog({ name: "Hive", isPending: false }), true); diff --git a/desktop/src/features/agents/ui/teamDialogSelection.ts b/desktop/src/features/agents/ui/teamDialogSelection.ts index b13ae534be..0ab4977a02 100644 --- a/desktop/src/features/agents/ui/teamDialogSelection.ts +++ b/desktop/src/features/agents/ui/teamDialogSelection.ts @@ -9,19 +9,23 @@ export function copySelectedPersonaIds(personaIds: string[]): string[] { } /** - * Whether the team dialog's submit button should be enabled. + * Tells you if the submit button in the team dialog is enabled. * - * A team name is required; an **empty roster is deliberately allowed**. Emptying - * a team is the only way out of the delete deadlock — `delete_team` refuses - * while agents still reference the team, and agent deletion refuses because the - * agent belongs to a team. "Remove every member, then delete the team" is the - * escape hatch, so the editor must be able to save a zero-member team. The Rust - * side already supports it (`update_team` has no minimum-member check and - * `apply_team_membership_delta` detaches the removed members' instances); the - * min-1 rule that remains applies to snapshot *import* only. + * The team must have a name. The team does not need a member. An empty roster + * is correct. * - * Extracted as a pure function so the "empty roster is submittable" contract is - * covered by a test rather than living in a JSX `disabled` expression. + * An empty roster is necessary. It is the only way to delete a team. The + * `delete_team` command stops if an agent points to the team. The agent delete + * command stops if the agent is in a team. Thus you must first remove all the + * members from the team. Then you can delete the team. + * + * The Rust code permits a team that has no members. The `update_team` command + * does not count the members. The `apply_team_membership_delta` function clears + * the `team_id` field of each member that you remove. Only the import of a + * snapshot needs one member or more. + * + * This is a separate function because a test can then read the rule. Before, + * the rule was in a JSX `disabled` expression, where a test cannot read it. */ export function canSubmitTeamDialog({ name, diff --git a/desktop/tests/e2e/agents.spec.ts b/desktop/tests/e2e/agents.spec.ts index 23a400a7d1..92f45972d2 100644 --- a/desktop/tests/e2e/agents.spec.ts +++ b/desktop/tests/e2e/agents.spec.ts @@ -2709,12 +2709,12 @@ test("duplicate instances move from the agents gallery into the agent profile", ).toHaveCount(0); }); -// Regression for the team-deletion deadlock: `delete_team` refuses while agents -// still reference the team, and deleting a team's agent refuses because it -// belongs to a team. The only non-circular way out is to empty the roster and -// then delete the team — which the editor used to forbid, because the submit -// button also required at least one selected member. This walks the full user -// sequence: edit → deselect every member → save → delete. +// This test shows the correction of a defect. You could not delete a team. The +// `delete_team` command stops if an agent points to the team. The agent delete +// command stops if the agent is in a team. Thus you must first remove all the +// members from the team, then delete the team. But the editor did not permit +// this, because the submit button needed one member or more. This test does the +// full sequence: edit the team, remove each member, save, then delete. test("a team can be emptied and then deleted", async ({ page }) => { await installMockBridge(page, { personas: [ @@ -2751,15 +2751,16 @@ test("a team can be emptied and then deleted", async ({ page }) => { await roster.getByRole("option", { name: /Deadlock A/ }).click(); await roster.getByRole("option", { name: /Deadlock B/ }).click(); - // The regression: with no members selected, save must still be enabled. + // This is the corrected behavior. No member is selected, and the save + // button must be enabled. const save = page.getByRole("button", { name: "Save changes" }); await expect(save).toBeEnabled(); await save.click(); - // Removing members prompts about the underlying agents; keep them. + // The app asks what to do with the agents of the removed members. Keep them. await page.getByRole("button", { name: "Keep agents" }).click(); - // The team survives with an empty roster and says so. + // The team is still present, it has no members, and the card shows this. await expect(teamCard).toContainText("This team has no agents"); const teams = await invokeTauri>( page, @@ -2769,7 +2770,7 @@ test("a team can be emptied and then deleted", async ({ page }) => { teams.find((team) => team.id === "team-deadlock")?.persona_ids, ).toEqual([]); - // With nothing referencing it, the team is now deletable — the deadlock is gone. + // No agent points to the team now. Thus you can delete the team. await page.getByLabel("Deadlock Team team actions").click(); await page.getByRole("menuitem", { name: "Delete" }).click(); await page diff --git a/desktop/tests/e2e/team-snapshot.spec.ts b/desktop/tests/e2e/team-snapshot.spec.ts index 6246c7ac8c..027c66d2a7 100644 --- a/desktop/tests/e2e/team-snapshot.spec.ts +++ b/desktop/tests/e2e/team-snapshot.spec.ts @@ -49,6 +49,22 @@ const ANALYST_PERSONA_ID = "test-analyst"; const ANALYST_PUBKEY = "953d3363262e86b770419834c53d2446409db6d918a57f8f339d495d54ab001f"; +// The share tests use this team. The team has one member. +// +// You cannot share a team that has no members. A team snapshot must have one +// member or more, and the Share item is disabled for a team that has no +// members. The default mock team "Engineering" has no members. Therefore each +// share test seeds this team, and the team holds the seeded Analyst persona. +// The name must be different from "Engineering". If the names are the same, a +// locator finds two teams and the test stops. +const SHARE_TEAM_NAME = "Delivery Crew"; +const SHARE_TEAM_SEED = { + id: "team-share-001", + name: SHARE_TEAM_NAME, + description: "Team for the share tests", + personaIds: [ANALYST_PERSONA_ID], +}; + // ── (a) Confirm-fail + retry ──────────────────────────────────────────────── test("team_snapshot_import_confirm_fail_renders_error_and_retry_succeeds", async ({ @@ -251,16 +267,17 @@ test("team sharing uses the people picker and gates memory before sending", asyn displayName: "Charlie", }, ], + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); const shareDialog = page.getByTestId("team-share-dialog"); await expect(shareDialog).toBeVisible(); await expect( - shareDialog.getByRole("heading", { name: "Share Engineering" }), + shareDialog.getByRole("heading", { name: `Share ${SHARE_TEAM_NAME}` }), ).toBeVisible(); const search = shareDialog.getByTestId("team-share-recipient-search"); @@ -289,9 +306,11 @@ test("team sharing uses the people picker and gates memory before sending", asyn expect(encodeLevelsBeforeConfirmation).toEqual([]); await memoryConfirmation.getByTestId("team-share-memory-confirm").click(); - await expect(page.getByText("Sent a copy of Engineering")).toBeVisible({ - timeout: 8_000, - }); + await expect(page.getByText(`Sent a copy of ${SHARE_TEAM_NAME}`)).toBeVisible( + { + timeout: 8_000, + }, + ); const log = await readCommandLog(page); expect( @@ -307,7 +326,7 @@ test("team sharing uses the people picker and gates memory before sending", asyn ); expect(sendEntry).toBeTruthy(); const sendPayload = sendEntry?.payload as { content?: string } | undefined; - expect(sendPayload?.content).toContain("[Engineering]("); + expect(sendPayload?.content).toContain(`[${SHARE_TEAM_NAME}](`); expect(sendPayload?.content).not.toContain("![image]("); }); @@ -332,11 +351,12 @@ test("team share level carries memories onto the link path too", async ({ }, ], agentMemory: createMockAgentMemoryListing(), + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); const shareDialog = page.getByTestId("team-share-dialog"); await expect(shareDialog).toBeVisible(); @@ -397,12 +417,13 @@ test("team sharing keeps link copy and export in the shared surface", async ({ personaId: ANALYST_PERSONA_ID, }, ], + teams: [SHARE_TEAM_SEED], uploadDescriptors: [MOCK_UPLOAD_DESCRIPTOR], uploadDelayMs: 800, }); await gotoAgentsPage(page); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); const menu = page.getByRole("menu"); await expect( menu.getByRole("menuitem", { name: "Export snapshot" }), @@ -508,24 +529,24 @@ test("team sharing keeps link copy and export in the shared surface", async ({ const composerTeamCard = page.getByTestId("composer-team-snapshot-card"); await expect(composerTeamCard).toBeVisible(); - await expect(composerTeamCard).toContainText("Engineering"); + await expect(composerTeamCard).toContainText(SHARE_TEAM_NAME); await expect(composerTeamCard.locator("img")).toHaveCount(0); await page.getByTestId("send-message").click(); const sentTeamCard = page.getByTestId("agent-snapshot-card").last(); await expect(sentTeamCard).toBeVisible(); - await expect(sentTeamCard).toContainText("Engineering"); + await expect(sentTeamCard).toContainText(SHARE_TEAM_NAME); await expect(sentTeamCard).toContainText("Add team"); await expect(sentTeamCard.locator("img")).toHaveCount(0); await page.getByTestId("open-agents-view").click(); - await page.getByLabel("Engineering team actions").click(); + await page.getByLabel(`${SHARE_TEAM_NAME} team actions`).click(); await page.getByRole("menuitem", { name: "Share" }).click(); await page.getByTestId("team-share-export").click(); const exportDialog = page.getByTestId("team-snapshot-export-dialog"); await expect(exportDialog).toBeVisible(); await expect( - exportDialog.getByRole("heading", { name: "Export Engineering" }), + exportDialog.getByRole("heading", { name: `Export ${SHARE_TEAM_NAME}` }), ).toBeVisible(); await expect( exportDialog.getByTestId("team-snapshot-memory-trigger"),