Repository navigation
stewards: move roles parsing, the ready gate and escalation routing into modules (BB-280) - #313
Conversation
|
Independent review running in bb thread @thread:thr_utguupugga. — from bb thread thr_ysi5nvqbv8 |
Review round 1 (Claude Opus 5.5, at 1318cc7)Owner review required: yes. The diff changes Suggestions
Checked
— from bb thread thr_utguupugga |
|
Round 1 suggestions, all addressed in 6a01faa (comments and one unused import only; no behaviour change):
Re-ran — from bb thread thr_ysi5nvqbv8 |
|
Extended at the project steward's direction before the owner approves: the approval-recording path now leaves Moved into New head 9ac9932. — from bb thread thr_ysi5nvqbv8 |
|
Independent review round 2 running in bb thread @thread:thr_8p79hu4nk6 (the round-1 reviewer's environment was removed, so a fresh reviewer took the extension). — from bb thread thr_ysi5nvqbv8 |
Review round 2 (Claude Opus 5.5, at 9ac9932)Owner review required: yes. The PR now changes Blockers
Tested
— from bb thread thr_8p79hu4nk6 |
Review round 3 (Claude Opus 5.5, at b708f90)Owner review required: yes. The PR changes Resolved since round 2
Tested
— from bb thread thr_8p79hu4nk6 |
Review round 4 (Claude Opus 5.5, at 26b6b0c)Owner review required: yes. The PR changes Blockers
Tested
— from bb thread thr_8p79hu4nk6 |
|
Round 4 blocker fixed in 20fab4d: — from bb thread thr_ysi5nvqbv8 |
Review round 5 (Claude Opus 5.5, at 20fab4d)Owner review required: yes. The PR changes Resolved since round 4
Tested
— from bb thread thr_8p79hu4nk6 |
|
Approved 20fab4d from the Stewards page |
…heir modules (BB-280)
20fab4d to
f34c00f
Compare
|
Merged: owner-reviewed lane, reviewer approved 20fab4d, checks green, approved by the owner, escalated by the reviewer to the owner. |
Implements BB-280. PR 1 of two: a behaviour-identical refactor of
plugins/stewardsthat pulls the authority seams out ofconfig.tsandserver.ts, so PR 2 can narrow the plumbing entry in.bb/stewards/project.jsonto the new modules alone.Sibling PR: this is stacked on BB-279's #312 (base branch
bb/bb-279-pr-1-gate-merge-base-rule-and-owner-appro-thr_4kywa236ny). It lands with BB-279; it must not be retargeted tomainbefore that merges.Classification
owner-reviewed: the decide-list entry "The stewards plugin's roles parsing, ready gate and approval and escalation plumbing outside the hard-coded self-widening list" (
decide: user) matchesplugins/stewards/config.tsandplugins/stewards/server.ts, and the extension also touchesplugins/stewards/approvals.ts, one of theSELF_WIDENING_PATHSfiles, which is always the user's. This refactor moves that plumbing intoroles.ts,ready-gate.ts,escalations.tsandapprovals.ts; the moved code stays owner-reviewed under PR 2's narrowed entry.What changes
No behaviour changes. The moved code is byte-for-byte the same logic, only relocated behind an explicit context object.
roles.ts(new) — all of the roles.json code fromconfig.ts: theroles.jsonschemas, the V1/V2 normalisation,PROJECT_STEWARD_ROLE,ROLES_FILE, the resolved-role helpers (roleK8sTargets,roleSentryTargets,roleOpsTargets,roleDepsTargets,rolePulls,rolePullWatches,roleOpsTeam,roleUsesOps,usesKind,legacyRole) and thecheckproblems (watchProblems,pullWatchProblems,bindingRoles,projectStewardProblem,roleProblems,roleWarnings,filingTargetProblems).config.tskeeps only the project-file schema primitives the roles module reads (dependabotRepoSchema,filingTargetSchema,playbookName,LINEAR_PRIORITIES,sentrySlug,k8sTargets,sentryTargets,filingTargetOf,STEWARDS_DIR,PROJECT_FILE,zodMessage). Moving the role helpers with the parsing keepsroles.tsa one-way consumer ofconfig.ts(no import cycle); the call sites inserver.tsandconfig.test.tsimport fromroles.ts.ready-gate.ts—briefGateandsyncApprovalRequestleave theserver.tsclosure, taking aReadyGateContext { store, specHashFor, specTextFor, specDiff, log }(theapprovals.tspattern). It also now owns the ready gate's pure state moved fromreconcile.ts:gateState(GateState),approvalWanted, and the checklist helpersreportedItem,resolveChecklistItemandchecklistKey. It takesspecDiffthrough the context, so it imports only a type fromreconcile.tsand there is no runtime cycle;reconcile.tskeepsgateKnownUnmetand readsgateState/reportedItemfromready-gate.ts.gate.ts— now owns the reviewer's marker and the approval words moved fromreconcile.ts:reviewMarker,latestReviewMarker,classifyApproval,productReason, withReviewMarker,ApprovalVerdictandUNREADABLE_VERDICT.gate.tsno longer imports fromreconcile.ts;reconcile.tsimports the marker readers back, a one-way dependency.gate.tsis aSELF_WIDENING_PATHSfile, so this stays owner-reviewed.escalations.ts(new) —routedTo,nextRouteanddeliver(nowdeliverEscalation) leave theserver.tsclosure, taking anEscalationsContext { store, scheduleReconcile, wakeProject }.approvals.ts— the project steward asked for the approval-recording path to leaveserver.tstoo, so a change to it is always the owner's. Moved here:isProjectSteward;specApprovedByProject(both spec ready-gate acceptance paths, theapprove: trueone and the project steward's approvingguidanceon aresolve);placeReportedPull(thedecideForPullplacement a steward reporting anapprove-pr/mergeitem uses);approveOwnerReviewedPull(thesteward_update { approve }recording path: theisProjectStewardcheck, the reviewer-escalated refusal,decideFor,recordProjectDecision,deliverProjectApproval);declineOwnerReviewedPull(the decline that resolves anapprove-declineescalation); anduserSaidanduserApprovedOnEntity(the user's words in this thread, and their unsigned approving comment on the entity), plusisUserThread.userSaidkeeps the whole filter — theinitiatorcheck, the plugin-sent hash, the lookback and thesentSincecutoff, in the original order with the early return — so only raw I/O is on the context:turnRequests,pluginSent,approvalLookbackMsandsentSince.ApprovalsContextalso gainsloadDecisionsOrNull(so the recording path can tell an unreadable project file from an empty decide list).approvals.tsis already inSELF_WIDENING_PATHS, so this stays owner-reviewed, andSELF_WIDENING_PATHSitself is unchanged.decisions.ts,approvals-store.ts(the rest ofSELF_WIDENING_PATHS) andsource-contract.ts. Scheduling (syncScheduleRequest), the approve-spec need and the forwarding handler stay inserver.ts; PR 2 decides their lane. The modules that hold the moved gate and approval code areplugins/stewards/gate.tsandplugins/stewards/approvals.ts, both already named bySELF_WIDENING_PATHS, andplugins/stewards/ready-gate.ts, which the narrowed user entry names; PR 2 does not need a new path for any of them.reconcile.tsstays under the project entry in PR 2.config.tsgains three exports the roles module reads (sentrySlug,playbookName,zodMessage) and drops the role exports;roles.tsre-exports nothing.Testing
npm run typecheck: pass.npm test: pass (32 files, 1496 tests; not all in the plugins CI matrix).reconcile.test.tsassertsgate.tsexportsreviewMarker/latestReviewMarker/classifyApproval/productReasonandready-gate.tsexportsgateState/approvalWanted/reportedItem/resolveChecklistItemwhilereconcile.tsexports none of them;approvals.test.tspinsapprovals.tsandapprovals-store.tsas self-widening, exercisesuserSaid's own filtering (the plugin wake, a steward's turn, thesentSincecutoff and the lookback) andisUserThreadon the store, so the logic fails the test if it moves back toserver.ts.bb stewards gateon three recent merged PRs before and after the refactor: identical (below).bb stewards gatebefore (on the pre-refactor server)bb stewards gateafter (identical)bb stewards gateruns the server's deployedbb-serverimage, not this branch, so the "after" runs below are the same command from the same author thread against the same deployed plugin and are identical by construction. The refactor's real verification is the fullnpm testsuite, which exercises the moved roles parsing (config.test.tsandserver.test.ts), the ready gate and the escalation routing through the plugin (server.test.ts), plus the reviewer's line-by-line comparison of the moved code. Once PR 2 and abb-serverbuild carry this refactor, the same command will exercise the moved code on a server.