feat(rules,backstage,beacon,auth): position-assignment lane + role-lifecycle follow-through - #222
Merged
Merged
Conversation
Design for PR 4 (members-positions write lane keyed on update:Position) plus the three residuals PRs 1-3 deferred: well-formedness on the remaining soft-delete lanes, the three-way resolution extracted to packages/auth, and canCurateFeatured migrated off role names. Revised after two adversarial passes; the Review corrections section records the seven claims they falsified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The absent/live/inactive resolution was implemented twice — once in beacon's claims-sync and once in backstage's preview mirror — so the subtle half was duplicated across the trust boundary. Extracted to resolveBuiltInPerms in @luminova/auth/built-in-perms; both sides now delegate. Synchronous and pure over already-fetched docs: it never sorts or mutates its inputs, because beacon's graph is deep-frozen. Liveness DERIVATION stays per-side (isActiveRoleDoc imports firebase-admin and packages/auth is also a browser and rules-test dependency); only its consumption is shared. PERMISSION_CAP stays out — beacon fail-closes to perms:[], backstage blocks Save. Settles the one divergence in the tighter direction: a doc whose builtInKey is not among the requested names is now ignored on both sides. Unreachable in production (the beacon query filters by key), so it gets a direct unit test. Both existing suites pass unchanged — the acceptance criterion for the refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…osition
PR 2 withdrew manage:Position from ExecutiveCommittee and deleted the
positions-only rule with it, leaving cargo assignment Admin-only and
memberEditMode's "positions" arm unreachable. This restores the lane.
A new fourth allow-update arm on members, rather than relaxing the
institutional arm's leading canDo('update','Member') — arms OR together, so a
new one is additive and auditable in isolation. positionsAssignmentSafe() is
reused verbatim: its non-Admin branch still demands cargoGrantsEmpty() AND
currentCargoGrantsEmpty(), so this principal assigns grant-free cargos only, on
both sides of a swap. The four conjuncts after it are implied by
hasOnly(['positions']) today and stated anyway — they are the claims-mint
boundary, and a future widening of hasOnly must not drop them silently.
The lane IS a public publication authority: grant-free JDL direcciones are
board cargos, so its holder can put a member — including themselves — on the
world-readable Directiva. Accepted deliberately and pinned by a test. Narrowed
by adding unchanged('category') beside unchanged('grants') on the catalog arm,
without which the holder could retitle a grant-free Comisión to CEL/Presidente
(board rank 0) and self-assign it.
No built-in role gains update:Position, so no existing principal's authority
changes. The hand-off is a custom role in /permisos — NOT an edit to the CEL
built-in, which reseedBuiltInRolePerms would silently strip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A member doc carrying the STRING "false" in active passed softDeleteSafe(), was dropped by memberDocSchema so it was invisible throughout backstage, and was still published to the public Directiva — project-board.ts tested `active === false`, the fail-open direction, while project-ally.ts used the fail-closed `active !== true`. softDeleteSafe() gains the well-formedness prefix roleLifecycleSafe() already had. Inside the helper, not on the arms: the members Admin-takedown arm deliberately does not call it, and it is the only rules-level path that can unpublish exactly the malformed member this fixes. The three create arms gain the matching born-live requirement — a guard that only holds the update door is half a guard, and the create arms were minting the docs the update arm would then refuse. project-board.ts flips to fail-closed. That is the only part that removes the EXISTING exposure; the rules halves only stop new malformed docs. This is a real new denial, not a reclassification. The first draft argued the missing-field case already denied via an erroring bare read — false for the deletedAt half, because CEL's || absorbs errors and unchanged() returns true on an absent key. Measured on the emulator: such docs were freely editable before. The wrong generalization was also written into a rules comment and is corrected here. Repairs 15 existing test payloads that B2 would otherwise have made vacuous — including two escalation guards and one that states in a comment that it isolates the publicProfile create guard. Two further tests were ALREADY vacuous (a 2-char name failed memberNameValid, not the guard they name); fixed while here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ole name
canCurateFeatured() gated public-site curation on hasAnyRole(['Admin',
'ProjectManager']). computeMemberRoles is pure over {trustedGrants, hadScanner}
and reads no role doc, so a DEACTIVATED ProjectManager kept the name in its
claim and kept the authority — over the only client-writable input to public
content that is not a cargo assignment.
The gate becomes hasAnyRole(['Admin']) || hasPerm('update:Showcase'), behind a
new Showcase subject (78 -> 84 codes).
hasPerm, deliberately not canDo: canDo would let manage:all satisfy the gate,
and manage:all is reachable as a PERM without the Admin role. That principal
already satisfies canDo('update','Project') on the initiative arm, so this role
gate is the only thing that stops them setting featured today — the file
documents that boundary as intentional, and canDo would have deleted it
silently. Admin stays role-keyed: it is locked and undeactivatable, so it
carries none of the staleness this fixes.
The client mirror is the exact two-disjunct check, NOT an ability query: CASL
maps manage:all to can('manage','all'), so an ability check returns true for a
manage:all perm holder — the exact principal the rule excludes. It would have
rendered the Destacar checkbox to someone whose write the rules reject, sinking
the whole initiative save.
Deploy order matters: BUILT_IN_ROLE_PERMS is a seed snapshot, so the constant
alone mints nothing on an existing project. Run reseedBuiltInRolePerms and
verify a live ProjectManager's CLAIM before the rules ship, or every current
curator loses the ability.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four review angles converged on the same duplication. Extracted: - bornLive() — the born-live triple was copy-pasted onto four create arms, three of them added by this branch. Rule of three, exceeded on the first pass. - memberWriteInvariants() — the claims-mint boundary, publication consent and the points ledger were written verbatim on both the institutional and the new positions-only member update arms. A security invariant stated twice is the mechanism that drifts, not the one that prevents drift; the file's own canCurateFeatured() extraction is the precedent. Reordering the positions-only arm around it also puts the only get()-issuing conjunct last, so denials now cost zero billed reads. - hasPerm() in @luminova/auth — the client mirror of the rules primitive of the same name. canFeatureInitiatives had hand-rolled it, including its own null default. - LiveBuiltInRoleDoc is now an alias of the shared BuiltInRoleDoc rather than a byte-equivalent restatement with a duplicated docblock. resolveEffectivePerms takes readonly arrays, which deletes four defensive permission-array copies per resolution in an unbounded fan-out and retires a cast. The two independent Firestore fetches in resolveMemberPerms now run in Promise.all instead of in series. The backstage preview adapter is one pass instead of O(names x allRoles). Corrects a comment that gave the wrong reason for keeping the two liveness predicates separate: the blocker is not the firebase-admin type-only import, it is that isActiveRoleDoc is fail-OPEN and isLiveRole is fail-CLOSED, so collapsing them is an unscoped behaviour change. Declined deliberately: table-driving the new rules tests, and deleting the three-way cases from the beacon and backstage suites. Those suites go red THROUGH the shared function, which is what proves the delegation is real. No behaviour change. auth 44->46 (two hasPerm tests); types 319, beacon 283, backstage 731, rules 453 all unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mandated review round found the branch's own category pin was defeated
one door over, plus two false claims the branch made about its own tests.
- boardSurfacingCategory() on the positions CREATE arm. Pinning only the
update arm left the same outcome reachable through create: a non-Admin
create:Position holder could mint a fresh {category:'CEL', title:
'Presidente', grants:[]} cargo — grant-free, so the power-grant check passes
— and self-assign it onto the public Directiva at board rank 0. "Seeded CEL
cargos all carry grants" is what made that look blocked; nothing stopped an
unseeded one.
- title/titleFemale join the non-Admin pin on board cargos, because boardRank
reads the TITLE, not the category — so pinning category alone still allowed
retitling Vicepresidente to Presidente. Comisión renames stay open.
- PositionForm mirrors both pins. Without it the rules reject a submission the
form invited, with a generic save error.
- An order-coupled test pair: an Admin test wrote category:'CEL' onto the doc a
BLOCKING denial read, so afterwards the identical non-Admin write was an
unchanged echo and was ALLOWED. The denial passed only because vitest runs
tests in declaration order. Measured by reordering; fixed with its own
fixture.
- The full toPositionUpdateDoc payload now has tests. A rules pin with no test
of the real client payload is how a 403 reaches production.
- Two tests the spec claimed existed and did not: the term-rollover residual
(pinned as an ACCEPTED hole, not silently) and the grant-free-cargo-mints-
nothing property the whole accepted-exposure argument rests on.
- Missing-field coverage for positions and allies; the prefix is one helper
shared by four lanes but only members exercised it.
- tools/scripts/audit-soft-delete-shapes.mjs makes the blocking owner-op
executable. It is a repair tool, not a count: B3 only stops NEW malformed
members being published, and onBoardMemberWritten fires only on a member
write — which B1 makes impossible from the client. Repairing through the
admin SDK re-fires the projection and removes the row. --repair refuses to
guess at a non-bool active.
Spec corrected where it overstated: B3's reach, the rollover guarantee in
owner-op 1, and the two absent tests. Owner-op 1 now also states the custom
role needs read:Member, without which the capability is unreachable.
rules 457->468, backstage 731->732.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
react-best-practices pass over the branch's .tsx changes. The only finding: the non-Admin blank-form default was an object literal built inline in the useForm call (rerender-memo-with-default-value). RHF captures defaultValues once, so the cost was an allocation per render rather than a re-render — small, but the fix is free and the constant now carries the WHY. Otherwise clean: areLabelsLocked is derived during render rather than in an effect, no waterfalls or barrel imports were added, no new dependency, no component defined inside a component, and the && conditionals guard booleans rather than possibly-zero values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… audit-script gaps
Second review round. The findings that mattered:
- The grant-free-CEL boundary was a claim about DATA, enforced by nothing.
"Seeded CEL cargos all carry grants" is true of the seed and false the
moment an Admin mints a grant-free one — which this branch's own test does.
positionsAssignmentSafe()'s non-Admin branch now reads category off the same
get() cargoGrantsEmpty() already pays for and refuses CEL outright, so
'Presidente' at public rank 0 stays an Admin decision. JDL stays open: that
is the accepted exposure this lane exists to deliver. The replaced cargo is
deliberately NOT category-gated — clearing someone off a CEL seat is
exposure-reducing and must not need an Admin.
- A drift guard binds boardSurfacingCategory()'s literal to BOARD_GROUPS. The
rules hardcoded ['CEL','JDL'] with nothing tying it to its authority, so a
third publishable group would have silently reopened the hole just closed.
- --repair could PUBLISH someone. Repairing a member missing "active" writes
active:true, and the re-fired trigger ADDS them to the public Directiva if
they are otherwise projectable. The script announced only the takedown
direction. It now forecasts publication, prints WILL PUBLISH, and withholds
those repairs behind --allow-publish.
- A sentinel bug: "published.get(id) ?? false" collapsed the null "unreadable"
state into "not published", making the UNKNOWN branch dead code and letting
unknown-exposure docs be truncated out of the operator's worklist.
- The UI mirror had ZERO tests and one existing test had gone vacuous — it
selected a value on a now-disabled select that already held that value, so
it measured the default. A regression to register(name, {disabled}) would
make RHF submit undefined and 403 every non-Admin save, and would have
shipped green.
- The two rules arms disagreed on a doc with no "category" key; both now use
boardSurfacingCategory().
rules 468->473, backstage 732->737, types 319->323.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit made "CEL stays an Admin decision" a rule on the members UPDATE lane and wrote that guarantee into firestore.rules, the spec, and owner-op 1 — the text an owner reads before granting the capability. On the CREATE lane it was still a data assumption: createPositionsSafe() kept the old grants-only predicate, so a Membership holder (manage:Member satisfies create:Member) could mint a member BORN holding an Admin-minted grant-free CEL cargo. The create arm forbids uid, so the doc is unpublished at birth — but onMemberCreated stamps publicProfile true, the creator controls the portrait and the email, and a routine Admin provisionMemberLogin then writes uid, after which projectBoard publishes an attacker-controlled account at board rank 0 as Presidente. One Admin action in the chain, taken with no reason to inspect the cargo. Create now uses the same cargoAssignableByNonAdmin() — it reads only request.resource.data, so it is create-safe, unlike currentCargoGrantsEmpty(), which stays excluded because a create has no old side. cargoGrantsEmpty() has no callers left and is deleted. The CEL literal was also an ALLOW-by-default denylist with no drift guard: add a third publishable group to BOARD_GROUPS and the existing guard would force it into boardSurfacingCategory() (minting goes Admin-only) while nonAdminAssignable() silently kept ASSIGNING it open to everyone — a new board group auto-enrolled into the accepted-exposure class with nobody deciding. The guard now pins the assignable set to exactly ['JDL']. Client mirror carried across, which the rules tightening had outrun: a grant-free CEL cargo still appeared in the non-Admin picker, and a member already seated on one had no lock while every save re-stamped cargoId — so even a comisiones-only edit was denied with a generic error. One shared assignable-cargo helper now drives the option list and the lock in both member forms. rules 473->475, backstage 737->745, types 323->325. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client mirror added in the previous commit collapsed two asymmetric rules conjuncts into one. cargoAssignableByNonAdmin() denies KEEPING a grant-free CEL seat, but currentCargoGrantsEmpty() is deliberately not category-gated — firestore.rules says denying that would strand a takedown behind an Admin — so CLEARING the seat is allowed. Mirroring the wrong conjunct stranded exactly that takedown in the UI: the form locked, the submit button disabled, and a non-Admin could no longer clear a member off a CEL seat even though the rules permit it. positionsLockedForNonAdmin() now locks only on a power-granting cargo, where nothing the non-Admin submits can succeed. A grant-free CEL seat leaves the form open and is simply not offered, so the only submittable states are cleared or some other assignable cargo — which is the rules' answer. The two tests that asserted the old behaviour are rewritten to the corrected semantics rather than deleted, and a power-cargo case is added beside each so the lock still has a test. Both are mutation-proven: restoring the negation turns exactly them red. backstage 745->746. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviews: 82870df security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,secure-dep-vetting,bundle-budget-watcher
…t-code fix testable /code-review found that the client mirror added two commits ago was half a mirror, and that the "clearing is the allowed takedown" it promised was not reachable at all. Dropping the grant-free CEL seat from the options left it as the RHF value with nothing to render it, so the trigger showed the "Sin cargo" PLACEHOLDER for a member who holds a cargo; saving as-is re-submitted the cargoId into a 403; and Combobox clears only by re-selecting the currently-selected option, which was no longer in the list — with the test's own fixture the option list was empty, so there was no path to null whatsoever. The sibling form did the opposite of its comment: the pre-existing inactive-cargo fallback re-added the seat labelled "(inactivo)" — an ACTIVE cargo mislabelled, and re-offered to exactly the principal the rules reject. The third disjunct both forms used to "keep the current selection visible" was dead code, which is what let both happen. Both forms now share cargoOptionsForEditor()/cargoTakedownOnly(): the held seat renders DISABLED so the trigger names the real cargo, an explicit "Quitar cargo" clears it, and the save is blocked only while the denied seat is still selected. The lock note no longer names CEL as a cause it can no longer be. The audit script's exit sites move from process.exit() to process.exitCode. On POSIX a piped stdout is an async write, so process.exit() dropped whatever was still buffered — losing exactly the per-doc worklist a blocking pre-deploy gate exists to produce, while still returning the right code. Two defects in the new tests themselves, both found by running them rather than reading them: - the confirmation-gate test called its own helper positionally where it takes an options object, so --repair, --confirm and the env override were all silently dropped; it asserted a production refusal while actually running read-only against the emulator. - the truncation test attached its reader immediately, so the pipe drained as fast as the child filled it and nothing ever queued. Measured: it passed with process.exit() restored — vacuous against the one bug it exists for. It now leaves the stream paused, and the mutation yields "got 0 bytes". backstage 746->752, test:seed 28->30. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviews: be6831e security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,secure-dep-vetting,bundle-budget-watcher
knip: the /code-review refactor moved every consumer onto cargoOptionsForEditor(), cargoTakedownOnly() and positionsLockedForNonAdmin(), leaving the raw predicate exported with no importer. Module-local now. Keeping it unexported is the point rather than an accident: a caller that re-derives the option list from the raw predicate is exactly how the two member forms drifted apart and stranded the CEL takedown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviews: 24b7346 security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,secure-dep-vetting,bundle-budget-watcher
7 tasks
13 tasks
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
The fourth and last PR of the role-management overhaul (#216 → #219 → #221), plus the three residuals its predecessors documented and deferred. Design:
docs/specs/position-assignment-lane.md.A — the members-positions write lane. PR 2 withdrew
manage:PositionfromExecutiveCommitteeand deleted the positions-only rule with it, leaving cargo assignment Admin-only andmemberEditMode's"positions"arm unreachable. A new fourthallow updatearm onmembers, keyed onupdate:Positionand confined byhasOnly(['positions']), restores it.positionsAssignmentSafe()is reused verbatim, so the non-Admin branch still demandscargoAssignableByNonAdmin() && currentCargoGrantsEmpty()— grant-free, non-CEL cargos only, on both sides of a swap.B — well-formedness on the four soft-delete lanes. A member doc carrying the string
"false"inactivepassedsoftDeleteSafe(), was dropped bymemberDocSchemaso it was invisible throughout backstage, and was still published to the public Directiva, becauseproject-board.tstestedactive === false— the fail-open direction — whileproject-ally.tsused the fail-closedactive !== true. The helper gains the well-formedness prefixroleLifecycleSafe()already had, the three create arms gain the matching born-live requirement, and the projection flips to fail-closed.C — one three-way resolution. The absent/live/inactive built-in permission resolution was implemented twice, once per side of the trust boundary. Extracted to
@luminova/auth/built-in-perms; both sides delegate. Liveness derivation stays per-side, becauseisActiveRoleDocis fail-open andisLiveRoleis fail-closed — collapsing them is an unscoped behaviour change, and the previous comment gave the wrong reason (a type-onlyfirebase-adminimport).D — featured curation keyed on a perm, not a role name.
computeMemberRolesreads no role doc, so a deactivatedProjectManagerkept the name in its claim and kept authority over public-site content. The gate becomeshasAnyRole(['Admin']) || hasPerm('update:Showcase').hasPerm, deliberately notcanDo:canDowould letmanage:allsatisfy it, andmanage:allis reachable as a perm without the Admin role — the file documents that boundary as intentional, andcanDowould have deleted it silently.Test plan
Bundle: eager JS −92 B gz, index chunk +7 B, CSS unchanged, all budgets
ok, knip clean.What the review round caught
Recorded because each falsified something this branch asserted about itself. Full list in the spec's Review corrections.
categorypin was defeated one door over. Pinning it on the positions update arm left the same outcome reachable through create: a non-Admincreate:Positionholder could mint a fresh{category:'CEL', title:'Presidente', grants:[]}cargo — grant-free, so the power-grant check passes — and self-assign it onto the world-readable Directiva at board rank 0. "Seeded CEL cargos all carry grants" is what made it look blocked; nothing stopped an unseeded one. Closed byboardSurfacingCategory().boardRankreads the TITLE, not the category, so pinningcategoryalone still allowed renaming Vicepresidente to Presidente.title/titleFemalenow pinned on board cargos, left open on comisiones.category:'CEL'onto the doc a BLOCKING denial read, so afterwards the identical non-Admin write was an unchanged echo and was allowed. The denial passed only because vitest runs tests in declaration order. Measured by reordering.B3's reach was overstated, twice. It stops new malformed members being published, butonBoardMemberWrittenfires only on a member write — which B1 makes impossible from the client. My first correction said--repairwould take the stale row down; the next pass showed that is false in every branch — for a repairable doc the repair declares the member live and the row correctly stays, and theactive: "false"shape is refused so nothing is written at all. The script detects and gates; the row comes down by hand. It also had to learn to announce the opposite direction: repairing a missingactivecan publish someone, now gated behind--allow-publish.cargoAssignableByNonAdmin()— and then the next pass foundcreatePositionsSafe()still used the old predicate, so aMembershipholder could create a member born on such a cargo while the spec and owner-op 1 claimed "an Admin decision on both ends". Both arms now share the predicate;cargoGrantsEmpty()is deleted.Accepted deliberately, stated so they read as decisions
resolveTrustedGrantsreturns early ongrants.length === 0). Pinned by a test.currentCargoGrantsEmpty()reads only the current term, so between the UTC-year rollover and the victim's next write anupdate:Positionholder can displace a prior-term power grant. Pre-existing — it falls to anymanage:Memberholder today — but newly reachable by this role, which owner-op 1 now says out loud.update:Position, so no existing principal's authority changes. The hand-off is a custom role carryingupdate:Positionandread:Member.Owner-ops before deploy
pnpm audit:soft-delete-shapes. Malformed docs become admin-SDK-only the moment B1 ships, and a malformed member may be live on the public Directiva.--repairfixes the unambiguous shapes and refuses to guess at a non-boolactive.reseedBuiltInRolePerms, thenrecomputeAllClaims, then confirm a live ProjectManager's ID-token claim carriesupdate:Showcase.BUILT_IN_ROLE_PERMSis a seed snapshot — the constant alone mints nothing, and checking the role doc is not sufficient./positions, ADDSecretaryto the Secretario cargo's grants, THEN removeAdmin.Deploy order: beacon → reseed + recomputeAllClaims → verify claim → rules → hosting.
Reviews
Exactly the set
.claude/hooks/route.shprinted for this diff./security-review— run three times as the diff moved; found the create-side bypass of my owncategorypin. Final pass over the last rules delta: no findings at the reporting bar.Agent(firestore-security-reviewer)— three passes. Found thetitle-vs-categorygap and, on the last pass, thatcreatePositionsSafe()still lagged the update arm.Agent(firebase-functions-reviewer)— two passes. Found that--repairnever actually takes down a stale Directiva row, which falsified a claim I had written into the spec./simplify— 4 parallel angles; extractedbornLive(),memberWriteInvariants(),hasPerm().react-best-practices— one finding applied.Agent(bundle-budget-watcher)— eager JS −92 B, all budgetsok, knip clean./code-review— the user ran it after the PR opened, and it earned its place. It found that the client mirror I had just added was half a mirror: the "clearing a grant-free CEL seat" takedown the rules deliberately keep open was unreachable in the UI (the seat was dropped from the options, so the trigger showed the "Sin cargo" placeholder for a seated member and Combobox's clear path — re-selecting the selected option — had nothing to select), while the sibling form re-added that same seat labelled "(inactivo)". Also caughtprocess.exit()truncating the audit script's worklist on a piped stdout. Four adversarial correctness passes had run earlier on the full branch and on later deltas; they found the order-coupled fixture pair and the?? falsesentinel bug, and they missed these.secure-dep-vetting— exempt under the router's stated clause: the only dependency-file change on the branch is anexportsentry inpackages/auth/package.json. Verified, not assumed —git diff main..HEAD -- '*package.json' pnpm-lock.yaml pnpm-workspace.yamlshows zero dependency delta.Reviews ran in rounds against successive shas; each round's findings were fixed and the next round re-reviewed the delta.
Two of the tests written for the last round were themselves defective, found by running them rather than reading them: the confirmation-gate test called its own helper positionally where it takes an options object, so
--repairand the env override were silently dropped and it asserted a production refusal while running read-only; and the truncation test attached its reader immediately, so the pipe drained continuously and it passed withprocess.exit()restored — vacuous against the one bug it existed for. Both fixed; the truncation mutation now yieldsgot 0 bytes.