diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java index 0095b14c..c1772f6a 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java @@ -175,6 +175,10 @@ public Selection select() { AND (rate_limited_until IS NULL OR rate_limited_until <= now()) ORDER BY exhausted_at NULLS FIRST, last_used_at NULLS FIRST LIMIT 1) + -- Checked again here, after any wait for the row: a member switched off or erased + -- while this pick waited must not be returned, or its missing key would be read as + -- a corrupt one and the member marked refused (review of PR #179). + AND enabled AND erased_at IS NULL RETURNING id, label, type, base_url, api_key """; UUID chosen = null; @@ -384,7 +388,7 @@ public List list() { String sql = """ SELECT id, label, type, base_url, enabled, rate_limited_until, rejected_at, last_used_at, auth_mode, auth_mode <> 'SUBSCRIPTION' OR account_ref IS NOT NULL AS identified - FROM harness_credential ORDER BY label + FROM harness_credential WHERE erased_at IS NULL ORDER BY label """; List members = new ArrayList<>(); try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(sql); @@ -444,7 +448,7 @@ INSERT INTO harness_credential (id, label, type, base_url, api_key) * person to their phone. Finding out afterwards means a completed sign-in with nowhere to go. */ public boolean hasLabel(Connection c, String label) throws SQLException { - try (PreparedStatement ps = c.prepareStatement("SELECT 1 FROM harness_credential WHERE label = ?")) { + try (PreparedStatement ps = c.prepareStatement("SELECT 1 FROM harness_credential WHERE label = ? AND erased_at IS NULL")) { ps.setString(1, label); try (ResultSet rs = ps.executeQuery()) { return rs.next(); } } @@ -555,7 +559,7 @@ private int identifySeats(Connection c) throws SQLException { int switchedOff = 0; try (PreparedStatement ps = c.prepareStatement(""" SELECT id, type, api_key FROM harness_credential - WHERE auth_mode = 'SUBSCRIPTION' AND account_ref IS NULL ORDER BY updated_at DESC + WHERE auth_mode = 'SUBSCRIPTION' AND account_ref IS NULL AND erased_at IS NULL ORDER BY updated_at DESC """); ResultSet rs = ps.executeQuery()) { while (rs.next()) { UUID id = rs.getObject("id", UUID.class); @@ -607,10 +611,88 @@ public boolean remove(UUID id) { id) == 1; } + /** What {@link #delete} did. */ + public enum Deletion { + /** No run used it, so the row is gone. */ + DELETED, + /** A run used it: its secret is erased and it is hidden, but the row stays so the run can name it. */ + ERASED, + /** It is still switched on. Switch it off first, so no build is picking it while it goes. */ + STILL_ON, + NOT_FOUND + } + + /** + * Delete a switched-off member (operator feedback, 2026-09-27). + * + *

Switching off was the only way to remove one, and it kept the key or the person's sign-in file + * stored for ever. A member no run used is now deleted outright. One a run used cannot be — the run's + * attribution points at it — so its secret is erased and it leaves the pool screen, keeping only the + * name a finished run shows. Either way the secret is gone. + */ + public Deletion delete(UUID id) { + try (Connection c = dataSource.getConnection()) { + c.setAutoCommit(false); + try { + Deletion outcome = delete(c, id); + c.commit(); + return outcome; + } catch (SQLException | RuntimeException failure) { + c.rollback(); + throw failure; + } + } catch (SQLException e) { + throw new IllegalStateException("The harness credential " + id + " could not be deleted", e); + } + } + + private Deletion delete(Connection c, UUID id) throws SQLException { + try (PreparedStatement ps = c.prepareStatement( + "SELECT enabled FROM harness_credential WHERE id = ? AND erased_at IS NULL FOR UPDATE")) { + ps.setObject(1, id); + try (ResultSet rs = ps.executeQuery()) { + if (!rs.next()) return Deletion.NOT_FOUND; + if (rs.getBoolean("enabled")) return Deletion.STILL_ON; + } + } + // Used by a run, or picked lately: a dispatch picks a member and registers its run a moment later, + // outside one transaction, so a member picked in the last hour may be about to be named by a run + // not written yet. Erasing keeps the row that run will point at; deleting would fail its insert. + boolean used; + try (PreparedStatement ps = c.prepareStatement(""" + SELECT EXISTS (SELECT 1 FROM factory_run WHERE harness_credential_id = ?) + OR EXISTS (SELECT 1 FROM harness_credential WHERE id = ? AND last_used_at > now() - interval '1 hour') + """)) { + ps.setObject(1, id); + ps.setObject(2, id); + try (ResultSet rs = ps.executeQuery()) { rs.next(); used = rs.getBoolean(1); } + } + if (used) { + try (PreparedStatement ps = c.prepareStatement(""" + UPDATE harness_credential SET api_key = NULL, account_ref = NULL, erased_at = now(), updated_at = now() + WHERE id = ? + """)) { + ps.setObject(1, id); + ps.executeUpdate(); + } + return Deletion.ERASED; + } + // The sign-in that produced a seat is history; it keeps its row and forgets the member. + try (PreparedStatement ps = c.prepareStatement("UPDATE harness_sign_in SET credential_id = NULL WHERE credential_id = ?")) { + ps.setObject(1, id); + ps.executeUpdate(); + } + try (PreparedStatement ps = c.prepareStatement("DELETE FROM harness_credential WHERE id = ?")) { + ps.setObject(1, id); + ps.executeUpdate(); + } + return Deletion.DELETED; + } + /** Bring a disabled member back, because disabling is not deletion and must not be one-way. */ public boolean enable(UUID id) { try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( - "UPDATE harness_credential SET enabled = TRUE, updated_at = now() WHERE id = ? AND NOT enabled")) { + "UPDATE harness_credential SET enabled = TRUE, updated_at = now() WHERE id = ? AND NOT enabled AND erased_at IS NULL")) { ps.setObject(1, id); return ps.executeUpdate() == 1; } catch (SQLException e) { diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java index d8a555bc..da4a5431 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java @@ -113,6 +113,25 @@ public Response disable(@PathParam("id") String id) { return Response.noContent().build(); } + /** + * Delete a switched-off member: outright when no run used it, otherwise by erasing its secret and hiding + * it, keeping the name a finished run shows. A member still switched on is refused (409): a build could + * be picking it at that moment. + */ + @POST + @Path("/{id}/delete") + @Consumes(MediaType.WILDCARD) + public Response delete(@PathParam("id") String id) { + HarnessCredentialPool.Deletion outcome = pool.delete(uuid(id)); + switch (outcome) { + case NOT_FOUND -> throw new NotFoundException("no such harness credential: " + id); + case STILL_ON -> throw new ClientErrorException(Response.status(Response.Status.CONFLICT) + .entity("harness_credential_still_on").type(MediaType.TEXT_PLAIN).build()); + case DELETED, ERASED -> LOG.warnf("harness credential %s was deleted by an operator (%s)", id, outcome); + } + return Response.ok(java.util.Map.of("outcome", outcome.name())).build(); + } + /** Return a disabled member to the pool. Disabling is not deletion, so it is not one-way. */ @POST @Path("/{id}/enable") diff --git a/spire-orchestrator/src/main/resources/db/migration/V86__harness_credential_delete.sql b/spire-orchestrator/src/main/resources/db/migration/V86__harness_credential_delete.sql new file mode 100644 index 00000000..a3f04849 --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V86__harness_credential_delete.sql @@ -0,0 +1,19 @@ +-- Deleting a harness key or subscription seat (operator feedback, 2026-09-27). +-- +-- A member no run used is deleted outright. A member a run used cannot be: factory_run points at it, +-- and that attribution is the record of who paid. So such a member is ERASED instead: its stored key or +-- sign-in file is removed, it can never be switched on again, and it disappears from the pool screen. +-- The row stays only so a finished run can still name what paid for it. +ALTER TABLE harness_credential ALTER COLUMN api_key DROP NOT NULL; +ALTER TABLE harness_credential ADD COLUMN erased_at TIMESTAMPTZ; + +-- A live member has a secret; an erased one has none, is off, and names no account. +ALTER TABLE harness_credential ADD CONSTRAINT harness_credential_secret_unless_erased + CHECK (erased_at IS NOT NULL OR api_key IS NOT NULL); +ALTER TABLE harness_credential ADD CONSTRAINT harness_credential_erased_holds_nothing + CHECK (erased_at IS NULL OR (api_key IS NULL AND NOT enabled AND account_ref IS NULL)); + +-- An erased member's name is free again, so the operator can reuse it. It stays on the erased row, which +-- is what a finished run shows. +ALTER TABLE harness_credential DROP CONSTRAINT harness_credential_label_key; +CREATE UNIQUE INDEX harness_credential_label_live ON harness_credential (label) WHERE erased_at IS NULL; diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCredentialDeleteTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCredentialDeleteTest.java new file mode 100644 index 00000000..db7ddfb1 --- /dev/null +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCredentialDeleteTest.java @@ -0,0 +1,196 @@ +package dev.codespire.orchestrator.factory; + +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.security.TestSecurity; +import jakarta.inject.Inject; +import org.junit.jupiter.api.Test; + +import javax.sql.DataSource; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.UUID; + +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.*; + +/** + * Deleting a harness key or seat (operator feedback, 2026-09-27): switching off kept the key or the + * person's sign-in file stored for ever. Placeholder values throughout. + */ +@QuarkusTest +class HarnessCredentialDeleteTest { + + @Inject HarnessCredentialPool pool; + @Inject FactoryRunProjection runs; + @Inject HarnessSignIns signIns; + @Inject dev.codespire.encryption.EncryptionService encryption; + @Inject DataSource dataSource; + + private static String label() { + return "TEST-delete-" + UUID.randomUUID(); + } + + private UUID switchedOffKey(String label) { + UUID id = pool.add(label, "openai", "https://api.openai.com", "TEST-key-to-delete").id(); + pool.remove(id); + return id; + } + + private long rows(String sql, Object id) throws SQLException { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(sql)) { + ps.setObject(1, id); + try (ResultSet rs = ps.executeQuery()) { rs.next(); return rs.getLong(1); } + } + } + + private boolean listed(UUID id) { + return pool.list().stream().anyMatch(member -> member.id().equals(id)); + } + + /** A member no run used is gone, and its name is free again. */ + @Test + void aMemberNoRunUsedIsDeletedOutright() throws SQLException { + String label = label(); + UUID id = switchedOffKey(label); + + assertEquals(HarnessCredentialPool.Deletion.DELETED, pool.delete(id)); + + assertEquals(0, rows("SELECT count(*) FROM harness_credential WHERE id = ?", id)); + UUID again = pool.add(label, "openai", "https://api.openai.com", "TEST-key-again").id(); + pool.remove(again); + } + + /** + * A member a run used keeps the row the run points at, but loses its secret, leaves the screen and can + * never be switched on again. + */ + @Test + void aMemberARunUsedIsErasedAndKeptOnlyForTheRun() throws SQLException { + String label = label(); + UUID id = switchedOffKey(label); + String runId = "run::github:TEST-acme/app:subject-" + UUID.randomUUID() + ":1"; + assertTrue(runs.queued(new FactoryRunProjection.QueuedRun(runId, "codex", "TEST-model", "main", + "abc1234", "spire/TEST-delete", "TEST-bot", id), null, null)); + + assertEquals(HarnessCredentialPool.Deletion.ERASED, pool.delete(id)); + + assertEquals(1, rows("SELECT count(*) FROM harness_credential WHERE id = ? AND api_key IS NULL AND erased_at IS NOT NULL", id), + "the secret is gone; the row stays for the run's attribution"); + assertFalse(listed(id), "an erased member is not on the pool screen"); + assertFalse(pool.enable(id), "an erased member has nothing to switch on"); + HarnessSignIns.Started signIn = signIns.start(label, "codex", "TEST-operator"); + assertNull(signIn.refusal(), "an erased member's name is free for a new sign-in"); + signIns.cancel(signIn.view().id(), "TEST: only the name was checked"); + UUID again = pool.add(label, "openai", "https://api.openai.com", "TEST-key-again").id(); + pool.remove(again); + } + + /** + * A dispatch picks a member and writes its run a moment later; a member picked lately may be about to + * be named by a run not written yet, so it is erased rather than deleted (review of PR #179). + */ + @Test + void aMemberPickedLatelyIsErasedRatherThanDeleted() throws SQLException { + UUID id = switchedOffKey(label()); + try (Connection c = dataSource.getConnection(); + PreparedStatement ps = c.prepareStatement("UPDATE harness_credential SET last_used_at = now() WHERE id = ?")) { + ps.setObject(1, id); + ps.executeUpdate(); + } + + assertEquals(HarnessCredentialPool.Deletion.ERASED, pool.delete(id)); + assertEquals(1, rows("SELECT count(*) FROM harness_credential WHERE id = ?", id), "the row a pending run needs stays"); + } + + /** + * A key switched off and erased while a pick waits for it is not handed out, and not mistaken for a + * corrupt key and marked refused (review of PR #179). Every other usable key is held and switched off + * in the same transaction, so whichever the pick chose, it has to wait and re-check. + */ + @Test + void aKeyErasedWhileAPickWaitsIsNotReturnedNorMarkedRefused() throws Exception { + UUID id = pool.add(label(), "openai", "https://api.openai.com", "TEST-key-erased-in-flight").id(); + java.util.List others = new java.util.ArrayList<>(); + try (Connection other = dataSource.getConnection()) { + other.setAutoCommit(false); + try (PreparedStatement ps = other.prepareStatement(""" + UPDATE harness_credential SET enabled = FALSE + WHERE enabled AND auth_mode = 'API_KEY' AND rejected_at IS NULL AND id <> ? + RETURNING id + """)) { + ps.setObject(1, id); + try (ResultSet rs = ps.executeQuery()) { while (rs.next()) others.add(rs.getObject(1, UUID.class)); } + } + try (PreparedStatement ps = other.prepareStatement( + "UPDATE harness_credential SET enabled = FALSE, api_key = NULL, erased_at = now() WHERE id = ?")) { + ps.setObject(1, id); + ps.executeUpdate(); + } + java.util.concurrent.CompletableFuture picking = + java.util.concurrent.CompletableFuture.supplyAsync(pool::select); + + assertThrows(java.util.concurrent.TimeoutException.class, + () -> picking.get(1, java.util.concurrent.TimeUnit.SECONDS), "the pick waits for the held rows"); + other.commit(); + HarnessCredentialPool.Selection selection = picking.get(30, java.util.concurrent.TimeUnit.SECONDS); + + assertFalse(selection instanceof HarnessCredentialPool.Selection.Chosen, "every candidate was switched off meanwhile"); + assertEquals(0, rows("SELECT count(*) FROM harness_credential WHERE id = ? AND rejected_at IS NOT NULL", id), + "an erased key is not a refused one"); + } finally { + try (Connection c = dataSource.getConnection(); + PreparedStatement ps = c.prepareStatement("UPDATE harness_credential SET enabled = TRUE WHERE id = ANY (?)")) { + ps.setArray(1, c.createArrayOf("uuid", others.toArray())); + ps.executeUpdate(); + } + } + } + + /** A member still switched on could be picked by a build at this moment, so it must be switched off first. */ + @Test + void aMemberStillSwitchedOnIsRefused() { + UUID id = pool.add(label(), "openai", "https://api.openai.com", "TEST-key-on").id(); + try { + assertEquals(HarnessCredentialPool.Deletion.STILL_ON, pool.delete(id)); + assertTrue(listed(id)); + } finally { + pool.remove(id); + } + } + + /** A seat's sign-in record stays as history and forgets the seat. */ + @Test + void aSeatsSignInRecordForgetsTheDeletedSeat() throws SQLException { + String label = "TEST-delete-seat-" + UUID.randomUUID(); + HarnessSignIns.Started started = signIns.start(label, "codex", "TEST-operator"); + assertNull(started.refusal(), started.refusal()); + UUID signIn = started.view().id(); + String file = "{\"auth_mode\":\"TEST-chatgpt\",\"tokens\":{\"account_id\":\"TEST-account-" + UUID.randomUUID() + "\"}}"; + signIns.completed(new dev.codespire.contract.event.HarnessSignInResult.Completed(signIn.toString(), + encryption.encryptString(file, dev.codespire.contract.event.HarnessSignInResult.sealedAad(signIn.toString())), + "TEST-chatgpt", "TEST-chatgpt")); + UUID seat = signIns.get(signIn).orElseThrow().credentialId(); + pool.remove(seat); + + assertEquals(HarnessCredentialPool.Deletion.DELETED, pool.delete(seat)); + + assertNull(signIns.get(signIn).orElseThrow().credentialId()); + } + + @Test + @TestSecurity(user = "op", roles = "spire-admin") + void theScreenIsToldToSwitchItOffFirst() { + UUID id = pool.add(label(), "openai", "https://api.openai.com", "TEST-key-rest").id(); + try { + given().when().post("/api/harness-credentials/" + id + "/delete") + .then().statusCode(409).body(org.hamcrest.Matchers.containsString("harness_credential_still_on")); + pool.remove(id); + given().when().post("/api/harness-credentials/" + id + "/delete") + .then().statusCode(200).body("outcome", org.hamcrest.Matchers.is("DELETED")); + } finally { + pool.remove(id); + } + } +} diff --git a/spire-ui/src/api.ts b/spire-ui/src/api.ts index 3e858f54..52ed8a09 100644 --- a/spire-ui/src/api.ts +++ b/spire-ui/src/api.ts @@ -1732,6 +1732,13 @@ export const enableHarnessCredential = (id: string) => credentialAction(id, '/en export const clearHarnessCredentialRejection = (id: string) => credentialAction(id, '/clear-rejection', 'POST', 'Failed to clear the rejection'); export const restHarnessCredential = (id: string) => credentialAction(id, '/rest', 'POST', 'Failed to rest the credential'); +/** DELETED: gone. ERASED: a run used it, so its secret is erased and only its name stays, for that run. */ +export async function deleteHarnessCredential(id: string): Promise<'DELETED' | 'ERASED'> { + const res = await apiFetch(`/api/harness-credentials/${encodeURIComponent(id)}/delete`, { method: 'POST' }); + if (!res.ok) return throwResponse(res, 'Failed to delete the credential'); + return (await res.json()).outcome; +} + /** * A subscription sign-in in progress (M3.5 part F). * diff --git a/spire-ui/src/components/ReviewDetail.archive.test.tsx b/spire-ui/src/components/ReviewDetail.archive.test.tsx index 4df9ae14..0af15266 100644 --- a/spire-ui/src/components/ReviewDetail.archive.test.tsx +++ b/spire-ui/src/components/ReviewDetail.archive.test.tsx @@ -112,7 +112,7 @@ describe('ReviewDetail — archive and unarchive', () => { vi.spyOn(api, 'fetchReviewDetail').mockResolvedValue(detail(null)); renderDetail(); - fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i })); + fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i }, { timeout: 5000 })); const dialog = await screen.findByRole('dialog'); fireEvent.click(within(dialog).getByRole('button', { name: /^archive review$/i })); @@ -126,7 +126,7 @@ describe('ReviewDetail — archive and unarchive', () => { vi.spyOn(api, 'fetchReviewDetail').mockResolvedValue(detail(null)); renderDetail(); - fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i })); + fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i }, { timeout: 5000 })); const dialog = await screen.findByRole('dialog'); expect(within(dialog).queryByText(/permanently|cannot be undone/i)).not.toBeInTheDocument(); @@ -174,7 +174,7 @@ describe('ReviewDetail — archive and unarchive', () => { stubFetch('This review is still running. Wait for it to finish, or cancel it, then archive.'); renderDetail(); - fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i })); + fireEvent.click(await screen.findByRole('button', { name: /^archive review$/i }, { timeout: 5000 })); const dialog = await screen.findByRole('dialog'); fireEvent.click(within(dialog).getByRole('button', { name: /^archive review$/i })); diff --git a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx index e6db698e..644f227b 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx @@ -131,3 +131,21 @@ it('says why a second seat of one account cannot be switched back on', async () expect(await screen.findByText('Another seat is already signed in to this account. Switch that one off first.')).toBeInTheDocument(); }); + +// Switching off kept the key or the sign-in stored for ever; Delete erases it (feedback, 2026-09-27). +it('deletes a switched-off member only after the operator confirms, and never one still on', async () => { + const remove = vi.spyOn(api, 'deleteHarnessCredential').mockResolvedValue('ERASED'); + vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ + member({ id: 'TEST-off', label: 'TEST-off', enabled: false }), + member({ id: 'TEST-on', label: 'TEST-on' }), + ]); + render(); + + expect(within(await row('TEST-on')).queryByRole('button', { name: 'Delete' })).toBeNull(); + fireEvent.click(within(await row('TEST-off')).getByRole('button', { name: 'Delete' })); + expect(remove).not.toHaveBeenCalled(); + fireEvent.click(within(await row('TEST-off')).getByRole('button', { name: 'Delete it' })); + + await waitFor(() => expect(remove).toHaveBeenCalledWith('TEST-off')); + expect(await screen.findByText('TEST-off is deleted. Runs that used it still show its name; its secret is erased.')).toBeInTheDocument(); +}); diff --git a/spire-ui/src/components/SettingsHarnessCredentials.tsx b/spire-ui/src/components/SettingsHarnessCredentials.tsx index fc18ecaf..a13ded74 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.tsx @@ -1,7 +1,7 @@ import { useEffect, useState } from 'react'; import { KeyRound } from 'lucide-react'; import { - addHarnessCredential, clearHarnessCredentialRejection, disableHarnessCredential, + addHarnessCredential, clearHarnessCredentialRejection, deleteHarnessCredential, disableHarnessCredential, enableHarnessCredential, fetchHarnessCredentials, restHarnessCredential, type HarnessCredentialView, } from '../api'; @@ -33,6 +33,7 @@ const when = (value: string | null) => (value ? new Date(value).toLocaleString() function sentence(message: string): string { if (message.includes('subscription_account_taken')) return 'Another seat is already signed in to this account. Switch that one off first.'; + if (message.includes('harness_credential_still_on')) return 'Switch it off before deleting it.'; return message; } @@ -51,6 +52,8 @@ export default function SettingsHarnessCredentials() { const [error, setError] = useState(''), [notice, setNotice] = useState(''); const [adding, setAdding] = useState(false), [busy, setBusy] = useState(false); const [signingIn, setSigningIn] = useState(false); + // The member an operator asked to delete, waiting for them to confirm. + const [deleting, setDeleting] = useState(null); const [form, setForm] = useState({ label: '', type: 'openai', baseUrl: '', apiKey: '' }); const [refresh, setRefresh] = useState(0); @@ -68,7 +71,19 @@ export default function SettingsHarnessCredentials() { setBusy(true); setError(''); try { await action(); reload(message); } catch (failure) { setError(sentence(String(failure instanceof Error ? failure.message : failure))); } - finally { setBusy(false); } + finally { setBusy(false); setDeleting(null); } + } + + // Its own path rather than act(): the message depends on what the server did. + async function remove(member: HarnessCredentialView) { + setBusy(true); setError(''); + try { + const outcome = await deleteHarnessCredential(member.id); + reload(outcome === 'ERASED' + ? `${member.label} is deleted. Runs that used it still show its name; its secret is erased.` + : `${member.label} is deleted.`); + } catch (failure) { setError(sentence(String(failure instanceof Error ? failure.message : failure))); } + finally { setBusy(false); setDeleting(null); } } async function save() { @@ -174,10 +189,20 @@ export default function SettingsHarnessCredentials() { Switch off ) : ( + <> + {deleting !== member.id && } + {deleting === member.id && <> + Its stored key or sign-in is erased for good. + + + } + )} diff --git a/spire-ui/src/components/accounts.ts b/spire-ui/src/components/accounts.ts index 9850fa9e..782bdcd0 100644 --- a/spire-ui/src/components/accounts.ts +++ b/spire-ui/src/components/accounts.ts @@ -45,8 +45,10 @@ export function scopeLabel(scopes: string | null | undefined): string { * FACTORY and a migrated CONTEXT row. The kind and role are what tell them apart. */ export function accountOptionLabel( - account: { name: string; type: string; role: string; enabled: boolean }, + account: { name: string; type: string; role: string; enabled: boolean; botUsername?: string | null }, ): string { + // The bot handle too: two accounts can share a name, and the handle is what a ticket comment shows. return `${account.name} · ${account.type} · ${roleLabel(account.role)}` + + (account.botUsername ? ` · @${account.botUsername}` : '') + (account.enabled ? '' : ' (disabled)'); } diff --git a/spire-ui/src/components/repositories/factory/BuildStep.tsx b/spire-ui/src/components/repositories/factory/BuildStep.tsx index bb26e417..be6377eb 100644 --- a/spire-ui/src/components/repositories/factory/BuildStep.tsx +++ b/spire-ui/src/components/repositories/factory/BuildStep.tsx @@ -82,6 +82,9 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang const levels = known?.status === 'OK' ? known.offered.find(model => model.slug === form.model)?.efforts ?? [] : []; const levelUnusable = !!form.effort && !levels.includes(form.effort); const complete = !!form.baseBranch.trim() && !!form.harness && !!picked && picked.blocked === null && !levelUnusable; + // Save stays off until these are filled; saying which is what an operator needs (feedback, 2026-09-27). + const missing = [!form.baseBranch.trim() && 'Base branch', !form.harness && 'Harness', form.harness && !form.model && 'Model'] + .filter((field): field is string => !!field); return 0 ? 'done' : 'missing'} actions={!editing && }> @@ -129,6 +132,7 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang {error.includes('Reload it') &&

} {busy === 'saving' &&

Saving the build setup. The form unlocks when the server answers.

} + {busy === null && missing.length > 0 &&

To save, fill in: {missing.join(', ')}.

}
diff --git a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx index b7cf3c21..8cca457f 100644 --- a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx +++ b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx @@ -263,6 +263,17 @@ describe('build setup', () => { model: 'TEST-unpriced-only', payWith: 'SUBSCRIPTION' }))); }); + // Save stays off until the form is complete; it now says which field is missing (feedback, 2026-09-27). + it('names the fields that keep the build setup from saving', async () => { + renderFactory(); + await open(); + fireEvent.change(await screen.findByLabelText('Harness', field), { target: { value: 'codex' } }); + + expect(await screen.findByText('To save, fill in: Base branch, Model.')).toBeInTheDocument(); + fireEvent.change(screen.getByLabelText('Base branch', field), { target: { value: 'main' } }); + expect(await screen.findByText('To save, fill in: Model.')).toBeInTheDocument(); + }); + // Every option disabled: the select ignores clicks and keys, which the operator read as broken. it('says why no model can be picked when every one lacks a price', async () => { const types = ['INPUT', 'CACHED_INPUT', 'CACHE_WRITE', 'OUTPUT', 'REASONING']; diff --git a/spire-ui/src/components/repositories/factory/SourceStep.test.tsx b/spire-ui/src/components/repositories/factory/SourceStep.test.tsx index d3dfa7db..b3f6c7eb 100644 --- a/spire-ui/src/components/repositories/factory/SourceStep.test.tsx +++ b/spire-ui/src/components/repositories/factory/SourceStep.test.tsx @@ -54,12 +54,14 @@ it('explains a missing account and links to Accounts instead of an empty picker' expect(within(form).getByRole('link', { name: 'Add an account' })).toHaveAttribute('href', '#/settings/accounts'); expect(within(form).getByRole('button', { name: 'Register work source' })).toBeDisabled(); }); +// The handle too: it is what a ticket comment shows, and two accounts can share a name (feedback, 2026-09-27). it('distinguishes duplicate credential names in the create and edit account pickers', async () => { - const accounts = [account('github', { name: 'TEST-shared name' }), account('github', { id: 'TEST-reviewer', name: 'TEST-shared name', role: 'REVIEWER' })]; + const accounts = [account('github', { name: 'TEST-shared name', botUsername: 'TEST-factory-bot' }), + account('github', { id: 'TEST-reviewer', name: 'TEST-shared name', role: 'REVIEWER', botUsername: 'TEST-reviewer-bot' })]; renderFactory({ accounts }); const expectDistinct = (select: HTMLElement) => { - expect(within(select).getByRole('option', { name: 'TEST-shared name · github · Factory' })).toHaveValue('TEST-github'); - expect(within(select).getByRole('option', { name: 'TEST-shared name · github · Reviewer' })).toHaveValue('TEST-reviewer'); + expect(within(select).getByRole('option', { name: 'TEST-shared name · github · Factory · @TEST-factory-bot' })).toHaveValue('TEST-github'); + expect(within(select).getByRole('option', { name: 'TEST-shared name · github · Reviewer · @TEST-reviewer-bot' })).toHaveValue('TEST-reviewer'); }; fireEvent.click(within(await step(1)).getByRole('button', { name: 'Add another source' })); expectDistinct(within(screen.getByRole('group', { name: 'Add where tickets come from' })).getByLabelText('Tracker account', field)); @@ -165,3 +167,10 @@ it('reports a scan request while it is in flight and says when the scanner reads await act(async () => { release(); }); expect(await screen.findByText(/within about 30 seconds/)).toBeInTheDocument(); }); + +// This account also writes the comments on the tickets, so its role and handle are shown, not only a name +// two accounts can share (feedback, 2026-09-27). +it('names the account that reads the tickets by its role and handle', async () => { + renderFactory(); + expect(within(await step(1)).getByText(/· Factory · @TEST-bot/)).toBeInTheDocument(); +}); diff --git a/spire-ui/src/components/repositories/factory/SourceStep.tsx b/spire-ui/src/components/repositories/factory/SourceStep.tsx index b0dbcf99..70fbfe07 100644 --- a/spire-ui/src/components/repositories/factory/SourceStep.tsx +++ b/spire-ui/src/components/repositories/factory/SourceStep.tsx @@ -1,6 +1,7 @@ import { useState } from 'react'; import type { ProviderView, WebhookRepoView } from '../../../api'; import type { Repository } from '../repositoriesApi'; +import { accountOptionLabel } from '../../accounts'; import * as api from '../../work-items/workSourcesApi'; import FactoryStep from './FactoryStep'; import InstantUpdates from './InstantUpdates'; @@ -22,7 +23,11 @@ export default function SourceStep({ repository, sources, accounts, webhooks, op const [scanning, setScanning] = useState(null); const reading = sources.some(source => source.enabled); const adding = open === 'source:new'; - const accountName = (id: string) => accounts.find(account => account.id === id)?.name ?? 'an unavailable account'; + // Role and handle too: two accounts can share a name, and this one writes the comments on the tickets. + const accountName = (id: string) => { + const account = accounts.find(candidate => candidate.id === id); + return account ? accountOptionLabel(account) : 'an unavailable account'; + }; // The request only sets a flag; the scanner picks it up on its next sweep, about half a minute // later. So the button reports the request, and never claims the scan itself has finished. async function rescan(source: api.WorkSource) { diff --git a/spire-ui/src/components/work-items/DecisionEvidence.tsx b/spire-ui/src/components/work-items/DecisionEvidence.tsx index b39d23c4..9bffcd9e 100644 --- a/spire-ui/src/components/work-items/DecisionEvidence.tsx +++ b/spire-ui/src/components/work-items/DecisionEvidence.tsx @@ -58,9 +58,14 @@ export default function DecisionEvidence({ item, approval, evidence, evidenceErr
Starts from
{preparation.baseBranch} @ {preparation.baseCommit.slice(0, 7)}
Agent
{preparation.harness} · {preparation.model}
Plan
· one step
+
Pays with
{preparation.payWith === 'SUBSCRIPTION' ? 'a Codex subscription' : 'an API key'}
- {evidence?.specification &&
{evidence.specification}
} - {evidence?.instruction &&
{evidence.instruction}
} + {evidence?.specification && <> +

Specification — the ticket text, as it was prepared

+
{evidence.specification}
} + {evidence?.instruction && <> +

Plan — the one step the build runs

+
{evidence.instruction}
} }

If you approve

diff --git a/spire-ui/src/components/work-items/DecisionPanel.test.tsx b/spire-ui/src/components/work-items/DecisionPanel.test.tsx index 8aeaca1d..26140402 100644 --- a/spire-ui/src/components/work-items/DecisionPanel.test.tsx +++ b/spire-ui/src/components/work-items/DecisionPanel.test.tsx @@ -38,6 +38,15 @@ async function approvable() { } function show() { return render(); } +// How it pays is part of what the gate binds, and which text is which was not said (feedback, 2026-09-27). +it('shows how the build pays and labels the specification and the plan', async () => { + vi.mocked(gateway.getWorkItem).mockResolvedValue({ ...item, preparation: { ...item.preparation, payWith: 'SUBSCRIPTION' } }); + show(); + expect(await screen.findByText('Specification — the ticket text, as it was prepared')).toBeInTheDocument(); + expect(screen.getByText('Plan — the one step the build runs')).toBeInTheDocument(); + expect(screen.getByText('a Codex subscription')).toBeInTheDocument(); +}); + // The old card showed a digest and a generation number. An approver has to see what they approve. it('shows what the gate binds before offering an answer', async () => { show(); diff --git a/spire-ui/src/components/work-items/WorkItemDetail.tsx b/spire-ui/src/components/work-items/WorkItemDetail.tsx index 697c71f9..919e1f56 100644 --- a/spire-ui/src/components/work-items/WorkItemDetail.tsx +++ b/spire-ui/src/components/work-items/WorkItemDetail.tsx @@ -17,6 +17,9 @@ import { WorkflowStatus } from './WorkItems'; * One work item as its journey. The heading names the ticket, the steps say where it is and what a * person can do there, and everything else — policy, ticket text, history — folds underneath. */ +/** How often an open work item is read again. */ +const DETAIL_POLL_MILLISECONDS = 10_000; + export default function WorkItemDetail() { const { id = '' } = useParams(); const [params, setParams] = useSearchParams(); @@ -31,6 +34,9 @@ export default function WorkItemDetail() { const action = useRef(0); // The item the page last showed, so a re-read of it keeps its tracker text while a new item starts blank. const shown = useRef(''); + // Every read of the item takes a number, and only the newest one's answer is shown: a slow poll must not + // replace what a later refresh already put on screen (review of PR #179). + const reads = useRef(0); const panel = params.get('decide') ? 'decide' : params.get('prepare') ? 'prepare' : null; useEffect(() => { setNotice(''); }, [id]); useEffect(() => { if (panel) { action.current++; setNotice(''); } }, [panel]); @@ -40,13 +46,36 @@ export default function WorkItemDetail() { // hide the notice the re-read was started for. setState(previous => previous.item?.id === id ? previous : { item: null, error: null }); setTracker(previous => shown.current === id ? previous : { value: null, error: null }); - getWorkItem(id).then(item => { if (active) { shown.current = id; setState({ item, error: null }); } }) - .catch(error => { if (active) setState(previous => ({ item: previous.item?.id === id ? previous.item : null, error: String(error) })); }); + const read = ++reads.current; + getWorkItem(id).then(item => { if (active && read === reads.current) { shown.current = id; setState({ item, error: null }); } }) + .catch(error => { if (active && read === reads.current) setState(previous => ({ item: previous.item?.id === id ? previous.item : null, error: String(error) })); }); getWorkItemTracker(id).then(value => { if (active) setTracker({ value, error: null }); }) .catch(error => { if (active) setTracker({ value: null, error: String(error) }); }); return () => { active = false; }; }, [id, refresh]); + // Follows the item like Runs does, so a gate opening or a build ending shows without a click. Not while + // a panel is open: a re-read under a decision the operator is reading would move what they approve. + useEffect(() => { + if (panel) return; + // One poll at a time, and none answered after the panel opens or the page moves on. + let live = true, polling = false; + const timer = setInterval(() => { + if (document.visibilityState !== 'visible' || polling) return; + polling = true; + const read = ++reads.current; + getWorkItem(id).then(item => { + // Also fills an empty page: a poll that overtook a slow first load is now the newest read, and the + // load's own answer will be dropped (review of PR #179). + if (!live || read !== reads.current) return; + shown.current = id; + setState(previous => previous.item && previous.item.id !== id ? previous : { item, error: null }); + }).catch(() => { /* the last good read stays on screen; the next tick tries again */ }) + .finally(() => { polling = false; }); + }, DETAIL_POLL_MILLISECONDS); + return () => { live = false; clearInterval(timer); }; + }, [id, panel]); + function reread(message = '') { setNotice(message); setRefresh(value => value + 1); } function start() { setNotice(''); return ++action.current; } function open(name: 'decide' | 'prepare') { setParams({ [name]: '1' }); } diff --git a/spire-ui/src/components/work-items/WorkItems.test.tsx b/spire-ui/src/components/work-items/WorkItems.test.tsx index 311e9443..294e64c3 100644 --- a/spire-ui/src/components/work-items/WorkItems.test.tsx +++ b/spire-ui/src/components/work-items/WorkItems.test.tsx @@ -678,3 +678,75 @@ it('offers composing again while a plan decision is open', async () => { fireEvent.click(await screen.findByRole('button', { name: 'Prepare again from the ticket' })); await waitFor(() => expect(compose).toHaveBeenCalledWith(detail().id, detail().revision)); }); + +// The page follows the item like Runs does, so a build ending shows without a click (feedback, 2026-09-27). +it('reads the open work item again on its own', async () => { + vi.spyOn(auth, 'fetchMe').mockResolvedValue({ authEnabled: true, authenticated: true, user: 'TEST-admin', roles: ['spire-admin'] }); + vi.spyOn(api, 'getWorkItem').mockResolvedValue(detail()); + vi.spyOn(api, 'getWorkItemTracker').mockResolvedValue({ title: 'TEST-title', body: 'TEST-body', trackerStatus: 'open' }); + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('visible'); + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + showDetail(); + await waitFor(() => expect(api.getWorkItem).toHaveBeenCalledTimes(1)); + + await act(async () => { vi.advanceTimersByTime(10_000); }); + + await waitFor(() => expect(api.getWorkItem).toHaveBeenCalledTimes(2)); + } finally { + vi.useRealTimers(); + } +}); + +// A slow poll must not put back an older item than a refresh already showed (review of PR #179). +it('drops a poll answer that a later refresh overtook', async () => { + vi.spyOn(auth, 'fetchMe').mockResolvedValue({ authEnabled: true, authenticated: true, user: 'TEST-admin', roles: ['spire-admin'] }); + vi.spyOn(api, 'getWorkItemTracker').mockResolvedValue({ title: 'TEST-title', body: 'TEST-body', trackerStatus: 'open' }); + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('visible'); + const withEvent = (reason: string): Detail => ({ ...detail(), events: [{ sequence: 1, type: 'Admitted', reason, occurredAt: item().updatedAt }] }); + let answerPoll: (value: Detail) => void = () => {}; + vi.spyOn(api, 'getWorkItem') + .mockResolvedValueOnce(withEvent('TEST-first read')) + .mockImplementationOnce(() => new Promise(resolve => { answerPoll = resolve; })) + .mockResolvedValue(withEvent('TEST-refreshed read')); + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + showDetail(); + expect(await screen.findByText(/TEST-first read/)).toBeInTheDocument(); + await act(async () => { vi.advanceTimersByTime(10_000); }); + await waitFor(() => expect(api.getWorkItem).toHaveBeenCalledTimes(2)); + + fireEvent.click(screen.getByRole('button', { name: 'Refresh workflow' })); + expect(await screen.findByText(/TEST-refreshed read/)).toBeInTheDocument(); + await act(async () => { answerPoll(withEvent('TEST-stale poll')); }); + + expect(screen.getByText(/TEST-refreshed read/)).toBeInTheDocument(); + expect(screen.queryByText(/TEST-stale poll/)).toBeNull(); + } finally { + vi.useRealTimers(); + } +}); + +// A first load slower than the first poll must not leave the page loading for ever (review of PR #179). +it('shows the item from a poll that overtook a slow first load', async () => { + vi.spyOn(auth, 'fetchMe').mockResolvedValue({ authEnabled: true, authenticated: true, user: 'TEST-admin', roles: ['spire-admin'] }); + vi.spyOn(api, 'getWorkItemTracker').mockResolvedValue({ title: 'TEST-title', body: 'TEST-body', trackerStatus: 'open' }); + vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('visible'); + const withEvent = (reason: string): Detail => ({ ...detail(), events: [{ sequence: 1, type: 'Admitted', reason, occurredAt: item().updatedAt }] }); + let failLoad: (reason: Error) => void = () => {}; + vi.spyOn(api, 'getWorkItem') + .mockImplementationOnce(() => new Promise((_, reject) => { failLoad = reject; })) + .mockResolvedValue(withEvent('TEST-polled read')); + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + showDetail(); + await act(async () => { vi.advanceTimersByTime(10_000); }); + + expect(await screen.findByText(/TEST-polled read/)).toBeInTheDocument(); + await act(async () => { failLoad(new Error('TEST-superseded load failed')); }); + expect(screen.queryByText(/TEST-superseded load failed/)).toBeNull(); + expect(screen.getByText(/TEST-polled read/)).toBeInTheDocument(); + } finally { + vi.useRealTimers(); + } +}); diff --git a/spire-ui/src/components/work-items/WorkItems.tsx b/spire-ui/src/components/work-items/WorkItems.tsx index da278612..7e70b2a3 100644 --- a/spire-ui/src/components/work-items/WorkItems.tsx +++ b/spire-ui/src/components/work-items/WorkItems.tsx @@ -96,6 +96,8 @@ export default function WorkItems() { {notice &&

{notice}

} +

A labelled ticket is found by a scan that runs on a schedule (every 30 seconds by + default), then prepared. It can take a minute or two to appear here. This list refreshes by itself.

{needs !== null && needs > 0 &&