mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
fix(gateway): answer viewer reads on a history mount; name read-only refusals
Addresses maintainer review on #143601. Refs #143547
This commit is contained in:
1 parent
bc629f94e0
commit
690440524b
15 files changed
+83
-13
No files matched your search
@@ -1,7 +1,9 @@
|
||||
import type { AppendMessage } from '@assistant-ui/react'
|
||||
import { JsonRpcGatewayError } from '@hermes/shared'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { ChatMessage } from '@/lib/chat-messages'
|
||||
import { errorRecoveryPlan } from '@/lib/error-surface'
|
||||
|
||||
import {
|
||||
acquireSubmitInFlight,
|
||||
@@ -20,6 +22,7 @@ import {
|
||||
isSubmitInFlight,
|
||||
isTargetSessionBusy,
|
||||
markSessionRecentlyInterrupted,
|
||||
ownershipErrorSurface,
|
||||
readFileDataUrlForAttach,
|
||||
RECENT_INTERRUPT_COOLDOWN_MS,
|
||||
releaseSubmitInFlight,
|
||||
@@ -139,6 +142,16 @@ describe('session error classifiers', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('ownershipErrorSurface', () => {
|
||||
it('drops Retry for a read-only stored chat: resending reproduces the refusal', () => {
|
||||
const err = new JsonRpcGatewayError('session_read_only', { code: 4001, data: { reason: 'session_read_only' } })
|
||||
const { errorSurface } = ownershipErrorSurface(err)
|
||||
|
||||
expect(errorSurface).toEqual({ layer: 'gateway', code: 'session_read_only', retryable: false })
|
||||
expect(errorRecoveryPlan(errorSurface)).toMatchObject({ retry: false, startNewSession: true })
|
||||
})
|
||||
})
|
||||
|
||||
describe('withSessionNotFoundResume', () => {
|
||||
const STORED = 'stored-1'
|
||||
const DEAD = 'rt-dead'
|
||||
|
||||
@@ -336,12 +336,17 @@ function gatewayReason(error: unknown): unknown {
|
||||
|
||||
// The error card's surface for an ownership refusal: another surface owns the session (#106217,
|
||||
// deterministic: drop Retry, offer a new session), or the gateway owner is mid-turn and could not
|
||||
// queue the send (#140385, transient: Retry once the turn ends). Empty for any other error.
|
||||
// queue the send (#140385, transient: Retry once the turn ends), or the chat is stored history
|
||||
// nothing can continue (deterministic). Empty for any other error.
|
||||
export function ownershipErrorSurface(error: unknown): { errorSurface?: ErrorSurface } {
|
||||
if (isSessionNotOwnedError(error)) {
|
||||
return { errorSurface: { layer: 'gateway', code: 'SESSION_NOT_OWNED', retryable: false } }
|
||||
}
|
||||
|
||||
if (gatewayReason(error) === 'session_read_only') {
|
||||
return { errorSurface: { layer: 'gateway', code: 'session_read_only', retryable: false } }
|
||||
}
|
||||
|
||||
if (gatewayReason(error) === 'SESSION_OWNER_UNCONFIRMED') {
|
||||
// The owner may already hold the send: a Retry would run a second copy.
|
||||
return { errorSurface: { layer: 'gateway', code: 'SESSION_OWNER_UNCONFIRMED', retryable: false } }
|
||||
|
||||
@@ -108,6 +108,10 @@ export const enErrorCodes: ThreadCopy['errorCodes'] = {
|
||||
title: 'This chat belongs to a scheduled job',
|
||||
body: 'It only runs that job, so your message was not run. Start a new chat to send it.'
|
||||
},
|
||||
session_read_only: {
|
||||
title: 'This chat is read-only',
|
||||
body: 'It is stored history Hermes can show but not continue, so your message was not run. Start a new chat to send it.'
|
||||
},
|
||||
workspace_unavailable: {
|
||||
title: "This chat's folder is gone",
|
||||
body: 'The folder this chat works in was moved or deleted, so your message was not run. Recreate the folder, or use Move to project to give the chat another one, then retry.'
|
||||
|
||||
@@ -53,6 +53,9 @@ export const ERROR_CODE_KEYS = [
|
||||
'SESSION_OWNER_BUSY',
|
||||
// The chat is a scheduled job's session: it only runs that job, so a typed message never runs there.
|
||||
'cron_session_read_only',
|
||||
// Stored history the gateway shows but cannot continue (an old cron run, subagent, imported or
|
||||
// WebUI chat): prompt.submit refuses it with this reason.
|
||||
'session_read_only',
|
||||
// The chat's working folder was moved or deleted: nothing ran, and nothing will until it is back
|
||||
// or the chat moves to another folder.
|
||||
'workspace_unavailable',
|
||||
@@ -349,6 +352,7 @@ const CODE_PLANS: Partial<Record<ErrorCodeKey, Partial<ErrorRecoveryPlan>>> = {
|
||||
SESSION_OWNER_BUSY: { retry: true, startNewSession: false },
|
||||
SESSION_OWNER_UNCONFIRMED: { retry: false, startNewSession: false },
|
||||
cron_session_read_only: { retry: false, startNewSession: true },
|
||||
session_read_only: { retry: false, startNewSession: true },
|
||||
// Retry is the next click once the folder is back or the chat was moved (Move to project).
|
||||
workspace_unavailable: { retry: true, startNewSession: true },
|
||||
launch_credentials_unavailable: { retry: false, startNewSession: true },
|
||||
|
||||
@@ -443,7 +443,8 @@ def _owner_objects(authority, ref, *, include_fallback_evicted=False):
|
||||
|
||||
def physical_target(authority, ref):
|
||||
from gateway.config import Platform
|
||||
if authority.sessions[ref.session_id].source.platform == Platform.LOCAL:
|
||||
source = authority.sessions[ref.session_id].source # None: a history mount
|
||||
if source is not None and source.platform == Platform.LOCAL:
|
||||
from hermes_state_local import local_receipt
|
||||
return local_receipt(authority.db, ref.session_id)['entry']['session_id']
|
||||
return ref.session_id
|
||||
|
||||
@@ -456,6 +456,9 @@ async def execute_command(connection, ref, params, *, dispatch=False):
|
||||
if verdict == 'refused' or (verdict == 'control' and canonical in _OWNER_COMMANDS
|
||||
and ref.session_id in connection.subscriptions):
|
||||
raise RuntimeStoreError('unsupported_command', detail=REFUSAL_HINTS.get(canonical))
|
||||
from gateway.session_history_mount import is_history_mount, read_only
|
||||
if is_history_mount(live):
|
||||
raise read_only() # no policy or agent to run even a read command against
|
||||
from gateway.session_local import authorize_local_source
|
||||
if authorize_local_source(authority.runner, live.source) is not True:
|
||||
raise RuntimeStoreError('permission_denied')
|
||||
|
||||
@@ -68,9 +68,13 @@ def require_history_leaf(authority, sid):
|
||||
detail='this stored conversation has child sessions a delete would take with it')
|
||||
|
||||
|
||||
def read_only():
|
||||
return RuntimeStoreError('session_read_only', field='session_id',
|
||||
detail='this stored conversation can be read but not continued; start a new chat')
|
||||
|
||||
|
||||
def require_history_access(authority, actor, sid, capability):
|
||||
if not _reader(authority, actor, sid):
|
||||
raise RuntimeStoreError('not_found')
|
||||
if capability != 'session:read':
|
||||
raise RuntimeStoreError('session_read_only', field='session_id',
|
||||
detail='this stored conversation can be read but not continued; start a new chat')
|
||||
raise read_only()
|
||||
@@ -115,8 +115,8 @@ class LocalSessionAdapter(BasePlatformAdapter):
|
||||
|
||||
|
||||
def authorize_local_source(runner, source):
|
||||
"""None for legacy/nonlocal routes; registered local adapters fail closed."""
|
||||
if source.platform != Platform.LOCAL:
|
||||
"""None for legacy/nonlocal routes and history mounts; registered local adapters fail closed."""
|
||||
if source is None or source.platform != Platform.LOCAL:
|
||||
return None
|
||||
# A forged profile must not miss this adapter and fall into a messaging
|
||||
# allow-all policy. Local source identity is owned by this authority alone.
|
||||
|
||||
@@ -1038,7 +1038,7 @@ def _serving_local_desktop_within(scope):
|
||||
def policy_for_source(runner, source):
|
||||
from gateway.session_local import LocalSessionAdapter
|
||||
from gateway.config import Platform
|
||||
if source.platform != Platform.LOCAL:
|
||||
if source is None or source.platform != Platform.LOCAL: # None: a history mount, which runs nothing
|
||||
return None
|
||||
adapter = runner._adapter_for_source(source)
|
||||
if isinstance(adapter, LocalSessionAdapter) and adapter.authorize_source(source):
|
||||
|
||||
@@ -136,6 +136,22 @@ async def test_a_childless_continuation_whose_chain_has_a_delegate_is_not_delete
|
||||
assert db.get_session('sub-child')['parent_session_id'] == root.session_id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_reads_a_viewer_sends_for_an_open_chat_answer_on_a_mount(tmp_path, monkeypatch):
|
||||
"""Desktop polls the session-control store and loads its slash palette and model picker for the
|
||||
open chat; the mount has no source, and each of these answered ``internal error``."""
|
||||
conn, ref = await _connect(tmp_path, monkeypatch, 'cron_job1_20261001_120000')
|
||||
await conn.resume(ref, {})
|
||||
sid = {'session_id': ref.session_id}
|
||||
for method, params in (('session.control.read', sid), ('commands.catalog', sid),
|
||||
('config.get', {**sid, 'key': 'mtime'}), ('model.options', sid)):
|
||||
reply = await conn.dispatch({'id': 1, 'method': method, 'params': params})
|
||||
assert 'result' in reply, (method, reply)
|
||||
for command in ('fast status', 'model'):
|
||||
reply = await conn.dispatch({'id': 1, 'method': 'slash.exec', 'params': {**sid, 'command': command}})
|
||||
assert reply['error']['message'] == 'session_read_only', (command, reply)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_api_chat_opened_read_only_still_continues_through_the_api(tmp_path, monkeypatch):
|
||||
"""Binding the viewed row replaced nothing: the read-only mount had no source to check."""
|
||||
|
||||
@@ -118,11 +118,14 @@ async def test_http_import_and_delete_keep_atomic_receipts(tmp_path, monkeypatch
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_reports_already_absent_only_for_rows_that_are_gone(tmp_path, monkeypatch):
|
||||
"""M3: the authority also answers ``not_found`` for an EXISTING cold row it cannot restore
|
||||
(an API-server row with no binding). That is not an idempotent success — Desktop would drop
|
||||
the row and it reappears on reload. Only a genuinely absent id is ``already_absent``."""
|
||||
"""M3: the authority also answers ``not_found`` for an EXISTING cold row it will not restore
|
||||
for this caller (an unbound API-server row is mounted for its owner only). That is not an
|
||||
idempotent success — Desktop would drop the row and it reappears on reload. Only a genuinely
|
||||
absent id is ``already_absent``."""
|
||||
from gateway.session_contract import Principal
|
||||
from hermes_cli.web_routers.sessions import manage_router
|
||||
from hermes_cli import web_server as web
|
||||
import hermes_cli.web_server_sessions as web_sessions
|
||||
monkeypatch.setattr(web, '_SESSION_TOKEN', 'fixture-token')
|
||||
monkeypatch.setattr('hermes_state._default_db_path', lambda: tmp_path / 'state.db')
|
||||
app = FastAPI()
|
||||
@@ -131,8 +134,10 @@ async def test_delete_reports_already_absent_only_for_rows_that_are_gone(tmp_pat
|
||||
with SessionDB(db_path=tmp_path / 'state.db') as db:
|
||||
db.create_session('cold-api', source='api_server', chat_id='cold-api', session_key='agent:main:api:cold')
|
||||
epoch = begin_runtime_epoch(db, instance_id='owner')
|
||||
app.state.session_authority = SessionAuthority(SimpleNamespace(_draining=False), profile_id=str(tmp_path),
|
||||
instance_id='owner', db=db, epoch=epoch)
|
||||
authority = SessionAuthority(SimpleNamespace(_draining=False), profile_id=str(tmp_path),
|
||||
instance_id='owner', db=db, epoch=epoch)
|
||||
login = Principal('basic:alice', str(tmp_path), frozenset({'session:read', 'session:control'}), 'http')
|
||||
monkeypatch.setattr(web_sessions, '_session_mutation_context', lambda request, profile: (authority, login))
|
||||
query = {'request_id': 'delete', 'expected_revision': 0, 'expected_generation': 0}
|
||||
async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url='http://localhost',
|
||||
headers={'Authorization': 'Bearer fixture-token'}) as client:
|
||||
|
||||
@@ -141,7 +141,7 @@ def test_session_rename_runs_writer_open_and_update_off_event_loop(monkeypatch,
|
||||
authority = SessionAuthority(SimpleNamespace(_draining=False), profile_id="p", instance_id="owner",
|
||||
db=db, epoch=begin_runtime_epoch(db, instance_id="owner"))
|
||||
authority.sessions["sess-1"] = LiveSession(None, "route")
|
||||
actor = Principal("owner", "p", frozenset({"session:control"}), "http")
|
||||
actor = Principal("owner", "p", frozenset({"session:read", "session:control"}), "http")
|
||||
monkeypatch.setattr(_web_server_sessions, "_open_session_db_for_profile", _open_db)
|
||||
monkeypatch.setattr(_web_server_sessions, "_session_mutation_context", lambda request, profile: (authority, actor))
|
||||
monkeypatch.setattr(_mutations, "mutate_runtime_session", _recording_commit)
|
||||
|
||||
@@ -103,6 +103,14 @@ describe('describeRpcError', () => {
|
||||
expect(text).toContain('/resume')
|
||||
})
|
||||
|
||||
it('explains a read-only stored chat instead of echoing its reason code', () => {
|
||||
const text = describeRpcError(new JsonRpcGatewayError('session_read_only', { code: 4001 }))
|
||||
|
||||
expect(text).not.toContain('session_read_only')
|
||||
expect(text).toContain('/new')
|
||||
expect(describeRpcError(new JsonRpcGatewayError('session_has_children', { code: 4001 }))).toMatch(/not deleted/)
|
||||
})
|
||||
|
||||
it('passes ordinary domain errors through unchanged', () => {
|
||||
expect(describeRpcError(new JsonRpcGatewayError('hash required', { code: 4014 }))).toBe('hash required')
|
||||
})
|
||||
|
||||
@@ -135,6 +135,9 @@ const RPC_ERROR_ROWS: RpcErrorRow[] = [
|
||||
(code, text) => (code === RPC_SESSION_NOT_FOUND || code === undefined) && SESSION_NOT_FOUND_RE.test(text),
|
||||
() => t('userMessages.rpc.sessionNotFound')
|
||||
],
|
||||
// A stored chat the gateway opens read-only (gateway/session_history_mount.py) names its refusal by reason.
|
||||
[(_code, text) => text === 'session_read_only', () => t('userMessages.rpc.sessionReadOnly')],
|
||||
[(_code, text) => text === 'session_has_children', () => t('userMessages.rpc.sessionHasChildren')],
|
||||
[(_code, text) => NOT_CONNECTED_RE.test(text), () => t('userMessages.rpc.notConnected')],
|
||||
[(_code, text) => text.trim() === 'launch_credentials_unavailable', () => t('userMessages.rpc.launchCredentialsChanged')],
|
||||
[(_code, text) => TIMED_OUT_RE.exec(text), m => t('userMessages.rpc.timedOut', m?.[1] ?? '?')]
|
||||
|
||||
@@ -50,6 +50,10 @@ export const userMessagesEn = {
|
||||
/** The owner refused a control on a chat whose launch API key changed or was removed. */
|
||||
launchCredentialsChanged:
|
||||
"This chat can't use the API key it was started with any more (it changed or was removed), so that was not done. A chat never switches to a different key on its own. Type /new to start a new chat on your current key.",
|
||||
sessionReadOnly:
|
||||
'This chat is stored history Hermes can show but not continue, so your message was not run. Use /new to start a new chat.',
|
||||
sessionHasChildren:
|
||||
'This chat has child sessions (such as subagent runs) that deleting it would remove too, so it was not deleted.',
|
||||
notConnected:
|
||||
'Hermes is not connected right now, so that was not sent. It reconnects automatically; wait a moment and try again, or type /logs if this persists.',
|
||||
/** `{0}` = timeout in seconds. */
|
||||
|
||||
Reference in new issue
Block a user