feat(gateway): add workspace command for messaging sessions - #42577
feat(gateway): add workspace command for messaging sessions#42577qWaitCrypto wants to merge 4 commits into
Conversation
3f76553 to
28a6d2e
Compare
Code Review SummaryVerdict: Clean — no issues found OverviewThis PR adds a Key design decisions
Quality
No concerns
Reviewed by Hermes Agent (cron code-review-lite) |
|
In this pr, hermes showed stronger debugging capabilities than codex and claudecode.😂 |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the existing per-session cwd seam. The missing gateway command is still a valid gap: current main's registry/dispatch has no /workspace or /cwd (hermes_cli/commands.py:123-126, gateway/run.py:9876-9880), while set_session_vars(..., cwd=...) already supports the intended runtime handoff (gateway/session_context.py:157-214).
Problems
- The submitted handler uses synchronous
self.session_storeandself._session_dbcalls. Current main requires the async facades:AsyncSessionStoreoffloads store I/O (gateway/session.py:969-983) andGatewayRunnercreates_session_dbasAsyncSessionDB(gateway/run.py:3040-3044). Its forwardedget_session/update_session_cwdcalls are coroutines (hermes_state.py:6691-6704), so the submitted direct calls will not persist cwd. - Clearing only
cwdis incomplete for current workspace grouping:workspace_key()prefersgit_repo_root(hermes_state.py:35-49).
Suggested changes
- Port the command to
await self.async_session_storeandawait self._session_db, then implement an atomic clear for cwd plus stale git metadata. - Rebuild the terminal/file-tool portion from current main rather than applying the stale snapshots.
Automated hermes-sweeper review.
|
|
||
| return t("gateway.set_home.success", name=chat_name, chat_id=chat_id) | ||
|
|
||
| async def _handle_workspace_command(self, event: MessageEvent) -> str: |
There was a problem hiding this comment.
Current gateway handlers must use await self.async_session_store.get_or_create_session(source). session_store is synchronous, and the async facade exists to keep its I/O off the event loop (gateway/session.py:969-983).
| @@ -47,6 +47,86 @@ | |||
| class GatewaySlashCommandsMixin: | |||
| """In-session slash-command handlers for GatewayRunner.""" | |||
|
|
|||
There was a problem hiding this comment.
self._session_db is now an AsyncSessionDB; get_session() returns a coroutine and this helper will catch the resulting attribute error and return an empty cwd. Make this helper async and await the DB call.
| source=self._workspace_source_label(session_entry.session_id), | ||
| global_cwd=global_cwd, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This write is un-awaited under current main's AsyncSessionDB, so /workspace clear reports success without persisting. Use an awaited clear operation that also handles stale git_branch/git_repo_root metadata.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the missing messaging-gateway workspace control through different models: #42577 binds an absolute per-session cwd using the existing session/runtime cwd plumbing, while #62075 adds a named workspace registry with create, list, switch, and remove operations. Both cover the core /workspace command, but each diff currently conflicts with current-main APIs or cwd semantics identified in contributor reviews.
Related pull requests
- #42577
related— (+584/-36) — author action required: #42577 is the narrower implementation, adding/workspaceand/cwd, persisted session cwd, session-ID carry, runtime/tool propagation, and isolation tests. Despite the keep_open review on #42577, its diff calls the now-async session store and DB facades synchronously, so cwd persistence and carry cannot work as submitted; it also clearscwdwithout addressing thegit_repo_rootprecedence documented in that review. - #62075
duplicate— (+276/-2) — salvageable broader alternative: #62075 adds a persistent named-workspace registry, per-session selection, routing-state persistence, documentation, and tests. The keep_open review on #62075 identifies blocking diff-level issues: addingenv_typeturns a CWD-only override into an isolation signal, host paths are not mapped for container backends, and the branch's session-store calls do not match current main, including the concurrency state referenced by commit b3f77f5.
Duplicates
#42577 and #62075 overlap on registering and dispatching a messaging-gateway /workspace command and applying a per-session cwd, but they are not complete duplicates: #62075 additionally implements named workspace registration and lifecycle management, whereas #42577 focuses on direct absolute-path binding and deeper terminal/file-tool cwd isolation.
Suggested consolidation
Author action: rebase #42577 onto main and adapt its focused command to the async session/DB facades, including correct clearing of both cwd and the higher-priority workspace grouping state; alternatively, split out that corrected command and its isolation tests as the salvageable core. Keep #62075 open only with a salvage path for its named registry, routing-state persistence, documentation, and non-destructive removal behavior after removing the unintended env_type isolation and defining tested host-to-container path mapping. Do not close either as a duplicate yet because their retained scopes differ, and do not merge either as submitted because the blocking concerns in both keep_open reviews remain visible in the diffs.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup42577 ["PRs duplicating each other"]
P42577["PR #42577 (open)"]
P62075["PR #62075 (open)"]
end
class P42577 open
class P62075 open
class P42577 target
click P42577 "https://github.com/NousResearch/hermes-agent/pull/42577"
click P62075 "https://github.com/NousResearch/hermes-agent/pull/62075"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 58 kB of PR diffs, 7 kB of issue/PR text, 4 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Resolve async session persistence, atomic workspace metadata updates, and current terminal cwd integration from review feedback.
What does this PR do?
Adds a messaging-gateway
/workspacecommand, with/cwdas an alias, soTelegram/Discord/Slack-style gateway sessions can bind the current chat/thread
to a project directory.
This is the small missing user-facing piece from the gateway workspace design:
main already has the canonical per-session cwd plumbing, but messaging gateway
users did not have a slash command to set it.
Background
Related issue: #37277
Earlier PR: #37275
The use case from #37277 is one gateway bot serving multiple long-lived chats
or threads, where different sessions may be working on different repositories.
The old PR (#37275) implemented a broader workspace stack, but it was closed
because much of that machinery now exists on
main:agent/runtime_cwd.pyalready has session-scoped cwd resolution.gateway/session_context.set_session_vars(..., cwd=...)already forwards cwdinto the canonical session cwd contextvar.
session.cwd.set.This PR rebuilds only the worthwhile missing slice against current
main: amessaging gateway command that uses the existing cwd infrastructure instead of
introducing a parallel workspace model.
Changes made
/workspaceand/cwdto the central slash-command registry asgateway-only commands.
/workspacehandling in the messaging gateway:/workspaceor/workspace statusshows the effective workspace./workspace /absolute/pathvalidates and binds the current gateway sessionto that directory.
/workspace clearremoves the session override and returns to the globalgateway cwd.
SessionDB.sessions.cwd.SessionDB.update_session_cwd(session_id, "")to clear an existingcwd.
the existing
set_session_vars(cwd=...)seam.matching the existing TUI/ACP pattern.
workspace changes, so future turns rebuild against the selected cwd.
persistence, runtime cwd injection, and clearing stored cwd.
Scope
This PR intentionally does not reimplement the old workspace machinery from
#37275. It does not add a new
workspace_cwdfield, a separate workspace store,or a parallel cwd resolver.
The implementation is a thin command layer over existing main-branch plumbing:
Terminal cwd is also registered under the current
session_id, because thegateway agent run passes that id as the tool
task_id.How to test
Focused syntax/check validation run locally:
Both passed.
I did not run the full pytest suite in this checkout because the local virtual
environment does not have
pytestinstalled.Manual behavior to verify in a messaging gateway:
/workspace /absolute/path/to/project.project cwd.
/workspaceor/cwdto show the bound workspace./workspace clearand verify the session returns to the global gatewaycwd.
Checklist
Code
main.feat(gateway): add session workspace binding #37275.
Documentation & Housekeeping
messaging gateway command path.