refactor: run SkillFlows through core's executeFlow - #3
Conversation
The step loop moved into @open-gitagent/gitagent so the CLI, SDK and scheduler can run flows too (open-gitagent/gitagent#89). Voice now supplies only the parts that are genuinely browser- and chat-specific. Deleted: the step loop, the __approval_gate__ branch, prompt building, {input} substitution and context threading. Those now live in core. Kept and wired in as callbacks: - runStep — spawn query(), stream tool calls to the browser - onProgress — flowProgressToBrowser() maps core's events onto the WebSocket messages the UI already understood - requestApproval — the existing Telegram/WhatsApp sender Approval gates no longer auto-approve when no channel is connected. A gate guards a risky step; continuing because nobody could be reached silently removes the protection it exists to provide. The browser now gets a specific message for each outcome — no channel configured, timed out, or explicitly declined — instead of the same bare "denied" line, and the 5-minute timeout is no longer silent. A gate that omits `channel` now resolves to whichever channel is actually connected, rather than assuming Telegram. Hand-written flows previously failed on a WhatsApp-only setup. The @flow trigger is anchored to the start of the message, so an address like bob@daily-report.com no longer fires a flow. Schedules can run flows: runFlow is passed to the scheduler.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Two issues worth resolving before merge. One is a correctness bug — concurrent flows corrupt the shared approval slot. The other is a verification gap on the scheduler path. Inline comments below.
pendingApproval was a single slot set unconditionally, so a second gate (a cron flow and a browser @flow can now overlap) overwrote the first and a YES resolved the wrong one. Gates now queue: a second gate does not message anyone until the first is answered. The timeout never fired at all — it compared pendingApproval.resolve against the closure's resolve, but the slot holds a wrapper, so the check was always false. Match on a per-gate id instead.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Both previous findings are cleanly addressed. The queue approach is sound and the id-based timeout guard is correct. A few small observations below, none blocking.
The → split reads well: serialisation is handled in the outer wrapper and the inner function focuses on the actual send/wait. The pattern is correct — the finally block fires regardless of whether resolves or throws.
The id guard in the timeout closure is correct now. in the new code is the wrapper closure (not the original ), so an identity comparison would always be false. The id check is the right fix.
The runtime resolution is a nice improvement; it was previously author-time-only, which silently broke on channel switches.
Security pass: no dependency changes, no secrets, no injection surface in the changed code, no authz gaps.
| const priorGate = approvalQueue; | ||
| let releaseSlot!: () => void; | ||
| approvalQueue = new Promise<void>((r) => { releaseSlot = r; }); | ||
| await priorGate; |
There was a problem hiding this comment.
Minor: approvalQueue is overwritten before await priorGate returns. If a third concurrent call arrives while the first gate is still active, the third call captures the second's promise as priorGate and the chain stays correct — but the intent takes a second read to see. A one-line comment here ('each caller chains off its predecessor') would help the next person.
| case "approval_requested": | ||
| sendToBrowser({ type: "transcript", role: "assistant", | ||
| text: `⏸ Waiting for approval via ${e.channel || "telegram"}...` }); | ||
| break; |
There was a problem hiding this comment.
e.channel || "telegram" will show "telegram" in the browser even when the gate fires on WhatsApp, because e.channel is the value from the flow YAML and may be absent. Fine for now, but worth aligning with defaultApprovalChannel() if the UI message needs to be accurate.
2 participants
Companion to open-gitagent/gitagent#89 and its PR. Depends on that landing first — this imports executeFlow from core, which doesn't exist there yet.
What changed
The SkillFlow step loop moved into core so the CLI, SDK and scheduler can run flows too. Voice keeps everything genuinely specific to it — the visual builder, browser streaming, Telegram/WhatsApp — and supplies three callbacks instead of owning the engine.
Deleted (~61 lines): the step loop, the approval_gate branch, prompt construction, {input} substitution, context threading.
Added: flowProgressToBrowser(), which maps core's event stream onto the exact WebSocket messages the UI already understood. Browser behaviour is unchanged.
executeFlow() is now a ~30-line adapter that loads the YAML, calls core, and returns a string so the scheduler can log a result.
Behaviour changes
Gates no longer auto-approve when no channel is connected. Previously sendApprovalRequest returned true if neither Telegram nor WhatsApp was reachable — so a flow whose gate guarded "send this email" would just send it. It now denies.
Each denial reason is distinguishable. "No channel configured", "timed out after 5 minutes", and "you replied NO" produced the identical denied line before; the first two now explain themselves in chat and in the log. The timeout case was previously silent everywhere.
Gates without an explicit channel resolve at run time to whichever channel is connected, instead of assuming Telegram. A hand-written gate previously failed outright on a WhatsApp-only setup, and the builder bakes the choice in at authoring time so it goes stale if you switch channels.
The @flow trigger is anchored to the start of the message. email bob@daily-report.com about the migration used to fire the daily-report flow. Matches the CLI's behaviour.
Schedules can run flows — runFlow is passed into schedulerOpts.
Testing
Verified against a live agent: @flow from chat, the builder loading a flow with a gate, a gate denying with no channel connected, a gate approved over WhatsApp, and a cron-scheduled flow stopping at its gate.