fix(herdr,workflows): workflow lifecycle reporting and survival across session replacement (phase 2) - #2416
Open
makgunay wants to merge 45 commits into
Open
fix(herdr,workflows): workflow lifecycle reporting and survival across session replacement (phase 2)#2416makgunay wants to merge 45 commits into
makgunay wants to merge 45 commits into
Conversation
Every path that stops the agent to wait on a person now opens a block and releases it through the handle it was given. `pi.awaitUserDecision(label, reason)` returns a `UserBlock`; `release()` is idempotent and safe in a `finally`. There is deliberately no release-by-id, release-by-label, or release-all entry point, so one caller can never end another caller's wait. Blocks are reference counted and the oldest open label is the one presented, and `agent_blocked`/`agent_unblocked` publish on open and close as ordinary lifecycle events. `select`, `confirm`, `input`, `custom`, and `editor` are wrapped once, at the single `get ui()` accessor, so an extension gets this without knowing the block door exists. The wrapper is a memoized proxy rather than a spread: the host's optional members stay absent when it omits them, and accessor and method identity are unchanged. Return values, cancellation, and thrown errors pass through untouched, and the block is released in `finally` so an abort ends it. The project-trust prompt runs before any ExtensionRunner exists, so it mints its own `project_trust` block around its restricted UI call. The registry is module scope because the trust prompt predates the runner and successor sessions in one process must share the refcount. Runners subscribe on construction and detach on invalidation; `_buildRuntime()` detaches the outgoing runner, since a reload replaces it without invalidating it.
Running Atomic inside a Herdr pane now makes that pane show what Atomic is doing: `working` during a turn, `idle` once the turn has fully settled, and `blocked` while a dialog waits on the user or when the turn ended in a provider error. Nothing to install, nothing to configure. Idle is decided at `agent_settled`, which the host emits only after retries, compaction retries, and queued continuations, so the pane does not flicker between a failed request and its retry. That is why this needs no grace or debounce timer at all. The reducer is pure and total. Reporting is one serialized writer whose queued state coalesces to the newest value, over one strictly increasing sequence shared by state, session, and release messages — Herdr silently drops a report whose sequence is not above the last it accepted. A dead, refusing, or hung socket degrades to silence after one 500 ms attempt and one 1500 ms retry; no lifecycle path can be delayed or failed by it. Only state, pane id, a short label or error string capped at 120 characters, and a session reference cross the socket. Prompt text, tool arguments, and model output never do. Outside a Herdr TUI pane it is a complete no-op: no socket, no timer, no listener. It also stands down when a file-based `herdr-agent-state` integration loaded in the same cycle — decided from what actually loaded, not from what exists on disk, and re-checked at activation so a file integration that lands after this one still wins. A pane never gets two writers. Extension loading can be deferred until after the first frame, so the reporter binds on whichever lifecycle event it sees first and seeds its state from `ctx.isIdle()`. It is correct whether it arrived before or after the turn it is describing. Quitting drains the queue and then releases the pane. Reload, new, resume, and fork silence the outgoing instance instead; the successor re-reports, above the predecessor's sequence.
Adds a Herdr page covering what the reporter sends, when it is active, how it degrades when the socket is gone, and how it hands over across reload, new, resume, fork, and quit — including the cross-process sequence caveat. Documents `pi.awaitUserDecision()` and the `agent_blocked`/`agent_unblocked` events under Extensions, and records the Phase 1 work in the changelog. Also checks in the Phase 1 codebase research note.
Two defects found in review, both silent in the failure mode that matters. The session report went straight to the transport while state reports went through the queue, so a block opened during the session round trip started a second concurrent write. Herdr drops the lower sequence without saying so, so the pane simply lost a report. Session, state, and release now share one FIFO. Each request is built and sequenced when it is enqueued, and coalescing is limited to an adjacent pair of state entries — a session or release entry is a barrier, because collapsing across one would strand a sequence already drawn. Quit latches first, drains, enqueues the release through the same writer, then drains again, so a callback racing the quit cannot land behind the release. session_start no longer waits for the socket. Blocking it on a round trip let a hung Herdr delay the very session it was describing. Separately, a reporter that activated while a dialog was already open reported idle: block handlers returned before activation, and activation seeded only the idle/active flag. It now seeds from the block registry's snapshot before the first state is queued. A snapshot rather than a replay buffer, because events that fired before this extension loaded cannot be replayed, and the registry is module scope so a block outlives the runner a reload detached. Block events also activate now, so a wait can be the first lifecycle event a deferred load sees. The new single-writer tests gate the transport instead of using the socket fixture; the fixture answers immediately and hides a concurrent write behind the speed of loopback.
The reporter registered all seven handlers as soon as the Herdr environment was present, because ctx.mode does not exist at factory time. A headless RPC, JSON, or print run launched from inside a Herdr pane therefore carried listeners it could never use, which is not the complete no-op it is supposed to be. There is no extension-only fix: pi.on() returns void and cannot be undone, and a bootstrap listener would still leave one behind. So the row is withheld before the factory ever runs, through builtInExtensionsForHost(). The predicate is not appMode alone, and that distinction is the whole point. The isolated interactive engine child is spawned with piped stdio and no --mode, so it resolves to "print" while in fact driving the terminal UI through its host — gating on appMode would have silenced the one process that actually reports. The engine-child flag is part of hostPresentsTerminalPane() for exactly that reason, and a test pins it.
The block wrapper read through the proxy receiver and forwarded its own `this`, so a host method that keys per-instance state off its own identity — a WeakMap, a private field — saw a different receiver once wrapping was in place. Wrapping is supposed to be invisible, and this was the one way it was not. Reads and calls now both resolve against the host object, and the wrapper ignores its call-time `this`. Identity stability, absent optional members, cancellation, and error identity are unchanged, and a WeakMap-backed host method now pins the receiver in a test. The wrapper also typed its arguments, receiver, result, and Proxy trap as `unknown`, against the repo rule. Those are now mapped from the real ExtensionUIContext signatures. A source contract test greps for `any`/`unknown` in the block door and reporter files, since tsc accepts both.
The Herdr page and changelog claimed zero listeners outside a TUI and fully serialized writes. Both were true only after this round's fixes, so the wording now says what the code actually does: the builtin is withheld before its factory runs in a headless host, and state, session identity, and the release share one writer with exactly one request in flight. Also records that a reporter loading mid-turn or mid-dialog seeds from the host, and that the UI wrapper preserves host receiver identity.
The previous round fixed the receiver for the five blocking dialogs and left the
rest of the surface alone, which turned out to be the same bug with a wider
blast radius. `notify`, `setStatus`, and every other pass-through member were
returned raw, so calling them through `ctx.ui` ran them with the proxy as
`this`. A UI context whose methods read a private field threw outright —
`Cannot read private member #token` — on a host that worked before wrapping
existed. `AgentSession.bindExtensions({ uiContext })` is public, so that host is
a caller's to supply.
Every callable member is now forwarded on the host object, blocking or not. The
forwarders are cached per property and rechecked against the function the host
just returned, so a getter that hands back a different function later gets a new
forwarder instead of a stale one.
This does cost raw host-function identity: a forwarder cannot be the function it
forwards to, so `ctx.ui.notify === ui.notify` no longer holds. The two cannot
both be true, and the receiver is the one that changes behavior — the identity
that callers actually rely on is stable repeated lookup, which still holds and
is now asserted for `notify`, `setStatus`, and `requestRender`. The old
assertion has been replaced rather than deleted.
No `any` or `unknown` was added. The forwarder's declared `void` return never
reaches callers, because a `get` trap's return type does not affect a proxy
typed as ExtensionUIContext — which is what carries generic methods like
`custom<T>` and the `setWidget` overloads through untouched.
shortenReportMessage collapsed whitespace and trimmed every value, including ones already well under the 120-character cap, so a dialog titled " Keep\n spacing " reached the wire as "Keep spacing". The cap is the only thing this function is there to enforce; rewriting a caller's label beyond that is not its business. A value within the cap now passes through character for character. A longer one is its raw first 119 characters plus an ellipsis, rather than a collapsed rendering sliced afterwards. Embedded newlines stay safe on the wire because the transport serializes through JSON.stringify, which escapes them before the framing newline. Also adds a no-ack mode to the socket fixture and a latency test for it. Nothing about the queue or the timeouts changed — the test pins behavior the docs were describing wrongly: ordinary callbacks return without touching the socket, a non-quit shutdown returns at once, and quit spends a bounded budget attempting the release. The bound is expressed as FIRST_ATTEMPT_TIMEOUT_MS + RETRY_ATTEMPT_TIMEOUT_MS so it follows the transport rather than a literal.
The page claimed no lifecycle path can be delayed by the reporter, and the changelog repeated it. Against a Herdr that accepts connections and never answers, quit waits about six seconds: it drains the queue and then attempts the release, which is what L1 asks for. The implementation was right and the prose was wrong. The wording now separates the two cases — ordinary callbacks and non-quit shutdown never wait for socket I/O, quit deliberately does, and that wait is bounded by the transport budgets — and says the release is attempted rather than sent, since it can be dropped like any other report. Also records that a label within the cap is sent exactly as given and only a longer one is truncated.
A proxy may not hand back anything but the exact stored value for a non-configurable, non-writable data property on its target. The wrapper proxied the host directly and substituted forwarders, so the moment a caller passed a frozen UI context the first dialog threw: TypeError: 'get' on proxy: property 'select' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value Object.freeze makes every own method non-configurable, and a frozen ExtensionUIContext is a perfectly ordinary thing to hand to AgentSession.bindExtensions, so this broke a valid public caller. The proxy now sits on a fresh empty object that shares the host's prototype and resolves everything from the captured host. An extensible target carries no such invariant, so forwarders can still be substituted while `get`, `has`, `ownKeys`, and `getOwnPropertyDescriptor` all answer from the host. Reported descriptors are marked configurable, which is what the invariants require of a target with no own properties, and is what keeps Object.keys, spread, and JSON.stringify seeing what they saw before. Returning the raw function for frozen properties would have satisfied the invariant too, and was rejected: it silently drops the host receiver and skips block tracking for exactly the hosts that opted into immutability. Reflect.apply(original, host, args) is untouched, so the receiver fix stands.
Phase 1 binds this code to the ESM `.js` import form, and the herdr directory was the only builtin still spelling its own modules `.ts`. Its seven sibling imports now match llama, and a source test keeps them that way — nothing in biome or tsc distinguishes the two forms, so the rule only holds if asserted. The four imports that reach into src/core keep `.ts` deliberately. Those modules are spelled `.ts` everywhere else in the repository, and test/unit/module-import-specifier-consistency.test.ts requires one spelling per module; switching them turned that test red. llama resolves the same tension the same way. The assertion added here is therefore scoped to `./` specifiers, and the cross-directory half stays owned by the repository-wide test that already enforces it.
The Herdr page used "Trust project folder?" as an example blocked label, but that prompt is displayed by the terminal host before the interactive engine child exists, and the reporter lives in that child. The socket sees nothing until trust resolves, so the one label the docs named was the one label a user could never see there. The example is replaced with dialogs that do appear, and the limitation is now stated plainly on the Herdr page, beside the block events in the extensions guide, and in both changelog bullets. The events are session-scoped: a block opened before extensions are bound has no handler to reach. This documents the boundary rather than moving it. Reporting the startup prompt would need either pre-session reporting or host-to-child ownership transfer for block events, neither of which Phase 1 defines, and both of which need a writer handover story so the two processes cannot report the same pane at once.
The runner publishes block changes with a detached emit, so the open and close handlers for one dialog run concurrently. `ensureActivated()` was async: the first handler marked the reporter active and then suspended awaiting `onSessionStart()`, while the second handler saw "active" and took the fast path. The close therefore reported openBlocks 0 before the open reported 1, and the pane sat at blocked with an empty registry — until the next state change, which for an idle agent may never come. `onSessionStart()` never awaited anything; it only mutates reporter state and queues work. Both it and `ensureActivated()` are now synchronous, so concurrent handlers run to completion in arrival order and there is no window to overtake. The stand-down check, the ctx.isIdle() and block-snapshot seeding, the serialized writer, and the async shutdown drain are all unchanged. Shutdown now latches a closed flag before anything else. `_buildRuntime()` detaches the outgoing runner but cannot cancel an emit already in flight, so a detached callback could otherwise activate an instance the session had finished with. This enforces the existing non-quit silence rule rather than adding to it. The regression tests open and release a real block with no await in between, which is the interleaving that exposed this; the previous "block first" test awaited each emit and could not.
`blockLabel()` substituted the method name whenever a title was empty, so
`ui.select("", [...])` was reported as "select". Nothing asks for a non-empty
label — Herdr's own schema types `message` as a nullable string — and the
contract says unspecified input is preserved rather than rewritten, so this
invented text the caller never wrote.
Titled methods now use the title verbatim, empty string included. `custom()`
keeps its fixed label because it takes a component factory and has no title
argument at all, and the method-name fallback survives only for the impossible
case of a titled method called without a string title.
Nine annotation lines across three added test files used `unknown`, against the repo rule. The existing source-constraint scan only covered production files, so they survived a fully green `npm run check`. The socket fixture now parses through a named `JsonValue` and validates each field before constructing a `RecordedRequest`, rather than casting a candidate shape. The UI recorder types its arguments as the shapes the five dialogs actually receive. The assertion predicates are replaced by `captureThrow` and `captureRejection` helpers: `assert.throws`/`assert.rejects` take Node's `AssertPredicate`, which is `(thrown: unknown) => boolean`, so writing one inline forces the annotation — catching the error directly avoids it and still compares the exact object by identity. The scan now covers these files too, so the gap that hid them is closed.
Phase 1 binds this code to the ESM `.js` import form. `user-blocks` and `loaded-extension-paths` are modules this work introduced, so every importer of them is here to change and the whole spelling group moves at once — `module-import-specifier-consistency` requires one spelling per module, and a partial conversion is what turns it red. The two remaining `.ts` specifiers in herdr point at `core/extensions/types` and `core/session-manager-types`, which predate this work and are spelled `.ts` by twelve-plus importers across session-manager and compaction modules. Converting those means editing files with no connection to this feature for a purely cosmetic gain; the llama builtin leaves them `.ts` for the same reason.
The comment on `hostPresentsTerminalPane()` said the interactive engine child is spawned with no `--mode` and resolves to `print`. It is spawned through `RpcClient`, which prepends `--mode rpc`, so it resolves to `rpc`. The predicate is unchanged and was already right — it keys on the engine-child flag precisely because the mode is misleading — but the reason given for it was wrong, which is worse than no comment. The matching test comment is corrected in the same spirit, and now asserts the rpc-child case explicitly.
…r thenables Two defects in the UI wrapper, both introduced by earlier rounds of this work. Moving the proxy onto a surrogate target fixed frozen hosts but left writes unhandled, so every assignment landed on the surrogate instead of the host. The host never saw the change, and the surrogate gained an own property that then contradicted the descriptor this proxy reports — so the next read, `Object.keys`, or `JSON.stringify` threw outright. `set`, `defineProperty`, and `deleteProperty` now forward to the host, with the host as receiver so a host setter runs on its own object. One case still cannot be forwarded: defining a non-configurable property. A proxy may only report that as succeeding if its own target has a matching non-configurable property, and mirroring one would force `get` to hand back the raw function instead of the host-receiver forwarder. It therefore throws rather than being quietly normalized to configurable, because inventing a descriptor the caller did not ask for is the worse failure. The reasoning is recorded at the call site. Separately, the release check asked `result instanceof Promise`. A dialog from another realm — a vm context, an isolated host bridge — fails that check, and so does an ordinary thenable, and both released the block before the user had answered: the pane reported the agent free while it was still asking. The check is now structural, and settlement goes through `Promise.resolve(...).finally`. The synchronous path is kept so a synchronous test double behaves as before.
The stand-down answer lived in one module-scope array. Loading yields to the event loop between inline factories, and one process can run more than one loader — in-process subagent sessions do — so a second cycle could overwrite the first's answer while the first was still loading. The observable result was a pane with two Herdr reporters, or none, which is exactly what the stand-down rule exists to prevent. An AsyncLocalStorage handle scopes the answer to the load that produced it. `withLoadedFileExtensionPathCycle()` reuses an enclosing cycle rather than nesting a new one, so the pre-trust and final loads of a single reload still share one mutable answer — a file extension the later load discovers stays visible to a factory the earlier load already ran. A fallback handle keeps direct callers and existing tests working. The Herdr factory now captures its cycle instead of re-reading module scope, so its activation-time re-check consults the load it came from rather than whichever load happens to be current when the check runs. No public signature, event payload, or activation synchrony changes.
…with .js Both modules were introduced by this work, so every import site is here to move and the whole spelling group converts at once — `module-import-specifier-consistency` requires one spelling per module, and a partial conversion is what turns it red. The remaining `.ts` specifiers in this feature point at `core/extensions/types`, `core/extensions/ui-types`, and `core/session-manager-types`, which predate it. Converting those means editing 82 import sites across the package, most of them in files with no connection to Herdr, for a behavior-neutral spelling change. Left alone deliberately; the llama builtin makes the same trade.
…boundary Regression cases for this round's fixes: assignment, configurable definition and deletion reaching the host; a host setter running on the host; a cross-realm promise and a plain thenable each holding the block until they settle, including the rejecting path. Also pins the startup project-trust boundary that the Herdr docs describe. The prompt mints its `project_trust` block, and the registry is the only observer, because `resolveProjectTrusted()` runs before any ExtensionRunner exists — and under isolated interactive mode, in a different process from the reporter. Prose alone could drift; if a later phase adds pre-session reporting or a host-to-child handoff, this is the test that should fail and be rewritten. The new cycle test file joins the any/unknown scan.
…operations Three ways the UI wrapper could still misbehave, all found by review. Reading `then` to decide whether a result settles later can itself throw, because `then` may be a getter. That read sat outside the cleanup guard, so the error reached the caller with the block still open — the pane stayed `blocked` with nothing left to release it. The probe is now inside the guard: release, then rethrow the same error. `defineProperty` forwarded to the host before the proxy invariant was checked, so a non-configurable definition mutated the host and *then* threw. The caller saw a failure against an object that had already changed. It is now refused before the host is touched, so the operation fails and the host is exactly as it was. `Object.freeze(ctx.ui)` and `Object.preventExtensions(ctx.ui)` sealed the surrogate, and an empty non-extensible target cannot report the host's keys — so every later `Object.keys`, spread, or `JSON.stringify` on that context threw. `preventExtensions` now returns false: sealing a wrapped context fails immediately and the wrapper stays usable. `isExtensible` is deliberately not forwarded, since a frozen host behind an extensible surrogate would violate the invariants in the other direction. Making a wrapped context seal through to the host would need a mirrored shadow target instead of an empty surrogate, giving up the frozen-host and host-receiver behavior this wrapper exists to preserve. Phase 1 does not define integrity operations on a wrapped context; refusing them is the no-widening choice, and the supported path — freeze the host before wrapping — is tested.
… errors Two defects, both reachable in ordinary use. The reporter trusted the counts carried on `agent_blocked`/`agent_unblocked`. Each change is published with its own detached emit, and each emit awaits its handlers, so one other extension subscribing slowly to just one of the two events is enough to deliver them out of order. The late `agent_blocked` still claims a block is open, so the pane pinned at `blocked` over an empty registry — and for an idle agent nothing later corrects it. Both handlers now read the live registry, which is mutated synchronously before subscribers run. Branching on the live count rather than on which event arrived is the point: a late open for an already-closed block correctly reports released. Runner dispatch is deliberately not serialized. That would change delivery for every extension, and the pane reporter only needs the current registry state. Separately, `turnFailureMessage()` forwarded the provider's `errorMessage` verbatim. That field is whatever a provider or a custom streamSimple put there — `error.message`, a normalized response body, raw request metadata — and observed values carry authorization headers and echoed prompt and model output. Truncating it is not redaction. There is no typed error category to key on, and no pattern list is sound against arbitrary formats, so the reported text is now the fixed label that was already the fallback. The pane learns the turn failed; the detail stays in the transcript where it belongs.
…rompt A `project_trust` handler runs before Atomic's built-in prompt and receives the same restricted UI, so a dialog it opens stops the agent exactly like Atomic's does — and until now nothing reported it. Handlers get a derived context whose `select`, `confirm`, and `input` open a `project_trust` block for the duration of the prompt, on the host's own UI object, released on synchronous throw, on rejection, and on settlement alike. `notify` is untouched; it waits for nobody. The fallback prompt is untouched too: it already opens its own block, and wrapping both paths would count one wait twice. `ProjectTrustContext` keeps its public shape — this is a derived object with the same members, not a signature change. This is in-process bookkeeping. It does not make the startup trust prompt visible on the Herdr socket: that prompt runs in the host before the isolated engine child exists, which stays a documented Phase 1 boundary.
…c API The existing check reads the internal user-blocks module's exported names at runtime. That file is not in the root `tsc` program, and a name check cannot see a type — so the clause that actually matters, that a block ends only through its own handle, was never asserted against `ExtensionAPI` as consumers import it. This compiles against the published surface. The `@ts-expect-error` comments are the assertion: if a release-by-id method were added, or the reason union widened, or `release()` given a parameter, the directive becomes unused and the root typecheck fails. A positive assignment check would keep compiling through exactly those changes and prove nothing. The function is never called.
…ealed Two ways the ctx.ui wrapper still diverged from an unwrapped context. `Promise.resolve(result).finally(cleanup)` returns a *derived* promise, so a wrapped `ctx.ui.select()` no longer handed back the host's own object. Anything keyed on that identity — a host that caches or cancels by what it returned, a bridge, a test double — silently stopped matching. `observeSettlement` watches settlement on a separate branch through the intrinsic `then` and returns the original promise untouched; a plain thenable is adopted exactly once, because observing the raw one while also returning it would call a non-idempotent `then` twice. The observer's own rejection is swallowed so it cannot become a second unhandled rejection; the caller's reason is unchanged. The probe that decides whether a value settles later also has to consume the promise it creates. Left alone, it turned a perfectly handled caller rejection into an unhandled one. Separately, `Object.freeze`, `Object.seal`, and `Object.preventExtensions` threw on a wrapped context where they succeed on a raw one. Refusing them was the previous round's choice and it was wrong for the same reason the original bug was: it is a behavioural difference introduced by wrapping. Sealing now mirrors the host's own keys onto the proxy target — holding the *wrapped* values, so the receiver fix survives — and makes host and target non-extensible together. That keeps `isExtensible` truthful with no trap of its own and keeps `ownKeys` legal against a fixed key set. A host that arrives already sealed is mirrored at construction, so the wrapper never claims to be extensible when the host is not. The target stays empty until something actually seals, so every existing guarantee — forwarders for a frozen host's non-configurable methods, live host keys, later-added members — is unchanged in the ordinary case.
`ProjectTrustContext` is structurally typed and publicly exported, so a host may
hand over a class instance. Building the handler's context with `{...ui}` and
`{...ctx}` copied only enumerable own properties, which silently dropped a
prototype `notify` and any getter-backed `cwd`, `mode`, or `hasUI` — a handler
that called `ctx.ui.notify(...)` got `undefined is not a function` against a host
that worked before.
The derived context now names each member: the three prompts keep their block
wrappers, `notify` forwards to the host with the host as receiver, and `cwd`,
`mode`, and `hasUI` are accessors reading through to the original rather than
values snapshotted at wrap time.
The public shape is unchanged.
Clearing the queue and setting `silenced` left an attempt already in flight running. Against a socket that accepts and never answers, the predecessor still spent its 1500 ms retry and opened another connection *after* reload shutdown had returned — so a reporter that was supposed to have gone quiet could talk over the successor that replaced it. Sequence numbers stop a stale report being accepted after a newer one, but they do not stop the traffic or order it against a successor's first report. Each reporter now owns an AbortController. A non-quit shutdown aborts it, the attempt destroys its socket and resolves false, and the retry is skipped. Quit deliberately does not abort: it keeps draining and then releasing, which is what L1 asks for. The signal is optional on `HerdrTransport`, so a substituted transport can ignore it, and the 500 ms + 1500 ms discipline is otherwise untouched.
… sent The page and changelog still said the socket carries "a dialog title or a provider error string, both capped at 120 characters". Provider error text stopped crossing the socket when the fixed `Agent turn failed` label landed; the only free text left is a dialog title. The stale reporter comment said the same thing and is corrected too.
…flight `onAgentEnd()` wrote the provider failure straight into the reported state, so any later publish surfaced it — closing a dialog mid-turn reported the pane as `blocked: Agent turn failed` while a retry was still running and might yet succeed. That is the settled-only rule (S2) broken by a side door. The failure is now held pending and promoted only by an idle `agent_settled`; `agent_start` clears it, and a non-idle settle leaves it pending. Separately, two concurrent quits each sent a release. The silence guard ran before the `quitting` latch, so both callers got past it, and `released` is not set until the very end. `AgentSessionRuntime.dispose()` has no single-flight guard of its own. Quit is now latched into one shared promise before the first await: a second caller awaits the first rather than starting its own, and `quitting` counts as silenced so a late lifecycle event cannot queue a report behind the release. L1 is unchanged: existing work drains first, the release is still the final write, post-release reports are refused, and quit still does not abort the transport.
`type UiMemberValue = UiCallable | ReturnType<typeof Reflect.get>` looked narrow and was not: `Reflect.get` returns `any` in the lib types, so the union absorbed it and the alias *was* `any`. Every member read was unchecked, and the lexical `any`/`unknown` scan could not see it, because the word never appears. It is now `ExtensionUIContext[keyof ExtensionUIContext]`, read through a small `readUiMember()` helper that indexes the host directly — which keeps getter receivers, inherited members, and dynamic properties working exactly as `Reflect.get(host, key, host)` did. A compile-time assertion sits beside the alias, because a lexical scan cannot catch an inferred `any` and a regex for this one spelling would not catch the next one. Restoring the old alias fails the typecheck with `Type 'true' does not satisfy the constraint 'false'`, which is how it was verified rather than assumed.
Assistant-model: GPT-5.6 Luna
The neutral lifecycle bridge kept its own state machine beside the notice computation, and drifted from it twice. It published before storing the new signature, so a listener that synchronously invalidated the store re-entered `inspect` while the prior contribution was still absent and received the same event twice. It also had no name for a run that ended `killed`, `cancelled`, or `skipped`, and relabelled those as `completed`. The bridge now consumes one derivation shared with the notice path, which is total over `RunStatus`: a run that stopped without reaching a named outcome derives `quit`, the neutral "stopped without completing" kind, and a dropped contribution can only repeat a label already on the wire. A contribution stores just the three neutral fields that leave the bridge, so there is no route from stored state back to a fabricated notice. Publishing is queued and drained from the outermost pass, after every contribution is committed, on terminal paths too. Assistant-model: Claude Opus 5
…ment A non-quit shutdown cleared every active bridge event, and the replacement session reset the neutral snapshot before reinstalling notifications, so a successor had nothing to seed from. The run itself was still live at that point: the probe printed one active `started` contribution before shutdown and `[]` after it with the run still in the store, which is how a pane could read idle in the middle of a workflow after /reload, /new, resume, or fork. The shutdown now retains what it published, and the successor's bridge reconciles that against the store it can actually observe. A run still live keeps its contribution; one the successor cannot see has its contribution dropped explicitly rather than left behind as a phantom. That covers the ordering hazard from both sides, because a reporter activating before the successor installs sees the live run and is then corrected by the drop. Assistant-model: Claude Opus 5
`events` sat between the `on` overloads, which splits the overload set for no reason. `seedWorkflowContributions` had no production caller: the neutral lifecycle seed replaced it. Its one test now covers the same multi-run seeding through the seed the extension actually uses. Assistant-model: Claude Opus 5
The two consumers still classified runs separately: the chat notice path called `terminalNoticeKind`, `controlOccurrences`, and its own awaiting-input checks, while the bridge ran an ordered current-state machine beside them. They drifted, and the drift had a victim. `recordRunEnd` moves a run to its terminal status and leaves every stage status where it was, so a run killed, cancelled, or skipped while a stage awaited input, blocked, or paused still carried that stage. The bridge read the stage before the run and reported the pane blocked on a run that had already stopped — six of the nine status/stage combinations. Both now read one pure derivation. It returns the edge facts chat delivers and the single level state the bridge reconciles, and it is total over `RunStatus`: the terminal guard sits directly after `terminalNoticeKind`, before any stage is consulted, so a run that stopped without reaching a named outcome ends its contribution under `quit`. Chat keeps its own `notifyOn` filter, dedupe keys, retry state, and its deliberate silence on awaiting input. Assistant-model: Claude Opus 5
Every replacement session killed each in-flight top-level run and cleared the workflow store, so a successor genuinely had nothing to reconstruct from and a pane read idle in the middle of live work. That was right for two of the four boundaries and wrong for the other two. `/new` and `/resume` ask first. `session_before_switch` tells the user that switching stops the in-flight workflows and clears workflow history, and only proceeds when they agree, so stopping the runs there is the answer they gave and an idle pane afterwards is correct. `/reload` and `/fork` never ask, and the process keeps running those workflows either way; killing them destroyed work nobody asked to stop. They now leave the store alone, and the replacement bridge reports what is actually still running. The replacement tests drive the real `session_shutdown` and `session_start` handlers over all four reasons — no `setNotificationsActive` shortcut — and assert the pane state a real `HerdrReporter` receives. Assistant-model: Claude Opus 5
Lineage and terminal tombstones lived on the event bus with no eviction path at all once the successor stopped resetting the snapshot. A run that reused a retired id was then read as the continuation of a lineage that had ended long ago, and published nothing at all. `takeWorkflowLifecycleBridgeHandoff` moves both into the replacement bridge and clears them from the bus; active contributions stay, because a reporter that has not seeded yet still needs them. Inherited tombstones are consulted only by the first reconciliation. One it actually used is adopted, so the guard outlives the handoff for as long as that lineage is observable; one it did not is about runs nothing can see, and is discarded rather than left to suppress a later run. Assistant-model: Claude Opus 5
The last round kept the kill for `new` and `resume` on the grounds that the switch confirmation had promised it. Criterion 1 names all four boundaries, the literal contract says the criterion governs a conflict with existing copy, and an intercom peer adjudicated the same way. The confirmation text was the thing to change, not the criterion. `/reload`, `/new`, `/resume`, and `/fork` all replace the session inside one process that keeps running the workflows, so none of them now calls `killAllRuns()` or `store.clear()`; only `startup` and an unrecognised reason still clear, because neither names a predecessor that handed anything over. The successor reconstructs what it reports from the runs it can still observe, and a preserved run stays answerable — a stage waiting on input can be resolved after the switch, which the new test proves rather than asserting pane state alone. The `/new` and `/resume` confirmation no longer says switching stops in-flight workflows and clears history, because it does not; it says they keep running and points at `/workflow status`. Declining still cancels the switch. Two tests encoded the old destructive behavior incidentally and now assert the new one. The consequence to know about: the run store is process-scoped, so finished runs from before a switch stay visible in the new session. Retaining only live runs would need a store operation that does not exist, and inventing one is a separate contract decision. Assistant-model: Claude Opus 5
Assistant-model: GPT-5.6 Luna
…ation
The host loads file extensions through jiti with moduleCache: false and a
generation-busted specifier, so /reload evaluates the workflows package's
whole module graph again while the session keeps running. Every run-scoped
module singleton — the run store, stage and tool control registries, the
cancellation registry, the job tracker, and the stage-UI broker — restarted
empty: the successor answered "run not found" for a run its predecessor's
graph was still executing, no completion notice ever arrived, and the
lifecycle bridge dropped the run's contribution as unobservable, so a Herdr
pane read idle mid-run. The suite's replacement tests missed all of it by
reusing one module graph and injecting a raw bus as pi.events.
Add sessionScopedExtensionState(scope, key, create) to the host: a WeakMap
of keyed state per canonical event bus, the one object that outlives every
extension load generation. The workflows extension now exports each
run-scoped singleton as a stable facade and re-binds ("adopts") it to the
host session at factory time, so the first load registers its instances and
every later load of the same session finds them. Without a host bus (unit
tests, embedded use) nothing is adopted and the module-local instance is
used, exactly as before.
The orphaned predecessor graph keeps executing its run after the host
invalidates its API, so makePersistencePort now swallows exactly the
stale-extension-context rejection on transcript appends rather than letting
a run fail on an advisory entry.
The new regression test replays the host's real /reload ordering — real
loader runtime, real createExtensionAPI facades, session_shutdown, runner
invalidate, a genuine re-evaluation of the workflows module graph, then
session_start(reason "reload") — and fails without this fix at exactly the
live symptom: the successor cannot see the in-flight run.
Assistant-model: Claude Fable 5
Opening the workflow graph overlay goes through ctx.ui.custom, whose block-minting wrapper released its "Custom dialog" user block only when the dialog's promise settled. The overlay adapter deliberately never settles it on Ctrl+X: it hides the mounted overlay through the OverlayHandle so reopening needs no remount. The block therefore outlived the visible dialog, and user blocks outrank workflow contributions in the pane reducer, so a Herdr pane that had ever shown the overlay stayed blocked through every gate answer, run completion, and later settled turns — the wedged pane observed live with the two-gate demo-approvals run. The single-gate matrix scenario never opened the overlay (a run without inputs skips the inputs form and its auto-attach), which is why one gate settled while two "leaked": the discriminator was the overlay, not the gate count. The workflow lifecycle bridge itself was publishing the correct awaiting_input/resumed/completed sequence throughout. Interpose on options.onHandle for custom() dialogs: the caller receives a handle whose setHidden(true)/hide() release the current block and whose setHidden(false) opens a new one, while settlement still ends the bookkeeping exactly once. A hidden-but-mounted overlay is a wait nobody is in, so the pane reads the workflow's own state again. Callers that request no handle keep their exact arguments, object identity included. The new root regression test drives the full real path — a real executor run with two sequential ctx.ui gates, the real lifecycle-notifications bridge over the runner's bus, the real Herdr builtin, block door, and socket fixture, and the real graph overlay adapter mounted through the runner's wrapped ctx.ui — and on the pre-fix sources fails at exactly the live symptom: the pane never returns to working after the overlay hides. Verified live in a Herdr pane: blocked only while the overlay is shown or a gate waits, working after each answer, done at completion.
…ng to it With Herdr's installed pi integration present at ~/.pi/agent/extensions/herdr-agent-state.ts — the real-user default — the builtin reporter stood down because the file had loaded, and the asset itself is a silent no-op on this base: the Herdr server ACKs its source "herdr:pi" / agent "pi" reports for an Atomic pane but discards them (verified against the live socket: an identical report differing only in source/agent updates the pane). Loaded-therefore-defer meant nobody reported, and the pane sat at unknown with no agent label. Replace the blind deferral with explicit load-time supersession. A new core module recognizes the installed integration files by basename and mirrors the builtin's activation gate; inside a Herdr pane the resource loader skips those files at both extension-path assembly points, so the asset can neither report (no second writer, no label flapping if a future server accepts it) nor displace the builtin. The builtin's factory and activation stand-down checks are gone: it reports whenever it is active. Outside a Herdr pane nothing changes and the file loads exactly as before. herdr-supersession.test.ts pins the predicates and drives a real DefaultResourceLoader over an agent dir carrying the asset: skipped with the pane environment set, loaded without it. The former stand-down tests now pin the inverse — a loaded herdr-agent-state path no longer silences the builtin. All of them fail on the pre-fix sources. Verified live: with the asset present the pane reports atomic:idle within seconds of launch.
Contributor
Author
|
Note for reviewers: maintainer guidance on #2247 (comment) crossed with this PR's submission by minutes. This PR implements the boundary as continuation (runs keep executing across replacement) rather than the shared graceful-pause shape proposed there; the mapping against each point of that guidance, and our offer to rework toward pause-parity if preferred, is in this #2247 reply. |
Collaborator
|
@makgunay Thanks for this one. Clearing the bridge on defect A before pinning it on the overlay's hide-without-settle contract was the right order to work in, and writing down that the discriminator was the auto-attached graph overlay rather than gate count will save whoever hits this next. The live matrix and the fail-first evidence are useful. Two problems: how this landed, and how big it is. This PR isn't actually stackedThe description says the base is Not your mistake. Both PRs come from your fork, and a fork PR can only target a branch that exists upstream. The practical consequence is the important part: upstream never renders this as a stack. There's no per-layer view here, so the "review So our clear preference is separate, smaller PRs rather than a stack. One focused PR against SizeWorth saying that even your intended scope is too big for one PR. The 12 phase-2 commits alone are +3,845 / −190 across 43 files and do at least five separable things — a new public core API, two independent lifecycle fixes, the bridge, and two defect fixes. Suggested split, following your commit structure:
#2247 and #2022We've moved #2247 to P0 and are taking it, together with #2022 (the We're building on your commits, not around them. Reasoning, including where we land on continuation vs pause-parity, is in the issue. For you: those commits drop out on rebase and this PR ends up scoped to the lifecycle reporting. What we'd want fixed
We want the Herdr work. Splitting it is how it gets reviewed and lands sooner. |
6 tasks
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
2 participants
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.
Base:
herdr-phase1· Head:herdr-phase2@b06ef6d4304ddc317905f50f2093f771e9d95f61· 12 commits · 43 files (+3845/−190)Closes #2247 · Related: #2210 · Herdr-side: herdrdev/herdr#2423
Problem
Three, in escalating severity:
idlewhile stages execute, and a human-in-the-loop gate never readsblocked.session_startrankillAllRuns()+store.clear()(extension-lifecycle.ts:91-92), so/reload,/fork, and/newdestroyed live runs. Upstream still does this on currentmain(04e83e38e) — verified during the rebase./reloadloses the runs anyway./reloadre-evaluates the workflows extension's whole module graph while the session keeps running (clearExtensionCache()+ generation-busted specifiers,loader-virtual-modules.ts:361-366, jitimoduleCache: falseat:484), so every run-scoped module singleton restarts empty:/workflow statusanswered "run not found", the bridge dropped the live contribution (pane idle mid-run), and the completion notice never arrived — the predecessor's notifications were uninstalled at shutdown and its API facades staled byinvalidate().Two further defects were found by live testing of the stack itself; both are phase-1-introduced and disclosed in PR1:
blockedfrom gate 1 to pane close, through answers and completion.~/.pi/agent/extensions/herdr-agent-state.tspresent (the real-user default), the pane sat atnone:unknownforever.Solution
Lifecycle bridge (
fa40536b7, refined by4c3756bdd,c23dad4ec,f30fbff7b,0f756b5c4): the workflows extension publishes run lifecycle events; the Herdr builtin consumes them as pane contributions. State is derived once and committed before publishing; chat notices and the bridge share onederiveRunLifecyclepath; the bridge handoff is bounded to the handoff itself.Survival across replacement (
6556cb6ac,74e2cce85,58a90e881,9e605fdf1): stop the successor'skillAllRuns/store.clearteardown, preserve live stages and reconstruct pane contributions across every session replacement.Survival across
/reload's module re-evaluation (df630c97d): new host APIsessionScopedExtensionState(scope, key, create)(packages/coding-agent/src/core/extension-session-state.ts, exported fromsrc/index.ts) — a WeakMap of keyed state per canonical event bus, the one object that outlives every extension load generation. The workflows package's six run-scoped singletons (store,stageControlRegistry,cancellationRegistry,toolControlRegistry,jobTracker,stageUiBroker) become stable facades adopted to the host session at factory time (shared/session-scoped-singleton.ts,extension/adopt-session-run-state.ts). Without a host bus (unit tests, embedded use) the module-local instance is used — prior behavior unchanged. Containment:makePersistencePortswallows exactly the stale-extension-context rejection on transcript appends from the orphaned predecessor graph.Defect A fix (
10b430bbe): the bridge was proven innocent — reproduced three ways, it published the correctawaiting_input → resumed → … → completedsequence with an empty contribution map at completion. The leak was a user-decision block minted for the workflow graph overlay: the overlay hides on Ctrl+X via itsOverlayHandlewithout settling (a deliberate, preserved remount-avoidance contract, pre-existing onmain), and the phase-1 wrapper released the "Custom dialog" block only at settlement. The single-vs-multi-gate discriminator was the auto-attached graph overlay (inputs form), not gate count — do not chase the bridge. Fix in the core wrapper (runner-ui-blocks.ts): interpose onoptions.onHandle; hidden overlay ⇒ block released; re-shown ⇒ block re-minted; settlement ends bookkeeping exactly once; callers withoutonHandlekeep exact argument identity.Defect B fix (
b06ef6d43): the builtin blindly stood down wheneverherdr-agent-state.ts/jsmerely loaded, while the current Herdr server ACKs but discards that asset'sherdr:pireports. Replaced with explicit load-time, env-gated supersession: inside a Herdr pane the resource loader skips the installed integration files by basename at both extension-path assembly points (never a second writer, even if a future server acceptsherdr:pi); the builtin reports whenever active. Out-of-pane behavior is byte-identical.Regression evidence (fail first, then pass)
test/unit/workflow-run-state-real-reload.test.tsreplays the host's exact/reloadsequence —session_shutdown("reload")→runtime.invalidate()→ genuine module-graph re-evaluation → fresh generation →session_start("reload"). On pre-fix sources (verified at503019133, the pre-rebase twin of58a90e881) it fails at the live symptom:AssertionError: successor session must still observe the in-flight run after /reload. Post-fix: passes.test/unit/herdr-pane-workflow-multi-gate.test.ts(real executor run, twoctx.uigates, real bridge, real builtin, real block door, real overlay adapter, socket fixture). With the fix stashed it fails at the live symptom:timed out waiting for working; saw [... {"state":"blocked","message":"Custom dialog"}]. Post-fix: passes. Plus 4 new wrapper unit tests inextensions-ui-block-wrapping.test.ts(all 39 in file pass).herdr-supersession.test.ts(realDefaultResourceLoader; asset skipped with pane env set, loaded without) and flipped stand-down tests inherdr-activation.test.ts/herdr-extension-integration.test.ts. With the B wiring stashed, 5 tests fail; post-fix all pass.Test output
At
b06ef6d43(post-rebase verification):npm run typecheck— clean on the first run (roottsc --noEmit+ coding-agenttsgo).npx biome checkon the hand-mergedextension-runtime-state.ts— clean.Earlier full runs on pre-rebase twins (content lines byte-identical per the transplant-fidelity check): at
e302bf24f,npm run test:unit6193 passed / 1 skipped (639 files) andnpm run test:integration487 passed (37 files); atd9ee0587a, final fix-effort runs 202 coding-agent + 136 root tests green,npm run typecheckandnpm run checkclean.Live verification
Scenario matrix at this exact tip (
b06ef6d43, live Herdr panew1J:pF, real session, real socket, real workflow runs, pi asset present): 9/9 pass (.atomic/workflows/runs/herdr-live-matrix/matrix-i1.json).launch=atomic:idleidle→workingsustained across 11 polls →donedialog=atomic:blocked after=atomic:doneearly=working chatIdle=working after=donebothLive=working shortDone=working bothDone=donerun=working dialog=blocked back=working settled=doneblockedSeen=true settled=atomic:done completed=truepre=working, 4 post-reload polls allworking,notice=truefinal=none:unknownW8
/reloadprobe (panew1J:p5, ate302bf24f):livetest seconds=45,/reloadmid-run → pane stayedworking(8 samples), background panel survived,/workflow statusfound the run, both stages completed (~1m30s), completion notice delivered in the successor, pane →done.runSurvived: true,pass: true.Two-gate + coexistence probe (pane
w1J:pD, atd9ee0587a, pi asset present throughout):coexistOk,g1BlockedSeen,postG1WorkingSeen,g2BlockedSeen,postG2WorkingSeen,completedIdle,postSettleIdleall true;flapSeenfalse.Reviewer scrutiny points
makeTerminalNoticemerge (lifecycle-notifications.ts, lines 531–567) — the one place upstream's feat/ctx exit failed 2242 #2330 author-exit refactor and this branch overlap inside single functions. The merged function carries both the feat/ctx exit failed 2242 #2330 fields (outputs, failed-author-exitresumable,exitReasonfallback) and phase 2's shape;deriveRunLifecycle/deriveCurrentRunStateroute through it.outputsnever enters a bridge contribution (kind/label only), so nothing new crosses the Herdr socket. Verified by hand; please read it.registerEventBusFacade/canonicalEventBusForlive inevent-bus.ts, not the lifecycle module, so the loader does not import workflow code. If upstream later gives extensions direct bus access, the registry becomes a resolvable no-op./reloadfix assumes theResourceLoader(and itseventBus) survives session replacement; confirmed on this base (resource-loader-reload.tspassesstate.eventBusthrough). A future upstream change that rebuilds the loader per session would empty the handoff again; the "later load" test covers facade churn over one bus only.sessionScopedExtensionStatehost API — a new public core API (WeakMap keyed on the canonical event bus). Keys carry@1version suffixes to force a fresh start on deliberate shape changes; an incompatible unstamped change would hand new code an old-shaped instance. Kept in its own conventional commit rather than a fixup so the API is not buried in a workflows-only change.extension-runtime-state.tsinterleave — commits 1, 3, and 7 merged the bridge-handoff rewrites (takeWorkflowLifecycleBridgeHandoff) into upstream's new heartbeat-scheduler context (commit 1 had an import-block conflict; 3 and 7 auto-merged). Line placement hand-verified, tests cover the handoff path, but this is the file where the two series interleave most tightly.herdr:pireports for an Atomic pane is inferred from black-box socket probes (likely agent-vs-process validation); the server source was not read. The fix does not depend on the reason.custom()overlays."Custom dialog", not the gate label; the original findings never recorded the label, so that detail could not be cross-checked./reloadare dropped (stale-context containment) rather than rerouted; run state, notices, and pane reporting are unaffected.pi.events; only the scope test exercises the loader-built facade path.Breaking changes / follow-ups
sessionScopedExtensionState(see scrutiny point 4). Out-of-pane and embedded behavior unchanged.[Unreleased]with upstream's bug(workflows): late continuation can erase open-claude-design feedback and silently approve a stale preview #2401 entries after0.9.14-alpha.1was cut; confirm the shipping vehicle.blockedafter custom UI other than the graph overlay, refcount drift across long sessions, and label behavior if a future Herdr server starts acceptingherdr:pi(the supersession fix already prevents a second writer).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
This change preserves active top-level workflow reporting when sessions are reloaded, replaced, resumed, or forked, and ensures Herdr reflects terminal workflow cleanup, user-dialog priority, and custom-overlay visibility correctly.
The exercised workflow replacement path confirms that live stage controls now survive the session boundary. The same behavior failed before these changes, while the updated workflow, Herdr socket, lifecycle-priority, and hidden-overlay checks pass with the change. No defects were found.
Confidence Score: 5/5
The changed workflow lifecycle and Herdr reporting behavior is safe to merge based on direct execution of session replacement, workflow-state reconciliation, socket reporting, and overlay visibility flows.
No actionable findings remain. The focused checks exercised every session replacement mode, terminal workflow cleanup, user-dialog precedence, and hidden custom-overlay behavior, with the updated implementation passing all covered assertions.
Files Needing Attention: No files require follow-up. The primary exercised areas were workflow lifecycle state, session-scoped run adoption, Herdr reporting, and UI block wrapping.
What T-Rex did
Reviews (1): Last reviewed commit: "fix(herdr): supersede the installed pi i..." | Re-trigger Greptile