feat(acp): Hermes runtime discovery and durable session load#2633
feat(acp): Hermes runtime discovery and durable session load#2633joelbrilliant wants to merge 1 commit into
Conversation
6050c14 to
12168f8
Compare
Review requestFocused Hermes path for local managed agents (not the external-agent directory work in #2468). What this adds
Dogfood (2026-07-24) Checks
Companion Happy to split or drop anything that blocks merge. External-agent / kind 10100 / relay-observer stays out of this PR on purpose so it can land cleanly. |
|
Hey Joel — I built on #2633 locally, preserving your three commits and authorship, and found one data-loss issue worth flagging before merge.
The full branch also includes optional Hermes profile selection ( Verification: focused session-store 3/3; lost-update regression passed 75 repeated runs; clippy, formatting, and diff checks green; I didn’t open a competing PR. Happy to split the session-store fix or profile slice however is easiest for you to land. |
SessionStore was process-local cache + whole-file rewrite, so two buzz-acp processes sharing the same agent command/args identity could clobber each other's channel bindings. Reload under a sibling lockfile for every get/put/remove, keep atomic writes, recover put from corrupt sidecars, and cover independent-open interleaving in regression tests. Reported and prototyped by @rungmc357 on PR block#2633. Co-authored-by: Georgio Constantinou <210088133+rungmc357@users.noreply.github.com> Co-authored-by: joelbrilliant <joelbrilliant1@gmail.com> Signed-off-by: joelbrilliant <joelbrilliant1@gmail.com>
|
Thanks - good catch. Agreed on the SessionStore lost-update: process-local cache + whole-file rewrite is unsafe when two Verification here:
I'd like to keep #2633 scoped to discovery + durable load + onboarding. Optional Hermes Credits stay as noted for #2468 / @NYTEMODEONLY and your store fix. No need for a competing PR. |
|
Thanks, that sounds right. I don’t want to open a competing PR. Happy to help get #2633 through, and I’ll hold the optional profile support for a separate follow-up after this lands. |
|
Appreciate it. Store fix is on the branch ( If you want to help while we wait, the highest-leverage things are:
Otherwise we can leave it for review as-is. Thanks again for catching the lost-update before merge. |
|
I took a second pass against I found two residual session-binding edges:
I didn’t find anything else material. |
Failed session/load used key-only remove, so process A reading X then failing load could delete process B's newer Y for the same channel. Clear bindings with remove_if_equals under the existing lock, and cover the interleaving race in regression tests. Durable store identity is command + args + channel, not HERMES_HOME. Require unique -p <profile> acp args for multi-profile managed agents and drop the HERMES_HOME-only alternative from dogfood docs until the store is namespaced. Also note long-lived ACP OAuth stay-alive ops in the Rocky dogfood guide. Reported by @rungmc357 on PR block#2633. Co-authored-by: Georgio Constantinou <210088133+rungmc357@users.noreply.github.com> Co-authored-by: joelbrilliant <joelbrilliant1@gmail.com> Signed-off-by: joelbrilliant <joelbrilliant1@gmail.com>
|
Second pass taken - both residual edges were real. 1. Failed load deleting a newer binding Landed 2. HERMES_HOME-only multi-profile collision Commit: 3c83c15 Verification:
Thanks again for the careful review. |
|
Repositioning this against #2773, now that #2656 is being closed in its favour. The overlap I flagged on #2656 was runtime registration and model-selection UX, and BYOH supersedes that half properly - a generic seam beats two harness-specific PRs, so that's the right call. What it doesn't cover is durable sessions: I had a look through #2773 and there's no That's the half this PR does, and it's independent of how a runtime gets registered. So the plan I proposed on #2656 still works with #2773 as the target instead: land BYOH, then I rebase this down to just Also worth calling out that @rungmc357's review here found two real session-binding edges (a failed restore deleting a newer binding, and a multi-profile identity collision). Both are fixed in the current head with @wpfleger96 no rush on this at all - happy to wait until BYOH is settled and then do the rebase against whatever the final seam looks like. Just sing out if you'd rather I close this and open a fresh persistence-only PR instead, either way suits. |
Channel to ACP session bindings are in-memory only, so every harness restart starts a cold conversation even against an agent that advertises `loadSession`. This persists the mapping as a small JSON sidecar under the process data directory, keyed by (agent command identity, agent args, channel id) so different agent binaries and profiles never share bindings. Heartbeats stay ephemeral and are never stored. The store is shared between processes, so a process-local cache alone is unsafe: two buzz-acp processes with the same agent identity would clobber each other. Every read and mutation takes a sibling lockfile, reloads the on-disk map under that lock, then writes atomically. Removal is conditional on purpose. After a failed `session/load` another process may already have written a newer session for the same channel, so `remove_if_equals` only drops the binding it actually failed to load. A key-only remove would delete the fresher one. Signed-off-by: joelbrilliant <joelbrilliant1@gmail.com>
3c83c15 to
bfd1009
Compare
Rescoped to persistence only, now that #2773 has landed.
The original version of this PR also did Hermes runtime discovery and registration. #2773 makes that generic (Hermes ships as a tier-2 preset on main), so all of it is dropped. What is left is the one piece none of the competing Hermes PRs covered: durable session bindings. That takes this from +1051/-29 across 18 files to +643/-7 across 6.
What
Channel to ACP session bindings are in-memory only, so every harness restart begins a cold conversation even against an agent that advertises
loadSession. This persists the mapping as a small JSON sidecar under the process data directory, keyed by(agent command identity, agent args, channel id)so different agent binaries and profiles never share bindings. Heartbeats stay ephemeral and are never stored.Cross-process safety
The store is a shared file, so a process-local cache alone is unsafe: two
buzz-acpprocesses with the same agent identity would clobber each other. Every read and mutation takes a sibling lockfile, reloads the on-disk map under that lock, then writes atomically.Removal is conditional on purpose. After a failed
session/loadanother process may already have written a newer session for the same channel, soremove_if_equalsonly drops the binding it actually failed to load. A key-only remove would delete the fresher one, so the unconditional variant is deliberately not part of the API.Verification
cargo test -p buzz-acpcargo clippy -p buzz-acp --all-targets -- -D warningscargo fmt -p buzz-acp --checkcargo check -p buzz-acp --lockedRebased onto
mainat the post-#2773 merge.Cargo.lockis included this time, so--lockedbuilds do not drift.Coordination
The split was agreed publicly on #2656: land the runtime registration work first, then rebase this down to session persistence on top. #2773 landed that seam generically instead, which is the better outcome, so this now sits on top of #2773 rather than #2656. No overlap remains with either.