fix: guard chat deletion and research idempotency

This commit is contained in:
snowspeeder committed 2026-10-03 08:54:08 -04:00
1 parent 2048b33799
commit fe4192c8bc
2 files changed
+55 -5

No files matched your search

+11 -2
View File
@@ -216,6 +216,14 @@ class ChatStore:
def delete_conversation(self, owner, cid): def delete_conversation(self, owner, cid):
with self.lock, self.db: with self.lock, self.db:
self._row('chat_conversations',owner,cid) self._row('chat_conversations',owner,cid)
if self.db.execute("SELECT 1 FROM chat_runs WHERE owner=? AND conversation_id=? AND status IN ('queued','running','awaiting_approval') LIMIT 1",(owner,cid)).fetchone():
raise ChatConflict('Execution still active')
if self.db.execute("SELECT 1 FROM sqlite_master WHERE type='table' AND name='chat_transport'").fetchone():
for row in self.db.execute('SELECT metadata FROM chat_transport WHERE owner=? AND conversation_id=?',(owner,cid)):
metadata=json.loads(row['metadata'])
if metadata.get('upstream_id') and not metadata.get('remote_terminal'):
raise ChatConflict('Remote execution still active')
self.db.execute('DELETE FROM chat_transport WHERE owner=? AND conversation_id=?',(owner,cid))
self.db.execute('DELETE FROM chat_conversations WHERE id=? AND owner=?',(cid,owner)) self.db.execute('DELETE FROM chat_conversations WHERE id=? AND owner=?',(cid,owner))
self._change(owner,'conversation',cid) self._change(owner,'conversation',cid)
return {'ok': True} return {'ok': True}
@@ -290,14 +298,15 @@ class ChatStore:
self.db.execute('INSERT INTO chat_messages(id,owner,conversation_id,role,content,status,run_id,created_at,position) VALUES(?,?,?,?,?,?,?,?,?)',(identity,owner,cid,role,content,status,run_id,time.time(),position)) self.db.execute('INSERT INTO chat_messages(id,owner,conversation_id,role,content,status,run_id,created_at,position) VALUES(?,?,?,?,?,?,?,?,?)',(identity,owner,cid,role,content,status,run_id,time.time(),position))
return self._message(self._row('chat_messages',owner,identity)) return self._message(self._row('chat_messages',owner,identity))
def send(self, owner, cid, content, key, attachment_ids=None): def send(self, owner, cid, content, key, attachment_ids=None, mode='normal'):
"""Atomic admission; replay returns same IDs and never creates new work.""" """Atomic admission; replay returns same IDs and never creates new work."""
text(content,'content',32000); text(key,'idempotency key',200) text(content,'content',32000); text(key,'idempotency key',200)
if attachment_ids is None: attachment_ids=[] if attachment_ids is None: attachment_ids=[]
if not isinstance(attachment_ids,list) or len(attachment_ids)>8: raise ValueError('Invalid attachments') if not isinstance(attachment_ids,list) or len(attachment_ids)>8: raise ValueError('Invalid attachments')
for attachment_id in attachment_ids: text(attachment_id,'attachment id',128) for attachment_id in attachment_ids: text(attachment_id,'attachment id',128)
if len(set(attachment_ids))!=len(attachment_ids): raise ValueError('Duplicate attachments') if len(set(attachment_ids))!=len(attachment_ids): raise ValueError('Duplicate attachments')
payload=json.dumps({'content':content,'attachment_ids':attachment_ids},sort_keys=True) if not isinstance(mode,str) or mode not in ('normal','research'): raise ValueError('Invalid mode')
payload=json.dumps({'content':content,'attachment_ids':attachment_ids,'mode':mode},sort_keys=True)
with self.lock,self.db: with self.lock,self.db:
self._row('chat_conversations',owner,cid) self._row('chat_conversations',owner,cid)
prior=self.db.execute('SELECT * FROM chat_idempotency WHERE owner=? AND conversation_id=? AND key=?',(owner,cid,key)).fetchone() prior=self.db.execute('SELECT * FROM chat_idempotency WHERE owner=? AND conversation_id=? AND key=?',(owner,cid,key)).fetchone()
+44 -3
View File
@@ -48,9 +48,9 @@ def test_preferences_and_validation(setup):
with pytest.raises(ChatConflict): s.set_preferences(a, 0, 'opal', 'light') with pytest.raises(ChatConflict): s.set_preferences(a, 0, 'opal', 'light')
assert s.get_preferences(b)['palette']=='opal' assert s.get_preferences(b)['palette']=='opal'
invalid = [lambda:s.create_conversation(a, title=''), lambda:s.create_conversation(a, temporary=1), invalid = [lambda:s.create_conversation(a, title=''), lambda:s.create_conversation(a, temporary=1),
lambda:s.create_workspace(a, 'x', instructions=7), lambda:s.list_conversations(a, limit=True), lambda:s.create_conversation(a, temporary=True), lambda:s.create_workspace(a, 'x', instructions=7),
lambda:s.list_conversations(a, cursor='-1'), lambda:s.set_preferences(a, True, 'opal', 'system'), lambda:s.list_conversations(a, limit=True), lambda:s.list_conversations(a, cursor='-1'),
lambda:s.set_preferences(a, 1, [], 'dark')] lambda:s.set_preferences(a, True, 'opal', 'system'), lambda:s.set_preferences(a, 1, [], 'dark')]
for call in invalid: for call in invalid:
with pytest.raises(ValueError): call() with pytest.raises(ValueError): call()
@@ -64,6 +64,7 @@ def test_send_durable_idempotency_concurrent_claim_and_fk(setup):
assert len({x['run']['id'] for x in replies})==1 assert len({x['run']['id'] for x in replies})==1
assert len(s.get_conversation(a,cid)['messages'])==1 assert len(s.get_conversation(a,cid)['messages'])==1
with pytest.raises(ChatConflict): s.send(a,cid,'different','unique-key') with pytest.raises(ChatConflict): s.send(a,cid,'different','unique-key')
with pytest.raises(ChatConflict): s.send(a,cid,'hello','unique-key',mode='research')
with pytest.raises(KeyError): s.send(b,cid,'hello','unique-key') with pytest.raises(KeyError): s.send(b,cid,'hello','unique-key')
with pytest.raises(KeyError): s.send(a,cid,'hello','attachments',['fake']) with pytest.raises(KeyError): s.send(a,cid,'hello','attachments',['fake'])
s2 = ChatStore(accounts) s2 = ChatStore(accounts)
@@ -74,6 +75,8 @@ def test_send_durable_idempotency_concurrent_claim_and_fk(setup):
other = s.create_conversation(b)['id'] other = s.create_conversation(b)['id']
with pytest.raises(sqlite3.IntegrityError), accounts.db: with pytest.raises(sqlite3.IntegrityError), accounts.db:
accounts.db.execute('UPDATE chat_conversations SET workspace_id=? WHERE id=?',('missing',other)) accounts.db.execute('UPDATE chat_conversations SET workspace_id=? WHERE id=?',('missing',other))
with pytest.raises(ChatConflict): s.delete_conversation(a,cid)
s.update_run(a,rid,'cancelled')
s.delete_conversation(a,cid) s.delete_conversation(a,cid)
assert accounts.db.execute('SELECT count(*) FROM chat_runs WHERE id=?',(rid,)).fetchone()[0]==0 assert accounts.db.execute('SELECT count(*) FROM chat_runs WHERE id=?',(rid,)).fetchone()[0]==0
assert accounts.db.execute('PRAGMA foreign_key_check').fetchall()==[] assert accounts.db.execute('PRAGMA foreign_key_check').fetchall()==[]
@@ -138,6 +141,7 @@ def test_pagination_search_and_revocation(setup):
assert second['next_cursor'] is None assert second['next_cursor'] is None
s.add_message(a,c1,'user','1'); s.add_message(a,c1,'assistant','2') s.add_message(a,c1,'user','1'); s.add_message(a,c1,'assistant','2')
page = s.get_conversation(a,c1,limit=1) page = s.get_conversation(a,c1,limit=1)
assert page['messages'][0]['content']=='2'
assert s.get_conversation(a,c1,cursor=page['next_cursor'],limit=1)['messages'][0]['content']=='1' assert s.get_conversation(a,c1,cursor=page['next_cursor'],limit=1)['messages'][0]['content']=='1'
accounts.begin_delete(b) accounts.begin_delete(b)
with pytest.raises(KeyError): s.get_preferences(b) with pytest.raises(KeyError): s.get_preferences(b)
@@ -159,3 +163,40 @@ def test_disk_reopen_preserves_admission_and_events(tmp_path):
assert store.send(owner,cid,'durable','key')==admitted assert store.send(owner,cid,'durable','key')==admitted
assert store.events(owner,admitted['run']['id'])==[event] assert store.events(owner,admitted['run']['id'])==[event]
accounts.close() accounts.close()
def test_attachments_owned_persisted_and_copied(setup):
accounts,s,a,b=setup
with accounts.db:
accounts.db.execute('CREATE TABLE hope_attachments(id TEXT PRIMARY KEY, owner TEXT, deleted_at REAL)')
accounts.db.execute('INSERT INTO hope_attachments VALUES(?,?,NULL)',('owned',a))
accounts.db.execute('INSERT INTO hope_attachments VALUES(?,?,NULL)',('foreign',b))
cid=s.create_conversation(a)['id']
sent=s.send(a,cid,'file','key',['owned'])
assert sent['message']['attachments']==['owned']
assert s.send(a,cid,'file','key',['owned'])['message']['attachments']==['owned']
with pytest.raises(KeyError): s.send(a,cid,'file','other',['foreign'])
with pytest.raises(ValueError): s.send(a,cid,'file','dup',['owned','owned'])
branched=s.branch(a,cid,sent['message']['id'])
assert s.get_conversation(a,branched['id'])['messages'][0]['attachments']==['owned']
def test_delete_remote_guard_and_final_sync(setup):
import json
accounts,s,a,_=setup
cid=s.create_conversation(a)['id']
sent=s.send(a,cid,'hello','key')
rid=sent['run']['id']
s.update_run(a,rid,'running')
cursor=s.sync(a)['cursor']
m=s.add_message(a,cid,'assistant','done',run_id=rid)
assert s.sync(a,cursor)['changes'][-1]['id']==cid
s.update_run(a,rid,'completed')
with accounts.db:
accounts.db.execute('CREATE TABLE chat_transport(run_id TEXT PRIMARY KEY,owner TEXT,conversation_id TEXT,metadata TEXT)')
accounts.db.execute('INSERT INTO chat_transport VALUES(?,?,?,?)',(rid,a,cid,json.dumps({'upstream_id':'remote'})))
with pytest.raises(ChatConflict): s.delete_conversation(a,cid)
with accounts.db:
accounts.db.execute('UPDATE chat_transport SET metadata=?',(json.dumps({'upstream_id':'remote','remote_terminal':True}),))
s.delete_conversation(a,cid)
assert accounts.db.execute('SELECT count(*) FROM chat_transport').fetchone()[0]==0