Skip to content

feat(acp): Hermes runtime discovery and durable session load#2633

Open
joelbrilliant wants to merge 1 commit into
block:mainfrom
joelbrilliant:feat/hermes-native-profile
Open

feat(acp): Hermes runtime discovery and durable session load#2633
joelbrilliant wants to merge 1 commit into
block:mainfrom
joelbrilliant:feat/hermes-native-profile

Conversation

@joelbrilliant

@joelbrilliant joelbrilliant commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

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-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, so the unconditional variant is deliberately not part of the API.

Verification

Result
cargo test -p buzz-acp 604 passed, 0 failed
cargo clippy -p buzz-acp --all-targets -- -D warnings clean
cargo fmt -p buzz-acp --check clean
cargo check -p buzz-acp --locked clean

Rebased onto main at the post-#2773 merge. Cargo.lock is included this time, so --locked builds 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.

@joelbrilliant

Copy link
Copy Markdown
Contributor Author

Review request

Focused Hermes path for local managed agents (not the external-agent directory work in #2468).

What this adds

  • Desktop + buzz-acp Hermes discovery (hermes / hermes-acp)
  • Durable channel→session bind via session/load
  • Onboarding picker entry
  • Readiness: missing ACP deps → AdapterMissing (not fake logged-out)
  • Exact python/python3 marker-owned cleanup for Hermes venv scripts

Dogfood (2026-07-24)
Owner-only Rocky DM on Desktop: full Hermes tools after companion Nous acp.tool_policy: profile, instance parallelism=1, BUZZ_ACP_NO_MEMORY=true (string true, not 1), multi-turn replies + session restore path.

Checks

  • DCO / Semgrep / zizmor green
  • cargo test -p buzz-acp --lib 595 passed locally
  • Desktop hermes + interpreter unit tests green

Companion
NousResearch/hermes-agent#70326

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.

@rungmc357

Copy link
Copy Markdown

Hey Joel — I built on #2633 locally, preserving your three commits and authorship, and found one data-loss issue worth flagging before merge.

SessionStore is keyed by command/args but cached per process, so two buzz-acp processes using the same Hermes invocation can overwrite each other’s channel bindings. I fixed it with a stable sibling lockfile, a fresh read under lock for every get/put/remove, and atomic mutation writes. The regression opens two stores before either writes and proves interleaved writes and removals cannot clobber or resurrect bindings.

The full branch also includes optional Hermes profile selection (hermes --profile NAME acp) and canonical supports_profile metadata from my earlier implementation. Your #2633 work remains credited as the operational backbone; #2468 and @NYTEMODEONLY remain credited as prior Hermes groundwork.

Verification: focused session-store 3/3; lost-update regression passed 75 repeated runs; clippy, formatting, and diff checks green; buzz-acp 595/596 with the sole keepalive timing failure reproduced unchanged on the frozen base; broader Hermes discovery/runtime/profile/onboarding/frontend gates passed.

I didn’t open a competing PR. Happy to split the session-store fix or profile slice however is easiest for you to land.

joelbrilliant added a commit to joelbrilliant/buzz that referenced this pull request Jul 24, 2026
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>
@joelbrilliant

joelbrilliant commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks - good catch.

Agreed on the SessionStore lost-update: process-local cache + whole-file rewrite is unsafe when two buzz-acp processes share the same command/args identity. I've landed the session-store fix on this PR (lock + reload under lock + atomic write + independent-open regression, plus put recovery from a corrupt sidecar):

Verification here:

  • cargo test -p buzz-acp --lib session_store4/4
  • cargo test -p buzz-acp --lib596 passed / 1 failed, sole failure acp::tests::keepalive_resets_idle_past_deadline (timing flake; same class you saw on the frozen base)

I'd like to keep #2633 scoped to discovery + durable load + onboarding. Optional Hermes --profile / supports_profile is better as a follow-up PR so this one stays reviewable. Happy to review that split if you open it against main (or against this branch after CI settles).

Credits stay as noted for #2468 / @NYTEMODEONLY and your store fix. No need for a competing PR.

@rungmc357

Copy link
Copy Markdown

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.

@joelbrilliant

Copy link
Copy Markdown
Contributor Author

Appreciate it.

Store fix is on the branch (3025359), DCO / Semgrep / zizmor green, and the PR is mergeable pending maintainer review. Holding profile support for a follow-up after this lands is perfect.

If you want to help while we wait, the highest-leverage things are:

  1. A second pair of eyes on the dogfood notes in docs/hermes-agent-rocky.md (especially parallelism=1, BUZZ_ACP_NO_MEMORY=true as the string true, and acp.tool_policy: profile on the Hermes side)
  2. Flagging any residual multi-process session-store edge cases you still see against 3025359

Otherwise we can leave it for review as-is. Thanks again for catching the lost-update before merge.

@rungmc357

Copy link
Copy Markdown

I took a second pass against 3025359. The dogfood notes look right on parallelism=1, BUZZ_ACP_NO_MEMORY=true rather than 1, and acp.tool_policy: profile.

I found two residual session-binding edges:

  1. The restore path can still delete a newer binding. Process A reads session X, process B writes Y, then A’s failed load of X triggers a key-only remove that deletes Y. A remove_if_equals(X) under the existing lock, plus an interleaving regression, should close it.
  2. The multi-profile section presents a different HERMES_HOME as equivalent to unique -p <profile> acp args, but durable store identity excludes environment and managed-agent identity. Two managed agents in the same channel with identical command/args and different homes can therefore collide. The simplest short-term fix is to require unique profile args and remove the HERMES_HOME-only alternative until the store is namespaced.

I didn’t find anything else material.

joelbrilliant added a commit to joelbrilliant/buzz that referenced this pull request Jul 24, 2026
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>
@joelbrilliant

joelbrilliant commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Second pass taken - both residual edges were real.

1. Failed load deleting a newer binding
Confirmed in try_load_persisted_session: key-only remove after a failed session/load of X could drop a concurrent Y.

Landed remove_if_equals(expected) under the existing lock, wired the restore path to use it, and added an interleaving regression (remove_if_equals_does_not_delete_newer_binding).

2. HERMES_HOME-only multi-profile collision
Agreed. Durable store identity is command + args + channel, not env / managed-agent id. Updated dogfood docs + buzz-acp README to require unique -p <profile> acp args for a second Hermes profile and removed the HERMES_HOME-only alternative until the store is namespaced.

Commit: 3c83c15

Verification:

  • cargo test -p buzz-acp --lib session_store6/6

Thanks again for the careful review.

@joelbrilliant

Copy link
Copy Markdown
Contributor Author

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 session_store, no pool binding, no session/load (the only persistence in there is composer draft state). So Buzz's channel-to-session map is still in-memory only after BYOH lands, and a restart still orphans live agent sessions - the agent has the conversation, Buzz has forgotten which session belonged to which channel, and the user quietly starts over.

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 session_store.rs and pool.rs on top of the new seam and drop anything that overlaps. I've already built that slice locally against current main - 651 lines, cargo test -p buzz-acp green at 604 including the six store tests - so it's a rebase rather than a rewrite whenever #2773 is in.

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 remove_if_equals under the existing lock and an interleaving regression, so the persistence layer has had a proper pass rather than going in cold.

@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>
@joelbrilliant
joelbrilliant force-pushed the feat/hermes-native-profile branch from 3c83c15 to bfd1009 Compare July 27, 2026 00:29
@joelbrilliant

Copy link
Copy Markdown
Contributor Author

Thanks, appreciate it.

One thing that'll help when you look: CI on this PR is stuck on action_required. Every run since the branch opened is waiting on a maintainer to approve workflows for a fork contributor, so the Rust suite has never actually run here. The three green checks are just DCO, Semgrep and zizmor.

Locally against the merged head: cargo test -p buzz-acp 604 passed, clippy -p buzz-acp --all-targets -- -D warnings clean, fmt --check clean, check --locked clean. If someone kicks the workflow approval it should confirm that, but I can't trigger it from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants