Skip to content

feat(acp): honor --toolsets and add per-session tool scoping - #45958

Open
awizemann wants to merge 2 commits into
NousResearch:mainfrom
awizemann:feat/acp-toolset-scoping
Open

feat(acp): honor --toolsets and add per-session tool scoping#45958
awizemann wants to merge 2 commits into
NousResearch:mainfrom
awizemann:feat/acp-toolset-scoping

Conversation

@awizemann

Copy link
Copy Markdown
Contributor

Closes #45955.

Problem

ACP sessions always received enabled_toolsets=["hermes-acp"]: acp_adapter/session.py::_make_agent hardcoded the list, and the hermes acp --toolsets flag was parsed but never threaded into the adapter. An ACP client driving a single long-lived hermes acp process across many sessions (e.g. a GUI with per-project chats) had no way to scope a session's tools.

Change (additive, backward compatible)

  • SessionManager(default_toolsets=…) — a process-wide default (accepts a list or a comma/space string).
  • create_session(toolsets=…)_make_agent(toolsets=…) resolve through a new SessionManager._resolved_toolsets, with precedence: per-session override > process default > ["hermes-acp"].
  • session/new reads an optional per-session toolsets (top-level field or under ACP _meta).
  • hermes acp --toolsets is forwarded (cmd_acpentry._parse_argsHermesACPAgent(default_toolsets=…)) and finally honored.

A session that specifies nothing still resolves to ["hermes-acp"], so existing behavior is unchanged.

Notes / scope

  • Resumed/loaded sessions use the process default (per-session overrides aren't persisted to the session DB — could be a follow-up if desired).
  • Open to adjusting the wire shape for the per-session override (top-level toolsets vs _meta.toolsets) — happy to align with whatever you prefer.

Tests

tests/acp_adapter/test_acp_toolsets.py (4 tests) covering normalization, the resolution precedence, the resolved list feeding _expand_acp_enabled_toolsets, and session/new param extraction.

$ pytest tests/acp_adapter/ -q
23 passed

(19 existing + 4 new; modified modules import clean.)

@alt-glitch alt-glitch added type/feature New feature or request comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists labels Jun 14, 2026
@awizemann

Copy link
Copy Markdown
Contributor Author

Prior art + a design note for reviewers — re: why this threads toolsets as explicit params rather than reusing gateway/session_context.py.

This change serves the same per-session-isolation concern that motivated gateway/session_context.py (#23847, originally requested in #23199). That module moved session-scoped state off process-global os.environ to task-local contextvars precisely because, in the concurrent asyncio gateway, "Message A's value was silently overwritten by Message B." A long-lived hermes acp process multiplexing per-project chats has the identical concurrency-isolation requirement — this PR addresses it one layer up, at agent construction.

I deliberately thread toolsets as explicit parameters (SessionManager(default_toolsets=…)create_session(toolsets=…)_make_agent(toolsets=…)) instead of reusing the session_context ContextVars, because:

  • Toolset selection is a constructor input, not ambient runtime state. It's consumed at agent-construction time (_make_agent), whereas contextvars fit read-at-runtime routing metadata (platform / chat_id / thread_id / session_id).
  • Explicit params are concurrency-safe by construction. Each create_session / _make_agent call receives its own value, so there's no shared mutable state to clobber across concurrent ACP sessions — the exact failure mode that pushed session state to ContextVars in the first place.

Precedence stays additive and backward-compatible: per-session override > process default_toolsets > ["hermes-acp"].

awizemann and others added 2 commits June 28, 2026 12:12
ACP sessions previously always got enabled_toolsets=["hermes-acp"]:
`_make_agent` hardcoded the list and `hermes acp --toolsets` was parsed but
never threaded through, so per-project tool scoping was impossible for an
ACP client driving one process across many sessions.

- SessionManager(default_toolsets=...) sets a process-wide default
- create_session(toolsets=...) / _make_agent(toolsets=...) resolve via a new
  _resolved_toolsets: per-session override > process default > ["hermes-acp"]
- session/new accepts an optional per-session `toolsets` (top-level or _meta)
- `hermes acp --toolsets` is forwarded to the adapter and honored
- tests/acp_adapter/test_acp_toolsets.py (4 tests)

Backward compatible: a session specifying nothing still gets ["hermes-acp"].

Closes NousResearch#45955

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Map alan@wizemann.com -> awizemann so the Contributor Attribution Check
passes for NousResearch#45958 (first contribution).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@awizemann
awizemann force-pushed the feat/acp-toolset-scoping branch from 416fc95 to bdbad40 Compare June 28, 2026 16:15
@awizemann

Copy link
Copy Markdown
Contributor Author

Heads up for reviewers: the failing Build&Test Docker image / build-arm64 check here is a fork-PR CI issue, not a problem with this PR's code. Everything that exercises the change is green — all Python test slices + e2e, ruff/ty lints, attribution, uv.lock, and the amd64 image build — and this PR only touches Python.

Root cause (in .github/workflows/docker.yml): the build-arm64 job is gated with if: github.repository == 'NousResearch/hermes-agent' ("only run on the upstream repository, not on forks"), but on pull_request events github.repository is always the base repo, so the guard is true for fork PRs too and the job runs. It then performs an unconditional cache-to: type=registry,ref=ghcr.io/nousresearch/hermes-agent:buildcache-arm64, which a fork PR's read-only GITHUB_TOKEN can't write → denied: installation not allowed to Write organization package.

The amd64 job is unaffected because it uses cache-to: type=gha (fork-safe), and the previous docker-publish.yml sidestepped this by making the arm64 PR build cache-from-only. This regressed in the docker-publish.ymldocker.yml rewrite, and it affects every fork PR, not just this one.

Possible fixes:

  • Make the arm64 cache-to conditional on a non-fork PR (restore the old "PR = cache-from only" behavior), e.g. only write when github.event.pull_request.head.repo.full_name == github.repository; or
  • Have the job's fork guard check the head repo (github.event.pull_request.head.repo.fork == false) instead of github.repository.

Happy to open a small workflow PR if that'd help.

starascendin pushed a commit to starascendin/scarf that referenced this pull request Jun 29, 2026
Promote Project to a first-class, fleet-aware entity per the Milestone-1
implementation spec.

- ScarfProject (ScarfCore): canonical Codable record with lenient/additive
  decoding and ISO-8601 dates; deferred tool/skill scoping fields stay empty.
- ProjectStore (ScarfCore): load/save the canonical <project>/.scarf/project.json
  plus the registry index; additive, idempotent, non-destructive derive()
  migration. Extend ProjectEntry with a stable `uuid` (excluded from
  Equatable/Hashable so selection identity stays name+path).
- Invert ProjectAgentContextService.renderBlock to render from ScarfProject;
  SECRET-SAFE / IDEMPOTENT / BOUNDED / NON-FATAL invariants preserved. Cron
  line now matches [proj:<id>] and legacy [tmpl:<id>].
- ProjectCockpitView: a DashboardTab.cockpit tab reusing ProjectSessionsView
  and ProjectKanbanTab plus Context/Cron/Memory/Secrets/Templates panels.
- ProjectScaffolder mints the UUID and writes project.json via ProjectStore.
- Lazy migration wired into ProjectsView.task.

Tool-scoping stays deferred (NousResearch/hermes-agent#45958); mini-apps
are Milestone 2.

Tests: ScarfCore 637 pass (incl. ScarfProjectTests, ProjectStoreTests);
ProjectAgentContextServiceTests 13/13, ProjectScaffolderTests 3/3.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
starascendin pushed a commit to starascendin/scarf that referenced this pull request Jun 29, 2026
…d host (Phase-1 Milestone 2)

First increment of Milestone 2 (Cowork-style mini-apps): the model layer
plus the sandboxed rendering host. The window.scarf bridge, permission
preview sheet, and cockpit integration land in follow-on increments.

- MiniAppManifest + MiniAppPermission (ScarfCore): lenient Codable
  manifest; default-deny permission model (parameterized `query:<kind>`,
  unknown→deny, sensitive-surface flags).
- ScarfProject.miniApps + MiniAppRef — the M1→M2 bridge field. The
  portable record carries only the registry (id + `generated` provenance);
  permission grants are deliberately NOT portable (a clone re-approves
  untrusted/agent-generated content for itself).
- MiniAppService: discovers/loads `.scarf/miniapps/<id>/miniapp.json`
  (forces id = dir name so a manifest can't redirect the scheme handler);
  ProjectStore.derive now populates miniApps.
- MiniAppAssetResolver (ScarfCore, pure + unit-tested): `scarf-miniapp://`
  path containment (rejects traversal escapes), MIME, strict CSP
  (`connect-src 'none'`).
- MiniAppSchemeHandler + MiniAppHostView (Mac): directory-scoped,
  read-only asset server + WKWebView host with navigation locked to the
  scheme.

Tool-scoping stays deferred (NousResearch/hermes-agent#45958).
Tests: 16 new (MiniAppTests 9, MiniAppAssetResolverTests 7); ScarfCore 8/8
full-suite parallel runs green; app builds.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the underlying hardcode is still present on current main at acp_adapter/session.py:619-622, so the feature is still needed. Two correctness gaps need resolution.

Problems

  • The per-session override is used only for the initial session/new construction (acp_adapter/server.py:1140-1141 in this PR). A later /model reconstructs the same session’s agent without a toolsets argument (acp_adapter/server.py:1776-1781 on current main), and SessionState does not retain the override. That silently resets a scoped session to the process/default toolset.
  • The PR forwards args.toolsets in cmd_acp, but current ACP command parsing is in hermes_cli/subcommands/acp.py:14-52, which has no --toolsets option. Thus the documented hermes acp --toolsets ... form remains unparseable after salvage.

Suggested changes

  • Retain the resolved per-session selection in SessionState and forward it through agent rebuilds; add a model-switch regression test.
  • Add the ACP-subcommand argument in build_acp_parser and cover hermes acp --toolsets web,file parsing/forwarding.

This is an automated hermes-sweeper review.

Comment thread acp_adapter/server.py
) -> NewSessionResponse:
state = self.session_manager.create_session(cwd=cwd)
toolsets = self._toolsets_from_new_session_params(kwargs)
state = self.session_manager.create_session(cwd=cwd, toolsets=toolsets)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes the override only to the first agent construction. /model later reconstructs the same session through _make_agent(...) without toolsets (acp_adapter/server.py:1776-1781 on current main), so this session silently reverts to the manager default. Store the resolved selection on SessionState and forward it on rebuilds.

Comment thread hermes_cli/main.py
if toolsets:
if isinstance(toolsets, (list, tuple)):
toolsets = ",".join(str(t) for t in toolsets)
acp_argv.extend(["--toolsets", str(toolsets)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forwarding is not enough for the documented hermes acp --toolsets ... form: current ACP parser construction is hermes_cli/subcommands/acp.py:14-52 and declares no --toolsets flag. Add it there during salvage and cover the post-subcommand spelling.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP: honor --toolsets and add per-session tool scoping (_make_agent hardcodes enabled_toolsets)

3 participants