feat(accounts): absorb an accidental alt account, and stop auto-granting green (deploy 1 of 2) - #84
Conversation
Two defects from an accidental login with the wrong character: the stranded account cannot be folded into the operator's real one, and every new account lands on green, which carries a Discord role. Design: linkCharacter absorbs a provably-same-owner source account when it is empty of everything but the character; new accounts start on a fourth tier value, pending, which grants no managed role until an admin approves it to green (unlocked) or blue (locked). Derole and existing greens are unchanged.
…udit claim Review found four defects: - A single deploy is unsafe. Migrations run as a release command before the rolling replacement, so an old worker sees pending accounts written by new web code and transitions them to green, defeating the gate. Split into two deploys: readers learn pending first, account creation switches second. - The absorbability predicate omitted status and status_note, so an admin's cryo and operational note could be silently destroyed by a merge. - Adding pending to TIER_RANK does not surface the queue: the table defaults to name sort. Keep that default and add an explicit pending-count link. - audit_log.actor is plain text with no FK, so it does not null on delete. The uuid survives and renders as actorKind unresolved.
Eleven tasks across two deploys. Tasks 1-10 teach every reader about pending while creation still writes green; task 11 is the single creation line and must ship as a separate release.
- pending is no longer assignable via setTierManual: TIERS (manual set) and TIER_FILTERS (query whitelist + chips) are separate arrays - absorbability rejects payout_payment.actor, reachable by an ex-flygd account - approveAction matches the file's guard contract and per-error handling - the pending notice is an extractable leaf component, not a page render - the queue count is computed independently of the active filters
Appends `pending` to the tier enum and widens every hand-written union that mirrors it. Nothing creates or reads a pending account yet — this is the "every reader learns pending" half of the two-deploy rollout. diffRoles ships in the same commit rather than a follow-up: the enum widening makes `row.tier` four-valued at the call site (src/jobs/discord-roles.ts), so there is no typecheck-clean intermediate state where diffRoles accepts pending but does not handle it. A pending account is one nobody has approved, so it gets no managed role: add nothing, strip whatever it carries.
Green stays unlocked so a later alliance join still auto-promotes; blue locks because an unlocked blue converges back to green on the next membership run.
Default sort stays name; the queue gets its own count link rather than reordering every admin's table.
Linking a character whose owner hash matches but whose account is otherwise empty now folds that account in instead of refusing. Anything with an admin bit, a lock, cryo, a note, a Discord link, payout history, or a second character still refuses.
…or boundary The merge feature (mergeAccountInto) deletes a pending account's row, ignoring tier in isAbsorbable, so an admin's Approve click can now legitimately race against it. Treat not_found the same as the sibling not_pending race: a queue notice, not a thrown error. Corrects two stale comments that predated the merge feature and adds coverage for approveAccount's not_found return.
setTierAction, returnToAutoAction, setStatusAction, saveNoteAction, and promoteAdminAction all threw new Error(result.error) on not_found, landing the admin on the generic error boundary. mergeAccountInto deletes the source account outright and isAbsorbable doesn't gate on tier, so an ordinary account can vanish between page render and any of these clicks — a race between two legitimate users, not a server fault. Route all six actions (these five plus approveAction) through one exhaustive redirectOnMutationError switch, mirroring admin-guard's denyAdmin: a variant added to AdminMutationResult or ApproveResult without a matching case now fails the build instead of silently reaching a throw. Reword the shared not_found notice so it no longer reads as approval-only.
… cost The admin accounts page computed its "N awaiting approval" count by calling getAdminAccountsList a second time and taking .length. That service issues five unbounded full-table scans and assembles a full per-account row, including per-character token/scope/ACL work — ten scans per page load instead of five, to obtain one integer. Add countAccountsByTier, a single count(*) filtered by tier, and have the page call that instead. The count stays independent of the page's active status/tier filters and gated on > 0, as before.
…e copy The redirectOnMutationError doc comment claimed a new error variant would fail the build with "not every code path returns a value". A mutation experiment showed the first error is actually TS2345 at the four call sites, because the widened error type stops being assignable to the helper's parameter; the end-point error only appears once the parameter union is widened too. The guarantee is real, the stated mechanism was not. Also trims "nothing left there to act on" to "nothing left to act on".
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a ChangesAccount lifecycle changes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
# Conflicts: # e2e/admin.spec.ts
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-08-04-account-merge-and-pending-tier.md`:
- Around line 320-331: Add blank lines immediately before the opening TypeScript
fence and after its closing fence in the approveAccount documentation section,
without changing the fenced code content.
- Around line 1217-1221: Update the Step 2 verification command in the
account-merge and pending-tier plan to search only src/services/accounts.ts and
the account-creation write path, excluding reader declarations in files such as
account-view.ts and role-diff.ts. Preserve the check’s purpose of detecting
premature pending-tier writes and keep its expected no-match result accurate.
In `@docs/superpowers/specs/2026-08-04-account-merge-and-pending-tier-design.md`:
- Around line 121-127: Update the migration/design statement around
createAccountWithCharacter to describe the deploy-1 target accurately: account
creation continues writing tier "green" until deploy 2. Mark the explicit tier:
"pending" behavior as a deploy-2 change, keeping the migration’s enum-only
update and rollout sequence consistent.
- Around line 21-26: Update the fenced code block in the account-merge design
document to declare the TypeScript language using the ts fence label, preserving
the block’s existing content.
In `@src/app/admin/accounts/actions.ts`:
- Around line 39-68: Update demoteAdminAction to preserve its existing
last_admin handling, then pass any remaining mutation errors, including
not_found, to the shared redirectOnMutationError helper instead of throwing.
Ensure stale revoke requests use the defined not_found redirect behavior.
In `@tests/account-view.test.ts`:
- Around line 327-335: Update the tier-sorting test around getAdminAccountsList
to seed an account with the blue tier and include blue in the expected sorted
tier sequence, ensuring pending is verified ahead of blue as well as green and
flygd.
In `@tests/accounts.test.ts`:
- Around line 344-371: Update the merge audit test around handleEveLogin and
linkCharacter to insert an auditLog row authored by stray.accountId before the
merge, then query auditLog by actor equal to stray.accountId instead of
filtering by target. Preserve the existing account.merged assertions and verify
the source-authored row remains unresolved after the merge.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c4a2652-077b-498c-bd3b-6b992fefd66a
📒 Files selected for processing (28)
docs/superpowers/plans/2026-08-04-account-merge-and-pending-tier.mddocs/superpowers/specs/2026-08-04-account-merge-and-pending-tier-design.mddrizzle/0006_powerful_infant_terrible.sqldrizzle/meta/0006_snapshot.jsondrizzle/meta/_journal.jsone2e/account.spec.tse2e/admin.spec.tse2e/helpers.tssrc/app/_components/ui.tsxsrc/app/account/page.tsxsrc/app/account/standing.tsxsrc/app/admin/accounts/actions.tssrc/app/admin/accounts/page.tsxsrc/app/globals.csssrc/core/role-diff.tssrc/core/tier.tssrc/db/schema.tssrc/lib/admin-guard.tssrc/services/account-view.tssrc/services/accounts.tssrc/services/admin-accounts.tstests/account-page.test.tstests/account-view.test.tstests/accounts.test.tstests/admin-accounts.test.tstests/helpers/seed.tstests/role-diff.test.tstests/tier.test.ts
| ``` | ||
| src/services/accounts.ts:244 | ||
| if (existing.ownerHash === ch.ownerHash) { | ||
| return { ok: false, error: "already_linked" }; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add a language to the fenced block.
markdownlint reports MD040 for this block. The block quotes TypeScript.
📝 Proposed fix
-```
+```ts
src/services/accounts.ts:244
if (existing.ownerHash === ch.ownerHash) {
return { ok: false, error: "already_linked" };
}</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-08-04-account-merge-and-pending-tier-design.md`
around lines 21 - 26, Update the fenced code block in the account-merge design
document to declare the TypeScript language using the ts fence label, preserving
the block’s existing content.
Source: Linters/SAST tools
| * `not_found` is reachable from every action here, not just approval: | ||
| * mergeAccountInto (services/accounts.ts) deletes the source account outright | ||
| * on merge, and isAbsorbable deliberately doesn't gate on tier, so any admin | ||
| * control targeting an account can find the row gone between page render and | ||
| * the click — a race between two legitimate users, not a server fault. | ||
| * | ||
| * `fromQueue` sends `not_found` back to the pending-tier filter rather than | ||
| * the unfiltered list: only approveAction's callers were looking at that | ||
| * filter when they clicked. `not_pending` always goes there regardless of the | ||
| * flag, since it can only ever be produced by approveAccount. | ||
| */ | ||
| function redirectOnMutationError( | ||
| error: "not_authorized" | "not_found" | "not_pending", | ||
| opts: { fromQueue?: boolean } = {}, | ||
| ): never { | ||
| switch (error) { | ||
| case "not_authorized": | ||
| return redirectNotAdmin(); | ||
| case "not_pending": | ||
| // Two admins working the queue, or one with a stale tab: the account is | ||
| // approved, just not by them. | ||
| return redirect("/admin/accounts?tier=pending&error=not_pending"); | ||
| case "not_found": | ||
| return redirect( | ||
| opts.fromQueue | ||
| ? "/admin/accounts?tier=pending&error=not_found" | ||
| : "/admin/accounts?error=not_found", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Route demoteAdminAction failures through the shared handler.
Line 39 states that a merge can delete the target of any admin action. demoteAdminAction still throws for not_found. A stale revoke request then returns a 500 instead of the defined not_found redirect. Preserve last_admin handling, then route the remaining failures through redirectOnMutationError.
Proposed fix
export async function demoteAdminAction(accountId: string): Promise<void> {
const { accountId: actor } = await requireAdminAction();
const result = await getDb().transaction((tx) => demoteAdmin(tx, actor, accountId));
- if (!result.ok && result.error === "last_admin") {
- // Surface the service's protection instead of a 500 (carry-over).
- redirect("/admin/accounts?error=last_admin");
- }
- if (!result.ok && result.error === "not_authorized") redirectNotAdmin();
- if (!result.ok) throw new Error(result.error);
+ if (!result.ok) {
+ if (result.error === "last_admin") {
+ return redirect("/admin/accounts?error=last_admin");
+ }
+ if (result.error === undefined) {
+ throw new Error("demoteAdmin: ok:false without an error code");
+ }
+ return redirectOnMutationError(result.error);
+ }
revalidatePath("/admin/accounts");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/admin/accounts/actions.ts` around lines 39 - 68, Update
demoteAdminAction to preserve its existing last_admin handling, then pass any
remaining mutation errors, including not_found, to the shared
redirectOnMutationError helper instead of throwing. Ensure stale revoke requests
use the defined not_found redirect behavior.
| it("audits the merge and leaves the source's own audit rows unresolved", async () => { | ||
| const main = await seedAccount(ctx.db, { tier: "flygd" }); | ||
| await seedCharacter(ctx.db, cfg, { id: 90000331, accountId: main.id, main: true }); | ||
| const stray = await ctx.db.transaction((tx) => | ||
| handleEveLogin(tx, cfg, ch({ characterId: 90000332, ownerHash: "oh-332" })), | ||
| ); | ||
|
|
||
| await ctx.db.transaction((tx) => | ||
| linkCharacter(tx, cfg, main.id, ch({ characterId: 90000332, ownerHash: "oh-332" })), | ||
| ); | ||
|
|
||
| const merged = await ctx.db | ||
| .select() | ||
| .from(auditLog) | ||
| .where(eq(auditLog.action, "account.merged")); | ||
| expect(merged).toHaveLength(1); | ||
| expect(merged[0].details).toEqual({ | ||
| sourceAccountId: stray.accountId, | ||
| characterId: 90000332, | ||
| }); | ||
| // audit_log.actor is plain text with no FK: rows the deleted account wrote | ||
| // survive with a uuid that resolves to nothing (actorKind "unresolved"). | ||
| const orphaned = await ctx.db | ||
| .select() | ||
| .from(auditLog) | ||
| .where(eq(auditLog.target, stray.accountId)); | ||
| expect(orphaned.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the audit rows written during account creation and character linking.
ast-grep run --pattern 'logAudit($$$)' --lang typescript src/services/accounts.tsRepository: guarzo/authGD
Length of output: 3578
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- accounts service symbols ---'
ast-grep outline src/services/accounts.ts
printf '%s\n' '--- relevant service sections ---'
sed -n '80,230p' src/services/accounts.ts
sed -n '300,430p' src/services/accounts.ts
printf '%s\n' '--- test context and adjacent tests ---'
sed -n '250,380p' tests/accounts.test.ts
printf '%s\n' '--- audit schema and helper definitions ---'
rg -n -C 8 'auditLog|audit_log|function logAudit|const logAudit' src testsRepository: guarzo/authGD
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/services/accounts.ts").read_text()
for name, end in [
("handleEveLogin", "async function isAbsorbable"),
("mergeAccountInto", "export async function linkCharacter"),
("maybeGrantBootstrapAdmin", "export async function reclaimTransferredCharacter"),
]:
start = source.index(name)
stop = source.index(end, start)
block = source[start:stop]
print(f"--- {name} ---")
for match in re.finditer(r"logAudit\s*\(\s*[\w.]+,\s*\{(?P<body>.*?)\}\s*\)", block, re.S):
body = " ".join(match.group("body").split())
print(body)
PY
printf '%s\n' '--- test helpers that can create audit actors ---'
rg -n -C 5 'function seedAccount|const seedAccount|seedAccount|insert\(auditLog\)|wakeSelf|handleEveLogin|linkCharacter' tests/accounts.test.ts | head -180Repository: guarzo/authGD
Length of output: 8339
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- handleEveLogin and absorbability flow ---'
sed -n '228,320p' src/services/accounts.ts
printf '%s\n' '--- test seed helpers ---'
fd -i 'seed' tests
rg -n -C 12 'export async function seedAccount|export const seedAccount|async function seedAccount|insert\(auditLog\)' tests
printf '%s\n' '--- direct audit inserts in account tests ---'
rg -n -C 4 'auditLog\)\.values|insert\(auditLog\)' tests/accounts.test.ts tests/helpersRepository: guarzo/authGD
Length of output: 5389
Seed a source-authored audit row before the merge.
handleEveLogin records account.created with actor: "system", so filtering on auditLog.actor = stray.accountId finds no row in this scenario. Insert an audit row with actor: stray.accountId before linkCharacter, then query auditLog.actor to test unresolved source actors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/accounts.test.ts` around lines 344 - 371, Update the merge audit test
around handleEveLogin and linkCharacter to insert an auditLog row authored by
stray.accountId before the merge, then query auditLog by actor equal to
stray.accountId instead of filtering by target. Preserve the existing
account.merged assertions and verify the source-authored row remains unresolved
after the merge.
…eploy 1 CodeRabbit review of #84. The tier-sort fixture seeded green/pending/flygd but never blue, so a regression ranking blue ahead of pending passed. Verified by mutation: with TIER_RANK blue:0 the strengthened assertion fails with ['blue','pending','flygd','green']; the old fixture passed unchanged. The spec said createAccountWithCharacter writes tier: "pending", which contradicts its own Rollout section and this PR — a reader following that paragraph alone would make the deploy-2 change during deploy 1. The plan's step-2 check grepped src/ for 'tier: "pending"' expecting no match, but deploy 1 deliberately adds readers of the pending tier, so the pattern matches type annotations and the queue-count filter arg on a correct tree. Points at the two account write sites instead.
|
Thanks — five of these were real and are fixed in 1d288b0. Rejecting the
The contrast with Worth noting the one real (pre-existing, out of scope) wrinkle your comment surfaced: demoting an already-deleted account would write a spurious |
|
@coderabbitai review Two follow-up commits since the last pass:
The |
|
I will also re-evaluate the ✅ Action performedReview finished.
|
1 participant
Do not merge the follow-up PR until this one is live in production.
This is the expand half of an expand/migrate/contract rollout. It teaches the
whole system to handle a
pendingaccount without any code path creatingone. Deploy 2 is a one-line change making
createAccountWithCharacterwritetier: "pending"instead of"green".The split exists because Fly does a rolling replacement: during the overlap
window, old and new instances serve traffic simultaneously. If creation and
handling shipped together, an old instance could load an account whose tier is
a value its enum doesn't know. It also matters at the database level —
Postgres cannot use an enum value in the same transaction that adds it, which
is why the migration only appends the label.
What this fixes
Two defects, one branch, because they share the account state machine:
separate account with no way to link it to the main one. A single-character
account with no history is now absorbed into the account that already owns
the character, rather than stranded.
auto-granted green, which carries a Discord role. Green now has to be
granted, not assumed — but see the deploy note above: nothing lands in
pendinguntil deploy 2.Deploy-1 safety
Verified on the merged tree, not assumed:
pending. Both account tier writes still say"green"(
src/services/accounts.ts:163,:207), andtests/accounts.test.tspinsthe creation invariant so deploy 2 cannot land here by accident.
ALTER TYPE "public"."tier" ADD VALUE 'pending';. NoUPDATE, noSET DEFAULT, no backfill. The column default staysgreen. An accountthat is green when this ships stays green; a migration touching rows would
strip a genuinely deroled ex-member's Discord access on deploy.
pendingcomparisons, so existing accounts don't move.The merge is the risky part — review it first
mergeAccountInto(src/services/accounts.ts:321-343) contains the only codein the repo that deletes an
accountrow. It is gated byisAbsorbable(
:265-295), which refuses an account that is admin, tier-locked, non-active,annotated, or that carries a Discord link, payout participation, an operation
it created, or a payment it actioned — that last check exists so a formerly
flygd account can't satisfy the predicate and lose financial attribution.
Two things a reviewer should know:
isAbsorbabledeliberately does not inspect tier. That is intentional,but it means an ordinary green single-character account is absorbable today,
which is why the admin actions needed the
not_foundhandling below.:335is load-bearing and must precede the accountdelete:
session.accountIdis notNull with noonDelete.Safety rests on EVE's
ownerHash, which rotates on character transfer — atransferred character cannot be used to absorb the previous owner's account.
Verification
All run on the merged tree (main was merged in; it had advanced by two commits
that touch
services/accounts.ts):npm testnpm run test:e2enpm run typechecknpm run lint<img>warnings)npm run format:checkKnown follow-ups (deliberately not in scope)
account.mergedandtier.approvedaren't in the audit page'ssummarize.tsaction map. They fall through to its documented generickey=value rendering, so this is cosmetic, not a break.
promoteAdmin's return type could be tightened to a proper discriminatedunion.
Summary by CodeRabbit
New Features
Bug Fixes
Tests