diff --git a/benchmarks/src/basic_memory_benchmarks/agent_tasks/driver.py b/benchmarks/src/basic_memory_benchmarks/agent_tasks/driver.py index 6847021f3..227d4d7c6 100644 --- a/benchmarks/src/basic_memory_benchmarks/agent_tasks/driver.py +++ b/benchmarks/src/basic_memory_benchmarks/agent_tasks/driver.py @@ -258,9 +258,11 @@ def _prepare_task_project( question is untenable, and grouped runs are read-only so reuse cannot leak state between tasks. """ + # The CLI already prefixes generated run ids with "at-"; prepending another + # literal here produced "at-at-..." project names in the first real run. if task.group is None: project = TaskProject( - name=f"at-{run_id}-{task.id}", directory=surface_home / "projects" / task.id + name=f"{run_id}-{task.id}", directory=surface_home / "projects" / task.id ) source_dir = corpus_dir else: @@ -268,11 +270,17 @@ def _prepare_task_project( if cached is not None: return cached project = TaskProject( - name=f"at-{run_id}-{task.group}", directory=surface_home / "projects" / task.group + name=f"{run_id}-{task.group}", directory=surface_home / "projects" / task.group ) source_dir = corpus_dir / task.group / "docs" copy_corpus(source_dir, project.directory) run_command(prefix + ["project", "add", project.name, str(project.directory)], env=env) + # `project add` registers but does not index: without an explicit index + # pass the DB stays empty, `status` reports ready vacuously (zero pending + # work was ever queued), and every retrieval tool sees an empty project. + # The scripted smoke masked this — its canned answers never needed the + # index — so the first real-model run surfaced it as 24 empty projects. + run_command(prefix + ["reindex", "-p", project.name, "--full", "--search"], env=env) settle_index( prefix=prefix, env=env, @@ -321,6 +329,16 @@ def dispatch(name: str, arguments: dict[str, Any]) -> ToolOutcome: budget=config.budget, ) if needs_state: + # Trigger: the task is graded on project state (files, SQLite index). + # Why: a wikilink written mid-loop lands as an unresolved relation row; + # BM resolves forward references in a later index pass, and settle only + # watches file-sync work — grading straight after settle raced that + # pass (first real run: rich curate-connect failed RelationResolves + # despite a correct, project-scoped edit_note). + # Outcome: re-run the project index (its completion step resolves + # forward references — same pass _prepare_task_project relies on), + # then settle, so graders read converged state. + run_command(prefix + ["reindex", "-p", project.name, "--search"], env=env) settle_index( prefix=prefix, env=env, @@ -549,7 +567,7 @@ def _write_jsonl(path: Path, rows: list[dict]) -> None: def run_agent_tasks( config: AgentTasksConfig, *, - model_factory: Callable[[str], ToolAgentModel] = create_tool_agent_model, + model_factory: Callable[[str], ToolAgentModel] | None = None, session_factory: Callable[[SurfaceRuntime], AgentSession] | None = None, judge_factory: Callable[[str], LLMRunner] = create_runner, ) -> Path: @@ -598,7 +616,19 @@ def run_agent_tasks( # Model and judge parse before any on-disk state is created, so a bad spec # fails fast without leaving an empty benchmark home behind. - model = model_factory(config.model_spec) + # Trigger: a programmatic caller builds AgentTasksConfig and calls this + # directly, rather than through the CLI, which pre-binds temperature. + # Why: manifest.json records config.model_temperature, so constructing the + # model without it would run at the factory default while the artifact + # claimed the configured value — a silent provenance lie. + # Outcome: the default path carries the recorded temperature; an injected + # factory owns its own configuration and is called unchanged. The default + # is a None sentinel rather than the function itself so the branch reads + # the module attribute at call time, which is also what makes it testable. + if model_factory is None: + model = create_tool_agent_model(config.model_spec, temperature=config.model_temperature) + else: + model = model_factory(config.model_spec) judge = judge_factory(config.judge_spec) if config.judge_spec else None build_session = session_factory or (lambda runtime: McpAgentSession(runtime)) diff --git a/benchmarks/src/basic_memory_benchmarks/agent_tasks/grading.py b/benchmarks/src/basic_memory_benchmarks/agent_tasks/grading.py index ab9ec87b7..c0ecda994 100644 --- a/benchmarks/src/basic_memory_benchmarks/agent_tasks/grading.py +++ b/benchmarks/src/basic_memory_benchmarks/agent_tasks/grading.py @@ -108,6 +108,17 @@ def normalize_answer_item(value: str) -> str: return value.strip().lstrip("/").removesuffix(".md").lower() +def strip_own_project_prefix(item: str, project_name: str) -> str: + """Drop the task's OWN project-name prefix from a normalized answer item. + + Agents quote permalinks exactly as tools return them — ``/`` + — while gold values are project-relative. Only this task's project name is + stripped: an item carrying a DIFFERENT project's prefix is genuinely wrong + (cross-project leakage) and must keep failing. + """ + return item.removeprefix(normalize_answer_item(project_name) + "/") + + # --- File helpers --- @@ -193,7 +204,7 @@ def _eval_answer_set(grader: AnswerSetEquals, ctx: GradingContext) -> PredicateR raw = payload.get(grader.key) if not isinstance(raw, list) or not all(isinstance(item, str) for item in raw): return _result(grader, False, f"answer key '{grader.key}' is not a list of strings") - got = {normalize_answer_item(item) for item in raw} + got = {strip_own_project_prefix(normalize_answer_item(item), ctx.project_name) for item in raw} gold = {normalize_answer_item(item) for item in grader.gold} if got == gold: return _result(grader, True, f"{len(gold)} items match") @@ -271,21 +282,31 @@ def _eval_relation_resolves(grader: RelationResolves, ctx: GradingContext) -> Pr if row is None: raise RuntimeError(f"Project '{ctx.project_name}' not found in {ctx.db_path}") project_id = int(row[0]) + # Stored permalinks are project-prefixed (verified against a live run + # DB: 'at--/notes/redis-cache-tuning'), while task specs + # use project-relative gold — match either form, same policy as + # strip_own_project_prefix for answer-set graders. query = ( "SELECT target.permalink, r.relation_type" " FROM relation r" " JOIN entity source ON r.from_id = source.id" " JOIN entity target ON r.to_id = target.id" - " WHERE source.project_id = ? AND source.permalink = ?" + " WHERE source.project_id = ? AND source.permalink IN (?, ?)" " AND r.to_id IS NOT NULL" ) - rows = connection.execute(query, (project_id, grader.source_permalink)).fetchall() + prefixed_source = f"{ctx.project_name}/{grader.source_permalink}" + rows = connection.execute( + query, (project_id, grader.source_permalink, prefixed_source) + ).fetchall() finally: connection.close() targets = {normalize_answer_item(item) for item in grader.targets} for target_permalink, relation_type in rows: - if normalize_answer_item(str(target_permalink)) not in targets: + resolved = strip_own_project_prefix( + normalize_answer_item(str(target_permalink)), ctx.project_name + ) + if resolved not in targets: continue if grader.relation_type is not None and relation_type != grader.relation_type: continue diff --git a/benchmarks/src/basic_memory_benchmarks/agent_tasks/models.py b/benchmarks/src/basic_memory_benchmarks/agent_tasks/models.py index 33d4ab632..9907b3064 100644 --- a/benchmarks/src/basic_memory_benchmarks/agent_tasks/models.py +++ b/benchmarks/src/basic_memory_benchmarks/agent_tasks/models.py @@ -2,9 +2,10 @@ from __future__ import annotations +import re from typing import Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator from basic_memory_benchmarks.models import RuntimeInfo @@ -12,6 +13,14 @@ # partial accounting is kept on the errored row, never counted as a budget stop. StopReason = Literal["final", "turns", "tokens", "wall_clock", "error"] +# run_id is not merely a label. The driver splices it into a `bm project add` +# argv token (project names are `{run_id}-{task.id}`) and into two filesystem +# paths (the benchmark home and the run dir), so it must be safe as both. A +# leading "-" makes the BM CLI parse the project name as options and abort with +# "No such option: -t" — naming a flag the operator never typed — and a path +# separator would place run artifacts outside the run dir. +RUN_ID_PATTERN = re.compile(r"[A-Za-z0-9_][A-Za-z0-9._-]*") + class AgentBudget(BaseModel): """Per-task budgets; identical across surfaces (fairness contract).""" @@ -29,6 +38,15 @@ class AgentTasksConfig(BaseModel): # task set. Manifest runs are grouped and read-only (see driver). task_manifest: str | None = None model_spec: str + # None = the temperature parameter is omitted from requests entirely + # (Claude 5 endpoints reject it). Provenance: the value the model factory + # used for this run. + # allow_inf_nan=False because nan/inf survive float() and Pydantic's plain + # float schema: they would reach the request body, where httpx raises a bare + # ValueError mid-run. Worse for provenance, model_dump_json() encodes them + # as null — which is exactly this field's "temperature omitted" sentinel, so + # the recorded run config would silently misreport what was sent. + model_temperature: float | None = Field(default=0.0, allow_inf_nan=False) judge_spec: str | None = None corpus_dir: str = "benchmarks/datasets/agent-tasks/corpus" output_root: str = "benchmarks/runs" @@ -39,6 +57,23 @@ class AgentTasksConfig(BaseModel): settle_timeout_seconds: float = Field(default=180.0, gt=0.0) allow_surface_skip: bool = True + @field_validator("run_id") + @classmethod + def _run_id_is_argv_and_path_safe(cls, value: str) -> str: + # Rejected here rather than in the driver so the run dies before any + # setup cost: without this the first `bm project add` fails only after + # the benchmark home, a warm `bm mcp` subprocess, and a full corpus copy + # exist — and run_command captures stderr, so the operator sees a bare + # CalledProcessError exit status, not the CLI's own complaint. The + # abandoned home then blocks re-running the same run_id. + if not RUN_ID_PATTERN.fullmatch(value): + raise ValueError( + "run_id must start with a letter, digit, or underscore and use only " + "letters, digits, '.', '_', or '-'; it becomes both a CLI argument " + f"and a path component, got {value!r}" + ) + return value + class TurnRecord(BaseModel): """One model turn or one tool dispatch inside a task's agent loop.""" diff --git a/benchmarks/src/basic_memory_benchmarks/cli.py b/benchmarks/src/basic_memory_benchmarks/cli.py index 46814532e..e122d082a 100644 --- a/benchmarks/src/basic_memory_benchmarks/cli.py +++ b/benchmarks/src/basic_memory_benchmarks/cli.py @@ -3,11 +3,14 @@ from __future__ import annotations import json +import math import shutil import uuid +from functools import partial from pathlib import Path import typer +from pydantic import ValidationError from rich.console import Console from basic_memory_benchmarks.agent_tasks.driver import run_agent_tasks @@ -465,6 +468,21 @@ def run_agent_tasks_command( "--model", help="Agent under test: openai-compat:@ | scripted:", ), + model_header: list[str] | None = typer.Option( + None, + "--model-header", + help="Extra HTTP header for the agent endpoint as 'Name=value' (repeatable); " + "e.g. anthropic-workspace-id=wrkspc_... for identity-linked Anthropic keys. " + "Header names are matched case-insensitively, so 'authorization=...' " + "replaces the bearer derived from OPENAI_API_KEY rather than joining it. " + "Values are never recorded in run artifacts.", + ), + model_temperature: str = typer.Option( + "0", + "--model-temperature", + help="Sampling temperature for the agent endpoint, or 'omit' to send none " + "(Claude 5 models reject the parameter). Recorded in the run config.", + ), judge: str | None = typer.Option( None, "--judge", @@ -535,10 +553,38 @@ def run_agent_tasks_command( except (ValueError, FileNotFoundError) as exc: raise typer.BadParameter(str(exc)) from exc + # Extra endpoint headers stay out of AgentTasksConfig (and therefore out + # of run artifacts): values may be sensitive, so they ride only in the + # model factory closure below. + header_pairs: dict[str, str] = {} + for raw_header in model_header or []: + name, separator, value = raw_header.partition("=") + if not separator or not name.strip() or not value.strip(): + raise typer.BadParameter(f"--model-header must be 'Name=value', got {raw_header!r}") + header_pairs[name.strip()] = value.strip() + + if model_temperature.strip().lower() in {"omit", "none"}: + temperature: float | None = None + else: + try: + temperature = float(model_temperature) + except ValueError: + raise typer.BadParameter( + f"--model-temperature must be a number or 'omit', got {model_temperature!r}" + ) from None + # nan/inf parse cleanly and pass config validation, but JSON has no + # encoding for them: httpx raises a bare ValueError when it serializes + # the request body. That escapes _post's handled transports, so the run + # dies mid-flight after surface setup with no artifacts written. + if not math.isfinite(temperature): + raise typer.BadParameter( + f"--model-temperature must be a finite number or 'omit', got {model_temperature!r}" + ) + # Fail fast at parse time: a bad model spec (including claude:) and a # missing judge for judge-graded tasks must not survive to mid-run. try: - create_tool_agent_model(model) + create_tool_agent_model(model, extra_headers=header_pairs or None, temperature=temperature) except ValueError as exc: raise typer.BadParameter(str(exc)) from exc judged = [ @@ -550,27 +596,41 @@ def run_agent_tasks_command( raise typer.BadParameter(f"Tasks {judged} use judge_rubric graders; pass --judge") resolved_run_id = run_id or f"at-{uuid.uuid4().hex[:12]}" - config = AgentTasksConfig( - run_id=resolved_run_id, - surfaces=surface_list, - task_ids=task_ids, - task_manifest=str(task_manifest) if task_manifest is not None else None, - model_spec=model, - judge_spec=judge, - corpus_dir=str(corpus_dir), - output_root=str(output_root), - bm_source=bm_source, - bm_local_path=str(bm_local_path), - budget=AgentBudget( - max_turns=max_turns, - max_total_tokens=max_total_tokens, - max_task_seconds=task_timeout, + # AgentTasksConfig owns the field rules (finite temperature, argv/path-safe + # run_id) so the direct run_agent_tasks(config) path is guarded too. Render + # its rejection as a CLI parameter error instead of a Pydantic traceback. + try: + config = AgentTasksConfig( + run_id=resolved_run_id, + surfaces=surface_list, + task_ids=task_ids, + task_manifest=str(task_manifest) if task_manifest is not None else None, + model_spec=model, + model_temperature=temperature, + judge_spec=judge, + corpus_dir=str(corpus_dir), + output_root=str(output_root), + bm_source=bm_source, + bm_local_path=str(bm_local_path), + budget=AgentBudget( + max_turns=max_turns, + max_total_tokens=max_total_tokens, + max_task_seconds=task_timeout, + ), + tool_timeout_seconds=tool_timeout, + settle_timeout_seconds=settle_timeout, + allow_surface_skip=allow_surface_skip, + ) + except ValidationError as exc: + raise typer.BadParameter(str(exc)) from exc + run_dir = run_agent_tasks( + config, + model_factory=partial( + create_tool_agent_model, + extra_headers=header_pairs or None, + temperature=temperature, ), - tool_timeout_seconds=tool_timeout, - settle_timeout_seconds=settle_timeout, - allow_surface_skip=allow_surface_skip, ) - run_dir = run_agent_tasks(config) console.print(f"Agent-task run complete: [green]{run_dir}[/green]") console.print(f"See [cyan]{run_dir / 'summary.md'}[/cyan]") diff --git a/benchmarks/src/basic_memory_benchmarks/converters/beam_to_corpus.py b/benchmarks/src/basic_memory_benchmarks/converters/beam_to_corpus.py index 6a4e86d1f..183b6c759 100644 --- a/benchmarks/src/basic_memory_benchmarks/converters/beam_to_corpus.py +++ b/benchmarks/src/basic_memory_benchmarks/converters/beam_to_corpus.py @@ -42,11 +42,17 @@ BEAM_CITATION = "BEAM: Beyond a Million Tokens (ICLR 2026, arXiv 2510.27246)" BEAM_LICENSE_NOTE = "Code MIT; benchmark data CC BY-SA 4.0" -# Trailing probe-index marker in message content (e.g. "... ->-> 1,2"). The -# second field is not always numeric — upstream 100K/1/chat.json ends one -# message with "->-> 2,N/A" — so both fields match any non-space token. It -# links transcript text back to probe indices, so it must never be ingested. -_INDEX_MARKER_PATTERN = re.compile(r"\s*->->\s*\S+,\S+\s*$") +# Trailing probe-index marker in message content: "->->" followed by a +# comma-separated id list. A survey of every marker in the 100K tier +# (2,199 total) found exactly five shapes: " 1,1" (2190), " 1,5)" (6), +# " 2,N/A" (1), " 2,22, 24" (1), and a double-space variant (1). The +# first id is always an int; later ids are ints or N/A, spaces around +# commas optional; the trailing ")" is generator junk (every such message +# contains zero opening parens), so it strips with the marker. Kept this +# narrow deliberately — anything else containing "->->" trips the +# fail-fast below instead of silently stripping content. The marker links +# transcript text back to probe indices, so it must never be ingested. +_INDEX_MARKER_PATTERN = re.compile(r"\s*->->\s*\d+(?:\s*,\s*(?:\d+|N/A))*\)?\s*$") @dataclass(frozen=True) diff --git a/benchmarks/src/basic_memory_benchmarks/datasets/beam.py b/benchmarks/src/basic_memory_benchmarks/datasets/beam.py index d5785b614..6c95fa16a 100644 --- a/benchmarks/src/basic_memory_benchmarks/datasets/beam.py +++ b/benchmarks/src/basic_memory_benchmarks/datasets/beam.py @@ -140,8 +140,11 @@ def _normalize_source_chat_ids(value: object, *, ability: str, conv_dir: Path) - """Normalize the per-probe evidence ids to a flat sorted list. Usually a list of ints, but knowledge_update ships a dict (e.g. - ``{"original_info": [86], "updated_info": [114]}``) and abstention omits - the field entirely — normalize to dict-values union / empty list. + ``{"original_info": [86], "updated_info": [114]}``), abstention omits + the field entirely, and event_ordering mixes ints with one level of + int-list groups when a single event's evidence spans chats (observed in + the live 100K tier: ``[116, 118, ..., [136, 138]]``) — normalize to the + flattened union / empty list. """ if value is None: return [] @@ -161,11 +164,16 @@ def _normalize_source_chat_ids(value: object, *, ability: str, conv_dir: Path) - ) chat_ids: set[int] = set() for item in merged: - if not isinstance(item, int): - raise ValueError( - f"BEAM {ability} source_chat_ids must contain ints in {conv_dir}: {item!r}" - ) - chat_ids.add(item) + # One level of nesting only: an int-list group is an event whose + # evidence spans chats; anything deeper or non-int still fails fast. + group_items = item if isinstance(item, list) else [item] + for chat_id in group_items: + if not isinstance(chat_id, int): + raise ValueError( + f"BEAM {ability} source_chat_ids must contain ints or int lists " + f"in {conv_dir}: {item!r}" + ) + chat_ids.add(chat_id) return sorted(chat_ids) diff --git a/benchmarks/src/basic_memory_benchmarks/llm/tool_agent.py b/benchmarks/src/basic_memory_benchmarks/llm/tool_agent.py index d471df5d8..a16e4ab5c 100644 --- a/benchmarks/src/basic_memory_benchmarks/llm/tool_agent.py +++ b/benchmarks/src/basic_memory_benchmarks/llm/tool_agent.py @@ -19,6 +19,7 @@ import json import os +import re import time from abc import ABC, abstractmethod from collections.abc import Mapping, Sequence @@ -96,6 +97,136 @@ def describe(self) -> dict[str, str]: # --- OpenAI-compatible transport --- +REDACTION_MARKER = "[redacted]" + +# Substituted for the whole body when masking provably failed to remove a +# secret. Dropping the diagnostic is the intended trade: the operator still +# gets the HTTP status and URL from the underlying exception, which is what +# names the rejection, while the credential never reaches an artifact. +WITHHELD_BODY_MARKER = "[body withheld: a secret survived masking in an unrecognized encoding]" + +# A gateway quotes the offending value into its JSON error body once; a proxy +# that wraps an upstream JSON body in a string field quotes it twice. Nothing +# an OpenAI-compatible endpoint emits nests deeper than that, and each level is +# strictly longer than the last, so two is where the form set stops earning its +# keep. +_MAX_JSON_ESCAPE_DEPTH = 2 + + +def _encoded_forms(secret: str) -> list[str]: + """Every spelling ``secret`` can take in an error body. + + HTTP allows any visible ASCII character in a header value, so an operator + may pass a ``--model-header`` secret containing ``"`` or ``\\``. Echoed + inside a JSON error body such a value arrives *escaped* — ``"`` as ``\\"``, + ``\\`` as ``\\\\`` — and a search for the plaintext never matches it, so + the credential would ride the body into the artifact intact. + + A value with nothing to escape yields the plaintext at every level, so the + common alphanumeric key still costs a single replacement. + """ + forms = [secret] + for _ in range(_MAX_JSON_ESCAPE_DEPTH): + # json.dumps wraps its result in quotes; [1:-1] drops them to leave the + # escaped payload exactly as it appears inside a surrounding JSON + # string. Re-applying it models one more level of nesting. + forms.append(json.dumps(forms[-1])[1:-1]) + return forms + + +# JSON lets an encoder spell any character either literally or as a backslash +# escape, and encoders disagree about which: Go's html-safe default emits +# \u003c for "<", PHP emits \/ for "/". Enumerating those spellings is +# unwinnable — any encoder may \u-escape any character — so detection runs +# against an unescaped view of the text instead of a longer form list. +_ESCAPE_SEQUENCE = re.compile(r"\\u[0-9a-fA-F]{4}|\\.", re.DOTALL) + +_CONTROL_ESCAPES = {"b": "\b", "f": "\f", "n": "\n", "r": "\r", "t": "\t"} + + +def _unescape_once(text: str) -> str: + """Resolve one layer of backslash escapes. Parses nothing, raises nothing. + + ``json.loads`` is deliberately not used here: an error body is truncated to + 300 characters downstream, arrives cut mid-string when the gateway itself + truncates, and is often not JSON at all — a parse failure inside a leak + backstop would trade a leak for a crash. + + Every alternative the pattern matches is longer than its replacement, so + each pass strictly shortens the text. That is what bounds the fixpoint loop + in ``redact_secrets``. + """ + + def resolve(match: re.Match[str]) -> str: + sequence = match.group() + # ``\uXXXX`` is the only alternative longer than two characters, so + # length alone identifies it — no re-inspection of the payload needed. + if len(sequence) == 6: + return chr(int(sequence[2:], 16)) + escaped = sequence[1] + # ``\"``, ``\\`` and ``\/`` spell themselves. Anything else is invalid + # JSON escape syntax, and dropping the backslash is the wider reading: + # a detector that over-matches costs a diagnostic, not a credential. + return _CONTROL_ESCAPES.get(escaped, escaped) + + return _ESCAPE_SEQUENCE.sub(resolve, text) + + +def redact_secrets(text: str, secrets: Sequence[str]) -> str: + """Return text that is safe to persist, masking or withholding secrets. + + Gateways quote the offending request back at you: an OpenAI-compatible 401 + can echo the rejected key, and proxies name the header they refused. That + body then rides an ``LLMRunnerError`` into ``per-task-agent.jsonl`` and + ``summary.md``, which ``publish`` copies into the public results bundle — + so it is not safe to persist raw. A response body is only the commonest + source: ``OpenAICompatToolAgent._error`` runs every foreign string through + here, including transport exception text, which quotes a rejected header + value without any body being involved. + + Redaction is value-based rather than pattern-based: masking exactly the + values we were handed is deterministic and cannot be defeated by an + unfamiliar credential format. Each value is masked in every spelling + ``_encoded_forms`` derives, because the body that leaks it is usually JSON. + + Masking alone cannot be complete, because any JSON encoder may spell any + character as ``\\uXXXX``. So the masked text is checked once more against + an unescaped view of itself, and a body that still yields a secret there is + dropped whole. The result therefore contains no secret in any spelling + reachable by backslash escapes, at any nesting depth. + """ + # Distinguished from "no secret present": with nothing configured there is + # nothing to mask and nothing for the safety net to search for, so the body + # passes through without paying for either pass. + if not secrets: + return text + + forms = {form for secret in secrets for form in _encoded_forms(secret)} + # Longest first: when one form contains another — a bare key and the same + # key inside a longer header value, or a plaintext value inside its own + # escaped spelling — masking the short one first would leave the remainder + # of the longer value exposed. Ties break on the form itself so that a set + # of equal-length secrets still redacts identically on every run. + for form in sorted(forms, key=lambda form: (-len(form), form)): + text = text.replace(form, REDACTION_MARKER) + + # Safety net for the spellings the form set cannot enumerate. ``forms`` + # always includes the plaintext and ``replace`` removes every occurrence, + # so the masked text provably holds no literal secret; only an *escaped* + # one can remain, and unescaping is what exposes it. + # + # Repeated to a fixpoint because a secret escaped once by an upstream + # encoder and again by the proxy that wrapped its body needs two passes to + # surface. The check runs after every pass rather than only at the end: a + # secret containing a backslash can be revealed by one pass and then + # consumed as an escape prefix by the next. + view = text + while (unescaped := _unescape_once(view)) != view: + view = unescaped + if any(secret in view for secret in secrets): + return WITHHELD_BODY_MARKER + return text + def _transcript_to_messages(transcript: Sequence[TranscriptItem]) -> list[dict[str, Any]]: messages: list[dict[str, Any]] = [] @@ -146,6 +277,8 @@ def __init__( base_url: str, *, api_key: str | None = None, + extra_headers: dict[str, str] | None = None, + temperature: float | None = 0.0, timeout_seconds: float = 300.0, max_retries: int = 2, ) -> None: @@ -153,14 +286,84 @@ def __init__( self.base_url = base_url.rstrip("/") self.spec = f"openai-compat:{model}@{self.base_url}" self._api_key = api_key + # Some endpoints require headers beyond auth — e.g. Anthropic's + # OpenAI-compat layer demands anthropic-workspace-id for + # identity-linked API keys. Values may be sensitive, so they are + # never recorded in run artifacts. + self._extra_headers = dict(extra_headers or {}) + # The exact values that must never reach a run artifact: the bearer + # token and every operator-supplied header value. Every error message + # this class raises is scrubbed against this set in _error, which is + # the one place it constructs an LLMRunnerError. + self._secret_values: tuple[str, ...] = tuple( + value for value in (api_key, *self._extra_headers.values()) if value + ) + # temperature=None omits the parameter entirely: Claude 5 models + # reject any temperature value ("`temperature` is deprecated for this + # model"), while local openai-compat servers (Ollama) default to a + # nonzero sampling temperature unless pinned — so the default stays 0 + # and omission is an explicit operator choice recorded in the config. + self._temperature = temperature self._timeout_seconds = timeout_seconds self._max_retries = max_retries - def _post(self, body: dict[str, Any]) -> dict[str, Any]: - headers = {"Content-Type": "application/json"} + def _request_headers(self) -> httpx.Headers: + """Merge derived auth with operator headers under HTTP's name equality. + + HTTP header names are case-insensitive, so ``authorization`` and + ``Authorization`` are the *same* header. A plain dict does not know + that: merging operator headers into one kept both spellings and httpx + serialized both, leaving the endpoint to pick — and disclosing the + ambient ``OPENAI_API_KEY`` to an endpoint the operator never meant to + hand it to. ``httpx.Headers.__setitem__`` drops every existing entry + with that name, which is exactly the merge HTTP describes. + + Operator headers are applied last and therefore win, rather than being + refused as a conflict: ``--model-header`` is a deliberate choice made + for this run, while the bearer is derived from whatever the shell + happens to export, so the explicit value is the one that should + survive. Refusing the pair would strand the common case of a shell that + exports ``OPENAI_API_KEY`` for unrelated tools. + """ + headers = httpx.Headers({"Content-Type": "application/json"}) if self._api_key: headers["Authorization"] = f"Bearer {self._api_key}" + # Assigned one at a time rather than passed as a mapping: --model-header + # is repeatable and its names are compared case-sensitively at parse, so + # the operator's own dict can spell one header two ways, and building + # Headers from that mapping in one step would keep both entries. + for name, value in self._extra_headers.items(): + headers[name] = value + return headers + + def _error(self, summary: str, detail: str | None = None) -> LLMRunnerError: + """Build this class's only ``LLMRunnerError``, scrubbing the foreign half. + + The credentials this agent holds can leave the process in exactly one + way — inside an error message — and every such message has the same two + parts: a ``summary`` the harness wrote, and a ``detail`` that came from + outside (a transport exception, a gateway body, a model turn). Scrubbing + here rather than at each origin is what makes the guarantee checkable by + reading one method. Three earlier fixes each masked one origin — an + echoed 401 body, its JSON-escaped spelling, its unicode-escaped spelling + — and the next origin arrived unredacted anyway: h11 quotes an illegal + header value into ``LocalProtocolError``, whose text is transport-made + and never passed through the body scrub at all. + + Only ``detail`` is redacted, because ``redact_secrets`` withholds its + whole input when masking provably failed. Keeping the summary outside + that blast radius preserves the trade the withhold marker documents: the + operator still learns which endpoint failed and how, even when the + diagnostic itself has to be dropped. + """ + if detail is None: + return LLMRunnerError(summary) + return LLMRunnerError(f"{summary}: {redact_secrets(detail, self._secret_values)}") + + def _post(self, body: dict[str, Any]) -> dict[str, Any]: + headers = self._request_headers() last_error: Exception | None = None + error_body = "" for _ in range(self._max_retries + 1): try: response = httpx.post( @@ -176,9 +379,18 @@ def _post(self, body: dict[str, Any]) -> dict[str, Any]: return payload except (httpx.HTTPError, KeyError, json.JSONDecodeError) as exc: last_error = exc - raise LLMRunnerError( - f"openai-compat call to {self.base_url} failed after " - f"{self._max_retries + 1} attempts: {last_error}" + # A 4xx/5xx body names the actual rejection (bad model id, + # missing header, quota) — without it the operator sees only + # a bare status code. + # Redact before truncating: a secret straddling the 300-char + # cut would survive as an unmatched prefix that the scrub in + # _error could no longer recognize either. + if isinstance(exc, httpx.HTTPStatusError): + error_body = redact_secrets(exc.response.text, self._secret_values)[:300] + body_suffix = f": {error_body}" if error_body else "" + raise self._error( + f"openai-compat call to {self.base_url} failed after {self._max_retries + 1} attempts", + f"{last_error}{body_suffix}", ) def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef]) -> AgentTurn: @@ -187,8 +399,9 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] "messages": _transcript_to_messages(transcript), "tools": _tools_to_functions(tools), "tool_choice": "auto", - "temperature": 0, } + if self._temperature is not None: + body["temperature"] = self._temperature started = time.perf_counter() payload = self._post(body) latency_ms = (time.perf_counter() - started) * 1000.0 @@ -196,7 +409,7 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] try: message = payload["choices"][0]["message"] except (KeyError, IndexError, TypeError) as exc: - raise LLMRunnerError(f"openai-compat response has no message: {exc}") from exc + raise self._error("openai-compat response has no message", str(exc)) from exc tool_calls: list[ToolCall] = [] for index, raw_call in enumerate(message.get("tool_calls") or []): @@ -207,14 +420,16 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] except json.JSONDecodeError as exc: # Malformed arguments are an explicit task error, never a # silent skip: the loop propagates this to the driver. - raise LLMRunnerError( - f"model returned malformed tool arguments for " - f"'{function.get('name')}': {raw_arguments[:200]}" + # The tool name is endpoint-supplied like the arguments are, so + # it rides in the detail half and is scrubbed with them. + raise self._error( + "model returned malformed tool arguments", + f"'{function.get('name')}': {raw_arguments[:200]}", ) from exc if not isinstance(arguments, dict): - raise LLMRunnerError( - f"model returned non-object tool arguments for " - f"'{function.get('name')}': {raw_arguments[:200]}" + raise self._error( + "model returned non-object tool arguments", + f"'{function.get('name')}': {raw_arguments[:200]}", ) tool_calls.append( ToolCall( @@ -229,7 +444,9 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] # never trip max_total_tokens, so a missing block is an explicit error. usage = payload.get("usage") if not isinstance(usage, dict): - raise LLMRunnerError( + # No detail: the whole message is harness-authored, so there is + # nothing from the endpoint here for _error to scrub. + raise self._error( f"openai-compat response from {self.base_url} has no 'usage' block; " "token accounting would be silently wrong" ) @@ -237,9 +454,10 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] input_tokens = int(usage["prompt_tokens"]) output_tokens = int(usage["completion_tokens"]) except (KeyError, TypeError, ValueError) as exc: - raise LLMRunnerError( + raise self._error( f"openai-compat usage block from {self.base_url} has missing or " - f"malformed token counts: {usage!r}" + "malformed token counts", + repr(usage), ) from exc return AgentTurn( text=str(message.get("content") or "").strip(), @@ -339,10 +557,20 @@ def propose(self, transcript: Sequence[TranscriptItem], tools: Sequence[ToolDef] # --- Spec parsing --- -def create_tool_agent_model(spec: str, *, api_key: str | None = None) -> ToolAgentModel: +def create_tool_agent_model( + spec: str, + *, + api_key: str | None = None, + extra_headers: dict[str, str] | None = None, + temperature: float | None = 0.0, +) -> ToolAgentModel: """Build a tool-use agent from a spec string. Formats: ``openai-compat:@`` or ``scripted:``. + ``extra_headers`` are sent on every openai-compat request (ignored for + scripted) and never recorded in run artifacts. A header naming the same + HTTP field as the derived bearer — ``authorization`` in any casing — + replaces it, so the ambient ``OPENAI_API_KEY`` is not also sent. """ transport, _, remainder = spec.partition(":") if transport == "claude": @@ -357,7 +585,13 @@ def create_tool_agent_model(spec: str, *, api_key: str | None = None) -> ToolAge f"openai-compat spec must be 'openai-compat:@', got: {spec}" ) resolved_api_key = api_key if api_key is not None else os.getenv("OPENAI_API_KEY") - return OpenAICompatToolAgent(model=model, base_url=base_url, api_key=resolved_api_key) + return OpenAICompatToolAgent( + model=model, + base_url=base_url, + api_key=resolved_api_key, + extra_headers=extra_headers, + temperature=temperature, + ) if transport == "scripted" and remainder: return ScriptedToolAgent.from_path(Path(remainder)) raise ValueError( diff --git a/benchmarks/tests/agent_tasks/test_driver.py b/benchmarks/tests/agent_tasks/test_driver.py index 43a532975..d51593a52 100644 --- a/benchmarks/tests/agent_tasks/test_driver.py +++ b/benchmarks/tests/agent_tasks/test_driver.py @@ -7,10 +7,12 @@ from types import SimpleNamespace from typing import Any +import httpx import pytest from mcp.types import CallToolResult, TextContent import basic_memory_benchmarks.agent_tasks.driver as driver +import basic_memory_benchmarks.llm.tool_agent as tool_agent from basic_memory_benchmarks.agent_tasks.driver import ( SessionTerminatedError, SurfaceRuntime, @@ -29,7 +31,12 @@ from basic_memory_benchmarks.converters.xafs_to_corpus import convert_xafs_to_corpus from basic_memory_benchmarks.fairness import validate_surface_fairness from basic_memory_benchmarks.llm.runners import LLMResult, LLMRunner, LLMRunnerError -from basic_memory_benchmarks.llm.tool_agent import ScriptedToolAgent, ToolDef +from basic_memory_benchmarks.llm.tool_agent import ( + OpenAICompatToolAgent, + ScriptedToolAgent, + ToolDef, + UserMessage, +) from basic_memory_benchmarks.utils import sha256_file from xafs_fixture import ( DP1_CROSS_FORMAT_ANSWER, @@ -151,6 +158,40 @@ def session_factory(runtime: SurfaceRuntime) -> FakeSession: ) +def test_default_factory_receives_the_recorded_temperature( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The temperature manifest.json records is the one the model is built with. + + A programmatic caller bypasses the CLI, which pre-binds temperature into its + factory. Without this the default path would construct at the factory default + while the artifact claimed the configured value. + """ + import basic_memory_benchmarks.agent_tasks.driver as driver_module + + captured: dict[str, object] = {} + + def fake_create(spec: str, **kwargs: object): + captured["spec"] = spec + captured["temperature"] = kwargs.get("temperature", "NOT PASSED") + raise RuntimeError("stop after model construction") + + # Patching the module attribute is what the None-sentinel default reads at + # call time; a default bound to the function itself would not see this. + monkeypatch.setattr(driver_module, "create_tool_agent_model", fake_create) + _stub_bm(monkeypatch) + monkeypatch.chdir(tmp_path) + + config = _config(tmp_path, surfaces=["rich"], task_ids=["curate-orphans"]) + config = config.model_copy(update={"model_temperature": None}) + + with pytest.raises(RuntimeError, match="stop after model construction"): + run_agent_tasks(config) + + assert captured["spec"] == config.model_spec + assert captured["temperature"] is None + + def test_happy_path_writes_all_artifacts(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: session = FakeSession(RICH_SURFACE.tool_allowlist) run_dir = _run( @@ -196,11 +237,56 @@ def test_happy_path_writes_all_artifacts(tmp_path: Path, monkeypatch: pytest.Mon turn_rows = [json.loads(line) for line in (run_dir / "per-turn.jsonl").read_text().splitlines()] assert {row["kind"] for row in turn_rows} == {"model", "tool"} - # The scripted {project} placeholder was substituted before dispatch. - assert session.calls[0][1]["project"] == "at-test-run-curate-orphans" + # The scripted {project} placeholder was substituted before dispatch. No + # "at-" literal here: generated run ids already carry it (the first real + # run produced "at-at-..." project names). + assert session.calls[0][1]["project"] == "test-run-curate-orphans" assert session.stopped is True +def test_state_graded_task_reindexes_before_grading( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # First real-model run: a correct, project-scoped edit_note left its new + # wikilink relation unresolved at grading time (rich curate-connect) — + # settle only watches file-sync work, while forward references resolve in + # a project-index pass. State-graded tasks must re-run that pass, then + # settle, before graders read the index. + commands, settles = _stub_bm_recording(monkeypatch) + monkeypatch.chdir(tmp_path) + agent = ScriptedToolAgent( + script={ + "tasks": { + "Migrate CI to uv": [ + { + "tool_calls": [ + { + "name": "edit_note", + "arguments": {"identifier": "x", "project": "{project}"}, + } + ] + }, + {"text": "done\n```json\n{}\n```"}, + ] + } + } + ) + run_agent_tasks( + _config(tmp_path, task_ids=["tasks-complete"]), + model_factory=lambda spec: agent, + session_factory=lambda runtime: FakeSession(RICH_SURFACE.tool_allowlist), + ) + + project = "test-run-tasks-complete" + reindexes = [cmd[cmd.index("reindex") + 1 :] for cmd in commands if "reindex" in cmd] + assert reindexes == [ + ["-p", project, "--full", "--search"], # seed indexing before the loop + ["-p", project, "--search"], # post-loop: resolve the agent's writes + ] + # Settle follows BOTH passes: seed settle, then the pre-grading settle. + assert settles == [project, project] + + def test_headline_is_none_not_zero_when_nothing_completes( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -586,7 +672,7 @@ def test_grouped_manifest_run_shares_projects_and_reports_groups( # snapshot_baseline (monkeypatched to raise) was never touched. adds = [cmd for cmd in commands if "project" in cmd and "add" in cmd] added_projects = sorted(cmd[cmd.index("add") + 1] for cmd in adds) - assert added_projects == ["at-xafs-run-xafs-dp001", "at-xafs-run-xafs-dp002"] + assert added_projects == ["xafs-run-xafs-dp001", "xafs-run-xafs-dp002"] assert sorted(settles) == added_projects assert len(settles) == 2 @@ -655,7 +741,7 @@ def test_grouped_manifest_run_shares_projects_and_reports_groups( assert "- rich: 60 tokens over 6 calls" in report # The scripted {project} placeholder resolved to the shared group project. - assert session.calls[0][1]["project"] == "at-xafs-run-xafs-dp001" + assert session.calls[0][1]["project"] == "xafs-run-xafs-dp001" assert len(judge.prompts) == 6 @@ -739,3 +825,138 @@ def test_structured_content_fallback(self) -> None: outcome = tool_outcome_from_result(result) assert outcome.is_error is False assert "title" in outcome.text + + +def test_endpoint_secrets_never_reach_the_saved_error_artifact( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """End-to-end leak proof for the path a 401 body takes into run artifacts. + + A gateway that echoes the rejected credentials would otherwise carry the + --model-header value and the bearer token through LLMRunnerError into + per-task-agent.jsonl, which `publish` copies into the public bundle. + """ + secret_key = "sk-live-0123456789abcdef" + secret_header = "wrkspc_sensitive_9999" + + def echoing_401(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=401, + json={ + "error": { + "message": "invalid api key", + "seen": {"authorization": secret_key, "workspace": secret_header}, + } + }, + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", echoing_401) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + api_key=secret_key, + extra_headers={"anthropic-workspace-id": secret_header}, + max_retries=0, + ) + task = AgentTaskSpec(id="t1", skill="s", source="src", prompt="p", graders=()) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [ToolDef("search", "d", {})]) + + # Exactly what run_agent_tasks does with an LLMRunnerError cause. + row = driver._errored_result("bm-rich", task, str(caught.value)) + artifact = tmp_path / "per-task-agent.jsonl" + driver._write_jsonl(artifact, [row.model_dump(mode="json", exclude={"turn_records"})]) + + saved = artifact.read_text(encoding="utf-8") + assert secret_key not in saved + assert secret_header not in saved + # The operator still learns why the call was rejected. + assert "invalid api key" in saved + + +def test_json_escaping_header_secret_never_reaches_the_saved_error_artifact( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Leak proof for a --model-header value containing JSON-escaping characters. + + HTTP allows any visible ASCII character in a header value, so `"` and `\\` + are legal in an operator-supplied secret. A gateway echoing one into a JSON + error body writes the *escaped* spelling, which a plaintext-only search + never matches. Scanning the raw file text for the plaintext passes + vacuously here, so this asserts on the decoded row: the credential must not + be recoverable from what `publish` copies into the public bundle. + """ + secret_header = 'wrk"space\\9999' + escaped = json.dumps(secret_header)[1:-1] + + def echoing_401(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=401, + json={"error": {"message": "invalid api key", "seen": {"workspace": secret_header}}}, + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", echoing_401) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": secret_header}, + max_retries=0, + ) + task = AgentTaskSpec(id="t1", skill="s", source="src", prompt="p", graders=()) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [ToolDef("search", "d", {})]) + + row = driver._errored_result("bm-rich", task, str(caught.value)) + artifact = tmp_path / "per-task-agent.jsonl" + driver._write_jsonl(artifact, [row.model_dump(mode="json", exclude={"turn_records"})]) + + saved_error = json.loads(artifact.read_text(encoding="utf-8"))["error"] + assert secret_header not in saved_error + assert escaped not in saved_error + # The operator still learns why the call was rejected. + assert "invalid api key" in saved_error + + +def test_transport_refusal_text_never_reaches_the_saved_error_artifact( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Leak proof for a --model-header value the transport itself refuses. + + CR and LF are illegal in a header value but survive --model-header, which + strips only surrounding whitespace. h11 then quotes the whole value into a + LocalProtocolError, so the credential rides into per-task-agent.jsonl + without any response body being involved — the source every earlier + body-side fix left open. + """ + secret_header = "wrkspc" + chr(13) + chr(10) + "secret_9999" + escaped = json.dumps(secret_header)[1:-1] + + def refusing_post(url: str, **kwargs: Any) -> httpx.Response: + raise httpx.LocalProtocolError(f"Illegal header value {secret_header.encode()!r}") + + monkeypatch.setattr(tool_agent.httpx, "post", refusing_post) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": secret_header}, + max_retries=0, + ) + task = AgentTaskSpec(id="t1", skill="s", source="src", prompt="p", graders=()) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [ToolDef("search", "d", {})]) + + row = driver._errored_result("bm-rich", task, str(caught.value)) + artifact = tmp_path / "per-task-agent.jsonl" + driver._write_jsonl(artifact, [row.model_dump(mode="json", exclude={"turn_records"})]) + + saved_error = json.loads(artifact.read_text(encoding="utf-8"))["error"] + assert secret_header not in saved_error + assert escaped not in saved_error + # The operator still learns what the transport refused, and where. + assert "Illegal header value" in saved_error + assert "http://localhost/v1" in saved_error diff --git a/benchmarks/tests/agent_tasks/test_grading.py b/benchmarks/tests/agent_tasks/test_grading.py index e0214bdfc..782c62940 100644 --- a/benchmarks/tests/agent_tasks/test_grading.py +++ b/benchmarks/tests/agent_tasks/test_grading.py @@ -14,6 +14,7 @@ extract_final_json, grade_task, normalize_answer_item, + strip_own_project_prefix, ) from basic_memory_benchmarks.agent_tasks.models import TurnRecord from basic_memory_benchmarks.agent_tasks.spec import ( @@ -101,6 +102,12 @@ def test_normalization(self) -> None: "notes/redis-cache-tuning" ) + def test_strip_own_project_prefix_only_at_the_boundary(self) -> None: + assert strip_own_project_prefix("proj/notes/a", "proj") == "notes/a" + # The bare project name and a merely prefix-similar project stay intact. + assert strip_own_project_prefix("proj", "proj") == "proj" + assert strip_own_project_prefix("projx/notes/a", "proj") == "projx/notes/a" + class TestAnswerGraders: def test_answer_set_equals_pass_and_fail(self, tmp_path: Path) -> None: @@ -114,6 +121,24 @@ def test_answer_set_equals_pass_and_fail(self, tmp_path: Path) -> None: assert result.passed is False assert "missing" in result.detail + def test_answer_set_strips_the_tasks_own_project_prefix(self, tmp_path: Path) -> None: + # Agents quote permalinks exactly as tools return them — prefixed with + # the task's project name — while gold is project-relative. The first + # real-model run failed every AnswerSetEquals task on exactly this. + gold = frozenset({"notes/a", "notes/b"}) + grader = AnswerSetEquals(key="permalinks", gold=gold) + ctx = _ctx(tmp_path, '```json\n{"permalinks": ["proj/notes/a", "/Proj/notes/b.md"]}\n```') + assert evaluate_grader(grader, ctx).passed is True + + def test_answer_set_keeps_a_different_projects_prefix_failing(self, tmp_path: Path) -> None: + # Cross-project leakage: a permalink quoted from ANOTHER task's project + # is genuinely wrong even though its suffix matches gold. + grader = AnswerSetEquals(key="permalinks", gold=frozenset({"notes/a"})) + ctx = _ctx(tmp_path, '```json\n{"permalinks": ["other-proj/notes/a"]}\n```') + result = evaluate_grader(grader, ctx) + assert result.passed is False + assert "other-proj/notes/a" in result.detail + def test_answer_set_without_json_fails_with_detail(self, tmp_path: Path) -> None: grader = AnswerSetEquals(key="permalinks", gold=frozenset({"a"})) result = evaluate_grader(grader, _ctx(tmp_path, "no fenced block")) @@ -306,6 +331,43 @@ def test_resolved_relation_passes(self, tmp_path: Path) -> None: ) assert evaluate_grader(grader, ctx).passed is True + def test_project_prefixed_storage_matches_relative_gold(self, tmp_path: Path) -> None: + # Live-run DBs store permalinks project-prefixed + # ('at--/notes/...'); relative gold in the spec must still + # match, and a target under a DIFFERENT project prefix must not. + ctx = _ctx(tmp_path) + _make_db( + ctx.db_path, + { + "project": [(1, "proj")], + "entity": [ + (10, 1, "proj/notes/source"), + (11, 1, "proj/notes/target"), + (12, 1, "other-proj/notes/target"), + ], + "relation": [(100, 10, 11, "Target", "relates_to")], + }, + ) + grader = RelationResolves( + source_permalink="notes/source", targets=frozenset({"notes/target"}) + ) + assert evaluate_grader(grader, ctx).passed is True + + foreign_only = _ctx(tmp_path / "foreign") + (foreign_only.project_dir / "tasks").mkdir(parents=True, exist_ok=True) + _make_db( + foreign_only.db_path, + { + "project": [(1, "proj")], + "entity": [ + (10, 1, "proj/notes/source"), + (12, 1, "other-proj/notes/target"), + ], + "relation": [(100, 10, 12, "Target", "relates_to")], + }, + ) + assert evaluate_grader(grader, foreign_only).passed is False + def test_relation_type_filter(self, tmp_path: Path) -> None: ctx = _ctx(tmp_path) _make_db( diff --git a/benchmarks/tests/agent_tasks/test_models.py b/benchmarks/tests/agent_tasks/test_models.py new file mode 100644 index 000000000..a4427050d --- /dev/null +++ b/benchmarks/tests/agent_tasks/test_models.py @@ -0,0 +1,105 @@ +"""AgentTasksConfig field rules: values that cannot survive a run are rejected. + +Both rules live on the model rather than only in the CLI because +``run_agent_tasks(config)`` is an explicitly supported entrypoint, and each bad +value fails late and illegibly once a run is underway. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from pydantic import BaseModel, ValidationError + +from basic_memory_benchmarks.agent_tasks.models import AgentTasksConfig + +NON_FINITE = ["nan", "inf", "-inf"] + + +def _config(**overrides: Any) -> AgentTasksConfig: + defaults: dict[str, Any] = { + "run_id": "at-abc123", + "model_spec": "scripted:inline", + "bm_local_path": "/tmp/bm-checkout", + } + defaults.update(overrides) + return AgentTasksConfig(**defaults) + + +@pytest.mark.parametrize("raw", NON_FINITE) +def test_rejects_non_finite_model_temperature(raw: str) -> None: + with pytest.raises(ValidationError, match="finite"): + _config(model_temperature=float(raw)) + + +def test_finite_temperatures_and_the_omit_sentinel_still_pass() -> None: + assert _config(model_temperature=0.0).model_temperature == 0.0 + assert _config(model_temperature=0.7).model_temperature == 0.7 + # None is the documented "omit the parameter entirely" sentinel. + assert _config(model_temperature=None).model_temperature is None + + +def test_non_finite_temperature_can_no_longer_be_recorded_as_omitted() -> None: + """The substantive half: a corrupted run record is worse than a crash. + + JSON has no nan/inf spelling, so Pydantic serializes both as null — which is + exactly this field's "temperature omitted" sentinel. Before the guard, a nan + run wrote a manifest.json claiming no temperature was sent, and the run still + looked valid afterwards. + """ + for raw in NON_FINITE: + with pytest.raises(ValidationError): + _config(model_temperature=float(raw)) + + # A recorded run config still round-trips the real value, including the + # sentinel — the guard rejects, it does not rewrite. + assert '"model_temperature":0.7' in _config(model_temperature=0.7).model_dump_json() + assert '"model_temperature":null' in _config(model_temperature=None).model_dump_json() + + +def test_unguarded_float_field_really_does_collapse_to_null() -> None: + """Pins the reason the guard exists so its rationale cannot silently rot.""" + + class Unguarded(BaseModel): + model_temperature: float | None = 0.0 + + for raw in NON_FINITE: + dumped = Unguarded(model_temperature=float(raw)).model_dump_json() + assert dumped == '{"model_temperature":null}' + + +@pytest.mark.parametrize( + "run_id", + [ + "-trial", # parsed as options by `bm project add`: "No such option: -t" + "--run", + "", + ".hidden", + "..", + "nested/run", + "back\\slash", + "has space", + ], +) +def test_rejects_run_ids_that_are_unsafe_as_argv_or_path(run_id: str) -> None: + with pytest.raises(ValidationError, match="run_id must start with"): + _config(run_id=run_id) + + +@pytest.mark.parametrize("run_id", ["at-abc123", "test-run", "run_2026.09.01", "A1", "_local"]) +def test_accepts_ordinary_run_ids(run_id: str) -> None: + assert _config(run_id=run_id).run_id == run_id + + +def test_accepted_run_ids_generate_option_safe_project_names() -> None: + """The invariant the rule exists to protect. + + The driver builds project names as ``{run_id}-{task.id}`` and hands them + straight to ``bm project add``; a name starting with "-" is parsed as options + and aborts the run after the corpus copy, with stderr captured so the + operator sees only a bare CalledProcessError exit status. + """ + for run_id in ("at-abc123", "test-run", "_local"): + project_name = f"{_config(run_id=run_id).run_id}-curate-orphans" + assert not project_name.startswith("-") diff --git a/benchmarks/tests/beam_fixture.py b/benchmarks/tests/beam_fixture.py index 1af173164..e445fa805 100644 --- a/benchmarks/tests/beam_fixture.py +++ b/benchmarks/tests/beam_fixture.py @@ -103,7 +103,8 @@ def conversation_one_chat() -> list[dict[str, Any]]: Message ids are global and interleaved (0..5). Message 2 carries the trailing ``->-> 1,2`` probe-index marker the converter must strip; message 3 carries the non-numeric ``->-> 2,N/A`` variant observed in - upstream 100K/1/chat.json. + upstream 100K/1/chat.json; message 4 carries the multi-id + ``->-> 2,22, 24`` variant (space after a comma) observed in 100K. """ return [ { @@ -138,10 +139,13 @@ def conversation_one_chat() -> list[dict[str, Any]]: message( "user", 4, - "Update: my salary is now $75,000.", + "Update: my salary is now $75,000. ->-> 2,22, 24", time_anchor="April-02-2024", ), - message("assistant", 5, "Got it, salary updated."), + # ")" after the ids is the paren-suffixed variant ("1,5)" + # x6 in the live 100K tier); it is generator junk, not + # content, and strips with the marker. + message("assistant", 5, "Got it, salary updated. ->-> 1,5)"), ] ], }, @@ -177,7 +181,9 @@ def conversation_one_probes() -> dict[str, list[dict[str, Any]]]: "List the events we discussed in the order they happened.", rubric=["Adopted Biscuit", "Dentist appointment", "Salary update"], reference_answer="Adopted Biscuit -> Dentist appointment -> Salary update", - source_chat_ids=[0, 2, 4], + # Mixed int / int-list shape from the live 100K tier: one event's + # evidence can span chats ([2, 4]); the loader flattens the union. + source_chat_ids=[0, [2, 4]], difficulty="hard", ordering_type="full", ) diff --git a/benchmarks/tests/converters/test_beam_converter.py b/benchmarks/tests/converters/test_beam_converter.py index 718313ebe..2737da61d 100644 --- a/benchmarks/tests/converters/test_beam_converter.py +++ b/benchmarks/tests/converters/test_beam_converter.py @@ -114,6 +114,12 @@ def test_index_marker_stripped(self, chats_root: Path, tmp_path: Path) -> None: assert "- **User:** My dentist appointment is on March 29." in corpus_text # The non-numeric "->-> 2,N/A" variant is stripped too. assert "- **Assistant:** Noted: the dentist appointment is March 29." in corpus_text + # The multi-id "->-> 2,22, 24" variant (space after a comma) as well. + assert "- **User:** Update: my salary is now $75,000." in corpus_text + # And the paren-suffixed "->-> 1,5)" variant: the ")" is generator + # junk (no matching open paren upstream) and strips with the marker. + assert "- **Assistant:** Got it, salary updated." in corpus_text + assert "salary updated. )" not in corpus_text def test_surviving_marker_variant_fails_fast(self, tmp_path: Path) -> None: # A non-trailing marker escapes the strip pattern; conversion must diff --git a/benchmarks/tests/llm/test_tool_agent.py b/benchmarks/tests/llm/test_tool_agent.py index b73a24609..df6e83a4e 100644 --- a/benchmarks/tests/llm/test_tool_agent.py +++ b/benchmarks/tests/llm/test_tool_agent.py @@ -69,6 +69,506 @@ def _response(payload: dict[str, Any]) -> httpx.Response: ) +def test_extra_headers_ride_every_request(monkeypatch: pytest.MonkeyPatch) -> None: + """--model-header values reach the endpoint (e.g. anthropic-workspace-id).""" + captured: dict[str, Any] = {} + + def fake_post(url: str, **kwargs: Any) -> httpx.Response: + captured["headers"] = kwargs["headers"] + return _response( + { + "choices": [{"message": {"content": "ok", "tool_calls": []}}], + "usage": {"prompt_tokens": 1, "completion_tokens": 1}, + } + ) + + monkeypatch.setattr(tool_agent.httpx, "post", fake_post) + agent = create_tool_agent_model( + "openai-compat:claude-sonnet-5@https://api.anthropic.com/v1", + api_key="k", + extra_headers={"anthropic-workspace-id": "wrkspc_test"}, + ) + assert isinstance(agent, OpenAICompatToolAgent) + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + assert captured["headers"]["anthropic-workspace-id"] == "wrkspc_test" + assert captured["headers"]["Authorization"] == "Bearer k" + + +AMBIENT_KEY = "sk-ambient-must-not-be-sent" +OPERATOR_AUTHORIZATION = "Bearer op-token-intended" + + +def _capturing_post(captured: dict[str, Any]) -> Any: + def fake_post(url: str, **kwargs: Any) -> httpx.Response: + captured["headers"] = kwargs["headers"] + return _response( + { + "choices": [{"message": {"content": "ok", "tool_calls": []}}], + "usage": {"prompt_tokens": 1, "completion_tokens": 1}, + } + ) + + return fake_post + + +def _authorization_values(headers: Any) -> list[bytes]: + """Every Authorization value as httpx would put it on the wire. + + Reads the raw list rather than indexing, because indexing collapses the + duplicate this asserts the absence of. Wrapping in ``httpx.Headers`` is + what a plain dict would go through inside httpx anyway, so a dict spelling + the name twice still yields two entries here. + """ + return [value for name, value in httpx.Headers(headers).raw if name.lower() == b"authorization"] + + +def test_operator_authorization_header_replaces_the_ambient_bearer( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A differently-cased operator header must replace the bearer, not join it. + + HTTP header names are case-insensitive, so sending both is not "two + headers": the endpoint picks one, and the ambient OPENAI_API_KEY — which + the operator exported for some other tool — is disclosed to a custom + endpoint they never meant to hand it to. + """ + captured: dict[str, Any] = {} + monkeypatch.setattr(tool_agent.httpx, "post", _capturing_post(captured)) + monkeypatch.setenv("OPENAI_API_KEY", AMBIENT_KEY) + + agent = create_tool_agent_model( + "openai-compat:m@http://localhost/v1", + extra_headers={"authorization": OPERATOR_AUTHORIZATION}, + ) + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + assert _authorization_values(captured["headers"]) == [OPERATOR_AUTHORIZATION.encode()] + # The ambient credential must not ride along under any header name. + assert all( + AMBIENT_KEY.encode() not in value for _, value in httpx.Headers(captured["headers"]).raw + ) + + +def test_repeated_model_header_spellings_collapse_to_the_last_one( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """--model-header is repeatable and its names are case-sensitive at parse. + + So the operator's own header map can spell one HTTP field two ways, which + is the same duplicate-serialization defect without an ambient key involved. + """ + captured: dict[str, Any] = {} + monkeypatch.setattr(tool_agent.httpx, "post", _capturing_post(captured)) + + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"Authorization": "Bearer first", "authorization": OPERATOR_AUTHORIZATION}, + ) + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + assert _authorization_values(captured["headers"]) == [OPERATOR_AUTHORIZATION.encode()] + + +def test_temperature_none_omits_the_parameter(monkeypatch: pytest.MonkeyPatch) -> None: + """Claude 5 endpoints reject any temperature; None must drop the key entirely.""" + captured: dict[str, Any] = {} + + def fake_post(url: str, **kwargs: Any) -> httpx.Response: + captured["body"] = kwargs["json"] + return _response( + { + "choices": [{"message": {"content": "ok", "tool_calls": []}}], + "usage": {"prompt_tokens": 1, "completion_tokens": 1}, + } + ) + + monkeypatch.setattr(tool_agent.httpx, "post", fake_post) + agent = OpenAICompatToolAgent("m", "http://localhost/v1", temperature=None) + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + assert "temperature" not in captured["body"] + + +def test_http_error_includes_response_body(monkeypatch: pytest.MonkeyPatch) -> None: + """A 4xx body names the actual rejection instead of a bare status code.""" + + def fake_post(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=400, + json={"error": {"message": "anthropic-workspace-id is required"}}, + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", fake_post) + agent = OpenAICompatToolAgent("m", "http://localhost/v1", max_retries=0) + with pytest.raises(LLMRunnerError, match="anthropic-workspace-id is required"): + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + +SECRET_KEY = "sk-live-0123456789abcdef" +SECRET_HEADER_VALUE = "wrkspc_sensitive_9999" + + +def _echoing_401(url: str, **kwargs: Any) -> httpx.Response: + """A gateway that quotes the offending request headers back in its body.""" + return httpx.Response( + status_code=401, + json={ + "error": { + "message": "invalid api key", + "request_headers": { + "Authorization": f"Bearer {SECRET_KEY}", + "anthropic-workspace-id": SECRET_HEADER_VALUE, + }, + } + }, + request=httpx.Request("POST", url), + ) + + +def _secretive_agent(**kwargs: Any) -> OpenAICompatToolAgent: + return OpenAICompatToolAgent( + "m", + "http://localhost/v1", + api_key=SECRET_KEY, + extra_headers={"anthropic-workspace-id": SECRET_HEADER_VALUE}, + max_retries=0, + **kwargs, + ) + + +def test_error_body_redacts_secrets_but_keeps_the_reason( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An echoed key/header must not ride the error into run artifacts.""" + monkeypatch.setattr(tool_agent.httpx, "post", _echoing_401) + + with pytest.raises(LLMRunnerError) as caught: + _secretive_agent().propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + message = str(caught.value) + assert SECRET_KEY not in message + assert SECRET_HEADER_VALUE not in message + assert tool_agent.REDACTION_MARKER in message + # The diagnostic the body was included for must survive redaction. + assert "invalid api key" in message + + +def test_redaction_precedes_body_truncation(monkeypatch: pytest.MonkeyPatch) -> None: + """A secret straddling the 300-char cut must not survive as a prefix.""" + + # 290 padding + a 20-char key: a naive text[:300] keeps the first 10 + # characters of the key, which is what this test must catch. + def padded_401(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=401, + text=("x" * 290) + SECRET_KEY, + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", padded_401) + + with pytest.raises(LLMRunnerError) as caught: + _secretive_agent().propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + assert SECRET_KEY[:10] not in str(caught.value) + + +def test_redact_secrets_masks_longest_match_first() -> None: + # A short secret contained in a longer one must not be masked first, or + # the remainder of the longer value would stay in the text. + masked = tool_agent.redact_secrets("token=abc123-suffix", ["abc123", "abc123-suffix"]) + assert masked == f"token={tool_agent.REDACTION_MARKER}" + + +def test_redact_secrets_without_secrets_is_identity() -> None: + assert tool_agent.redact_secrets("nothing to hide", []) == "nothing to hide" + + +# HTTP allows any visible ASCII character in a header value, so " (0x22) and +# \ (0x5C) are both legal in a --model-header secret — and both are escaped +# when a gateway echoes the value inside a JSON error body. +ESCAPING_SECRET = 'wrk"space\\9999' + + +def test_redact_secrets_masks_the_json_escaped_spelling() -> None: + """A secret echoed into a JSON body appears escaped, not plaintext.""" + body = json.dumps({"error": {"message": "invalid api key", "seen": ESCAPING_SECRET}}) + escaped = json.dumps(ESCAPING_SECRET)[1:-1] + # Precondition: the plaintext genuinely is absent, so a plaintext-only + # search has nothing to match and would leave the body untouched. + assert ESCAPING_SECRET not in body + assert escaped in body + + masked = tool_agent.redact_secrets(body, [ESCAPING_SECRET]) + + assert escaped not in masked + assert tool_agent.REDACTION_MARKER in masked + assert "invalid api key" in masked + + +def test_redact_secrets_masks_a_secret_nested_two_json_levels_deep() -> None: + """A proxy that wraps an upstream JSON body in a string escapes it twice.""" + upstream = json.dumps({"error": {"seen": ESCAPING_SECRET}}) + body = json.dumps({"error": {"message": "upstream rejected", "upstream": upstream}}) + double_escaped = json.dumps(json.dumps(ESCAPING_SECRET)[1:-1])[1:-1] + assert double_escaped in body + + masked = tool_agent.redact_secrets(body, [ESCAPING_SECRET]) + + assert double_escaped not in masked + assert "upstream rejected" in masked + + +def test_error_body_redacts_a_json_escaping_secret(monkeypatch: pytest.MonkeyPatch) -> None: + """End-to-end at the seam: the escaped spelling must not reach the error.""" + + def echoing_401(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=401, + json={"error": {"message": "invalid api key", "seen": ESCAPING_SECRET}}, + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", echoing_401) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": ESCAPING_SECRET}, + max_retries=0, + ) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + message = str(caught.value) + assert ESCAPING_SECRET not in message + assert json.dumps(ESCAPING_SECRET)[1:-1] not in message + assert "invalid api key" in message + + +def _unicode_escape(character: str, *, uppercase: bool = False) -> str: + """The JSON ``\\uXXXX`` spelling of one character. + + Built rather than written literally so the tests state which character is + being escaped, and so the same helper produces both hex cases. + """ + return f"\\u{ord(character):04X}" if uppercase else f"\\u{ord(character):04x}" + + +def _go_encoded(value: str) -> str: + """``value`` as Go's default JSON encoder spells it inside a string. + + Go HTML-escapes ``<``, ``>`` and ``&`` unless the caller opts out, so a + gateway written in Go echoes a header value in a spelling ``json.dumps`` + never produces and ``_encoded_forms`` therefore cannot enumerate. + """ + return "".join( + _unicode_escape(character) if character in "<>&" else character for character in value + ) + + +# A --model-header value is any visible ASCII, so it may contain the characters +# a Go encoder escapes. This is the value from the reported reproduction. +GO_ESCAPING_SECRET = "wrk" + + +def test_redact_secrets_withholds_a_body_using_go_style_unicode_escapes() -> None: + """The reported leak: a valid spelling that masking alone cannot reach.""" + body = '{"seen":"' + _go_encoded(GO_ESCAPING_SECRET) + '"}' + # Precondition: neither the plaintext nor any json.dumps spelling is + # present, so every form _encoded_forms derives provably fails to match + # and the body would otherwise pass through untouched. + assert GO_ESCAPING_SECRET not in body + assert json.dumps(GO_ESCAPING_SECRET)[1:-1] not in body + + masked = tool_agent.redact_secrets(body, [GO_ESCAPING_SECRET]) + + assert masked == tool_agent.WITHHELD_BODY_MARKER + assert _go_encoded(GO_ESCAPING_SECRET) not in masked + + +def test_redact_secrets_detects_unicode_escapes_in_either_hex_case() -> None: + """JSON allows both hex cases, so neither spelling may be privileged.""" + lower = '{"seen":"wrk' + _unicode_escape("<") + "secret" + _unicode_escape(">") + '"}' + upper = ( + '{"seen":"wrk' + + _unicode_escape("<", uppercase=True) + + "secret" + + _unicode_escape(">", uppercase=True) + + '"}' + ) + assert lower != upper + + for body in (lower, upper): + assert tool_agent.redact_secrets(body, [GO_ESCAPING_SECRET]) == ( + tool_agent.WITHHELD_BODY_MARKER + ) + + +def test_redact_secrets_handles_a_truncated_body_without_parsing_it() -> None: + """Error bodies arrive cut mid-string; a json.loads backstop would raise.""" + body = '{"error":{"seen":"wrk' + _unicode_escape("<") + "secret" + _unicode_escape(">") + with pytest.raises(json.JSONDecodeError): + json.loads(body) + + assert tool_agent.redact_secrets(body, [GO_ESCAPING_SECRET]) == ( + tool_agent.WITHHELD_BODY_MARKER + ) + + +def test_redact_secrets_detects_a_doubly_escaped_unicode_spelling() -> None: + """A proxy wrapping an upstream body escapes the upstream's own escapes.""" + upstream = '{"seen":"' + _go_encoded(GO_ESCAPING_SECRET) + '"}' + body = json.dumps({"error": {"message": "upstream rejected", "upstream": upstream}}) + # One pass only recovers the upstream text; the secret needs the second, + # which is what the fixpoint loop (rather than a single unescape) buys. + assert GO_ESCAPING_SECRET not in tool_agent._unescape_once(body) + + assert tool_agent.redact_secrets(body, [GO_ESCAPING_SECRET]) == ( + tool_agent.WITHHELD_BODY_MARKER + ) + + +def test_redact_secrets_keeps_an_ordinary_body_readable() -> None: + """Withholding is the exception: a maskable body keeps its diagnostic.""" + body = json.dumps({"error": {"message": "invalid api key", "key": SECRET_KEY}}) + + masked = tool_agent.redact_secrets(body, [SECRET_KEY]) + + assert SECRET_KEY not in masked + assert tool_agent.REDACTION_MARKER in masked + assert "invalid api key" in masked + assert masked != tool_agent.WITHHELD_BODY_MARKER + + +# A control escape must decode to its character rather than merely lose its +# backslash: dropping it would splice "…i" and "formation" into a literal match +# and withhold a multi-line diagnostic that never contained the secret at all. +NEWLINE_STRADDLING_SECRET = "wrkspc_information" + + +def test_redact_secrets_decodes_control_escapes_rather_than_dropping_them() -> None: + body = json.dumps({"error": "check wrkspc_i\nformation and retry"}) + assert NEWLINE_STRADDLING_SECRET not in body + + assert tool_agent.redact_secrets(body, [NEWLINE_STRADDLING_SECRET]) == body + + +def test_error_body_withheld_when_a_secret_survives_masking( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """End-to-end: a Go-style echo costs the body, never the credential.""" + + def go_escaping_401(url: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + status_code=401, + text='{"error":{"message":"invalid api key","seen":"' + + _go_encoded(GO_ESCAPING_SECRET) + + '"}}', + request=httpx.Request("POST", url), + ) + + monkeypatch.setattr(tool_agent.httpx, "post", go_escaping_401) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": GO_ESCAPING_SECRET}, + max_retries=0, + ) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + message = str(caught.value) + assert GO_ESCAPING_SECRET not in message + assert _go_encoded(GO_ESCAPING_SECRET) not in message + assert tool_agent.WITHHELD_BODY_MARKER in message + # The status still names the rejection, so dropping the body does not + # leave the operator with nothing to act on. + assert "401" in message + + +# HTTP forbids CR and LF in a header value, but --model-header only strips +# surrounding whitespace, so an embedded one reaches httpx intact. Built from +# chr() rather than written as escapes so the test states which bytes it means. +CRLF_HEADER_SECRET = "wrkspc" + chr(13) + chr(10) + "secret_9999" + + +def test_transport_exception_text_does_not_leak_a_header_value( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The leak source that is not a response body: h11 quoting a bad header. + + Verified against a live socket: h11 refuses to serialize the value and + raises LocalProtocolError("Illegal header value b'wrkspc...'"), whose text + the harness interpolates straight into LLMRunnerError. Nothing in that path + touches exc.response, so masking the body alone never reached it. + """ + quoted = f"Illegal header value {CRLF_HEADER_SECRET.encode()!r}" + + def refusing_post(url: str, **kwargs: Any) -> httpx.Response: + raise httpx.LocalProtocolError(quoted) + + # Precondition: a bytes repr escapes CR and LF, so the plaintext genuinely + # is absent and a plaintext-only search would pass vacuously. + escaped = json.dumps(CRLF_HEADER_SECRET)[1:-1] + assert CRLF_HEADER_SECRET not in quoted + assert escaped in quoted + + monkeypatch.setattr(tool_agent.httpx, "post", refusing_post) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": CRLF_HEADER_SECRET}, + max_retries=0, + ) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + message = str(caught.value) + assert CRLF_HEADER_SECRET not in message + assert escaped not in message + assert tool_agent.REDACTION_MARKER in message + # The summary half is harness-authored, so the operator still learns which + # endpoint failed and why even though the value was masked out. + assert "Illegal header value" in message + assert "http://localhost/v1" in message + + +def test_error_summary_survives_a_withheld_detail(monkeypatch: pytest.MonkeyPatch) -> None: + """Withholding must cost the diagnostic, never the endpoint that failed. + + redact_secrets replaces its whole input when masking provably failed, so + routing the entire message through it would drop the summary too. This + pins the split: a transport exception spelling the secret in a form the + form set cannot enumerate loses the detail and keeps the locator. + """ + + def go_escaping_post(url: str, **kwargs: Any) -> httpx.Response: + raise httpx.ConnectError('{"seen":"' + _go_encoded(GO_ESCAPING_SECRET) + '"}') + + monkeypatch.setattr(tool_agent.httpx, "post", go_escaping_post) + agent = OpenAICompatToolAgent( + "m", + "http://localhost/v1", + extra_headers={"anthropic-workspace-id": GO_ESCAPING_SECRET}, + max_retries=0, + ) + + with pytest.raises(LLMRunnerError) as caught: + agent.propose([UserMessage(text="hi")], [SEARCH_TOOL]) + + message = str(caught.value) + assert GO_ESCAPING_SECRET not in message + assert _go_encoded(GO_ESCAPING_SECRET) not in message + assert tool_agent.WITHHELD_BODY_MARKER in message + assert "http://localhost/v1" in message + + class TestOpenAICompatToolAgent: def _agent(self) -> OpenAICompatToolAgent: return OpenAICompatToolAgent("qwen3", "http://localhost:11434/v1", max_retries=0) diff --git a/benchmarks/tests/test_beam_dataset.py b/benchmarks/tests/test_beam_dataset.py index 67c339f85..7f0b4cbac 100644 --- a/benchmarks/tests/test_beam_dataset.py +++ b/benchmarks/tests/test_beam_dataset.py @@ -109,6 +109,8 @@ def test_source_chat_ids_normalization(self, chats_root: Path) -> None: assert by_ability["knowledge_update"].source_chat_ids == [1, 4] # abstention omits the field entirely. assert by_ability["abstention"].source_chat_ids == [] + # event_ordering mixes ints with int-list groups (live 100K shape). + assert by_ability["event_ordering"].source_chat_ids == [0, 2, 4] def test_extras_passthrough(self, chats_root: Path) -> None: conv = load_beam_conversation(chats_root / "100K" / "1", "100K") @@ -173,6 +175,14 @@ def test_missing_ability_key_raises(self, tmp_path: Path) -> None: with pytest.raises(ValueError, match="missing abilities"): load_beam_conversation(conv_dir, "100K") + def test_non_int_in_chat_id_group_raises(self, tmp_path: Path) -> None: + probes = minimal_probes() + probes["information_extraction"][0]["source_chat_ids"] = [0, ["not-an-int"]] + conv_dir = write_conversation(tmp_path / "1", conversation_two_full_chat(), probes) + + with pytest.raises(ValueError, match="must contain ints"): + load_beam_conversation(conv_dir, "100K") + def test_empty_rubric_raises(self, tmp_path: Path) -> None: probes = minimal_probes() probes["information_extraction"][0]["rubric"] = [] diff --git a/benchmarks/tests/test_cli_surface.py b/benchmarks/tests/test_cli_surface.py index fa3209f45..4f52328b1 100644 --- a/benchmarks/tests/test_cli_surface.py +++ b/benchmarks/tests/test_cli_surface.py @@ -1,7 +1,9 @@ import json +from functools import partial from pathlib import Path from typing import Any +import httpx import pytest from typer.testing import CliRunner @@ -162,7 +164,7 @@ def test_task_manifest_derives_groups_corpus_dir( ) -> None: captured: dict[str, AgentTasksConfig] = {} - def fake_run(config: AgentTasksConfig) -> Path: + def fake_run(config: AgentTasksConfig, **kwargs: object) -> Path: captured["config"] = config return tmp_path / "run" @@ -200,7 +202,7 @@ def test_task_manifest_respects_explicit_corpus_dir( ) -> None: captured: dict[str, AgentTasksConfig] = {} - def fake_run(config: AgentTasksConfig) -> Path: + def fake_run(config: AgentTasksConfig, **kwargs: object) -> Path: captured["config"] = config return tmp_path / "run" @@ -293,7 +295,7 @@ def test_run_agent_tasks_dedupes_repeated_surfaces( # mid-run creating the second identical surface home. captured: dict[str, list[str]] = {} - def fake_run(config: AgentTasksConfig) -> Path: + def fake_run(config: AgentTasksConfig, **kwargs: object) -> Path: captured["surfaces"] = config.surfaces return tmp_path / "run" @@ -319,3 +321,180 @@ def fake_run(config: AgentTasksConfig) -> Path: assert result.exit_code == 0, result.output assert captured["surfaces"] == ["rich"] + + +def test_run_agent_tasks_rejects_malformed_model_header(tmp_path: Path) -> None: + script = tmp_path / "script.json" + script.write_text('{"tasks": {}}', encoding="utf-8") + bm_checkout = tmp_path / "bm" + bm_checkout.mkdir() + + result = runner.invoke( + app, + [ + "run", + "agent-tasks", + "--model", + f"scripted:{script}", + "--bm-local-path", + str(bm_checkout), + "--model-header", + "missing-separator", + ], + ) + + assert result.exit_code != 0 + assert "Name=value" in result.output + + +def test_run_agent_tasks_passes_model_headers_to_factory( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Headers ride only in the model-factory closure — never in the config, + # so they can never leak into run artifacts. + captured: dict[str, object] = {} + + def fake_run(config: AgentTasksConfig, **kwargs: object) -> Path: + captured["config"] = config + captured["model_factory"] = kwargs.get("model_factory") + return tmp_path / "run" + + monkeypatch.setattr(cli, "run_agent_tasks", fake_run) + script = tmp_path / "script.json" + script.write_text('{"tasks": {}}', encoding="utf-8") + bm_checkout = tmp_path / "bm" + bm_checkout.mkdir() + + result = runner.invoke( + app, + [ + "run", + "agent-tasks", + "--model", + f"scripted:{script}", + "--bm-local-path", + str(bm_checkout), + "--model-header", + "anthropic-workspace-id=wrkspc_test", + ], + ) + + assert result.exit_code == 0, result.output + factory = captured["model_factory"] + assert isinstance(factory, partial) + assert factory.keywords["extra_headers"] == {"anthropic-workspace-id": "wrkspc_test"} + config = captured["config"] + assert isinstance(config, AgentTasksConfig) + assert "wrkspc_test" not in config.model_dump_json() + + +def test_run_agent_tasks_model_temperature_omit( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Claude 5 endpoints reject the temperature parameter entirely; 'omit' + # drops it from requests and the choice is recorded in the run config. + captured: dict[str, object] = {} + + def fake_run(config: AgentTasksConfig, **kwargs: object) -> Path: + captured["config"] = config + captured["model_factory"] = kwargs.get("model_factory") + return tmp_path / "run" + + monkeypatch.setattr(cli, "run_agent_tasks", fake_run) + script = tmp_path / "script.json" + script.write_text('{"tasks": {}}', encoding="utf-8") + bm_checkout = tmp_path / "bm" + bm_checkout.mkdir() + + result = runner.invoke( + app, + [ + "run", + "agent-tasks", + "--model", + f"scripted:{script}", + "--bm-local-path", + str(bm_checkout), + "--model-temperature", + "omit", + ], + ) + + assert result.exit_code == 0, result.output + factory = captured["model_factory"] + assert isinstance(factory, partial) + assert factory.keywords["temperature"] is None + config = captured["config"] + assert isinstance(config, AgentTasksConfig) + assert config.model_temperature is None + + +@pytest.mark.parametrize("raw", ["nan", "inf", "-inf", "Infinity"]) +def test_run_agent_tasks_rejects_non_finite_model_temperature(tmp_path: Path, raw: str) -> None: + # float() accepts these and the config model stores them, but JSON cannot + # encode them: httpx raises a bare ValueError while serializing the request + # body, which is not one of _post's handled transport failures. The run + # would die after surface setup with no artifacts, so reject at parse time. + script = tmp_path / "script.json" + script.write_text('{"tasks": {}}', encoding="utf-8") + bm_checkout = tmp_path / "bm" + bm_checkout.mkdir() + + result = runner.invoke( + app, + [ + "run", + "agent-tasks", + "--model", + f"scripted:{script}", + "--bm-local-path", + str(bm_checkout), + "--model-temperature", + raw, + ], + ) + + assert result.exit_code != 0 + assert "finite" in result.output + + +def test_non_finite_temperature_would_break_the_request_body() -> None: + """Pins the reason the CLI guard exists: httpx rejects non-finite floats.""" + with pytest.raises(ValueError, match="not JSON compliant"): + httpx.Request( + "POST", + "http://localhost/v1/chat/completions", + json={"model": "m", "temperature": float("nan")}, + ) + + +@pytest.mark.parametrize("raw", ["-trial", "nested/run", ".."]) +def test_run_agent_tasks_rejects_unsafe_run_id(tmp_path: Path, raw: str) -> None: + # AgentTasksConfig owns the rule; the CLI must surface it as a parameter + # error rather than letting a Pydantic traceback escape. Without it, + # --run-id=-trial reached `bm project add -trial-`, which parses the + # name as options and aborts after the corpus copy. + script = tmp_path / "script.json" + script.write_text('{"tasks": {}}', encoding="utf-8") + bm_checkout = tmp_path / "bm" + bm_checkout.mkdir() + + result = runner.invoke( + app, + [ + "run", + "agent-tasks", + "--model", + f"scripted:{script}", + "--bm-local-path", + str(bm_checkout), + # The "=" form is what lets a leading-hyphen value through Typer's + # own option parsing — exactly how the bad run_id reaches the model. + f"--run-id={raw}", + ], + ) + + assert result.exit_code != 0 + assert "run_id must start with" in result.output + # A parameter error, not an escaped traceback. + assert result.exception is None or isinstance(result.exception, SystemExit) diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index 3bc75604d..6586d3113 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -123,7 +123,9 @@ def _directory_page_summary(result: dict[str, Any]) -> str: # --- cat / head rendering --- -def _write_slice_footer(result: dict[str, Any], *, plain: bool) -> None: +def _write_slice_footer( + result: dict[str, Any], *, plain: bool, content_terminated: bool = True +) -> None: """Describe an applied slice under the content. Rich mode prints a dim footer on stdout; plain mode sends it to stderr so @@ -153,6 +155,11 @@ def _write_slice_footer(result: dict[str, Any], *, plain: bool) -> None: # newline, so its last line can sit in stdout's line buffer on a TTY; # flush before the unbuffered stderr write or the footer prints first. sys.stdout.flush() + # An unterminated slice would visually concatenate the footer onto the + # last content line in a terminal or merged capture. Lead with the + # newline on STDERR so stdout stays byte-exact for pipes. + if not content_terminated: + text = f"\n{text}" print(text, file=sys.stderr) else: console.print(Text(text, style="dim")) @@ -171,8 +178,9 @@ def _render_cat( # newline — so `bm cat x --plain` pipes and redirects like cat(1) and # round-trips the file; slice info goes to stderr. content = result.get("content") - sys.stdout.write(content if isinstance(content, str) else "") - _write_slice_footer(result, plain=True) + text = content if isinstance(content, str) else "" + sys.stdout.write(text) + _write_slice_footer(result, plain=True, content_terminated=text.endswith("\n") or not text) return # cat's payload is the read_note JSON shape, so the read-note renderer applies. _display_read_note(result, include_frontmatter=include_frontmatter) diff --git a/tests/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index d1b3676b4..03dda4601 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -359,6 +359,10 @@ def test_cat_plain_slice_footer_goes_to_stderr(mock_cat): assert result.exit_code == 0, result.output assert result.stdout == CAT_SLICE_RESULT["content"] assert "lines 5-7 of 7" in result.stderr + # The slice content carries no trailing newline, so the footer must open + # with one on stderr — otherwise it visually concatenates onto the last + # content line in a terminal while stdout stays byte-exact for pipes. + assert result.stderr.startswith("\n") @patch("basic_memory.mcp.tools.cat", new_callable=AsyncMock, return_value=CAT_TRUNCATED_RESULT)