From 2a6fc783a74d1f8181f69c901873b7289a327155 Mon Sep 17 00:00:00 2001 From: Lafnaps <177272356+Lafnaps@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:37:59 +0300 Subject: [PATCH] fix(rules): report proposal resolution failures instead of pretending success --- app.py | 15 +++-- static/index.html | 2 +- static/rules-panel.js | 18 +++++- tests/test_rule_proposals.py | 104 +++++++++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 tests/test_rule_proposals.py diff --git a/app.py b/app.py index 60a6434e..8ce739cf 100644 --- a/app.py +++ b/app.py @@ -1778,13 +1778,19 @@ async def resolve_rule_proposal(msg_id: int, request: Request): rule_id = meta.get("rule_id") if action == "activate" and rule_id is not None: - rules.activate(int(rule_id)) + if not rules.activate(int(rule_id)): + return JSONResponse( + {"error": "could not activate rule (active limit reached or rule not found)"}, + status_code=409, + ) meta["status"] = "activated" elif action == "draft" and rule_id is not None: - rules.make_draft(int(rule_id)) + if not rules.make_draft(int(rule_id)): + return JSONResponse({"error": "rule not found"}, status_code=409) meta["status"] = "drafted" elif action == "dismiss" and rule_id is not None: - rules.delete(int(rule_id)) + if not rules.delete(int(rule_id)): + return JSONResponse({"error": "rule not found"}, status_code=409) meta["status"] = "dismissed" else: return JSONResponse({"error": "invalid action"}, status_code=400) @@ -1814,7 +1820,8 @@ async def demote_rule_proposal(msg_id: int): meta = msg.get("metadata", {}) rule_id = meta.get("rule_id") if rule_id is not None: - rules.delete(int(rule_id)) + if not rules.delete(int(rule_id)): + return JSONResponse({"error": "rule not found"}, status_code=409) text = meta.get("text", msg.get("text", "")) updated = store.update_message(msg_id, { "type": "chat", diff --git a/static/index.html b/static/index.html index f015bf49..6a57ad61 100644 --- a/static/index.html +++ b/static/index.html @@ -344,7 +344,7 @@

agentchattr

- + diff --git a/static/rules-panel.js b/static/rules-panel.js index 70a8560c..b3a43a6c 100644 --- a/static/rules-panel.js +++ b/static/rules-panel.js @@ -534,25 +534,39 @@ function cancelDeleteRule(id) { async function resolveRuleProposal(msgId, action) { try { - await fetch(`/api/messages/${msgId}/resolve_rule_proposal`, { + const response = await fetch(`/api/messages/${msgId}/resolve_rule_proposal`, { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Session-Token': window.SESSION_TOKEN }, body: JSON.stringify({ action }), }); + if (!response.ok) { + const payload = await response.json().catch(() => ({})); + throw new Error(payload.error || `Request failed (HTTP ${response.status})`); + } } catch (e) { console.error('Failed to resolve rule proposal:', e); + if (typeof showToast === 'function') { + showToast(`Failed to update rule: ${e.message}`, 'error'); + } } } async function dismissRuleProposal(msgId) { // Demote to regular chat message — same as job proposal dismiss try { - await fetch(`/api/messages/${msgId}/demote_rule_proposal`, { + const response = await fetch(`/api/messages/${msgId}/demote_rule_proposal`, { method: 'POST', headers: { 'X-Session-Token': window.SESSION_TOKEN }, }); + if (!response.ok) { + const payload = await response.json().catch(() => ({})); + throw new Error(payload.error || `Request failed (HTTP ${response.status})`); + } } catch (e) { console.error('Failed to dismiss rule proposal:', e); + if (typeof showToast === 'function') { + showToast(`Failed to dismiss rule proposal: ${e.message}`, 'error'); + } } } diff --git a/tests/test_rule_proposals.py b/tests/test_rule_proposals.py new file mode 100644 index 00000000..4a18af3e --- /dev/null +++ b/tests/test_rule_proposals.py @@ -0,0 +1,104 @@ +import asyncio +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + +import app +from rules import MAX_ACTIVE_RULES, RuleStore +from store import MessageStore + + +class FakeRequest: + def __init__(self, headers=None, body=None): + self.headers = headers or {} + self._body = body + + async def json(self): + if self._body is None: + raise ValueError("no body") + return self._body + + +class RuleProposalResolutionTests(unittest.TestCase): + """Resolving a rule proposal must fail loudly when the RuleStore rejects + the action (active limit reached, rule already gone) instead of stamping + the message as resolved anyway.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + + self.store = MessageStore(str(Path(self.tmp.name) / "messages.jsonl")) + self.rules = RuleStore(str(Path(self.tmp.name) / "rules.json")) + + app.store = self.store + app.rules = self.rules + + def _propose(self, text="keep replies short"): + rule = self.rules.propose(text, "claude") + msg = self.store.add( + "claude", + text, + msg_type="rule_proposal", + metadata={"rule_id": rule["id"], "status": "pending", "text": text}, + ) + return rule, msg + + def test_activate_at_active_limit_returns_409_and_keeps_pending(self): + for i in range(MAX_ACTIVE_RULES): + filler = self.rules.propose(f"rule {i}", "claude") + self.rules.activate(filler["id"]) + rule, msg = self._propose() + + resp = asyncio.run( + app.resolve_rule_proposal(msg["id"], FakeRequest(body={"action": "activate"})) + ) + + self.assertEqual(resp.status_code, 409) + current = self.store.get_by_id(msg["id"]) + self.assertEqual(current["metadata"]["status"], "pending") + self.assertEqual(self.rules.get(rule["id"])["status"], "pending") + + def test_activate_unknown_rule_returns_409_and_keeps_pending(self): + rule, msg = self._propose() + self.rules.delete(rule["id"]) + + resp = asyncio.run( + app.resolve_rule_proposal(msg["id"], FakeRequest(body={"action": "activate"})) + ) + + self.assertEqual(resp.status_code, 409) + current = self.store.get_by_id(msg["id"]) + self.assertEqual(current["metadata"]["status"], "pending") + + def test_activate_success_marks_message_activated(self): + rule, msg = self._propose() + + result = asyncio.run( + app.resolve_rule_proposal(msg["id"], FakeRequest(body={"action": "activate"})) + ) + + self.assertEqual(result["metadata"]["status"], "activated") + current = self.store.get_by_id(msg["id"]) + self.assertEqual(current["metadata"]["status"], "activated") + self.assertEqual(self.rules.get(rule["id"])["status"], "active") + + def test_demote_unknown_rule_returns_409_and_keeps_proposal(self): + rule, msg = self._propose() + self.rules.delete(rule["id"]) + + resp = asyncio.run(app.demote_rule_proposal(msg["id"])) + + self.assertEqual(resp.status_code, 409) + current = self.store.get_by_id(msg["id"]) + self.assertEqual(current["type"], "rule_proposal") + self.assertEqual(current["metadata"]["status"], "pending") + + +if __name__ == "__main__": + unittest.main()