Repository navigation
fix(workflow): node Bots inherit full normal-Bot permissions - #1512
Merged
Merged
Conversation
…e Bots with no explicit tools A Workflow node Bot with no explicit tools list (or an empty list) now gets the baseline tools: read, edit, bash, github. This gives node Bots normal Bot permissions by default, enabling GitHub access and shell operations that node prompts often expect. Before this change: - A node with tools: [] or no tools field got no tools at all - permissionFor([]) returned workspace-write only - Tool notes were not added to the prompt (tools.length === 0) - Escalate and other nodes failed on github/bash with ~6-7 min delays After this change: - A node with no explicit tools gets BASELINE_TOOLS = ['read', 'edit', 'bash', 'github'] - permissionFor() sees the baseline and returns danger-full-access (bash is present) - Tool notes are added to the prompt (github grant, etc.) - Node Bots behave like normal Bots unless explicitly restricted - An explicit tools list still uses those tools only Implementation: - defaultedTools() in ephemeral.js returns BASELINE_TOOLS when tools is empty - nodeConfig() calls defaultedTools() instead of cleanTools() directly - permissionFor() calls defaultedTools() and checks for shell tools as before - Tests updated: baseline tools are verified, explicit tools still work - Docs updated: workflow.md explains the baseline behavior The host-level EPHEMERAL_DENIED_TOOLS (#1466) still narrows what a run Bot may reach (team tools like message_bot, create_bot, etc.), so the baseline does not grant team-admin powers. Co-authored-by: Danyang Chen <birfychen@gmail.com>
… list Corrected approach: a Workflow node Bot with no explicit tools list now omits the tools field from fb.bot.create, letting it inherit ALL registered tools (file/shell, GitHub, Composio, team tools) just like a normal sidebar Bot, limited only by EPHEMERAL_DENIED_TOOLS at the guard/approval layer. Previous attempt (baseline tools): - Used hardcoded baseline ['read', 'edit', 'bash', 'github'] - Excluded Composio and other connectors - Owner correction: all Bots need same full permissions Current approach: - No explicit tools → tools field omitted from create → inherits all - Explicit tools list → restricts to those tools only - Guard/approval layer handles EPHEMERAL_DENIED_TOOLS (29 team-admin tools) - Node Bots get same permissions as normal Bots (minus team-admin) Implementation: - nodeConfig() returns tools: undefined when node has no explicit list - spawn() omits tools field when undefined, includes it when explicit - permissionFor() handles undefined safely (workspace-write, no shell) - Tests verify: no tools → field omitted; explicit tools → passed through - Docs: ephemeral.js file table + invariant 6 explain inheritance This fixes Escalate and other nodes that were failing on github/bash with 6-7 min delays, and ensures Composio and future connectors work without hardcoding an allowlist. Co-authored-by: Danyang Chen <birfychen@gmail.com>
…mentation only Owner requirement (final): EVERY Workflow node Bot must get the same full permissions and connectors a normal sidebar Bot gets, INCLUDING when the node has an explicit tools list. node.tools should NOT strip connectors. Final correct implementation: - tools field is ALWAYS omitted from fb.bot.create() for node Bots - Every node Bot inherits all registered tools (GitHub, Composio, file/shell, team tools) - Guard layer (EPHEMERAL_DENIED_TOOLS #1466) restricts only 29 team-admin tools - node.tools is stored in fields.tools as documentation but does not restrict - All node Bots get danger-full-access permission - No tool-specific prompt notes are added This ensures: ✓ Triage, Review, Escalate, Enqueue all get full GitHub/Composio access ✓ LIVE Review node with tools: [bash, read, write, edit] still gets GitHub/Composio ✓ Node tools lists are preserved as documentation without restricting grant ✓ Matches owner ask: 所有的bot要能拿到正常bot的所有权限，包括github、composio等等 Previous attempts: 1. Baseline ['read', 'edit', 'bash', 'github'] - excluded Composio 2. Inherit when empty, restrict when explicit - still stripped connectors Current: Always inherit, never restrict. Simple and correct. Co-authored-by: Danyang Chen <birfychen@gmail.com>
…ools, shellTools Lint fix for CI: - Removed unused SHELL constant (permissionFor no longer checks it) - Removed unused var assignments in test (noTools, emptyTools, customTools, shellTools) - Tests still pass (86/86) - Plugins lint now clean Urgent fix per owner request for CI green before Birfy review. Co-authored-by: Danyang Chen <birfychen@gmail.com>
|
Verdict: PASS at b9ed05b Ran:
Not run:
Review findingsNo actionable findings. This change is correct and complete. Implementation (correct)The change ensures every Workflow node Bot gets the same full permissions a normal sidebar Bot gets:
Architecture docs (satisfied)The owned path Security & DSH hard rules (pass)
CorrectnessThe implementation matches the owner requirement: "所有的bot要能拿到正常bot的所有权限，包括github、composio等等" (all bots get the same permissions normal bots get, including github, composio, etc.). Before: After: This fixes the reported issue where Escalate and Review nodes failed on GitHub operations with 6-7 minute delays. Tests pass (verified)All tests pass at tip
This verdict is not merge authority. The implementer and reviewer roles are separate. Per AGENTS.md, the agent that gated this PR will enqueue it after this PASS is recorded. |
Collaborator
Author
|
Verdict: PASS at b9ed05b Ran:
Not run:
FindingsNo actionable findings. Empty or explicit |
Birfy
changed the title
This was referenced Oct 28, 2026
Birfy
added a commit
that referenced
this pull request
Oct 28, 2026
Second replay of 6181504, onto main 92a6aab. The reviewed tip 39c65d0 sat on fe6c906 and no longer merges: #1512 rewrote the ephemeral permission docs and test, and carrying the old file copies over would have reverted it. The reviewed change is kept whole: one endRun helper every ending path shares (reap() at load, dispose(), reset() stopping one run, and the catch around a step that threw), the three #1424 tests, the workflow.md sentences and the changelog fragment. Only the #1424 hunks are re-applied on top of current main; #1512's permission test and docs stay as main has them.
This was referenced Oct 28, 2026
Closed
Birfy
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 28, 2026
…owLab#1424) (ThinkFlowLab#1431) Fixes ThinkFlowLab#1424 Owner: Chief Reviewer: Reviewer Written-by: Dev4 Replayed-by: Rework Tip SHA: `0ed32f78de597f25793d9d75d574b36ef2b440e0` Base: `92a6aabd` (`main`) Tree: `33730bfa3468a8527693514675b11451822670f6` ## Reproduce On `fe6c9069` (the `main` the review started from), with the plugin's own hooks: 1. Define a two-node workflow, `runNamed({ workflow: 'Lane', … })` with a `spawn` hook; the run starts with `nodeStatus: { plan: "running", code: "queued" }` and `ephemeral: { plan, code }`. 2. Stop that one run: `reset({ workflowId, runId })` — the leg a user reaches with ■ in the Runs list. The run is archived as `failed` and its records go, but `nodeStatus.plan` stays `running` (`reset` built `ended = { ...run, status: 'failed', … }` and never touched the map). 3. `publicRun()` builds `running: orderedIds(workflow, run.nodeStatus, 'running')`, so the API still reports `['plan']`; the strip reads that and paints "Running now · Planner" and its chip. `marks`, `doneCount` and `layerMarks` are derived from `nodeStatus` too. 4. Same shape in `startLive`'s catch around `handOff` (the old `settle()` catch): it archived the run `failed` without settling the map. 5. The load-time `reap()` leg was already settled by `ThinkFlowLab#1414`, so this tip keeps that path on the same helper instead of adding a second one. ## What changed One helper ends a run, and every path that abandons one calls it: - `endRun(item, run)` in `plugins/workflow/src/service.js` — the reviewed tip's helper, same body: a run that already ended keeps its own ending (ThinkFlowLab#1388); otherwise every node still `running` goes through `failNode`, each in its own `try/catch` (the graph can be edited under a live run, and `failNode` throws for a node the graph no longer has); then `status = 'failed'` and `updatedAt = now()`. - `reap()` at load and `dispose()` keep ending abandoned live runs through it (they were on `failCutoffRun`, the same body); `reset()` stopping one run now calls it before archiving; `startLive`'s catch calls it before `releaseRun`. - The reviewed tip also appended the `{ task, status: 'failed', at }` line to `run.history` inside `endRun`. On this base history moved to the lane: `archiveCutoffRun()` and `releaseRun()` already append `{ task, status, at: updatedAt }` to `lane.history` (`.slice(-20)`), and the stage renders `workflow.history` (`client.js:517`). A second append inside `endRun` would double the line, so the helper stops at the map and the status; the ending still reaches the list the stage renders, which the stop test asserts. - Files: `plugins/workflow/src/service.js` (`+14/−9`), `plugins/workflow/test/ephemeral.test.js` (`+113`), `docs/subsystems/workflow.md` (invariant 3 and the test list), `changelog.d/1424-workflow-reap-node-status.fixed.md`. No client change: the strip already renders `run.running` from the API, so settling the server side is what removes the ghost. ## Replay onto current `main` `6181504e` sat on `26657c8a`, 700+ commits behind. The first replay put the reviewed change on `fe6c9069` (`39c65d05`), one commit whose parent was then-current main. That tip stopped merging once `ThinkFlowLab#1512` rewrote the ephemeral permission test and docs on `main`, so this tip is re-applied on current main: one commit whose parent is `92a6aabd`, and `compare 92a6aab...0ed32f7` lists only the four reviewed files. Only the `ThinkFlowLab#1424` hunks are carried; `ThinkFlowLab#1512`'s permission test and docs stay as `main` has them. main absorbed part of the reviewed change first: `ThinkFlowLab#1414` moved the store to lanes and already ends a reaped or disposed live run through `failCutoffRun`, which is the reviewed `endRun` body. The replay keeps that helper under the reviewed name and routes the two legs main did not fix — `reset()` stopping one run and `startLive`'s catch — through it, so the map cannot drift on any of the four endings. Of the reviewed tests, three still apply and are ported (`ThinkFlowLab#1388`'s already-ended acceptance included); the fourth — "a dispatch that throws ends the run" — is not ported: on this base `handOff()` catches a refused dispatch, warns and leaves the node open for the next hand-off (ThinkFlowLab#1389), so a dispatch failure is not an ending, and the outer catch is only for an unexpected throw. ## What I ran At this tip (`0ed32f78de597f25793d9d75d574b36ef2b440e0`, base `92a6aabd`): - `node --test plugins/workflow/test/ephemeral.test.js` → **16 tests, 16 pass, 0 fail** (13 on main + 3 new). - `node --test plugins/workflow/test/*.test.js` → **93 tests, 93 pass, 0 fail** (90 on main + 3 new). - `node .github/scripts/changelog.mjs check` → `changelog: 48 fragment(s) in changelog.d/ are well formed`. - `node .github/scripts/check-mojibake.mjs docs plugins/workflow` → 0 occurrences of U+FFFD in 45 text files. - `DSH_HARNESS=/data/dsh/bot/workspace/lintenv node scripts/lint-plugins.mjs workflow` → **not conclusive here**: that prepared oxlint is older than the one CI installs and reports pre-existing `no-useless-fallback-in-spread` in untouched code (`src/graph.js:583-584`, `src/service.js:283`). This diff adds no spread fallback. ## Red without the source change Same test file, `plugins/workflow/src/service.js` taken from `92a6aabd`: ``` node --test plugins/workflow/test/ephemeral.test.js ok 13 - the plugin load itself reaps a run the restart left running (ThinkFlowLab#1388) not ok 14 - stopping a live run settles its node map: no node stays running (ThinkFlowLab#1424) ok 15 - a reaped run whose node map names a node the graph no longer has settles the rest (ThinkFlowLab#1424) ok 16 - an ended run keeps its ending when reap drops the records it still holds (ThinkFlowLab#1424) # tests 16 # pass 15 # fail 1 ``` - `not ok 14`: `run.running` is `['plan']` where `[]` is expected and `nodeStatus.plan` is `'running'` where `'failed'` is expected — the ghost the strip paints. - `ok 15` and `ok 16` are green on the old code on purpose: `ThinkFlowLab#1414` already settles the reap leg, and `ThinkFlowLab#1388`'s acceptance is that an ended run keeps its ending. They pin both, so the next change here cannot quietly regress them. ## Not run - Root `npm run verify` end to end: this box has no `npm ci` / `npm run harness` install. I ran the parts that cover this change; CI runs the rest. - `arch-docs` and `ds-bot-patches check`: both need a git checkout, and this tree is a tarball. No `core/`, patch or generated file is touched; the owned arch doc `docs/subsystems/workflow.md` is updated in this diff, which is what `arch-docs` checks. - `build-client --check` and the smoke test: no client or generated file changes. - The browser: I did not open the Workflows page in a running host. The claim is on the API the strip reads (`publicRun().running` empty after a stop), not on a screenshot. ## Risk - **History grows by one line per ended run.** Capped at 20 (`.slice(-20)`), same as every other history write, so a run that is ended repeatedly does not grow without bound. - **A node the graph no longer has is left at `running` in `nodeStatus`.** It cannot be moved through `failNode` (it throws), and dropping it silently would lose what happened; `publicRun().running` walks the graph, so it is not painted, and `saveGraph` rebuilds `nodeStatus` from the graph's ids on the next save. Recorded here rather than hidden. - **`updatedAt` moves when a run is ended**, on every path that ends one. Deliberate: the alternative is the drift this issue is about. - **`reap()` and `dispose()` warn and continue on a run they cannot end**; the ids are collected before the record is dropped, so its Bots are still released. - **Not fixed here:** `ThinkFlowLab#1387` (`dispose()` deletes the records before the release). Not widened by this PR. No `Docs-not-needed:` trailer: `docs/subsystems/workflow.md` (owned path `plugins/workflow/`) is updated in this PR. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1431, "claim": "Chief", "issue": 1424, "summary": "A run ended without settling its node map left publicRun().running naming the node it was on, so the strip kept painting 'Running now' for a run that was over. Replayed onto main fe6c906: ThinkFlowLab#1414 already settles the reap/dispose leg through failCutoffRun, so that helper is the reviewed endRun and the two legs main did not fix — workflow/reset stopping one run and the catch around a step that threw — now go through it too. An ended run keeps its ending, a node the graph no longer has keeps its record, and the ending reaches the lane history the stage renders.", "files": ["plugins/workflow/src/service.js", "plugins/workflow/test/ephemeral.test.js", "docs/subsystems/workflow.md", "changelog.d/1424-workflow-reap-node-status.fixed.md"], "checks": { "ran": "node --test plugins/workflow/test/*.test.js; node --test plugins/workflow/test/ephemeral.test.js; node .github/scripts/changelog.mjs check; node .github/scripts/check-mojibake.mjs docs plugins/workflow", "result": "green", "at": "0ed32f78de597f25793d9d75d574b36ef2b440e0" }, "next": "Chief: enqueue 0ed32f7 and land it; the reviewed PASS at 39c65d0 does not carry over to this tip" } ``` <!-- /flybot:checkpoint -->
Birfy
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 28, 2026
…eep (ThinkFlowLab#1425) (ThinkFlowLab#1427) Owner: Chief Reviewer: Reviewer Fixes ThinkFlowLab#1425 Tip SHA: `04655e4a91770f40918c17ca6b56f4f34a1b11c2` Base: `92a6aabd0e4bbe679476cd62438ed5bae13405fd` (`main`) Tree: `fa59bf2ca67a6d42ab39e6c210bf50d574203f0d` ## Reproduce `plugins/workflow/index.js:92` starts reap without awaiting it (`void service.reap?.()`), so `apply()` returns while the release is still in flight. The test paid for that with one fixed sleep and then asserted: ```js await new Promise((resolve) => setTimeout(resolve, 20)) assert.deepEqual(released, ['eph-plan']) ``` 20 ms is a wall-clock dependency, not a condition. On a loaded runner the release lands later than 20 ms and the test reports "nothing was released" / "the run is still running" — the exact symptoms of the bug the test exists to catch. I made that concrete by keeping the new wait out and slowing the release to 150 ms: **1 test fails** (`# fail 1`), `Expected values to be strictly deep-equal: + [] - [ 'eph-plan' ]`. ## What changed One file, tests only; no product code and no `patches/ds-bot/**` (that tree does not cover `plugins/workflow`). - New local helper `untilRun(dir, workflowId, settled, { timeout = 2000, interval = 10 })` polls through the file's existing `persistedRun(dir, workflowId)` until the predicate holds. The deadline exists only to bound a failure, and its message names the state it last saw: `the run was not reaped within ${timeout} ms; last state: …`. - The `ThinkFlowLab#1388` plugin-load test now waits for what reap actually writes — `run.status === 'failed' && run.ephemeral === undefined` — then asserts `'handed' in settled === false`, `released === ['eph-plan']`, and the run's final `failed`/`ephemeral: undefined`. - Before/after for the user: nothing visible. The test stops being a timer race and starts being a guard: slow release passes, a missing reap still fails. No `changelog.d/` fragment: this is a test-only change with no user-visible behavior (AGENTS.md item 4). Docs-not-needed: test-only change; the plugin-load reap behavior described in `docs/subsystems/workflow.md` is unchanged, only the test's wait condition moved from a fixed 20 ms sleep to the state the test asserts. ## Rework at `04655e4a` (Written-by: Rework) Written-by: Rework main moved three commits past the base this PR last sat on (`4d92f4f7` → `92a6aabd`: ThinkFlowLab#1510, ThinkFlowLab#1430, ThinkFlowLab#1512), and ThinkFlowLab#1512 rewrote the very test file this change watches, so the reviewed wait is replayed onto current main as one commit whose parent is `92a6aabd`. `compare` `main...04655e4` lists exactly one file, `plugins/workflow/test/ephemeral.test.js` (+18/−1), `ahead_by 1`, `behind_by 0`; every other blob in the tree is main's, because the commit's tree is main's tree with that one entry replaced. The commit carries a `Docs-not-needed:` trailer in its message as well as in this body: `npm run lint` (`arch-docs`) red-flagged the previous replay with `plugins/workflow/test/ephemeral.test.js → workflow (docs/subsystems/workflow.md)` because it reads the trailer from `git log <base>..HEAD`, not from the body. The tree is byte-identical to that earlier replay (`fa59bf2c`); only the commit message changed. The fix itself is unchanged: the same `untilRun` helper (deadline 2000 ms, interval 10 ms, failure message naming the last state) and the same wait on `status === 'failed' && ephemeral === undefined` instead of the fixed 20 ms sleep. It composes with ThinkFlowLab#1512's node-Bot grant assertions in the same file, which are untouched. The comment's line reference now reads `index.js:92`, which is where main keeps the unawaited `void service.reap?.()`. The body's earlier claim mismatch is fixed and not re-touched here: the checkpoint `claim` is `Chief`, matching the `Owner:` line. ## What I ran At tip `04655e4a91770f40918c17ca6b56f4f34a1b11c2` (base `92a6aabd0e4bbe679476cd62438ed5bae13405fd`), in `plugins/workflow`: - `node --test test/ephemeral.test.js` → **13/13 pass**, 0 fail (226 ms) - `npm test` → **90/90 pass**, 0 fail (323 ms) main's version of the file now holds 13 tests and the workflow suite 90; the counts differ from the previous tip because ThinkFlowLab#1512 added tests, not because this change added or dropped any. ## Red without the source change Each knife is the same tree with one thing reverted, run from a copy: - **New wait + the plugin-load Bot removal slowed to 150 ms** → **13/13 pass** (368 ms). The old 20 ms could not have survived this. - **Old 20 ms wait + the same 150 ms removal** → **`not ok 13`**, `# fail 1`, 12 pass. This is the flake the issue describes, reproduced on demand. - **New wait + the `void service.reap?.()` call removed from `index.js:92`** → **`not ok 13`**, `# fail 1`, 12 pass: `the run was not reaped within 2000 ms; last state: {"id":"run-4c5302fc",…"status":"running","nodeStatus":{"plan":"running"},"ephemeral":{"plan":"eph-plan"},"handed":{"plan":true},…}` So the guard still fires when the behavior it protects actually disappears — it is not merely tolerant. ## Not run - Full `core` suite and `npm run verify`: not runnable on this box (no git, no oxlint, no esbuild; `verify` needs `origin/main`). `core` is untouched by this diff. - `node scripts/ds-bot-patches.mjs generate`/`check`: not needed — no file under `core/`, `ds-bot/`, or `patches/ds-bot/` is in this diff. - CI: left to the tip's checks. ## Risk Low. `plugins/workflow/test/ephemeral.test.js` is the only changed file and no production path moves. The trade-off is deliberate: the test can now wait up to 2 s before failing instead of 20 ms, so a broken reap costs ~2 s of test time rather than a false green. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1427, "claim": "Chief", "issue": 1425, "summary": "The ThinkFlowLab#1388 plugin-load reap test slept a fixed 20 ms after apply(), which starts reap without awaiting it; on a loaded runner the release landed later and the test flaked like the bug it guards. It now polls workflows.json through a local untilRun helper until the run is failed with its ephemeral Bot released, bounded by a 2 s deadline that names the last state seen. Tests only; no product code, nothing under core/ or patches/ds-bot/. Replayed by Rework onto main 92a6aab.", "files": ["plugins/workflow/test/ephemeral.test.js"], "checks": { "ran": "node --test test/ephemeral.test.js 13/13 pass (226 ms); npm test 90/90 pass (323 ms) in plugins/workflow; knives: new wait + 150 ms slow removal 13/13 pass; old 20 ms + 150 ms removal 1 fail; reap call removed 1 fail with the 2000 ms deadline message", "result": "green", "at": "04655e4a91770f40918c17ca6b56f4f34a1b11c2" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
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
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.
Owner: Dev
Reviewer: Reviewer
EVERY Workflow node Bot gets the same full permissions a normal sidebar Bot gets (GitHub, Composio, shell, …). Spawn always omits
toolsfromfb.bot.create;node.toolsis documentation only. Limited only by EPHEMERAL_DENIED_TOOLS (#1466).Checkpoint
{ "pr": 1512, "claim": "Dev", "issue": 1516, "summary": "Workflow ephemeral node Bots no longer spawn with a stripped or empty tools list. Create always omits the tools field so every node Bot inherits the full normal sidebar Bot grant, including GitHub and Composio. node.tools stays as documentation only and does not restrict connectors. All node Bots get danger-full-access; team-admin tools remain denied by EPHEMERAL_DENIED_TOOLS. This stops Escalate and partial-list nodes from burning minutes on failed tool calls.", "files": ["plugins/workflow/src/ephemeral.js", "plugins/workflow/index.js", "plugins/workflow/test/ephemeral.test.js", "docs/subsystems/workflow.md", "changelog.d/workflow-node-bot-baseline-tools.fixed.md"], "checks": { "ran": "npm --prefix plugins/workflow test; plugins lint; CI tip green", "result": "green", "at": "b9ed05bfa46f3d9b584cb9ff5e509bcbb23d689e" }, "next": "none — ready for review" }Fixes #1516
Docs-not-needed: no — docs/subsystems/workflow.md updated in this PR