Repository navigation
fix(workflow): prevent ephemeral node Bot replies from leaking into Chief chat - #1527
Conversation
89175e7 to
9b89794
Compare
Verify that ephemeral workflow node Bot replies are denied by the send middleware and do not land in Main Bot / Chief conversation as queued messages. Node I/O must stay in the run/graph only. Co-authored-by: Danyang Chen <birfychen@gmail.com>
…hief chat Fixes #1524 Workflow ephemeral node Bot replies now stay in the run/graph only and do not appear as queued '[Reply from ...]' messages in Chief or other persistent Bot conversations. The workflow plugin's send middleware intercepts these replies at ROUTING priority and returns { action: 'deny' } to block delivery. Changes: - Added defensive check in workflow send hook to split the ownsBot condition - Added warning when send middleware is not available - Added regression test verifying ephemeral replies are denied - Added test verifying persistent Bot replies are not affected Ran: cd /workspace/core && npm test cd /workspace/plugins/workflow && npm test node scripts/ds-bot-patches.mjs generate && check All tests pass. Patches regenerated and verified in sync. Co-authored-by: Danyang Chen <birfychen@gmail.com>
Remove unused mockDispatch, mockDeliver, and delivered variables that were triggering eslint errors. The test still verifies the hook behavior correctly without these unused mock functions. Co-authored-by: Danyang Chen <birfychen@gmail.com>
…hief chat Fixes #1524 Workflow ephemeral node Bot replies now stay in the run/graph only and do not appear as queued '[Reply from ...]' messages in Chief or other persistent Bot conversations. The workflow plugin's send middleware intercepts these replies at ROUTING priority and returns { action: 'deny' } to block delivery. Changes: - Added defensive check in workflow send hook to split the ownsBot condition - Added warning when send middleware is not available - Added regression test verifying ephemeral replies are denied - Added test verifying persistent Bot replies are not affected Ran: cd /workspace/core && npm test cd /workspace/plugins/workflow && npm test node scripts/ds-bot-patches.mjs generate && check All tests pass. Patches regenerated and verified in sync. Docs-not-needed: Internal defensive improvement to workflow send hook; no architecture changes, no new APIs, no behavior changes to workflow subsystem that require documentation updates. Co-authored-by: Danyang Chen <birfychen@gmail.com>
CI package check requires kind be one of: added, changed, deprecated, removed, fixed, security (lowercase). Changed .fix.md → .fixed.md. Co-authored-by: Danyang Chen <birfychen@gmail.com>
… path Add logging to identify where workflow node Bot replies leak into Chief: - Log when Bot-to-Bot reply is NOT gated (bypasses middleware) - Log when reply IS gated and middleware denies it - Log the gated() check conditions when reply bypasses This will help identify whether: 1. Replies bypass gated() check (wrong role, flags, or missing bot) 2. Middleware deny is ignored 3. Different delivery path exists Once root cause is identified from logs, will implement proper fix. Co-authored-by: Danyang Chen <birfychen@gmail.com>
…hemeral flag (#1524) LIVE leak root cause: workflow node Bot replies that bypass the send middleware (deferred, resumed, post-release) were queued into Chief because: 1. After stepResult→releaseRun, ownsBot() returns false (bot no longer in run.ephemeral) 2. Second/deferred replies bypass gated() check (deferred/resumed flags set) 3. These paths call innerDeliver directly, queueing [Reply from ...] messages Real fix: Check durable bot.ephemeral flag in wrapped rt.deliver BEFORE gated() check. Deny ALL bot-to-bot deliveries (reply, stopped, report) from bots marked ephemeral, regardless of: - Gating state (gated vs non-gated paths) - Delivery flags (deferred, resumed, retried, resent) - Workflow ownership (owned vs post-release) The workflow send hook still runs for gated deliveries and calls stepResult; the new check catches bypass scenarios where middleware never ran. Changes: - core/src/host/hooks.js: Check bot.ephemeral before gated() in rt.deliver - core/test/workflow-reply-leak.test.js: Add bypass-path regression test - changelog.d/1524-workflow-reply-leak.fixed.md: Describe real fix Docs-not-needed: Delivery gate logic; no subsystem architecture change Co-authored-by: Danyang Chen <birfychen@gmail.com>
#1524) CRITICAL fixes for bugs in previous attempt (9b57230): Bug 1 - stepResult never ran: Early return bypassed middleware, so workflow plugin's workflow-step hook never called stepResult, leaving runs stuck. Bug 2 - post-removal still leaked: After bot.remove, rt.botOf() returned undefined, so ephemeral check failed and innerDeliver still ran. Real fix: 1. EXPAND gated() check: ephemeral bot deliveries (or tombstoned ids) ALWAYS return true, EVEN with deferred/resumed/retried flags. This routes them through run()/middleware where workflow-step hook calls stepResult. 2. TOMBSTONE on remove: workflow plugin calls rt.tombstoneEphemeral(id) before bot.remove. Late replies from tombstoned ids are denied even though bot no longer exists. 3. Post-removal check: if tombstoned AND bot is gone, deny before middleware. 4. Workflow send hook unchanged: still calls stepResult then returns deny. This ensures: - First reply: middleware runs → stepResult advances → deny blocks delivery - Deferred/resumed/retried reply: gated by ephemeral flag → middleware runs - Post-removal reply: tombstone check denies (bot gone, no middleware needed) - Request TO ephemeral: allowed (sender is not ephemeral) Changes: - core/src/host/hooks.js: Expand gated(), add tombstone Set + rt.tombstoneEphemeral - plugins/workflow/index.js: Call rt.tombstoneEphemeral before bot.remove - core/test/workflow-reply-leak.test.js: Verify all scenarios including stepResult TZ=UTC npm run verify: green Docs-not-needed: Delivery gate logic; no subsystem architecture change Co-authored-by: Danyang Chen <birfychen@gmail.com>
Three final fixes before review: 1. Tombstone via ctx.runtime was dead - plugins cannot access ctx.runtime. Moved tombstoning into removeBot() in hooks.js, called immediately before rt.deleteBot(id) when bot.ephemeral is set. Covers every fb.bot.remove path. Removed ctx.runtime call from workflow plugin. 2. Resent edge - gated() now handles resent from ephemeral/tombstone same as deferred: ephemeral bots always route through middleware even when resent. Normal bots still skip middleware when resent. 3. Ready for rebase onto origin/main after this commit. Changes: - core/src/host/hooks.js: Tombstone in removeBot before deleteBot; handle resent in gated() - plugins/workflow/index.js: Remove ctx.runtime?.tombstoneEphemeral call (dead code) TZ=UTC npm run verify: green Co-authored-by: Danyang Chen <birfychen@gmail.com>
202f5bc to
680d516
Compare
|
Verdict: INC at 680d516 (Signature not enabled per AGENTS.md #1110/#1111: Ran at 680d516:
Not run:
Findings: Incomplete test coverage (INC)The three tests in Code changes themselves look correct (gated ephemeral+tombstone, removeBot tombstone, post-removal deny). INC until defect tests fail on main and pass with the fix (preferred), or real smoke evidence at this tip. |
…g work Reviewer INC requested fail-without-change defect tests that exercise the REAL hooks.js gated()/rt.deliver path, not mock reimplementations. Added workflow-reply-leak-real.test.js with focused unit tests proving: 1. Ephemeral with deferred/resumed/retried/resent MUST be gated - RED on origin/main: bypasses gating → leaks to Chief - GREEN with fix: ephemeral forces gated()=true → routes through middleware 2. Post-removal tombstone prevents late deliver - RED on origin/main: bot gone, no tombstone → bypasses gating → leaks - GREEN with fix: tombstone exists → gated()=true → denied 3. Normal (non-ephemeral) bot behavior unchanged - First reply still gated - deferred/resumed/retried/resent still skip gating (as before) - Request TO ephemeral still allowed These test the actual gated() logic from hooks.js directly, showing the before/after behavior difference that fixes the LIVE leak. TZ=UTC npm run verify: green Co-authored-by: Danyang Chen <birfychen@gmail.com>
d70948f to
84d5f54
Compare
|
Verdict: PASS at 84d5f54 Ran:
Not run:
FindingsNo actionable findings. Ephemeral/tombstoned senders always route through middleware even with deferred/resumed/retried/resent; normal bots unchanged; tombstone before removeBot; real defect tests fail-without-change. |
Owner: Dev
Reviewer: Reviewer
Written-by: Dev
Fixes #1524
What changed
Before: Workflow ephemeral node Bot replies still reached Chief chat —
gated()skipped deferred/resumed/retried/resent deliveries, and afterbot.removethe ephemeral check failed so late replies leaked.After:
gated()returns true for ephemeral (or tombstoned) senders even with deferred/resumed/retried/resent, so middleware still runsstepResultthen denies deliveryremoveBot()tombstones ephemeral ids beforedeleteBot; post-removal replies are denied without needing a live botctx.runtime?.tombstoneEphemeralcall removed from the workflow pluginVerification at the tip
Tip SHA: 680d516
Ran:
TZ=UTC npm run verify→ greenNot run: Desktop / LIVE smoke (needs prod tip with this fix)
Checkpoint
{ "pr": 1527, "claim": "Dev", "issue": 1524, "summary": "Ephemeral Workflow node Bot replies no longer leak into Chief chat. gated() always routes ephemeral or tombstoned senders through middleware so stepResult can advance the run before deny, including deferred/resumed/retried/resent deliveries. removeBot tombstones ephemeral ids before deleteBot so late replies after removal are denied. The dead ctx.runtime tombstone call in the workflow plugin is gone.", "files": ["core/src/host/hooks.js", "plugins/workflow/index.js", "core/test/workflow-reply-leak.test.js", "patches/ds-bot/files/src/host/hooks.js", "patches/ds-bot/files/test/workflow-reply-leak.test.js", "changelog.d/1524-workflow-reply-leak.fixed.md"], "checks": { "ran": "TZ=UTC npm run verify", "result": "green", "at": "680d51624bd5e63ef05d38626d06bb66a65f9ac7" }, "next": "none — ready for review" }Owner: Dev
Reviewer: Reviewer
Fixes #1524
Root Cause
Production tip
55e1c27ahad the workflow send hook that returns{action: 'deny'}for owned ephemeral bot replies, but the LIVE leak persisted because:stepResult → advance → releaseRun,ownsBot()returns false, so subsequent replies pass through the send hook (returnsundefined)deferred,resumed,retried, orresentflags bypassgated()entirely, skipping middleware and going straight toinnerDeliverbot.remove,rt.botOf()returns undefined, so ephemeral checks fail and late replies leakThe Fix
Expand
gated()check so ephemeral bot deliveries ALWAYS route through middleware (wherestepResultruns), then deny delivery:gated(): Returns true for ephemeral bots (or tombstoned ids) EVEN withdeferred/resumed/retried/resentflags, forcing them throughrun()/middlewareremoveBot(): When any plugin callsfb.bot.remove(id)on an ephemeral bot,removeBot()inhooks.jstombstones it beforedeleteBot; late replies from tombstoned ids are denied even after bot is gonestepResultthen returns denyThis ensures:
stepResultadvances workflow → deny blocks Chief delivery ✓Changes
core/src/host/hooks.js: Expandgated()for ephemeral bots including resent; tombstone inremoveBot()beforedeleteBot(); add tombstone Setplugins/workflow/index.js: Remove deadctx.runtime?.tombstoneEphemeralcall (tombstoning now inremoveBot)core/test/workflow-reply-leak-real.test.js: Real defect tests proving gated() fix and tombstoning (RED on origin/main, GREEN with fix)core/test/workflow-reply-leak.test.js: Existing mock-based testschangelog.d/1524-workflow-reply-leak.fixed.md: Describe the real fixTesting
Real defect tests in
workflow-reply-leak-real.test.jsdirectly test the actualgated()logic from hooks.js, proving:Rebased onto
origin/main(includes #1529). All tests pass:TZ=UTC npm run verify✓{ "pr": 1527, "claim": "Dev", "issue": 1524, "summary": "Fixed workflow ephemeral Bot reply leak by expanding gated() so ephemeral deliveries always route through middleware (ensuring stepResult advances workflow) while workflow send hook denies Chief delivery. Tombstoning moved to removeBot in hooks.js. Added real defect tests proving the fix. Rebased onto main.", "files": [ "core/src/host/hooks.js", "plugins/workflow/index.js", "core/test/workflow-reply-leak-real.test.js", "core/test/workflow-reply-leak.test.js", "patches/ds-bot/files/src/host/hooks.js", "patches/ds-bot/files/test/workflow-reply-leak-real.test.js", "patches/ds-bot/0005-social-seams.patch", "changelog.d/1524-workflow-reply-leak.fixed.md" ], "checks": { "ran": "TZ=UTC npm run verify (core tests, plugin tests, patches check)", "result": "green", "at": "84d5f54e08fd32ab4a7b21ce88f42d7f8e03a95e" }, "next": "none — ready for review" }Docs-not-needed: Delivery gate logic; no subsystem architecture change