diff --git a/changelog.d/corpus-default-agent.added.md b/changelog.d/corpus-default-agent.added.md new file mode 100644 index 0000000000..2a872da9dc --- /dev/null +++ b/changelog.d/corpus-default-agent.added.md @@ -0,0 +1,44 @@ +- **`Corpus.default_agent`, plus the two changes that make it mean anything.** + A corpus could not have a default agent. A chat opened with no `agent_id` + always resolved to the GLOBAL `default-corpus-agent` slug, so an + `AgentConfiguration` scoped to a corpus could never be its default however it + was configured — and even when one *was* resolved, its `system_instructions` + and `available_tools` were discarded, because those apply only on the + explicit-`?agent_id=` path. Three things follow: + - `Corpus.default_agent` (FK, mirroring `CorpusGroup.default_agent`). An + explicit pointer, not "pick a CORPUS-scoped agent": corpora already carry + scoped agents for other purposes (the inline moderator), and choosing + positionally would silently hand chat to whichever sorted first. + `Corpus.save` refuses a pointer at an agent scoped to a *different* + corpus — that would serve one corpus's private instructions to another's + users. + - Resolution priority 3 consults it before the global slug. An inactive + target falls back rather than failing, which is what switching an agent + off is asking for. + - Fallback-resolved configs are applied when `system_instructions_mode` is + `EXTEND`. The exclusion was written when REPLACE was the only mode, and + its stated reason — a generic default would clobber the corpus persona — + is specific to REPLACE. EXTEND appends, so there is nothing to clobber; + excluding it only produced a default that resolved and was then silently + ignored. REPLACE keeps the original behaviour on the fallback path. +- **`install_domain_pack --consumer-corpus ` (contract assertion C8).** + A domain pack may now declare a `consumer_agent` — instructions, tools, and + `mode: EXTEND` — for the corpus that *consumes* the domain. The pack supplies + the text, because the group slug it names is the pack's own invention; the + operator supplies the corpus, because which corpus consumes a domain is not + knowable when the pack is written. `EXTEND` is required rather than + preferred: REPLACE would overwrite the consuming corpus's persona with + third-party text, which is the coupling `DOMAIN_PACKS.md` forbids. Declaring + one without binding it reports that it was not applied (C5) instead of + passing silently, and the same group-slug check the orchestrator is held to + applies here — `search_across_corpora` takes the slug as a required + argument, so an agent never told it cannot call it. `consumer_agent.preferred_llm` + gets the same C3-style pre-validation as the orchestrator's, so an unusable + model spec fails cleanly instead of surfacing as a bare `ValidationError` + traceback from inside `consumer.save()`. And every part of C8 that is + decidable from the files — `mode`, `instructions_file`, `tools`, the + group-slug check, `preferred_llm` — is now previewed by `--check` too + (`_preflight`, shared with the real-install path via + `_consumer_agent_violations`), matching what C1-C7 already guaranteed; + previously `--check` reported "0 violations" for a `consumer_agent` that + would hard-fail the moment `--consumer-corpus` was supplied. diff --git a/config/websocket/consumers/unified_agent_conversation.py b/config/websocket/consumers/unified_agent_conversation.py index 9706cc7ce2..2ec922b85b 100644 --- a/config/websocket/consumers/unified_agent_conversation.py +++ b/config/websocket/consumers/unified_agent_conversation.py @@ -331,12 +331,21 @@ async def _resolve_agent_config(self) -> AgentConfiguration | None: Priority: 1. Explicit agent_id → use that agent (gated by ``visible_to_user``) 2. document_id present → default-document-agent - 3. corpus_id present → default-corpus-agent + 3. corpus_id present → ``Corpus.default_agent``, else default-corpus-agent The explicit-agent path uses ``visible_to_user`` rather than a bare ``aget(pk=...)`` so a caller can't load another user's private agent by guessing the pk. The default agents are GLOBAL/active so the guard isn't load-bearing for paths 2/3. + + ``Corpus.default_agent`` is consulted before the global slug so a + corpus can carry its own default without every caller having to pass + ``agent_id``. It is a deliberate FK rather than a query over + CORPUS-scoped agents: corpora hold scoped agents for unrelated + purposes, and picking positionally among them would silently hand + chat to whichever sorted first. ``Corpus.save`` guarantees the target + is GLOBAL or scoped to this same corpus, so no cross-corpus leak is + reachable through this path. """ # Priority 1: Explicit agent_id (visibility-gated) if self.agent_config_id: @@ -371,8 +380,15 @@ def _visible_agent_lookup() -> AgentConfiguration | None: ) return None - # Priority 3: Corpus context → default corpus agent + # Priority 3: Corpus context → the corpus's own default, else global if self.corpus_id: + corpus_default = await self._corpus_default_agent() + if corpus_default is not None: + logger.info( + f"[Session {self.session_id}] Using corpus default agent " + f"{corpus_default.slug!r} for corpus {self.corpus_id}" + ) + return corpus_default try: return await AgentConfiguration.objects.aget( slug="default-corpus-agent", is_active=True @@ -384,8 +400,30 @@ def _visible_agent_lookup() -> AgentConfiguration | None: ) return None + # Neither a document nor a corpus in scope: nothing to resolve. return None + async def _corpus_default_agent(self) -> AgentConfiguration | None: + """``Corpus.default_agent`` for this session's corpus, if usable. + + An inactive target is treated as absent rather than as an error: the + fallback to the global default keeps corpus chat working while an + operator has a corpus agent switched off, which is the behaviour a + deactivation is asking for. + """ + from opencontractserver.corpuses.models import Corpus + + def _lookup() -> AgentConfiguration | None: + row = ( + Corpus.objects.filter(pk=self.corpus_id) + .select_related("default_agent") + .first() + ) + agent = getattr(row, "default_agent", None) + return agent if (agent is not None and agent.is_active) else None + + return await database_sync_to_async(_lookup)() + # ------------------------------------------------------------------------- # Message sending # ------------------------------------------------------------------------- @@ -652,13 +690,30 @@ async def _initialize_agent( # Explicitly-selected agents (``?agent_id=...``) get their configured # persona and tool list threaded into the factory, matching the # delegation sub-agent path (``build_delegation_tool``) and the - # Celery corpus-action path (``_resolve_action_tools``). The default - # agents (resolved by slug fallback when no ``agent_id`` is given) - # intentionally do NOT override the factory's context-derived prompt - # (e.g. ``corpus_agent_instructions``) — their generic - # ``system_instructions`` would clobber the richer per-corpus persona. + # Celery corpus-action path (``_resolve_action_tools``). + # + # Agents resolved by FALLBACK (no ``agent_id`` given) were historically + # excluded outright, because a generic default's ``system_instructions`` + # would REPLACE — and so clobber — the richer per-corpus persona. That + # reasoning is specific to REPLACE, which was the only mode when the + # guard was written. An EXTEND config appends to the context-derived + # prompt instead of consuming it, so the clobber it guards against + # cannot occur; excluding EXTEND configs here only means a corpus + # default is resolved and then silently ignored, which is the harder + # failure to diagnose. REPLACE keeps the original behaviour on the + # fallback path. + # + # ``getattr`` with a default rather than a direct attribute read: the + # field is non-null on the model, but this attribute is also reached on + # objects that only stand in for one, and REPLACE is the documented + # default — so "absent" and "REPLACE" must behave identically. A direct + # read turns a partially-populated config into an AttributeError on a + # path that previously never touched the field at all. config_tools: list[Any] = [] - if self.agent_config and self.agent_config_id: + if self.agent_config and ( + self.agent_config_id + or getattr(self.agent_config, "system_instructions_mode", None) == "EXTEND" + ): if self.agent_config.system_instructions: # ``system_instructions_mode`` decides whether these instructions # REPLACE the context-derived persona (the historical default) diff --git a/mypy.ini b/mypy.ini index e49969d1ff..4b16774114 100644 --- a/mypy.ini +++ b/mypy.ini @@ -661,6 +661,18 @@ disable_error_code = django-manager-missing [mypy-opencontractserver.documents.models] disable_error_code = django-manager-missing +# Same limitation again, for AgentConfiguration's reverse relation +# 'default_for_corpuses' from ``Corpus.default_agent``. Corpus declares its +# manager as ``PermissionedTreeQuerySet.as_manager(with_tree_fields=True)``, +# so the plugin cannot introspect it to resolve the reverse accessor — the +# identical cause already disabled for users/annotations/documents above. +# Note the sibling relation ``CorpusGroup.default_agent`` does NOT trip this: +# CorpusGroup overrides its manager to ``PermissionManager()``, which the +# plugin can introspect. Typing Corpus's manager properly removes the need +# for this entry and the three above it together. +[mypy-opencontractserver.agents.models] +disable_error_code = django-manager-missing + # --------------------------------------------------------------------------- # Baseline: authority-pack and deep-research modules (issue #1331 workflow) # diff --git a/opencontractserver/corpuses/management/commands/install_domain_pack.py b/opencontractserver/corpuses/management/commands/install_domain_pack.py index 0e286e7ecf..7578f3e781 100644 --- a/opencontractserver/corpuses/management/commands/install_domain_pack.py +++ b/opencontractserver/corpuses/management/commands/install_domain_pack.py @@ -118,6 +118,20 @@ def add_arguments(self, parser): action="store_true", help="Fetch and report the plan; write nothing.", ) + parser.add_argument( + "--consumer-corpus", + type=int, + default=None, + help=( + "Primary key of the corpus that CONSUMES this domain — the one " + "holding the documents users ask about. Binds the pack's " + "`consumer_agent` to it and makes that agent the corpus's " + "default. Which corpus consumes a domain is an installation " + "fact the pack cannot know, so it is an argument and not a " + "manifest field. Omit it and the pack installs exactly as " + "before: group plus orchestrator, nothing bound." + ), + ) # ------------------------------------------------------------------ # # fetch @@ -263,7 +277,7 @@ def handle(self, *args, **options): # Everything decidable from the FILES is decided before anything is # written, for --check and for a real install alike. A preflight that # only runs under --check is a preflight the install path never gets. - violations = self._preflight(manifest, pack_dirs) + violations = self._preflight(manifest, pack_dirs, domain_dir) violations += self._preflight_base_packs(pack_dirs, options) if options["check"]: @@ -363,6 +377,13 @@ def _report_plan(self, manifest, pack_dirs: dict[str, Path]) -> None: self.stdout.write( f" cross-pack equivalences: " f"{len(manifest.get('equivalences') or [])}" ) + consumer = manifest.get("consumer_agent") + if isinstance(consumer, dict): + self.stdout.write( + " consumer_agent: mode=" + f"{consumer.get('mode')!r}, tools={', '.join(consumer.get('tools') or [])} " + "(pass --consumer-corpus to bind on install)" + ) def _member_slugs(self, pack_dirs: dict[str, Path], group: dict): """Corpus slugs every required base pack contributes, minus exclusions.""" @@ -460,7 +481,96 @@ def _preflight_base_packs(self, pack_dirs: dict[str, Path], options) -> list[str return violations # ------------------------------------------------------------------ # - def _preflight(self, manifest, pack_dirs: dict[str, Path]) -> list[str]: + def _consumer_agent_violations( + self, spec: dict, domain_dir: Path, group_slug: str + ) -> tuple[list[str], str, list[str]]: + """The C8 checks decidable from ``spec`` and the pack's files alone. + + Shared by ``_preflight`` (so ``--check`` previews them the way it + previews C1-C7) and ``_wire_consumer_agent`` (so the real install + enforces the identical rule, not a hand-kept copy of it). Excludes the + two checks that are NOT file-decidable — whether ``spec`` is a mapping + at all, and whether ``--consumer-corpus`` resolves to a real corpus — + which stay with their respective callers. + """ + violations: list[str] = [] + + # EXTEND is required, not merely preferred. A REPLACE consumer agent + # would overwrite the consuming corpus's own persona with text shipped + # by a third party — which is exactly the coupling DOMAIN_PACKS.md + # forbids. EXTEND appends, so the corpus persona stays single-sourced + # on the corpus and the pack contributes only its increment. + mode = str(spec.get("mode") or "") + if mode != "EXTEND": + violations.append( + f"C8: consumer_agent.mode must be 'EXTEND' (got {mode!r}). " + "REPLACE would overwrite the consuming corpus's persona with " + "pack-supplied text." + ) + + # Same traversal reasoning as the orchestrator's instructions_file: the + # root is derived from the argument-checked domain name, never from the + # manifest, so the manifest cannot move the root and the guard together. + instructions = "" + rel = spec.get("instructions_file") + if not rel: + violations.append("C8: consumer_agent.instructions_file is required") + else: + root = domain_dir.resolve() + path = (root / str(rel)).resolve() + if not str(path).startswith(str(root) + "/") or not path.is_file(): + violations.append( + f"C8: consumer_agent instructions_file unreadable: {rel}" + ) + else: + instructions = path.read_text(encoding="utf-8") + if not instructions.strip(): + violations.append( + f"C8: consumer_agent instructions_file is empty: {rel}" + ) + + tools = [str(t) for t in (spec.get("tools") or [])] + for tool in tools: + if tool not in GRANTABLE_TOOLS: + violations.append( + f"C8: consumer_agent.tools names {tool!r}, which a domain " + f"pack may not grant (allowed: {', '.join(sorted(GRANTABLE_TOOLS))})" + ) + # Same rule the orchestrator is held to, and the same reason: the tool + # takes the group slug as a required argument, so an agent never told + # the slug cannot call it — and that looks identical to the model + # deciding not to. + if "search_across_corpora" in tools and group_slug not in instructions: + violations.append( + f"C8: consumer_agent declares search_across_corpora but its " + f"instructions never name the group slug {group_slug!r}" + ) + + # C3-style validation for the sibling field: `AgentConfiguration.save()` + # validates `preferred_llm` and raises Django's ValidationError, which + # is NOT a CommandError and so is caught nowhere. Checked here instead, + # same as the orchestrator's preferred_llm below, so a typo'd model + # spec fails cleanly instead of surfacing as a bare traceback. + preferred_llm = spec.get("preferred_llm") + if preferred_llm: + from opencontractserver.llms.llm_registry import ( + LLMProviderNotRegistered, + validate_model_spec, + ) + + try: + validate_model_spec(str(preferred_llm)) + except (ValueError, LLMProviderNotRegistered) as exc: + violations.append( + f"C8: consumer_agent.preferred_llm {preferred_llm!r} is not " + f"usable on this platform: {exc}" + ) + + return violations, instructions, tools + + def _preflight( + self, manifest, pack_dirs: dict[str, Path], domain_dir: Path + ) -> list[str]: """Contract assertions decidable from the FILES, before anything is written. These used to be split: C2 was reported by ``--check`` as styled text @@ -589,6 +699,24 @@ def _preflight(self, manifest, pack_dirs: dict[str, Path]) -> list[str]: f"C4: equivalence to_key {to!r} names no section in any " "required base pack" ) + + # C8 — the consumer_agent half of the contract that IS decidable from + # the files: everything except whether ``--consumer-corpus`` resolves. + # `_wire_consumer_agent` enforces the full contract at install time, + # but it only runs inside `_wire`, which `handle()` never reaches + # under `--check`. Without this, `--check` reported "0 violations" for + # a manifest whose consumer_agent would hard-fail — REPLACE mode, a + # missing instructions_file, an ungrantable tool, an unusable + # preferred_llm — the moment an operator actually ran the install. + spec = manifest.get("consumer_agent") + if spec is not None: + if not isinstance(spec, dict): + violations.append("C8: consumer_agent must be a mapping") + else: + violations += self._consumer_agent_violations( + spec, domain_dir, group_slug + )[0] + return violations # ------------------------------------------------------------------ # @@ -694,6 +822,21 @@ def _wire( f"tools={tools}, {len(instructions):,} chars of instructions" ) + # --- C8 consumer agent -------------------------------------------- # + # + # The orchestrator above is GLOBAL: it knows the domain but not the + # documents. The agent that answers a user's question is the one + # scoped to the corpus those documents live in, and it needs BOTH the + # corpus persona and the reach to call across the group. + # + # A domain pack can author the reach increment — the group slug is its + # own invention — but it cannot know which corpus consumes it. So the + # text is a manifest field and the binding is a command-line argument. + # Neither half is useful alone, which is why both are checked here. + self._wire_consumer_agent( + manifest, domain_dir, options, user, group, unmet, AgentConfiguration + ) + # --- cross-pack equivalences -------------------------------------- # # # Written through the shared upsert rather than the ORM directly. That @@ -772,3 +915,115 @@ def _wire( "cross-pack equivalence(s)." ) ) + + # ------------------------------------------------------------------ # + # consumer agent (C8) + # ------------------------------------------------------------------ # + def _wire_consumer_agent( + self, + manifest, + domain_dir: Path, + options, + user, + group, + unmet, + AgentConfiguration, + ) -> None: + """Bind the pack's ``consumer_agent`` to the corpus named on the CLI. + + The two halves are supplied by different parties on purpose. The pack + authors the instructions, because the reach they describe is the group + the pack itself defines. The operator names the corpus, because which + corpus consumes a domain is not knowable when the pack is written. + Supplying one without the other is a mistake in both directions, and + each gets its own diagnostic rather than a silent no-op. + """ + from opencontractserver.corpuses.models import Corpus + + spec = manifest.get("consumer_agent") + corpus_pk = options.get("consumer_corpus") + + if spec is None and corpus_pk is None: + return + if spec is None: + raise CommandError( + "--consumer-corpus was given but this domain pack declares no " + "`consumer_agent`. Nothing to bind; re-run without the flag." + ) + if not isinstance(spec, dict): + unmet.append("C8: consumer_agent must be a mapping") + return + if corpus_pk is None: + # C5 — a declared capability that was not applied has to be said + # out loud. Silence here is what made the previous arrangement + # reproducible only by whoever remembered to run a script. + self.stdout.write( + self.style.WARNING( + "\nconsumer_agent: declared by the pack but NOT bound — pass " + "--consumer-corpus to attach it to the corpus holding " + "your documents. Without it this install gives you the " + "group and the orchestrator only." + ) + ) + return + + # The file-decidable half of the contract — mode, instructions_file, + # tools, search_across_corpora/group-slug, preferred_llm — is shared + # with `_preflight`, so `--check` previews exactly what a real install + # enforces here. Unreachable on a real install once `_preflight` has + # run — it evaluates the same `spec` from the same files — but kept + # (and exercised directly in + # `test_wire_consumer_agent_still_refuses_a_bad_mode_if_preflight_is_bypassed`) + # so a future refactor that calls `_wire` without `_preflight` first + # still cannot bind a REPLACE-mode or otherwise-invalid consumer agent. + violations, instructions, tools = self._consumer_agent_violations( + spec, domain_dir, group.slug + ) + unmet.extend(violations) + if unmet: + # Don't write a half-valid agent, and don't let a DB-dependent + # check below (the corpus lookup) mask a manifest-authoring + # mistake that's already known from the files alone. handle() + # reports `unmet` and fails. + return + + corpus = Corpus.objects.filter(pk=corpus_pk).first() + if corpus is None: + raise CommandError(f"--consumer-corpus {corpus_pk} does not exist") + + consumer, created = AgentConfiguration.objects.get_or_create( + slug=f"{manifest['name']}-consumer-{corpus.pk}", + defaults={ + "name": ( + f"{manifest.get('title') or manifest['name']} — {corpus.title}" + ), + "creator": user, + "is_public": bool(options["public"]), + }, + ) + consumer.description = ( + f"Corpus agent for {corpus.title!r}: keeps the corpus persona and " + f"adds reach into the {group.slug!r} group. Installed by domain " + f"pack {manifest['name']!r}." + ) + consumer.system_instructions = instructions + consumer.system_instructions_mode = "EXTEND" + consumer.available_tools = tools + consumer.scope = AgentConfiguration.SCOPE_CORPUS + consumer.corpus = corpus + consumer.is_active = True + if spec.get("preferred_llm"): + consumer.preferred_llm = str(spec["preferred_llm"]) + consumer.save() + + # The binding. Without this the agent exists and is never chosen, + # which is the state this whole feature exists to end. + corpus.default_agent = consumer + corpus.save(update_fields=["default_agent"]) + + self.stdout.write( + f"consumer agent {consumer.slug!r}: " + f"{'created' if created else 'updated'}, EXTEND, tools={tools}, " + f"{len(instructions):,} chars appended to corpus {corpus.pk} " + f"({corpus.title!r}), now its default_agent" + ) diff --git a/opencontractserver/corpuses/migrations/0060_corpus_default_agent.py b/opencontractserver/corpuses/migrations/0060_corpus_default_agent.py new file mode 100644 index 0000000000..b9486e6645 --- /dev/null +++ b/opencontractserver/corpuses/migrations/0060_corpus_default_agent.py @@ -0,0 +1,27 @@ +# Generated by Django 5.2.16 on 2026-08-21 04:02 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("agents", "0018_agentconfiguration_system_instructions_mode"), + ("corpuses", "0059_corpusgroup_corpusgroupgroupobjectpermission_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="corpus", + name="default_agent", + field=models.ForeignKey( + blank=True, + help_text="Agent used for corpus chat when the caller names none. Must be GLOBAL or scoped to this same corpus. Prefer system_instructions_mode=EXTEND, which appends to this corpus's corpus_agent_instructions rather than replacing them.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="default_for_corpuses", + to="agents.agentconfiguration", + ), + ), + ] diff --git a/opencontractserver/corpuses/models.py b/opencontractserver/corpuses/models.py index fa0ef4df2a..24c7757fb3 100644 --- a/opencontractserver/corpuses/models.py +++ b/opencontractserver/corpuses/models.py @@ -291,6 +291,30 @@ class Corpus(InstanceUserCanMixin, TreeNode): "If not set, uses DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS from settings." ), ) + # The corpus-level counterpart of ``CorpusGroup.default_agent``. Without + # it, a chat opened against a corpus with no explicit ``agent_id`` always + # resolves to the GLOBAL ``default-corpus-agent`` slug, so an + # AgentConfiguration scoped to this corpus can never be the default no + # matter how it is configured. + # + # An explicit pointer rather than "pick a CORPUS-scoped agent for this + # corpus": corpora already carry scoped agents for unrelated purposes (the + # inline moderator created by ``corpus_mutations``), and a positional + # choice among them would silently hand corpus chat to whichever one + # happened to sort first. + default_agent = django.db.models.ForeignKey( + "agents.AgentConfiguration", + on_delete=django.db.models.SET_NULL, + null=True, + blank=True, + related_name="default_for_corpuses", + help_text=( + "Agent used for corpus chat when the caller names none. Must be " + "GLOBAL or scoped to this same corpus. Prefer " + "system_instructions_mode=EXTEND, which appends to this corpus's " + "corpus_agent_instructions rather than replacing them." + ), + ) # Agent memory memory_enabled = django.db.models.BooleanField( @@ -445,6 +469,39 @@ def save(self, *args: Any, **kwargs: Any) -> None: raise ValidationError({"preferred_llm": str(exc)}) from exc self.preferred_llm = normalise_model_spec(self.preferred_llm) + # ``default_agent`` is what corpus chat falls back to, so a pointer at + # a CORPUS-scoped agent belonging to a *different* corpus would serve + # that corpus's private instructions to this corpus's users. Checked + # here rather than as a DB constraint because the condition spans two + # tables. Only fires when the FK is set, so ordinary saves pay nothing. + if self.default_agent_id: + from django.core.exceptions import ValidationError + + from opencontractserver.agents.models import AgentConfiguration + + agent = ( + AgentConfiguration.objects.filter(pk=self.default_agent_id) + .values("scope", "corpus_id") + .first() + ) + if agent is None: + raise ValidationError( + {"default_agent": "AgentConfiguration does not exist"} + ) + if ( + agent["scope"] == AgentConfiguration.SCOPE_CORPUS + and agent["corpus_id"] != self.pk + ): + raise ValidationError( + { + "default_agent": ( + "a CORPUS-scoped agent may only be the default for " + "the corpus it is scoped to; this one is scoped to " + f"corpus {agent['corpus_id']}" + ) + } + ) + # Ensure slug exists and is unique within creator scope if not self.slug or not isinstance(self.slug, str) or not self.slug.strip(): base_value = self.title or "corpus" diff --git a/opencontractserver/tests/test_corpus_default_agent.py b/opencontractserver/tests/test_corpus_default_agent.py new file mode 100644 index 0000000000..c10ddabaaa --- /dev/null +++ b/opencontractserver/tests/test_corpus_default_agent.py @@ -0,0 +1,82 @@ +"""``Corpus.default_agent`` — the guard on what a corpus may default to. + +``default_agent`` is what corpus chat falls back to, so a pointer at a +CORPUS-scoped agent belonging to a *different* corpus would serve that corpus's +private instructions to this corpus's users. The condition spans two tables, so +it is enforced in ``Corpus.save`` rather than as a DB constraint. + +The *resolution* half — that priority 3 prefers this field over the global +``default-corpus-agent`` slug — lives in +``tests/websocket/test_unified_agent_consumer.py``. It needs committed-data +semantics (the consumer resolves on its own connection), which is what +``WebsocketFixtureBaseTestCase`` provides and a plain ``TestCase`` does not. +""" + +from __future__ import annotations + +from django.core.exceptions import ValidationError +from django.test import TestCase + +from opencontractserver.agents.models import AgentConfiguration +from opencontractserver.corpuses.models import Corpus +from opencontractserver.users.models import User + + +class CorpusDefaultAgentGuardTestCase(TestCase): + """A corpus may only default to a GLOBAL agent or one of its own.""" + + def setUp(self) -> None: + self.user = User.objects.create_user( + username="corpusowner", password="x", email="a@b.c" + ) + self.corpus = Corpus.objects.create(title="Mine", creator=self.user) + self.other = Corpus.objects.create(title="Theirs", creator=self.user) + + def _agent(self, slug: str, *, scope: str, corpus: Corpus | None = None): + return AgentConfiguration.objects.create( + name=slug, + slug=slug, + creator=self.user, + scope=scope, + corpus=corpus, + system_instructions="hi", + ) + + def test_global_agent_is_accepted(self) -> None: + agent = self._agent("g", scope=AgentConfiguration.SCOPE_GLOBAL) + self.corpus.default_agent = agent + self.corpus.save() + self.corpus.refresh_from_db() + self.assertEqual(self.corpus.default_agent_id, agent.pk) + + def test_agent_scoped_to_this_corpus_is_accepted(self) -> None: + agent = self._agent( + "mine", scope=AgentConfiguration.SCOPE_CORPUS, corpus=self.corpus + ) + self.corpus.default_agent = agent + self.corpus.save() + self.corpus.refresh_from_db() + self.assertEqual(self.corpus.default_agent_id, agent.pk) + + def test_agent_scoped_to_another_corpus_is_refused(self) -> None: + """The leak this guard exists to stop.""" + agent = self._agent( + "theirs", scope=AgentConfiguration.SCOPE_CORPUS, corpus=self.other + ) + self.corpus.default_agent = agent + with self.assertRaises(ValidationError) as ctx: + self.corpus.save() + self.assertIn("default_agent", ctx.exception.message_dict) + + self.corpus.refresh_from_db() + self.assertIsNone(self.corpus.default_agent_id) + + def test_pointer_at_a_nonexistent_agent_is_refused(self): + """``default_agent_id`` can be set without loading the row — a stale + pk from a fixture or a fork would otherwise persist and only surface + when chat tried to resolve it.""" + self.corpus.default_agent_id = 99999999 + with self.assertRaises(ValidationError) as ctx: + self.corpus.save() + self.assertIn("default_agent", ctx.exception.message_dict) + self.assertIn("does not exist", str(ctx.exception)) diff --git a/opencontractserver/tests/test_domain_pack_install.py b/opencontractserver/tests/test_domain_pack_install.py index 9be3b60224..701dd09acb 100644 --- a/opencontractserver/tests/test_domain_pack_install.py +++ b/opencontractserver/tests/test_domain_pack_install.py @@ -140,10 +140,18 @@ def _base_pack(self, name: str, corpora: list[tuple[str, str, list[str]]]) -> No encoding="utf-8", ) - def _domain(self, name: str, manifest: dict, instructions: str) -> None: + def _domain( + self, + name: str, + manifest: dict, + instructions: str, + extra_files: dict[str, str] | None = None, + ) -> None: d = self.root / "registry" / "domains" / name d.mkdir(parents=True, exist_ok=True) (d / "orchestrator.txt").write_text(instructions, encoding="utf-8") + for rel, body in (extra_files or {}).items(): + (d / rel).write_text(body, encoding="utf-8") (d / "domain.yaml").write_text(yaml.safe_dump(manifest), encoding="utf-8") def _tarball(self) -> str: @@ -171,11 +179,13 @@ def _standard_registry(self, **domain_overrides) -> str: {"from_key": "bb:1", "to_key": "aa:1", "note": "shared title"} ], } + extra_files = domain_overrides.pop("_extra_files", None) manifest.update(domain_overrides) self._domain( "testdomain", manifest, "Call search_across_corpora with corpus_group=test-group.", + extra_files=extra_files, ) return self._tarball() @@ -1214,3 +1224,299 @@ def test_materialise_pack_refuses_a_pack_missing_its_manifest(self): with self.assertRaises(CommandError) as ctx: materialise_pack(staged_pack, "pack-without-manifest") self.assertIn("missing pack.yaml", str(ctx.exception)) + + # ---- C8: the consumer agent ----------------------------------------- # + # + # The orchestrator is GLOBAL and knows the domain; the consumer agent is + # scoped to the corpus holding the documents and needs BOTH the corpus + # persona and reach into the group. The pack supplies the text (the group + # is its own invention); the operator supplies the corpus (the pack cannot + # know it). Each half is useless alone, so each has its own diagnostic. + + CONSUMER_TEXT = ( + "Answer from this corpus. When the answer turns on authority, call " + "search_across_corpora with corpus_group=test-group." + ) + + def _consumer_registry(self, *, spec_overrides=None, text=None, **overrides) -> str: + spec = { + "instructions_file": "consumer.txt", + "tools": ["search_across_corpora"], + "mode": "EXTEND", + } + spec.update(spec_overrides or {}) + return self._standard_registry( + consumer_agent=spec, + _extra_files={ + "consumer.txt": text if text is not None else self.CONSUMER_TEXT + }, + **overrides, + ) + + def test_consumer_agent_binds_to_the_named_corpus(self): + """The whole point: an installed pack yields an agent the corpus uses.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + out = self._run(self._consumer_registry(), consumer_corpus=corpus.pk) + + cfg = AgentConfiguration.objects.get(slug=f"testdomain-consumer-{corpus.pk}") + self.assertEqual(cfg.system_instructions_mode, "EXTEND") + self.assertEqual(cfg.scope, AgentConfiguration.SCOPE_CORPUS) + self.assertEqual(cfg.corpus_id, corpus.pk) + self.assertEqual(cfg.available_tools, ["search_across_corpora"]) + self.assertIn("corpus_group=test-group", cfg.system_instructions) + + corpus.refresh_from_db() + self.assertEqual( + corpus.default_agent_id, + cfg.pk, + "creating the agent without binding it is the state this exists to end", + ) + self.assertIn("consumer agent", out) + + def test_consumer_agent_declared_but_unbound_says_so(self): + """C5 — a declared capability that was not applied must be reported.""" + out = self._run(self._consumer_registry()) + self.assertIn("NOT bound", out) + self.assertIn("--consumer-corpus", out) + self.assertFalse( + AgentConfiguration.objects.filter( + slug__startswith="testdomain-consumer" + ).exists() + ) + + def test_consumer_corpus_without_a_consumer_agent_is_an_error(self): + """The mirror mistake: a binding with nothing to bind.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run(self._standard_registry(), consumer_corpus=corpus.pk) + self.assertIn("declares no", str(ctx.exception)) + + def test_consumer_agent_must_be_extend(self): + """REPLACE would overwrite the consuming corpus's own persona with + text shipped by a third party — the coupling DOMAIN_PACKS.md forbids.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._consumer_registry(spec_overrides={"mode": "REPLACE"}), + consumer_corpus=corpus.pk, + ) + self.assertIn("C8", str(ctx.exception)) + self.assertIn("EXTEND", str(ctx.exception)) + corpus.refresh_from_db() + self.assertIsNone(corpus.default_agent_id) + + def test_consumer_agent_that_never_names_the_group_slug_fails(self): + """Same rule the orchestrator is held to: search_across_corpora takes + the slug as a required argument, so an agent never told it cannot call + it — and that is indistinguishable from choosing not to.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._consumer_registry(text="Answer from this corpus."), + consumer_corpus=corpus.pk, + ) + self.assertIn("never name the group slug", str(ctx.exception)) + + def test_ungrantable_consumer_tool_is_refused(self): + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._consumer_registry( + spec_overrides={"tools": ["search_across_corpora", "rm_rf"]} + ), + consumer_corpus=corpus.pk, + ) + self.assertIn("rm_rf", str(ctx.exception)) + + def test_consumer_agent_install_is_idempotent(self): + """C6 — re-running converges rather than duplicating.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + tarball = self._consumer_registry() + self._run(tarball, consumer_corpus=corpus.pk) + self._run(tarball, consumer_corpus=corpus.pk) + self.assertEqual( + AgentConfiguration.objects.filter( + slug=f"testdomain-consumer-{corpus.pk}" + ).count(), + 1, + ) + + def test_missing_consumer_instructions_file_fails(self): + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._consumer_registry( + spec_overrides={"instructions_file": "nope.txt"} + ), + consumer_corpus=corpus.pk, + ) + self.assertIn("unreadable", str(ctx.exception)) + + # Each of the following exercises a C8 guard. A guard with no test is + # indistinguishable from a guard that never fires. + + def test_consumer_agent_that_is_not_a_mapping_is_refused(self): + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._standard_registry( + consumer_agent="just a string", + _extra_files={"consumer.txt": self.CONSUMER_TEXT}, + ), + consumer_corpus=corpus.pk, + ) + self.assertIn("must be a mapping", str(ctx.exception)) + + def test_consumer_agent_without_instructions_file_is_refused(self): + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._standard_registry( + consumer_agent={"mode": "EXTEND", "tools": []}, + _extra_files={"consumer.txt": self.CONSUMER_TEXT}, + ), + consumer_corpus=corpus.pk, + ) + self.assertIn("instructions_file is required", str(ctx.exception)) + + def test_empty_consumer_instructions_file_is_refused(self): + """An empty file is a likelier authoring mistake than an intentional + no-op, and an agent with no increment is not worth binding.""" + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + with self.assertRaises(CommandError) as ctx: + self._run( + self._consumer_registry(text=" \n\n "), + consumer_corpus=corpus.pk, + ) + self.assertIn("is empty", str(ctx.exception)) + + def test_consumer_agent_preferred_llm_is_threaded(self): + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + self._run( + self._consumer_registry( + spec_overrides={"preferred_llm": "anthropic:claude-haiku-4-5"} + ), + consumer_corpus=corpus.pk, + ) + cfg = AgentConfiguration.objects.get(slug=f"testdomain-consumer-{corpus.pk}") + self.assertEqual(cfg.preferred_llm, "anthropic:claude-haiku-4-5") + + def test_consumer_corpus_that_does_not_exist_is_an_error(self): + with self.assertRaises(CommandError) as ctx: + self._run(self._consumer_registry(), consumer_corpus=99999999) + self.assertIn("does not exist", str(ctx.exception)) + + def test_unusable_consumer_preferred_llm_fails_before_any_write(self): + """The C3-style guard `test_unusable_preferred_llm_fails_before_any_write` + enforces for the orchestrator, mirrored for the consumer agent. + + `AgentConfiguration.save()` raises Django's ValidationError, not + CommandError — nothing catches that. Without validating first, a + typo'd `consumer_agent.preferred_llm` would hit `consumer.save()` and + surface as a bare traceback instead of a C8-prefixed diagnosis. + """ + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + tarball = self._consumer_registry( + spec_overrides={"preferred_llm": "not-a-registered-provider:nope"} + ) + with self.assertRaises(CommandError) as ctx: + self._run(tarball, consumer_corpus=corpus.pk) + self.assertIn("C8", str(ctx.exception)) + self.assertIn("preferred_llm", str(ctx.exception)) + corpus.refresh_from_db() + self.assertIsNone(corpus.default_agent_id) + + def test_wire_consumer_agent_still_refuses_a_bad_mode_if_preflight_is_bypassed( + self, + ): + """Defense in depth, mirroring + `test_wire_still_refuses_an_over_cap_group_if_preflight_is_bypassed`: + `_preflight` now evaluates the same file-decidable C8 checks first, so + on every OTHER test above `_wire_consumer_agent`'s own copy never + fires — `handle()` raises from `_preflight` before `_wire` runs at + all. Calling `_wire` directly, as the C2 bypass test does, proves the + writer-side copy still independently refuses a REPLACE-mode consumer + agent should a future refactor ever call `_wire` without `_preflight` + first. + """ + corpus = Corpus.objects.create(title="Docs", creator=self.owner) + domain_dir = self.root / "registry" / "domains" / "bypassdomain" + domain_dir.mkdir(parents=True) + (domain_dir / "consumer.txt").write_text(self.CONSUMER_TEXT, encoding="utf-8") + manifest = { + "name": "bypassdomain", + "corpus_group": {"slug": "test-group", "title": "G"}, + "consumer_agent": { + "instructions_file": "consumer.txt", + "tools": ["search_across_corpora"], + "mode": "REPLACE", + }, + } + options = { + "creator": "domainowner", + "public": False, + "consumer_corpus": corpus.pk, + } + with self.assertRaises(CommandError) as ctx: + DomainPackCommand(stdout=StringIO())._wire( + manifest, {}, domain_dir, options + ) + message = str(ctx.exception) + self.assertIn("C8", message) + self.assertIn("EXTEND", message) + corpus.refresh_from_db() + self.assertIsNone(corpus.default_agent_id) + + # ---- --check must preview C8 too ------------------------------------ # + # + # Every one of these is decidable from the files alone — none of them + # need `--consumer-corpus` — so `--check` must catch them before a real + # install ever reaches `_wire_consumer_agent`, the same guarantee C1-C7 + # already give. + + def test_check_previews_a_bad_consumer_agent_mode(self): + tarball = self._consumer_registry(spec_overrides={"mode": "REPLACE"}) + with self.assertRaises(CommandError) as ctx: + self._run(tarball, check=True) + self.assertIn("C8", str(ctx.exception)) + self.assertIn("EXTEND", str(ctx.exception)) + self.assertFalse(CorpusGroup.objects.filter(slug="test-group").exists()) + + def test_check_previews_a_missing_consumer_instructions_file(self): + tarball = self._consumer_registry( + spec_overrides={"instructions_file": "nope.txt"} + ) + with self.assertRaises(CommandError) as ctx: + self._run(tarball, check=True) + self.assertIn("unreadable", str(ctx.exception)) + + def test_check_previews_an_ungrantable_consumer_tool(self): + tarball = self._consumer_registry( + spec_overrides={"tools": ["search_across_corpora", "rm_rf"]} + ) + with self.assertRaises(CommandError) as ctx: + self._run(tarball, check=True) + self.assertIn("rm_rf", str(ctx.exception)) + + def test_check_previews_a_consumer_agent_missing_the_group_slug(self): + tarball = self._consumer_registry(text="Answer from this corpus.") + with self.assertRaises(CommandError) as ctx: + self._run(tarball, check=True) + self.assertIn("never name the group slug", str(ctx.exception)) + + def test_check_previews_an_unusable_consumer_preferred_llm(self): + tarball = self._consumer_registry( + spec_overrides={"preferred_llm": "not-a-registered-provider:nope"} + ) + with self.assertRaises(CommandError) as ctx: + self._run(tarball, check=True) + self.assertIn("C8", str(ctx.exception)) + self.assertIn("preferred_llm", str(ctx.exception)) + + def test_check_reports_the_consumer_agent_plan(self): + """A valid `consumer_agent` must be named in the `--check` plan, not + just C1-C7 — zero mention was zero signal for the pack author.""" + out = self._run(self._consumer_registry(), check=True) + self.assertIn("No changes were written", out) + self.assertIn("consumer_agent", out) + self.assertIn("EXTEND", out) diff --git a/opencontractserver/tests/websocket/test_unified_agent_consumer.py b/opencontractserver/tests/websocket/test_unified_agent_consumer.py index a2378f1ce0..992bac3052 100644 --- a/opencontractserver/tests/websocket/test_unified_agent_consumer.py +++ b/opencontractserver/tests/websocket/test_unified_agent_consumer.py @@ -839,7 +839,9 @@ class UnifiedAgentConsumerAgentConfigThreadingTestCase(WebsocketFixtureBaseTestC factory's context-derived prompt and tool set untouched. """ - def _make_corpus_consumer(self, *, explicit: bool) -> UnifiedAgentConsumer: + def _make_corpus_consumer( + self, *, explicit: bool, mode: str = "REPLACE" + ) -> UnifiedAgentConsumer: consumer = UnifiedAgentConsumer() consumer.session_id = "test-session" consumer.user_id = self.user.id @@ -850,7 +852,7 @@ def _make_corpus_consumer(self, *, explicit: bool) -> UnifiedAgentConsumer: consumer.agent_config = SimpleNamespace( preferred_llm="", system_instructions="You are the cross-corpus orchestrator.", - system_instructions_mode="REPLACE", + system_instructions_mode=mode, available_tools=["search_across_corpora"], ) # ``agent_config_id`` is only set when the client passed @@ -874,9 +876,14 @@ async def test_explicit_agent_threads_instructions_and_tools(self) -> None: self.assertEqual(kwargs.get("tools"), ["search_across_corpora"]) async def test_default_agent_keeps_factory_prompt_and_tools(self) -> None: - """A slug-resolved default agent must NOT override the factory's - context-derived prompt (e.g. ``corpus_agent_instructions``) or tools.""" - consumer = self._make_corpus_consumer(explicit=False) + """A slug-resolved REPLACE default must NOT override the factory's + context-derived prompt (e.g. ``corpus_agent_instructions``) or tools. + + REPLACE is the case the exclusion was written for: a generic default's + instructions would substitute for the corpus persona rather than add + to it. + """ + consumer = self._make_corpus_consumer(explicit=False, mode="REPLACE") with patch( "config.websocket.consumers.unified_agent_conversation.agents.for_corpus" ) as mock_for_corpus: @@ -886,6 +893,30 @@ async def test_default_agent_keeps_factory_prompt_and_tools(self) -> None: self.assertNotIn("system_prompt", kwargs) self.assertNotIn("tools", kwargs) + async def test_extend_default_agent_is_applied_on_the_fallback_path(self) -> None: + """An EXTEND default DOES reach the factory without ``?agent_id=``. + + The exclusion above exists because REPLACE would clobber the corpus + persona. EXTEND appends instead, so there is nothing to clobber — and + excluding it would mean a corpus default that resolves and is then + silently ignored. It must arrive as ``extra_system_context``: setting + ``system_prompt`` would consume the "caller supplied nothing" signal + and discard the persona, which is the #2247 bug. + """ + consumer = self._make_corpus_consumer(explicit=False, mode="EXTEND") + with patch( + "config.websocket.consumers.unified_agent_conversation.agents.for_corpus" + ) as mock_for_corpus: + mock_for_corpus.return_value = _StubAgent(lambda: iter(())) + await consumer._initialize_agent() + kwargs = mock_for_corpus.call_args.kwargs + self.assertEqual( + kwargs.get("extra_system_context"), + "You are the cross-corpus orchestrator.", + ) + self.assertNotIn("system_prompt", kwargs) + self.assertEqual(kwargs.get("tools"), ["search_across_corpora"]) + async def test_explicit_agent_tools_merge_with_extra_tools(self) -> None: """Config tools come first, per-turn delegation extra_tools append.""" consumer = self._make_corpus_consumer(explicit=True) @@ -1068,3 +1099,74 @@ async def fake_send_safe(msg_type, content="", data=None): await consumer.receive(json.dumps({"query": "Legacy agent query"})) self.assertTrue(stream_called) + + +@override_settings(USE_AUTH0=False) +@pytest.mark.django_db(transaction=True) +class UnifiedAgentConsumerCorpusDefaultAgentTestCase(WebsocketFixtureBaseTestCase): + """Priority 3 prefers ``Corpus.default_agent`` over the global slug. + + Before this field existed, a chat opened with no ``agent_id`` always + resolved ``default-corpus-agent``, so an AgentConfiguration scoped to the + corpus could never be its default however it was configured. + + TransactionTestCase-backed on purpose: resolution reads the corpus on its + own connection, so the rows have to be committed. A plain ``TestCase`` + wraps them in a transaction the lookup cannot see and fails with + "connection already closed". + """ + + def _consumer(self) -> UnifiedAgentConsumer: + consumer = UnifiedAgentConsumer() + consumer.session_id = "test-session" + consumer.agent_config_id = None + consumer.document_id = None + consumer.corpus_id = self.corpus.id + return consumer + + def _scoped_agent(self, slug: str, *, active: bool = True): + from opencontractserver.agents.models import AgentConfiguration + + agent = AgentConfiguration.objects.create( + name=slug, + slug=slug, + creator=self.user, + scope=AgentConfiguration.SCOPE_CORPUS, + corpus=self.corpus, + system_instructions="specific", + system_instructions_mode="EXTEND", + is_active=active, + ) + self.corpus.default_agent = agent + self.corpus.save(update_fields=["default_agent"]) + return agent + + async def test_falls_back_to_the_global_slug_when_unset(self) -> None: + resolved = await self._consumer()._resolve_agent_config() + assert resolved is not None + self.assertEqual(resolved.slug, "default-corpus-agent") + + async def test_corpus_default_wins(self) -> None: + await database_sync_to_async(self._scoped_agent)("corpus-default") + resolved = await self._consumer()._resolve_agent_config() + assert resolved is not None + self.assertEqual(resolved.slug, "corpus-default") + + async def test_inactive_corpus_default_falls_back_rather_than_failing( + self, + ) -> None: + """Switching a corpus agent off should restore the global default, not + break corpus chat — that is what deactivating it is asking for.""" + await database_sync_to_async(self._scoped_agent)( + "corpus-default-off", active=False + ) + resolved = await self._consumer()._resolve_agent_config() + assert resolved is not None + self.assertEqual(resolved.slug, "default-corpus-agent") + + async def test_no_corpus_and_no_document_resolves_nothing(self) -> None: + """Neither context in scope: there is nothing to resolve, and the + caller must not receive a default it did not ask for.""" + consumer = self._consumer() + consumer.corpus_id = None + self.assertIsNone(await consumer._resolve_agent_config())