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 5999ed1f..6a993373 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 @@ -307,4 +307,98 @@ 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 +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 4 + +The round-3 re-review reported 0 Critical, 2 Important, and 0 Minor findings. This round restores the pre-cutover legacy bulk result contract at the wrapper boundary and makes committed-database/emulator-sync debt explicit in the successful partial operator result. Canonical Housekeeping accounting, audit evidence, routes, commands, and ACLs remain unchanged. + +### Pre-Task12 parity evidence + +`git show e1b31ff7^:src/actions/bulk-users.ts` confirms that currency and badge wrappers awaited RCON inside the same `try`: an RCON exception entered the catch, did not increment `given`, and appended `{ userId, reason: "Database error" }`; an RCON `false` return did not throw and therefore remained a legacy success. Positive bulk adjustment delegated to the same currency wrapper and had the same result semantics. + +### RED evidence + +```text +pnpm exec vitest run --coverage.enabled=false src/actions/bulk-users.test.ts src/actions/bulk-adjust-wrapper.test.ts +Test Files 2 failed (2) +Tests 3 failed | 3 passed (6) +Currency, badge, and positive-adjust wrappers returned given/adjusted=1 with no failedIds for the canonical external-sync debt produced by a thrown RCON call; historical results require 0 plus Database error. + +pnpm exec vitest run --coverage.enabled=false src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx +Test Files 1 failed (1) +Tests 2 failed | 12 passed (14) +The operator result rendered only Partially completed and exposed neither an alert/do-not-retry instruction nor the typed user sync debt returned by the real form adapter. +``` + +### GREEN implementation + +- `src/actions/bulk-users.ts` translates only `externalSyncFailures` at the legacy wrapper boundary into historical `Database error` failures and subtracts those entries from `given`/positive `adjusted`. Canonical completed counts and sync-debt evidence are untouched; the existing legacy `false` path still produces no external-sync entry and remains successful. Failure entries are restored in input order, including duplicate IDs. +- `src/features/housekeeping/domains/people/pages/people-command-form.tsx` reads only a successful typed partial result with failed external completion and a bounded `after.externalSyncFailures` array. It renders an alert, explicit do-not-retry instruction, and safe user/reason debt entries. Unknown payload fields and malformed entries are never rendered, and the generated reset password path remains one-time and unchanged. + +Focused GREEN: + +```text +pnpm exec vitest run --coverage.enabled=false src/actions/bulk-users.test.ts src/actions/bulk-adjust-wrapper.test.ts +Test Files 2 passed (2) +Tests 6 passed (6) + +pnpm exec vitest run --coverage.enabled=false src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx +Test Files 1 passed (1) +Tests 14 passed (14) + +pnpm exec vitest run --coverage.enabled=false src/actions/bulk-users.test.ts src/actions/bulk-adjust-wrapper.test.ts src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts +Test Files 4 passed (4) +Tests 48 passed (48) +``` + +### Cumulative verification + +```text +People + foundation + Task12 legacy wrappers + route/audit/staff-smoke matrix +Test Files 52 passed (52) +Tests 491 passed (491) + +pnpm test:housekeeping +Test Files 58 passed (58) +Tests 518 passed (518) + +pnpm test +Test Files 206 passed | 3 skipped (209) +Tests 1364 passed | 5 skipped (1369) + +pnpm typecheck +tsc --noEmit +Exit 0 + +pnpm exec biome check --formatter-enabled=false src/actions/bulk-users.ts src/actions/bulk-users.test.ts src/actions/bulk-adjust-wrapper.test.ts src/features/housekeeping/domains/people/pages/people-command-form.tsx src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx +Checked 5 files. No fixes applied. + +git diff --check +Exit 0 +``` + +The only warning is the approved Node engine mismatch: the repository requests Node `>=26.8.1 <27`, while the host runs Node `v26.7.0` with pnpm `11.24.0`. + +### Exact tracked paths + +- `.superpowers/sdd/2026-08-26-housekeeping-completion/task-12-report.md` +- `src/actions/bulk-adjust-wrapper.test.ts` +- `src/actions/bulk-users.test.ts` +- `src/actions/bulk-users.ts` +- `src/features/housekeeping/domains/people/pages/people-command-form.tsx` +- `src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx` + +The required controller lines were appended to the git-ignored `.superpowers/sdd/2026-08-26-housekeeping-completion/progress.md`; it is excluded from the commit. `.remember/` remains untouched. + +### Self-review + +- Scope and compatibility: the production mutation service, canonical audit/accounting, manifest, route, command, ACL, database, redirect, and cutover behavior are unchanged. The adapter applies only to legacy currency/badge results and their historical positive-adjust delegate. +- Security: the operator surface requires an `ok: true` partial/external-failed envelope, accepts at most 100 positive safe-integer user IDs, renders only the canonical safe reason, and does not inspect or serialize arbitrary result payloads. Reset-password display and audit redaction tests remain green. +- Test quality: legacy tests exercise the real exported wrappers against a complete canonical partial response and fail on either wrong count or missing historical failure; UI tests render the real component, invoke the real form adapter, and prove malformed/extra payload is not displayed. + +### Commit + +Single local commit message: `fix(housekeeping): restore people partial compatibility`. The final SHA of the commit containing this report is returned to the controller after creation. + +No database operation, deployment, push, pull, or pull-request update was performed. diff --git a/src/actions/bulk-adjust-wrapper.test.ts b/src/actions/bulk-adjust-wrapper.test.ts index e06238aa..8b8d6d7a 100644 --- a/src/actions/bulk-adjust-wrapper.test.ts +++ b/src/actions/bulk-adjust-wrapper.test.ts @@ -54,3 +54,46 @@ it("keeps one ACL check while delegating a positive bulk adjustment", async () = { userIds: [7, 8], amount: 25, type: "credits" }, ); }); + +it("keeps the pre-Task12 positive-adjust result when currency RCON throws after the database commit", async () => { + execute.mockResolvedValueOnce({ + ok: true, + data: { + before: { completed: 0, total: 1, failedIds: [] }, + after: { + completed: 1, + total: 1, + failedIds: [], + externalSyncFailures: [ + { + userId: 7, + reason: + "Database applied; emulator sync failed. Do not retry automatically.", + }, + ], + }, + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + }, + correlationId: "legacy-positive-adjust", + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + }); + + await expect( + bulkAdjustCurrency({ userIds: [7], amount: 25, type: "credits" }), + ).resolves.toEqual({ + ok: true, + data: { + adjusted: 0, + total: 1, + failedIds: [{ userId: 7, reason: "Database error" }], + }, + }); +}); diff --git a/src/actions/bulk-users.test.ts b/src/actions/bulk-users.test.ts index 92e712e6..08d67f3d 100644 --- a/src/actions/bulk-users.test.ts +++ b/src/actions/bulk-users.test.ts @@ -91,4 +91,64 @@ describe("legacy bulk user wrappers", () => { { userId: 9, untilUnix: 1234 }, ); }); + + it.each([ + [ + "currency", + "users.bulk-currency", + () => bulkGiveCurrency({ userIds: [7], amount: 100, type: "credits" }), + ], + [ + "badge", + "users.bulk-badge", + () => bulkGiveBadge({ userIds: [7], badgeCode: "ADM" }), + ], + ] as const)( + "restores the pre-Task12 legacy result when %s RCON throws after the database commit", + async (_kind, operation, invoke) => { + execute.mockResolvedValueOnce({ + ok: true, + data: { + before: { completed: 0, total: 1, failedIds: [] }, + after: { + completed: 1, + total: 1, + failedIds: [], + externalSyncFailures: [ + { + userId: 7, + reason: + "Database applied; emulator sync failed. Do not retry automatically.", + }, + ], + }, + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + }, + correlationId: "legacy-external-sync-failure", + completion: { + status: "partial", + external: "failed", + audit: "persisted", + }, + }); + + await expect(invoke()).resolves.toEqual({ + ok: true, + data: { + given: 0, + total: 1, + failedIds: [{ userId: 7, reason: "Database error" }], + }, + }); + expect(execute).toHaveBeenCalledWith( + expect.anything(), + operation, + expect.anything(), + ); + }, + ); }); diff --git a/src/actions/bulk-users.ts b/src/actions/bulk-users.ts index 400115df..9e1dcdf2 100644 --- a/src/actions/bulk-users.ts +++ b/src/actions/bulk-users.ts @@ -52,6 +52,35 @@ function failedIds(value: unknown): Array<{ userId: number; reason: string }> { : []; } +function legacyBulkOutcome( + after: Readonly> | null, + userIds: readonly number[], +): { + readonly completed: number; + readonly total: number; + readonly failedIds: Array<{ userId: number; reason: string }>; +} { + const databaseFailures = failedIds(after?.failedIds); + const externalSyncFailures = failedIds(after?.externalSyncFailures).map( + ({ userId }) => ({ userId, reason: "Database error" }), + ); + const pendingFailures = [...databaseFailures, ...externalSyncFailures]; + const orderedFailures = userIds.flatMap((userId) => { + const index = pendingFailures.findIndex( + (failure) => failure.userId === userId, + ); + return index === -1 ? [] : pendingFailures.splice(index, 1); + }); + return { + completed: Math.max( + 0, + numberValue(after?.completed) - externalSyncFailures.length, + ), + total: numberValue(after?.total), + failedIds: [...orderedFailures, ...pendingFailures], + }; +} + export async function bulkUnban({ userIds, }: { @@ -113,12 +142,13 @@ export async function bulkGiveCurrency({ type, }); if (!result.ok) return { ok: false, error: "Bulk currency failed" }; + const outcome = legacyBulkOutcome(result.data.after, userIds); return { ok: true, data: { - given: numberValue(result.data.after?.completed), - total: numberValue(result.data.after?.total), - failedIds: failedIds(result.data.after?.failedIds), + given: outcome.completed, + total: outcome.total, + failedIds: outcome.failedIds, }, }; } @@ -142,12 +172,13 @@ export async function bulkGiveBadge({ badgeCode, }); if (!result.ok) return { ok: false, error: "Bulk badge failed" }; + const outcome = legacyBulkOutcome(result.data.after, userIds); return { ok: true, data: { - given: numberValue(result.data.after?.completed), - total: numberValue(result.data.after?.total), - failedIds: failedIds(result.data.after?.failedIds), + given: outcome.completed, + total: outcome.total, + failedIds: outcome.failedIds, }, }; } @@ -178,12 +209,13 @@ export async function bulkAdjustCurrency({ type, }); if (!result.ok) return { ok: false, error: "Currency adjustment failed" }; + const outcome = legacyBulkOutcome(result.data.after, userIds); return { ok: true, data: { - adjusted: numberValue(result.data.after?.completed), - total: numberValue(result.data.after?.total), - failedIds: failedIds(result.data.after?.failedIds), + adjusted: outcome.completed, + total: outcome.total, + failedIds: outcome.failedIds, }, }; } diff --git a/src/features/housekeeping/domains/people/pages/people-command-form.tsx b/src/features/housekeeping/domains/people/pages/people-command-form.tsx index fc5993b0..0c06a405 100644 --- a/src/features/housekeeping/domains/people/pages/people-command-form.tsx +++ b/src/features/housekeeping/domains/people/pages/people-command-form.tsx @@ -59,6 +59,14 @@ function parseField(field: PeopleCommandField, formData: FormData): unknown { } const initialState: HousekeepingResult | null = null; +const externalSyncFailureReason = + "Database applied; emulator sync failed. Do not retry automatically."; + +interface ExternalSyncFailure { + readonly key: string; + readonly userId: number; + readonly reason: typeof externalSyncFailureReason; +} export async function submitPeopleCommandForm( configuration: PeopleCommandSubmission, @@ -96,6 +104,7 @@ export function PeopleCommandResult({ readonly result: HousekeepingResult; }) { const generatedPassword = readGeneratedPassword(result); + const externalSyncFailures = readExternalSyncFailures(result); const message = result.ok ? result.completion?.status === "partial" ? `Partially completed (${result.correlationId})` @@ -103,10 +112,28 @@ export function PeopleCommandResult({ : `Failed: ${result.error.messageKey} (${result.correlationId})`; return (
0 ? "alert" : "status"} className="space-y-1 text-xs text-[var(--admin-text-muted)]" >

{message}

+ {externalSyncFailures.length > 0 ? ( + <> +

+ Do not retry automatically. The database updates + were applied; retrying may duplicate them. +

+
    + {externalSyncFailures.map((failure) => ( +
  • + User #{failure.userId}: {failure.reason} +
    • + ))} +
+ + ) : null} {generatedPassword ? (

Generated temporary password:{" "} @@ -119,6 +146,42 @@ export function PeopleCommandResult({ ); } +function readExternalSyncFailures( + result: HousekeepingResult, +): readonly ExternalSyncFailure[] { + if ( + !result.ok || + result.completion?.status !== "partial" || + result.completion.external !== "failed" || + !isRecord(result.data) || + !isRecord(result.data.after) || + !Array.isArray(result.data.after.externalSyncFailures) + ) { + return []; + } + const occurrences = new Map(); + const failures: ExternalSyncFailure[] = []; + for (const value of result.data.after.externalSyncFailures.slice(0, 100)) { + if ( + !isRecord(value) || + typeof value.userId !== "number" || + !Number.isSafeInteger(value.userId) || + value.userId <= 0 || + value.reason !== externalSyncFailureReason + ) { + continue; + } + const occurrence = (occurrences.get(value.userId) ?? 0) + 1; + occurrences.set(value.userId, occurrence); + failures.push({ + key: `${value.userId}:${occurrence}`, + userId: value.userId, + reason: externalSyncFailureReason, + }); + } + return failures; +} + function readGeneratedPassword( result: HousekeepingResult, ): string | null { diff --git a/src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx b/src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx index f6b72ae3..31a79902 100644 --- a/src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx +++ b/src/features/housekeeping/domains/people/pages/people-primary-pages.test.tsx @@ -401,6 +401,115 @@ describe("People actionable form contract", () => { expect(html.match(new RegExp(password, "gu"))).toHaveLength(1); }); + it("renders typed external sync debt with an explicit do-not-retry warning", () => { + const completion = { + status: "partial", + external: "failed", + audit: "persisted", + } as const; + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain('role="alert"'); + expect(html).toContain("Do not retry automatically."); + expect(html).toContain('aria-label="External synchronization debt"'); + expect(html).toContain("User #7"); + expect(html).toContain("Database applied; emulator sync failed."); + expect(html).not.toContain("unsafe payload from an untyped result"); + expect(html).not.toContain("sensitive internal payload"); + }); + + it("keeps form-returned external sync debt visible in the operator result surface", async () => { + const completion = { + status: "partial", + external: "failed", + audit: "persisted", + } as const; + vi.mocked(executeHousekeepingCommand).mockResolvedValue( + ok( + { + before: { completed: 0, total: 1, failedIds: [] }, + after: { + completed: 1, + total: 1, + failedIds: [], + externalSyncFailures: [ + { + userId: 23, + reason: + "Database applied; emulator sync failed. Do not retry automatically.", + }, + ], + }, + completion, + }, + "form-sync-debt", + completion, + ), + ); + const formData = new FormData(); + formData.set("userIds", "23"); + formData.set("amount", "100"); + formData.set("type", "credits"); + formData.set("reason", "Planned grant"); + + const result = await submitPeopleCommandForm( + { + commandId: "people.users.bulk-currency", + input: {}, + fields: [ + { name: "userIds", label: "User IDs", type: "number-list" }, + { name: "amount", label: "Amount", type: "number", min: 1 }, + { + name: "type", + label: "Currency", + type: "select", + options: [{ value: "credits", label: "Credits" }], + }, + ], + requiresReason: true, + }, + null, + formData, + ); + const html = renderToStaticMarkup(); + + expect(executeHousekeepingCommand).toHaveBeenCalledWith({ + commandId: "people.users.bulk-currency", + input: { userIds: [23], amount: 100, type: "credits" }, + reason: "Planned grant", + }); + expect(html).toContain("Do not retry automatically."); + expect(html).toContain("User #23"); + }); + it("provides a real Next loading boundary", () => { const source = readFileSync( "src/app/ase-next/[domain]/[[...segments]]/loading.tsx",