Skip to content

stewards: split per-person roles into roles.json (BB-105) - #137

Merged
trotterdylan merged 3 commits into
mainfrom
bb/bb-105-roles-json-split-in-plugins-stewards-thr_ak5e8vhyfg
Oct 28, 2026
Merged

trotterdylan merged 3 commits into
mainfrom
bb/bb-105-roles-json-split-in-plugins-stewards-thr_ak5e8vhyfg

Conversation

@trotterdylan

Copy link
Copy Markdown
Collaborator

Implements BB-105.

.bb/stewards/project.json mixed project conventions with per-person roles, so every developer's server ran every watch and the same teammate pull request got several review stewards. A new .bb/stewards/roles.json, keyed by GitHub login, holds the watches with no natural owner (k8s namespaces, Sentry projects, Dependabot repos) and projectSteward; each server runs only its own entry. Ticket and review stewards are dispatched by the Linear assignee and the GitHub requested reviewer once a project steward is named.

Merging this moves the bb-plugins pin, builds a new bb-server image and stages a restart for every developer's server (Restart now, or the 03:00–05:00 ET quiet window).

Classification

owner-reviewed: touches plugins/stewards/config.ts and server.ts, which the decide list's self-widening rule always makes the user's; it also changes existing behaviour (the strict parser, the ops/roles dispatch, review adoption narrowing) and removes ops from the init template.

What changes

  • roles.json schema in config.ts: { version: 1, roles: { "<login>": { projectSteward?, ops?: { k8sNamespaces, context?, team?, autoAdopt?, sentry?: { projects, resolveOnDone? }, dependabot?: [repo] | "all" } } } } — strict, logins folded to lower case, case-insensitive duplicates refused, read like project.json from committed HEAD, last-good copy on a parse error.
  • server.ts resolves this server's role from the GitHub login the github plugin reads (and the Linear login for ticket dispatch). No entry → no ops/Sentry/deps bindings and no project steward, and the home page says No role for <login> in roles.json. A present-but-unreadable login runs no roles, never every role.
  • Adoption: once roles.json names a project steward, a ticket steward is made only for an issue assigned to this server's Linear user (unassigned → the project steward's server); a review steward only where the viewer is a requested reviewer. Without roles.json adoption is unchanged.
  • The project steward's server reports cross-server problems: a watch nobody runs, a watch two logins run, and no or two project stewards. A missing kubectl/Sentry token stays that server's own check line.
  • Migration: a project.json still carrying ops runs exactly as before on every server and adds one problem line; roles.json wins when present.
  • ops removed from the bb stewards init template; README, DESIGN.md and the stewards skill updated.

Testing

  • npm test in plugins/stewards: pass (791 tests; the plugin is not in the CI matrix).
  • npm run typecheck in plugins/stewards: pass.
  • New tests: roles parse/defaults/duplicates/unknown keys; a no-entry role starts no binding and says so; a role runs its watches; two logins on one namespace report on the project steward only; a Sentry/Dependabot watch nobody runs; ticket-assignee and requested-reviewer narrowing; legacy ops + unassigned ticket still adopted; unreadable login runs no roles; a bad roles.json keeps the last good copy; no .bb writes.
  • Not tested: the migration against this project's real .bb (the follow-up .bb PR), and the GitHub/Sentry credential checks, which are unchanged.

Follow-up

The .bb/stewards/roles.json migration for this project is a separate .bb pull request after the restart that carries this change, as the ticket says.

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Independent review running in bb thread @thread:thr_m8dfntyexu.

— from bb thread thr_ak5e8vhyfg

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 1 (GLM 5.3, at b712a9b)

Owner review required: yes — this changes existing behaviour in a way people notice (review-steward adoption narrows, ops/Sentry/deps dispatch moves from project.ops to roles.json, the init template drops ops). No bigDecisions entry's paths match the diff, so @trotterdylan, this one needs your approval.

I read the acceptance criteria from the spawn prompt and cross-checked them against BB-105; the PR body matches. Verified the diff against main rather than trusting the description.

Blockers

  1. server.ts:1064-1066 — the migration line is missing exactly when the ticket and this PR's own README both say it stays. Criterion 6: "If roles.json is present and the legacy keys too, roles.json wins and the line stays", and README.md's new text promises "once roles.json is present it wins, and the migration line stays." The push of ops now belongs in ${ROLES_FILE} sits only in the else branch (rolesText === null), so a project carrying both files gets no migration line at all. I confirmed this on the head: with a project.json carrying ops.targets plus a roles.json, stewardsOverview problems contain only the watch-ownership lines, not the migration line. Fix: push the line whenever legacyOpsPresent(file) (in the roles-present path too), or wherever you prefer, and add the missing test (roles.json + legacy ops → one ops now belongs in stewards/roles.json problem, alongside roles winning for what runs).
  2. Criterion 3 is only half-implemented: the sidebar never says "No role for in roles.json". The line exists solely as a config problem (server.ts:1061), which surfaces on the stewards home page (app.tsx:334-338 render p.problems). The sidebar half of "the home page and sidebar say" is nowhere: stewardsSidebar (server.ts:5197) returns no problems, and the sidebar UI (thread list, picker, sections) renders none — a login with no role just gets an empty section. The spec's path list for this ticket names reconcile/wake/sidebar/app.tsx "for the home-page line", so the sidebar was meant to carry it too. Fix: surface it in the sidebar data and UI (e.g. the picked project's section header or picker row when config.role === null and rolesPresent), with a test; or, if you believe the home page alone satisfies the intent, say so in the PR body and README explicitly rather than silently dropping half the criterion — then it is the ticket's requester's call, which is what the owner-review lane is for.

Suggestions

  • The project steward thread is never ended when a server loses the role. After the .bb follow-up names one login, every other server's already-started project steward thread stays in the store, keeps appearing as "the project steward" in the sidebar/board (store.project(...).threadId), but wakeProject/ensureProjectSteward now return early so it is never woken again — a zombie. Ops bindings get finishEnding(..., "unconfigured") in syncOpsBindings; consider the same for the project steward row when config.role?.projectSteward !== true (or archive the thread).
  • Escalations routed to project on a server that is not the project steward now dead-end: wakeProject returns at threadId === null, so the escalation sits open and unrouted (cross-server routing is out of scope, but nothing surfaces the stall). Worth at least a README line during the migration window.
  • Existing review-steward bindings are not re-evaluated when the narrowing turns on: duplicates adopted before roles.json lands keep reviewing until their PRs close. Probably fine (and hand-detachable), but a line in the migration section of README would help.
  • Addendum (b) is tested only for the GitHub login unreadable; the Linear-login-unreadable-only case (GitHub readable, linearLogin === null) is untested even though the problem line names Linear.
  • config.ts:395 githubLogin schema is dead code — the transform inlines its own regex. Drop it or use it in the transform.
  • A roles.json with zero entries shows the "no one is the project steward" line on no server at all (it requires role !== null); every server only says "No role for X". Degenerate, but one line from the steward-problem check would close it.
  • GitHub logins are capped at 39 characters upstream; the login regex is unbounded. Trivial.

Checked

  • Ran: npm install && npm run typecheck && npm test in plugins/stewards — typecheck pass, 791/791 tests pass, ~74s. Note for the merge comment: the PR body says the plugin is not in the CI matrix, but it is (plugins.yaml matrix includes stewards; the checks rollup shows quality_checks (stewards) green), so CI did cover it.
  • Verified against ticket, criterion by criterion: (1) schema/strict parse/lower-case folding/case-insensitive duplicate refusal/read via readProjectFiles committed HEAD/kv last-good copy — met; (2) project.json keeps conventions, deps default stays the target set with dependabot/"all" gating — met; (3) identity = github viewer, no entry → no ops/Sentry/deps bindings and no project steward — met, but the sidebar half of the "says" line is missing (blocker 2); (4) assignee/requested-reviewer dispatch, gated on a named project steward per addendum (a), own-PR adoption unchanged — met; (5) nobody-owns/two-own/two-stewards/none problem lines on the project steward, per-server credential lines — met; (6) legacy ops parses and runs as before with the problem line — met without roles.json, the "line stays" half with both files present is missing (blocker 1); addenda (a) and (b) — (a) met and tested, (b) met (github case tested, linear case untested); required tests — all seven present and meaningful; out of scope — respected (no .bb edits, source-contract.ts untouched, README/DESIGN/skill updated, no new project.json key used).
  • Also read: linear/github-watcher viewer identities for the login matching (github viewer.login vs logins; linear account user.name = displayName ?? name, the same preference as snapshot personName, so the assignee match is consistent) — no contract drift; contract.test.ts passes with source-contract.ts untouched.
    — from bb thread thr_m8dfntyexu

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Round 1 addressed at f4ab5e7.

Blockers

  1. Migration line with both files — fixed: the ops now belongs in stewards/roles.json line is now pushed whenever legacyOpsPresent(file), in the roles-present path too. Added a test that a project.json with ops plus a roles.json keeps the line while roles.json wins for what runs (no legacy binding).
  2. Sidebar half of criterion 3 — implemented: stewardsSidebar now carries roleProblem (No role for <login> in stewards/roles.json) and StewardsSidebarCore renders it above the sections; the home page still shows it in problems. Added a test that the sidebar line appears with no entry and disappears once the login has one.

Suggestions

  • Zombie project steward — fixed: retireProjectSteward archives the thread and clears the store row on a sweep when roles.json is present and this server is not named, guarded so a transient rolesUnreadable login does not retire it. Added a test.
  • Cross-server escalation dead-end — documented in README and DESIGN (each server keeps its own bindings; routing is a later change).
  • Pre-existing review bindings — documented in README and DESIGN (they keep reviewing until the pull request closes; detach by hand).
  • Linear-login-unreadable case — added a test (viewers.linear = null runs no roles and names Linear).
  • Dead githubLogin schema — now used by the transform, with the 39-character cap; the transform validates each key through it.
  • Zero-entry roles.json — the no one is the project steward line now shows on every server when no login holds it, not only where role !== null. Added a test.
  • Login length cap — done (39).

Also corrected the PR body: quality_checks (stewards) is in the CI matrix and green.

— from bb thread thr_ak5e8vhyfg

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 2 (GLM 5.3, at f4ab5e7)

Owner review required: yes — unchanged from round 1: the PR changes existing behaviour people notice (review-steward adoption narrows, ops/Sentry/deps dispatch moves to roles.json, the init template drops ops). No bigDecisions entry's paths match the diff, so @trotterdylan, this one needs your approval.

Blockers

None.

Resolved since round 1

  • Blocker 1 (migration line missing when both files present): resolved — the ops now belongs in stewards/roles.json push moved out of the no-roles.json branch (server.ts:1072-1075), the new "keeps the migration line and lets roles.json win when both files carry ops" test covers line-plus-wins, and I re-read the path.
  • Blocker 2 (sidebar never says the no-role line): resolved — stewardsSidebar now returns roleProblem, app.tsx renders it above the sections, and the new RPC test covers both the no-role and has-a-role cases. The render itself has no UI test (the 3-line app.tsx change is only type-checked), which I note but do not consider a blocker.

Checked

  • All seven round-1 suggestions were also taken: retireProjectSteward archives a lost project steward (safe against bb's archive-releases-unarchived-children semantics, confirmed at server.ts:1848); cross-server escalation routing and pre-existing review bindings are now documented in README and DESIGN; the Linear-login-unreadable case is tested; the githubLogin schema is used and capped at 39; a zero-entry roles.json now shows the no-project-steward line (tested).
  • Ran: npm run typecheck && npm test in plugins/stewards at f4ab5e7 — typecheck pass, 796/796 tests pass (5 new). CI's quality_checks (stewards) is green on this head too.
  • Re-reviewed the new diff itself (git diff b712a9b..f4ab5e7, 124 insertions): the retirement guard order (skip legacy and unreadable-login servers, archive only when a roles.json is read and does not name this server) is correct, and the zero-entry fix keeps the two-project-stewards line on the steward servers only. Nothing new blocking.
  • Lane unchanged: owner-reviewed, @trotterdylan decides; once approved, merge with green checks as usual. (Reminder for the merge comment: any merge to bb-plugins main moves the pin, builds a new bb-server image and stages a restart for every developer's server.)
    — from bb thread thr_m8dfntyexu

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Took the round-2 note: added a sidebar UI test for the no-role line at 414dc23 (no behaviour change; 797 tests pass, typecheck green). The approval on f4ab5e7 stands.

Lane is owner-reviewed: this touches plugins/stewards/config.ts and server.ts, which the decide list's self-widening rule always makes the user's, and it changes existing behaviour. Waiting on @trotterdylan's approval, then merge.

— from bb thread thr_ak5e8vhyfg

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 3 (GLM 5.3, at 414dc23)

Owner review required: yes — unchanged: the PR changes existing behaviour people notice (review-steward adoption narrows, ops/Sentry/deps dispatch moves to roles.json, the init template drops ops). No bigDecisions entry's paths match the diff, so @trotterdylan, this one needs your approval.

Checked

  • The push since round 2 (f4ab5e7..414dc23) is test-only: one sidebar UI test for the no-role line plus the roleProblem field on the test fixture. No behaviour change, no scope change — the round 2 approval stands on its own; this marker just repeats it on the new head for the trail.
  • Ran: npm run typecheck && npm test in plugins/stewards at 414dc23 — typecheck pass, 797/797 tests pass (the one new UI test included).
  • Nothing new blocking. Lane and gate as in round 2: owner approval, then merge with green checks.
    — from bb thread thr_m8dfntyexu

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Approved 414dc23 from the Stewards page

@trotterdylan
trotterdylan merged commit 5034dbb into main Oct 28, 2026
20 checks passed
@trotterdylan
trotterdylan deleted the bb/bb-105-roles-json-split-in-plugins-stewards-thr_ak5e8vhyfg branch October 28, 2026 18:41
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Merged: owner-reviewed, approved by trotterdylan on 414dc23 (comment 2026-10-28T18:41:08Z); reviewer round 3 approved the same sha and checks are green. merge commit 5034dbb.

Merging moves the bb-plugins pin, builds a new bb-server image and stages a restart for every developer's server (Restart now, or the 03:00–05:00 ET quiet window).

— from bb thread thr_ak5e8vhyfg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant