feat(herdr): builtin Herdr pane reporter and user-decision block door (phase 1) - #2415
Open
makgunay wants to merge 33 commits into
Open
feat(herdr): builtin Herdr pane reporter and user-decision block door (phase 1)#2415makgunay wants to merge 33 commits into
makgunay wants to merge 33 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.
This was referenced Aug 15, 2026
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Collaborator
|
@makgunay Thanks for this. The block door fills a real gap — we had no way for the core to say "a human is being waited on," so nothing could report We want the feature. We need it split before we can review it. SizeThis PR is +5,887 / −50 across 46 files in 33 commits, and #2416 is +9,626 across 75 files — about 15,500 lines between them. Why your stack didn't come through as oneYou clearly meant to stack these, and this part isn't your fault. #2416 says its base is So So our clear preference is separate, smaller PRs rather than a stack. Open one focused PR against Suggested splitYour commits are already factored close to this:
1–3 are useful on their own even before 4 lands. What we'd want fixed
Scope noteWe've moved #2247 to P0 and are taking that fix in-house, along with #2022 (the The split is about getting this reviewed, not about the code. |
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:
main· Head:herdr-phase1@70be6eeb96356b9ee813c8b3bec95400822705c4· 33 commits · 46 files (+5887/−50)Related: #2210 · Herdr-side: herdrdev/herdr#2423
Problem
An Atomic session running inside a Herdr pane reports nothing. The pane shows
none:unknown: Herdr cannot tell whether the agent is idle, working, or waiting on a person. Nothing in the core tracks "a user decision is pending" either — an extension dialog, a trust prompt, or anyctx.uiwait is invisible to the host, so no reporter could ever know when to sayblocked.Solution
Four layers, bottom up:
0184f029a): a core registry of open user blocks (user-blocks.ts,block-types.ts), thepi.awaitUserDecisionAPI, andagent_blocked/agent_unblockedsession events. Blocks are handle-scoped and released exactly once.68a26731f…cb301f634): the singleget ui()accessor inrunner-context.tsreturns a proxy over a surrogate target that mints a block for everyselect/confirm/input/custom/editorwait. Wrapped members are called on the host object; getter receivers, inherited members, and dynamic properties survive (readUiMember); native, cross-realm, and structurally valid promises keep their identity and cancellation (settlement-observer.ts); the thenable probe is guarded and proxy integrity operations are refused.70be6eeb9closes the review gaps: interactive shortcut handlers now get the wrapped context too, and a failed turn's message stays latched across duplicateagent_settledevents.b17ad18de,6095446b0):withTrustPromptBlocksinrunner-project-trust.tsmints a block for an extension's own project-trust prompt, so that wait also readsblocked.85b691cbcand follow-ons): activates only inside a Herdr pane (env + socket gate, withheld from hosts with no terminal pane), seeds blocks already open before activation, serializes every socket write, publishesidle/working/blockedper the reducer (user blocks outrank everything), latches failures until an idle settlement, and sends exactly one release on quit (single-flight latch). Documented indocs/herdr.mdanddocs/extensions.md.Rebase provenance
Freshly rebased onto
origin/main@04e83e38e(2026-08-15): 33/33 commits replayed, one conflict (packages/coding-agent/CHANGELOG.md, entries moved under the new[Unreleased]; released sections untouched). An earlier rebase round dropped one pure.ts→.jsrespell commit to match upstream spelling; see scrutiny point 2.Test output
At
70be6eeb9(post-rebase verification):npm ci --ignore-scripts— clean, 0 vulnerabilities.npm run typecheck— clean (roottsc --noEmit+ coding-agenttsgo -p tsconfig.build.json --noEmit).pi-0.83.0-direct-fixes.test.ts(branch-modified), 18 passed.extension-block-door-public-api.test.ts, 1 passed.e4ee77d2the coding-agent workspace ran 3194 passed / 0 failed and roottest:unit5826 passed / 0 failed. The full suites have not yet been re-run at70be6eeb9; runnpm run check+npm run test:unitbefore merge.AssertNotAnybesideUiMemberValue; restoring the old alias fails typecheck withTS2344.Live: the reporter wire was verified against a real Herdr socket (strictly increasing sequence numbers, exactly one release), and the full pane-state behavior of the complete stack passed a 9/9 live scenario matrix at the
herdr-phase2tip (see PR2).Known defects in this PR, fixed in the stacked PR2
Both are phase-1-introduced and are fixed at the
herdr-phase2tip with regression tests and live verification. They stay in PR2 because the headline regression test for (A) needs PR2's lifecycle bridge, and moving the commits would invalidate the live-verified tip. If this PR is expected to land alone for any meaningful period, say so and we will split the fixes down.runner-ui-blocks.ts): the wrapper releases acustom()dialog's block only when its promise settles. An overlay hidden via its handle without settling (a pre-existing, deliberate contract of the workflows graph overlay onmain) holds the block forever, wedging the pane atblocked. Latent here; observable once any such caller runs. Fixed by10b430bbe(hide releases, show re-mints, settlement still ends bookkeeping once).925e50bde's rebased twin85b691cbclineage; predicate atsrc/extensions/herdr/index.ts): the builtin stands down whenever Herdr's installedherdr-agent-state.tsmerely loads — but the current Herdr server silently discards that asset'sherdr:pireports for an Atomic pane, so in the real-user default config nobody reports and the pane staysunknown. Fixed byb06ef6d43(explicit load-time supersession, env-gated; out-of-pane behavior unchanged).Reviewer scrutiny points
runner-project-trust.tsmerge — the largest semantic merge of the rebase. The branch's wholewithTrustPromptBlocksblock landed on upstream's drifted shape; confirm it still composes with any main-side changes toemitProjectTrustEventcallers.types,ui-types,session-manager-types) stay.ts; branch-owned new modules (user-blocks,block-types,runner-ui-blocks,settlement-observer, herdr siblings) use.js.module-import-specifier-consistency.test.tsenforces one spelling per module and passes, but the retained respell commit messages describe a convention the stack no longer fully carries._buildRuntimeauto-merge (agent-session-tool-registry.ts) — both hunks (thedetachUserBlocks()reload guard and the runner wiring) auto-merged onto upstream's new heartbeat-scheduler context. Verified:detachUserBlocks()precedesnew ExtensionRunner(...); covered by the loaded-extension-paths-cycle and single-writer tests.[Unreleased]after upstream cut0.9.14-alpha.1; confirm that matches the intended shipping vehicle.Breaking changes / limitations / follow-ups
pi.awaitUserDecision,agent_blocked/agent_unblocked. Behavioral note:ctx.uiis now a wrapping proxy — identity differs from the raw host context, integrity operations (sealetc. through the proxy) are refused, promise identity is preserved.docs/herdr.md/docs/extensions.md.herdr-phase2→herdr-phase1) carries workflow lifecycle reporting, the Non-quit session shutdown (/reload, /new, fork) orphans in-flight workflow runs instead of pausing them like quit does #2247 session-replacement fix, and the two defect fixes above.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
This change adds session-scoped tracking for extension waits on user decisions, instruments blocking UI and project-trust prompts, and publishes Herdr reporter state over its local socket lifecycle. One P2 repository-convention issue remains: the new runner UI block module uses a
.tsimport suffix where the project requires.jsESM specifiers. Update that import before merging.Confidence Score: 4/5
Merge is safe once the ESM import specifier is corrected.
The final finding set contains one non-security P2 issue and no P0 or P1 findings, which maps to a score of 4.
Files Needing Attention: packages/coding-agent/src/core/extensions/runner-ui-blocks.ts
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(herdr): close phase one review gaps" | Re-trigger Greptile
Context used: