Reconcile deputies by membership instead of by slot position - #9
Open
DJAscendance wants to merge 2 commits into
Open
Reconcile deputies by membership instead of by slot position#9DJAscendance wants to merge 2 commits into
DJAscendance wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes deputy-role reconciliation to be order-insensitive by comparing deputy assignments by membership (set semantics) instead of by slot index, and centralizes the previously copy-pasted deputy sync logic into RoleAssignmentService.
Changes:
- Added
RoleAssignmentService.syncDeputies()with set-based reconciliation and an explicit incoming deputy-slot cap (DEPUTY_SLOTS). - Refactored
block,hood,colony, andplaceservices to delegate deputy reconciliation to the shared helper. - Added a focused unit test suite covering reorder/no-op, add/remove/swap, sentinel handling, dedupe, slot cap, missing deputy role, and “continue past failure”.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/services/role-assignment/role-assignment.service.ts | Introduces syncDeputies() helper and DEPUTY_SLOTS cap to reconcile deputies by set membership. |
| api/src/services/role-assignment/role-assignment.service.spec.ts | Adds unit tests that lock in the fixed reorder-safe behavior and other edge cases. |
| api/src/services/place/place.service.ts | Replaces index-based deputy sync loop with a call to RoleAssignmentService.syncDeputies(). |
| api/src/services/hood/hood.service.ts | Delegates deputy reconciliation to the shared helper. |
| api/src/services/colony/colony.service.ts | Delegates deputy reconciliation to the shared helper. |
| api/src/services/block/block.service.ts | Delegates deputy reconciliation to the shared helper. |
Comments suppressed due to low confidence (1)
api/src/services/role-assignment/role-assignment.service.ts:115
- Same as above: use
console.errorfor caught errors when adding deputy assignments so operational logs clearly surface failures.
try {
await this.roleAssignmentRepository.addIdToAssignment(placeId, memberId, deputyRoleId);
} catch (e) {
console.log(e);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
+90
| public async syncDeputies( | ||
| placeId: number, | ||
| deputyRoleId: number, | ||
| oldDeputyIds: number[], | ||
| newDeputyIds: number[], | ||
| ): Promise<void> { | ||
| if (deputyRoleId === undefined || deputyRoleId === null) return; |
Comment on lines
+98
to
+101
| const removed = () => | ||
| roleAssignmentRepository.removeIdFromAssignment.mock.calls.map(call => call[1]).sort(); | ||
| const added = () => | ||
| roleAssignmentRepository.addIdToAssignment.mock.calls.map(call => call[1]).sort(); |
DJAscendance
added a commit
that referenced
this pull request
Jul 30, 2026
Four of five findings from the CodeRabbit App review of #6. The fifth is a heavy lift that needs its own change; see below. awaitRoleMap no longer poisons itself. The constructor's population promise had no .catch(), so a transient database error at startup was an unobserved rejection -- a warning normally, fatal under --unhandled-rejections=throw. Worse, the rejected promise stayed memoized in roleMapReady, so every later awaitRoleMap re-awaited the same rejection and the process could not recover without a restart. That turned the canManageAccess fix from the previous commit into a liability: those call sites would throw rather than fail closed. Population is now started through a helper that clears the memo on failure so the next caller retries, with an identity check so a late-settling older attempt cannot clear a newer one's memo. The eager rejection is observed and discarded. Kept deliberately: awaitRoleMap still rejects rather than returning a half-empty map. Returning {} would put callers back on the silent-denial path this method exists to close, telling a real admin "no" instead of "could not determine". The rejection reaches the controllers' existing try/catch, and the cleared memo means the next request retries. Six role-code reads were awaiting a number. `await roleMap.BlockDeputy` awaits an already-resolved value and waits for nothing, so it read the unpopulated map exactly as a bare access would -- the await was pure decoration. getAccessInfoByUsername and postAccessInfo in all three place services now await the map itself. place.service is fixed at findRoleIdsBySlug, which is the single point where every slug's codes are resolved, so all four of its callers are covered by one await. getAccessInfoByID no longer throws for places with no deputy role. 'jail' (Security Chief) and 'cityhall' (City Council) have an owner role and no deputy, and `.where('role_id', undefined)` makes knex throw "Undefined binding(s) detected", taking down the owner lookup along with it. The deputy query is skipped instead. Guarded in the repository because every caller had the same exposure -- and because the guard added for this in the previous commit sat AFTER the getAccessInfoByID call in place.service, so it could never have been reached. The seed validates before it destroys. removePreviousFixtures and createFixtureMembers ran before the place queries, so seeding a database with no colonies deleted the existing fixtures and only then threw, leaving it emptier than a failed seed found it. Reads and validation now come first, so a failed precondition is a no-op. Verified: tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors on the eight changed files. Suite compared against a stashed baseline test-name by test-name: no regressions, and three tests went from failing to passing -- MemberService > createMemberAndLogin > {should not store the provided member password in clear text, should return a session token for the new member, should tell the database to create a member with the provided name and email}. Those were being killed by the uncaught constructor rejection, which is independent evidence the first finding was real and reached past authorization. Totals 41 -> 44 passing, 5 -> 2 failing. Deliberately not done -- the fifth finding. reconcilePrimaryRole observes a temporary gap: the callers remove an assignment, reconcile, then insert the replacement, so a member who keeps a role at a DIFFERENT place has primary_role_id cleared even though the final state still holds it. The fix is to reconcile after the complete mutation set inside one transaction, which changes the shape of every caller and wants a transfer regression test. It also affects syncDeputies on #9, which removes and adds in two passes. Doing it here would mix a transaction boundary change into a review-response commit.
The deputy sync paired the old and new deputy lists BY INDEX, so it was order-sensitive. Given old [A, B] and new [B, A]: index 0 saw A != B, so it removed A and added B; index 1 then saw B != A, so it removed B -- which had just been added and was meant to stay -- and added A back. Net effect, B silently lost the deputy role. Worse, B took a reconcilePrimaryRole call while still a deputy, which is exactly the spurious primary-role clearing that reconcilePrimaryRole was added to prevent. Reordering the slots on the access-rights page was enough to trigger it; no field had to change. A deputy assignment means membership, not position, so the comparison is now between sets. A member in both lists is left alone, which also stops needless remove-then-re-add churn on rows that were never changing. The loop was duplicated verbatim in block, hood, colony and place, so this replaces four copies with one helper on RoleAssignmentService -- next to reconcilePrimaryRole, which already documents being extracted from the same four services plus admin. Each call site keeps only the part that is genuinely its own: which role id counts as deputy there. Behaviour deliberately preserved rather than tidied: - The eight-slot cap is kept, as RoleAssignmentService.DEPUTY_SLOTS, applied to incoming ids only. The fixed [0,0,0,0,0,0,0,0] arrays imposed it implicitly and dropping it would quietly widen how many deputies a place can be given, which is not a review-fix decision. - Old ids are NOT capped, so any deputy beyond the eighth still gets cleaned up if one somehow exists. - 0 remains the empty-slot sentinel and is filtered on both sides. - Failures stay caught per member, so one bad row does not abandon the rest of the reconciliation, as before. - The undefined-deputy-role guard added for 'jail' and 'cityhall' now lives in the helper too, so all four call sites get it rather than just place. Verified: 11 new tests covering reorder, add, remove, swap, the 0 sentinel, duplicate ids, the slot cap, a place with no deputy role, and continuing past a failed write. tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors. Suite compared against the 6e3e131 baseline suite-for-suite rather than by count: the same five suites fail, all MySQL connection errors needing a live database, and passing tests go 41 -> 52. The reorder case is the one that would have failed before this change; it is written as a test of the helper, so it documents the fixed behaviour rather than reproducing the old loop.
DJAscendance
force-pushed
the
fix/deputy-reconciliation
branch
from
July 30, 2026 14:58
8b8ac70 to
39fce38
Compare
Three findings, all in code added by this PR. syncDeputies' deputyRoleId is now typed `number | null | undefined`. The method already guarded for undefined and the specs already passed it, but the signature said `number` -- so the guard looked like dead defensive code and a call site passing the genuinely-optional deputy code would have been a type error under stricter settings. 'jail' and 'cityhall' have an owner role and no deputy role, so undefined is a real input, not a hypothetical. Caught errors go to console.error rather than console.log. These are failed role_assignment writes; logging them on stdout alongside ordinary output is how they get missed. The surrounding code uses console.log and this commit does not go change all of it, but new error paths should not add to that. The spec's sort helpers take a numeric comparator. `.sort()` with no comparator is lexicographic, so it orders 10 before 2. The current ids (101-103, and 1-8 in the slot-cap test) happen to sort the same either way, so the tests passed -- which is exactly why it was worth fixing before someone changes an id and gets a confusing failure. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. 17/17 syncDeputies specs, 55 passing overall with the same 2 pre-existing failures.
This was referenced Jul 30, 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
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.
Stacked on #6 (
fix/access-rights-place-scoping) — review that first; the diff here is only the last commit.The bug
The deputy sync paired the old and new deputy lists by index, so it was order-sensitive. Given old
[A, B]and new[B, A]:A !== BB !== ANet effect: B silently loses the deputy role. Worse, B takes a
reconcilePrimaryRolecall while still a deputy, which is exactly the spurious primary-role clearing thatreconcilePrimaryRolewas added to prevent. Reordering the slots on the access-rights page was enough to trigger it — no field had to change.This predates #6:
origin/masterhas the same pairing inside aforEach. #6 converted theforEachto aforloop so the awaits actually landed, but kept the index pairing.The fix
A deputy assignment means membership, not position, so the comparison is now between sets. A member present in both lists is left alone, which also stops needless remove-then-re-add churn on rows that were never changing.
The loop was duplicated verbatim in four services —
block,hood,colonyandplace— so this replaces four copies with oneRoleAssignmentService.syncDeputies, next toreconcilePrimaryRole, whose own docblock already records being extracted from the same four services plusadmin. Each call site keeps only what is genuinely its own: which role id counts as deputy there.place.service.tswas not flagged by the review but carries the identical loop, so it is included.Behaviour deliberately preserved, not tidied
RoleAssignmentService.DEPUTY_SLOTS, and applied to incoming ids only. The fixed[0,0,0,0,0,0,0,0]arrays imposed it implicitly; dropping it would quietly widen how many deputies a place can be given, which is not a review-fix decision.0remains the empty-slot sentinel and is filtered on both sides.jailandcityhallnow lives in the helper, so all four call sites get it rather than onlyplace.Verification
0sentinel, duplicate ids, the slot cap, a place with no deputy role, and continuing past a failed write.tscreports only the pre-existing missingsharpmodule, which fails identically on the base branch.eslint0 errors.6e3e131base suite-for-suite rather than by count: the same five suites fail, all MySQL connection errors needing a live database; passing tests go 41 → 52.The reorder case is the one that would have failed before this change. It is written as a test of the helper, so it documents the fixed behaviour rather than reproducing the old loop.
Not done here
getAccessLeveland the widerroleMappopulation race are untouched. Roughly 100 sites still readroleRepository.roleMapsynchronously, and several are writtenawait roleRepository.roleMap.X, which awaits a number and does nothing to wait for population. #6 fixed the threecanManageAccessmethods; the rest needs its own pass.