Plan 2: Sync Engine — pg-boss worker, outbox dispatcher, five sync jobs - #2
Merged
Conversation
…claim, rotation CAS, queue policy, X-Pages, subject binding, retry classification, worker-routing test)
…ck + strict schema, fail-closed payloads, schedule singleton keys, relink resync)
Removing a non-desired blocked member restored access via any inert corp/alliance ACL entry, effectively un-banning them. Blocked access must only ever be lifted deliberately via unblock. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
verifyEveAccessToken ran outside any try/catch, so one character with a bad token (missing claim, malformed subject) threw and aborted the loop, permanently skipping every later character. Now a deterministic EveSsoError marks that character invalid (guarded on tokenEnc, like the existing subject-mismatch path) and continues; anything else (JWKS/network trouble) counts as transient with no state change. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
The non-retry failed branch returned silently, so pg-boss saw a handled job and never dead-lettered it — a rotated API key would cause a silent, permanent outage. Now mirrors discord-roles.ts's permanent-config path: posts to the ops webhook before returning. Threaded cfg (and optional fetchImpl for tests) into runWandererJob's deps. Transient read failures still throw and retry without alerting. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
A biomassed (affiliation_invalid) character stayed in the desired set and ESI 400-rejected it during addContacts; the shared try/catch then skipped both edits and deletes for every remaining target in the run. Fixed on both sides: getFlygdCharacters now excludes affiliation_invalid characters (they can't be valid contact targets or ids), and the delete step runs in its own try/catch per character so removals survive a failed add/edit — the add/edit failure is still classified and recorded as before. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
Optional counts field needed an optional-chain access, and the mocked fetch needed explicit parameter types for the mock.calls tuple. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
📝 WalkthroughWalkthroughThe pull request adds a background synchronization engine. It adds external service clients, synchronization jobs, token and tier processing, transactional outbox dispatch, pg-boss workers, retry tracking, and integration tests. ChangesSynchronization engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MembershipJob
participant Database
participant OutboxDispatcher
participant PgBoss
participant SyncJobs
participant ExternalAPIs
MembershipJob->>ExternalAPIs: resolve affiliations
MembershipJob->>Database: persist tier changes and outbox events
OutboxDispatcher->>Database: claim undispatched events
OutboxDispatcher->>PgBoss: enqueue coalesced jobs
PgBoss->>SyncJobs: deliver validated payloads
SyncJobs->>ExternalAPIs: synchronize contacts, ACLs, and roles
SyncJobs->>Database: persist outcomes and audit events
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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 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 `@src/core/chunk.ts`:
- Around line 1-5: Update chunk to validate size before the loop and reject
non-positive values, ensuring invalid inputs cannot enter the non-advancing
iteration; preserve the existing slicing behavior for positive sizes.
In `@src/core/role-diff.ts`:
- Around line 49-56: Update the role permission check in the surrounding
role-diff function to accept optional everyoneRoleId, include the matching
`@everyone` role in the permission union even when it is absent from
input.botRoleIds, and parse each role’s permissions defensively so malformed
non-numeric values do not throw; treat invalid values as lacking permissions and
preserve the existing failure result when no valid role grants Manage Roles or
Administrator.
In `@src/jobs/contacts.ts`:
- Around line 135-141: Update the grouping logic in the diff.update loop to
normalize each contact’s labelIds before generating the Map key, such as by
sorting a copied array while preserving the original data. Use the normalized
labels consistently in the group’s labelIds so identical label sets share one
group and produce a single editContacts call.
In `@src/jobs/discord-roles.ts`:
- Around line 61-76: Update the unlink strip flow around getGuildMember and
removeMemberRole to classify DiscordApiError failures using the same
permanent-versus-transient handling as the account loop near lines 137-143.
Return a failed or partial result for permanent errors such as 403 or 404, while
rethrowing transient errors so pg-boss can retry; preserve the existing
successful role-removal and audit behavior.
In `@src/jobs/membership.ts`:
- Around line 35-45: Replace the per-row update loop in the membership
resolution flow with batched updates: group entries from outcome.resolved by the
(corporationId, allianceId) pair, then issue one character update per group
using a WHERE id IN (...) predicate while preserving affiliationCheckedAt and
affiliationInvalid values.
In `@src/jobs/purge.ts`:
- Around line 30-33: Update the purge predicate in the outbox cleanup flow to
reuse the existing now value captured earlier instead of calling Date.now()
again, and compare outbox.dispatchedAt against the retention cutoff rather than
outbox.createdAt. Preserve the isNotNull(outbox.dispatchedAt) guard so only
dispatched rows older than the retention period are removed.
- Around line 14-35: The three purge deletes for sessions, oauth transactions,
and outbox rows currently return deleted records unnecessarily. Remove
`.returning(...)` from each delete, retain their existing filters, and use each
delete result’s `rowCount ?? 0` when calculating purge counts.
In `@src/jobs/token-health.ts`:
- Around line 18-27: Update the token-health scan around the character query and
processing loop to fetch characters in bounded chunks instead of loading all
rows at once, and process each chunk with a fixed concurrency limit rather than
sequentially awaiting every token refresh. Order batches by the characters’
last-check timestamp so older or least-recently checked records are handled
first, while preserving the existing counts and token-status handling.
- Around line 43-88: Extract the duplicated compare-and-swap invalidation
transactions from the token-health flow into a shared invalidateTokenIfUnchanged
helper in the token service, accepting the character ID, expected encrypted
token, and audit action/details. Reuse this helper for both token.verify_failed
and token.subject_mismatch paths, and update the existing invalidateIfUnchanged
call to use it as well, preserving each site’s audit payload and boolean result
handling.
In `@src/lib/discord/rest.ts`:
- Around line 62-77: Update getGuildRoles, getBotUserId, and getGuildMember to
use safeParse-style response handling like the ESI client, converting both
invalid JSON and Zod validation failures into non-transient DiscordApiError
instances while preserving the existing 404-null behavior and successful return
values.
In `@src/lib/esi/client.ts`:
- Around line 106-109: Validate the numeric conversions for
x-esi-error-limit-remain and x-esi-error-limit-reset before assigning them to
remain and resetAt in the header-parsing flow. Only update these values when the
parsed numbers are finite, preserving the existing defaults so throttle checks
continue to work when headers are malformed.
In `@src/lib/wanderer/client.ts`:
- Around line 23-24: Update eveIdSchema to transform digit strings to numbers
and refine the resulting identifier so only positive safe integers are accepted,
returning the validated numeric value directly. Preserve the union’s supported
input forms while preventing unsafe digit strings from being rounded before
diffAcl reconciliation, and add boundary tests covering invalid and valid
numeric and string identifiers.
In `@src/worker/dispatcher.ts`:
- Around line 85-100: Refactor dispatchOutbox so the database transaction only
claims a bounded batch and marks rows after successful sends, while all send
calls occur outside the transaction. In dispatchOutbox and the related
batching/planning flow, limit the batch size and deduplicate planned jobs by
queue and singletonKey before sequentially invoking send; preserve rows as
undispatched when any send fails. Document the resulting at-least-once delivery
behavior, including that a commit failure may enqueue jobs while leaving rows
undispatched.
In `@src/worker/index.ts`:
- Around line 31-34: Update the pg-boss callback in the handlers registration
loop to accept the delivered jobs array, iterate over every job, and await each
handler invocation before resolving; avoid destructuring the first element so
empty arrays complete safely without throwing.
- Around line 38-44: Wrap the dead-letter worker callback around
deadLetterSchema.parse and postOpsWebhook in error handling, and log any failure
locally before allowing the handler to complete or propagate according to the
existing worker conventions. Ensure both invalid job.data and webhook failures
produce an operator-visible error signal, using the surrounding worker’s
established logger.
- Around line 51-58: Update shutdown handling around shutdown and the
SIGTERM/SIGINT listeners to guard against re-entry, await the dispatcher’s
in-flight dispatchOutbox work by making startDispatcher/stopDispatcher expose or
await the active run, and ensure boss.stop and pool.end execute safely once.
Catch shutdown failures and always terminate the process, using a nonzero exit
status when cleanup rejects.
In `@tests/contacts-job.test.ts`:
- Around line 121-127: Update the assertions for character IDs in the contacts
job test, especially the calls.edits and calls.adds expectations, so they
compare ids order-independently by sorting or using an equivalent
order-insensitive matcher. Preserve the existing character, label, and deletion
expectations while avoiding dependence on the SELECT row order.
In `@tests/deprovision-flow.test.ts`:
- Line 135: Update the outbox count assertion in the deprovision flow test to
require exactly one dispatched row for the single demoted account by replacing
the lower-bound expectation with an exact count assertion.
- Around line 155-159: Update the Discord deprovisioning assertions in the
test’s stayer case to also verify that roleOps.removed does not contain
["u-stayer", "10"], preserving coverage that the untouched account receives
neither role additions nor removals.
In `@tests/discord-roles-job.test.ts`:
- Around line 133-145: Add a test in the Discord roles job suite covering the
unlink strip path when removeMemberRole throws a DiscordApiError with transient:
false. Use the existing fakeDiscord/runDiscordRolesJob setup, assert the job
resolves with a result rather than throwing, and verify the expected status or
outcome for the permanent error.
In `@tests/dispatcher.test.ts`:
- Around line 13-19: Centralize the duplicated database reset logic by adding
the exported truncateAll(db) helper in tests/helpers/db.ts with the existing
11-table TRUNCATE statement. Replace the inline TRUNCATE in
tests/dispatcher.test.ts lines 13-19, tests/purge-job.test.ts lines 13-19, and
tests/deprovision-flow.test.ts lines 21-27 with truncateAll(ctx.db) calls,
importing the helper where needed.
In `@tests/purge-job.test.ts`:
- Line 47: Update the outbox assertion in the purge test to verify the surviving
rows’ dispatchedAt values, preserving the invariant that undispatched rows
remain after purging. Replace the count-only check near the existing purge test
setup with an assertion over the selected outbox records that distinguishes the
expected dispatched and undispatched rows.
In `@tests/worker-queues.test.ts`:
- Around line 5-7: Export the shared TEST_URL from tests/helpers/db.ts, then
replace the locally duplicated database URL definitions in
tests/worker-queues.test.ts, tests/auth-routes.test.ts, and
tests/discord-link.test.ts with imports of that exported symbol.
🪄 Autofix (Beta)
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: b0196131-fa6d-41c4-8df9-b8c254c40158
📒 Files selected for processing (52)
docs/superpowers/plans/2026-08-02-authgd-2-sync-engine.mdpackage.jsonsrc/core/acl-diff.tssrc/core/affiliation.tssrc/core/chunk.tssrc/core/contacts-diff.tssrc/core/role-diff.tssrc/core/tier.tssrc/jobs/contacts.tssrc/jobs/discord-roles.tssrc/jobs/membership.tssrc/jobs/purge.tssrc/jobs/token-health.tssrc/jobs/wanderer.tssrc/lib/discord/oauth.tssrc/lib/discord/rest.tssrc/lib/esi/client.tssrc/lib/ops-webhook.tssrc/lib/wanderer/client.tssrc/services/accounts.tssrc/services/desired.tssrc/services/sync-run.tssrc/services/tokens.tssrc/worker/dispatcher.tssrc/worker/handlers.tssrc/worker/index.tssrc/worker/queues.tstests/acl-diff.test.tstests/affiliation.test.tstests/contacts-diff.test.tstests/contacts-job.test.tstests/deprovision-flow.test.tstests/desired.test.tstests/discord-link.test.tstests/discord-oauth.test.tstests/discord-rest.test.tstests/discord-roles-job.test.tstests/dispatcher.test.tstests/esi-client.test.tstests/helpers/config.tstests/helpers/seed.tstests/membership-job.test.tstests/ops-webhook.test.tstests/purge-job.test.tstests/role-diff.test.tstests/sync-run.test.tstests/tier.test.tstests/token-health-job.test.tstests/tokens.test.tstests/wanderer-client.test.tstests/wanderer-job.test.tstests/worker-queues.test.ts
Comment on lines
+35
to
+45
| for (const [id, aff] of outcome.resolved) { | ||
| await db | ||
| .update(character) | ||
| .set({ | ||
| corporationId: aff.corporationId, | ||
| allianceId: aff.allianceId, | ||
| affiliationCheckedAt: checkedAt, | ||
| affiliationInvalid: false, | ||
| }) | ||
| .where(eq(character.id, id)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Batch the per-character affiliation updates.
This loop issues one UPDATE round trip per resolved character. The global membership run resolves every character, so the query count grows linearly with the roster size. Group the resolved rows by (corporationId, allianceId) and issue one UPDATE ... WHERE id IN (...) per group, or use a single CASE expression.
♻️ Proposed grouping approach
const checkedAt = new Date();
- for (const [id, aff] of outcome.resolved) {
- await db
- .update(character)
- .set({
- corporationId: aff.corporationId,
- allianceId: aff.allianceId,
- affiliationCheckedAt: checkedAt,
- affiliationInvalid: false,
- })
- .where(eq(character.id, id));
- }
+ const byAffiliation = new Map<string, { aff: Affiliation; ids: number[] }>();
+ for (const [id, aff] of outcome.resolved) {
+ const key = `${aff.corporationId}:${aff.allianceId ?? ""}`;
+ const g = byAffiliation.get(key) ?? { aff, ids: [] };
+ g.ids.push(id);
+ byAffiliation.set(key, g);
+ }
+ for (const { aff, ids } of byAffiliation.values()) {
+ await db
+ .update(character)
+ .set({
+ corporationId: aff.corporationId,
+ allianceId: aff.allianceId,
+ affiliationCheckedAt: checkedAt,
+ affiliationInvalid: false,
+ })
+ .where(inArray(character.id, ids));
+ }📝 Committable suggestion
‼️ 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.
Suggested change
| for (const [id, aff] of outcome.resolved) { | |
| await db | |
| .update(character) | |
| .set({ | |
| corporationId: aff.corporationId, | |
| allianceId: aff.allianceId, | |
| affiliationCheckedAt: checkedAt, | |
| affiliationInvalid: false, | |
| }) | |
| .where(eq(character.id, id)); | |
| } | |
| const byAffiliation = new Map<string, { aff: Affiliation; ids: number[] }>(); | |
| for (const [id, aff] of outcome.resolved) { | |
| const key = `${aff.corporationId}:${aff.allianceId ?? ""}`; | |
| const g = byAffiliation.get(key) ?? { aff, ids: [] }; | |
| g.ids.push(id); | |
| byAffiliation.set(key, g); | |
| } | |
| for (const { aff, ids } of byAffiliation.values()) { | |
| await db | |
| .update(character) | |
| .set({ | |
| corporationId: aff.corporationId, | |
| allianceId: aff.allianceId, | |
| affiliationCheckedAt: checkedAt, | |
| affiliationInvalid: false, | |
| }) | |
| .where(inArray(character.id, ids)); | |
| } |
🤖 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/jobs/membership.ts` around lines 35 - 45, Replace the per-row update loop
in the membership resolution flow with batched updates: group entries from
outcome.resolved by the (corporationId, allianceId) pair, then issue one
character update per group using a WHERE id IN (...) predicate while preserving
affiliationCheckedAt and affiliationInvalid values.
Adjudicated CodeRabbit findings on PR #2: - chunk(): reject non-positive-integer size before looping. - validateRoleConfig: fold @everyone's guild role into the bot's permission union (Discord omits it from member role arrays), and parse each role's permissions defensively so a malformed value counts as no permissions instead of throwing. - contacts.ts update-grouping: sort each update's labelIds before building the group key/value so identical label sets in different orders share one editContacts call. - discord-roles.ts strip path: classify DiscordApiError like the account loop (permanent -> failed, no retry; anything else rethrows for pg-boss retry) instead of leaving Discord calls unguarded. - purge.ts: reuse the single captured `now`, anchor outbox retention on dispatchedAt instead of createdAt, and use rowCount from each delete instead of a wasted RETURNING. - tokens.ts: generalize the private CAS-invalidate helper into an exported invalidateTokenIfUnchanged(db, characterId, expectedEnc, audit) so token-health.ts's two duplicated transactions can reuse it. - discord/rest.ts: wrap the three zod parses so malformed bodies throw a permanent DiscordApiError instead of an unclassified exception. - esi/client.ts: only assign remain/resetAt when the parsed rate-limit headers are finite, preserving previous values otherwise. - wanderer/client.ts eveIdSchema: refine to a positive safe integer so an oversized id string fails the whole ACL read closed. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
- Handler registration iterates the whole delivered jobs array instead of destructuring only the first job. - Dead-letter handler wraps parse + webhook post in try/catch so a malformed dead-letter payload or a failed webhook post can't crash the worker or silently vanish. - Shutdown gets a re-entry guard (a second SIGTERM/SIGINT is a no-op), wraps cleanup in try/catch, and exits 1 on cleanup failure instead of hanging or exiting 0 on a broken shutdown. - startDispatcher's stop function is now async: it clears the interval and awaits any in-flight dispatch run, so shutdown can't race a dispatch that's mid-transaction. Documented the resulting at-least-once contract on dispatchOutbox. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
- Add coverage for the new src-side hardening: chunk() size validation, @everyone-role Manage Roles grant + malformed permissions, discord strip-path DiscordApiError classification, malformed Discord REST bodies, and oversized wanderer EVE ids. - purge-job.test.ts: anchor the retention check on dispatchedAt (not just createdAt) and assert the survivors' dispatchedAt values directly. - contacts-job.test.ts: sort both sides of the multi-id add assertions since getFlygdCharacters carries no ORDER BY guarantee. - deprovision-flow.test.ts: assert dispatched is exactly 1 and that the stayer's role is never removed. - helpers/db.ts: export truncateAll(db) and TEST_URL; replace the duplicated inline 11-table TRUNCATE and TEST_DATABASE_URL fallback across every test file that carried them. Claude-Session: https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
Owner
Author
|
@coderabbitai Addressed 20 of the 23 findings in ca52eb0, 009ac65, 54f2043 (220 tests green, typecheck + build clean):
Skipped 3, with reasons:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Reported commits ==='
git show --no-ext-diff --no-renames --format='commit %H%n%s' --stat ca52eb0 009ac65 54f2043
printf '%s\n' '=== Relevant source outlines ==='
ast-grep outline src/core/role-diff.ts --items all 2>/dev/null || true
ast-grep outline src/jobs/purge.ts --items all 2>/dev/null || true
ast-grep outline src/worker/dispatcher.ts --items all 2>/dev/null || true
ast-grep outline src/jobs/token-health.ts --items all 2>/dev/null || true
ast-grep outline src/jobs/membership.ts --items all 2>/dev/null || true
printf '%s\n' '=== Relevant implementation excerpts ==='
rg -n -C 8 'at-least-once|dispatchOutbox|transaction|validateRoleConfig|dispatchedAt|const now|invalidateTokenIfUnchanged|Number\.isFinite|Number\.isSafeInteger|stop|inFlight|Promise\.all|for \(const' \
src/core/role-diff.ts src/jobs/purge.ts src/worker/dispatcher.ts src/services/tokens.ts \
src/lib/esi/client.ts src/lib/wanderer/client.ts src/worker/handlers.ts src/worker/index.ts \
src/jobs/token-health.ts src/jobs/membership.ts 2>/dev/null || true
printf '%s\n' '=== Targeted test references ==='
rg -n -C 3 'at-least-once|validateRoleConfig|dispatchedAt|invalidateTokenIfUnchanged|Number\.isFinite|Number\.isSafeInteger|shutdown|dispatcher|blocked|labelIds' tests 2>/dev/null || trueLength of output: 55046
The inspected implementation and tests cover the 20 addressed findings. This includes permission parsing, The dispatcher comment accurately documents the at-least-once contract. The transaction rollback behavior and pg-boss singleton keys provide the stated duplicate coalescing behavior. I accept the sequential token-health scan for the stated ESI and SSO error-budget constraint. I also accept the current dispatcher transaction structure. The membership update batching remains a valid Plan 3 follow-up because the current deployment size does not make it material for this PR. I verified the source and test coverage. I did not execute the reported test, typecheck, or build commands in this environment. ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/discord/rest.ts (1)
25-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
res.json()parse failures still escapeDiscordApiErrorwrapping.
safeParse(Lines 25-34) only wrapsschema.parse(data). At each call site (Lines 76, 81, 89),await res.json()is evaluated as an argument beforesafeParseruns, so a non-JSON response body throws a rawSyntaxErroroutsidesafeParse's try/catch. This is the second half of the previously flagged issue:getGuildRoles,getBotUserId, andgetGuildMemberstill don't classify a malformed body correctly when the body isn't valid JSON at all, only when it's valid JSON with the wrong shape.Since
runDiscordRolesJobclassifies retryability viaerr instanceof DiscordApiError, an uncaughtSyntaxErrorfalls outside that check and risks the same indefinite-retry behavior the original finding described.Move the
res.json()call insidesafeParseso both failure modes are wrapped.🐛 Proposed fix
- function safeParse<T>(schema: z.ZodSchema<T>, data: unknown, method: string, path: string): T { - try { - return schema.parse(data); - } catch { + async function safeParse<T>( + res: Response, + schema: z.ZodSchema<T>, + method: string, + path: string, + ): Promise<T> { + try { + return schema.parse(await res.json()); + } catch { throw new DiscordApiError(`discord ${method} ${path}: malformed response body`, { transient: false, }); } }async getGuildRoles() { const path = `/guilds/${guild}/roles`; const res = await request(path); - return safeParse(z.array(roleSchema), await res.json(), "GET", path); + return safeParse(res, z.array(roleSchema), "GET", path); }, async getBotUserId(): Promise<string> { const path = "/users/@me"; const res = await request(path); - return safeParse(userSchema, await res.json(), "GET", path).id; + return (await safeParse(res, userSchema, "GET", path)).id; }, /** null when the user is not in the guild (404). */ async getGuildMember(userId: string): Promise<{ roles: string[] } | null> { const path = `/guilds/${guild}/members/${userId}`; const res = await rawRequest(path); if (res.status === 404) return null; assertOk(res, "GET", path); - return safeParse(memberSchema, await res.json(), "GET", path); + return safeParse(res, memberSchema, "GET", path); },🤖 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/lib/discord/rest.ts` around lines 25 - 90, Update safeParse and its callers so response JSON parsing occurs inside safeParse’s try/catch, wrapping both invalid JSON and schema-validation failures in DiscordApiError. Adjust getGuildRoles, getBotUserId, and getGuildMember to pass the response to safeParse (or otherwise invoke res.json() within that function) while preserving their existing schemas, paths, and return behavior.
🤖 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 `@src/core/role-diff.ts`:
- Around line 36-39: Update parsePermissions to validate permissions against a
digits-only decimal pattern before calling BigInt, rejecting hexadecimal,
whitespace-padded, and other non-decimal values; preserve the existing
non-negative clamping for valid decimal inputs.
In `@tests/wanderer-client.test.ts`:
- Around line 61-68: Strengthen the rejection assertion in the “fails closed on
an id that overflows safe integer range” test by matching the safe-integer
validation error and its eve_character_id path or message, rather than only
asserting that getAclMembers rejects. Keep the existing oversized ID fixture and
request setup unchanged.
---
Duplicate comments:
In `@src/lib/discord/rest.ts`:
- Around line 25-90: Update safeParse and its callers so response JSON parsing
occurs inside safeParse’s try/catch, wrapping both invalid JSON and
schema-validation failures in DiscordApiError. Adjust getGuildRoles,
getBotUserId, and getGuildMember to pass the response to safeParse (or otherwise
invoke res.json() within that function) while preserving their existing schemas,
paths, and return behavior.
🪄 Autofix (Beta)
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: ff3b3a53-fa97-4099-9bb4-a06148e5cadc
📒 Files selected for processing (31)
src/core/chunk.tssrc/core/role-diff.tssrc/jobs/contacts.tssrc/jobs/discord-roles.tssrc/jobs/purge.tssrc/jobs/token-health.tssrc/lib/discord/rest.tssrc/lib/esi/client.tssrc/lib/wanderer/client.tssrc/services/tokens.tssrc/worker/dispatcher.tssrc/worker/index.tstests/accounts.test.tstests/auth-routes.test.tstests/contacts-job.test.tstests/deprovision-flow.test.tstests/desired.test.tstests/discord-link.test.tstests/discord-rest.test.tstests/discord-roles-job.test.tstests/dispatcher.test.tstests/esi-client.test.tstests/helpers/db.tstests/membership-job.test.tstests/purge-job.test.tstests/role-diff.test.tstests/token-health-job.test.tstests/tokens.test.tstests/wanderer-client.test.tstests/wanderer-job.test.tstests/worker-queues.test.ts
…sification, sharper wanderer test
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/discord/rest.ts (1)
25-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve transient classification for response-body failures.
res.json()also consumes the response body. If the 30-second timeout aborts that read, this catch marks the transport failure as permanent and suppresses retry. Classify only JSON and schema errors as permanent; preservetransient: truefor body-consumption and abort failures.🤖 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/lib/discord/rest.ts` around lines 25 - 40, Update parseBody so only JSON parsing and schema validation failures throw DiscordApiError with transient: false; allow response-body consumption and abort errors from res.json() to propagate or retain transient: true so retry behavior is preserved. Keep the existing malformed-response context in the permanent-error path.
🤖 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 `@src/lib/discord/rest.ts`:
- Around line 91-96: Update getGuildMember so a 404 response is parsed and
mapped to null only when the Discord error code is 10007; for code 10004 or
malformed error bodies, raise a non-transient DiscordApiError. Preserve assertOk
handling for other statuses, and update the existing fixture to include code
10007 with an additional 10004 test case.
---
Outside diff comments:
In `@src/lib/discord/rest.ts`:
- Around line 25-40: Update parseBody so only JSON parsing and schema validation
failures throw DiscordApiError with transient: false; allow response-body
consumption and abort errors from res.json() to propagate or retain transient:
true so retry behavior is preserved. Keep the existing malformed-response
context in the permanent-error path.
🪄 Autofix (Beta)
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: c6639209-c134-48d8-9b34-4d60d3ac6fc1
📒 Files selected for processing (4)
src/core/role-diff.tssrc/lib/discord/rest.tstests/discord-rest.test.tstests/wanderer-client.test.ts
Comment on lines
+91
to
+96
| async getGuildMember(userId: string): Promise<{ roles: string[] } | null> { | ||
| const path = `/guilds/${guild}/members/${userId}`; | ||
| const res = await rawRequest(path); | ||
| if (res.status === 404) return null; | ||
| assertOk(res, "GET", path); | ||
| return parseBody(memberSchema, res, "GET", path); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline src/lib/discord/rest.ts --view expanded
printf '%s\n' '--- target implementation ---'
sed -n '1,180p' src/lib/discord/rest.ts
printf '%s\n' '--- related Discord error handling ---'
rg -n -C 3 'DiscordApiError|assertOk|memberSchema|rawRequest|getGuildMember|10004|10007|Unknown member|Unknown guild' src test tests 2>/dev/null || trueRepository: guarzo/authGD
Length of output: 21333
🌐 Web query:
Discord API Get Guild Member 404 error code 10007 Unknown Member Unknown Guild 10004 official documentation
💡 Result:
In the Discord API, error codes 10007 and 10004 are standard JSON error responses indicating that a requested resource could not be found [1][2]. Error Code 10007: Unknown Member This error signifies that the requested user is not a member of the specified guild [1][3]. Common causes include: - The user has left the server or was kicked/banned. - The user is not in the guild the application is trying to access. - In libraries like discord.js, the member might not be cached [4]. If you receive this error, ensure the member exists in the guild and consider using a fetch operation (e.g., interaction.guild.members.fetch(user.id)) to retrieve the most up-to-date member object from the API [4][5]. Error Code 10004: Unknown Guild This error signifies that the requested guild ID does not exist, or the bot does not have access to it [1][2]. Common causes include: - The guild ID is incorrect or invalid [6]. - The bot has not been added to the guild [7][8]. - The bot lacks the necessary permissions or intents to view the guild or the requested resource within it [8]. - In some scenarios, this error may also be returned when the application lacks authorization to access a guild's data [8]. For official reference, these codes are documented under the Discord API "Opcodes and Status Codes" documentation [1]. When using Discord libraries like discord.js, these are often wrapped as specific error classes (e.g., DiscordAPIError) that provide these codes to help with debugging [4][7].
Citations:
- 1: https://docs.discord.com/developers/topics/opcodes-and-status-codes
- 2: API Error Codes discord/discord-api-docs#61
- 3: https://discord-api-types.dev/api/discord-api-types-v10/enum/RESTJSONErrorCodes
- 4: https://stackoverflow.com/questions/70416839/how-to-fix-discordapierror-unknown-member
- 5: https://stackoverflow.com/questions/75332110/discordapierror-10007-unknown-member
- 6: https://stackoverflow.com/questions/79594593/unknown-guild-error-when-retrieving-guild-server-from-discords-api
- 7: https://stackoverflow.com/questions/50942405/discord-js-unknown-guild-when-removing-all-roles-from-a-user
- 8: Error code when accessing guild application is not in has changed discord/discord-api-docs#5840
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Discord REST tests ---'
sed -n '1,90p' tests/discord-rest.test.ts
printf '%s\n' '--- error construction and status handling ---'
rg -n -C 4 'new DiscordApiError|status:|transient:' src tests
printf '%s\n' '--- package/runtime metadata ---'
sed -n '1,180p' package.jsonRepository: guarzo/authGD
Length of output: 48256
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/lib/discord/rest.ts").read_text()
tests = Path("tests/discord-rest.test.ts").read_text()
match = re.search(
r"async getGuildMember\(userId: string\).*?\
return parseBody\(memberSchema, res, \"GET\", path\);",
source,
re.S,
)
assert match, "getGuildMember implementation not found"
body = match.group(0)
print("current 404 branch:", "if (res.status === 404) return null;" in body)
print("reads 404 error body before null:", bool(re.search(r"404.*(?:json|text|clone)", body, re.S)))
print("delegates non-404 responses to assertOk:", "assertOk(res, \"GET\", path)" in body)
fixture = re.search(
r'HttpResponse\.json\((\{.*?\}), \{ status: 404 \}\)',
tests,
re.S,
)
print("existing 404 fixture:", fixture.group(1).replace("\n", " ") if fixture else "not found")
print("existing fixture includes Discord error code:", bool(fixture and re.search(r"\bcode\s*:", fixture.group(1))))
PYRepository: guarzo/authGD
Length of output: 366
Restrict the 404-to-null mapping to error code 10007.
Parse the Discord error body and return null only for code === 10007 (Unknown Member). For other 404 responses, including 10004 (Unknown Guild) and malformed bodies, raise a non-transient DiscordApiError. Update the existing test fixture to include code: 10007 and add a 10004 case.
🤖 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/lib/discord/rest.ts` around lines 91 - 96, Update getGuildMember so a 404
response is parsed and mapped to null only when the Discord error code is 10007;
for code 10004 or malformed error bodies, raise a non-transient DiscordApiError.
Preserve assertOk handling for other statuses, and update the existing fixture
to include code 10007 with an additional 10004 test case.
This was referenced Aug 3, 2026
Merged
This was referenced Aug 4, 2026
Merged
Merged
guarzo
added a commit
that referenced
this pull request
Aug 10, 2026
* feat(design): retune the palette off the generated navy-and-gold axis Feedback was that the app reads as machine-made, with the colour scheme named specifically. The old ground was the second reflex, not the first: this design correctly refused cyan-on-black sci-fi, then landed on navy-with-a-gold-accent, which is the most-generated "premium dark tool" palette in circulation. The ground is now a neutral near-black whose tint scales up with lightness, and the tint ramp is the rule rather than an oversight. "Tint every neutral toward the brand hue" only holds for mid and light neutrals; at near-black a warm hue reads as brown. Two warm grounds were built and rejected on exactly that before landing here: navy #080f1f R/B 0.26 warm #1 #0e0906 R/B 2.33 (chroma copied from the navy's 0.035) warm #2 #0c0a08 R/B 1.50 (halved; red-minus-blue only 4, still brown) neutral #0a0a0a R/B 1.00 Red-minus-blue is the wrong statistic at these luminances — a gap of 4 is invisible at rgb(200) and a 50% cast at rgb(12). Judge a ground by its hex, and at near-black by the ratio. Healthy states stop being green. DESIGN.md's status-token rule has always said colour is only for actionable state, and `.st--ok` was where the shipped UI contradicted it: every ok chip, every LINKED, every "all healthy" count rendered full-chroma green. A screenful of green dots reporting that nothing needs doing is the generic-dashboard signature. `--signal-bad` lifts to 0.66 because 0.64 measured 4.42:1 against `--hull-hi` — under the AA floor exactly when the pointer is on the row, the same class of bug as the disabled-opacity one already recorded. The account illustration is recut for a dark ground (lightness -40); at full brightness it was the lightest object on any screen in the app. The same treatment was tried on the 34px header mark and rejected — the gold rope goes out and the disc turns to mud. Artwork modification is cleared by Faoble; the master is in git history. Every text token measured against all three grounds: worst case --ink-faint at 4.63:1 on a hovered row, nothing under 4.5:1. * feat(account): compose the page, and let the crew speak only when it differs Two problems on /account, both structural rather than cosmetic. The page had a dead half. `.page` is a 78rem column but nothing in it used more than the manifest's 48rem, so roughly 470px of the column sat empty at 1440px while the page ran long vertically. "Sync schedule" and the closing illustration are material a member reads but does not operate, so they now sit in a rail beside the manifest instead of stacked under it, and the page is substantially shorter. Below 64rem the rail collapses under the manifest, manifest first. `.page` and `--measure-page` are untouched, so the one-column origin — H1 left edge, rule origins, header seal on one vertical across every route — is unchanged. The manifest contradicted its own header. `isNominal` required `onMapAcl`, while this page's own comments and core/account-health.ts both hold that map membership cannot substantiate a fault. So a member on no map was disqualified from the collapsed treatment and every row recited "token ok, standings ok, map off" — underneath a head reading "10 characters — all healthy". `crewNorms` measures deviation against the crew rather than an absolute ideal: a fact every character shares is one fact about the account, stated once in the head, not ten times in the table. Parity holds — the head gained "no characters on the map", so nothing left only one channel — and the table's `<caption>` now says where the fact lives rather than promising it per row. The per-row disclosure drops its visible "actions" caption, which repeated identically on every row and was the loudest pattern in the table. KNOWN DEFECT, recorded in globals.css rather than fixed: that marker now sits alone at the table's right edge, ~490px from the name it expands. Two fixes were built and measured, and both cost more than the defect — leading the row with ACTIONS costs 52px of the 320px forced-scroll budget (134 -> 186 against a 170 tripwire), and making NAME fit-width so ACTIONS takes the slack starves the long-location case (352px wanted, 151px measured). The route that works is moving the control inside the NAME cell, which needs `leadCells` split across page.tsx and character-row.tsx. Specs: four rewritten where `crewNorms` changed what they measure, one rewritten and one added for the rail. The alignment test seeds a payout so two rule heads remain in the main column — without it the count guard had to drop to `> 0` and the "several headings share one edge" half of the claim stopped being tested. `crewNorms` gains unit coverage for the empty, single, uniform and mixed cases.
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Plan 2 of 3: the complete background sync engine on top of the Plan 1 auth foundation (#1). Spec:
docs/superpowers/specs/2026-08-02-authgd-design.md("Sync jobs", "Error handling", tier model). Plan (committed on this branch):docs/superpowers/plans/2026-08-02-authgd-2-sync-engine.md.src/worker/index.ts,npm run worker): pg-boss v10 with explicit queues (policy: "short"so singleton-key coalescing actually works), retry5×/60s/backoff, staggered schedules carrying global singleton keys, dead-letter queue → ops webhook, graceful shutdown.FOR UPDATE SKIP LOCKED, sends, and marks dispatched in ONE transaction; failed sends roll the claim back.{kind:"account"}fans out to scoped membership/roles + global contacts/wanderer./characters/affiliationwith bisection ONLY on deterministic 400s;affiliation_invalidflagging + weekly recheck queue; tier transitions only on a confirmed read of the main, committed with their outbox trigger in one locked transaction.missing_labelskip, all-pages-before-destructive-diff (fail-closed X-Pages), label-ownership reconciliation preserving personal labels, per-job scope gating (needs_reauthis not a global blocker), per-character result recording.GET /api/acls/:id, members underdata.members, strict role enum + exactly-one-external-id validation); admins and blocked entries never removed; desired-but-blocked reset to viewer; post-mutation re-read persisted intowanderer_acl_observation.reclaimTransferredCharacter, no last-character guard) with session revocation; scope shortfall →needs_reauth.Review process
Plan went through two pre-execution review rounds (Wanderer contract, rotation races, queue policy, fail-closed payloads). Each task was independently reviewed during execution; a final whole-branch review found four seam-level issues — one character's JWT failure sinking token-health, silent permanent Wanderer failures, a biomassed character halting fleet-wide removals, and an ACL un-ban edge — all fixed with tests in the last five commits. Deferred to Plan 3 (recorded in the review): ESI User-Agent header, recheck
sync_runlabeling, minor count/CAS polish.Test plan
npm test✅npm run typecheck✅,npm run build✅🤖 Generated with Claude Code
https://claude.ai/code/session_016odmULfR3ptiZhsDEnsU7L
Summary by CodeRabbit
New Features
Documentation
Tests