fix(housekeeping): restore people partial compatibility
This commit is contained in:
Simo committed
1 parent
fcd1dfd96e
commit
a6a288d9ff
6 files changed
+412
-11
No files matched your search
@@ -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.
|
||||
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.
|
||||
@@ -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" }],
|
||||
},
|
||||
});
|
||||
});
|
||||
@@ -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(),
|
||||
);
|
||||
},
|
||||
);
|
||||
});
|
||||
@@ -52,6 +52,35 @@ function failedIds(value: unknown): Array<{ userId: number; reason: string }> {
|
||||
: [];
|
||||
}
|
||||
|
||||
function legacyBulkOutcome(
|
||||
after: Readonly<Record<string, unknown>> | 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,
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -59,6 +59,14 @@ function parseField(field: PeopleCommandField, formData: FormData): unknown {
|
||||
}
|
||||
|
||||
const initialState: HousekeepingResult<unknown> | 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<unknown>;
|
||||
}) {
|
||||
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 (
|
||||
<div
|
||||
role="status"
|
||||
role={externalSyncFailures.length > 0 ? "alert" : "status"}
|
||||
className="space-y-1 text-xs text-[var(--admin-text-muted)]"
|
||||
>
|
||||
<p>{message}</p>
|
||||
{externalSyncFailures.length > 0 ? (
|
||||
<>
|
||||
<p>
|
||||
<strong>Do not retry automatically.</strong> The database updates
|
||||
were applied; retrying may duplicate them.
|
||||
</p>
|
||||
<ul
|
||||
aria-label="External synchronization debt"
|
||||
className="list-disc space-y-1 pl-5"
|
||||
>
|
||||
{externalSyncFailures.map((failure) => (
|
||||
<li key={failure.key}>
|
||||
User #{failure.userId}: {failure.reason}
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
</>
|
||||
) : null}
|
||||
{generatedPassword ? (
|
||||
<p>
|
||||
Generated temporary password:{" "}
|
||||
@@ -119,6 +146,42 @@ export function PeopleCommandResult({
|
||||
);
|
||||
}
|
||||
|
||||
function readExternalSyncFailures(
|
||||
result: HousekeepingResult<unknown>,
|
||||
): 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<number, number>();
|
||||
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<unknown>,
|
||||
): string | null {
|
||||
|
||||
@@ -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(
|
||||
<PeopleCommandResult
|
||||
result={ok(
|
||||
{
|
||||
before: { completed: 0, total: 2, failedIds: [] },
|
||||
after: {
|
||||
completed: 2,
|
||||
total: 2,
|
||||
failedIds: [],
|
||||
externalSyncFailures: [
|
||||
{
|
||||
userId: 7,
|
||||
reason:
|
||||
"Database applied; emulator sync failed. Do not retry automatically.",
|
||||
},
|
||||
{
|
||||
userId: "8",
|
||||
reason: "unsafe payload from an untyped result",
|
||||
},
|
||||
],
|
||||
debug: "sensitive internal payload",
|
||||
},
|
||||
completion,
|
||||
},
|
||||
"bulk-sync-debt",
|
||||
completion,
|
||||
)}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(<PeopleCommandResult result={result} />);
|
||||
|
||||
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",
|
||||
|
||||
Reference in new issue
Block a user