feat: make workflow runs observable and resumable - #74
Conversation
5825343 to
cdfcc09
Compare
cdfcc09 to
af55f2d
Compare
|
Thanks @SSS135 — this is a substantial, carefully-tested piece of work (892 tests green here, verified). Before diving into review I want to be upfront about how I'd like to land it: right now it bundles several independent changes, and a few flip default behavior in ways that deserve their own focused review rather than riding in with the rest:
Could you split these into separate PRs, the way #46 was split into #62 + the rest? The One clarification so expectations line up: this PR is about manual lifecycle control and resume-safety — it doesn't include auto-resume-on-usage-limit (#27), which is a separate track. No problem, just noting it so nothing falls through the cracks. Really appreciate the depth here — keen to get the control tool in. Let me know if you'd like a hand identifying the split boundaries. |
…age limit resets (#78) Closes #27. A run that pauses on a provider usage/quota limit now auto-resumes once the quota is likely refilled, instead of sitting paused until a human runs /workflows resume. New standalone module src/usage-limit-scheduler.ts (UsageLimitScheduler): consumes only the manager's public event/query surface, staying decoupled from manager/persistence internals (minimal footprint against the pending large #74 rewrite). Event-driven 'fire and watch': arm a timer on a usage_limit pause, call resume() when it fires, and let the existing 'paused'/'complete'/'error' subscriptions drive backoff and cleanup. Default-on with per-run opt-out via ExecOptions.autoResume (persisted). Exponential backoff, hard attempt cap (5), 1m delay floor, 6h ceiling. resetHint parsed best-effort ('~3h', '1h30m', 'in 5m'), falls back to 5m. Cold-start re-arm: a run still paused-on-usage_limit at process start is rescheduled using time REMAINING (now - updatedAt), not a fresh full delay. An injectable clock/timer makes the timer-fire path fully unit-tested (26 tests): normal fire, backoff-on-repause, attempt-cap give-up, cold-start remaining-time math, opt-out skip, resetHint parsing, delay floor, and resume()->false not consuming an attempt. Additive-only to existing files: ExecOptions.autoResume + two PersistedRunState fields + scheduler wiring in extensions/workflow.ts (disposed on session_shutdown).
|
Thanks — I’ve started splitting #74 along the requested review boundaries and moved this monolith back to draft as the immutable reference. Ready for review
Placeholder drafts (implementation not started)
The default-16/no-hard-cap change from #74 has been dropped. The bounded navigator and paused-script amendment work remain local-only and are not in these replacements. I’ll keep #74 and |
|
Thanks @SSS135 — this is exactly the split I was hoping for, and I really appreciate that you also turned the three default-behavior changes into non-breaking ones: concurrency staying within the existing cap (#82), child-session resume as opt-in (#81), and keyword matching without changing defaults (#79). That removes the main thing that would have needed careful gating. One heads-up before you polish these: main moved since you branched — two changes just landed that touch the same area as your durable-resume (#80) and child-session (#81) PRs:
Rebasing onto latest main before you finalize will save you a conflict later. Happy to review each as you mark it ready — the |
|
Closing this original combined PR as superseded by the focused replacement series requested in review. All replacement PRs now contain their actual implementations, are rebased onto current
The original branch and Dropped from the replacement policy changes:
The local-only bounded navigator and paused-script amendment were not included in these replacements (edited-script resume is now independently present upstream via #84). |
Summary
This PR hardens workflow triggering, lifecycle control, observability, persistence, and resume behavior as one runtime-reliability change:
workflow_controltool for list/status/set_concurrency/pause/resume/stop/restart/remove16, accepts any positive integer without a plugin hard cap, and can be resized while a run is live or paused/resumepicker, and reopen paused agents at the last durable message/tool-result boundary;persistAgentSessions: falseremains an explicit opt-outWhy forced keyword triggering is disabled by default
Previously, merely typing
workfloworworkflowsin an ordinary message could rewrite the prompt and force workflow mode. That made it difficult to discuss the feature itself, mention documentation or paths containing the word, or use the word naturally without unexpectedly launching orchestration. Hidden prompt mutation based on one keyword is poor UX and produces false positives.The lexical forcing hack is also unnecessary: current models can select the advertised
workflowtool from its schema and system guidance when orchestration is appropriate. Explicit/workflows run ..., direct tool selection, saved workflow commands, and slash-command controls remain available. Users who still want keyword activation can opt in with/workflows-trigger on; when enabled, matching is Unicode/identifier-bounded rather than a raw substring check.Resume semantics
Default behavior changes
Child session persistence is now on by default because reliable turn-boundary resume requires a durable transcript. Users can opt out with:
{ "persistAgentSessions": false }Child transcripts live under
~/.pi/workflows/projects/<project>/agent-sessions/, not~/.pi/agent/sessions/, so they remain available through each run artifact'ssessionFilelinks without cluttering Pi's normal/resumepicker. The README calls out that full child transcripts may contain sensitive context.Concurrency now defaults to
16when neither the run nor settings specify it. The workflow schema accepts any positive integer and the plugin no longer clamps concurrency to 16; provider capacity andmaxAgentsremain the practical ceilings.workflow_control set_concurrencyand/workflows concurrency <runId> <n>resize running or paused workflows. Raising the limit releases queued agents immediately; lowering it never aborts active agents and waits for activity to drain below the new limit before starting replacements. The mutable FIFO gate is shared with nested workflows and the updated value is persisted for resume/restart.Automated validation
npm test: 892/892 passed (Biome, TypeScript, 59 suites)git diff --check: passedcreateAgentSessionfaux-provider integration verifies reopening and appending to the same session JSONL128and10_000are accepted without clamping, invalid/non-integer values are rejected, and the omitted default is161 → 3immediately starts queued work,3 → 1lets existing agents drain without aborting them, a later increase releases additional work, FIFO order is retained, UI/persistence update immediately, and cold paused runs retain the new value for resumeReal-provider validation
Provider/model:
openai-codex/gpt-5.6-solthrough the installed local extension.Pause/resume smoke
running, paused it during a tool turn, resumed the same run, and completed withRESUME_CHILD_OKSubagent was abortednotification; this draft includes the regression fix so lifecycle-controlled pause/stop no longer also emiterror(unexpected external aborts still do)42-agent concurrency/UI stress
8; max agents:42PROBE_nidentifier after ignoring terminal punctuation. The workflow's ownok:falsefield came from an overly strict test assertion that expected a final period while the agents returned the requested identifiers without one; it was not a runtime/concurrency failure.Process restart after 27 completed calls
Before forced process termination:
1After restarting Pi:
pausedrather than auto-runningpaused mid-run (session saved), and the never-started agent remained queued1After
workflow_control resume:INTERRUPTED_OKQUEUED_OKLive concurrency resize
1: one running, five queuedworkflow_control set_concurrencychanged1 → 3; queued agents were released immediately and status showed three running3 → 1while three agents were active; none were aborted, status truthfully showed three running against limit one, and no replacement started until activity drained3 done / 1 running / 2 queued, then4 done / 1 running / 1 queued1agent-sessionsdirectory outside/resumeRemaining manual checklist
The high-scale concurrency and real process-restart scenarios are now complete. This remains a draft pending any maintainer-requested manual checks for legacy artifact migration, safe-typing permutations, and the full navigator/keybinding compatibility matrix from
CONTRIBUTING.md; those paths have automated coverage.