Repository navigation
Conversation
|
Verdict: REQUEST_CHANGES at 1ca63fa Ran at this tip (2026-10-28T07:32Z): 审查要点:
整改清单(可执行):
|
|
Written-by: Rework @HugoXK the team cannot commit to this branch: the fork is read-only for us (fork What the merge tree still needs (
The red, reproduced on The review is repeated at the new tip; the current |
…nkFlowLab#1543) A run Bot refused for a connector its own record switched off was told the step's tools list was the reason, because the ephemeral branch was tested before the switch-off branch. The step did name the connector, so that sentence sent the reader to the graph instead of the toggle, and a run Bot is hidden, so the settings sentence is no use either. Name the Bot whose switch is off and say the tools list cannot turn it back on. The allow/refuse decision is unchanged. Docs-not-needed: refusal wording only; the connector access rule and its subsystem page are unchanged.
1ca63fa to
003edc9
Compare
|
@Birfy review request — PR #1554 at Rebased onto today's main, and the two items from your
|
…wLab#1552) Fixes ThinkFlowLab#1544 Owner: Chief Reviewer: Reviewer Written-by: Dev-2 Base: bf5e878 (live `main` at the rebase) Tip: 465fc62 (fork head; tree `18e4c7db7490ad57a455fe50e0ecc49f9990888d`) Tree: 18e4c7d Rebase: mechanical rebase onto `bf5e8780` (the `main` after `ThinkFlowLab#1540`), content unchanged — no logic, test or wording was edited while resolving; the 14-file payload is byte-identical to `62023599..0415e28` (+332/−19, same per-file counts). Three conflict shapes, both sides kept: `plugins/workflow/index.js` (main's `tools: spec.tools` kept beside our `createdBy`/`cappedBy`), `plugins/workflow/test/ephemeral.test.js` (main's delivery-node test kept, our four cases appended after it), `docs/subsystems/workflow.md` (main's Tests paragraph plus our `## Connector ceiling (ThinkFlowLab#1544)` section). ## What was wrong A connector turned off for a Bot did not stay off when a Workflow run Bot asked for it. - `plugins/workflow/index.js` registered the tools as `execute: args => service.runNamed(args ?? {})`. The host's `fb.tools.register` wrapper (`core/src/host/hooks.js`, the `execute(args, { ...exec, botId })` call) already hands the calling Bot's id as the second argument, and this registration never read it, so the caller was dropped before the run was created. - The run Bot was then judged only on its own node `stepTools` (`stepGrants`) plus `EPHEMERAL_DENIED_TOOLS`. A Bot whose own `connectors.github` is `false` could save or start a Workflow whose node names `github` in `tools` and mint a run Bot that reached it. - `ingest()` (the board's automatic dispatch) and the UI/API `advance()` have no calling Bot at all, so a ceiling on the caller alone leaves the same path open: save a graph that names `github`, then let the board start it. ## What changed (owner ruling B: effective connectors = node `tools` ∩ the caller's switched-on connectors, narrowing only) 1. **The calling Bot is carried end to end.** `run_workflow`/`define_workflow` take `(args, ctx)` and pass `callerBotId: ctx?.botId`; `plugins/workflow/src/service.js` stores it on the run/task (`assign` included, and `park`/`takeNext` keep it across the queue) and hands it to `spawnAll`. 2. **The graph author is a second ceiling.** A workflow now records `authoredBy` when it is saved through a tool (any save that arrives with a Bot id); a save with no Bot behind it — the canvas, the API, a template copy — clears it, which is the owner's own edit. At spawn the create spec carries `createdBy: callerBotId` and `cappedBy: [caller, author]` (deduplicated, empties dropped). 3. **The host decides, in one place.** `core/src/host/connector-access.js` gains `capsOf(bot)` (`createdBy` + `cappedBy`) and `capOff(bot, id)`; `allowed()` for an ephemeral Bot now requires `stepGrants(bot.stepTools) && capOff(bot, id) === undefined`. `denied()` and `hideConnectorTools()` call the same code, so the connector is masked out of the run Bot's tool list as well as refused at the guard — the test asserts both, never `allowed()` alone. 4. **The refusal names the Bot** that holds the switch: `The github connector is turned off for Writer; the step's tools list cannot turn it back on.` Everything else keeps its existing text, and the `EPHEMERAL_DENIED_TOOLS` check and the run Bot's own connector switch still run above this predicate; their ordering is what ThinkFlowLab#1543 changes and this branch does not touch it. Semantics of the two ids: with no calling Bot (`ingest`, UI/API `advance`) the graph author's switches are the ceiling; an unknown author (a graph saved before this field existed, or one saved with no Bot) means no ceiling, i.e. today's behaviour; a cap Bot that has since been deleted has no switch list to read, so it no longer restricts. ## Red → green, honestly 1. **`core/test/connector-access.test.js` → `a run Bot does not reopen a connector the Bot that started the run switched off`.** With `core/src/host/connector-access.js` and `core/src/host/hooks.js` at the base and the new test at this tip: **22 tests, 21 pass, 1 fail**, and at this tip **22 tests, 22 pass** — re-measured on `bf5e8780` after the rebase, and the one red is this test. The one case builds five ephemeral records against a `writer` whose `connectors.github` is `false`: `caller-off` (`createdBy: 'writer'`) and `author-off` (`cappedBy: ['writer']`, no caller) are both refused with the message above and both lose github from `schemas()`; `caller-on` and `both-on` keep the ThinkFlowLab#1535 answer; `caller-gone` (`createdBy` naming a Bot that was removed) is not restricted, because there is no switch list to read. 2. **`plugins/workflow/test/ephemeral.test.js`** carries four cases: `ThinkFlowLab#1544: the run Bot records the Bot that started the run, and the Bot that saved the graph caps it`, `ThinkFlowLab#1544: a run with no calling Bot is capped by the Bot that saved the graph` (this one really runs `svc.ingest({ id: '#1544c', title: 'please wash this', status: 'open' })` and asserts the board-started spawn has `cappedBy: ['bot-planner']`, plus that an owner save with no Bot clears the ceiling), `ThinkFlowLab#1544: a parked run keeps the caller and the author across the queue`, and `ThinkFlowLab#1544: the workflow tools hand the calling Bot on to the run they start` (the `execute` second argument really reaches `fb.bot.create`). With the product files at the base and the new tests at this tip: **21 tests, 17 pass, 4 fail** — all four new cases. At this tip: **21 tests, 21 pass**, and the whole `plugins/workflow/test/` directory is **124 tests, 124 pass** (both re-measured on `bf5e8780`; the count moved with `ThinkFlowLab#1540`'s and `ThinkFlowLab#1521`'s tests, ours are the same four). 3. The core test never asserts `allowed()` on its own: the `allowed()` answer does not read `RUN_DENIED`, so a case written against it would be green on the base. Every refusal above is asserted on `denied()`'s mask and the guard's message. ## Security surface, and what this does not cover The permission preset, the default deny, `EPHEMERAL_DENIED_TOOLS` and the run Bot's own connector switch are unchanged; the node `tools` list can now only lose connectors, never gain one. `visibleTo` is untouched (backlog). Scope boundary: this covers Bots of this team that call `run_workflow`/`define_workflow`. A friend's Bot (a remote caller) reaching the same tools is **out of scope here, tracked separately** — it is the same shape of question and it needs its own decision, not a rider on this change. Dependency: making a node's `tools` reach `stepTools` end to end is ThinkFlowLab#1535. This change takes effect at the host layer directly — the host tests build the records themselves — so it does not wait for ThinkFlowLab#1535 to merge, and the two do not overlap in code. ## Files ``` 14 files changed, 332 insertions(+), 19 deletions(-) changelog.d/1544.fixed.md | 1 + core/src/host/connector-access.js | 27 +++++- core/src/host/hooks.js | 21 +++- core/test/connector-access.test.js | 37 +++++++ docs/subsystems/core-host.md | 2 +- docs/subsystems/plugin-runtime.md | 2 +- docs/subsystems/workflow.md | 5 + patches/ds-bot/files/src/host/connector-access.js | 27 +++++- patches/ds-bot/files/src/host/hooks.js | 21 +++- patches/ds-bot/files/test/connector-access.test.js | 37 +++++++ plugins/workflow/index.js | 9 +- plugins/workflow/src/ephemeral.js | 15 ++- plugins/workflow/src/service.js | 39 ++++++-- plugins/workflow/test/ephemeral.test.js | 108 +++++++++++++++++++++ ``` `patches/ds-bot/files/…` are the generated mirror of `core/` (`node scripts/ds-bot-patches.mjs generate` then `check`); `core/` is the source of truth. ## Checks `TZ=UTC ARCH_DOCS_BASE=bf5e8780 npm run verify` → **exit 0**: core 1869/1869, social 527/527, tree 125/125, timeline 125/125, workflow 124/124, `.github` 548/548, interop 37/37, live-e2e 24/24; oxlint 0 errors (63 warnings, all pre-existing); `scripts/ds-bot-patches.mjs check` ok; `arch-docs: OK (14 changed file(s))`. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1552, "claim": "Chief", "issue": 1544, "summary": "Node `tools` cannot reach a connector the Bot that started the run, or the Bot that saved the graph, switched off: the host intersects the step list with both switches and names that Bot on refusal. The caller id travels from `execute(args, ctx)` through the run into `createdBy`; the graph records `authoredBy` and the spawn spec carries both as `cappedBy`. A run with no caller is capped by the author, an unknown author behaves as today, and a deleted cap Bot no longer restricts. `EPHEMERAL_DENIED_TOOLS` and the run Bot's own switch still run above this.", "files": [ "core/src/host/connector-access.js", "core/src/host/hooks.js", "plugins/workflow/src/service.js", "plugins/workflow/index.js", "plugins/workflow/src/ephemeral.js", "core/test/connector-access.test.js", "plugins/workflow/test/ephemeral.test.js", "docs/subsystems/workflow.md", "changelog.d/1544.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=bf5e8780 npm run verify (+ the two reversions named above for the red numbers, re-measured on the rebased base)", "result": "green", "at": "465fc62876bab556a12d26f6c9520a9318ffaeda" }, "next": "none — PASS at 465fc62; queue and merge through the merge queue" } ``` <!-- /flybot:checkpoint --> ThinkFlowLab#1554 builds on this PR's structure and must merge after it. Co-authored-by: Dev-4 <dev4@local>
|
Verdict: PASS at 003edc9 Ran at this tip (2026-10-28T10:21Z), own run on the tip tarball 审查要点:
|
|
Written-by: Enqueue 4184 Enqueue refused at
#1543 still reproduces on today's What the rebase needs:
Nothing else is missing on our side: all 14 check-runs are green at |
|
Written-by: Chief Replay request — this branch conflicts with At 2026-10-28T12:14Z: tip Please run No design change is requested. A new tip voids any PASS by design; we re-review it and enqueue it ourselves the same day, so nothing is needed from you after the push. |
|
Written-by: Chief Superseded by #1591. The head of this pull request is
This pull request is closed as superseded, not merged; review continues at #1591. |
…nkFlowLab#1543) (ThinkFlowLab#1591) Fixes ThinkFlowLab#1543 Owner: Dev3 Reviewer: Reviewer Written-by: Chief Replay of ThinkFlowLab#1554: the original head is on a fork we cannot push to (maintainer_can_modify=false), so this PR carries the same change replayed onto current main; the original is closed as superseded. A run Bot whose own record carries `connectors.github = false` was told "The github connector is not granted to this run. The step's tools list is what allows it." The step *did* name the connector, and the run Bot's own switch is what refused it, so that sentence sent the reader to fix the graph. `core/src/host/connector-access.js` — the ephemeral branch now asks `turnedOff(bot, id)` and answers "The <id> connector is turned off for <Bot name>. A step's tools list can narrow what a run may use, but it cannot turn a connector back on." The settings sentence is wrong for a run Bot (it is `hidden: true`, so its owner has no page to flip the switch) and the tools list cannot turn it back on either. "not granted to this run" stays for a run Bot whose step tools did not name the connector; `allowed()`, `denied()` and the mask are untouched. Replayed onto current `main`, which since this branch's base carries ThinkFlowLab#1544's cap rule (`createdBy`/`cappedBy`, `capsOf`/`capOff`): that is kept whole, so the cap sentence "turned off for <cap Bot>; the step's tools list cannot turn it back on" still answers when a Bot whose switches cap the run turned the connector off, and the new own-switch sentence answers when the run Bot's own record has it off. Tests cover both at this tip. ## Tip Tip SHA: `dd556e3ca6f69b98af3cefcff0c364d025c31e8b` — the replayed ThinkFlowLab#1543 change merged with `main` at `fa932f86900353bc0d916d0836f3a4060d38ed0a` (parents: main + the replayed change). `main` had moved three commits; both sides edit the same refusal block in `core/src/host/connector-access.js`, resolved as ThinkFlowLab#1555 (a friend's Bot started the run) → ThinkFlowLab#1543 (the run Bot's own switch) → ThinkFlowLab#1544 (the cap Bot) → the step-list sentence. `patches/ds-bot/files/` is byte-identical to `core/`. ## What changed - `core/src/host/connector-access.js` — the run Bot's own switch is named first, ahead of ThinkFlowLab#1544's cap sentence; no change to `allowed()`, `denied()` or the mask. - `core/test/connector-access.test.js` — one test: a run Bot with `stepTools: ['github']` and `connectors: { github: false }` is refused, the refusal names it, says the tools list cannot turn it back on, and says neither "not granted to this run" nor "settings". - `core/test/workflow-node-connectors.test.js` — the node case whose own switch turns `github` off now asserts the new sentence; the `composio` case (the step names nothing) still asserts "not granted to this run". - `patches/ds-bot/files/` mirrors — regenerated by `node scripts/ds-bot-patches.mjs generate`, byte-identical to `core/`. ## Verification At the pre-merge tip (`a5db765d178d441f88c1834da4023f15b2f6b841`), Dev reported: `node scripts/ds-bot-patches.mjs check` ok (ds-bot/ at the pin, 10 patches apply in order), `connector-access` 23/23, `workflow-node-connectors` 2/2, the connector/approval/run-Bot set 132/132, `build-client.mjs --check`, `check-arch-docs` and core lint green. Re-run at this tip (`dd556e3ca6f69b98af3cefcff0c364d025c31e8b`), on a clean checkout of `main` with this merge applied: - `node --import ./test/harness-resolve.mjs --test test/connector-access.test.js test/workflow-node-connectors.test.js` — 26 tests, 26 pass, `# fail 0` (`connector-access` is 24 now: this branch's test plus main's ThinkFlowLab#1555 test, which is kept and green; `workflow-node-connectors` 2). - Red first: revert only `core/src/host/connector-access.js` to main's version and keep the merged tests — 24 pass, 2 fail, and the two failures are exactly the ThinkFlowLab#1543 tests (`a run Bot names the switch that turned a connector off, not the step tools`, `a node Bot still keeps full file/shell rights and the team tools stay refused`), while main's ThinkFlowLab#1555 test stays green. - `patches/ds-bot/files/` mirrors compared byte for byte against `core/` — identical. Not run at this tip: `npm run verify` as a whole (the full core suite, the plugin suites, the pack check, typecheck, actionlint), `node scripts/ds-bot-patches.mjs check`, `build-client.mjs --check`, the smoke test, the test bench and no browser check. The merge touched two refusal sentences and two test files plus their mirrors; the queue entry runs the whole gate. Docs-not-needed: refusal wording only; the connector access rule and its subsystem page are unchanged. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1591, "claim": "Dev3", "issue": 1543, "summary": "A run Bot whose own record switched a connector off was refused with “not granted to this run”, even though its step had named the connector, so the sentence sent the reader to fix the graph. The refusal now names the run Bot’s own switch and says a step’s tools list can narrow a run but cannot turn a connector back on; the settings sentence stays out because a run Bot is hidden. The allow/refuse decision is untouched. Replayed onto current main, it keeps ThinkFlowLab#1544’s cap sentence for a connector turned off by the Bot that started or saved the run.", "files": [ "core/src/host/connector-access.js", "core/test/connector-access.test.js", "core/test/workflow-node-connectors.test.js", "patches/ds-bot/files/src/host/connector-access.js", "patches/ds-bot/files/test/connector-access.test.js", "patches/ds-bot/files/test/workflow-node-connectors.test.js" ], "checks": { "ran": "node --test connector-access + workflow-node-connectors (repo harness resolver): 26/26 pass; red-first with connector-access.js reverted to main: 24/26, the 2 ThinkFlowLab#1543 tests fail; patches/ds-bot/files mirrors byte-identical to core", "result": "green", "at": "dd556e3ca6f69b98af3cefcff0c364d025c31e8b" }, "next": "PASS at this tip is void: the merge changed core/src/host/connector-access.js, so Reviewer re-judges this tip before enqueue" } ``` <!-- /flybot:checkpoint -->
Fixes #1543
Owner: Dev3
Reviewer: Reviewer
Written-by: Dev-3
Base:
ad976b4090b3ad7d2f8a135dc5cf6e5d97f4a685(main, #1547)Tip:
003edc97c4c867512c8f415c320de63f92a6b562Tree:
776c1dbfd4470bec237086024f0750da97d6605fA run Bot whose own record carries
connectors.github = falsewas told "The github connector is not granted to this run. The step's tools list is what allows it." The step did name the connector, and the Bot's own switch is what refused it, so that sentence sent the reader to fix the graph. The reason is order in the dispatch: the ephemeral branch ran before the switch-off branch.The flag is the same
connectorsfield every Bot record carries.bot-connectorsis its only writer, and it refuses only an unknown id or a friend's Bot, so a run Bot's id passes and its record can holdconnectors: { github: false }. The test writes that field straight onto the ephemeral record — next tostepTools: ['github'], whichhooks.jsstores from thetoolsits creator passes — rather than flipping a switch, because the settings page lists no hidden Bot.The refusal now names the switch: "The github connector is turned off for . A step's tools list can narrow what a run may use, but it cannot turn a connector back on." The second sentence is there instead of the normal Bot's "Its owner can turn it on in this Bot's settings" because a run Bot is
hidden: true— its owner cannot find it on the settings page — and because the tools list cannot open it either. The allow/refuse decision is untouched:allowed()still refuses the call, only the reason moved.What changed
core/src/host/connector-access.js— the ephemeral refusal asksturnedOff(bot, id)and returns the switch sentence when it is true; "not granted to this run" stays for a run Bot whose step tools did not name the connector. No change toallowed(),denied()or the mask.core/test/connector-access.test.js— one test: an ephemeral Bot withstepTools: ['github']andconnectors: { github: false }is refused, the refusal names the Bot and says tools cannot turn it back on, and it neither says "not granted to this run" nor points at settings.core/test/workflow-node-connectors.test.js:171— the case fix(workflow): a node's tools selects connectors instead of granting everything #1535 added landed while this branch was based on62023599: a node Bot namesgithub, then its own switch turns it off, and it still asserted/not granted to this run/, the sentence this PR replaces. It now asserts/The github connector is turned off for Runner ab\./and/cannot turn a connector back on/. The:159composio case (the step names nothing) still asserts/not granted to this run/, unchanged.patches/ds-bot/files/mirrors are byte-identical tocore/, andnode scripts/ds-bot-patches.mjs checkpasses.generatealso rewritespatches/ds-bot/0008-client-bundle.patchon a clean livemain(708/386, an environment-only difference in how the bundle diff is rendered; the resultingcore/client.jsis byte-identical), so this branch keepsmain's copy of that file and carries no 0008 churn.Red, then green
At the original base
62023599(core/test/connector-access.test.js):62023599: 21 tests / 21 pass / 0 fail.62023599plus the new test only: 22 / 21 / 1 — the only red isa run Bot names the switch that turned a connector off, not the step tools, whose first failing assertion isthe Bot whose switch is off is named, with the actual textThe github connector is not granted to this run. The step's tools list is what allows it.On the merge tree (live
mainad976b40plus this PR's files):core/test/workflow-node-connectors.test.jswith the pre-rebase:171assertion: 1 / 2, the red beinga node Bot still keeps full file/shell rights and the team tools stay refusedat:171:10, expected/not granted to this run/, actual the switch sentence. With the assertion updated: 2 / 2 / 0.:159passed on both.core/test/connector-access.test.js22 / 22,core/test/flybot.test.js46 / 46,core/test/run-bot-tools.test.js6 / 6.Verification at this tip
TZ=UTC ARCH_DOCS_BASE=ad976b40 npm_config_cache=<writable> npm run verify— exit 0: core 1869, social 527, tree 125, timeline 125, workflow 124,.github548, interop 37, live-e2e 24, every suite# fail 0; lint 0 errors (core 63 warnings, social 36, timeline 9, tree 2, workflow 12);arch-docs: OK (4 changed file(s));build-client --checkandds-bot-patches checkpass.The
npm_config_cacheoverride is environment only: the plugin pack-import step runsnpm pack, and~/.npmis read-only in my sandbox. The suite counts are from this tip (003edc97, tree776c1dbf).Not run: the smoke test and the test bench (this changes one refusal sentence and one test assertion; no wiring, route, mask or permission decision moved), and no browser check (no client change).
Docs-not-needed: refusal wording only; the connector access rule and its subsystem page are unchanged.
Checkpoint
{ "pr": 1554, "claim": "Dev3", "issue": 1543, "summary": "A run Bot whose own record switched a connector off was refused with \"not granted to this run\", because the ephemeral branch ran first; the step had named the connector, so the sentence blamed the graph. The refusal now says the connector is turned off for that Bot and that a step's tools list cannot turn it back on — the only fix a hidden run Bot has, since the settings page cannot show it. The allow/refuse decision is unchanged; only the dispatch order and two sentences moved. #1535's node-connector case now asserts the new sentence, and the patches/ds-bot/files mirrors are byte-identical.", "files": [ "core/src/host/connector-access.js", "core/test/connector-access.test.js", "core/test/workflow-node-connectors.test.js", "patches/ds-bot/files/src/host/connector-access.js", "patches/ds-bot/files/test/connector-access.test.js", "patches/ds-bot/files/test/workflow-node-connectors.test.js" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=ad976b40 npm run verify; workflow-node-connectors, connector-access, flybot, run-bot-tools on the merge tree", "result": "green", "at": "003edc97c4c867512c8f415c320de63f92a6b562" }, "next": "none — ready for review at this tip" }Merge order: nothing here waits on another open pull request. It rebased onto live
mainfor#1535'sworkflow-node-connectors.test.js;#1552edits the sameconnector-access.jshunk, so whichever lands second rebases.