Hermes gateway provider — architecture review

Verdict

The outer architecture is genuinely good. A remote, process-less, network-backed agent was added to a provider system built entirely around spawning local CLI processes, and it required zero widening of ProviderDriver.ts. That is the strongest available evidence the SPI abstraction was drawn correctly. The protocol is versioned and fails closed, secrets are handled with real care, and the test suite contains the best concurrency tests in the repo.

The inner architecture is where the cost sits. State for a single Hermes instance lives in three places with no single writer and hand-rolled compensating writes that are Effect.ignored. A 1,258-line broker owns five separable concerns behind one global lock. And the reconnect story — the thing protocol v2 was specifically designed for — has a missing trigger, so it does not actually work end to end.

The shape

Two halves meeting at a versioned wire contract. Hermes dials into T3, so nothing on the Hermes side needs a public port.

What's working

The SPI held without modification

HermesDriver.ts:49–131 implements ProviderDriver exactly as CodexDriver does — same record shape, same defaultProviderContinuationIdentity, same maintenance-capabilities helper. Registration is a two-line change in builtInDrivers.ts. HermesDriverEnv = Crypto.Crypto against Codex's eight-service union shows the R-channel design scales down as well as up.

Unsupported capabilities are declared, not faked

Four typed TextGenerationError failures rather than stubs (HermesDriver.ts:28–47); attachments rejected explicitly at HermesAdapter.ts:416–422; rollbackThread fails after resolving the session (:544–555) so "no such thread" stays distinguishable from "unsupported." Each of these is a spot where a lazier implementation returns Effect.void and produces silent data loss.

Credential handling is defensively correct throughout

Timing-safe comparison with length pre-check (:135–138). Token consumption is a genuine compare-and-swap — Ref.modify deletes only if the entry is still identical, then re-checks expiry (:960–977). Old credential removed before the new one is issued (:431–442).

Best part: HermesGatewayEnrollmentResult (hermesGateway.ts:144–151) structurally cannot carry the credential — it only exists on the server→plugin connection.accepted. The token/credential split is enforced by the type system, not by review convention.

Generation-fenced connection state

Every accepted connection gets a monotonic generation (:1007); receive and disconnect bail unless it matches (:1081, :1085, :1121). The check sits inside the Ref.update, so it is atomic rather than TOCTOU. This is the correct fix for the classic reconnect race and is worth copying to any reconnecting transport.

A partially-degrading service default

Services/HermesGatewayBroker.ts:92–112 defines the broker as a Context.Reference whose default Effect.dies for operations but returns isConnected: false and empty streams for status reads. Status paths degrade gracefully; anything that would actually talk to a nonexistent gateway defects immediately instead of hanging. No other provider here has an equivalent.

content.snapshot is a real primitive, not a Hermes special case

Hermes emits cumulative revisable text, which a delta-only model cannot represent. Rather than special-casing, the change threads a proper primitive through providerRuntime.ts:423 → orchestration.ts:816 → decider.ts:1021 → both projections. It lands as an optional field on an existing event, so every persisted event decodes unchanged — zero migration. Any future provider with revisable output now has a first-class representation. (See finding H1 — the client reducer was missed.)

updateSettingsWith is a real improvement to shared infrastructure

serverSettings.ts:129–134 adds read-modify-write-under-lock, with the old updateSettings preserved as a special case. providerInstances is a whole-map field; before this, any patch computed outside the write lock could clobber a concurrent edit. Hermes needed it, but the fix is provider-agnostic.

Tombstoned removal, honored everywhere

Removal requires prior revocation (:729–736), writes a tombstone, and rolls back metadata on settings failure. Critically, ProviderInstanceRegistryHydration.ts:88–93 opts Hermes out of legacy-default synthesis — otherwise a removed instance would resurrect in the picker. Thread bindings outlive gateways, so id reuse would silently re-point old threads at a different physical machine.

Whitelist sanitization on both ends of the wire

HermesAdapter.ts:60–108 and protocol.py:128–168 independently project only per-type whitelisted fields, bounded at 4,096 chars. Tool arguments contain secrets; a pass-through would persist them into the event store and NDJSON logs. Defense in depth means a bug on either side cannot leak.

Fatal-versus-transient reconnect classification

connection.py:219–226 stops the retry loop entirely on instance-revoked, invalid-authentication, and version-incompatible, with the reasoning in a comment. Most reconnect loops get this wrong and hammer a dead credential forever without ever surfacing to an operator.

Test architecture is the best in the repo

Fakes sit at service boundaries; unused methods are Effect.die(new Error("unused")) so an accidental call is loud. Failure injection decorates the real fake rather than replacing it. Concurrency uses Deferred gates and explicit yieldNow loops, never sleeps — HermesGatewayBroker.test.ts:296–343 proves a colliding enrollment is actually blocked by asserting metadataPersistCount === 1 before releasing. Test names state invariants: "authenticates before applying incompatible connection state" is a regression test for an auth-bypass ordering bug.

What needs rework

H1 · The client reducer never learned about replace

High · verified. textOperation appears in exactly four non-test places: the contract, the decider, and the two server projections. The live client reducer at packages/client-runtime/src/state/threadReducer.ts:249–254 still does:

text: message.streaming
  ? `${entry.text}${message.text}`   // replace snapshot gets APPENDED
  : ...

The decider emits streaming: true and textOperation: "replace". So when Hermes revises its output mid-turn — the exact case v2 was built for — every connected browser renders "Hello worldHello, world!", and a refresh silently corrects it from the server projection.

Fix: the append-vs-replace rule is now hand-written in three implementations. Extract one pure mergeAssistantMessageText(existing, payload) into contracts and have all three call it. That structurally prevents the next payload-semantics field from landing in two of three.

H2 · Three-way state duplication with unrollbackable rollbacks

High. Instance state lives in a Ref<Map> (:177), a secret-store JSON blob (:221–233), and ServerSettings.providerInstances[id].displayName. renameInstance writes metadata first, then settings, then hand-rolls compensation at :620–622 and :634 — both Effect.ignored. If the rollback write fails (disk full, the same failure class that broke the settings write), the nickname now differs between stores and the operation reports failure. statusFromState reads the nickname from metadata while the model picker reads displayName from settings, so the UI shows two names for one gateway with no reconciler.

Also: readSettingsUpdate (:582–608) is a closure over a let mutated inside the update callback — a side channel out of a function the semaphore contract expects to be pure, silently yielding internal-error if the callback ever runs zero or twice.

H3 · The secret store is being used as a document database

High. Nickname, connectorUrl, revoked/removed flags, and a lastSeen blob are JSON-serialized into ServerSecretStore — a directory of 0600 files with no listing API, no index, no transactions. Consequences already visible in the code:

• No enumeration. Nickname-uniqueness does a full O(N) secret-file scan per call, in two nearly identical 38-line loops.
• Write amplification on a hot path. Every connect and disconnect rewrites the whole secret file (temp + chmod + rename) just to record connectedAt. Reconnect churn becomes fsync churn on the secrets directory.
• Tombstones leak forever. Verified: metadataSecretName is only ever passed to get and set, never remove. Removed instances leave a metadata file on disk permanently.

Fix: credential stays in the secret store — that is what it is for. Metadata moves to settings or SQLite. Volatile observation data (lastSeen, activeSessionCount) should not be persisted on every connect at all.

H4 · The broker is a god object with clean, uncut seams

High. One Effect.gen owns enrollment TTL, credential issuance, metadata persistence, nickname uniqueness, settings mutation, the connection registry, request correlation, two PubSubs, and a boot-time legacy migration. The seams are already visible:

• InstanceStore — persist/read/getState plus tombstones
• NicknameRegistry — the two duplicated loops, which already disagree
• ConnectionRegistry — generations, transports, replacement semantics
• RequestCorrelator — fully generic, nothing Hermes-specific in it
• EnrollmentService — mint, consume, expire

All of it serializes on one enrollmentSemaphore that also guards registerConnection. A slow secret write during one instance's enrollment blocks the WebSocket handshake of every other instance — a global lock protecting mostly per-instance invariants.

H5 · Reconnect-during-active-turn drops the turn

High · verified. The v2 recovery field session.ready.activeTurnId exists on both sides. The trigger does not. Verified: HermesAdapter subscribes to broker.stream for message forwarding only — it has no streamStatuses subscription, so nothing re-issues session.ensure after a reconnect. Trace it:

1. Socket drops; broker fails all pending requests.
2. Plugin's _active_turns survives — Hermes keeps generating.
3. Plugin reconnects and re-authenticates.
4. Nobody calls session.ensure.
5. The eventual turn.completed arrives for a turn orchestration already considers dead, gets rejected by the conflict gate, and the thread never settles.

The README lists "reconnect with bounded backoff" as in scope. The transport reconnects; the session does not. This is the missing half of a design that is otherwise already built.

H6 · Resume only works while both processes stay up

High. HermesAdapter.sessions is a plain Map; the plugin's five correlation dicts are plain dicts. Session ids are derived from threadId, so after a plugin restart resumed: true is reported while the plugin has no memory of the transcript — that is a lucky consequence of determinism, not a design.

Worse, _approval_requests and _user_input_requests are lost. A pending approval in the T3 UI after a plugin restart resolves to request-not-found and the turn hangs. Separately, SessionContext.turns accumulates every completed item unboundedly and is read only by readThread, which has no Hermes caller — an unbounded leak feeding dead code.

M1 · Generic activity fires for Codex, not just Hermes

Medium · verified. CodexAdapter.ts:472 deliberately emits itemType: "unknown" for item.updated and suppresses it otherwise. Before this branch those hit isToolLifecycleItemType and returned []. Now they produce a provider.activity row — and Codex's itemTitle returns undefined for unknown, so the summary falls back to the literal string "Provider activity".

Meanwhile provider.activity is verified to have exactly one non-test occurrence in the whole repo: the emitter itself. No UI handles it, so it falls through to a bare summary with none of the tool affordances.

Fix: unknown is the canonical "we couldn't classify this" sentinel and other adapters rely on it being inert. Add an explicit status-text item type instead, and give it real UI treatment.

M2 · activeTurnId persistence is a Hermes-shaped hole in shared code

Medium-high. ProviderCommandReactor.ts:530 changed a hard activeTurnId: null — whose deleted comment read "Provider turn ids are not orchestration turn ids" — to session.activeTurnId ?? null. Only Hermes sets that at startSession. A Hermes-motivated invariant was asserted globally by deleting the comment saying it did not hold.

There is also a concrete bug, verified at ProviderService.ts:332–334: terminalMatchesActiveTurn is folded into the same ternary branch as turn.started, so a matching turn.completed writes binding status "running" with activeTurnId: null. Not read for status decisions today — a persisted lie waiting for its first consumer.

M3 · The steer contextvar can swallow real assistant output

Medium-high. COMPATIBILITY.md acknowledges the ⏩ Steer queued string match. It does not acknowledge the interception mechanism. _capture_steer_control_response runs at the top of both send and edit_message and swallows any content whose chat_id matches while the contextvar is set. A steer targets a running turn — so if Hermes emits assistant output on that thread during the await, it is captured as control traffic and silently dropped, and because only messages[-1] is inspected, the prefix check then fails the steer closed.

Cheap fix: filter captures by message_id == control.request_id, which the plugin already sets, rather than by chat_id.

M4 · Protocol versioning cannot negotiate

Medium-high. A single global integer, hand-synced across two languages with no generated artifact, re-validated as a literal on every frame. The two halves ship on independent cadences and are installed by hand (ln -s), so the deployment model guarantees skew while the protocol guarantees hard failure on skew. A T3 auto-update to v3 bricks every paired Hermes until each user manually re-links and restarts.

The irony: HermesGatewayHelloCapabilities is a genuinely thoughtful forward-compatibility seam — it accepts any positive version specifically so a structured rejection is possible. That mechanism should be the foundation of range negotiation, not just of a nicer error message.

M5 · Ping and pong are specified, wired on the plugin, dead on the server

Medium-low · verified. HermesGatewayPing is in the T3→plugin union and the plugin implements the handler. Verified: nothing in the server ever sends one. So a half-open connection leaves isConnected true, the UI showing "connected," and send succeeding into the void until a request times out 30s later. Either implement the loop the protocol already defines, or remove it — unimplemented protocol surface is worse than absent surface, because the next reader assumes liveness is handled.

M6 · stopSession deletes local state even when the stop was never sent

Medium-low. If the gateway is offline the send is skipped but the local session is deleted anyway. The plugin still holds the thread, Hermes keeps running, and on reconnect emits deltas for a thread T3 has forgotten — which toRuntimeEvent then forwards contextless. The session.exited branch already guards for this correctly; content.delta and the item cases do not. stopAll runs this for every session in the shutdown finalizer.

Also worth queueing

• Enrollment tokens never expire from memory — no sweeper, expiry is lazy at redemption, and any operator-scoped client can call createEnrollment in a loop.
• The add-instance dialog writes settings before enrollment — a failed enrollment leaves an orphaned instance visible in the picker but permanently instance-not-found.
• request() has an interrupt window between registering the pending entry and entering the await pipeline where neither cleanup runs; nothing sweeps by age.
• One 30s timeout constant covers both a fast turn.start ack and a turn.steer that waits through a full dispatch into a running agent.
• _schedule does not verify loop identity before create_task, and holds no reference to the created task — GC can collect it mid-flight and exceptions surface only as warnings.
• session.stop reports recoverable: true, which T3 maps to status: "error" — so every deliberate stop is recorded as a failure.

Where the two reads disagree

Three places where the same code drew opposite verdicts. The disagreement is more informative than either side alone.

The pattern across all three: the contract-level design is consistently stronger than its rollout. Good primitives were added and then not threaded all the way to every consumer. That is a review-process gap more than a design gap — and it argues for shared helpers over parallel hand-written implementations, which is exactly what H1's fix does.

Recommended sequence

Ordered by return on effort, not severity.

Lower priority but worth queueing: range negotiation (M4), the steer message_id filter (M3, cheap), the ping loop or its removal (M5), and lifting toRuntimeEvent to a pure exported function so the state transitions become unit-testable without constructing a whole adapter.