Skip to content

fix(set-main): scope replace to the caller’s own Main Bot (#613) - #617

Merged
Birfy merged 1 commit into
mainfrom
fix/613-set-main-owner
Oct 21, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/613-set-main-owner

Conversation

@Birfy

@Birfy Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

set-main checked only body.id, so a signed-in member could take another owner's Main Bot's place: { id: <my Bot>, replace: <their Main Bot> } promoted their own Bot into the deployment-wide state.mainBotIds and demoted the other owner's Main Bot, whose Bot then lost the Main-only tools. This is option 1 of #613: owner-scoped replace, the smallest change that closes the demote path exactly.

What changed

  • core/src/host/authz.js — set-main keeps the body.id check and now also requires the actor to own body.replace when it names a different Bot. Taking another owner's Main Bot's place is bot/forbidden before the roster runs.
  • core/src/host/roster.js — setMain refuses a replace whose Bot has a different ownerId and leaves state.mainBotIds unchanged, so the invariant holds for every caller of the roster, not only the HTTP endpoint.
  • core/test/authz.test.js, core/test/roster.test.js — one test per half.
  • docs/subsystems/core-host.md (the core/src/host/ owner), changelog.d/613-main-bot-owner.security.md, and the regenerated patches/ds-bot/ (node scripts/ds-bot-patches.mjs generate + check).

Deliberately not in this PR: a per-owner state.mainBotIds (option 2) and blocked-shared for set-main with more than one member (option 3). Both are the team-mode decision #613 leaves open; today's shape — one deployment-wide list, where a member may still promote a Bot they own — is unchanged.

撤掉修复后变红

Reverted the two source hunks only (tests kept, authz.js back to if (ep === 'set-main') return requireBot(body?.id) and setMain back to its old replace branch):

cd core && DSH_HARNESS=<dsh checkout> node --import ./test/harness-resolve.mjs --test test/authz.test.js test/roster.test.js
# tests 48 / pass 46 / fail 2
not ok - #613: a member cannot take another owner’s Main Bot’s place
not ok - #613: a Main Bot’s place is only taken by a Bot of the same owner

Exactly the two new tests fail; the other 46 in those files pass.

修复后变绿

Same command with the fix in place:

cd core && DSH_HARNESS=<dsh checkout> node --import ./test/harness-resolve.mjs --test test/authz.test.js test/roster.test.js
# tests 48 / pass 48 / fail 0

The authz test covers: a member's swap of another owner's Main Bot is bot/forbidden; demoting that Main Bot by naming it as the target is refused; a swap inside one owner's team is allowed; the caller's own Bot may still be promoted plainly. The roster test covers the cross-owner replace being refused with state.mainBotIds unchanged on disk, and a same-owner swap still working.

跑了什么

  • node .github/scripts/changelog.mjs check → 23 fragments well formed.
  • node core/scripts/check-arch-docs.mjs → OK (6 changed file(s)) against a local origin/main at 81c708d.
  • node scripts/ds-bot-patches.mjs generate then check → core/ equals ds-bot/ + patches/ds-bot/.
  • The targeted suites above, red then green.

没跑什么

  • The full npm run verify: this box has no npm ci/npm run harness install for core/ (oxlint comes with it), so lint and the whole core suite run on CI here. CI's tests-ok covers lint, arch-docs, the client check, the patch check and the full core + plugin suites.
  • The plugin suites, the smoke test and the pack/typecheck of the package job: not run locally; unchanged code paths, and CI runs them.
  • No manual call to set-main in a live deployment; the two-owner behaviour is asserted at the authz and roster layers.

风险

Permissions surface. The change only narrows: it adds an owner check on replace and an owner invariant in setMain. No new configuration, no secret path, no change to rt.isMain or to the tool gates it drives. risk:T* is set by the controller.

Fixes #613

set-main checked only body.id, so a member could hand another owner's Main
Bot's place to their own Bot: the swap both promoted theirs to the
deployment-wide Main list and demoted the other owner's Main Bot. The
endpoint now checks the Bot named in replace, and setMain refuses a replace
across owners so the state invariant holds for every caller.

Tests: core/test/authz.test.js and core/test/roster.test.js, one per half;
docs/subsystems/core-host.md and a changelog fragment go with it.
@Birfy

Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at d974ee6

Tip SHA: d974ee63af7c6805789b464fd9d37e91d1f464f7
CI at tip: 8/8 success (tests-ok, package, plugins social/tree/timeline, ubuntu-latest Node 24,
           reviewed tip, risk tier)
Tier (from the diff): T3 — core/src/host/authz.js is a T3 path in .github/agent/risk-rules.json

Ran at this tip, not inferred from CI:

$ cd core && TZ=UTC DSH_HARNESS=… node --import ./test/harness-resolve.mjs --test test/authz.test.js test/roster.test.js
# tests 48 / pass 48 / fail 0

Gate ③, both directions — authz.js and roster.js restored to the base commit 81c708d0, tests kept:

not ok 20 - #613: a member cannot take another owner's Main Bot's place
not ok 48 - #613: a Main Bot's place is only taken by a Bot of the same owner
# tests 48 / pass 46 / fail 2

Exactly the two new tests go red, and only they.

What I checked beyond the tests:

  • set-main now runs requireBot(body.replace) when replace names a different Bot, so a member can no longer promote their own Bot into the deployment-wide Main list on the strength of owning body.id alone. The body.id check stays, so the endpoint still needs the caller to own the target.
  • roster.js throws unless replace and id share an ownerId, which keeps the invariant for any caller that bypasses the endpoint — the state layer refuses even if authz is ever refactored away.
  • A replace that is not a string, or names a Bot that does not exist, does not reach the swap: the first is skipped by the typeof guard and then handled (and refused) by roster.js, the second fails requireBot. Both fail closed; neither grants.

One note, not a blocker: the branch is based on 81c708d0, two merges behind main (3995d9b9). mergeable_state is clean and the queue rebuilds the tip, so this is informational.

Entering the merge queue now.

@Birfy
Birfy added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 3b2ab41 Oct 21, 2026
8 checks passed
@Birfy
Birfy deleted the fix/613-set-main-owner branch October 21, 2026 12:35
@Birfy

Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Post-merge record: 3b2ab414 (squash, via the merge queue). Reviewed tip d974ee63 was unchanged between the verdict and the queue. #613 is closed by this merge.

@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Birfy added a commit to Birfy/FlyBot that referenced this pull request Oct 23, 2026
…#670)

Fixes ThinkFlowLab#609

Tip SHA: 4c8d98c

## Reproduce

The rule text says `risk:T*` is set by the controller from the diff,
never by hand, and the
mechanism does not hold up: `github-actions[bot]` labelled ThinkFlowLab#574 at
07:28:43Z, while ThinkFlowLab#606, ThinkFlowLab#610,
ThinkFlowLab#611, ThinkFlowLab#612, ThinkFlowLab#616, ThinkFlowLab#617, ThinkFlowLab#618 and ThinkFlowLab#619 carry no `risk:T*` at all even
though `risk tier (report
only)` succeeded on every one of them. The owner decision on the issue
is ②: keep the workflow
read-only, never label — and make the computed tier auditable instead of
remembered.

At this tip, `node .github/scripts/risk-gate.mjs --pr <n>` ends its
report with

    risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3

so a review can quote the tier the rules produced instead of restating a
number.

## What changed

- `.github/scripts/risk-gate.mjs`: `judge()` records every changed path
(and a rename's old path)
with the tier `tierOf()` gave it, and `summaryLine()` renders the final
line of the report from
that same result — highest tier first, paths capped at 10 with a `+N
more` tail. `render()` ends
the job summary with it, after the prose, so the two can never disagree.
- `.github/scripts/risk-gate.test.mjs`: four cases — the line's exact
shape for a T3 PR, the
ordering and the cap, the no-changed-files case, and "the line is the
last line of the report".
- `AGENTS.md` gains a bullet under the label section: quote the summary
line, the workflow never
writes a label, the controller copies it on its poll, no review
transcribes a tier.
- `.github/labels.json`: every `risk:T*` description keeps `advisory`
and `the merge gate
recomputes it` and now also names the controller as the writer (all ≤
100 characters).
- `docs/controller.md`: the tier stays auditable without the label —
quote the summary line.
- `.github/workflows/risk-tier.yml`: header comment only — no permission
added, no step changed.
It keeps `contents: read` and `pull-requests: read` and still never
labels, comments or merges.

## What I ran

- `TZ=UTC node --test .github/scripts/*.test.mjs` at this tip (with
`git` on `PATH`) →
  **144/144**.
- `node --test .github/scripts/risk-gate.test.mjs` → **18/18**,
including the four new cases.
- `node .github/scripts/risk-gate.mjs` behaviour checked by hand through
the harness result:
`judge([AGENTS.md, plugins/social/src/social.js])` → `risk-tier: T3;
basis: AGENTS.md=T3,
plugins/social/src/social.js=T3`; no files → `risk-tier: T0; basis: none
(no changed files)`.
- `changelog.mjs check` not run: a change users cannot see needs no
fragment (changelog.d/README).

## Red without the source change

- Removing the `basis.push([name, t])` line from `judge()` (the tests
untouched) →
`risk-gate.test.mjs` **16/18, 2 red**, exactly `summaryLine: one
machine-readable line…` and
  `summaryLine: highest tier first…`.
- Removing `summaryLine(result)` from the end of `render()` instead →
**17/18, 1 red**, exactly
`render: the machine-readable line is the last line of the report`.
Restored → 18/18.

## Not run

- The workflow itself (it needs Actions): the change to `risk-tier.yml`
is a comment, and the
  existing `the workflow stays report only` test still passes with it.
- `npm run verify` end to end: no `core/node_modules` in this sandbox
(the same on `main`).
- The `risk-tier` job on this PR runs the base copy of the script, so
its summary will only show
the new line after merge — that is the workflow's normal base-copy
behaviour, not a gap here.

## Risk

T3 by the repository's own rules (`.github/**`, `AGENTS.md`): a
reporting script plus text. It
adds no permission, no network call, no label, comment or merge, and
cannot block anything — the
job is still `continue-on-error: true` and not a required check. The
only behaviour it changes is
the last line of that job's summary. The one thing a reader must not
conclude is that the label
now arrives on time: it does not, and the docs keep saying why (ThinkFlowLab#629).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:security Auth, grants, approvals, secrets bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set-main lets a member demote another owner's Main Bot and promote their own (owner check covers only body.id)

1 participant