diff --git a/.superpowers/sdd/2026-08-26-housekeeping-completion/task-12-report.md b/.superpowers/sdd/2026-08-26-housekeeping-completion/task-12-report.md index 0538f78b..5999ed1f 100644 --- a/.superpowers/sdd/2026-08-26-housekeeping-completion/task-12-report.md +++ b/.superpowers/sdd/2026-08-26-housekeeping-completion/task-12-report.md @@ -254,4 +254,57 @@ pnpm exec biome check --formatter-enabled=false <28 exact changed TypeScript/TSX Checked 28 files. No fixes applied. ``` +The approved Node engine warning remains: the repository requests Node `>=26.8.1 <27`, while the host runs Node `v26.7.0` with pnpm `11.24.0`. No database operation, deployment, push, or pull-request update was performed. + +## Official review fix round 3 + +The round-2 re-review reported 0 Critical, 2 Important, and 2 adjacent Minor findings. This round addresses all four findings without changing the nine-route manifest, the public command IDs, or legacy external call ordering and permissions. + +### RED evidence + +```text +Focused external-audit, bulk-production, and dispatcher matrix +Test Files 2 failed (2) +Tests 15 failed | 54 passed (69) + +The ten external-only false/throw cases persisted optimistic desired after-state instead of confirmed unchanged or unknown delivery evidence. Four bulk currency/badge false/throw cases reported completed=0 and Database error after a committed database write. The dispatcher accepted one ok:false result carrying impossible completion metadata. +``` + +### GREEN implementation + +- External-only alert, disconnect, mute, unmute, and send-currency keep desired state in intent/success evidence. Confirmed RCON `false` now writes a dedicated unchanged/no-delivery failure snapshot; an exception writes unknown delivery with a null after-state. Correlation and failure outcome stay identical across intent/outcome records. +- Bulk currency and badge count a successful database write before RCON. RCON false/throw is additive `externalSyncFailures` sync debt, never a database failure; `failedIds` remains reserved for database/business failures and the result is typed partial with an explicit no-automatic-retry warning. +- Webhook notification remains explicit fire-and-forget best effort (`void notify(...)`) and no longer participates in mutation completion. The impossible promise-rejection test was replaced with the real void contract. +- The dispatcher runtime schema now accepts `completion` only for `ok: true`, matching the TypeScript `HousekeepingResult` contract; failure envelopes containing it are rejected as malformed. + +### Final verification after review fix round 3 + +```text +Focused external audit + production bulk + dispatcher +Test Files 3 passed (3) +Tests 73 passed (73) + +People + foundation + legacy wrappers + staff-smoke matrix +Test Files 48 passed (48) +Tests 470 passed (470) + +pnpm test:housekeeping +Test Files 58 passed (58) +Tests 516 passed (516) + +pnpm test +Test Files 206 passed | 3 skipped (209) +Tests 1359 passed | 5 skipped (1364) + +pnpm typecheck +tsc --noEmit +Exit 0 + +pnpm exec biome check --formatter-enabled=false <4 exact changed source/test files> +Checked 4 files. No fixes applied. + +git diff --check +Exit 0 +``` + The approved Node engine warning remains: the repository requests Node `>=26.8.1 <27`, while the host runs Node `v26.7.0` with pnpm `11.24.0`. No database operation, deployment, push, or pull-request update was performed. \ No newline at end of file diff --git a/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts b/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts index 79001aec..6d123986 100644 --- a/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts +++ b/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts @@ -578,6 +578,117 @@ describe("People production workflow adapter", () => { ]); }); + it.each([ + [ + "user.alert", + "alertUser", + { userId: 7, message: "Hello" }, + { userId: 7, alertDelivered: false }, + ], + [ + "user.disconnect", + "disconnectUser", + { userId: 7, reason: "Stuck session" }, + target({ online: true }), + ], + [ + "user.mute", + "muteUser", + { userId: 7, duration: 30, reason: "Chat abuse" }, + target({ online: true }), + ], + [ + "user.unmute", + "unmuteUser", + { userId: 7, reason: "Appeal accepted" }, + target({ online: true }), + ], + [ + "user.send-currency", + "giveCredits", + { userId: 7, amount: 25, reason: "Event award" }, + target({ online: true }), + ], + ] as const)( + "audits confirmed external-only %s failure without optimistic state", + async (operation, rconMethod, input, failureAfter) => { + if (operation !== "user.alert") mocks.selectQueue.push([target()]); + mocks.rcon[rconMethod].mockResolvedValueOnce(false); + const correlationId = `confirmed-${operation}`; + + const result = await peopleMutationService.execute( + { ...invocation, correlationId, legacy: false }, + operation, + input, + ); + + expect(result).toMatchObject({ + ok: false, + error: { code: "DEPENDENCY_UNAVAILABLE" }, + correlationId, + }); + const entries = mocks.audit.mock.calls.map( + (call) => call[0] as AuditEntry, + ); + expect(entries.map(({ outcome }) => outcome)).toEqual([ + "intent", + "failure", + ]); + expect(entries[1]).toMatchObject({ correlationId, after: failureAfter }); + expect(entries[1]?.after).not.toEqual(entries[0]?.after); + }, + ); + + it.each([ + ["user.alert", "alertUser", { userId: 7, message: "Hello" }], + [ + "user.disconnect", + "disconnectUser", + { userId: 7, reason: "Stuck session" }, + ], + [ + "user.mute", + "muteUser", + { userId: 7, duration: 30, reason: "Chat abuse" }, + ], + ["user.unmute", "unmuteUser", { userId: 7, reason: "Appeal accepted" }], + [ + "user.send-currency", + "giveCredits", + { userId: 7, amount: 25, reason: "Event award" }, + ], + ] as const)( + "audits unknown external-only %s delivery with a null after-state", + async (operation, rconMethod, input) => { + if (operation !== "user.alert") mocks.selectQueue.push([target()]); + mocks.rcon[rconMethod].mockRejectedValueOnce( + new Error("RCON unavailable"), + ); + const correlationId = `unknown-${operation}`; + + const result = await peopleMutationService.execute( + { ...invocation, correlationId, legacy: false }, + operation, + input, + ); + + expect(result).toMatchObject({ + ok: false, + error: { code: "DEPENDENCY_UNAVAILABLE" }, + correlationId, + }); + const entries = mocks.audit.mock.calls.map( + (call) => call[0] as AuditEntry, + ); + expect(entries.map(({ outcome }) => outcome)).toEqual([ + "intent", + "failure", + ]); + expect(entries[1]).toMatchObject({ correlationId }); + expect(entries[1]?.after).toBeUndefined(); + }, + ); + it("executes bulk ban, currency, and badge with database, RCON, and legacy activity outcomes", async () => { await expect( peopleMutationService.execute(invocation, "users.bulk-ban", { @@ -625,81 +736,131 @@ describe("People production workflow adapter", () => { ); }); - it("reports bulk currency as partial after database commit when RCON reports false", async () => { - mocks.rcon.giveCredits.mockResolvedValue(false); - const result = await peopleMutationService.execute( - { ...invocation, correlationId: "bulk-currency-partial", legacy: false }, - "users.bulk-currency", - { userIds: [7], type: "credits", amount: 50 }, - ); - - expect(result).toMatchObject({ - ok: true, - completion: { - status: "partial", - external: "failed", - audit: "persisted", - }, - data: { - after: { - completed: 0, - total: 1, - failedIds: [{ userId: 7, reason: "Database error" }], + it.each(["false", "throw"] as const)( + "reports bulk currency database completion separately when RCON reports %s", + async (failureMode) => { + if (failureMode === "false") { + mocks.rcon.giveCredits.mockResolvedValue(false); + } else { + mocks.rcon.giveCredits.mockRejectedValue(new Error("RCON unavailable")); + } + const result = await peopleMutationService.execute( + { + ...invocation, + correlationId: "bulk-currency-partial", + legacy: false, }, - }, - correlationId: "bulk-currency-partial", - }); - expect( - mocks.audit.mock.calls.map((call) => (call[0] as AuditEntry).outcome), - ).toEqual(["intent", "partial"]); - }); - it("returns typed partial after committed update or reset when notification fails", async () => { - mocks.notify.mockRejectedValue(new Error("notification unavailable")); + "users.bulk-currency", + { userIds: [7], type: "credits", amount: 50 }, + ); + + expect(result).toMatchObject({ + ok: true, + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + data: { + after: { + completed: 1, + total: 1, + failedIds: [], + externalSyncFailures: [ + { + userId: 7, + reason: + "Database applied; emulator sync failed. Do not retry automatically.", + }, + ], + }, + }, + correlationId: "bulk-currency-partial", + }); + expect( + mocks.audit.mock.calls.map((call) => (call[0] as AuditEntry).outcome), + ).toEqual(["intent", "partial"]); + }, + ); + + it.each(["false", "throw"] as const)( + "reports bulk badge database completion separately when RCON reports %s", + async (failureMode) => { + mocks.selectQueue.push([], [{ maxSlot: 2 }]); + if (failureMode === "false") { + mocks.rcon.giveBadge.mockResolvedValue(false); + } else { + mocks.rcon.giveBadge.mockRejectedValue(new Error("RCON unavailable")); + } + const result = await peopleMutationService.execute( + { ...invocation, correlationId: "bulk-badge-partial", legacy: false }, + "users.bulk-badge", + { userIds: [7], badgeCode: "ADM" }, + ); + + expect(result).toMatchObject({ + ok: true, + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + data: { + after: { + completed: 1, + total: 1, + failedIds: [], + externalSyncFailures: [ + { + userId: 7, + reason: + "Database applied; emulator sync failed. Do not retry automatically.", + }, + ], + }, + }, + correlationId: "bulk-badge-partial", + }); + expect( + mocks.audit.mock.calls.map((call) => (call[0] as AuditEntry).outcome), + ).toEqual(["intent", "partial"]); + }, + ); + + it("keeps best-effort notification outside update and reset completion", async () => { mocks.selectQueue.push([target()]); const updated = await peopleMutationService.execute( - { ...invocation, correlationId: "update-notify-partial", legacy: false }, + { ...invocation, correlationId: "update-notify", legacy: false }, "user.update", { userId: 7, fields: { motto: "Updated" } }, ); - expect(updated).toMatchObject({ - ok: true, - completion: { - status: "partial", - external: "failed", - audit: "persisted", - }, - correlationId: "update-notify-partial", - }); + expect(updated).toMatchObject({ ok: true, correlationId: "update-notify" }); + expect(updated).not.toHaveProperty("completion"); expect( mocks.audit.mock.calls.map((call) => (call[0] as AuditEntry).outcome), - ).toEqual(["intent", "partial"]); + ).toEqual(["intent", "success"]); vi.clearAllMocks(); mocks.resolveServerContext.mockResolvedValue(context()); mocks.audit.mockResolvedValue(undefined); mocks.hashPassword.mockResolvedValue("hashed-password"); - mocks.notify.mockRejectedValue(new Error("notification unavailable")); mocks.selectQueue.push([target()]); const reset = await peopleMutationService.execute( - { ...invocation, correlationId: "reset-notify-partial", legacy: false }, + { ...invocation, correlationId: "reset-notify", legacy: false }, "user.reset-password", { userId: 7, reason: "Verified owner" }, ); expect(reset).toMatchObject({ ok: true, - completion: { - status: "partial", - external: "failed", - audit: "persisted", - }, data: { output: { newPassword: expect.any(String) } }, - correlationId: "reset-notify-partial", + correlationId: "reset-notify", }); + expect(reset).not.toHaveProperty("completion"); expect( mocks.audit.mock.calls.map((call) => (call[0] as AuditEntry).outcome), - ).toEqual(["intent", "partial"]); + ).toEqual(["intent", "success"]); + expect(mocks.notify).toHaveBeenCalledTimes(1); }); - it("preserves legacy throw and false semantics while commands retain partial truth", async () => { mocks.selectQueue.push( [target()], diff --git a/src/features/housekeeping/domains/people/services/mutations.ts b/src/features/housekeeping/domains/people/services/mutations.ts index 19b6fc29..24833d68 100644 --- a/src/features/housekeeping/domains/people/services/mutations.ts +++ b/src/features/housekeeping/domains/people/services/mutations.ts @@ -118,6 +118,8 @@ class PeopleMutationFailure extends Error { } } +class ConfirmedExternalNoopFailure extends PeopleMutationFailure {} + function operationCapability(operation: PeopleMutationOperation) { if (operation === "user.ban" || operation === "user.unban") { return anyCapability(PERMS.USERS_BAN); @@ -307,7 +309,7 @@ function userIds(value: unknown, legacy: boolean): number[] { async function requireRcon(result: boolean): Promise { if (!result) { - throw new PeopleMutationFailure( + throw new ConfirmedExternalNoopFailure( "DEPENDENCY_UNAVAILABLE", "errors.housekeeping.dependencyUnavailable", ); @@ -402,10 +404,16 @@ async function finalizeExternalWithAudit( snapshot: PeopleMutationSnapshot, execute: () => Promise, mutationCommitted = true, + confirmedFailureSnapshot?: PeopleMutationSnapshot, ): Promise { try { await execute(); } catch (error) { + const failureSnapshot = mutationCommitted + ? snapshot + : error instanceof ConfirmedExternalNoopFailure + ? (confirmedFailureSnapshot ?? { before: snapshot.before, after: null }) + : { before: snapshot.before, after: null }; let audit: HousekeepingPartialCompletion["audit"] = "persisted"; try { await logAudit( @@ -414,7 +422,7 @@ async function finalizeExternalWithAudit( operation, target, targetId, - snapshot, + failureSnapshot, mutationCommitted ? "partial" : "failure", ), ); @@ -482,6 +490,7 @@ async function runExternalWithAudit( targetId: number | undefined, snapshot: PeopleMutationSnapshot, execute: () => Promise, + confirmedFailureSnapshot: PeopleMutationSnapshot, ): Promise { await logAudit( canonicalAuditEntry( @@ -501,6 +510,7 @@ async function runExternalWithAudit( snapshot, execute, false, + confirmedFailureSnapshot, ); } async function executeUserMutation( @@ -524,6 +534,7 @@ async function executeUserMutation( userId, snapshot, async () => requireRcon(await rcon.alertUser(userId, message)), + { before: null, after: { userId, alertDelivered: false } }, ); } @@ -592,13 +603,12 @@ async function executeUserMutation( snapshot, async () => { invalidateLoginCache(target.username); - const notification = notify({ + void notify({ action: "user_edit", actor: context.capability.actor.username, target: target.username, targetId: userId, }); - if (!context.legacy) await notification; }, ); } @@ -704,7 +714,7 @@ async function executeUserMutation( snapshot, async () => { await requireRcon(await rcon.disconnectUser(userId)); - await notify({ + void notify({ action: "ban", actor: context.capability.actor.username, target: target.username, @@ -787,7 +797,7 @@ async function executeUserMutation( ); return value; }); - notify({ + void notify({ action: "unban", actor: context.capability.actor.username, target: target.username, @@ -805,12 +815,13 @@ async function executeUserMutation( snapshot, async () => { await requireRcon(await rcon.disconnectUser(userId)); - await notify({ + void notify({ action: "disconnect", actor: context.capability.actor.username, target: target.username, }); }, + { before, after: before }, ); } @@ -838,6 +849,7 @@ async function executeUserMutation( await requireRcon(await rcon.unmuteUser(userId)); } }, + { before, after: before }, ); } @@ -877,13 +889,12 @@ async function executeUserMutation( snapshot, async () => { invalidateLoginCache(target.username); - const notification = notify({ + void notify({ action: "user_edit", actor: context.capability.actor.username, target: target.username, details: "Password reset", }); - if (!context.legacy) await notification; }, ); } @@ -898,6 +909,7 @@ async function executeUserMutation( userId, snapshot, async () => requireRcon(await rcon.giveCredits(userId, amount)), + { before, after: before }, ); } @@ -1014,8 +1026,10 @@ async function executeBulkMutation( const data = record(input); const ids = userIds(data.userIds, context.legacy); const failures: Array<{ userId: number; reason: string }> = []; + const externalSyncFailures: Array<{ userId: number; reason: string }> = []; + const externalSyncFailureReason = + "Database applied; emulator sync failed. Do not retry automatically."; let completed = 0; - let externalFailed = false; let mutationReason: string | undefined; if (operation === "users.bulk-unban") { @@ -1126,16 +1140,12 @@ async function executeBulkMutation( | "pixels" | "points"; for (const userId of ids) { - let databaseCommitted = false; try { if (type === "credits") { await db .update(User) .set({ credits: sql`${User.credits} + ${amount}` }) .where(eq(User.id, userId)); - databaseCommitted = true; - const delivered = await rcon.giveCredits(userId, amount); - if (!context.legacy) await requireRcon(delivered); } else { const currencyType = type === "pixels" ? 0 : 101; await db @@ -1144,19 +1154,25 @@ async function executeBulkMutation( .onDuplicateKeyUpdate({ set: { amount: sql`${UsersCurrency.amount} + ${amount}` }, }); - databaseCommitted = true; - if (type === "pixels") { - const delivered = await rcon.giveDuckets(userId, amount); - if (!context.legacy) await requireRcon(delivered); - } else { - const delivered = await rcon.givePointsGotw(userId, amount); - if (!context.legacy) await requireRcon(delivered); - } } - completed += 1; } catch { - if (databaseCommitted) externalFailed = true; failures.push({ userId, reason: "Database error" }); + continue; + } + completed += 1; + try { + const delivered = + type === "credits" + ? await rcon.giveCredits(userId, amount) + : type === "pixels" + ? await rcon.giveDuckets(userId, amount) + : await rcon.givePointsGotw(userId, amount); + if (!context.legacy) await requireRcon(delivered); + } catch { + externalSyncFailures.push({ + userId, + reason: externalSyncFailureReason, + }); } } await logStaffActivity({ @@ -1168,7 +1184,7 @@ async function executeBulkMutation( } else { const badgeCode = normalizedText(data.badgeCode, 20); for (const userId of ids) { - let databaseCommitted = false; + let shouldSync = false; try { const [existing] = await db .select({ id: UsersBadges.id }) @@ -1190,14 +1206,22 @@ async function executeBulkMutation( slotId: (aggregate?.maxSlot ?? 0) + 1, badgeCode, }); - databaseCommitted = true; - const delivered = await rcon.giveBadge(userId, badgeCode); - if (!context.legacy) await requireRcon(delivered); + shouldSync = true; } completed += 1; } catch { - if (databaseCommitted) externalFailed = true; failures.push({ userId, reason: "Database error" }); + continue; + } + if (!shouldSync) continue; + try { + const delivered = await rcon.giveBadge(userId, badgeCode); + if (!context.legacy) await requireRcon(delivered); + } catch { + externalSyncFailures.push({ + userId, + reason: externalSyncFailureReason, + }); } } await logStaffActivity({ @@ -1208,11 +1232,13 @@ async function executeBulkMutation( }); } + const hasPartialCompletion = + failures.length > 0 || externalSyncFailures.length > 0; const completion: HousekeepingPartialCompletion | undefined = - failures.length > 0 + hasPartialCompletion ? { status: "partial", - external: externalFailed ? "failed" : "not-required", + external: externalSyncFailures.length > 0 ? "failed" : "not-required", audit: "persisted", } : undefined; @@ -1222,6 +1248,7 @@ async function executeBulkMutation( completed, total: ids.length, failedIds: failures, + ...(externalSyncFailures.length > 0 ? { externalSyncFailures } : {}), ...(mutationReason ? { reason: mutationReason } : {}), }, ...(completion === undefined ? {} : { completion }), @@ -1234,7 +1261,7 @@ async function executeBulkMutation( "User", undefined, snapshot, - failures.length > 0 ? "partial" : "success", + hasPartialCompletion ? "partial" : "success", ), ); } catch (auditOutcomeError) { diff --git a/src/features/housekeeping/foundation/commands/dispatcher.test.ts b/src/features/housekeeping/foundation/commands/dispatcher.test.ts index e53240f2..af8f760b 100644 --- a/src/features/housekeeping/foundation/commands/dispatcher.test.ts +++ b/src/features/housekeeping/foundation/commands/dispatcher.test.ts @@ -952,6 +952,22 @@ describe("dispatchHousekeepingCommand", () => { "invalid failure error", { ok: false, error: null, correlationId: "forged" }, ], + [ + "failure completion metadata", + { + ok: false, + error: { + code: "DEPENDENCY_UNAVAILABLE", + messageKey: "errors.housekeeping.dependencyUnavailable", + }, + correlationId: "forged", + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + }, + ], [ "throwing getter", Object.defineProperty({}, "ok", { diff --git a/src/features/housekeeping/foundation/commands/dispatcher.ts b/src/features/housekeeping/foundation/commands/dispatcher.ts index 051ecb8d..d4d60158 100644 --- a/src/features/housekeeping/foundation/commands/dispatcher.ts +++ b/src/features/housekeeping/foundation/commands/dispatcher.ts @@ -71,13 +71,6 @@ const housekeepingResultSchema = z.discriminatedUnion("ok", [ ok: z.literal(false), error: housekeepingErrorSchema, correlationId: z.string().min(1).max(160), - completion: z - .strictObject({ - status: z.literal("partial"), - external: z.enum(["not-required", "completed", "failed"]), - audit: z.enum(["persisted", "unavailable"]), - }) - .optional(), }), ]);