diff --git a/docs/auth.md b/docs/auth.md index ea33b82..1701f77 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -136,7 +136,8 @@ acquiring any permission. The rule lives in one predicate, `admin_only_blocked(profile, role)` in `navi/profiles/base.py`, consulted by every place a profile can be reached — the profile list, session creation, `switch_profile`, `list_profiles`, `spawn_agent`, the -"Available profiles" block of the system prompt, and the Synapse reaction router. An +"Available profiles" block of the system prompt, and the Synapse reaction router — which +asks it with the role of the account the event was delivered for, not with a default. An unknown or absent role is treated as non-admin. Admins are unaffected: for `role == "admin"` the predicate is always `False`. diff --git a/docs/profiles.md b/docs/profiles.md index 291213e..d36ad0e 100644 --- a/docs/profiles.md +++ b/docs/profiles.md @@ -205,7 +205,12 @@ tool surface a user actually has is the **union** of the three. The native sets are kept identical for exactly that reason. - The reaction dispatcher routes a Synapse event into one of the profiles its triggering - user may use, so a regular user's reactions run in one of these three. + user may use, so a regular user's reactions run in one of these three. The role is read + from the account the event was delivered for, so an admin's reaction may reach an + admin-only profile — with that profile's full tool set, `tgclient` and `ssh_exec` + included. An admin owns the delivery target and writes its routing rules, so that is + what they asked for; the reaction document is where the guardrails on third-party + content belong. #### Accepted residual risk diff --git a/docs/synapse.md b/docs/synapse.md index 7c997e5..b8237fe 100644 --- a/docs/synapse.md +++ b/docs/synapse.md @@ -48,16 +48,26 @@ Input: the user's **dispatcher instructions** (routing — which kind of event goes to which profile, and what counts as one conversation, i.e. how to derive the conversation key), the user's **reaction instructions** (what to - do), and the event envelope. Output: strict JSON + do), the event envelope, and the list of profiles the run may reach, each as + `id (name) — description`. The names are not decoration: a routing document says + "the sysadmin profile", and an id alone gives the model nothing to match that + against. Output: strict JSON `{profile_id, task, understood, session_key}` or `{skip: true}`. - `session_key` is `null` when the event opens a new thread; unparseable output - or a backend failure = skip (logged, never a crash). -3. **Profile resolution** — only a profile the triggering user may actually use can host a - reaction. Hidden profiles, and profiles marked `is_admin_only` (a reaction run carries - role `user`, so landing on one would hand that user a wider tool surface than their own - profile list offers), fall through to the first user-visible profile. The configured - fallback `secretary` is admin-only, so for a regular user it is never the answer. - The check runs on every event, continuation included. + `session_key` is folded to one spelling (whitespace collapsed, lowercased) so a + chat cannot fork on `TG:42` vs `tg:42`; it is `null` when the event opens a new + thread. Unparseable output or a backend failure = skip (logged, never a crash). +3. **Profile resolution** — the profile that hosts the reaction is the dispatcher's + choice, subject to the one predicate the whole app uses, `admin_only_blocked`, + asked with **the triggering account's role** read from `navi_users.role` — the same + value the WebSocket, `POST /sessions`, `switch_profile` and `list_profiles` ask it + with. An admin's event may therefore reach an admin-only profile and its full tool + set (`tgclient`, `ssh_exec`): the reaction was fired by that one account, not by an + anonymous caller. A regular user's event may not. Hidden profiles are out for + everyone. A refused id is never swapped in silence — it falls to the fallback for + that role (`secretary` for an admin, `assistant` for a regular user, instead of + "whichever `all()` yields first") and the substitution is logged as + `synapse.reaction_profile_downgraded` with both ids. The check runs on every event, + continuation included. 4. **Session — fresh or continued** — with a non-empty `session_key` and a positive continuation window (`reaction_session_ttl_minutes`), the runner looks for the newest `special=True` session of that user whose diff --git a/manuals/synapse_instructions.md b/manuals/synapse_instructions.md index 88276a8..46262aa 100644 --- a/manuals/synapse_instructions.md +++ b/manuals/synapse_instructions.md @@ -17,6 +17,14 @@ reaction detail in `dispatcher` makes the router guess. Put a rule where its reader is. +One rule travels the other way: **what counts as one conversation** (the key that +keeps a Telegram chat in one session) is read from whichever document states it, so a +rule left in `reaction` is still honoured — but `dispatcher` is where it belongs, and +it is the document that also has to say which profile handles the event. A profile +named there is matched by name as well as by id, and it has to be one the *delivery +account's role* may use: an admin's events may be routed to an admin-only profile, a +regular user's may not, and a refusal is logged rather than applied silently. + ## Parameters | Parameter | Required | Description | diff --git a/navi/profiles/dispatcher/system_prompt.txt b/navi/profiles/dispatcher/system_prompt.txt index 36378bd..e8ad445 100644 --- a/navi/profiles/dispatcher/system_prompt.txt +++ b/navi/profiles/dispatcher/system_prompt.txt @@ -28,20 +28,26 @@ payload facts worth keeping, and what the instructions say to do about it. - Do NOT include anything the reaction agent cannot verify itself (no assumptions, no made-up data). The payload is the ground truth. -- "profile_id" must be one of the listed profile ids, and the dispatcher - instructions decide it: when they name a profile for this kind of event, take - that one even if another looks more natural; when they name none, pick the - profile whose purpose fits the event best, and fall back to "secretary" only - when nothing fits. +- "profile_id" must be one of the listed profile ids. The dispatcher instructions + decide it: when they name a profile for this kind of event, take that one even if + another looks more natural — they will usually name it the way their author speaks + ("the sysadmin profile"), not by id, so match against the name and the description + as well. When they name none, pick the profile whose purpose fits the event best. + When nothing fits, take the first profile in the list. +- If the instructions name a profile that is not in the list, route to the closest + profile that is and say so at the end of "understood" — one short clause naming the + missing profile. Never answer with an id that is not in the list. - "session_key" names the conversation, so that a later event of the same conversation joins the session this one opens: - - derive it from values actually present in the payload, following the - dispatcher instructions (e.g. for Telegram, the chat or thread id); - - the same conversation must always produce exactly the same string, so keep it - short and literal — a stable id, optionally prefixed with the source (e.g. - "tg:42"); - - use null when the event belongs to no conversation, when the instructions say - nothing about conversations, or when the payload holds no stable + - derive it from values actually present in the payload, following whichever of the + user's two documents speaks about conversations (e.g. for Telegram, the chat or + thread id); + - the same conversation must always produce exactly the same string: lowercase, no + spaces, short and literal — a stable id, optionally prefixed with the source + (e.g. "tg:42"). Two spellings of one chat are two sessions, so pick one spelling + and keep to it whatever the event says; + - use null only when neither document says anything about conversations, when the + event belongs to no conversation, or when the payload holds no stable conversation id. Never invent one, and never key on the event id — every event has its own. - If the instructions say to IGNORE events of this kind, return exactly: diff --git a/navi/synapse/reactions.py b/navi/synapse/reactions.py index 425a983..4fd9646 100644 --- a/navi/synapse/reactions.py +++ b/navi/synapse/reactions.py @@ -37,13 +37,19 @@ import structlog from navi.config import settings +from navi.exceptions import ProfileNotFound from navi.llm.base import Message from navi.profiles.base import admin_only_blocked log = structlog.get_logger() _DISPATCHER_PROFILE_ID = "dispatcher" -_FALLBACK_PROFILE_ID = "secretary" + +# The profile a reaction falls back to when the dispatcher names one the run cannot +# use, per the role the run carries. Role-aware on purpose: `secretary` is itself +# admin-only, so for a regular user the documented fallback was a second refusal in a +# row and the event landed on whichever profile `all()` happened to yield first. +_FALLBACK_PROFILE_IDS = {"admin": "secretary", "user": "assistant"} # How many past events a continued thread remembers in its metadata. The trail is # for the reader of the session row, not for the model, so a short tail is enough. @@ -95,20 +101,26 @@ from navi.synapse.settings_store import SynapseSettingsStore session_store = get_session_store() - settings_row = await SynapseSettingsStore(await _pool_of(session_store)).get(user_id) + pool = await _pool_of(session_store) + settings_row = await SynapseSettingsStore(pool).get(user_id) if not settings_row.reactions_enabled: log.info("synapse.reaction_disabled", event_id=envelope.get("event_id"), user_id=user_id) return + # The role the run inherits, read from the account the event was delivered for. + # It decides which profiles the dispatcher is even offered, so a wrong answer here + # is invisible until someone notices their instruction was not honoured. + role = await _user_role(pool, user_id) + profile_id, task_text, understood, session_key = await _dispatch( - envelope, settings_row.instructions, settings_row.dispatcher_instructions + envelope, settings_row.instructions, settings_row.dispatcher_instructions, role=role ) if profile_id is None: log.info("synapse.reaction_skipped", event_id=envelope.get("event_id"), user_id=user_id) return profiles = get_profile_registry() - profile = _resolve_profile(profiles, profile_id) + profile = _resolve_profile(profiles, profile_id, role=role) task_text = (task_text or "").strip() if not task_text: log.warning("synapse.reaction_empty_task", event_id=envelope.get("event_id")) @@ -142,6 +154,7 @@ session_store, user_id, instructions=settings_row.instructions, + role=role, ) errors: list[str] = [] @@ -169,6 +182,30 @@ ) +async def _user_role(pool, user_id: str) -> str: + """The role a reaction of this account runs under. + + The same `navi_users.role` the rest of the app reads before it asks + `admin_only_blocked` — the WebSocket (`user.role`), `POST /sessions`, + `switch_profile`, `list_profiles`. A reaction is triggered by one known account, + so it inherits that account's role instead of a default; the gate then means the + same thing here as everywhere else. + + Falling back to "user" is deliberate: an unreadable role must never *widen* a run, + and the failure is logged rather than swallowed. + """ + try: + row = await pool.fetchrow("SELECT role FROM navi_users WHERE id = $1", user_id) + except Exception: + log.exception("synapse.reaction_role_lookup_failed", user_id=user_id) + return "user" + try: + role = row["role"] + except (KeyError, IndexError, TypeError): + return "user" + return role if isinstance(role, str) and role else "user" + + async def _open_thread( session_store, orchestrator, @@ -300,6 +337,7 @@ user_id: str, *, instructions: str = "", + role: str = "user", ): """Start the agent run with the target user's tool-sandbox context. @@ -307,6 +345,12 @@ ContextVar: ContextBuilder folds it into the system prompt of every request the run makes, so the model always has it while the transcript and session_messages never do. create_task snapshots the context, so sub-agents inherit it too. + + `role` is the triggering account's role, so the run asks the same questions of + `admin_only_blocked` and of the per-tool role checks that account's own sessions + ask. The reaction is fired by one known account, not by an anonymous caller; a + hardcoded "user" here cut every admin's own events down to a user tool surface, + which is how a route to an admin profile lost the tools it was routed for. """ from navi.tools._internal.base import ( current_reaction_instructions, @@ -316,7 +360,7 @@ ) current_user_info.set(None) - role_token = current_user_role.set("user") + role_token = current_user_role.set(role) uid_token = current_user_id.set(user_id) instr_token = current_reaction_instructions.set(instructions or None) try: @@ -333,7 +377,7 @@ async def _dispatch( - envelope: dict, instructions: str, routing_instructions: str = "", + envelope: dict, instructions: str, routing_instructions: str = "", role: str = "user", ) -> tuple[str | None, str, str, str | None]: """One-shot dispatcher call. @@ -350,17 +394,17 @@ log.exception("synapse.reaction_dispatcher_unavailable") return None, "", "", None - # Admin-only profiles count as hidden here: the reaction session inherits - # the triggering user's role ("user"), so routing an ordinary user's event - # to an admin profile would hand out a wider tool surface than the profile - # list offers that user. + # The profiles the *run* may reach, not the profiles that exist: the run carries + # the triggering account's role, so that is the role the gate is asked with. + # Offering an id `_resolve_profile` will refuse produces a routing rule that looks + # accepted and is silently discarded one step later. visible = [ - p.id + p for p in profiles.all() - if not getattr(p, "is_hidden", False) and not admin_only_blocked(p, "user") + if not getattr(p, "is_hidden", False) and not admin_only_blocked(p, role) ] system = dispatcher.system_prompt + ( - "\n\n## Available profile ids\n" + "\n".join(f"- {pid}" for pid in visible) + "\n\n## Available profile ids\n" + "\n".join(_profile_line(p) for p in visible) ) user = ( "## The user's dispatcher instructions (routing)\n" @@ -384,9 +428,7 @@ parsed = _parse_decision(resp.content or "") if parsed is None or parsed.get("skip"): return None, "", "", None - session_key = parsed.get("session_key") - if not isinstance(session_key, str) or not session_key.strip(): - session_key = None + session_key = _normalise_session_key(parsed.get("session_key")) return ( parsed.get("profile_id") or "", parsed.get("task") or "", @@ -395,6 +437,34 @@ ) +def _profile_line(profile) -> str: + """One offered profile: the id to answer with, then what it actually is. + + The name and the description are not decoration. A routing document names + profiles the way its author speaks about them — "the sysadmin profile" — and an + id on its own leaves the model matching that phrase against `server_admin` with + nothing to match on. + """ + name = getattr(profile, "name", "") or profile.id + description = (getattr(profile, "description", "") or "").strip() + line = f"- {profile.id} ({name})" + return f"{line} — {description}" if description else line + + +def _normalise_session_key(value) -> str | None: + """The conversation key as it is stored and matched. + + One conversation must produce one spelling or the thread forks, and the spelling + comes from a model. Case and stray whitespace carry no meaning in an id-sized key, + so they are flattened here instead of being allowed to split a chat in two + (`TG:42` and `tg:42` are the same conversation). + """ + if not isinstance(value, str): + return None + key = " ".join(value.split()).lower() + return key or None + + def _parse_decision(text: str) -> dict | None: text = text.strip() if not text: @@ -412,29 +482,60 @@ return None -def _resolve_profile(profiles, profile_id: str): - """Only user-visible profiles may host a reaction session. +def _resolve_profile(profiles, profile_id: str, role: str = "user"): + """The profile that will host the reaction session. - Admin-only profiles count as hidden: the run inherits the triggering user's - role, so landing on one would hand that user an admin tool surface. The - configured fallback is subject to the same rule, otherwise an unroutable - event would reach exactly the profile the dispatcher was not offered. + The dispatcher's choice stands when the rule lets this *role* reach it — the very + same `admin_only_blocked` every other surface consults, asked with the triggering + account's role, so an admin's event can reach the admin profiles their own profile + list offers them. Hidden profiles are out for everyone. + + A refusal never happens in silence: the substitution is logged (requested id, + resolved id, role), because a routing rule that is quietly discarded is + indistinguishable from a routing rule that does not work. """ def _allowed(profile) -> bool: - return not getattr(profile, "is_hidden", False) and not admin_only_blocked(profile, "user") + return ( + profile is not None + and not getattr(profile, "is_hidden", False) + and not admin_only_blocked(profile, role) + ) try: - profile = profiles.get(profile_id) - except Exception: - profile = None - if profile is not None and _allowed(profile): - return profile + requested = profiles.get(profile_id) + except ProfileNotFound: + requested = None + if _allowed(requested): + return requested - fallback = profiles.get(_FALLBACK_PROFILE_ID) - if _allowed(fallback): - return fallback - return next((p for p in profiles.all() if _allowed(p)), fallback) + fallback = _fallback_profile(profiles, role) + resolved = fallback if _allowed(fallback) else next( + (p for p in profiles.all() if _allowed(p)), fallback + ) + if profile_id: + log.warning( + "synapse.reaction_profile_downgraded", + requested=profile_id, + resolved=getattr(resolved, "id", None), + role=role, + admin_only=bool(getattr(requested, "is_admin_only", False)), + hidden=bool(getattr(requested, "is_hidden", False)), + ) + return resolved + + +def _fallback_profile(profiles, role: str): + """The profile an unroutable event lands on, chosen for the role it runs under. + + A fixed id per role, so the landing spot is a decision rather than a side effect of + the order `all()` happens to return. Both ids exist in every shipped profile set; an + installation that dropped one falls back to `None` and the caller walks `all()`. + """ + try: + return profiles.get(_FALLBACK_PROFILE_IDS.get(role, "assistant")) + except ProfileNotFound: + return None def _reaction_context_message(envelope: dict, *, fresh: bool) -> str: diff --git a/tests/unit/core/test_synapse_reactions.py b/tests/unit/core/test_synapse_reactions.py index c83f6c6..c94d71a 100644 --- a/tests/unit/core/test_synapse_reactions.py +++ b/tests/unit/core/test_synapse_reactions.py @@ -12,6 +12,8 @@ import navi.synapse.reactions as reactions from navi.core.registry import ProfileRegistry from navi.profiles import ALL_PROFILES +from navi.tools._internal.base import current_user_role +from tests.conftest_factory import FakeConnection, FakePool, FakeRecord def _registry() -> ProfileRegistry: @@ -42,6 +44,24 @@ assert decision is not None and decision["skip"] is True +# ── _normalise_session_key ─────────────────────────────────────────────────── + +def test_one_conversation_gets_one_spelling(): + """The spelling comes from a model, and a key that differs by case or by a stray + space is a different conversation to the session lookup: the thread forks.""" + assert reactions._normalise_session_key("TG:42") == "tg:42" + assert reactions._normalise_session_key(" tg:42 ") == "tg:42" + assert reactions._normalise_session_key("tg : 42") == "tg : 42" + assert reactions._normalise_session_key("tg:42\n") == "tg:42" + + +def test_a_key_that_is_not_a_string_is_no_key(): + assert reactions._normalise_session_key(None) is None + assert reactions._normalise_session_key(42) is None + assert reactions._normalise_session_key(["tg:42"]) is None + assert reactions._normalise_session_key(" ") is None + + # ── _resolve_profile ──────────────────────────────────────────────────────── def test_a_user_visible_profile_resolves(): @@ -49,19 +69,65 @@ assert reactions._resolve_profile(reg, "assistant").id == "assistant" -def test_an_admin_only_profile_is_not_a_reaction_target(): - """A reaction run inherits role "user" (see _spawn_run_task), so landing on an - admin-only profile would hand the triggering user a wider tool surface than - their own profile list offers them.""" +def test_an_admin_only_profile_is_refused_for_a_regular_user(): + """The role is the deciding factor, exactly as everywhere else: a run carrying + role "user" must not land on a profile that user's own profile list withholds.""" reg = _registry() assert reg.get("secretary").is_admin_only - resolved = reactions._resolve_profile(reg, "secretary") + resolved = reactions._resolve_profile(reg, "secretary", role="user") assert resolved.id != "secretary" assert not resolved.is_admin_only +def test_an_admin_only_profile_resolves_for_an_admin(): + """Regression: with the role hardcoded to "user" the owner's own events could not + reach a single profile they know by name — every one of them is admin-only.""" + reg = _registry() + assert reg.get("server_admin").is_admin_only + + assert reactions._resolve_profile(reg, "server_admin", role="admin").id == "server_admin" + + +def test_the_fallback_depends_on_the_role_not_on_the_order_of_all(): + """`secretary` is admin-only, so a regular user was refused it and the event slid + onto whichever profile `all()` yielded first — an accident of ordering.""" + reg = _registry() + assert reactions._resolve_profile(reg, "astronaut", role="admin").id == "secretary" + assert reactions._resolve_profile(reg, "astronaut", role="user").id == "assistant" + + +def test_a_refused_profile_is_logged(monkeypatch): + """A substitution nobody can see is indistinguishable from a rule that does not + work — which is how this bug survived a live test.""" + from unittest.mock import MagicMock + + reg = _registry() + log = MagicMock() + monkeypatch.setattr(reactions, "log", log) + + reactions._resolve_profile(reg, "server_admin", role="user") + + event, kwargs = log.warning.call_args.args[0], log.warning.call_args.kwargs + assert event == "synapse.reaction_profile_downgraded" + assert kwargs["requested"] == "server_admin" + assert kwargs["resolved"] == "assistant" + assert kwargs["role"] == "user" + assert kwargs["admin_only"] is True + + +def test_a_profile_that_resolves_is_not_logged(monkeypatch): + from unittest.mock import MagicMock + + log = MagicMock() + monkeypatch.setattr(reactions, "log", log) + + reactions._resolve_profile(_registry(), "server_admin", role="admin") + + assert log.warning.call_count == 0 + + def test_hidden_profile_falls_back_to_a_user_visible_one(): reg = _registry() # dispatcher exists but is hidden → must never host a session @@ -170,6 +236,47 @@ assert (await reactions._dispatch({}, "i", ""))[3] is None +async def test_the_offered_profiles_depend_on_the_role_and_carry_their_names(monkeypatch): + """Two halves of one bug: a routing document names profiles in words, so ids alone + give the model nothing to match on; and an id the run cannot reach must not be + offered at all, or the rule looks accepted and is discarded one step later.""" + backend = MagicMock() + backend.complete = AsyncMock(return_value=SimpleNamespace(content=json.dumps( + {"profile_id": "assistant", "task": "t", "understood": "u", "session_key": None} + ))) + monkeypatch.setattr(deps, "get_profile_registry", lambda: _registry()) + monkeypatch.setattr( + deps, "get_backend_registry", lambda: SimpleNamespace(get=lambda key: backend) + ) + + async def offered(role: str) -> str: + await reactions._dispatch({}, "i", "", role=role) + return backend.complete.await_args.args[0][0].content + + as_admin = await offered("admin") + as_user = await offered("user") + + assert "- server_admin (Server Administrator)" in as_admin + assert "- server_admin" not in as_user + assert "- dispatcher" not in as_admin # hidden for everyone + # the description is what a phrase like "the sysadmin profile" can match against + assert "Server administration, monitoring" in as_admin + assert "- assistant (Assistant)" in as_user + + +async def test_dispatch_normalises_the_key_it_was_given(monkeypatch): + backend = MagicMock() + backend.complete = AsyncMock(return_value=SimpleNamespace(content=json.dumps({ + "profile_id": "assistant", "task": "t", "understood": "u", "session_key": " TG:42 ", + }))) + monkeypatch.setattr(deps, "get_profile_registry", lambda: _registry()) + monkeypatch.setattr( + deps, "get_backend_registry", lambda: SimpleNamespace(get=lambda key: backend) + ) + + assert (await reactions._dispatch({}, "i", ""))[3] == "tg:42" + + async def test_dispatch_backend_failure_yields_skip(monkeypatch): backend = MagicMock() backend.complete = AsyncMock(side_effect=RuntimeError("boom")) @@ -194,7 +301,7 @@ self.last_active = datetime.now(timezone.utc) -def _fake_store(saved, *, found=None): +def _fake_store(saved, *, found=None, role="user"): store = MagicMock() async def fake_create(profile_id, user_id=None): @@ -208,9 +315,12 @@ # AsyncMock(return_value=None), never a bare MagicMock: a MagicMock returns a # truthy mock and every event would look like a continuation. store.find_reaction_session = AsyncMock(return_value=found) - # PgSessionStore._get_pool is async; the settings store is mocked out in - # these tests, so the pool itself is never used. - store._get_pool = AsyncMock(return_value=None) + # The runner reads the triggering account's role through this pool before it can + # ask `admin_only_blocked` anything; the settings store is mocked out in these + # tests, so the role row is the first thing off the queue. + pool = FakeConnection() + pool.enqueue(FakeRecord(role=role)) + store._get_pool = AsyncMock(return_value=FakePool(pool)) return store @@ -274,17 +384,20 @@ async def test_run_reaction_happy_path(monkeypatch): - """Gate passes → dispatcher picks a profile → special session is created and run.""" + """Gate passes → dispatcher picks a profile → special session is created and run. + + The account behind the event is an admin here, and the profile it is routed to is + admin-only: that is the whole point of the role lookup. Before it, the id would have + been refused and swapped for a user-visible profile (see _resolve_profile). + """ saved: list = [] - store = _fake_store(saved) + store = _fake_store(saved, role="admin") monkeypatch.setattr(deps, "get_session_store", lambda: store) _settings(monkeypatch, instructions="watch gntodo") monkeypatch.setattr(deps, "get_profile_registry", lambda: _registry()) - monkeypatch.setattr( - reactions, "_dispatch", - AsyncMock(return_value=("assistant", "fix it", "ok", "tg:42")), - ) + dispatch = AsyncMock(return_value=("server_admin", "fix it", "ok", "tg:42")) + monkeypatch.setattr(reactions, "_dispatch", dispatch) queue: asyncio.Queue = asyncio.Queue() orchestrator, _run = _fake_orchestrator(queue) @@ -292,9 +405,11 @@ # run_agent completes right away: drain must see "done" from queue. ran: list = [] + roles: list = [] async def fake_run_agent(*args, **kwargs): ran.append((args, kwargs)) + roles.append(current_user_role.get()) await queue.put(("done", None)) orchestrator.run_agent = fake_run_agent @@ -307,8 +422,12 @@ user_id="u1", event_type="gntodo.task.created", ) + # The role the pool handed over reaches the dispatcher and the run itself. + assert dispatch.await_args.kwargs["role"] == "admin" + assert roles == ["admin"] + profile_id, user_id, session = saved[0] - assert (profile_id, user_id) == ("assistant", "u1") + assert (profile_id, user_id) == ("server_admin", "u1") assert session.special is True assert session.name == "Synapse: gntodo.task.created" assert session.session_metadata["synapse"]["event_id"] == "e1" @@ -535,6 +654,44 @@ assert await reactions._pool_of(_SessionStore()) is pool +async def test_user_role_reads_the_account_s_role_from_navi_users(): + conn = FakeConnection() + conn.enqueue(FakeRecord(role="admin")) + assert await reactions._user_role(FakePool(conn), "u1") == "admin" + assert "navi_users" in conn.calls[0][1] + + +async def test_user_role_fails_closed(): + """An unreadable role must never *widen* a run, so "user" is the answer and the + failure is logged rather than guessed at.""" + assert await reactions._user_role(FakePool(FakeConnection()), "u1") == "user" + + conn = FakeConnection() + conn.enqueue(FakeRecord(no_role_column="here")) + assert await reactions._user_role(FakePool(conn), "u1") == "user" + + class _Pool: + async def fetchrow(self, *args): + raise RuntimeError("db down") + + assert await reactions._user_role(_Pool(), "u1") == "user" + + +async def test_spawn_run_task_carries_the_role_into_the_run(): + """The ContextVar is set only long enough to snapshot the new task's context, so + the role is observable inside the run — which is where the tools read it.""" + seen: list[str] = [] + + class _Orchestrator: + async def run_agent(self, *args, **kwargs): + seen.append(current_user_role.get()) + + await reactions._spawn_run_task(_Orchestrator(), "s1", "hi", None, "u1", role="admin") + + assert seen == ["admin"] + assert current_user_role.get() == "user" # the caller's own context is untouched + + async def test_run_reaction_reads_settings_through_the_real_pool(monkeypatch): """The disabled gate is decided by the row that comes out of the pool — SynapseSettingsStore and the pool are both real here."""