019fbcf7 - Backfill staff verified names on PRD - #4574
Merged
Conversation
TaprootFreak
changed the title
Collaborator
Author
|
Review complete: 4 full review passes were required to reach zero blocking and zero actionable findings on the final head. All CI checks are green. |
TaprootFreak
marked this pull request as ready for review
August 1, 2026 12:41
TaprootFreak
pushed a commit
that referenced
this pull request
Aug 2, 2026
* feat(gs): grant staff clearance to the Debug account The staff-clearance rule (#4395 -> #4572) requires a non-empty user_data.verifiedName on top of the role for every elevated RoleGuard, including the Debug gate on POST /gs/debug. The backfill in #4574 covered two accounts; user data 403938 was not among them and still answers every elevated endpoint with STAFF_KYC_REQUIRED. Adds a PRD-only, idempotent, audited migration in the shape of #4574. The personal name is read from the deployment variable STAFF_VERIFIED_NAME_403938 and never appears in this repository. The closing assertion checks the clearance predicate itself — the BlankChars-aware BTRIM that StaffKycClearanceService uses — instead of equality with the supplied value. Should an identity-verified path have written a different, correct name in the meantime, that account is cleared and the deploy must not fail over the spelling. * fix(gs): make the backfill precondition the exact negation of its assertion The update repaired only a NULL verifiedName while the closing assertion demanded a non-blank one, so a present-but-blank name (a lone tab, a non-breaking space) was a state the migration refused to repair and then refused to accept. Because migrationsTransactionMode defaults to 'all', that throw rolls back the whole release's migration batch and fails DataSource.initialize() — a PRD crash-loop, not a failed tool. The precondition now uses the same BTRIM predicate as the assertion, so the two are complementary by construction and the blank case becomes a repair. Keeping a divergent existing name is no longer silent either: the audit entry records the outcome, so the deployed state cannot differ from the reviewed one without a trace. A re-run after a successful backfill stays a true no-op. Adds the migration spec the two preceding data migrations ship, including the blank-name cases that were the defect, so the verification runs in CI on the postgres:16 service instead of living in a throwaway container. * test(gs): pin the assertion's account scope and the duplicated BlankChars The spec left the non-target account's verifiedName NULL in every database case, so `count(*) = 1` held whether or not the closing assertion was scoped to the account. Dropping `"id" = 403938 AND` from the postcondition passed all 20 tests while in production it would count every cleared account and throw. The fixture now gives that account a cleared name, which kills the mutant with 10 failures. The blank-name cases covered four hand-picked characters, so the migration's copy of BlankChars could lose any of the other 21 code points unnoticed. The set is now derived from the runtime — every character String.prototype.trim() strips, which is what BlankChars is defined as — and the migration must repair a name built from all of them. Also drops a vacuous assertion: the log count was read after the rollback, where it holds regardless of what the migration wrote. docs/staff-kyc-clearance.md still prescribed the `verifiedName IS NULL` precondition this branch removed, so the next author following it would reproduce the boot-fatal gap. The recipe now states the exact-negation requirement, and step 2 no longer says the value is applied "only when present" when PRD in fact throws without it. * docs(gs): correct the JSDoc precondition and state the assertion the recipe requires The migration's JSDoc still said the update "only touches a still-null verifiedName", carried over from the migration this one is modelled on and false since the precondition became the negation of the closing assertion. This has to be fixed before the merge: api-migration-check.yaml allows comment-only edits to an existing migration by stripping whole lines starting with `//`, and JSDoc lines start with `*`, so afterwards the sentence could never be corrected. The runbook demanded the precondition be the exact negation of the closing assertion without saying what that assertion must be. The only precedent in the repo asserts equality with the supplied name, so an author following both would write an equality assertion — which throws when an identity-verified path wrote a different but valid name, and under the 'all' transaction mode takes the release's whole migration batch and the boot with it. The recipe now states the clearance-predicate assertion explicitly, and names `develop` rather than `main` as the last point where the deploy order can still be arranged. Two spec corrections: the BlankChars pin claimed a drifted copy would fail the migration's own assertion, when in fact the postcondition shares the drifted constant and reports success — the drift is silent, which is precisely why the test asserts the repaired state. And a test name still promised a post-rollback assertion that no longer exists. Adds the sanity floor on the derived character set that the sibling spec already carries. * docs(gs): make the recipe's negation NULL-total and require the audit coupling Taken literally, "the exact negation of that assertion" produced `BTRIM("verifiedName", <BlankChars>) = ''` — without the COALESCE that the printed precondition one paragraph earlier does carry. That is not the negation over NULL: a NULL name yields NULL rather than true, so the ordinary un-backfilled account is neither repaired nor accepted, which is the same boot-fatal shape the paragraph exists to prevent. Verified as a mutant against the shipped SQL: three tests fail with the postcondition error. The instruction now spells out both forms and says which is wrong. The ban on equality assertions was justified with only one of its two failure modes, and that one is unreachable once the negation rule is applied — inviting the conclusion that the ban is redundant. Both branches are now stated: against a blankness precondition an equality assertion throws over a spelling difference, against its own negation it silently overwrites an identity-verified name. The recipe also never mentioned the audit coupling, though both shipped backfills gate the write on EXISTS (SELECT 1 FROM "audit") and CONTRIBUTING treats unaudited mutation of a PII column as blocking. An author following only the runbook produced an unaudited migration. Corrects the release-PR mechanism too: auto-release-pr.yaml checks for an open PR first and creates one only when none exists, so it keeps a release PR open continuously rather than opening one per push. The conclusion — develop is the last point where the deploy order can be arranged — is unchanged.
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.
What changed
STAFF_VERIFIED_NAME_375162and is not stored in this public repository.GSheet.Why
Since #4572, staff clearance is based on a populated
verifiedNamerather than KYC level 50. The two existing production accounts do not have that value yet and therefore remain blocked withSTAFF_KYC_REQUIRED.Impact and deployment
developtomainrelease and production deployment are required before the backfill takes effect.Checks
node --check migration/1785584840000-BackfillStaffVerifiedNames.jstsc --noEmit