diff --git a/CHANGELOG.md b/CHANGELOG.md index 735d98f4e..1ba8a1e38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,25 +7,167 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added - -- **`[cascade]` settings section** — the four maintenance cadences - (`optimize_heartbeat_seconds`, `optimize_prune_interval_seconds`, - `optimize_prune_retention_seconds`, `optimize_rebuild_interval_seconds`) are - now configurable. They were already constructor arguments on `CascadeWorker`, - but `CascadeConfig` did not carry them and no production path passed one, so - the defaults were unreachable — which is why the 12h rebuild sweep could not - be exercised by any soak run shorter than half a day. The deadlines that - bound a hung call are deliberately **not** exposed: they are hang-catchers - sized from measured durations, where too low manufactures failures on a - healthy table and too high leaves a wedged one invisible for longer. Note - `optimize_prune_retention_seconds` has a second effect worth reading before - tuning — it also decides how long index files keep a manifest naming them, - and below LanceDB's 7-day unverified window they then wait out the full 7 - days. +## [1.2.3] - 2026-08-07 ### Fixed +- **Agent skill extraction is no longer stuck in a retry-then-dead-letter + loop.** Target case data now travels on `SkillClusterUpdated` and existing + skills for the cluster are read from markdown (strong-consistency), so the + strategy never races cascade indexing. Prior to this fix, running a fresh + agent trajectory produced zero `SKILL.md` files — `.skills/` did not exist. + **The related stale-index clobber is fully closed only for clusters at or + below `MAX_SKILLS_IN_PROMPT` (10).** Above it, markdown still supplies the + candidate set but LanceDB orders it, and the skill a lagging index omits is + by definition the one written most recently — the one most likely to need + `update` — so it can be ranked out of the prompt and re-added instead. The + window is narrow (it needs a cluster over 10 skills *and* an index that has + not caught up) and the consequence is the pre-existing full-replace, not a + new failure mode. +- **`POST /api/v2/ome/trigger` no longer masks strategy state.** The `status` + field now distinguishes `not_dispatched` (all dispatch gates rejected the + strategy — usually a missing `"force": true`) from `ok` (dispatched and + settled). The new `runs` field surfaces dead-lettered strategy runs that + were previously invisible to the caller. **If your client matches + `status` exhaustively (Python `Literal`, TypeScript union), add a + `not_dispatched` branch.** +- **Agentic search on agent memory now uses the skill-shaped rerank + passage.** The cross-encoder previously saw only the raw `description` + field instead of the `name + description + skill instruction` triple that + the HYBRID lane uses. A skill with empty `description` (a legal everalgo + output — see `everalgo/agent_memory/skill_ops.py:294`) no longer causes + HTTP 500 during the LLM sufficiency check. +- **OME strategy retries now back off between attempts.** A retry-class error + (e.g. waiting on eventually-consistent state) previously exhausted its + `max_retries` budget in milliseconds; the loop now sleeps + `min(base * 2**(attempt-1), cap)` plus up to `jitter` seconds + (defaults: `1s` base / `10s` cap / `0.5s` jitter — code-only defaults, + not currently exposed via `everos.toml` or `ome.toml`). **`engine_sem` is + now held per attempt rather than across the whole retry chain**, so the + backoff sleep does not occupy a concurrency slot. The cap bounds + concurrent strategy *work* — LLM calls, embeddings, storage IO — and a + coroutine waiting to retry consumes none of it; holding the slot would + have turned a partial outage into a total stall, since enough + simultaneously-failing runs park every one of the + `max_concurrent_runs` slots in `asyncio.sleep` and starve strategies that + would have succeeded. Backpressure on failing work is intended; + backpressure on everything else is not. +- **Path-traversal hardening for LLM-generated agent-skill names (CWE-22).** + `AgentSkillFrontmatter.name` comes straight from LLM output + (`extract_agent_skill`) and was concatenated unsanitized into the + `skills/skill_/` directory segment on both the write and read + paths; given a sufficiently long `../` prefix, the write target could + escape the memory root. This is the same class of defect previously + fixed for knowledge-upload titles/categories (see `knowledge_writer.py` + in an earlier 1.2.x). The sanitizer is now a single shared helper + (`everos.core.persistence.markdown.sanitize_dirname`) used by both + `KnowledgeWriter` and the new `SkillPathMixin.skill_dir_name()` / + `sanitize_skill_name()`, instead of two independently maintained copies. + `extract_agent_skill` now sanitizes the LLM-emitted name *before* + constructing `AgentSkillFrontmatter`, so **`AgentSkillFrontmatter.name` + and the LanceDB `agent_skill` primary key now hold the sanitized name** + (spaces become `_`, characters outside `[\w\-.]` are dropped, capped at + 50 chars), not the raw LLM output — a user-visible change for anything + that reads a skill's `name` field expecting the verbatim LLM string. + `AgentSkillFrontmatter.name` also gained a validator rejecting a name + containing a path separator, or being exactly `..`, so a hand-edited + `SKILL.md` that bypasses the writer's sanitization is caught on read + rather than silently relocated (the substring form, e.g. a name that + merely *contains* `..`, is deliberately allowed — sanitized output can + legitimately contain runs of literal dots). `sanitize_dirname` itself + falls back (not just on an empty result, but also on `.` or `..`) so a + short input that is itself a sanitizer fixpoint — e.g. `"../"` sanitizes + to `".."` verbatim without this fallback — cannot resolve to the same + directory or its parent; this closes both the agent-skill case and an + equivalent one-level escape on the knowledge-upload path, which has no + `skill_`-style prefix protecting its sanitized segment. **No data + migration is needed for agent skills**: extraction has never + successfully produced a `SKILL.md` before this release (see the + cascade-lag fix above), so there is no legacy skill corpus whose + directory names would change. **Knowledge documents do have a + pre-existing corpus**, and two inputs resolve to a different directory + than before: a decomposed (NFD) topic or category now keeps its + combining marks (`"Résumé"` no longer degrades to `"Resume"`) because + the shared helper NFC-normalizes first, and a topic or category of + exactly `.` or `..` now falls back instead of resolving onto the + parent directory. Precomposed input — including CJK — is unaffected; + the character class is unchanged from the previous private copy. + Sanitizing is lossy: skills whose raw names + differ only in characters the sanitizer drops or replaces (e.g. + `"fix django"` vs. `"fix_django"`) now share one `SKILL.md`, and so do + names differing only in a combining mark regardless of script (e.g. + Devanagari `"किताब"` vs. `"कताब"` — a combining mark alone is not `\w` + and is stripped either way; same for Thai tone marks, Hebrew niqqud, + Arabic harakat). The later write wins — the earlier skill's + `source_case_ids`, `maturity_score`, and body are silently lost, not + merged. Case is *not* folded, so `"Fix Django"` and `"fix django"` stay + two distinct sanitized names — two LanceDB rows, but one directory on a + case-insensitive filesystem (macOS APFS and Windows NTFS defaults), + where the index then advertises a name whose content was overwritten. + This is accepted for now rather than mitigated: detecting a collision + and raising would reintroduce the dead-letter DoS the sanitizer was + built to avoid, and a disambiguating suffix — the workable option — + needs a collision probe plus a case-folding rule, so it is deferred to + a deliberate pass rather than added here. +- **A renamed skill no longer leaves an orphan directory that pollutes the + next extraction.** everalgo treats a name change as a first-class update + (`skill_ops._apply_update` preserves `prior.id` while swapping the name), + so the emitted skill was written to a new `skill_/` while the + old directory survived carrying the same `cluster_id`. Because existing + skills are now read from markdown rather than LanceDB, that orphan did + not merely sit on disk — it came back in the next run's + `existing_relevant_skills` as a duplicate of a skill the LLM had already + renamed, feeding exactly the `add`-instead-of-`update` full-replace + clobber this release set out to close, once more per rename. The old + directory is now reaped after the new one is written, keyed on the + skill's `id` (the only thing that survives a rename; a fresh `add` mints + a uuid and can never match). A prior name that another skill in the same + batch just claimed is never deleted. +- **`extract_agent_skill` retire ops are documented as unimplemented rather + than silently mispersisted.** `AgentSkillExtractor.aextract` returns a + flat list with no op discriminator, so a retirement arrives as an + ordinary skill with `confidence < retire_confidence` and was written back + like any other — staying in markdown, in the next prompt, and in search. + The behaviour is unchanged; the module docstring no longer claims retire + is handled. Honouring it is a design decision (delete the directory, or + add a `retired` flag that the enumeration, cascade, and search all + filter on) deferred to its own change. +- **`reference_name` and `script_filename` are sanitized.** Both are + appended *after* the `skill_` segment, so `skill_dir_name` never + covered them; they now go through the same `sanitize_dirname` primitive + on both the reader and the writer. No caller in `src/` reaches them + today, so nothing was exploitable — this closes the gap before + progressive disclosure wires them up. +- **A single unparseable `SKILL.md` no longer disables skill extraction + for its whole cluster.** `AgentSkillReader.list_by_cluster` propagated + any frontmatter `ValidationError`, which aborted the enumeration that + feeds `extract_agent_skill` its existing skills — so one hand-edited + file (or, after a future schema revision adds a required field, every + existing file at once) dead-lettered that cluster's extraction on every + run. Offending files are now logged and skipped. `read_main` still + raises, since a caller naming one specific skill needs to hear about + corruption rather than receive the `None` that already means "not + created yet". + merged. This is accepted, not mitigated, on two grounds: a + disambiguating suffix would break the `name` ≡ directory-suffix + identity the reader/writer relies on, and detecting a collision and + raising would reintroduce the dead-letter DoS the sanitizer was built + to avoid. + `"fix django"` vs. `"fix_django"`) now share one `SKILL.md`, and the + later write wins — the earlier skill's `source_case_ids`, + `maturity_score`, and body are silently lost, not merged. This is + accepted, not mitigated: the LLM's add/update decision is keyed on the + name it sees, so a collision usually reads as an intended update + anyway. + under the new sanitizer. + `KnowledgeWriter` and the new `SkillPathMixin.skill_dir_name()`, instead + of two independently maintained copies. `AgentSkillFrontmatter.name` + also gained a validator rejecting path separators / `..` so a + hand-edited `SKILL.md` is caught on read rather than silently + relocated. No data migration: agent-skill extraction has never + successfully produced a `SKILL.md` before this release (see the + cascade-lag fix above), so there is no legacy skill corpus whose + directory names would change under the new sanitizer. - **Reads now carry a deadline** (`count` / `get_by_id` / `find_where` / `find_where_paginated` / `search`). The write-side deadline work skipped them on the reasoning that a read takes no lock and so blocks no writer — true, but @@ -134,8 +276,73 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 near the legitimate hold turns a slow migration into startup crashes for every waiting process. +### Added + +- `OfflineEngine.trigger_manual` now returns + `tuple[BaseEvent, list[tuple[StrategyMeta, str]]]` instead of `None`, + enabling the `dispatched`/`runs` fields below. +- `TriggerResponse` gains `dispatched: int` and `runs: list[RunSummary]`. +- `OMEConfig` gains `retry_backoff_base_seconds`, `retry_backoff_cap_seconds`, + and `retry_jitter_seconds` for the retry-loop sleep. +- `AgentSkillReader.list_by_cluster()` enumerates the cluster's SKILL.md + files from markdown (strong-consistency existence check). +- **`[cascade]` settings section** — the four maintenance cadences + (`optimize_heartbeat_seconds`, `optimize_prune_interval_seconds`, + `optimize_prune_retention_seconds`, `optimize_rebuild_interval_seconds`) are + now configurable. They were already constructor arguments on `CascadeWorker`, + but `CascadeConfig` did not carry them and no production path passed one, so + the defaults were unreachable — which is why the 12h rebuild sweep could not + be exercised by any soak run shorter than half a day. The deadlines that + bound a hung call are deliberately **not** exposed: they are hang-catchers + sized from measured durations, where too low manufactures failures on a + healthy table and too high leaves a wedged one invisible for longer. Note + `optimize_prune_retention_seconds` has a second effect worth reading before + tuning — it also decides how long index files keep a manifest naming them, + and below LanceDB's 7-day unverified window they then wait out the full 7 + days. + ### Changed +- **`extract_foresight` now ships disabled** (`enabled=False`). Not because + it is broken — the crash below is fixed — but because it is one LLM call + per sender per memcell whose output nothing in EverOS reads today: no + search route surfaces foresights and no prompt slot consumes them. Until + something does, running it by default spends tokens on write-only data. + **Re-enable per install** in `ome.toml` (hot-reloaded, no restart): + + ```toml + [strategies.extract_foresight] + enabled = true + ``` + + Editing `default_ome.toml` alone would not have reached existing installs + — `everos init` does not overwrite an existing `~/.everos/ome.toml` — so + the code default is what changed. +- **`extract_foresight` no longer crashes on a memcell containing tool + calls.** The sender scan read `m.role` off every item, but only + `ChatMessage` carries it (`ToolCallRequest` has `sender_id` without it, + `ToolCallResult` has neither), so the first tool call raised + `AttributeError` — before any sender was resolved. The strategy was + correct on plain user chat and dead-lettered every time on agent + trajectories. everalgo explicitly contracts for the mixed case + (`user_memory/_render.chat_messages`: the caller need not pre-filter), + and every other user-memory extractor gets that for free by delegating; + this was the one place the filter was hand-rolled. The scan now tests + `isinstance(m, ChatMessage)`, so a pure agent trajectory yields no senders + and returns without an LLM call. Matters even with the strategy off by + default: it is what makes the opt-in above actually usable. +- **`SkillClusterUpdated` carries the case's 1024-dim embedding, growing the + OME `run_record` table.** The event payload is persisted verbatim in + `run_record.event_payload` (and in the APScheduler jobstore while a job is + queued), so a `skill_cluster_updated` record goes from roughly 0.8 KB to + 14 KB. At the default `max_records_per_strategy = 1000` ring buffer that is + ~14 MB for this one strategy instead of ~0.8 MB. **Operators sizing + `~/.everos/.index/sqlite/ome.db` should expect this.** The vector is only + read when a cluster holds more skills than `MAX_SKILLS_IN_PROMPT`, so it + usually rides along unused; trimming it from the persisted copy is not a + local change, because crash recovery replays `event_payload` to rebuild the + event and a trimmed payload would silently take the recovered run down a + different branch than the original. Tracked as a follow-up. - **`cascade_lancedb_optimize_conflict` now records `pruned`** — which maintenance beat lost the commit race. Lance labels both beats' commit the same way (`This Rewrite transaction was preempted by concurrent transaction diff --git a/docs/api.md b/docs/api.md index 2d7c2548c..7bb529b45 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1070,8 +1070,32 @@ Manually trigger a registered OME strategy. | Field | Type | Notes | |---|---|---| -| `status` | `"ok" \| "timeout"` | Whether the strategy completed within the timeout | +| `status` | `"ok" \| "timeout" \| "not_dispatched"` | `ok` = every dispatched run settled — a dead-lettered run still counts as settled (see `runs[*].error`); `timeout` = at least one run had not settled when `timeout` elapsed; `not_dispatched` = no strategy was dispatched (see below) | | `name` | `string` | Echoes the requested strategy name | +| `dispatched` | `int` | Number of strategy routes enqueued. `0` iff `status == "not_dispatched"` | +| `runs` | `list[RunSummary]` | One entry per strategy run *attempt*, not per dispatched route: `{run_id: string, status: string, error?: string}`. A strategy that retried before settling contributes multiple entries sharing one `event_id`. `status` is one of `running` / `success` / `failed` / `dead_letter` / `crashed`. Includes dead-lettered runs | + +**`not_dispatched`** means every subscriber was rejected by one of the +four dispatch gates (`_routes_to` / `enabled` / `applies_to` / +`Counter`). The most common cause is forgetting `"force": true` on a +strategy that is `enabled=false` in `ome.toml` — e.g. triggering +`reflect_episodes` without `force` while it is disabled in config +returns `{"status": "not_dispatched", "dispatched": 0, "runs": []}` +instead of an error. + +> `status: "ok"` means all dispatched strategy runs settled — including +> runs that dead-lettered (their errors are in `runs[*].error`). It does +> **not** mean the LanceDB index has caught up. Markdown is written +> synchronously; the index syncs asynchronously (see +> [Eventual consistency](#eventual-consistency)). +> +> If you need read-your-write semantics, poll `GET /health`'s +> `cascade.pending` field until it reads `0` on two consecutive samples +> (a single zero can be a false convergence — the watcher-input window +> can briefly report an empty queue between md write and enqueue). + +See [docs/openapi.json](openapi.json) for the exact generated schema +(`TriggerResponse` / `RunSummary`) behind this table. #### Errors diff --git a/docs/openapi.json b/docs/openapi.json index 63653c6db..ebe35b820 100644 --- a/docs/openapi.json +++ b/docs/openapi.json @@ -3,7 +3,7 @@ "info": { "title": "everos", "description": "md-first memory extraction framework", - "version": "1.2.2" + "version": "1.2.3" }, "paths": { "/health": { @@ -222,7 +222,7 @@ "ome" ], "summary": "Trigger", - "description": "Manually trigger a registered OME strategy and wait for completion.", + "description": "Manually trigger a registered OME strategy and wait for its runs to\nsettle. Returns without waiting for the LanceDB index — the response\nreflects markdown state only; poll ``GET /health``'s ``cascade.pending``\nfor index convergence (two consecutive zero samples to guard against the\nwatcher-input window). See docs/api.md#eventual-consistency.", "operationId": "trigger_api_v1_ome_trigger_post", "requestBody": { "content": { @@ -982,7 +982,7 @@ "ome" ], "summary": "Trigger", - "description": "Manually trigger a registered OME strategy and wait for completion.", + "description": "Manually trigger a registered OME strategy and wait for its runs to\nsettle. Returns without waiting for the LanceDB index — the response\nreflects markdown state only; poll ``GET /health``'s ``cascade.pending``\nfor index convergence (two consecutive zero samples to guard against the\nwatcher-input window). See docs/api.md#eventual-consistency.", "operationId": "trigger_api_v2_ome_trigger_post", "requestBody": { "content": { @@ -3089,6 +3089,36 @@ ], "title": "MessageItemDTO" }, + "RunSummary": { + "properties": { + "run_id": { + "type": "string", + "title": "Run Id" + }, + "status": { + "type": "string", + "title": "Status" + }, + "error": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Error" + } + }, + "type": "object", + "required": [ + "run_id", + "status" + ], + "title": "RunSummary", + "description": "One strategy run within a trigger response." + }, "SearchAgentCaseItem": { "properties": { "id": { @@ -4006,15 +4036,28 @@ "name": { "type": "string", "title": "Name" + }, + "dispatched": { + "type": "integer", + "title": "Dispatched" + }, + "runs": { + "items": { + "$ref": "#/components/schemas/RunSummary" + }, + "type": "array", + "title": "Runs", + "default": [] } }, "type": "object", "required": [ "status", - "name" + "name", + "dispatched" ], "title": "TriggerResponse", - "description": "Response body for ``POST /api/v2/ome/trigger``." + "description": "Response body for ``POST /api/v2/ome/trigger``.\n\n``status`` distinguishes three outcomes that were previously masked as\na single ``ok``:\n\n- ``ok``: at least one strategy was dispatched and all runs settled\n within ``timeout``. Individual run outcomes are in ``runs`` (a\n ``dead_letter`` there is still ``ok`` at this level — the strategy\n *ran*, it just failed permanently).\n- ``timeout``: at least one strategy was dispatched but the engine did\n not go idle within ``timeout``. Runs may be partially complete;\n poll ``GET /health`` for cascade convergence separately.\n- ``not_dispatched``: no strategy was dispatched — the subscriber was\n rejected by one of the dispatch gates (``_routes_to`` / ``enabled`` /\n ``applies_to`` / ``Counter``). Common cause: forgetting\n ``force=true`` on a strategy that is ``enabled=false`` in ome.toml." }, "UnprocessedMessageDTO": { "properties": { diff --git a/pyproject.toml b/pyproject.toml index a413d2d7f..991d9c91b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "everos" -version = "1.2.2" +version = "1.2.3" description = "EverOS — local-first markdown memory framework for AI agents and user chats; lightweight, dev-friendly, small-team" license = {text = "Apache-2.0"} readme = "README.md" diff --git a/src/everos/config/default_ome.toml b/src/everos/config/default_ome.toml index e326346a6..f20de373a 100644 --- a/src/everos/config/default_ome.toml +++ b/src/everos/config/default_ome.toml @@ -29,8 +29,12 @@ # Atomic fact extraction runs per memcell. Always enabled — search # depends on atomic facts for MaxSim retrieval. -# Foresight extraction (runs per memcell). Heavy LLM call — set -# enabled = false to skip in evaluation / benchmark runs. +# Foresight extraction (runs per memcell). Heavy LLM call. +# +# DISABLED BY DEFAULT since 1.2.3: nothing in EverOS reads foresights yet +# — no search route surfaces them, no prompt slot consumes them — so +# running it spends one LLM call per sender per memcell on write-only +# data. Uncomment to opt in; it works on agent and chat input alike. # [strategies.extract_foresight] # enabled = true diff --git a/src/everos/core/persistence/__init__.py b/src/everos/core/persistence/__init__.py index cb114b84e..6e934555f 100644 --- a/src/everos/core/persistence/__init__.py +++ b/src/everos/core/persistence/__init__.py @@ -16,6 +16,8 @@ # Frontmatter schema chassis BaseFrontmatter, UserScopedFrontmatter, AgentScopedFrontmatter, DailyLogPathMixin, SkillPathMixin, + # Path safety + sanitize_dirname, # Async SQLite (SQLModel / SA 2.0) create_system_engine, create_session_factory, session_scope, SQLModel, Field, Relationship, BaseTable, RepoBase, @@ -49,6 +51,7 @@ from .markdown import parse_frontmatter as parse_frontmatter from .markdown import parse_structured_entry as parse_structured_entry from .markdown import render_structured_entry as render_structured_entry +from .markdown import sanitize_dirname as sanitize_dirname from .markdown import split_entries as split_entries from .memory_root import MemoryRoot as MemoryRoot from .memory_root import app_dir_name as app_dir_name @@ -100,6 +103,7 @@ "project_dir_name", "project_id_from_dir", "render_structured_entry", + "sanitize_dirname", "session_scope", "split_entries", "touch", diff --git a/src/everos/core/persistence/markdown/__init__.py b/src/everos/core/persistence/markdown/__init__.py index 54a9b0e13..0315086f9 100644 --- a/src/everos/core/persistence/markdown/__init__.py +++ b/src/everos/core/persistence/markdown/__init__.py @@ -21,6 +21,9 @@ KnowledgeScopedMixin, KnowledgeDocumentPathMixin, KnowledgeTopicPathMixin, ) + +External usage (path safety): + from everos.core.persistence.markdown import sanitize_dirname """ from .entries import Entry as Entry @@ -42,6 +45,7 @@ from .frontmatter import dump_frontmatter as dump_frontmatter from .frontmatter import parse_frontmatter as parse_frontmatter from .parsed import ParsedMarkdown as ParsedMarkdown +from .path_safety import sanitize_dirname as sanitize_dirname from .reader import MarkdownReader as MarkdownReader from .writer import MarkdownWriter as MarkdownWriter @@ -66,5 +70,6 @@ "parse_frontmatter", "parse_structured_entry", "render_structured_entry", + "sanitize_dirname", "split_entries", ] diff --git a/src/everos/core/persistence/markdown/frontmatter.py b/src/everos/core/persistence/markdown/frontmatter.py index 78760aadd..97eb77f91 100644 --- a/src/everos/core/persistence/markdown/frontmatter.py +++ b/src/everos/core/persistence/markdown/frontmatter.py @@ -36,6 +36,8 @@ import yaml from pydantic import BaseModel, ConfigDict +from .path_safety import sanitize_dirname + # ── YAML helpers ──────────────────────────────────────────────────────── _DELIM = "---" @@ -229,6 +231,15 @@ class AgentSkillFrontmatter(SkillPathMixin, AgentScopedFrontmatter): SKILL_DIR_PREFIX: ClassVar[str] = "skill_" SKILL_MAIN_FILENAME: ClassVar[str] = "SKILL.md" ... + + ``skill_dir_name`` / ``sanitize_skill_name`` are the single + sanitization point both ``AgentSkillWriter`` and ``AgentSkillReader`` + derive their ``skill_`` directory segment from, and that + ``memory.strategies.extract_agent_skill._persist_skill`` uses to + sanitize LLM-emitted ``skill_name`` *before* constructing + ``AgentSkillFrontmatter`` — ``skill_name`` is LLM output and must not + reach the filesystem, or the frontmatter's traversal validator, + unsanitized (CWE-22). """ SKILLS_CONTAINER_NAME: ClassVar[str] @@ -244,6 +255,83 @@ def path_glob(cls) -> str: f"{cls.SKILL_DIR_PREFIX}*/{cls.SKILL_MAIN_FILENAME}" ) + @classmethod + def sanitize_skill_name(cls, skill_name: str) -> str: + """Bare sanitized skill name (no ``skill_`` prefix). + + The single sanitization point for a skill's ``name`` value itself — + as opposed to :meth:`skill_dir_name`, which additionally prefixes + it for the directory segment. Callers building + ``AgentSkillFrontmatter.name`` from LLM output (see + ``memory.strategies.extract_agent_skill._persist_skill``) route + through this *before* constructing the frontmatter, so + ``frontmatter.name`` ends up byte-identical to the directory-derived + name rather than merely idempotent-if-resanitized. + + This is lossy: distinct raw names can collapse onto the same + sanitized name. Dropped punctuation, space/underscore collapse, and + the 50-character cap are the visible cases (``"fix django"`` and + ``"fix_django"`` both become ``"fix_django"``; ``"fix!django"`` and + ``"fixdjango"`` both become ``"fixdjango"``). The larger case is + every combining mark: a combining mark alone is not ``\\w``, so it + is stripped regardless of script, and two names that differ only in + their marks collide — e.g. Devanagari ``"किताब"`` and ``"कताब"`` + both sanitize to ``"कतब"``; the same holds for Thai tone marks, + Hebrew niqqud, and Arabic harakat. + + Case is *not* folded, which makes the collision above + filesystem-dependent rather than universal, and is the dimension an + LLM varies most freely: ``"Fix Django"`` → ``"Fix_Django"`` and + ``"fix django"`` → ``"fix_django"`` are two distinct sanitized + names, so they are two rows in LanceDB (a case-sensitive Python + string key) but one directory on a case-insensitive filesystem — + macOS APFS and Windows NTFS in their default configurations. That + splits the invariant this seam otherwise maintains: the surviving + ``SKILL.md`` carries one of the two names in its frontmatter while + the index still advertises both, so a search hit on the shadowed + name resolves to the other skill's content. On a case-sensitive + filesystem the same pair simply stays two independent skills. + + Because ``AgentSkillWriter.write_main`` is a full-file replace and + the LanceDB primary key is ``f"{agent_id}_{sanitized_name}"``, a + collision means the later skill silently overwrites the earlier + one — its accumulated ``source_case_ids``, ``maturity_score``, and + body are lost, not merged. + + This is deliberate, not an oversight — but not because a collision + "usually reads as an intended update". ``_persist_skill`` sanitizes + *before* constructing the frontmatter, so the LLM is shown the + already-sanitized name in ``existing_relevant_skills``; when it + then emits a raw name like ``"fix django"`` after having just been + shown ``"fix_django"``, it has affirmatively treated them as two + different skills, and the write silently merges them anyway. + + What justifies accepting it is narrower: the two alternatives are + both worse here. Detecting a collision and raising would + reintroduce the dead-letter DoS this sanitizer was built to avoid — + LLM output would again decide whether a run survives. Appending a + disambiguating suffix is the real candidate and is left for a + deliberate design pass, not dismissed: it does *not* break the + ``frontmatter.name`` ≡ directory-suffix identity (writing + ``"fix_django_2"`` into both keeps that intact), but it does need a + collision probe on a path that currently touches no other skill, + and a rule for the case-insensitive-filesystem variant above where + the probe must compare case-folded while the key stays exact. + """ + return sanitize_dirname(skill_name, fallback="unnamed") + + @classmethod + def skill_dir_name(cls, skill_name: str) -> str: + """Sanitized ``skill_`` directory segment (traversal-safe). + + Idempotent in ``skill_name``: calling this again on an already + sanitized name (e.g. one recovered by walking the directory tree) + returns the same segment, so a reader deriving ``skill_name`` from + the on-disk directory and a writer deriving it from raw LLM output + land on the same path. + """ + return f"{cls.SKILL_DIR_PREFIX}{cls.sanitize_skill_name(skill_name)}" + class ProfilePathMixin: """Path strategy for single-file profile markdown. diff --git a/src/everos/core/persistence/markdown/path_safety.py b/src/everos/core/persistence/markdown/path_safety.py new file mode 100644 index 000000000..974061c7a --- /dev/null +++ b/src/everos/core/persistence/markdown/path_safety.py @@ -0,0 +1,83 @@ +"""``sanitize_dirname`` — the single path-safety primitive for md directory names. + +Several markdown layouts turn free-text into a filesystem path segment: +knowledge document/category titles, and agent-skill names. Both sources are +untrusted in the same way — knowledge titles come from parsed source +documents, skill names come straight from LLM output — so a name containing +``../`` or a path separator must never survive into a directory segment +(CWE-22 path traversal). + +This module is the one place that decision is made. Callers that need a +filesystem-safe segment from a free-text string route through +:func:`sanitize_dirname` rather than keeping a private regex copy — see +``writers/knowledge_writer.py`` and +:meth:`SkillPathMixin.skill_dir_name() <.frontmatter.SkillPathMixin.skill_dir_name>`. +Some callers (``knowledge_writer.py``) concatenate the result directly under a +shared directory with no per-caller prefix, so the guarantee below has to hold +on its own, without relying on a prefix like ``skill_`` to absorb a degenerate +result. + +``sanitize_dirname`` is idempotent (``sanitize_dirname(sanitize_dirname(x), +fb) == sanitize_dirname(x, fb)``): a name built by re-sanitizing an +already-sanitized segment (e.g. one derived by walking the directory tree) +lands on the same string as sanitizing the original raw name. That property +is what lets a reader and a writer agree on a path even when one side has +only the raw name and the other only the on-disk directory name. +""" + +from __future__ import annotations + +import re +import unicodedata + +_MAX_DIRNAME_LEN = 50 +_SAFE_CHARS = re.compile(r"[^\w\-.]", re.UNICODE) +_DEGENERATE = frozenset({"", ".", ".."}) + + +def sanitize_dirname(raw: str, fallback: str) -> str: + """Produce a safe directory/file name segment from free-text input. + + * NFC-normalize first. For an ordinary decomposed (NFD) input — a base + letter plus a combining mark, e.g. ``"e"`` + combining acute accent — + this collapses to the precomposed form before the character filter + runs, so the accent survives (a combining mark alone is not ``\\w`` + and would otherwise be silently stripped). This is best-effort, not a + guarantee: for the ~1,082 Unicode *composition exclusion* codepoints + (e.g. Devanagari ``क़``/``ख़``, U+0958/U+0959), NFC does the + opposite — it *decomposes* an already-precomposed exclusion + character, because recomposing it is explicitly excluded from the + NFC algorithm, and the resulting combining mark is then stripped just + the same. Normalizing here improves fidelity for the common case; it + does not make every Unicode script round-trip losslessly. + * Replace spaces with underscores. + * Strip characters outside ``[a-zA-Z0-9_\\-.]`` (``\\w`` is Unicode-aware, + so CJK and other non-ASCII scripts survive readably). Note that ``.`` + is a *safe* character, not stripped — a run of literal dots is a legal + result of this step. + * Truncate to 50 characters. + * Fall back to *fallback* if the result is empty, ``"."``, or ``".."``. + + Every path separator (``/``, ``\\``) is stripped by the character-class + filter, so no separator survives and the result is always exactly one + path component — it can never be split into multiple segments by a + downstream ``Path(...) / result``. The fallback on ``""`` / ``"."`` / + ``".."`` closes the remaining gap: those are the only single components + that resolve to *no new child* (``""`` and ``"."`` both mean "this same + directory", ``".."`` means "its parent") rather than a genuinely new + entry. With both guarantees together, ``Path(some_dir) / sanitize_dirname(raw, fb)`` + can never escape ``some_dir`` and never silently collapses back onto it + or its parent — unconditionally, including for a caller with no + additional prefix (like ``skill_``) protecting the segment. + + This function is lossy and not injective: distinct inputs can sanitize + to the same output (dropped characters, space/underscore collapse, and + truncation are all many-to-one). A caller that needs distinct outputs + for distinct inputs must disambiguate before or after calling this — + the function itself makes no such guarantee. + """ + slug = unicodedata.normalize("NFC", raw) + slug = slug.replace(" ", "_") + slug = _SAFE_CHARS.sub("", slug) + slug = slug[:_MAX_DIRNAME_LEN] + return slug if slug not in _DEGENERATE else fallback diff --git a/src/everos/entrypoints/api/routes/ome.py b/src/everos/entrypoints/api/routes/ome.py index c4ac2a22a..375b519a2 100644 --- a/src/everos/entrypoints/api/routes/ome.py +++ b/src/everos/entrypoints/api/routes/ome.py @@ -2,12 +2,23 @@ from __future__ import annotations +from typing import TYPE_CHECKING + from fastapi import APIRouter from pydantic import BaseModel from everos.core.errors import NotFoundError from everos.core.observability.logging import get_logger +if TYPE_CHECKING: + # Type-only — used solely to annotate `_summarize_runs`. Importing the + # engine eagerly costs ~750ms (apscheduler + aiosqlite, 26 modules), so + # keep it out of the runtime path even though `service.memorize` already + # imports it eagerly and today's app startup pays that cost regardless: + # this router's own `_get_engine` import is deliberately deferred (see + # `trigger`), and an eager import here would contradict it. + from everos.infra.ome.engine import OfflineEngine + router = APIRouter(prefix="/ome", tags=["ome"]) logger = get_logger(__name__) @@ -21,27 +32,89 @@ class TriggerRequest(BaseModel): force: bool = False +class RunSummary(BaseModel): + """One strategy run within a trigger response.""" + + run_id: str + status: str + """One of: running / success / failed / dead_letter / crashed.""" + error: str | None = None + + class TriggerResponse(BaseModel): - """Response body for ``POST /api/v2/ome/trigger``.""" + """Response body for ``POST /api/v2/ome/trigger``. + + ``status`` distinguishes three outcomes that were previously masked as + a single ``ok``: + + - ``ok``: at least one strategy was dispatched and all runs settled + within ``timeout``. Individual run outcomes are in ``runs`` (a + ``dead_letter`` there is still ``ok`` at this level — the strategy + *ran*, it just failed permanently). + - ``timeout``: at least one strategy was dispatched but the engine did + not go idle within ``timeout``. Runs may be partially complete; + poll ``GET /health`` for cascade convergence separately. + - ``not_dispatched``: no strategy was dispatched — the subscriber was + rejected by one of the dispatch gates (``_routes_to`` / ``enabled`` / + ``applies_to`` / ``Counter``). Common cause: forgetting + ``force=true`` on a strategy that is ``enabled=false`` in ome.toml. + """ status: str + """One of: ok / timeout / not_dispatched.""" name: str + dispatched: int + """Number of strategy routes that were enqueued. ``0`` iff status is + ``not_dispatched``.""" + runs: list[RunSummary] = [] + """One entry per strategy run *attempt*, not per dispatched route: a + strategy that retried before settling contributes multiple entries + sharing one ``event_id``. Includes dead-lettered runs whose errors + would otherwise be invisible to the caller (they live in the SQLite + ``run_record`` table with no HTTP surface until this field was + added).""" @router.post("/trigger", response_model=TriggerResponse) async def trigger(req: TriggerRequest) -> TriggerResponse: - """Manually trigger a registered OME strategy and wait for completion.""" + """Manually trigger a registered OME strategy and wait for its runs to + settle. Returns without waiting for the LanceDB index — the response + reflects markdown state only; poll ``GET /health``'s ``cascade.pending`` + for index convergence (two consecutive zero samples to guard against the + watcher-input window). See docs/api.md#eventual-consistency. + """ # Deferred: avoid importing heavy OME engine at module level. from everos.service.memorize import _get_engine engine = _get_engine() try: - await engine.trigger_manual(req.name, force=req.force) + event, routes = await engine.trigger_manual(req.name, force=req.force) except KeyError: raise NotFoundError(f"strategy '{req.name}' not found") from None - logger.info("ome_trigger_manual", strategy=req.name) + + if not routes: + logger.info("ome_trigger_manual_not_dispatched", strategy=req.name) + return TriggerResponse( + status="not_dispatched", name=req.name, dispatched=0, runs=[] + ) + + logger.info("ome_trigger_manual", strategy=req.name, dispatched=len(routes)) idle = await engine.wait_idle(timeout=req.timeout) + runs = await _summarize_runs(engine, event.event_id) if not idle: logger.warning("ome_trigger_timeout", strategy=req.name, timeout=req.timeout) - return TriggerResponse(status="timeout", name=req.name) - return TriggerResponse(status="ok", name=req.name) + return TriggerResponse( + status="timeout", name=req.name, dispatched=len(routes), runs=runs + ) + return TriggerResponse( + status="ok", name=req.name, dispatched=len(routes), runs=runs + ) + + +async def _summarize_runs(engine: OfflineEngine, event_id: str) -> list[RunSummary]: + """Fetch and shape run records for one event into the response DTO.""" + records = await engine.list_runs_by_event_id(event_id) + return [ + RunSummary(run_id=r.run_id, status=r.status.value, error=r.error) + for r in records + ] diff --git a/src/everos/infra/ome/_dispatch/runner.py b/src/everos/infra/ome/_dispatch/runner.py index c7a1b1711..ac71c3393 100644 --- a/src/everos/infra/ome/_dispatch/runner.py +++ b/src/everos/infra/ome/_dispatch/runner.py @@ -20,6 +20,7 @@ from __future__ import annotations import asyncio +import random import traceback from collections.abc import Awaitable, Callable from typing import TYPE_CHECKING @@ -32,6 +33,7 @@ from everos.core.observability.tracing import memory_span, use_traceparent from everos.infra.ome._dispatch._state import _CURRENT_STRATEGY from everos.infra.ome._stores.run_record import RunRecordStore +from everos.infra.ome.config import OMEConfig from everos.infra.ome.decorator import StrategyMeta from everos.infra.ome.events import BaseEvent from everos.infra.ome.exceptions import EmitNotDeclaredError, StrategyContractError @@ -98,12 +100,14 @@ def __init__( run_record_store: RunRecordStore, engine_sem: asyncio.Semaphore, emit_hook: Callable[[BaseEvent], Awaitable[None]], + config: OMEConfig, on_dead_letter: Callable[[RunRecord], None] | None = None, engine: OfflineEngine, ) -> None: self._rec = run_record_store self._sem = engine_sem self._emit_hook = emit_hook + self._config = config self._on_dead_letter = on_dead_letter self._engine = engine @@ -118,23 +122,34 @@ async def run( ) -> None: """Execute ``meta.func(event, ctx)`` with the attempt retry loop. - Holds ``engine_sem`` for the full retry chain so concurrency cap - applies end-to-end. Each attempt gets a fresh ``run_id`` after - the first, so the run history records every try. + ``engine_sem`` is held per *attempt*, not across the whole retry + chain: the backoff sleep happens outside it. The cap exists to + bound concurrent strategy work — LLM calls, embeddings, storage + IO — and a coroutine sleeping between attempts consumes none of + that. Holding the slot across the sleep turned a partial outage + into a total stall: with ``max_concurrent_runs`` slots and a + ``1s → 2s → 4s`` backoff, enough simultaneously-failing runs + park every slot in ``asyncio.sleep`` and starve strategies that + would have succeeded. Backpressure on the failing work is + wanted; backpressure on everything else is not. + + Each attempt gets a fresh ``run_id`` after the first, so the run + history records every try. """ if max_retries_snapshot < 0: raise ValueError( f"max_retries_snapshot must be >= 0, got {max_retries_snapshot}" ) - async with self._sem: - event_topic = type(event).topic() - event_payload = event.model_dump_json() - current_run_id = run_id + event_topic = type(event).topic() + event_payload = event.model_dump_json() + current_run_id = run_id - for attempt in range(max_retries_snapshot + 1): - if attempt > 0: - current_run_id = uuid4().hex + for attempt in range(max_retries_snapshot + 1): + if attempt > 0: + await self._sleep_backoff(attempt) + current_run_id = uuid4().hex + async with self._sem: terminated = await self._run_one_attempt( meta=meta, event=event, @@ -145,8 +160,25 @@ async def run( max_retries_snapshot=max_retries_snapshot, traceparent=traceparent, ) - if terminated: - return + if terminated: + return + + async def _sleep_backoff(self, attempt: int) -> None: + """Sleep before retry ``attempt`` (1-indexed): ``base * 2**(attempt-1)``, + capped at ``retry_backoff_cap_seconds``, plus up to + ``retry_jitter_seconds`` of uniform jitter. ``retry_backoff_base_seconds + == 0.0`` disables backoff entirely (used by tests that don't + monkeypatch ``asyncio.sleep``). + """ + base = self._config.retry_backoff_base_seconds + if base <= 0.0: + return + cap = self._config.retry_backoff_cap_seconds + jitter_max = self._config.retry_jitter_seconds + sleep_seconds = min(base * (2 ** (attempt - 1)), cap) + if jitter_max > 0.0: + sleep_seconds += random.uniform(0.0, jitter_max) + await asyncio.sleep(sleep_seconds) async def _run_one_attempt( self, diff --git a/src/everos/infra/ome/config.py b/src/everos/infra/ome/config.py index d94671275..7786a3efc 100644 --- a/src/everos/infra/ome/config.py +++ b/src/everos/infra/ome/config.py @@ -109,6 +109,35 @@ class OMEConfig(BaseModel): "0 disables retries.", ), ] = 1 + retry_backoff_base_seconds: Annotated[ + float, + Field( + ge=0.0, + description=( + "Base seconds for exponential retry backoff (sleep between " + "attempts). attempt N waits base * 2**(N-1), capped at " + "retry_backoff_cap_seconds, plus up to retry_jitter_seconds " + "of random jitter. 0.0 disables backoff." + ), + ), + ] = 1.0 + retry_backoff_cap_seconds: Annotated[ + float, + Field( + ge=0.0, + description="Upper bound on the exponential backoff sleep before jitter.", + ), + ] = 10.0 + retry_jitter_seconds: Annotated[ + float, + Field( + ge=0.0, + description=( + "Uniform [0, retry_jitter_seconds] noise added to each " + "backoff sleep to spread retry storms." + ), + ), + ] = 0.5 max_records_per_strategy: Annotated[ int, Field( diff --git a/src/everos/infra/ome/engine.py b/src/everos/infra/ome/engine.py index 52f2409a0..212cef761 100644 --- a/src/everos/infra/ome/engine.py +++ b/src/everos/infra/ome/engine.py @@ -319,6 +319,7 @@ def _init_components(self) -> None: run_record_store=self._run_record_store, engine_sem=self._engine_sem, emit_hook=self._dispatch_event, + config=self._config, on_dead_letter=self._on_dead_letter, engine=self, ) @@ -614,7 +615,7 @@ async def trigger_manual( *, event: BaseEvent | None = None, force: bool = False, - ) -> None: + ) -> tuple[BaseEvent, list[tuple[StrategyMeta, str]]]: """Manually trigger one strategy. - ``event=None`` → engine self-emits ``ManualTick(strategy_name=name)`` @@ -624,6 +625,14 @@ async def trigger_manual( Routes through :meth:`EventDispatcher.dispatch` with ``strategy_filter=name`` so the same three-gate logic is applied as for engine-driven dispatch. + + Returns: + Tuple of ``(event, routes)``: + - ``event``: the event that was dispatched (either supplied + or the engine-generated ``ManualTick``). + - ``routes``: the ``(meta, run_id)`` pairs that were + enqueued. Empty list when every dispatch gate rejected + the strategy. """ if not self._started: raise OMEError("trigger_manual: engine not started") @@ -636,6 +645,7 @@ async def trigger_manual( ) for meta, run_id in routes: self._enqueue_run(meta, event, run_id) + return event, routes def _enqueue_run(self, meta: StrategyMeta, event: BaseEvent, run_id: str) -> None: """Add a one-shot APScheduler job that hands the event to Runner. diff --git a/src/everos/infra/ome/testing/harness.py b/src/everos/infra/ome/testing/harness.py index 6cde3d987..24ec0262d 100644 --- a/src/everos/infra/ome/testing/harness.py +++ b/src/everos/infra/ome/testing/harness.py @@ -42,6 +42,7 @@ def __init__(self) -> None: config_watch=False, max_concurrent_runs=20, max_retries=1, + retry_backoff_base_seconds=0.0, ) self._engine = OfflineEngine(config=cfg) diff --git a/src/everos/infra/persistence/markdown/mds/agent_skill.py b/src/everos/infra/persistence/markdown/mds/agent_skill.py index 65e4d6619..fcff0139e 100644 --- a/src/everos/infra/persistence/markdown/mds/agent_skill.py +++ b/src/everos/infra/persistence/markdown/mds/agent_skill.py @@ -19,6 +19,8 @@ import datetime as _dt from typing import ClassVar, Literal +from pydantic import field_validator + from everos.core.persistence.markdown import ( AgentScopedFrontmatter, SkillPathMixin, @@ -38,8 +40,44 @@ class AgentSkillFrontmatter(SkillPathMixin, AgentScopedFrontmatter): name: str """Skill identifier — also the directory suffix - (``skills/skill_/``). Keep snake_case so it is filesystem-safe - and ID-stable.""" + (``skills/skill_/``, sanitized via + :meth:`SkillPathMixin.skill_dir_name`). Keep snake_case so it stays + readable and ID-stable; the directory segment is sanitized regardless.""" + + @field_validator("name") + @classmethod + def _reject_path_traversal(cls, value: str) -> str: + """Catch a frontmatter ``name`` that bypassed the writer's sanitizer. + + The normal write path + (``memory.strategies.extract_agent_skill._persist_skill``) sanitizes + LLM-emitted ``skill_name`` via + :meth:`SkillPathMixin.sanitize_skill_name` *before* constructing this + model, so ``name`` is traversal-free by the time it gets here on that + path — this validator should not normally fire for LLM output at + all. It exists for the case that does bypass the writer: a + hand-edited ``SKILL.md`` (or any other direct + ``AgentSkillFrontmatter`` construction that skips pre-sanitization) + whose ``name`` contains a path separator, or is exactly ``".."`` — + raise loudly rather than silently relocating the skill on next + write. + + The check is deliberately narrower than "contains ``..``": a + sanitized name may legitimately contain a run of literal dots + (``sanitize_dirname`` keeps ``.`` as a safe character, so + ``"../" * 8 + "tmp/pwned"`` sanitizes to + ``"................tmppwned"``, which still contains the substring + ``".."`` many times over). With no path separator left, that string + is one opaque filename component, not a ``..`` traversal segment — + rejecting on substring containment would make this validator + reject the sanitizer's own safe output. + """ + if "/" in value or "\\" in value or value == "..": + raise ValueError( + f"skill name {value!r} must not contain path separators, " + "and must not be exactly '..'" + ) + return value description: str """One-line summary surfaced at Tier-1 prompt injection. Short — the diff --git a/src/everos/infra/persistence/markdown/readers/agent_skill_reader.py b/src/everos/infra/persistence/markdown/readers/agent_skill_reader.py index 580d54582..0dba34b87 100644 --- a/src/everos/infra/persistence/markdown/readers/agent_skill_reader.py +++ b/src/everos/infra/persistence/markdown/readers/agent_skill_reader.py @@ -5,16 +5,44 @@ - :meth:`read_main` reads ``SKILL.md`` and returns the caller's :class:`AgentSkillFrontmatter` subclass instance + the Tier-2 body, so the caller never deals with raw dicts. +- :meth:`list_by_cluster` walks every ``skill_*/SKILL.md`` under an agent + and returns ``(frontmatter, body)`` for the ones whose parsed + ``cluster_id`` matches. This is the strong-consistency source of truth + for cluster membership — LanceDB is cascade-lagged and must not be used + for that check. - :meth:`read_reference` / :meth:`read_script` are plain text reads; no frontmatter, no schema. -All three return ``None`` when the target is missing — readers do not -raise on absence, since "skill not yet created" is a normal state for -the upsert-style workflow. Callers that need to distinguish "missing" -from "empty body" check for ``None`` explicitly. +``read_main``, ``read_reference``, and ``read_script`` return ``None`` when +the target is missing — readers do not raise on absence, since "skill not +yet created" is a normal state for the upsert-style workflow. Callers that +need to distinguish "missing" from "empty body" check for ``None`` +explicitly. + +``reference_name`` / ``script_filename`` are appended after the skill +directory, so ``skill_dir_name`` does not cover them; both go through +:func:`sanitize_dirname` here exactly as :class:`AgentSkillWriter` does. +The two sides must agree on *every* segment — sanitizing one side only +would route a write and its matching read to different paths. Path resolution mirrors :class:`AgentSkillWriter` and reads the same -ClassVars off :class:`AgentSkillFrontmatter`. +ClassVars off :class:`AgentSkillFrontmatter`, including +:meth:`AgentSkillFrontmatter.skill_dir_name` for the traversal-safe +directory segment. ``read_main`` / ``read_reference`` / ``read_script`` +take a caller-supplied ``skill_name`` and re-derive the path from it, so +the reader and writer must never diverge on how a ``skill_name`` maps to +a directory. ``list_by_cluster`` never derives a path at all: it reads +each globbed ``SKILL.md`` path directly and hands back the body it +already read, rather than recovering a name from the directory and +leaving the caller to re-derive a path from that name for a second read. +A caller that discarded the body and re-read by name would recreate +exactly the re-sanitization risk this method exists to avoid — a +directory whose suffix isn't itself a sanitizer fixpoint (e.g. one +containing a raw space) would silently miss on that second, name-based +read even though the first, path-based read found it just fine. Returning +the body is what makes "the reader never derives a path" a property of +the full ``list_by_cluster`` → caller flow, not just of the enumeration +step in isolation. """ from __future__ import annotations @@ -23,13 +51,17 @@ from typing import TypeVar import anyio +from pydantic import ValidationError -from everos.core.persistence import MarkdownReader, MemoryRoot +from everos.core.observability.logging import get_logger +from everos.core.persistence import MarkdownReader, MemoryRoot, sanitize_dirname from ..mds import AgentSkillFrontmatter T = TypeVar("T", bound=AgentSkillFrontmatter) +logger = get_logger(__name__) + class AgentSkillReader: """Single-skill reader for the directory + progressive-disclosure layout.""" @@ -63,12 +95,88 @@ async def read_main( is stripped to give the *logical* body back. """ path = self._main_path(agent_id, skill_name, app_id, project_id) - if not await anyio.Path(path).is_file(): - return None - parsed = await MarkdownReader.read(path) - frontmatter = schema.model_validate(parsed.frontmatter) - body = parsed.body.rstrip("\n") - return frontmatter, body + return await self._read_path(path, schema=schema) + + async def list_by_cluster( + self, + agent_id: str, + cluster_id: str, + *, + app_id: str = "default", + project_id: str = "default", + ) -> list[tuple[AgentSkillFrontmatter, str]]: + """Enumerate this agent's ``SKILL.md`` files whose ``cluster_id`` matches. + + Walks ``skills/skill_*/SKILL.md`` under the agent's memory root and + returns ``(frontmatter, body)`` for each match. Skills whose + frontmatter has ``cluster_id is None`` (or a different cluster) are + filtered out. + + This is the strong-consistency source of truth for "which skills + belong to this cluster" — LanceDB is cascade-lagged and must not be + used for existence checks. Each glob match is read directly by its + already-resolved ``path`` (see :meth:`_read_path`), *not* by + recovering a ``skill_name`` from the directory and calling + :meth:`read_main` to re-derive the same path — the reader never + derives a path at all on this route, so this enumeration cannot drop + a skill whose on-disk directory suffix is not itself a sanitizer + fixpoint (e.g. one written with a raw, unsanitized name containing a + space). Returning the body here (rather than frontmatter alone) is + load-bearing, not a convenience: a caller that discarded it and + re-read by ``frontmatter.name`` would reintroduce the same + name-based re-derivation this method exists to avoid, one call + later. + + A ``SKILL.md`` whose frontmatter fails schema validation is logged + and skipped, not propagated. Isolating it matters because the + blast radius of propagating is the whole cluster, not the one + file: :meth:`_read_path` validates the full + :class:`AgentSkillFrontmatter` schema, so *any* constraint can + raise — a hand-edited ``name``, but equally a field that a later + schema revision made required and existing files therefore lack. + A single bad file would otherwise abort the enumeration, starve + ``extract_agent_skill`` of every existing skill in the cluster, + and dead-letter that cluster's extraction on every subsequent + run — the exact permanent-failure mode this md-first read path + exists to eliminate. + + Args: + agent_id: Owning agent. + cluster_id: Cluster to filter on. + app_id: App scope; defaults to ``"default"``. + project_id: Project scope; defaults to ``"default"``. + + Returns: + ``(frontmatter, body)`` pairs sorted by skill directory path. + Empty if the agent has no skill directory yet, none match, or + every candidate failed validation. + """ + skills_dir = self._skills_root(agent_id, app_id, project_id) + if not await anyio.Path(skills_dir).is_dir(): + return [] + pattern = ( + f"{AgentSkillFrontmatter.SKILL_DIR_PREFIX}*" + f"/{AgentSkillFrontmatter.SKILL_MAIN_FILENAME}" + ) + paths = await anyio.to_thread.run_sync(lambda: sorted(skills_dir.glob(pattern))) + matches: list[tuple[AgentSkillFrontmatter, str]] = [] + for path in paths: + try: + parsed = await self._read_path(path, schema=AgentSkillFrontmatter) + except ValidationError as exc: + logger.warning( + "agent_skill.list_by_cluster.unparseable_skill_skipped", + path=str(path), + cluster_id=cluster_id, + error=str(exc), + ) + continue + if parsed is None: + continue + frontmatter, body = parsed + if frontmatter.cluster_id == cluster_id: + matches.append((frontmatter, body)) + return matches async def read_reference( self, @@ -114,16 +222,41 @@ async def read_script( # ── Internals — same shape as AgentSkillWriter ──────────────────────────── - def _skill_dir( - self, agent_id: str, skill_name: str, app_id: str, project_id: str - ) -> Path: + async def _read_path(self, path: Path, *, schema: type[T]) -> tuple[T, str] | None: + """Read + parse an already-resolved ``SKILL.md`` path. + + Shared by :meth:`read_main` (path derived from a caller-supplied + ``skill_name``) and :meth:`list_by_cluster` (path taken directly + from a directory glob, never re-derived from a name). + + Raises: + ValidationError: the file's frontmatter violates *schema*. + Propagated to the caller — ``read_main`` asked for one + specific skill and a corrupt answer is not a substitute, + while ``list_by_cluster`` catches it per file so one bad + file cannot starve the rest of the cluster. + """ + if not await anyio.Path(path).is_file(): + return None + parsed = await MarkdownReader.read(path) + frontmatter = schema.model_validate(parsed.frontmatter) + body = parsed.body.rstrip("\n") + return frontmatter, body + + def _skills_root(self, agent_id: str, app_id: str, project_id: str) -> Path: return ( self._root.agents_dir(app_id, project_id) / agent_id / AgentSkillFrontmatter.SKILLS_CONTAINER_NAME - / f"{AgentSkillFrontmatter.SKILL_DIR_PREFIX}{skill_name}" ) + def _skill_dir( + self, agent_id: str, skill_name: str, app_id: str, project_id: str + ) -> Path: + return self._skills_root( + agent_id, app_id, project_id + ) / AgentSkillFrontmatter.skill_dir_name(skill_name) + def _main_path( self, agent_id: str, skill_name: str, app_id: str, project_id: str ) -> Path: @@ -143,7 +276,7 @@ def _reference_path( return ( self._skill_dir(agent_id, skill_name, app_id, project_id) / AgentSkillFrontmatter.SKILL_REFERENCES_DIR_NAME - / f"{reference_name}.md" + / f"{sanitize_dirname(reference_name, 'reference')}.md" ) def _script_path( @@ -157,5 +290,5 @@ def _script_path( return ( self._skill_dir(agent_id, skill_name, app_id, project_id) / AgentSkillFrontmatter.SKILL_SCRIPTS_DIR_NAME - / script_filename + / sanitize_dirname(script_filename, "script") ) diff --git a/src/everos/infra/persistence/markdown/writers/agent_skill_writer.py b/src/everos/infra/persistence/markdown/writers/agent_skill_writer.py index 074754823..9c09f8f2c 100644 --- a/src/everos/infra/persistence/markdown/writers/agent_skill_writer.py +++ b/src/everos/infra/persistence/markdown/writers/agent_skill_writer.py @@ -22,14 +22,30 @@ Path resolution comes from :class:`MemoryRoot` + the ClassVars on :class:`AgentSkillFrontmatter` (``SKILLS_CONTAINER_NAME`` / ``SKILL_DIR_PREFIX`` / etc.). The writer + reader pair is the single -addressing API for skills. +addressing API for skills. ``skill_name`` is LLM output (see +``memory.strategies.extract_agent_skill``), so the directory segment is +built via :meth:`AgentSkillFrontmatter.skill_dir_name` — the shared, +traversal-safe path-safety point both this writer and +:class:`AgentSkillReader` derive from. + +``reference_name`` and ``script_filename`` are appended *after* that +segment, so ``skill_dir_name`` does not cover them; both route through +:func:`sanitize_dirname` separately. Nothing in ``src/`` calls those two +methods yet, but they are public API and their inputs will come from the +same untrusted place the skill name does once progressive disclosure is +wired up. :class:`AgentSkillReader` sanitizes them identically — the two +sides must agree on every segment, not just the skill directory, or a +write and its matching read resolve to different paths. """ from __future__ import annotations +import shutil from pathlib import Path -from everos.core.persistence import MarkdownWriter, MemoryRoot +import anyio + +from everos.core.persistence import MarkdownWriter, MemoryRoot, sanitize_dirname from ..mds import AgentSkillFrontmatter @@ -135,6 +151,40 @@ async def write_script( ) return await self._writer.write(path, _ensure_trailing_newline(content)) + async def delete_skill( + self, + agent_id: str, + skill_name: str, + *, + app_id: str = "default", + project_id: str = "default", + ) -> bool: + """Remove ``skills/skill_/`` and everything under it. + + The one destructive operation on this writer. It exists for + reconciliation, not for expiry: when an update renames a skill, + the new name is written to a new directory and the old one has + to go, or it survives as a duplicate that + :meth:`AgentSkillReader.list_by_cluster` keeps feeding back into + the next extraction's prompt. + + Returns: + ``True`` if the directory existed and was removed, ``False`` + if it was already absent. Absence is not an error — the + caller reconciles against markdown it enumerated earlier, so + a concurrent delete is a benign race, and a directory whose + on-disk name is not a fixpoint of + :meth:`AgentSkillFrontmatter.skill_dir_name` simply is not + found. Failing closed here (leaving an orphan) is the safe + direction; the destructive alternative would be resolving the + target by anything looser than the writer's own path rule. + """ + skill_dir = self._skill_dir(agent_id, skill_name, app_id, project_id) + if not await anyio.Path(skill_dir).is_dir(): + return False + await anyio.to_thread.run_sync(lambda: shutil.rmtree(skill_dir)) + return True + # ── Path API (callers that need to echo paths in responses) ────────── def main_path( @@ -157,7 +207,7 @@ def _skill_dir( self._root.agents_dir(app_id, project_id) / agent_id / AgentSkillFrontmatter.SKILLS_CONTAINER_NAME - / f"{AgentSkillFrontmatter.SKILL_DIR_PREFIX}{skill_name}" + / AgentSkillFrontmatter.skill_dir_name(skill_name) ) def _main_path( @@ -179,7 +229,7 @@ def _reference_path( return ( self._skill_dir(agent_id, skill_name, app_id, project_id) / AgentSkillFrontmatter.SKILL_REFERENCES_DIR_NAME - / f"{reference_name}.md" + / f"{sanitize_dirname(reference_name, 'reference')}.md" ) def _script_path( @@ -193,7 +243,7 @@ def _script_path( return ( self._skill_dir(agent_id, skill_name, app_id, project_id) / AgentSkillFrontmatter.SKILL_SCRIPTS_DIR_NAME - / script_filename + / sanitize_dirname(script_filename, "script") ) diff --git a/src/everos/infra/persistence/markdown/writers/knowledge_writer.py b/src/everos/infra/persistence/markdown/writers/knowledge_writer.py index f6fd5a772..faf50f835 100644 --- a/src/everos/infra/persistence/markdown/writers/knowledge_writer.py +++ b/src/everos/infra/persistence/markdown/writers/knowledge_writer.py @@ -15,11 +15,18 @@ resolves ``knowledge_dir`` from ``MemoryRoot.knowledge_dir(app, project)`` and passes it in. This keeps the writer decoupled from the root-resolution logic and easier to test. + +``category_id`` / ``topic`` come from parsed source documents (untrusted +free text), so both are routed through +:func:`everos.core.persistence.markdown.sanitize_dirname` before becoming +a directory/file segment — the same shared helper +``AgentSkillFrontmatter.skill_dir_name`` uses for LLM-generated skill +names, so there is one CWE-22 path-traversal defense for md directory +names, not two independently maintained copies. """ from __future__ import annotations -import re import shutil from pathlib import Path @@ -28,12 +35,10 @@ from everalgo.types import KnowledgeMemory from everos.core.observability.logging import get_logger +from everos.core.persistence.markdown import sanitize_dirname logger = get_logger(__name__) -_MAX_DIRNAME_LEN = 50 -_SAFE_CHARS = re.compile(r"[^\w\-.]", re.UNICODE) - # ── Writer ──────────────────────────────────────────────────────────────── @@ -113,26 +118,12 @@ def _split_root_and_topics( return root, topics -def _sanitize_dirname(raw: str, fallback: str) -> str: - """Produce a safe directory/file name segment. - - * Replace spaces with underscores. - * Strip characters outside ``[a-zA-Z0-9_\\-.]``. - * Truncate to 50 characters. - * Fall back to *fallback* if the result is empty. - """ - slug = raw.replace(" ", "_") - slug = _SAFE_CHARS.sub("", slug) - slug = slug[:_MAX_DIRNAME_LEN] - return slug if slug else fallback - - def _resolve_doc_dir(knowledge_dir: Path, root: KnowledgeMemory) -> Path: """Build the document directory path from category, title, and doc_id.""" - category = _sanitize_dirname( + category = sanitize_dirname( root.category_id if root.category_id else "Others", "Others" ) - title_slug = _sanitize_dirname(root.topic, "doc") + title_slug = sanitize_dirname(root.topic, "doc") dir_name = f"{title_slug}_{root.doc_id}" return knowledge_dir / category / dir_name @@ -232,7 +223,7 @@ async def _write_topic( doc_id: str, ) -> None: """Write a numbered topic md file with frontmatter and content body.""" - slug = _sanitize_dirname(node.topic, f"topic_{node.topic_index}") + slug = sanitize_dirname(node.topic, f"topic_{node.topic_index}") filename = f"{node.topic_index}_{slug}.md" fm = _build_topic_frontmatter(node, doc_id) body = _ensure_trailing_newline(node.content) diff --git a/src/everos/memory/events.py b/src/everos/memory/events.py index cce4702ee..6a58972d2 100644 --- a/src/everos/memory/events.py +++ b/src/everos/memory/events.py @@ -65,18 +65,26 @@ class EpisodeExtracted(BaseEvent): class AgentCaseExtracted(BaseEvent): """Fired by ``extract_agent_case`` after the AgentCase md is written. - Carries ``task_intent`` so the skill-clustering strategy can embed it - directly, and ``quality_score`` so the strategy can short-circuit - before any embedding work when the case is below algo's quality floor - (``AgentCaseExtractor`` also short-circuits internally; this is the - upstream gate that saves an LLM call too). ``case_timestamp_ms`` - drives the algo-side ``Cluster.last_ts`` for the time-window filter - in :func:`everalgo.clustering.cluster_by_geometry`. + Carries the full case body (``task_intent`` / ``approach`` / ``key_insight``) + so downstream strategies do not need to read LanceDB — they receive strong- + consistency data on the event bus, avoiding the cascade lag that made + ``extract_agent_skill`` retry-then-dead-letter on every run. + + ``quality_score`` lets ``trigger_skill_clustering`` short-circuit before any + embedding work when the case is below algo's quality floor. + ``case_timestamp_ms`` drives the algo-side ``Cluster.last_ts`` for the + time-window filter in :func:`everalgo.clustering.cluster_by_geometry`. """ memcell_id: str case_entry_id: str task_intent: str + approach: str = "" + """Case's Approach section verbatim. Defaults empty for back-compat with + pending 1.2.2 events in the OME run_record queue.""" + key_insight: str | None = None + """Case's optional KeyInsight section. Defaults None for the same + back-compat reason as ``approach``.""" quality_score: float case_timestamp_ms: int agent_id: str @@ -103,8 +111,11 @@ class SkillClusterUpdated(BaseEvent): """Fired after the agent-case cluster strategy has merged a new case into a cluster. - Drives the agent-skill extraction strategy; ``cluster_id`` is the - new or merged cluster the source case now belongs to. + Drives the agent-skill extraction strategy. Carries a snapshot of the + triggering case body (``task_intent`` / ``approach`` / ``key_insight`` / + ``quality_score`` / ``case_timestamp_ms``) plus ``case_vector`` (already + embedded by ``trigger_skill_clustering``) so ``extract_agent_skill`` can + build its algo input without a LanceDB probe that races cascade. """ case_entry_id: str @@ -112,3 +123,14 @@ class SkillClusterUpdated(BaseEvent): agent_id: str app_id: str = "default" project_id: str = "default" + task_intent: str = "" + """Case task_intent for algo-side rendering. Default empty for back-compat + with 1.2.2 payloads in the OME run_record queue; 1.2.3+ emitters populate it.""" + approach: str = "" + key_insight: str | None = None + quality_score: float = 0.0 + case_timestamp_ms: int = 0 + case_vector: list[float] | None = None + """Case task_intent embedding, produced by trigger_skill_clustering when it + embeds for cluster matching. Passed through so extract_agent_skill does not + need a second embedding call for the > MAX_SKILLS_IN_PROMPT top-k branch.""" diff --git a/src/everos/memory/search/agentic_agent.py b/src/everos/memory/search/agentic_agent.py index 5bf713a83..584e1e9dd 100644 --- a/src/everos/memory/search/agentic_agent.py +++ b/src/everos/memory/search/agentic_agent.py @@ -22,7 +22,7 @@ import datetime as _dt from collections.abc import Awaitable, Callable -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Literal from everalgo.rank.agentic import aagentic_retrieve from everalgo.rank.hybrid import ahybrid_retrieve @@ -30,7 +30,12 @@ from everos.component.utils.datetime import from_timestamp, to_timestamp_ms from everos.core.observability.tracing import memory_span -from everos.memory.search.callbacks import build_rerank_fn +from everos.memory.search.callbacks import ( + _format_case_passage_from_metadata, + _format_skill_passage_from_metadata, + build_case_rerank_fn, + build_skill_rerank_fn, +) from everos.memory.search.shaper import ( shape_agent_case_from_candidate, shape_agent_skill_from_candidate, @@ -58,27 +63,41 @@ _REFINEMENT_STRATEGY: str = "multi_query" +_EMPTY_PASSAGE = "(empty)" +"""Stand-in body for a row whose every passage field is blank. Keeps +``everalgo.rank.agentic._format_docs`` from raising on an empty string.""" + + def _to_everalgo_doc_metadata( - metadata: dict[str, Any], *, text_field: str + metadata: dict[str, Any], *, format_passage: Callable[[dict[str, Any]], str] ) -> dict[str, Any]: """Bridge agent recall metadata to the everalgo ``_format_docs`` contract. ``aagentic_retrieve`` renders round-1 candidates into the sufficiency / multi-query LLM prompt via ``everalgo.rank.agentic._format_docs``, which reads ``metadata["episode"]`` as a dict with ``subject`` + ``content`` and - a ms-epoch ``metadata["timestamp"]``. Agent-kind rows carry their body in - ``text_field`` (``task_intent`` / ``skill``) and the time in ``timestamp`` - (datetime); without this bridge ``_format_docs`` raises ``TypeError``. + a ms-epoch ``metadata["timestamp"]``. Agent-kind rows carry their body + across several fields (``name``/``description`` for skills, + ``task_intent``/``approach`` for cases), so ``format_passage`` is the + same kind-shaped formatter the reranker uses — this keeps the passage the + LLM sufficiency check sees identical to the passage the reranker scores. + ``content`` falls back to ``_EMPTY_PASSAGE`` when the formatter yields + nothing. That happens only when *both* source fields are empty — a + degenerate row, but a reachable one on the case side, where nothing + guarantees ``task_intent`` is populated (the skill side is safe: the + sanitizer floors ``name`` at ``"unnamed"``). Without the fallback + ``_format_docs`` raises ``ValueError`` on the empty string and the whole + search request 500s, so one malformed row would take out a result set it + merely happens to appear in. A placeholder is strictly better: the LLM + sees a row it will rank last instead of the caller seeing nothing at all. Mirrors the episode path's bridge in ``agentic.py``. ``_restore_shaper_metadata`` reverts it before DTO shaping. """ bridged = dict(metadata) - content = metadata.get(text_field) - if isinstance(content, str): - bridged["episode"] = { - "subject": metadata.get("subject", ""), - "content": content, - } + bridged["episode"] = { + "subject": metadata.get("subject", ""), + "content": format_passage(metadata) or _EMPTY_PASSAGE, + } timestamp = metadata.get("timestamp") if isinstance(timestamp, _dt.datetime): bridged["timestamp"] = to_timestamp_ms(timestamp) @@ -137,6 +156,7 @@ async def search_agent_cases_agentic( reranker=reranker, llm=llm, top_k=top_k, + kind="case", ) return [ item @@ -178,6 +198,7 @@ async def search_agent_skills_agentic( reranker=reranker, llm=llm, top_k=top_k, + kind="skill", ) return [ item @@ -196,6 +217,7 @@ async def _run_agentic_retrieve( reranker: RerankProvider, llm: LLMClient, top_k: int, + kind: Literal["case", "skill"], ) -> list[Candidate]: """Shared flat agentic retrieve pipeline for agent memory kinds. @@ -203,7 +225,18 @@ async def _run_agentic_retrieve( hands it to ``aagentic_retrieve`` with hyperparameters aligned to the memsys_opensource ``AgenticConfig`` defaults. No cluster or MaxSim step: agent memory is small enough for a flat pass. + + ``kind`` selects the passage formatter and the rerank fn together — the + passage the LLM sufficiency check sees and the passage the cross-encoder + scores must be the same shape, or the two stages silently disagree on + what "relevant" means. """ + if kind == "case": + passage_formatter = _format_case_passage_from_metadata + rerank_fn = build_case_rerank_fn(reranker) + else: + passage_formatter = _format_skill_passage_from_metadata + rerank_fn = build_skill_rerank_fn(reranker) async def _dense(q: str, k: int) -> list[Candidate]: vec = await embed_query_fn(q) @@ -236,15 +269,13 @@ async def hybrid_full(q: str, k: int) -> list[Candidate]: c.model_copy( update={ "metadata": _to_everalgo_doc_metadata( - c.metadata, text_field=recaller.text_field + c.metadata, format_passage=passage_formatter ) } ) for c in hits ] - rerank_fn = build_rerank_fn(reranker, text_field=recaller.text_field) - candidates, _decision = await aagentic_retrieve( query, base_retrieve=hybrid_full, diff --git a/src/everos/memory/search/callbacks.py b/src/everos/memory/search/callbacks.py index 449069866..a480a7804 100644 --- a/src/everos/memory/search/callbacks.py +++ b/src/everos/memory/search/callbacks.py @@ -1,6 +1,6 @@ """Callback factories handed to ``everalgo.rank.arank``. -Three callbacks the rank pipeline expects: +Four callbacks the rank pipeline expects: * :func:`build_rerank_fn` — cross-encoder scorer used by ``agentic`` Round-1 + final rerank, and by ``rrf`` / ``lr`` when LLM rerank is @@ -12,10 +12,20 @@ shape doesn't fit the single-``text_field`` contract above) and uses a skill-specific instruction. Mirrors memsys_opensource ``_rerank_skill_items``. +* :func:`build_case_rerank_fn` — case-shaped variant, mirrors + :func:`build_skill_rerank_fn` for ``"Agent Case: {task_intent} - + {approach}"`` passages. * :func:`build_retrieve_fn` — Round-2 recall callback for ``agentic``. Re-runs the sparse + dense recall path for a refined query and fuses the two routes with RRF (``k=60``) before handing back to the agentic loop. + +``_format_skill_passage_from_metadata`` / ``_format_case_passage_from_metadata`` +take the raw ``Candidate.metadata`` dict rather than a ``Candidate`` so the +metadata bridge in ``agentic_agent.py`` (which formats a passage before a +``Candidate`` wrapping it exists) can reuse the exact same formatting logic +the rerank step uses — one implementation, no drift between what the LLM +sufficiency check sees and what the reranker sees. """ from __future__ import annotations @@ -90,11 +100,10 @@ async def _rerank( ) -def _format_skill_passage(candidate: Candidate) -> str: +def _format_skill_passage_from_metadata(meta: dict[str, object]) -> str: """``"Agent Skill: {name}"`` + ``" - {description}"`` when present. Mirrors opensource ``extract_text_from_hit`` for AGENT_SKILL. """ - meta = candidate.metadata name = str(meta.get("name", "") or "") description = str(meta.get("description", "") or "") if not name: @@ -104,6 +113,11 @@ def _format_skill_passage(candidate: Candidate) -> str: return f"Agent Skill: {name}" +def _format_skill_passage(candidate: Candidate) -> str: + """``Candidate``-shaped wrapper over :func:`_format_skill_passage_from_metadata`.""" + return _format_skill_passage_from_metadata(candidate.metadata) + + def build_skill_rerank_fn(provider: RerankProvider) -> RerankFn: """Skill-shaped ``RerankFn``: multi-field passage + :data:`_SKILL_RERANK_INSTRUCTION`. Output stays score-comparable @@ -136,6 +150,66 @@ async def _rerank( return _rerank +# Mirrors _SKILL_RERANK_INSTRUCTION: biases the reranker toward methodology / +# domain match for agent cases rather than generic Q-A relevance. +_CASE_RERANK_INSTRUCTION = ( + "Determine whether the case's task and approach are applicable to the " + "query, preferring same-domain cases with directly relevant methodology." +) + + +def _format_case_passage_from_metadata(meta: dict[str, object]) -> str: + """``"Agent Case: {task_intent}"`` + ``" - {approach}"`` when present. + + Mirrors ``_format_skill_passage_from_metadata``. Falls back to + ``task_intent`` alone when ``approach`` is empty (which is legal per the + cascade handler — no non-empty guard on ``approach``). + """ + task_intent = str(meta.get("task_intent", "") or "") + approach = str(meta.get("approach", "") or "") + if not task_intent: + return approach + if approach: + return f"Agent Case: {task_intent} - {approach}" + return f"Agent Case: {task_intent}" + + +def _format_case_passage(candidate: Candidate) -> str: + """``Candidate``-shaped wrapper over :func:`_format_case_passage_from_metadata`.""" + return _format_case_passage_from_metadata(candidate.metadata) + + +def build_case_rerank_fn(provider: RerankProvider) -> RerankFn: + """Case-shaped ``RerankFn``: multi-field passage + :data:`_CASE_RERANK_INSTRUCTION`. + Mirrors :func:`build_skill_rerank_fn`. + """ + + async def _rerank( + query: str, + candidates: Sequence[Candidate], + ) -> list[Candidate]: + items = list(candidates) + if not items: + return [] + passages = [_format_case_passage(c) for c in items] + with memory_span( + "everos.search.rank", + observation_type="span", + metadata={"phase": "cross_encoder_case"}, + ): + results = await provider.rerank( + query, passages, instruction=_CASE_RERANK_INSTRUCTION + ) + out: list[Candidate] = [] + for r in results: + if not 0 <= r.index < len(items): + continue + out.append(items[r.index].model_copy(update={"score": float(r.score)})) + return out + + return _rerank + + def build_retrieve_fn( recaller: KindRecaller, *, diff --git a/src/everos/memory/strategies/extract_agent_case.py b/src/everos/memory/strategies/extract_agent_case.py index 53d99c560..06577b6e7 100644 --- a/src/everos/memory/strategies/extract_agent_case.py +++ b/src/everos/memory/strategies/extract_agent_case.py @@ -100,6 +100,8 @@ async def extract_agent_case(event: AgentPipelineStarted, ctx: StrategyContext) memcell_id=event.memcell_id, case_entry_id=eid.format(), task_intent=case.task_intent, + approach=case.approach, + key_insight=case.key_insight, quality_score=case.quality_score, case_timestamp_ms=case.timestamp, agent_id=case.owner_id, diff --git a/src/everos/memory/strategies/extract_agent_skill.py b/src/everos/memory/strategies/extract_agent_skill.py index 7440e7390..7d4e6bc3b 100644 --- a/src/everos/memory/strategies/extract_agent_skill.py +++ b/src/everos/memory/strategies/extract_agent_skill.py @@ -3,28 +3,64 @@ Triggered by :class:`SkillClusterUpdated` after ``trigger_skill_clustering`` has assigned the fresh case to its cluster. The strategy: -1. Selects the ``existing_relevant_skills`` slice for this cluster: - - * cluster size ``≤ MAX_SKILLS_IN_PROMPT`` → scalar fetch (ranking - would be pointless on a fully-inclusive set); - * cluster size ``> MAX_SKILLS_IN_PROMPT`` and the target case has a - usable vector (either persisted on the row or re-embedded - on-the-fly from ``task_intent``) → cosine top-K against the - cluster; - * cluster size ``> MAX_SKILLS_IN_PROMPT`` but no vector signal is - obtainable → scalar fetch capped at K (logged warning so - truncation without ranking is observable). -2. Hydrates ``supporting_cases`` from the chosen skills' +1. Reconstructs the target case directly from the event payload + (:func:`_to_algo_case_from_event`) — no LanceDB read. This is the + cascade-lag rescue: the previous implementation probed + ``agent_case_repo.find_by_owner_entry`` for the freshly-written case + and raised a retry-class error when cascade hadn't indexed it yet, + which under sustained cascade lag meant the run died after + ``max_retries`` and OME dead-lettered it — the case was never + distilled into a skill. The case body now travels on the event bus, + so the strategy never races cascade indexing. +2. Selects the ``existing_relevant_skills`` slice for this cluster, + **md-first** (:func:`_select_existing_skills`): + + * ``AgentSkillReader.list_by_cluster`` is the source of truth for + "which skills exist in this cluster" — md is strongly consistent, + LanceDB is cascade-lagged and must not be used for existence + checks (a stale index previously made the LLM emit ``add()`` for a + skill that already existed in md, silently clobbering it on + write-back); + * cluster size ``≤ MAX_SKILLS_IN_PROMPT`` → every md skill is used + (ranking would be pointless on a fully-inclusive set); + * cluster size ``> MAX_SKILLS_IN_PROMPT`` and the event carries a + ``case_vector`` → LanceDB ranks by cosine relevance, md hydrates + the winning ids' content (LanceDB is a ranking index here, never + an existence check); + * cluster size ``> MAX_SKILLS_IN_PROMPT`` but no ``case_vector`` is + available (pre-1.2.3 event, or embedding was unavailable upstream) + → md ordering capped at K (logged warning so truncation without + ranking is observable). +3. Hydrates ``supporting_cases`` from the chosen skills' ``source_case_ids`` lineage. The algo prompt joins each existing skill to its ``source_case_ids`` via the ``supporting_cases`` map; cases that do not back any of the chosen skills would just inflate the prompt without informing the LLM. Hydrated cases are then ranked ``(quality_score desc, timestamp desc)`` and capped at ``MAX_SUPPORTING_CASES`` to keep the prompt bounded as a cluster - grows. -3. Feeds the target + existing + supporting trio to + grows. Unlike the target case and existing skills, this lineage read + stays LanceDB-backed: an un-indexed supporting case only means a + thinner prompt this run (non-corrupting; the next run catches up), + not a wrong write. +4. Feeds the target + existing + supporting trio to :class:`everalgo.agent_memory.AgentSkillExtractor`, then writes the - emitted skills back via :class:`AgentSkillWriter`. + emitted skills back via :class:`AgentSkillWriter` and reaps the + directory an update left behind when it renamed a skill (see + :func:`_reap_renamed_skills`). + +**Retire is not implemented.** ``AgentSkillExtractor.aextract`` returns a +flat ``list[AgentSkill]`` with no op discriminator; its retire branch +(``skill_ops._apply_update``, taken when ``confidence < +retire_confidence``, default ``0.1``) is an ordinary skill carrying a +lowered confidence and nothing else. This strategy writes every emitted +skill back the same way, so a retirement persists as a normal skill: it +stays in markdown, stays in the next run's prompt, and stays searchable. +Honouring it means choosing between deleting the directory — handing an +LLM-produced confidence score the authority to destroy the source of +truth — and a ``retired`` frontmatter flag, which only works if the +enumeration, cascade, and search all learn to filter on it. That is a +design decision, not an omission to patch over, so it is deferred and +stated here rather than left implied by a docstring listing three ops. Per-case granularity (one strategy run per fresh case) — algo short-circuits low-quality cases internally via its own @@ -34,16 +70,14 @@ from __future__ import annotations +from collections.abc import Mapping, Sequence + from everalgo.agent_memory import AgentSkillExtractor from everalgo.types import AgentCase as AlgoAgentCase from everalgo.types import AgentSkill as AlgoAgentSkill -from everos.component.embedding import ( - EmbeddingServiceError, - get_embedding_capability, -) +from everos.component.embedding import get_embedding_capability from everos.component.llm import get_llm_client -from everos.core.errors import ProviderNotConfiguredError from everos.core.observability.logging import get_logger from everos.core.persistence import MemoryRoot from everos.infra.ome.context import StrategyContext @@ -52,15 +86,13 @@ from everos.infra.persistence.lancedb import ( AgentCase as LanceAgentCase, ) -from everos.infra.persistence.lancedb import ( - AgentSkill as LanceAgentSkill, -) from everos.infra.persistence.lancedb import ( agent_case_repo, agent_skill_repo, ) from everos.infra.persistence.markdown import ( AgentSkillFrontmatter, + AgentSkillReader, AgentSkillWriter, ) from everos.infra.persistence.sqlite import cluster_repo @@ -74,8 +106,9 @@ The algo library expects the caller to pre-filter ``existing_relevant_skills`` to a relevant subset (cosine top-K over -the target case's ``task_intent`` embedding) so the prompt stays -bounded as a cluster grows.""" +``SkillClusterUpdated.case_vector``, embedded upstream by +``trigger_skill_clustering``) so the prompt stays bounded as a cluster +grows.""" MAX_SUPPORTING_CASES = 9 """Upper bound on ``supporting_cases`` after lineage hydration. @@ -92,11 +125,8 @@ class _ClusterMissingError(RuntimeError): """Race with the cluster strategy; OME retry will catch up.""" -class _CaseNotYetIndexedError(RuntimeError): - """The target case is in md but not yet in LanceDB; OME retry will catch up.""" - - _writer: AgentSkillWriter | None = None +_reader: AgentSkillReader | None = None def _get_writer() -> AgentSkillWriter: @@ -106,6 +136,13 @@ def _get_writer() -> AgentSkillWriter: return _writer +def _get_reader() -> AgentSkillReader: + global _reader + if _reader is None: + _reader = AgentSkillReader(root=MemoryRoot.resolve()) + return _reader + + @offline_strategy( name="extract_agent_skill", trigger=Immediate(on=[SkillClusterUpdated]), @@ -113,13 +150,16 @@ def _get_writer() -> AgentSkillWriter: max_retries=3, ) async def extract_agent_skill(event: SkillClusterUpdated, ctx: StrategyContext) -> None: - # Body-guard: capability is checked here for defensive degradation. - # Belt-and-suspenders even though the upstream - # trigger_skill_clustering already gates on the same capability — a - # direct emit of SkillClusterUpdated (tests, future features) should - # still degrade cleanly without an owner lock or OME retry pressure. - # Tier upgrades require a server restart; this guard is not a - # hot-reload mechanism. + # Body-guard: this strategy no longer embeds anything (the query + # vector rides the event), so the guard is not protecting a local + # call — it keeps the whole agent-skill track consistent with the + # tier the deployment is actually running. Upstream + # trigger_skill_clustering gates on the same capability, so no + # SkillClusterUpdated normally exists without an embedder; the guard + # covers a direct emit (tests, future features), which should degrade + # cleanly rather than take an owner lock and produce skills the + # ranking half of the pipeline cannot serve. Tier upgrades require a + # server restart; this guard is not a hot-reload mechanism. # # ``debug`` level (not ``info``) is intentional; see the body-guard # in :func:`everos.memory.strategies.trigger_profile_clustering` for @@ -143,44 +183,46 @@ async def extract_agent_skill(event: SkillClusterUpdated, ctx: StrategyContext) # 1. Check the cluster row exists. await _ensure_cluster_exists(event.cluster_id, event.case_entry_id) - # 2. Load the target AgentCase from LanceDB (scoped to space). - target_lance = await _load_target_case( - event.agent_id, - event.case_entry_id, - app_id=event.app_id, - project_id=event.project_id, - ) + # 2. Reconstruct the target AgentCase from the event payload — no + # LanceDB probe, so the strategy never races cascade indexing. + target = _to_algo_case_from_event(event) - # 3. Pick the top-K relevant existing skills in this cluster. + # 3. Pick the top-K relevant existing skills in this cluster, md-first. # (Cluster-scoped queries are implicitly space-scoped: cluster_id # is globally unique to one (app, project, owner) cluster set.) - existing_lance = await _select_existing_skills( + existing_skills = await _select_existing_skills( agent_id=event.agent_id, cluster_id=event.cluster_id, - target=target_lance, + app_id=event.app_id, + project_id=event.project_id, + case_vector=event.case_vector, ) # 4. Pull the supporting cases referenced by those skills. supporting_lance = await _select_supporting_cases( - existing_lance, + existing_skills, agent_id=event.agent_id, exclude_entry_id=event.case_entry_id, app_id=event.app_id, project_id=event.project_id, ) - # 5. Run the LLM extractor → add / update / retire skill operations. + # 5. Run the LLM extractor. Emits add / update ops; a retire op comes + # back as an ordinary skill with confidence < retire_confidence and + # is NOT honoured here — see the module docstring. extractor = AgentSkillExtractor(llm=get_llm_client()) emitted_skills = await extractor.aextract( - _to_algo_case(target_lance), - existing_relevant_skills=[_to_algo_skill(s) for s in existing_lance], + target, + existing_relevant_skills=existing_skills, supporting_cases=[_to_algo_case(c) for c in supporting_lance], ) - # 6. Write each emitted skill back to its SKILL.md. + # 6. Write each emitted skill back to its SKILL.md, then reap the + # directories that a rename left behind. writer = _get_writer() + written_names: dict[str, str] = {} for skill in emitted_skills: - await _persist_skill( + written_names[skill.id] = await _persist_skill( writer, skill, agent_id=event.agent_id, @@ -188,6 +230,14 @@ async def extract_agent_skill(event: SkillClusterUpdated, ctx: StrategyContext) app_id=event.app_id, project_id=event.project_id, ) + await _reap_renamed_skills( + writer, + written_names, + existing_skills=existing_skills, + agent_id=event.agent_id, + app_id=event.app_id, + project_id=event.project_id, + ) logger.info( "agent_skills_extracted", case_entry_id=event.case_entry_id, @@ -210,104 +260,104 @@ async def _ensure_cluster_exists(cluster_id: str, case_entry_id: str) -> None: ) -async def _load_target_case( - agent_id: str, - case_entry_id: str, - *, - app_id: str, - project_id: str, -) -> LanceAgentCase: - """Pull the target case row, raising a retry-class error on cascade lag.""" - target = await agent_case_repo.find_by_owner_entry( - agent_id, case_entry_id, app_id=app_id, project_id=project_id - ) - if target is None: - # Cascade hasn't indexed the freshly-written md yet. - raise _CaseNotYetIndexedError( - f"AgentCase entry_id={case_entry_id} not in LanceDB yet; retrying" - ) - return target - - async def _select_existing_skills( *, agent_id: str, cluster_id: str, - target: LanceAgentCase, -) -> list[LanceAgentSkill]: + app_id: str, + project_id: str, + case_vector: list[float] | None, +) -> list[AlgoAgentSkill]: """Pick at most ``MAX_SKILLS_IN_PROMPT`` existing skills for the prompt. - See module docstring for the three-branch routing rationale. + md is the source of truth for existence — this avoids the + stale-index clobber where a skill was written last run but hadn't + been indexed into LanceDB yet, causing the LLM to see no existing + skill and emit ``add()`` for one that already exists. LanceDB is + only consulted for relevance ordering when the cluster's md skill + count exceeds ``MAX_SKILLS_IN_PROMPT`` and ``case_vector`` is + available; when it isn't (pre-1.2.3 event, or embedding was + unavailable upstream), fall back to md ordering. + + ``list_by_cluster`` returns each skill's frontmatter *and* body + together, so there is no second, name-based read to hydrate + ``content`` — a prior version re-read each selected skill by + ``fm.name`` via ``read_main``, which re-derives (and re-sanitizes) a + path from that name and would silently miss a skill whose on-disk + directory suffix isn't itself a sanitizer fixpoint, even though the + first, path-based enumeration found it. See + ``AgentSkillReader.list_by_cluster``'s docstring. """ - total = await agent_skill_repo.count_in_cluster( - owner_id=agent_id, cluster_id=cluster_id + reader = _get_reader() + md_skills = await reader.list_by_cluster( + agent_id, cluster_id, app_id=app_id, project_id=project_id ) - if total <= MAX_SKILLS_IN_PROMPT: - return await agent_skill_repo.find_in_cluster( - owner_id=agent_id, cluster_id=cluster_id, limit=MAX_SKILLS_IN_PROMPT - ) + if not md_skills: + return [] - query_vector = await _resolve_query_vector(target) - if query_vector: - return await agent_skill_repo.find_topk_relevant_in_cluster( - owner_id=agent_id, + if len(md_skills) <= MAX_SKILLS_IN_PROMPT: + selected = md_skills + elif case_vector is not None: + selected = await _rank_skills_by_relevance( + md_skills, + agent_id=agent_id, cluster_id=cluster_id, - query_vector=query_vector, - top_k=MAX_SKILLS_IN_PROMPT, + case_vector=case_vector, ) + else: + logger.warning( + "agent_skill_topk_no_query_vector_md_fallback", + agent_id=agent_id, + cluster_id=cluster_id, + md_count=len(md_skills), + ) + selected = md_skills[:MAX_SKILLS_IN_PROMPT] - logger.warning( - "agent_skill_topk_no_query_vector_scalar_fallback", - agent_id=agent_id, - cluster_id=cluster_id, - cluster_size=total, - ) - return await agent_skill_repo.find_in_cluster( - owner_id=agent_id, cluster_id=cluster_id, limit=MAX_SKILLS_IN_PROMPT - ) - - -async def _resolve_query_vector(target: LanceAgentCase) -> list[float]: - """Return a usable query vector for cosine top-K, ``[]`` if unobtainable. - - Order of preference: + return [_md_to_algo_skill(fm, body) for fm, body in selected] - 1. ``target.vector`` if cascade has already populated the column — - this is the exact vector the recall path uses, so reusing it - keeps ranking semantics identical across reads. - 2. Compute on the fly from ``target.task_intent`` via the configured - embedder — matches the cascade handler's own vectorisation - contract (``cascade/handlers/agent_case.py``), so the two paths - agree on what "the case embedding" means. - Returns ``[]`` only when both options are unavailable (no persisted - vector, no ``task_intent`` text, or the embedder is not configured / - fails). The caller decides the policy for that case. +async def _rank_skills_by_relevance( + md_skills: list[tuple[AgentSkillFrontmatter, str]], + *, + agent_id: str, + cluster_id: str, + case_vector: list[float], +) -> list[tuple[AgentSkillFrontmatter, str]]: + """Ask LanceDB to rank the md skills by cosine relevance, capped at K. + + LanceDB is used purely as a ranking index here, never as the + existence check — the candidate set is always the md list. A LanceDB + row with no matching md name is stale and skipped; md skills LanceDB + didn't return (also stale index) then backfill in md order until the + ``MAX_SKILLS_IN_PROMPT`` budget is full. That backfill keeps a lagging + index from *under*-filling the prompt; it does not make the selection + lossless — this function only runs when the cluster already holds more + skills than the budget admits, so skills beyond K are dropped by + design either way. """ - if target.vector: - return list(target.vector) - if not target.task_intent: - return [] - # ``.require()`` is defensive: the strategy body-guard checks - # ``.available`` before we reach this helper, so this branch will - # not raise ``ProviderNotConfiguredError`` in normal operation. The - # catch stays so a misconfiguration surfacing later (or a direct - # unit-test call to this helper without the guard) still degrades - # to ``[]`` instead of blowing up mid-strategy. - try: - embedder = get_embedding_capability().require() - return list(await embedder.embed(target.task_intent)) - except (ProviderNotConfiguredError, EmbeddingServiceError) as exc: - logger.warning( - "agent_skill_query_embed_failed", - case_entry_id=target.entry_id, - error=str(exc), - ) - return [] + md_by_name = {fm.name: (fm, body) for fm, body in md_skills} + ranked_lance = await agent_skill_repo.find_topk_relevant_in_cluster( + owner_id=agent_id, + cluster_id=cluster_id, + query_vector=case_vector, + top_k=MAX_SKILLS_IN_PROMPT, + ) + selected: list[tuple[AgentSkillFrontmatter, str]] = [] + seen_names: set[str] = set() + for lance_row in ranked_lance: + pair = md_by_name.get(lance_row.name) + if pair is not None and pair[0].name not in seen_names: + selected.append(pair) + seen_names.add(pair[0].name) + for fm, body in md_skills: + if fm.name not in seen_names and len(selected) < MAX_SKILLS_IN_PROMPT: + selected.append((fm, body)) + seen_names.add(fm.name) + return selected async def _select_supporting_cases( - skills: list[LanceAgentSkill], + skills: list[AlgoAgentSkill], *, agent_id: str, exclude_entry_id: str, @@ -341,7 +391,7 @@ async def _select_supporting_cases( def _collect_supporting_entry_ids( - skills: list[LanceAgentSkill], *, exclude: str + skills: list[AlgoAgentSkill], *, exclude: str ) -> list[str]: """Dedup ``source_case_ids`` across ``skills``, preserving first-seen order.""" seen: list[str] = [] @@ -359,7 +409,11 @@ def _collect_supporting_entry_ids( def _to_algo_case(lance: LanceAgentCase) -> AlgoAgentCase: - """Project the LanceDB row onto the algo-side AgentCase type.""" + """Project the LanceDB row onto the algo-side AgentCase type. + + Used only for ``supporting_cases`` — that lineage read stays + LanceDB-backed (see module docstring, point 3). + """ return AlgoAgentCase( id=lance.entry_id, timestamp=int(lance.timestamp.timestamp() * 1000), @@ -370,24 +424,100 @@ def _to_algo_case(lance: LanceAgentCase) -> AlgoAgentCase: ) -def _to_algo_skill(lance: LanceAgentSkill) -> AlgoAgentSkill: - """Project the LanceDB row onto the algo-side AgentSkill type. +def _to_algo_case_from_event(event: SkillClusterUpdated) -> AlgoAgentCase: + """Reconstruct the target case from event fields. + + Strong-consistency: the case body travels on the event bus, so the + strategy never races cascade indexing. Pre-1.2.3 events default + missing fields to empty — those runs won't have useful data but also + won't crash. + """ + return AlgoAgentCase( + id=event.case_entry_id, + timestamp=event.case_timestamp_ms, + task_intent=event.task_intent, + approach=event.approach, + quality_score=event.quality_score, + key_insight=event.key_insight or "", + ) + + +def _md_to_algo_skill(fm: AgentSkillFrontmatter, body: str) -> AlgoAgentSkill: + """Project a SKILL.md frontmatter + body onto everalgo's AgentSkill type. - ``cluster_id`` rides along even though algo doesn't read it on input — - keeps the model fully populated for any consumer that introspects. + ``body`` populates ``AlgoAgentSkill.content`` so the extractor prompt's + existing-skills block (``everalgo.agent_memory.skill_ops._format_existing_skills``) + carries the real skill definition, not an empty placeholder — without it + the LLM cannot distinguish "add new" from "update existing". """ return AlgoAgentSkill( - id=lance.id, - cluster_id=lance.cluster_id or "", - name=lance.name, - description=lance.description, - content=lance.content, - confidence=lance.confidence, - maturity_score=lance.maturity_score, - source_case_ids=list(lance.source_case_ids), + id=fm.id, + cluster_id=fm.cluster_id or "", + name=fm.name, + description=fm.description, + content=body, + confidence=fm.confidence, + maturity_score=fm.maturity_score, + source_case_ids=list(fm.source_case_ids), ) +async def _reap_renamed_skills( + writer: AgentSkillWriter, + written_names: Mapping[str, str], + *, + existing_skills: Sequence[AlgoAgentSkill], + agent_id: str, + app_id: str, + project_id: str, +) -> None: + """Delete the directory an update left behind when it renamed a skill. + + everalgo treats a name change as a first-class update + (``skill_ops._apply_update`` computes ``name_changed`` and returns + ``prior.model_copy(update={"name": eff_name, ...})``), so the emitted + skill keeps ``prior.id`` while carrying the new name. ``_persist_skill`` + writes it to ``skill_/`` and the old directory would survive + with the same ``cluster_id``. + + That matters more here than it looks. Since this release the input to + the next extraction is the markdown enumeration, not LanceDB — so a + surviving pre-rename directory does not merely sit on disk, it comes + back in the next run's ``existing_relevant_skills`` as a duplicate of a + skill the LLM already renamed. Shown its own stale copy, the LLM emits + ``add`` for something that exists, and ``write_main`` full-replaces — + which is precisely the clobber this release set out to close. Left + unreaped, every rename adds another one. + + Identity comes from ``skill.id``, which is the only thing that survives + a rename: ``_apply_update`` preserves ``prior.id`` while ``_apply_add`` + mints a fresh ``uuid4().hex``, so an id present in the enumerated set is + an update by construction and a new skill can never match. + + A prior name that some *other* emitted skill just claimed is never + deleted — with two ops in one batch (rename ``a``→``b`` while another + op writes ``a``) the reap would otherwise remove a file written + moments earlier in the same loop. + """ + claimed = set(written_names.values()) + for prior in existing_skills: + new_name = written_names.get(prior.id) + prior_name = AgentSkillFrontmatter.sanitize_skill_name(prior.name) + if new_name is None or new_name == prior_name or prior_name in claimed: + continue + removed = await writer.delete_skill( + agent_id, prior_name, app_id=app_id, project_id=project_id + ) + logger.info( + "agent_skill_renamed_directory_reaped", + agent_id=agent_id, + skill_id=prior.id, + old_name=prior_name, + new_name=new_name, + removed=removed, + ) + + async def _persist_skill( writer: AgentSkillWriter, skill: AlgoAgentSkill, @@ -396,12 +526,29 @@ async def _persist_skill( cluster_id: str, app_id: str, project_id: str, -) -> None: - """Write one ``SKILL.md`` with the post-stamped ``cluster_id``.""" +) -> str: + """Write one ``SKILL.md`` with the post-stamped ``cluster_id``. + + Returns the sanitized name it wrote under, so the caller can + reconcile renames without re-deriving it. + + ``skill.name`` is LLM output and is sanitized once, up front, via + :meth:`AgentSkillFrontmatter.sanitize_skill_name` — the same helper + :meth:`AgentSkillFrontmatter.skill_dir_name` uses for the directory + segment. Sanitizing here (rather than handing the raw name to the + frontmatter constructor) keeps ``frontmatter.name`` byte-identical to + the on-disk directory name, and means a traversal-shaped LLM name + (reachable via prompt injection, since the LLM's input is user + conversation content) is made filesystem-safe *before* it reaches + ``AgentSkillFrontmatter``, instead of tripping the read-side traversal + validator and dead-lettering the whole extraction run for a name the + writer would have sanitized safely anyway. + """ + sanitized_name = AgentSkillFrontmatter.sanitize_skill_name(skill.name) frontmatter = AgentSkillFrontmatter( - id=f"{agent_id}_{skill.name}", + id=f"{agent_id}_{sanitized_name}", agent_id=agent_id, - name=skill.name, + name=sanitized_name, description=skill.description, confidence=skill.confidence, maturity_score=skill.maturity_score, @@ -410,9 +557,10 @@ async def _persist_skill( ) await writer.write_main( agent_id, - skill.name, + sanitized_name, frontmatter=frontmatter, body=skill.content, app_id=app_id, project_id=project_id, ) + return sanitized_name diff --git a/src/everos/memory/strategies/extract_foresight.py b/src/everos/memory/strategies/extract_foresight.py index b00df9e36..4583d6ddc 100644 --- a/src/everos/memory/strategies/extract_foresight.py +++ b/src/everos/memory/strategies/extract_foresight.py @@ -10,6 +10,36 @@ one batched ``append_entries`` call rather than ``N`` single appends, dropping IO complexity to ``O(N)`` per owner and narrowing the per-path lock window. + +**Disabled by default** (``enabled=False``). Not because it is broken — +the crash it used to have is fixed below — but because it is one LLM +call per sender per memcell whose output nothing in EverOS reads today: +no search route surfaces foresights, no prompt slot consumes them. Until +something does, running it by default spends tokens on write-only data. +Re-enable per install in ``ome.toml`` (hot-reloaded, ~2s): + +.. code-block:: toml + + [strategies.extract_foresight] + enabled = true + +The opt-in has to actually work, so the sender scan below filters on +``isinstance(m, ChatMessage)`` rather than reaching for ``m.role``. +Only ``ChatMessage`` carries ``role``: ``ToolCallRequest`` has +``sender_id`` without it, ``ToolCallResult`` has neither — so the old +attribute test raised ``AttributeError`` on the first tool call, making +the strategy correct on plain user chat and guaranteed to dead-letter on +agent trajectories. everalgo contracts for exactly this mixed input +(``user_memory/_render.chat_messages``: "the caller need not pre-filter; +an AgentMemCell-shaped MemCell is acceptable input"), and every other +user-memory extractor gets it for free by delegating; this strategy was +the one place that hand-rolled the filter. A pure agent trajectory now +yields no senders and returns without an LLM call. + +Extraction granularity is a separate, still-open question: this runs per +memcell, while ``atomic_fact`` runs per episode. Moving it needs an +everalgo entry point that does not exist yet — unrelated to the crash, +and not what the default-off above is about. """ from __future__ import annotations @@ -17,6 +47,7 @@ from collections import defaultdict from collections.abc import Mapping +from everalgo.types import ChatMessage from everalgo.user_memory import ForesightExtractor from everos.component.llm import get_llm_client @@ -47,11 +78,18 @@ def _get_writer() -> ForesightWriter: trigger=Immediate(on=[UserPipelineStarted]), emits=[], max_retries=2, + enabled=False, ) async def extract_foresight(event: UserPipelineStarted, ctx: StrategyContext) -> None: # 1. List the user senders in this memcell. memcell = event.memcell - sender_ids = sorted({m.sender_id for m in memcell.items if m.role == "user"}) + sender_ids = sorted( + { + m.sender_id + for m in memcell.items + if isinstance(m, ChatMessage) and m.role == "user" + } + ) extractor = ForesightExtractor(llm=get_llm_client()) if sender_ids else None # 2. Run the LLM extractor once per sender (prompt is per-sender). diff --git a/src/everos/memory/strategies/trigger_skill_clustering.py b/src/everos/memory/strategies/trigger_skill_clustering.py index 88213827c..3a3d34501 100644 --- a/src/everos/memory/strategies/trigger_skill_clustering.py +++ b/src/everos/memory/strategies/trigger_skill_clustering.py @@ -134,6 +134,12 @@ async def trigger_skill_clustering( agent_id=event.agent_id, app_id=event.app_id, project_id=event.project_id, + task_intent=event.task_intent, + approach=event.approach, + key_insight=event.key_insight, + quality_score=event.quality_score, + case_timestamp_ms=event.case_timestamp_ms, + case_vector=vector_list, ) ) logger.info( diff --git a/tests/e2e/test_add_flush_agent_pipeline_e2e.py b/tests/e2e/test_add_flush_agent_pipeline_e2e.py index 404b6f331..43d00212a 100644 --- a/tests/e2e/test_add_flush_agent_pipeline_e2e.py +++ b/tests/e2e/test_add_flush_agent_pipeline_e2e.py @@ -2,7 +2,13 @@ Drives the full HTTP route through to storage, exercising the agent-track pipeline (boundary → memcell → extract_agent_case → trigger_skill_clustering -→ extract_agent_skill) with real LLM and real embedder credentials. +→ extract_agent_skill) with real LLM and real embedder credentials — this +module's own ``_opt_in_real_embedding`` fixture opts the embedding +capability back in (see its docstring for why that is necessary and why +it does not weaken the global hermeticity fixture). Rerank is +deliberately left at its hermetic default: nothing on this write path +touches rerank, so opting it in would only widen the credential surface +with no coverage benefit. Mixed tenancy by design (sender_id alignment from fixture): @@ -24,21 +30,26 @@ - sqlite ``memcell`` rows per session_id - filesystem ``/agents//.cases/*.md`` presence - LanceDB ``agent_case`` rows by ``owner_id`` (count + session_id set) - - LanceDB ``agent_skill`` rows by ``owner_id`` (soft — LLM-dependent) + - LanceDB ``agent_skill`` rows by ``owner_id`` (aggregate floor — see + ``test_agent_pipeline_e2e_mixed_tenancy``'s section 4.5) + - OME ``run_record``: no dead-lettered ``extract_agent_skill`` run """ from __future__ import annotations import asyncio import json -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Iterator from pathlib import Path import httpx import pytest +import everos.component.embedding.accessor as _embedding_accessor +from everos.infra.ome.records import RunStatus from everos.infra.persistence.lancedb import agent_case_repo, agent_skill_repo from everos.infra.persistence.markdown import AgentCaseDailyFrontmatter +from everos.service.memorize import _get_engine _FIXTURE_DIR = Path(__file__).resolve().parents[1] / "fixtures" / "agent_trajectories" @@ -64,6 +75,41 @@ _DRAIN_INTER_ROUND_SLEEP_SECONDS = 5.0 +@pytest.fixture(autouse=True) +def _opt_in_real_embedding( + _reset_embedding_capability_singleton: None, +) -> Iterator[None]: + """Opt this module's test into a real embedding capability. + + ``tests/conftest.py``'s ``_reset_embedding_capability_singleton`` + autouse fixture pins the capability to unavailable for every test + (hermeticity); its docstring says a test may "explicitly opt in by + re-assigning ``acc._capability``". This test needs it: + ``trigger_skill_clustering`` and ``extract_agent_skill`` both + body-guard on ``get_embedding_capability().available`` and return + early when it is false, so without opting in here the skill chain + would never run — exactly the coverage gap this fixture closes. + Scoped to this file only (not the global fixture) so every other + test keeps its hermetic default. + + Requesting ``_reset_embedding_capability_singleton`` as a parameter + — rather than relying on collection/declaration order between this + file's conftest chain and the root conftest — makes pytest's + dependency graph guarantee this fixture's setup runs after it and its + teardown before it. Rerank is left untouched (see the module + docstring): this fixture reads and writes only the embedding + capability, so there is nothing to order it against. + + Setting ``_capability = None`` (rather than constructing a capability + object directly) makes the accessor rebuild lazily from + ``load_settings()`` on next call, picking up the real ``.env`` + credentials ``tests/e2e/conftest.py`` loads at import time. + """ + _embedding_accessor._capability = None + yield + _embedding_accessor._capability = None + + def _load_fixture(session_id: str) -> dict: return json.loads((_FIXTURE_DIR / f"{session_id}.json").read_text()) @@ -174,21 +220,56 @@ async def test_agent_pipeline_e2e_mixed_tenancy( f"want {set(_DJANGO_SESSIONS)}" ) - # 4.5 agent_skill — soft: emission depends on LLM clustering quality - # gate (skip_quality_threshold + cluster size). pytest/sympy are - # single-case clusters and may legitimately yield 0 skills. django - # has 3 cases and should aggregate into ≥1 cluster of size ≥2, - # producing ≥1 skill — but we keep this informational (LLM-dependent) - # rather than a hard floor to avoid flaky CI signal. + # 4.5 agent_skill — aggregate floor across all three agents. Per-agent + # emission depends on everalgo's per-case quality gate + # (skip_quality_threshold, see everalgo/agent_memory/skill_ops.py) — + # extract_agent_skill itself has no cluster-size gate, so a per-agent + # floor would be genuinely flaky (a single low-quality trajectory can + # legitimately yield 0 skills for that agent). Empirically, on this + # branch with real credentials, 1 django trajectory alone produced 1 + # SKILL.md (extract_agent_skill status "success", no retries); driving + # 5 trajectories across 3 agents should clear an aggregate floor of 1 + # even if any single agent's cluster is quality-gated to 0. pytest_skills = await agent_skill_repo.find_where(f"owner_id = '{_AGENT_PYTEST}'") sympy_skills = await agent_skill_repo.find_where(f"owner_id = '{_AGENT_SYMPY}'") django_skills = await agent_skill_repo.find_where(f"owner_id = '{_AGENT_DJANGO}'") - # Hard sanity: counts non-negative (the repo isn't broken). - assert len(pytest_skills) >= 0 - assert len(sympy_skills) >= 0 - assert len(django_skills) >= 0 + total_skills = len(pytest_skills) + len(sympy_skills) + len(django_skills) + assert total_skills >= 1, ( + "agent-skill chain produced nothing — the strategy chain " + "(extract_agent_case → trigger_skill_clustering → extract_agent_skill) " + "is broken or gated off " + f"(pytest={len(pytest_skills)}, sympy={len(sympy_skills)}, " + f"django={len(django_skills)})" + ) + + # 4.6 no dead-lettered extract_agent_skill run. Sharper signal than + # the skill-count floor above and targets this branch's actual defect + # directly: a dead-letter means the chain attempted and failed + # (exhausted retries), as opposed to a quality-gated 0-skill outcome, + # which is not a failure. + # + # The dead-letter check alone is vacuous if the strategy never ran at + # all — zero dead-letters is also what a never-executed strategy + # looks like. Assert (any status) runs exist first, so the dead-letter + # assertion below is only non-vacuous because of this check, not + # because the skill-count floor in 4.5 happened to run first. + engine = _get_engine() + all_skill_runs = await engine.list_runs("extract_agent_skill") + assert all_skill_runs, ( + "extract_agent_skill never ran at all — the dead-letter check " + "below would be vacuously satisfied by a strategy that never " + "executed" + ) + + dead_letters = await engine.list_runs( + "extract_agent_skill", status=RunStatus.DEAD_LETTER + ) + assert not dead_letters, ( + "extract_agent_skill dead-lettered " + f"{len(dead_letters)} run(s): {[r.error for r in dead_letters]}" + ) - # 4.6 strict md ↔ LanceDB parity across every cascade kind + # 4.7 strict md ↔ LanceDB parity across every cascade kind # # The per-owner counts above are loose (LLM-emission-dependent); this # check enforces byte-exact id-set + content_sha256 parity across diff --git a/tests/integration/test_ome_strategies_integration.py b/tests/integration/test_ome_strategies_integration.py index 69a5345ed..88d9c8811 100644 --- a/tests/integration/test_ome_strategies_integration.py +++ b/tests/integration/test_ome_strategies_integration.py @@ -141,12 +141,35 @@ async def test_emit_dispatches_both_strategies_to_success( # Ensure the sqlite dir exists before the engine creates ome.db. (tmp_path / ".index" / "sqlite").mkdir(parents=True, exist_ok=True) - (tmp_path / "ome.toml").write_text("# test\n") + # `extract_foresight` now ships disabled (see its module docstring). + # This test needs a `UserPipelineStarted` subscriber to cover the + # second trigger route, so it opts back in through the very `ome.toml` + # key that docstring points users at — which makes the opt-in path + # itself covered, rather than working around the new default. + (tmp_path / "ome.toml").write_text( + "[strategies.extract_foresight]\nenabled = true\n" + ) await _setup_system_db_schema(monkeypatch) engine = svc._get_engine() await engine.start() try: + # `ConfigReloader.start()` fires its initial load as a task, so + # `engine.start()` returns before `ome.toml` has been applied. + # An emit inside that window is judged against the coded defaults + # and silently dropped by the enabled gate — the event is not + # redelivered once the override lands. Wait for the override to + # be visible in the registry before emitting. + for _ in range(50): + if any( + m.name == "extract_foresight" and m.enabled + for m in engine._registry.all() + ): + break + await asyncio.sleep(0.1) + else: + pytest.fail("ome.toml override never reached the registry") + # Foresight still subscribes to UserPipelineStarted. await engine.emit( UserPipelineStarted( diff --git a/tests/unit/test_core/test_persistence/test_markdown/test_frontmatter.py b/tests/unit/test_core/test_persistence/test_markdown/test_frontmatter.py index 117d3e644..c63c508d4 100644 --- a/tests/unit/test_core/test_persistence/test_markdown/test_frontmatter.py +++ b/tests/unit/test_core/test_persistence/test_markdown/test_frontmatter.py @@ -155,6 +155,39 @@ class _AgentSkill(SkillPathMixin, AgentScopedFrontmatter): assert _AgentSkill.path_glob() == "*/*/agents/*/skills/skill_*/SKILL.md" +def test_skill_dir_name_sanitizes_traversal_payload() -> None: + """``skill_dir_name`` is the sanitization point ``AgentSkillWriter`` / + ``AgentSkillReader`` both derive from — a traversal payload must not + survive into the directory segment. + """ + + class _AgentSkill(SkillPathMixin, AgentScopedFrontmatter): + SKILLS_CONTAINER_NAME: ClassVar[str] = "skills" + SKILL_DIR_PREFIX: ClassVar[str] = "skill_" + SKILL_MAIN_FILENAME: ClassVar[str] = "SKILL.md" + type: Literal["_agent_skill_dirname"] = "_agent_skill_dirname" + + segment = _AgentSkill.skill_dir_name("../" * 8 + "tmp/pwned") + assert segment.startswith("skill_") + assert "/" not in segment + assert "\\" not in segment + # No separator survives, so the dots left behind form one opaque + # component — not a ``..`` path-traversal segment. + assert segment.split("/") == [segment] + + +def test_skill_dir_name_preserves_cjk_and_spaces() -> None: + class _AgentSkill(SkillPathMixin, AgentScopedFrontmatter): + SKILLS_CONTAINER_NAME: ClassVar[str] = "skills" + SKILL_DIR_PREFIX: ClassVar[str] = "skill_" + SKILL_MAIN_FILENAME: ClassVar[str] = "SKILL.md" + type: Literal["_agent_skill_dirname_cjk"] = "_agent_skill_dirname_cjk" + + segment = _AgentSkill.skill_dir_name("修复 Django 自动重载问题") + assert "修复" in segment + assert "Django" in segment + + def test_strategy_mixin_overrides_base_via_mro() -> None: """Strategy mixin placed first in the parent list wins over abstract base.""" diff --git a/tests/unit/test_core/test_persistence/test_markdown/test_path_safety.py b/tests/unit/test_core/test_persistence/test_markdown/test_path_safety.py new file mode 100644 index 000000000..d2948b642 --- /dev/null +++ b/tests/unit/test_core/test_persistence/test_markdown/test_path_safety.py @@ -0,0 +1,156 @@ +"""Unit tests for :func:`sanitize_dirname` — the shared CWE-22 path-safety helper. + +Pins the properties the callers rely on: + +- traversal payloads collapse to a single opaque path component with no + separator (the "no separator survives" half of the safety property); +- short inputs that are themselves sanitizer fixpoints — bare ``".."``, or + anything that strips down to ``".."`` / ``"."`` once a separator is + removed — fall back rather than being returned as-is (the other half: + without this, ``sanitize_dirname("../", fb)`` returns ``".."`` verbatim, + which is a real one-level escape for a caller with no additional prefix + protecting the segment, like ``KnowledgeWriter``); +- non-ASCII input (CJK, spaces, NFD-decomposed accents) survives readably + rather than being sanitized down to the empty-string fallback; +- the function is idempotent, which is what lets a reader (deriving a name + from an on-disk directory) and a writer (deriving it from raw input) + agree on the same path — see :mod:`.test_frontmatter`'s + ``SkillPathMixin.skill_dir_name`` coverage for the consumer side. +""" + +from __future__ import annotations + +import unicodedata +from pathlib import Path + +import pytest + +from everos.core.persistence.markdown import sanitize_dirname + + +def test_traversal_payload_has_no_separator() -> None: + """No path separator survives — a run of literal dots (``....``) is a + single opaque filename component, not a ``..`` path-traversal segment, + since there is no ``/`` left to divide it into components. + """ + payload = "../" * 8 + "tmp/pwned" + sanitized = sanitize_dirname(payload, fallback="unnamed") + + assert "/" not in sanitized + assert "\\" not in sanitized + assert sanitized != ".." + + +def test_traversal_payload_resolved_path_stays_under_root(tmp_path: Path) -> None: + payload = "../" * 8 + "tmp/pwned" + sanitized = sanitize_dirname(payload, fallback="unnamed") + + resolved = (tmp_path / sanitized).resolve() + assert resolved.is_relative_to(tmp_path.resolve()) + + +@pytest.mark.parametrize("raw", ["..", "../", "/../", ".", "./"]) +def test_degenerate_fixpoints_fall_back_instead_of_escaping(raw: str) -> None: + """A short input that strips down to exactly ``".."`` or ``"."`` must + fall back, not be returned as-is. + + Regression guard for the actual bug: ``"."`` is a *safe* character + (kept, not stripped), so ``"../"`` and ``"."``+``"/"`` both collapse to + ``".."`` / ``"."`` once the separator is removed — a fixpoint of the + old (empty-only) fallback check, since neither is the empty string. + Without this fallback, a caller with no extra prefix protecting the + segment (``KnowledgeWriter``, unlike the skill writer's ``skill_`` + prefix) resolves one directory level up or sideways instead of into a + new child. + """ + sanitized = sanitize_dirname(raw, fallback="unnamed") + assert sanitized == "unnamed" + + +def test_knowledge_style_unprefixed_concatenation_stays_under_root() -> None: + """The one-level escape the coordinator reproduced on the (unprefixed) + knowledge path: ``Path(root) / sanitize_dirname("../", fb) / "doc_123"`` + must resolve under ``root``, not to ``root``'s parent. + """ + root = Path("/root/knowledge") + resolved = root / sanitize_dirname("../", "Others") / "doc_123" + assert resolved == Path("/root/knowledge/Others/doc_123") + + +def test_nfc_normalizes_decomposed_accents() -> None: + """An NFD-decomposed accented character (base letter + combining mark) + must sanitize to the same result as its NFC (precomposed) form — + without normalization, the combining mark is not ``\\w`` and gets + silently stripped, losing the accent instead of preserving it. + """ + nfc = "café" + nfd = unicodedata.normalize("NFD", nfc) + assert nfc != nfd # sanity: the two forms really are distinct strings + + sanitized_nfc = sanitize_dirname(nfc, fallback="unnamed") + sanitized_nfd = sanitize_dirname(nfd, fallback="unnamed") + + assert sanitized_nfc == sanitized_nfd == "café" + + +def test_nfc_does_not_help_composition_exclusions() -> None: + """Pins the documented exception: for Unicode "composition exclusion" + codepoints, NFC normalization does not help — it decomposes an + already-precomposed character, and the resulting combining mark is + stripped either way. + + U+0958 / U+0959 (Devanagari letters formed from a base letter + nukta) + are composition exclusions: their canonical decomposition is excluded + from NFC recomposition, so ``normalize("NFC", precomposed)`` yields the + *decomposed* form, not the precomposed one. + """ + precomposed = "क़ख़" + assert unicodedata.normalize("NFC", precomposed) != precomposed + + sanitized = sanitize_dirname(precomposed, fallback="unnamed") + + # The nukta (combining mark, U+093C) is lost: NFC decomposes the + # precomposed input into base + nukta, and the nukta is then stripped + # (not \w) -- the opposite of what NFC does for an ordinary NFD accent. + assert sanitized == "कख" + + +def test_cjk_and_space_input_preserved_readably() -> None: + raw = "修复 Django 自动重载问题" + sanitized = sanitize_dirname(raw, fallback="unnamed") + + assert "修复" in sanitized + assert "Django" in sanitized + assert "_" in sanitized # spaces became underscores, not stripped + assert " " not in sanitized + + +@pytest.mark.parametrize( + "raw", + [ + "../" * 8 + "tmp/pwned", + "修复 Django 自动重载问题", + "normal_skill", + "../../etc/passwd", + " ", + "!!!@@@###", + "..", + "../", + "/../", + ".", + "./", + ], +) +def test_sanitize_is_idempotent(raw: str) -> None: + once = sanitize_dirname(raw, fallback="unnamed") + twice = sanitize_dirname(once, fallback="unnamed") + assert once == twice + + +def test_empty_result_falls_back() -> None: + assert sanitize_dirname("!!!@@@###", fallback="unnamed") == "unnamed" + + +def test_truncates_to_max_length() -> None: + sanitized = sanitize_dirname("a" * 200, fallback="unnamed") + assert len(sanitized) == 50 diff --git a/tests/unit/test_entrypoints/test_api/test_routes/test_ome.py b/tests/unit/test_entrypoints/test_api/test_routes/test_ome.py new file mode 100644 index 000000000..0ffe9994d --- /dev/null +++ b/tests/unit/test_entrypoints/test_api/test_routes/test_ome.py @@ -0,0 +1,131 @@ +"""Route tests for ``POST /api/v1/ome/trigger``. + +Pins the widened ``TriggerResponse`` contract from Task 7: a ``dispatched`` +count and a per-run ``runs`` list are always present, and ``status`` gains +``not_dispatched`` for strategies rejected by a dispatch gate (as opposed to +``ok``/``timeout``, which mean the strategy was actually enqueued and either +settled or didn't within the wait window). + +Each test wires a bare FastAPI app carrying only the ``ome`` router to a +real, started ``OfflineEngine`` (no lifespan, no LanceDB, no LLM) — the +route's deferred ``_get_engine()`` import resolves to whatever +``everos.service.memorize._ome_engine`` holds, which is patched per test. +""" + +from __future__ import annotations + +import importlib +from collections.abc import AsyncIterator +from pathlib import Path + +import pytest +from fastapi import FastAPI +from httpx import ASGITransport, AsyncClient + +from everos.entrypoints.api.routes.ome import router as ome_router +from everos.infra.ome.config import OMEConfig +from everos.infra.ome.context import StrategyContext +from everos.infra.ome.decorator import offline_strategy +from everos.infra.ome.engine import OfflineEngine +from everos.infra.ome.events import ManualTick +from everos.infra.ome.triggers import Immediate + + +async def _client_for( + engine: OfflineEngine, monkeypatch: pytest.MonkeyPatch +) -> AsyncClient: + """FastAPI app exposing only the ome router, wired to ``engine``.""" + svc = importlib.import_module("everos.service.memorize") + monkeypatch.setattr(svc, "_ome_engine", engine, raising=False) + app = FastAPI() + app.include_router(ome_router, prefix="/api/v1") + return AsyncClient(transport=ASGITransport(app=app), base_url="http://test") + + +@pytest.fixture +async def gated_off_engine(tmp_path: Path) -> AsyncIterator[OfflineEngine]: + """Engine with one strategy registered but ``enabled=False``.""" + + @offline_strategy( + name="gated_off_strategy", + trigger=Immediate(on=[ManualTick]), + emits=[], + enabled=False, + ) + async def _s(event: ManualTick, ctx: StrategyContext) -> None: + return None + + engine = OfflineEngine( + config=OMEConfig(jobstore_path=tmp_path / "ome.db", config_watch=False) + ) + engine.register(_s) + await engine.start() + try: + yield engine + finally: + await engine.stop() + + +@pytest.fixture +async def always_fails_engine(tmp_path: Path) -> AsyncIterator[OfflineEngine]: + """Engine with a strategy that raises unconditionally. + + ``max_retries=0`` reaches ``dead_letter`` on the very first attempt — + no retry backoff sleep, so the test stays fast under the default runner. + """ + + @offline_strategy( + name="always_fails", + trigger=Immediate(on=[ManualTick]), + emits=[], + max_retries=0, + ) + async def _s(event: ManualTick, ctx: StrategyContext) -> None: + raise RuntimeError("boom") + + engine = OfflineEngine( + config=OMEConfig(jobstore_path=tmp_path / "ome.db", config_watch=False) + ) + engine.register(_s) + await engine.start() + try: + yield engine + finally: + await engine.stop() + + +async def test_trigger_returns_not_dispatched_when_strategy_gated_off( + gated_off_engine: OfflineEngine, monkeypatch: pytest.MonkeyPatch +) -> None: + """A strategy disabled by config, triggered without ``force``, yields + ``dispatched=0`` and ``status='not_dispatched'`` with no runs.""" + async with await _client_for(gated_off_engine, monkeypatch) as client: + resp = await client.post( + "/api/v1/ome/trigger", json={"name": "gated_off_strategy"} + ) + assert resp.status_code == 200 + body = resp.json() + assert body["status"] == "not_dispatched" + assert body["dispatched"] == 0 + assert body["runs"] == [] + + +async def test_trigger_returns_runs_including_dead_letter( + always_fails_engine: OfflineEngine, monkeypatch: pytest.MonkeyPatch +) -> None: + """A strategy that raises through all retries dead-letters; the run + still appears in ``runs`` with its error, and the top-level ``status`` + stays ``ok`` — dispatch happened and the run settled (dead-letter is a + settled state, not an in-flight one).""" + async with await _client_for(always_fails_engine, monkeypatch) as client: + resp = await client.post( + "/api/v1/ome/trigger", + json={"name": "always_fails", "force": True, "timeout": 15}, + ) + assert resp.status_code == 200 + body = resp.json() + assert body["status"] == "ok" + assert body["dispatched"] == 1 + assert len(body["runs"]) == 1 + assert body["runs"][0]["status"] == "dead_letter" + assert body["runs"][0]["error"] diff --git a/tests/unit/test_infra/test_markdown/test_mds/test_agent_skill.py b/tests/unit/test_infra/test_markdown/test_mds/test_agent_skill.py index 3b7776ca1..a2c921a91 100644 --- a/tests/unit/test_infra/test_markdown/test_mds/test_agent_skill.py +++ b/tests/unit/test_infra/test_markdown/test_mds/test_agent_skill.py @@ -95,6 +95,75 @@ def test_skill_extra_fields_still_allowed() -> None: assert dumped["last_indexed_at"] == "2026-05-07T08:00:00Z" +@pytest.mark.parametrize( + "bad_name", + [ + "../../../etc/passwd", + "skills/../../escape", + "a/b", + "a\\b", + "..", + ], +) +def test_skill_name_rejects_path_traversal(bad_name: str) -> None: + """Defence in depth: a hand-edited ``SKILL.md`` with a traversal-shaped + ``name`` is caught on parse rather than silently relocating the skill + on the next write (see :mod:`.frontmatter`'s ``skill_dir_name``, which + sanitizes the directory segment independently of this validator). + """ + with pytest.raises(ValidationError, match="path separators"): + AgentSkillFrontmatter(**_kwargs(name=bad_name)) # type: ignore[arg-type] + + +def test_skill_name_allows_cjk_and_spaces() -> None: + """Non-ASCII / whitespace names are legitimate — only traversal shapes + are rejected.""" + fm = AgentSkillFrontmatter(**_kwargs(name="修复 Django 自动重载问题")) # type: ignore[arg-type] + assert fm.name == "修复 Django 自动重载问题" + + +@pytest.mark.parametrize( + "raw_name", + [ + "..", + "../", + "/../", + ".", + "./", + "!!!", # sanitizes to empty -> fallback + "a" * 200, # truncation + "修复 Django 自动重载问题", # CJK + space + "../" * 8 + "tmp/pwned", + ], +) +def test_frontmatter_accepts_presanitized_boundary_names(raw_name: str) -> None: + """Mirrors ``extract_agent_skill._persist_skill``'s write path: the + caller sanitizes ``skill_name`` via + :meth:`AgentSkillFrontmatter.sanitize_skill_name` *before* constructing + the frontmatter, so a traversal-shaped or degenerate LLM name never + reaches the validator above as a raw, unsanitized string — construction + succeeds and the resulting ``name`` is a single, non-degenerate + component, rather than raising and dead-lettering the extraction run + for a name the sanitizer would have handled safely anyway. + + Covers the boundary family that a single long traversal payload does + not exercise: bare ``".."``, and inputs that strip down to ``".."`` or + ``"."`` once a leading/trailing separator is removed (``"." is a safe + character, so it is not itself stripped) — these are sanitizer + fixpoints, not just substrings, and are the exact inputs the fallback + in ``sanitize_dirname`` exists to catch. + """ + sanitized = AgentSkillFrontmatter.sanitize_skill_name(raw_name) + + assert "/" not in sanitized + assert "\\" not in sanitized + assert sanitized not in ("", ".", "..") + + fm = AgentSkillFrontmatter(**_kwargs(name=sanitized)) + + assert fm.name == sanitized + + def test_skill_directory_shape_classvars() -> None: """Path-shape ClassVars pin the wiki layout for the writer/reader pair.""" assert AgentSkillFrontmatter.SKILLS_CONTAINER_NAME == "skills" diff --git a/tests/unit/test_infra/test_markdown/test_readers/test_agent_skill_reader.py b/tests/unit/test_infra/test_markdown/test_readers/test_agent_skill_reader.py index cce33122d..f8ab77398 100644 --- a/tests/unit/test_infra/test_markdown/test_readers/test_agent_skill_reader.py +++ b/tests/unit/test_infra/test_markdown/test_readers/test_agent_skill_reader.py @@ -127,3 +127,244 @@ async def test_read_script_round_trip( async def test_read_script_returns_none_when_missing(reader: AgentSkillReader) -> None: assert await reader.read_script("agent_x", "alpha", "ghost.py") is None + + +async def test_list_by_cluster_returns_matching_skills( + writer: AgentSkillWriter, reader: AgentSkillReader +) -> None: + """Enumerates SKILL.md under the agent, filters by frontmatter cluster_id.""" + await writer.write_main( + "a1", + "revive_replica", + frontmatter=_make_fm( + id="a1_revive_replica", + agent_id="a1", + name="revive_replica", + cluster_id="cl1", + ), + body="b", + ) + await writer.write_main( + "a1", + "drain_queue", + frontmatter=_make_fm( + id="a1_drain_queue", agent_id="a1", name="drain_queue", cluster_id="cl1" + ), + body="b", + ) + await writer.write_main( + "a1", + "rotate_secrets", + frontmatter=_make_fm( + id="a1_rotate_secrets", + agent_id="a1", + name="rotate_secrets", + cluster_id="cl2", + ), + body="b", + ) + + results = await reader.list_by_cluster("a1", "cl1") + + names = sorted(fm.name for fm, _body in results) + assert names == ["drain_queue", "revive_replica"] + assert all(body == "b" for _fm, body in results) + + +async def test_list_by_cluster_ignores_skills_without_cluster_id( + writer: AgentSkillWriter, reader: AgentSkillReader +) -> None: + """Skills whose frontmatter cluster_id is None never leak into any bucket.""" + await writer.write_main( + "a1", + "orphan", + frontmatter=_make_fm(id="a1_orphan", agent_id="a1", name="orphan"), + body="b", + ) + await writer.write_main( + "a1", + "assigned", + frontmatter=_make_fm( + id="a1_assigned", agent_id="a1", name="assigned", cluster_id="cl1" + ), + body="b", + ) + + results = await reader.list_by_cluster("a1", "cl1") + assert [fm.name for fm, _body in results] == ["assigned"] + assert await reader.list_by_cluster("a1", "cl_missing") == [] + + +async def test_list_by_cluster_missing_dir_returns_empty( + reader: AgentSkillReader, +) -> None: + """New agent with no skill dir yet — returns [] without raising.""" + assert await reader.list_by_cluster("a_new", "cl1") == [] + + +async def test_list_by_cluster_finds_skill_whose_directory_suffix_has_a_space( + root: MemoryRoot, reader: AgentSkillReader +) -> None: + """Regression guard: before the fix, ``list_by_cluster`` recovered + ``skill_name`` from the directory suffix and called ``read_main``, + which re-derives (and re-sanitizes) the path from that name. A + directory whose suffix is not itself already a sanitizer fixpoint — + e.g. ``skill_My Skill`` (a raw space, never passed through + ``sanitize_dirname``) — re-derived to ``skill_My_Skill``, a path that + does not exist, so the skill was silently dropped from the result. + ``list_by_cluster`` is documented as the strong-consistency existence + check for cluster membership, so a dropped skill here would make the + LLM emit ``add()`` for a skill that already exists, duplicating it at + the sanitized path and orphaning the original. + + Writes the ``SKILL.md`` directly to the filesystem (bypassing both + ``AgentSkillWriter`` and ``_persist_skill``) to reproduce a directory + that was never sanitized in the first place — the scenario the fix + (reading the globbed path directly, never re-deriving it) must cover + regardless of how such a directory came to exist. Asserts the body + too, not just enumeration: a fix that stopped at "the frontmatter is + found" but still forced a caller to re-read by name (re-derive, and + re-sanitize, the same path) would have moved the drop one layer + downstream rather than closing it — see + ``test_extract_agent_skill.test_select_existing_skills_finds_skill_whose_directory_suffix_has_a_space`` + for the end-to-end version of this same property. + """ + skill_dir = root.agents_dir() / "a1" / "skills" / "skill_My Skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\n" + "id: a1_My Skill\n" + "type: agent_skill\n" + "agent_id: a1\n" + "track: agent\n" + "name: My Skill\n" + "description: d\n" + "confidence: 0.5\n" + "maturity_score: 0.5\n" + "cluster_id: cl1\n" + "---\n" + "The real skill body.\n", + encoding="utf-8", + ) + + results = await reader.list_by_cluster("a1", "cl1") + + assert len(results) == 1 + fm, body = results[0] + assert fm.name == "My Skill" + assert body == "The real skill body." + + +@pytest.mark.parametrize( + "frontmatter_lines", + [ + # Any schema constraint can fail, not just the traversal validator — + # a field a later schema revision makes required is the case existing + # files on disk would hit at upgrade time, all at once. + pytest.param("name: broken\ncluster_id: cl1\n", id="missing_required_field"), + # The traversal validator, whose whole stated purpose is catching a + # hand-edited name — i.e. a file that reaches exactly this route. + pytest.param( + "name: ..\ndescription: d\nconfidence: 0.5\n" + "maturity_score: 0.5\ncluster_id: cl1\n", + id="traversal_name", + ), + ], +) +async def test_list_by_cluster_skips_unparseable_skill( + root: MemoryRoot, + reader: AgentSkillReader, + frontmatter_lines: str, +) -> None: + """One unparseable ``SKILL.md`` must not starve the whole cluster. + + ``_read_path`` validates the full :class:`AgentSkillFrontmatter` schema, + so a single malformed file used to abort the entire enumeration — and + the enumeration is what feeds ``extract_agent_skill`` its existing + skills. Propagating would leave that strategy raising on every run for + the whole cluster, i.e. permanently dead-lettered: the exact failure + mode the md-first read path was introduced to eliminate. The good files + on either side of the bad one pin that the skip is per-file rather than + "stop at the first error" — sorted glob order puts ``skill_bad`` between + them, so a fix that merely stopped raising would still lose ``zzz``. + """ + skills = root.agents_dir() / "a1" / "skills" + + def _write(dirname: str, body_frontmatter: str, body: str) -> None: + d = skills / dirname + d.mkdir(parents=True) + (d / "SKILL.md").write_text( + f"---\ntype: agent_skill\nagent_id: a1\ntrack: agent\n" + f"id: a1_{dirname}\n{body_frontmatter}---\n{body}\n", + encoding="utf-8", + ) + + good = "description: d\nconfidence: 0.5\nmaturity_score: 0.5\ncluster_id: cl1\n" + _write("skill_aaa", f"name: aaa\n{good}", "body aaa") + _write("skill_bad", frontmatter_lines, "body bad") + _write("skill_zzz", f"name: zzz\n{good}", "body zzz") + + results = await reader.list_by_cluster("a1", "cl1") + + assert [fm.name for fm, _ in results] == ["aaa", "zzz"] + assert [body for _, body in results] == ["body aaa", "body zzz"] + + +async def test_read_main_propagates_validation_error( + root: MemoryRoot, reader: AgentSkillReader +) -> None: + """``read_main`` keeps raising — the skip is scoped to enumeration. + + A caller naming one specific skill gets an error rather than ``None``: + ``None`` means "not created yet", a normal state this reader's callers + branch on, and silently reusing it for "exists but is corrupt" would let + an upsert overwrite the damaged file instead of surfacing it. + """ + skill_dir = root.agents_dir() / "a1" / "skills" / "skill_aaa" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\ntype: agent_skill\nagent_id: a1\ntrack: agent\n" + "id: a1_aaa\nname: aaa\n---\nbody\n", + encoding="utf-8", + ) + + with pytest.raises(ValidationError): + await reader.read_main("a1", "aaa", schema=AgentSkillFrontmatter) + + +async def test_read_main_rederivation_from_raw_name_is_idempotent_safe( + writer: AgentSkillWriter, reader: AgentSkillReader +) -> None: + """``read_main`` re-derives a path from a caller-supplied name; that + re-derivation must land on the same file a direct write produced, even + when the name it's given is raw and unsanitized. + + No production caller currently re-derives a path from + ``list_by_cluster``'s output — it returns each skill's body directly + (see ``AgentSkillReader.list_by_cluster``'s docstring), so + ``extract_agent_skill`` never calls ``read_main`` in that flow anymore. + ``read_main`` remains a general single-skill lookup on the reader's + public API, so this test pins its re-derivation as a property of the + method itself: writing via a *raw*, unsanitized name directly through + the writer, then reading back via that same raw name, must resolve to + the same file (idempotent-safe), for any future caller that does pass + a raw name. + """ + space_name = "修复 Django 自动重载问题" + await writer.write_main( + "a1", + space_name, + frontmatter=_make_fm( + id="a1_django_reload_fix", + agent_id="a1", + name=space_name, + cluster_id="cl1", + ), + body="The fix body.", + ) + + out = await reader.read_main("a1", space_name, schema=AgentSkillFrontmatter) + assert out is not None + fm_out, body = out + assert fm_out.name == space_name + assert body == "The fix body." diff --git a/tests/unit/test_infra/test_markdown/test_writers/test_agent_skill_writer.py b/tests/unit/test_infra/test_markdown/test_writers/test_agent_skill_writer.py index 17c09cff4..b3c271750 100644 --- a/tests/unit/test_infra/test_markdown/test_writers/test_agent_skill_writer.py +++ b/tests/unit/test_infra/test_markdown/test_writers/test_agent_skill_writer.py @@ -9,6 +9,7 @@ from everos.core.persistence import MarkdownReader, MemoryRoot from everos.infra.persistence.markdown import ( AgentSkillFrontmatter, + AgentSkillReader, AgentSkillWriter, ) @@ -135,6 +136,74 @@ def test_main_path_does_not_create_anything( assert not root.agents_dir().exists() +def test_main_path_sanitizes_traversal_skill_name( + root: MemoryRoot, writer: AgentSkillWriter +) -> None: + """A ``../``-laden ``skill_name`` (raw LLM output) must not escape the agent dir. + + CWE-22 regression guard: prior to sanitization, ``skill_name`` was + concatenated straight into the path, so a sufficiently long ``../`` + prefix resolved outside ``root.agents_dir()`` entirely. ``main_path`` + is a pure resolver (no frontmatter involved, no IO), matching how the + traversal was originally measured. + """ + traversal_name = "../" * 8 + "tmp/pwned" + + path = writer.main_path("agent_x", traversal_name) + + assert path.resolve().is_relative_to(root.agents_dir().resolve()) + assert path.name == "SKILL.md" + assert "/" not in path.parent.name + assert path.parent.parent == root.agents_dir() / "agent_x" / "skills" + + +_BOUNDARY_RAW_NAMES = [ + "..", + "../", + "/../", + ".", + "./", + "!!!", # sanitizes to empty -> fallback + "a" * 200, # truncation + "修复 Django 自动重载问题", # CJK + space + "../" * 8 + "tmp/pwned", +] + + +@pytest.mark.parametrize("raw_name", _BOUNDARY_RAW_NAMES) +async def test_presanitized_name_identical_to_directory_segment( + root: MemoryRoot, writer: AgentSkillWriter, raw_name: str +) -> None: + """Mirrors ``extract_agent_skill._persist_skill``: sanitize + ``skill_name`` once, up front, then use that same sanitized string for + both the frontmatter ``name`` field and the writer's ``skill_name`` + argument. Covers the boundary family that previously slipped through + the sanitizer as a fixpoint (``".."`` alone, or with a leading/trailing + separator that strips down to it; ``"."`` likewise) in addition to the + empty/truncation/CJK/traversal cases already covered. + + For each input: the sanitized name is a single path component + (contains no separator), is never ``""`` / ``"."`` / ``".."``, + constructing ``AgentSkillFrontmatter`` with it succeeds, and + ``frontmatter.name`` is byte-identical (an identity, not merely + idempotent-if-resanitized) to the directory segment actually written. + """ + sanitized_name = AgentSkillFrontmatter.sanitize_skill_name(raw_name) + + assert "/" not in sanitized_name + assert "\\" not in sanitized_name + assert sanitized_name not in ("", ".", "..") + + fm = _make_fm(name=sanitized_name, id=f"agent_x_{sanitized_name}") + + path = await writer.write_main("agent_x", sanitized_name, frontmatter=fm, body="b") + + dir_derived_name = path.parent.name.removeprefix( + AgentSkillFrontmatter.SKILL_DIR_PREFIX + ) + assert fm.name == dir_derived_name + + async def test_write_main_normalises_trailing_newline( root: MemoryRoot, writer: AgentSkillWriter ) -> None: @@ -145,3 +214,58 @@ async def test_write_main_normalises_trailing_newline( root.agents_dir() / "agent_x" / "skills" / "skill_alpha" / "SKILL.md" ).read_text(encoding="utf-8") assert text.endswith("no-newline-end\n") + + +@pytest.mark.parametrize( + ("reference_name", "script_filename"), + [ + pytest.param("../" * 6 + "etc/passwd", "../" * 6 + "evil.sh", id="traversal"), + pytest.param("..", "..", id="dotdot_fixpoint"), + pytest.param("", "", id="empty"), + pytest.param("notes/../../x", "run/../../x.sh", id="embedded_separators"), + ], +) +async def test_reference_and_script_segments_cannot_escape_the_skill_dir( + root: MemoryRoot, + writer: AgentSkillWriter, + reference_name: str, + script_filename: str, +) -> None: + """These two segments are appended *after* ``skill_dir_name``. + + ``skill_dir_name`` only sanitizes the ``skill_`` component, so it + offers these no protection at all — they need their own pass through + ``sanitize_dirname``. Nothing in ``src/`` calls them yet; they are + covered now because they are public API whose inputs will come from the + same untrusted place the skill name does once progressive disclosure is + wired up, and because the traversal fix would otherwise read as + repo-wide when it is not. + """ + skill_dir = root.agents_dir() / "agent_x" / "skills" / "skill_alpha" + + ref = await writer.write_reference("agent_x", "alpha", reference_name, "x") + script = await writer.write_script("agent_x", "alpha", script_filename, "x") + + for path in (ref, script): + assert path.is_relative_to(skill_dir) + assert ".." not in path.parts + assert path.is_file() + + +async def test_reader_resolves_the_same_sanitized_reference_and_script_paths( + root: MemoryRoot, writer: AgentSkillWriter +) -> None: + """Reader and writer must sanitize every segment identically. + + Sanitizing only one side would silently split a write from its matching + read — the write lands on the safe path, the read looks at the raw one + and reports the file missing. This is the same reader/writer symmetry + ``skill_dir_name`` maintains for the skill directory, extended to the + two segments appended after it. + """ + reader = AgentSkillReader(root) + await writer.write_reference("agent_x", "alpha", "my notes!", "ref body") + await writer.write_script("agent_x", "alpha", "run this.sh", "echo hi\n") + + assert await reader.read_reference("agent_x", "alpha", "my notes!") == "ref body" + assert await reader.read_script("agent_x", "alpha", "run this.sh") == "echo hi" diff --git a/tests/unit/test_infra/test_markdown/test_writers/test_knowledge_writer.py b/tests/unit/test_infra/test_markdown/test_writers/test_knowledge_writer.py index f5ff901f6..c91850ff2 100644 --- a/tests/unit/test_infra/test_markdown/test_writers/test_knowledge_writer.py +++ b/tests/unit/test_infra/test_markdown/test_writers/test_knowledge_writer.py @@ -218,6 +218,59 @@ async def test_empty_slug_fallback_for_title(tmp_path: Path) -> None: assert (doc_dir / "index.md").is_file() +async def test_decomposed_accent_survives_in_directory_name(tmp_path: Path) -> None: + """A decomposed (NFD) title keeps its accent in the directory name. + + Pins a deliberate behavior change: this writer's private sanitizer was + replaced by the shared ``core.persistence.markdown.path_safety`` + primitive, which NFC-normalizes before filtering. The character class + (``[^\\w\\-.]``, Unicode-aware) is unchanged, so precomposed input — + including CJK — resolved identically before and after; NFD input did + not. A bare combining mark is not ``\\w``, so ``"e"`` + U+0301 used to + lose the accent and land in ``Résumé_…`` → ``Resume_…``. + + This is the one directory-name change with a pre-existing corpus behind + it: knowledge upload shipped before this fix, so a document whose topic + arrived decomposed resolves to a *different* directory now than the one + already on disk. Kept because titles reaching here are overwhelmingly + precomposed already; pinned because nothing else in the suite would + notice a regression back to the stripping behavior. + """ + nfd_topic = "Re" + "́" + "sume" + "́" # "Résumé", decomposed + doc_id = "d_nfd0001" + doc_dir = await KnowledgeWriter.write( + [_root_node(doc_id=doc_id, topic=nfd_topic)], tmp_path + ) + + assert doc_dir == tmp_path / "Sports" / f"Résumé_{doc_id}" + assert (doc_dir / "index.md").is_file() + + +async def test_dot_only_topic_falls_back_instead_of_escaping(tmp_path: Path) -> None: + """A ``".."`` topic must not resolve the document dir to its parent. + + ``".."`` is a fixpoint of the character filter (``.`` is a safe + character), so before the fallback on degenerate results it survived + intact — and because this writer appends no prefix of its own, the + resulting ``//.._`` was one literal + component but ```` itself was not, letting a ``".."`` + *category* climb a level. Asserts containment rather than the exact + fallback string: the property that matters is that no path component + can walk back out of ``tmp_path``. Both assertions are needed and + neither implies the other — ``is_relative_to`` is prefix arithmetic + that a ``".."`` component would satisfy while still escaping, and the + ``parts`` check alone says nothing about where the path is rooted. + """ + doc_id = "d_dots001" + doc_dir = await KnowledgeWriter.write( + [_root_node(doc_id=doc_id, topic="..", category_id="..")], tmp_path + ) + + assert doc_dir.is_relative_to(tmp_path) + assert ".." not in doc_dir.parts + assert (doc_dir / "index.md").is_file() + + async def test_empty_category_id_fallback_to_others(tmp_path: Path) -> None: memories = [_root_node(category_id="")] doc_dir = await KnowledgeWriter.write(memories, tmp_path) diff --git a/tests/unit/test_infra/test_ome/test_runner.py b/tests/unit/test_infra/test_ome/test_runner.py index c7cb26a81..714544509 100644 --- a/tests/unit/test_infra/test_ome/test_runner.py +++ b/tests/unit/test_infra/test_ome/test_runner.py @@ -9,8 +9,9 @@ from everos.infra.ome._dispatch.runner import Runner from everos.infra.ome._stores.run_record import RunRecordStore from everos.infra.ome._stores.storage import OMEStorage +from everos.infra.ome.config import OMEConfig from everos.infra.ome.context import StrategyContext -from everos.infra.ome.decorator import offline_strategy +from everos.infra.ome.decorator import StrategyMeta, offline_strategy from everos.infra.ome.events import BaseEvent from everos.infra.ome.records import RunStatus from everos.infra.ome.triggers import Immediate @@ -26,12 +27,13 @@ async def setup(tmp_path: Path): await storage.init() rec_store = RunRecordStore(storage=storage, max_records_per_strategy=1000) sem = asyncio.Semaphore(20) - return rec_store, sem + config = OMEConfig(jobstore_path=tmp_path / "ome.db") + return rec_store, sem, config @pytest.mark.asyncio async def test_runner_success_marks_record(setup) -> None: - rec_store, sem = setup + rec_store, sem, config = setup @offline_strategy(name="ok", trigger=Immediate(on=[_E]), emits=[]) async def s(event: _E, ctx: StrategyContext) -> None: @@ -41,6 +43,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) await runner.run( @@ -55,8 +58,18 @@ async def s(event: _E, ctx: StrategyContext) -> None: @pytest.mark.asyncio -async def test_runner_retries_on_failure(setup) -> None: - rec_store, sem = setup +async def test_runner_retries_on_failure( + setup, monkeypatch: pytest.MonkeyPatch +) -> None: + # Retry loop now sleeps between attempts (real backoff config from + # ``setup``); fake the sleep so this test stays fast — it asserts + # retry *counting*, not backoff timing (see test_runner_applies_ + # exponential_backoff_between_attempts for that). + monkeypatch.setattr( + "everos.infra.ome._dispatch.runner.asyncio.sleep", + _instant_sleep, + ) + rec_store, sem, config = setup calls = {"n": 0} @offline_strategy( @@ -74,6 +87,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) await runner.run( @@ -94,8 +108,14 @@ async def s(event: _E, ctx: StrategyContext) -> None: @pytest.mark.asyncio -async def test_runner_dead_letter_after_exhaust(setup) -> None: - rec_store, sem = setup +async def test_runner_dead_letter_after_exhaust( + setup, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr( + "everos.infra.ome._dispatch.runner.asyncio.sleep", + _instant_sleep, + ) + rec_store, sem, config = setup @offline_strategy( name="bad", @@ -112,6 +132,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, on_dead_letter=lambda r: dl_calls.append(r), engine=MagicMock(), ) @@ -131,7 +152,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: @pytest.mark.asyncio async def test_runner_emit_must_be_declared(setup) -> None: - rec_store, sem = setup + rec_store, sem, config = setup class _Other(BaseEvent): pass @@ -148,6 +169,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) await runner.run( @@ -167,7 +189,7 @@ async def test_runner_negative_max_retries_raises(setup) -> None: constrains the user-supplied source to ``>= 0``), so the framework fails fast rather than silently no-op the run. """ - rec_store, sem = setup + rec_store, sem, config = setup @offline_strategy(name="ok", trigger=Immediate(on=[_E]), emits=[]) async def s(event: _E, ctx: StrategyContext) -> None: @@ -177,6 +199,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) with pytest.raises(ValueError, match=r"max_retries_snapshot must be >= 0"): @@ -198,7 +221,7 @@ async def test_runner_aborts_silently_when_mark_running_fails( crash recovery to pick up, so re-execution via recovery is impossible. The emergency log is the only audit trail. """ - rec_store, sem = setup + rec_store, sem, config = setup called = {"n": 0} @offline_strategy(name="ok", trigger=Immediate(on=[_E]), emits=[]) @@ -214,6 +237,7 @@ async def _boom(**_: object) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) # Must NOT raise; the framework swallows + logs. @@ -230,11 +254,17 @@ async def _no_emit(event: BaseEvent) -> None: return None +async def _instant_sleep(seconds: float) -> None: + """Drop-in ``asyncio.sleep`` replacement for tests that exercise the + retry loop but don't care about backoff timing.""" + return None + + @pytest.mark.asyncio async def test_runner_emits_ome_agent_span(setup) -> None: """Runner wraps the strategy body in an everos.ome. agent span (its own trace — runs in an APScheduler task, no request context).""" - rec_store, sem = setup + rec_store, sem, config = setup @offline_strategy(name="traced_strat", trigger=Immediate(on=[_E]), emits=[]) async def s(event: _E, ctx: StrategyContext) -> None: @@ -263,6 +293,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) await runner.run(s.meta, _E(), run_id="r_trace", max_retries_snapshot=1) @@ -282,7 +313,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: async def test_runner_ome_span_links_to_upstream_traceparent(setup) -> None: """Given a traceparent (captured where a request span was active), the everos.ome. span nests under that upstream trace, not a new root.""" - rec_store, sem = setup + rec_store, sem, config = setup @offline_strategy(name="linked_strat", trigger=Immediate(on=[_E]), emits=[]) async def s(event: _E, ctx: StrategyContext) -> None: @@ -318,6 +349,7 @@ async def s(event: _E, ctx: StrategyContext) -> None: run_record_store=rec_store, engine_sem=sem, emit_hook=_no_emit, + config=config, engine=MagicMock(), ) await runner.run( @@ -332,3 +364,165 @@ async def s(event: _E, ctx: StrategyContext) -> None: ] assert ome.context.trace_id == parent_tid # same trace as the request assert ome.parent is not None # child, not a fresh root + + +async def _make_runner_with_transient_failing_strategy( + tmp_path: Path, + *, + max_retries: int, + backoff_base: float = 1.0, + backoff_cap: float = 10.0, + jitter: float = 0.5, +) -> tuple[Runner, StrategyMeta]: + """Build a ``Runner`` wired to a strategy that raises on every attempt. + + Drives the retry loop to exhaustion so the backoff sleep fires between + each of the ``max_retries`` retries, for tests asserting on + ``asyncio.sleep`` call arguments. + """ + storage = OMEStorage(db_path=tmp_path / "ome.db") + await storage.init() + rec_store = RunRecordStore(storage=storage, max_records_per_strategy=1000) + sem = asyncio.Semaphore(20) + config = OMEConfig( + jobstore_path=tmp_path / "ome.db", + retry_backoff_base_seconds=backoff_base, + retry_backoff_cap_seconds=backoff_cap, + retry_jitter_seconds=jitter, + ) + + @offline_strategy( + name="transient_failing", + trigger=Immediate(on=[_E]), + emits=[], + max_retries=max_retries, + ) + async def s(event: _E, ctx: StrategyContext) -> None: + raise RuntimeError("transient") + + runner = Runner( + run_record_store=rec_store, + engine_sem=sem, + emit_hook=_no_emit, + config=config, + engine=MagicMock(), + ) + return runner, s.meta + + +@pytest.mark.asyncio +async def test_runner_applies_exponential_backoff_between_attempts( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Each attempt after the first sleeps ~ base * 2**(attempt-1), capped at + cap_seconds, with up to jitter_seconds added. Verifies that a strategy + raising a transient exception gets real wall-clock breathing room.""" + sleeps: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + sleeps.append(seconds) + + monkeypatch.setattr("everos.infra.ome._dispatch.runner.asyncio.sleep", fake_sleep) + + runner, meta = await _make_runner_with_transient_failing_strategy( + tmp_path, max_retries=3 + ) + await runner.run(meta, _E(), run_id="r1", max_retries_snapshot=3) + + # attempts 1, 2, 3 (0 has no preceding sleep); base=1, cap=10, jitter=0.5 + assert len(sleeps) == 3 + assert 1.0 <= sleeps[0] <= 1.5 # ~1s + jitter + assert 2.0 <= sleeps[1] <= 2.5 # ~2s + jitter + assert 4.0 <= sleeps[2] <= 4.5 # ~4s + jitter + + +@pytest.mark.asyncio +async def test_runner_releases_engine_sem_across_backoff_sleep( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The concurrency slot must be free while a run waits to retry. + + ``engine_sem`` bounds concurrent strategy *work* — LLM calls, + embeddings, storage IO — and a coroutine sleeping between attempts + consumes none of it. Holding the slot across the sleep turns a + partial outage into a total stall: with N slots and a 1s/2s/4s + backoff, N simultaneously-failing runs park every slot in + ``asyncio.sleep`` and starve strategies that would have succeeded. + + Uses a single-permit semaphore so ``locked()`` is unambiguous, and + asserts a second waiter actually acquires — ``locked()`` alone would + pass on an implementation that released the slot but left a waiter + unable to take it. + """ + storage = OMEStorage(db_path=tmp_path / "ome.db") + await storage.init() + rec_store = RunRecordStore(storage=storage, max_records_per_strategy=1000) + sem = asyncio.Semaphore(1) + config = OMEConfig( + jobstore_path=tmp_path / "ome.db", + retry_backoff_base_seconds=1.0, + retry_backoff_cap_seconds=10.0, + retry_jitter_seconds=0.0, + ) + + held_during_sleep: list[bool] = [] + acquired_during_sleep: list[bool] = [] + + async def fake_sleep(seconds: float) -> None: + held_during_sleep.append(sem.locked()) + try: + async with asyncio.timeout(0.5): + await sem.acquire() + except TimeoutError: + acquired_during_sleep.append(False) + else: + acquired_during_sleep.append(True) + sem.release() + + monkeypatch.setattr("everos.infra.ome._dispatch.runner.asyncio.sleep", fake_sleep) + + @offline_strategy( + name="sem_probe_failing", + trigger=Immediate(on=[_E]), + emits=[], + max_retries=2, + ) + async def s(event: _E, ctx: StrategyContext) -> None: + raise RuntimeError("transient") + + runner = Runner( + run_record_store=rec_store, + engine_sem=sem, + emit_hook=_no_emit, + config=config, + engine=MagicMock(), + ) + await runner.run(s.meta, _E(), run_id="r1", max_retries_snapshot=2) + + assert held_during_sleep == [False, False] + assert acquired_during_sleep == [True, True] + + +@pytest.mark.asyncio +async def test_runner_backoff_caps_at_configured_maximum( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Backoff cap prevents unbounded growth on high max_retries.""" + sleeps: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + sleeps.append(seconds) + + monkeypatch.setattr("everos.infra.ome._dispatch.runner.asyncio.sleep", fake_sleep) + + runner, meta = await _make_runner_with_transient_failing_strategy( + tmp_path, + max_retries=6, + backoff_base=1.0, + backoff_cap=3.0, + jitter=0.0, + ) + await runner.run(meta, _E(), run_id="r1", max_retries_snapshot=6) + + # 1, 2, 3, 3, 3, 3 -- all capped after attempt 3 + assert sleeps == [1.0, 2.0, 3.0, 3.0, 3.0, 3.0] diff --git a/tests/unit/test_memory/test_events.py b/tests/unit/test_memory/test_events.py index 086b63ad8..1328bcc09 100644 --- a/tests/unit/test_memory/test_events.py +++ b/tests/unit/test_memory/test_events.py @@ -4,7 +4,12 @@ import pytest from everalgo.types import ChatMessage, MemCell -from everos.memory.events import AgentPipelineStarted, UserPipelineStarted +from everos.memory.events import ( + AgentCaseExtracted, + AgentPipelineStarted, + SkillClusterUpdated, + UserPipelineStarted, +) def _sample_memcell() -> MemCell: @@ -83,3 +88,44 @@ def test_user_pipeline_started_nested_roundtrip_json() -> None: assert restored.memcell.items[0].id == "m1" assert restored.memcell.items[1].content == "hi back" assert restored.memcell.timestamp == 1_700_000_001_000 + + +def test_agent_case_extracted_new_fields_default() -> None: + """approach/key_insight default so a pre-1.2.3 event payload deserializes.""" + payload = { + "memcell_id": "m1", + "case_entry_id": "c1", + "task_intent": "cook risotto", + "quality_score": 0.8, + "case_timestamp_ms": 1_700_000_000_000, + "agent_id": "a1", + } + event = AgentCaseExtracted.model_validate(payload) + assert event.approach == "" + assert event.key_insight is None + + +def test_skill_cluster_updated_new_fields_default() -> None: + """All 6 pass-through fields default; a pre-1.2.3 payload deserializes.""" + payload = {"case_entry_id": "c1", "cluster_id": "cl1", "agent_id": "a1"} + event = SkillClusterUpdated.model_validate(payload) + assert event.task_intent == "" + assert event.approach == "" + assert event.key_insight is None + assert event.quality_score == 0.0 + assert event.case_timestamp_ms == 0 + assert event.case_vector is None + + +def test_skill_cluster_updated_carries_case_vector() -> None: + """When set, case_vector round-trips through JSON serialization.""" + payload = { + "case_entry_id": "c1", + "cluster_id": "cl1", + "agent_id": "a1", + "case_vector": [0.1, 0.2, 0.3], + } + event = SkillClusterUpdated.model_validate_json( + SkillClusterUpdated.model_validate(payload).model_dump_json() + ) + assert event.case_vector == [0.1, 0.2, 0.3] diff --git a/tests/unit/test_memory/test_search/test_agentic_agent.py b/tests/unit/test_memory/test_search/test_agentic_agent.py index 9572952f9..0fa6be0f8 100644 --- a/tests/unit/test_memory/test_search/test_agentic_agent.py +++ b/tests/unit/test_memory/test_search/test_agentic_agent.py @@ -1,7 +1,16 @@ """Unit tests for ``memory.search.agentic_agent``. -White-box: patches ``aagentic_retrieve`` to assert benchmark hyperparameters -are wired correctly, plus a shaping test to verify DTOs are built correctly. +Two groups of tests: + +* White-box (patches ``aagentic_retrieve``): assert benchmark hyperparameters + are wired correctly, plus a shaping test to verify DTOs are built + correctly. These never execute the real ``everalgo._format_docs`` / + rerank_fn wiring — they are dead coverage for the metadata bridge. +* Black-box (does NOT patch ``aagentic_retrieve``): exercises the real + ``_format_docs`` prompt-rendering path and the real kind-shaped rerank_fn + via ``everalgo.testing.fake_llm.FakeLLMClient``. These pin the regression + fixed by the metadata-bridge refactor (empty-description skill -> 500) + and the skill/case rerank-fn swap. The skill verify step has been removed from production code; this test module covers the agentic retrieve flow only. @@ -10,6 +19,7 @@ from __future__ import annotations import datetime as _dt +import json from typing import Any, ClassVar from unittest.mock import patch @@ -17,10 +27,15 @@ from everalgo.testing.fake_llm import FakeLLMClient from everalgo.types import Candidate +from everos.component.rerank import RerankResult from everos.memory.search.agentic_agent import ( search_agent_cases_agentic, search_agent_skills_agentic, ) +from everos.memory.search.callbacks import ( + _CASE_RERANK_INSTRUCTION, + _SKILL_RERANK_INSTRUCTION, +) from everos.memory.search.dto import SearchAgentCaseItem, SearchAgentSkillItem # ── Stubs ──────────────────────────────────────────────────────────────── @@ -270,3 +285,211 @@ async def fake_aagentic( assert isinstance(result[0], SearchAgentSkillItem) assert result[0].id == "s_1" assert result[0].name == "skill_s_1" + + +# ── Black-box tests: real _format_docs + real kind-shaped rerank_fn ──────── +# +# These deliberately do NOT patch ``aagentic_retrieve`` (unlike every test +# above), so the metadata bridge (``_to_everalgo_doc_metadata``) and the +# kind-shaped ``rerank_fn`` (``build_skill_rerank_fn`` / ``build_case_rerank_fn``) +# actually run. + +# JSON body a real LLM would return for the sufficiency-check prompt; parsed +# by ``everalgo.rank.agentic._call_llm_for_sufficiency``. ``is_sufficient=True`` +# short-circuits Round 2, so one LLM call is enough for every test below. +_SUFFICIENT_LLM_RESPONSE = json.dumps( + { + "is_sufficient": True, + "reasoning": "single relevant candidate", + "key_information_found": [], + "missing_information": [], + } +) + + +class _StubSkillRecallerAsym: + """Like ``_StubSkillRecaller`` but with independently controllable + dense/sparse routes, needed to pin an exact fused score.""" + + kind: ClassVar[str] = "agent_skill" + everalgo_memory_type: ClassVar[str] = "skill" + text_field: ClassVar[str] = "description" + + def __init__( + self, *, dense: list[Candidate], sparse: list[Candidate] | None = None + ) -> None: + self._dense = dense + self._sparse = sparse if sparse is not None else list(dense) + + async def sparse_recall(self, *_: Any, **__: Any) -> list[Candidate]: + return list(self._sparse) + + async def dense_recall(self, *_: Any, **__: Any) -> list[Candidate]: + return list(self._dense) + + +class _StubCaseRecallerAsym: + """Case-kind counterpart of :class:`_StubSkillRecallerAsym`.""" + + kind: ClassVar[str] = "agent_case" + everalgo_memory_type: ClassVar[str] = "case" + text_field: ClassVar[str] = "task_intent" + + def __init__( + self, *, dense: list[Candidate], sparse: list[Candidate] | None = None + ) -> None: + self._dense = dense + self._sparse = sparse if sparse is not None else list(dense) + + async def sparse_recall(self, *_: Any, **__: Any) -> list[Candidate]: + return list(self._sparse) + + async def dense_recall(self, *_: Any, **__: Any) -> list[Candidate]: + return list(self._dense) + + +class _IdentityReranker: + """Rerank stub that preserves input order; accepts ``instruction`` like + a real :class:`RerankProvider` (unlike ``_StubReranker`` above, which + only satisfies the white-box tests where rerank_fn is never called).""" + + async def rerank( + self, query: str, passages: list[str], *, instruction: str | None = None + ) -> list[RerankResult]: + return [RerankResult(index=i, score=1.0) for i in range(len(passages))] + + +def _skill_metadata(*, name: str, description: str) -> dict[str, Any]: + return { + "owner_id": "agent_a", + "owner_type": "agent", + "name": name, + "description": description, + "content": "some remediation content", + "confidence": 0.9, + "maturity_score": 0.6, + "source_case_ids": [], + } + + +async def test_agentic_survives_name_only_skill() -> None: + """A skill with an empty description is a valid everalgo output (see + everalgo ``agent_memory/skill_ops.py`` — the guard is ``if not name and + not description``). The metadata bridge must produce a non-empty + passage for ``_format_docs``, otherwise everalgo raises ``ValueError`` + and the request 500s. This is the regression test for that defect.""" + skill = Candidate( + id="s_name_only", + score=0.9, + source="vector", + metadata=_skill_metadata(name="rotate_secrets", description=""), + ) + recaller = _StubSkillRecallerAsym(dense=[skill]) + + result = await search_agent_skills_agentic( + "how to rotate credentials", + where="owner_id = 'agent_a' AND owner_type = 'agent'", + skill_recaller=recaller, + embed_query_fn=_fake_embed, + reranker=_IdentityReranker(), + llm=FakeLLMClient(responses=[_SUFFICIENT_LLM_RESPONSE]), + top_k=5, + ) + + assert len(result) == 1 + assert result[0].name == "rotate_secrets" + + +async def test_agentic_uses_skill_rerank_passage() -> None: + """Rerank input passages must be the skill-shaped multi-field format + (``build_skill_rerank_fn``'s ``"Agent Skill: {name} - {description}"``), + not the raw single-field text the generic ``build_rerank_fn`` would use. + Proves the rerank_fn swap in ``_run_agentic_retrieve`` actually happened.""" + captured_passages: list[str] = [] + captured_instruction: str | None = None + + class _SpyReranker: + async def rerank( + self, + query: str, + passages: list[str], + *, + instruction: str | None = None, + ) -> list[RerankResult]: + nonlocal captured_instruction + captured_passages[:] = passages + captured_instruction = instruction + return [RerankResult(index=i, score=1.0) for i in range(len(passages))] + + skill = Candidate( + id="s_revive", + score=0.9, + source="vector", + metadata=_skill_metadata(name="revive_replica", description="restart node"), + ) + recaller = _StubSkillRecallerAsym(dense=[skill]) + + await search_agent_skills_agentic( + "how to bring a replica back", + where="owner_id = 'agent_a' AND owner_type = 'agent'", + skill_recaller=recaller, + embed_query_fn=_fake_embed, + reranker=_SpyReranker(), + llm=FakeLLMClient(responses=[_SUFFICIENT_LLM_RESPONSE]), + top_k=5, + ) + + assert captured_passages == ["Agent Skill: revive_replica - restart node"] + assert captured_instruction == _SKILL_RERANK_INSTRUCTION + + +async def test_agentic_uses_case_rerank_passage() -> None: + """Symmetric to ``test_agentic_uses_skill_rerank_passage`` for the + agent_case kind: passages must be ``"Agent Case: {task_intent} - + {approach}"`` with ``_CASE_RERANK_INSTRUCTION``.""" + captured_passages: list[str] = [] + captured_instruction: str | None = None + + class _SpyReranker: + async def rerank( + self, + query: str, + passages: list[str], + *, + instruction: str | None = None, + ) -> list[RerankResult]: + nonlocal captured_instruction + captured_passages[:] = passages + captured_instruction = instruction + return [RerankResult(index=i, score=1.0) for i in range(len(passages))] + + case = Candidate( + id="c_restart", + score=0.9, + source="vector", + metadata={ + "owner_id": "agent_a", + "owner_type": "agent", + "session_id": "sess_b", + "timestamp": _ts(), + "task_intent": "restart the pod", + "approach": "kubectl rollout restart", + "quality_score": 0.8, + }, + ) + recaller = _StubCaseRecallerAsym(dense=[case]) + + await search_agent_cases_agentic( + "how to restart a stuck pod", + where="owner_id = 'agent_a' AND owner_type = 'agent'", + case_recaller=recaller, + embed_query_fn=_fake_embed, + reranker=_SpyReranker(), + llm=FakeLLMClient(responses=[_SUFFICIENT_LLM_RESPONSE]), + top_k=5, + ) + + assert captured_passages == [ + "Agent Case: restart the pod - kubectl rollout restart" + ] + assert captured_instruction == _CASE_RERANK_INSTRUCTION diff --git a/tests/unit/test_memory/test_strategies/test_extract_agent_case.py b/tests/unit/test_memory/test_strategies/test_extract_agent_case.py index 42642ae9f..42d59dece 100644 --- a/tests/unit/test_memory/test_strategies/test_extract_agent_case.py +++ b/tests/unit/test_memory/test_strategies/test_extract_agent_case.py @@ -225,6 +225,44 @@ async def test_fans_out_per_assistant_sender( assert matching[0]["fanout"] == 2 +async def test_emit_includes_approach_and_key_insight( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """1.2.3+ emitters populate the new case-body fields on AgentCaseExtracted.""" + monkeypatch.setattr(mod, "_writer", None, raising=False) + case = _algo_case( + task_intent="restore MongoDB replica", + approach="1. stop node 2. resync 3. verify", + quality_score=0.75, + key_insight="watch oplog lag", + ) + with ( + patch( + "everos.memory.strategies.extract_agent_case.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.extract_agent_case.AgentCaseExtractor" + ) as mock_cls, + patch( + "everos.memory.strategies.extract_agent_case.AgentCaseWriter" + ) as mock_wcls, + ): + mock_cls.return_value.aextract = AsyncMock(return_value=[case]) + mock_wcls.return_value.append_entry = AsyncMock(return_value=_fake_eid()) + ctx = FakeStrategyContext() + + await extract_agent_case(_event(), ctx) + + emitted = [e for e in ctx.emitted if isinstance(e, AgentCaseExtracted)] + assert len(emitted) == 1 + event = emitted[0] + assert event.approach == "1. stop node 2. resync 3. verify" + assert event.key_insight == "watch oplog lag" + assert event.task_intent == "restore MongoDB replica" + assert event.quality_score == 0.75 + + async def test_omits_key_insight_section_when_empty( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/unit/test_memory/test_strategies/test_extract_agent_skill.py b/tests/unit/test_memory/test_strategies/test_extract_agent_skill.py index 9c4b941de..74d37dfc1 100644 --- a/tests/unit/test_memory/test_strategies/test_extract_agent_skill.py +++ b/tests/unit/test_memory/test_strategies/test_extract_agent_skill.py @@ -1,16 +1,23 @@ """Tests for :func:`extract_agent_skill`. -Mocked seams: ``cluster_repo`` (sqlite), ``agent_case_repo`` / -``agent_skill_repo`` (LanceDB), :class:`EmbeddingCapability` (component, -injected via :func:`_install_embedder`), ``AgentSkillExtractor`` (algo), -``AgentSkillWriter`` (md). Each retry-class exception (cluster missing -/ case-not-indexed) bubbles up so OME's ``max_retries`` machinery -catches the race instead of the strategy implementing its own backoff -loop. +Mocked seams: ``cluster_repo`` (sqlite), ``agent_case_repo`` (LanceDB, +supporting-cases lineage only), ``agent_skill_repo`` (LanceDB, relevance +ranking only — never an existence check), :class:`AgentSkillReader` / +:class:`AgentSkillWriter` (md — the source of truth for which skills exist), +:class:`EmbeddingCapability` (component, injected via +:func:`_install_embedder`), ``AgentSkillExtractor`` (algo). + +The target case is reconstructed straight from the ``SkillClusterUpdated`` +event payload (:func:`_to_algo_case_from_event`) — the strategy never probes +LanceDB for it, so the old case-not-yet-indexed retry path no longer exists. +Only the cluster-missing race (``_ClusterMissingError``) still bubbles up +for OME's ``max_retries`` machinery to catch. LanceDB repo behaviour itself (predicate isolation, cosine ranking, ``_distance`` stripping) lives under -``tests/unit/test_infra/test_lancedb/test_repos/``; strategy tests only +``tests/unit/test_infra/test_lancedb/test_repos/``; ``AgentSkillReader`` / +``AgentSkillWriter`` behaviour lives under +``tests/unit/test_infra/test_markdown/test_readers/``. Strategy tests only verify routing decisions and orchestration glue. """ @@ -19,6 +26,7 @@ import asyncio import datetime as _dt import importlib +from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch import numpy as np @@ -27,21 +35,21 @@ from everalgo.clustering import Cluster as AlgoCluster from everalgo.types import AgentSkill as AlgoAgentSkill -from everos.component.embedding import ( - EmbeddingCapability, - EmbeddingProvider, - EmbeddingServiceError, -) +from everos.component.embedding import EmbeddingCapability, EmbeddingProvider +from everos.core.persistence import MemoryRoot from everos.infra.ome.testing import FakeStrategyContext +from everos.infra.persistence.markdown import ( + AgentSkillFrontmatter, + AgentSkillWriter, +) from everos.memory._partition_locks import _reset_for_tests from everos.memory.events import SkillClusterUpdated from everos.memory.strategies.extract_agent_skill import ( MAX_SKILLS_IN_PROMPT, MAX_SUPPORTING_CASES, - _CaseNotYetIndexedError, _ClusterMissingError, _collect_supporting_entry_ids, - _resolve_query_vector, + _reap_renamed_skills, _select_existing_skills, _select_supporting_cases, extract_agent_skill, @@ -66,12 +74,12 @@ def _install_embedder( ) -> None: """Install ``embedder`` as the process-wide embedding capability. - ``extract_agent_skill`` and its helpers resolve the embedder via - ``get_embedding_capability().require()``, so this is the only - injection knob. The autouse fixture in ``tests/conftest.py`` seeds - ``Capability(provider=None)`` for hermeticity; this helper swaps in - a live provider for both the body-guard check and any subsequent - ``.require().embed()`` calls. + ``extract_agent_skill`` resolves the embedder via + ``get_embedding_capability().available`` for its body-guard only (the + query vector itself now travels on the event, so nothing here embeds on + the fly). The autouse fixture in ``tests/conftest.py`` seeds + ``Capability(provider=None)`` for hermeticity; this helper swaps in a + live provider so the guard passes. """ import everos.component.embedding.accessor as acc @@ -80,11 +88,7 @@ def _install_embedder( @pytest.fixture def embed_available(monkeypatch: pytest.MonkeyPatch) -> None: - """Convenience fixture: install a no-op stub embedder as the capability. - - For tests that only need to exercise the body past the guard and do - not care what vector is returned. - """ + """Convenience fixture: install a no-op stub embedder as the capability.""" _install_embedder(monkeypatch, _StubEmbedder()) @@ -98,11 +102,27 @@ def _event( cluster_id: str = "cl_xxxxxxxxxxx1", case_entry_id: str = "ac_20260517_0001", agent_id: str = "agent_42", + app_id: str = "default", + project_id: str = "default", + task_intent: str = "", + approach: str = "", + key_insight: str | None = None, + quality_score: float = 0.0, + case_timestamp_ms: int = 0, + case_vector: list[float] | None = None, ) -> SkillClusterUpdated: return SkillClusterUpdated( case_entry_id=case_entry_id, cluster_id=cluster_id, agent_id=agent_id, + app_id=app_id, + project_id=project_id, + task_intent=task_intent, + approach=approach, + key_insight=key_insight, + quality_score=quality_score, + case_timestamp_ms=case_timestamp_ms, + case_vector=case_vector, ) @@ -126,39 +146,56 @@ def _lance_case( *, quality_score: float = 0.8, timestamp: _dt.datetime | None = None, - vector: list[float] | None = None, - task_intent: str | None = None, ) -> MagicMock: - """Stand-in for a LanceDB AgentCase row (only fields the strategy reads).""" + """Stand-in for a LanceDB AgentCase row (supporting-cases lineage only).""" case = MagicMock() case.entry_id = entry_id case.timestamp = timestamp or _dt.datetime(2026, 5, 17, tzinfo=_dt.UTC) - case.task_intent = ( - task_intent if task_intent is not None else f"intent of {entry_id}" - ) + case.task_intent = f"intent of {entry_id}" case.approach = f"approach of {entry_id}" case.quality_score = quality_score case.key_insight = "" - case.vector = vector or [] return case -def _lance_skill( +def _frontmatter( + name: str, *, - name: str = "old_skill", - cluster_id: str = "cl_xxxxxxxxxxx1", + agent_id: str = "a", + cluster_id: str | None = "cl_x", source_case_ids: list[str] | None = None, -) -> MagicMock: - skill = MagicMock() - skill.id = f"agent_42_{name}" - skill.cluster_id = cluster_id - skill.name = name - skill.description = f"desc {name}" - skill.content = f"content {name}" - skill.confidence = 0.5 - skill.maturity_score = 0.5 - skill.source_case_ids = source_case_ids or [] - return skill + confidence: float = 0.5, + maturity_score: float = 0.5, +) -> AgentSkillFrontmatter: + return AgentSkillFrontmatter( + id=f"{agent_id}_{name}", + agent_id=agent_id, + name=name, + description=f"desc {name}", + confidence=confidence, + maturity_score=maturity_score, + source_case_ids=source_case_ids or [], + cluster_id=cluster_id, + ) + + +def _reader_stub(fms: list[AgentSkillFrontmatter]) -> MagicMock: + """Reader double: ``list_by_cluster`` returns each frontmatter paired + with a synthetic body — mirroring the real ``AgentSkillReader``, which + returns ``(frontmatter, body)`` pairs directly rather than requiring a + second, name-based read to hydrate ``content``.""" + reader = MagicMock() + reader.list_by_cluster = AsyncMock( + return_value=[(fm, f"body of {fm.name}") for fm in fms] + ) + return reader + + +def _lance_skill_row(name: str) -> MagicMock: + """Stand-in for a LanceDB AgentSkill ranking row (only ``.name`` is read).""" + row = MagicMock() + row.name = name + return row def _algo_skill(name: str = "summarise_doc") -> AlgoAgentSkill: @@ -174,7 +211,7 @@ def _algo_skill(name: str = "summarise_doc") -> AlgoAgentSkill: ) -# ── strategy meta + retry-class errors ─────────────────────────────────── +# ── strategy meta + cluster-missing retry ──────────────────────────────── async def test_strategy_meta_is_attached() -> None: @@ -209,10 +246,10 @@ async def test_returns_without_side_effects_when_embedding_unavailable() -> None mock_repo.get_with_members = AsyncMock( side_effect=AssertionError("cluster_repo must not be touched"), ) - mock_case_repo.find_by_owner_entry = AsyncMock( + mock_case_repo.find_by_owner_entries = AsyncMock( side_effect=AssertionError("agent_case_repo must not be touched"), ) - mock_skill_repo.count_in_cluster = AsyncMock( + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( side_effect=AssertionError("agent_skill_repo must not be touched"), ) @@ -238,38 +275,284 @@ async def test_raises_when_cluster_missing_for_retry(embed_available: None) -> N await extract_agent_skill(_event(), FakeStrategyContext()) -async def test_raises_when_target_case_not_yet_in_lancedb( +# ── target case + existing skills regression (event-read, md-first) ───── + + +async def test_reads_target_case_from_event_no_lancedb_probe( + monkeypatch: pytest.MonkeyPatch, + embed_available: None, +) -> None: + """Strategy body reads task_intent / approach / key_insight straight off + the SkillClusterUpdated event. It never calls + agent_case_repo.find_by_owner_entry for the target case — that probe was + the cascade-lag corruption path this PR rescues: under sustained + cascade lag the run died after ``max_retries`` and OME dead-lettered it, + so the fresh case was never distilled into a skill. + """ + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + probe_calls: list[str] = [] + + async def fake_find(*args: object, **kwargs: object) -> None: + probe_calls.append("find_by_owner_entry") + return None + + monkeypatch.setattr(mod.agent_case_repo, "find_by_owner_entry", fake_find) + + mock_reader = MagicMock() + mock_reader.list_by_cluster = AsyncMock(return_value=[]) + monkeypatch.setattr(mod, "_reader", mock_reader) + monkeypatch.setattr(mod, "_writer", None, raising=False) + + emitted = [_algo_skill(name="revive_replica")] + + with ( + patch( + "everos.memory.strategies.extract_agent_skill.cluster_repo" + ) as mock_cluster_repo, + patch( + "everos.memory.strategies.extract_agent_skill.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.extract_agent_skill.AgentSkillExtractor" + ) as mock_extractor_cls, + patch( + "everos.memory.strategies.extract_agent_skill.AgentSkillWriter" + ) as mock_writer_cls, + ): + mock_cluster_repo.get_with_members = AsyncMock(return_value=_algo_cluster()) + mock_extractor_cls.return_value.aextract = AsyncMock(return_value=emitted) + mock_writer_cls.return_value.write_main = AsyncMock(return_value=None) + + event = _event( + case_entry_id="c1", + cluster_id="cl1", + agent_id="a1", + task_intent="restore replica", + approach="stop, resync, verify", + key_insight="watch oplog", + quality_score=0.8, + case_timestamp_ms=1_700_000_000_000, + case_vector=[0.1] * 1024, + ) + await extract_agent_skill(event, FakeStrategyContext()) + + assert probe_calls == [] # LanceDB never touched for the target case + + extractor_call = mock_extractor_cls.return_value.aextract.call_args + target_arg = extractor_call.args[0] + assert target_arg.id == "c1" + assert target_arg.task_intent == "restore replica" + assert target_arg.approach == "stop, resync, verify" + assert target_arg.key_insight == "watch oplog" + assert target_arg.quality_score == 0.8 + assert target_arg.timestamp == 1_700_000_000_000 + + +async def test_existing_skills_come_from_md_even_when_lancedb_stale( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + embed_available: None, +) -> None: + """LanceDB reports 0 skills in the cluster, but md has skill_X — strategy + sees skill_X via the reader and feeds it to the extractor as an existing + skill. Regresses the silent-clobber bug where a stale index made the LLM + see no existing skill and emit ``add()`` for one that already existed. + """ + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + monkeypatch.setattr( + MemoryRoot, "resolve", classmethod(lambda cls: MemoryRoot(root=tmp_path)) + ) + monkeypatch.setattr(mod, "_writer", None, raising=False) + monkeypatch.setattr(mod, "_reader", None, raising=False) + + seed_writer = AgentSkillWriter(root=MemoryRoot(root=tmp_path)) + await seed_writer.write_main( + "a1", + "revive_replica", + frontmatter=_frontmatter( + "revive_replica", + agent_id="a1", + cluster_id="cl1", + source_case_ids=["c0"], + maturity_score=0.6, + ), + body="## Revive replica\nstop, resync, verify.", + ) + + captured: dict[str, list] = {} + + async def spy_aextract(target, *, existing_relevant_skills, supporting_cases): + captured["existing"] = list(existing_relevant_skills) + return [] + + with ( + patch( + "everos.memory.strategies.extract_agent_skill.cluster_repo" + ) as mock_cluster_repo, + patch( + "everos.memory.strategies.extract_agent_skill.agent_skill_repo" + ) as mock_skill_repo, + patch( + "everos.memory.strategies.extract_agent_skill.agent_case_repo" + ) as mock_case_repo, + patch( + "everos.memory.strategies.extract_agent_skill.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.extract_agent_skill.AgentSkillExtractor" + ) as mock_extractor_cls, + ): + mock_cluster_repo.get_with_members = AsyncMock( + return_value=_algo_cluster(cluster_id="cl1", members=["c0", "c1"]) + ) + # Simulate cascade lag: LanceDB's skill ranking index is stale/empty. + # Cluster is within MAX_SKILLS_IN_PROMPT, so this must never be hit. + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + side_effect=AssertionError( + "must not be reached: cluster is within MAX_SKILLS_IN_PROMPT" + ) + ) + mock_case_repo.find_by_owner_entries = AsyncMock(return_value=[]) + mock_extractor_cls.return_value.aextract = spy_aextract + + event = _event( + cluster_id="cl1", + agent_id="a1", + case_entry_id="c1", + case_vector=[0.1] * 1024, + ) + await extract_agent_skill(event, FakeStrategyContext()) + + assert len(captured["existing"]) == 1 + hydrated = captured["existing"][0] + assert hydrated.name == "revive_replica" + assert hydrated.source_case_ids == ["c0"] + assert hydrated.content != "" + assert "stop, resync, verify" in hydrated.content + + +async def test_existing_skills_reaches_llm_for_skill_whose_directory_has_a_space( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, embed_available: None, ) -> None: - """LanceDB has not yet indexed the freshly-written case — let OME retry.""" + """End-to-end regression guard for the ``list_by_cluster`` drop bug: a + ``skill_My Skill/`` directory written outside the writer (raw space, + never sanitized) must reach ``existing_relevant_skills`` with + non-empty ``content`` — not merely be enumerable by ``list_by_cluster`` + in isolation, but survive the full enumeration→hydration path this + strategy drives without a second, name-based read dropping it one + layer downstream. + + A prior fix made ``list_by_cluster`` enumerate this directory + successfully, but ``_hydrate_algo_skills`` then re-read each selected + skill by ``fm.name`` via ``read_main`` — which re-derives (and + re-sanitizes) a path from ``"My Skill"`` to ``skill_My_Skill/``, a + path that doesn't exist, and dropped the skill again. The fix removed + that second read entirely: ``list_by_cluster`` now hands back the + body it already read, so there is no name-based re-derivation left + anywhere on this path. + """ + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + monkeypatch.setattr( + MemoryRoot, "resolve", classmethod(lambda cls: MemoryRoot(root=tmp_path)) + ) + monkeypatch.setattr(mod, "_writer", None, raising=False) + monkeypatch.setattr(mod, "_reader", None, raising=False) + + skill_dir = ( + MemoryRoot(root=tmp_path).agents_dir() / "a1" / "skills" / "skill_My Skill" + ) + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\n" + "id: a1_My Skill\n" + "type: agent_skill\n" + "agent_id: a1\n" + "track: agent\n" + "name: My Skill\n" + "description: d\n" + "confidence: 0.5\n" + "maturity_score: 0.5\n" + "cluster_id: cl1\n" + "---\n" + "The real skill body.\n", + encoding="utf-8", + ) + + captured: dict[str, list] = {} + + async def spy_aextract(target, *, existing_relevant_skills, supporting_cases): + captured["existing"] = list(existing_relevant_skills) + return [] + with ( patch( "everos.memory.strategies.extract_agent_skill.cluster_repo" ) as mock_cluster_repo, + patch( + "everos.memory.strategies.extract_agent_skill.agent_skill_repo" + ) as mock_skill_repo, patch( "everos.memory.strategies.extract_agent_skill.agent_case_repo" ) as mock_case_repo, + patch( + "everos.memory.strategies.extract_agent_skill.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.extract_agent_skill.AgentSkillExtractor" + ) as mock_extractor_cls, ): - mock_cluster_repo.get_with_members = AsyncMock(return_value=_algo_cluster()) - mock_case_repo.find_by_owner_entry = AsyncMock(return_value=None) - with pytest.raises(_CaseNotYetIndexedError): - await extract_agent_skill(_event(), FakeStrategyContext()) + mock_cluster_repo.get_with_members = AsyncMock( + return_value=_algo_cluster(cluster_id="cl1", members=["c0", "c1"]) + ) + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + side_effect=AssertionError( + "must not be reached: cluster is within MAX_SKILLS_IN_PROMPT" + ) + ) + mock_case_repo.find_by_owner_entries = AsyncMock(return_value=[]) + mock_extractor_cls.return_value.aextract = spy_aextract + + event = _event( + cluster_id="cl1", + agent_id="a1", + case_entry_id="c1", + case_vector=[0.1] * 1024, + ) + await extract_agent_skill(event, FakeStrategyContext()) + + assert len(captured["existing"]) == 1 + hydrated = captured["existing"][0] + assert hydrated.name == "My Skill" + assert hydrated.content == "The real skill body." # ── end-to-end orchestration (mocked) ──────────────────────────────────── -@pytest.mark.asyncio async def test_extracts_and_persists_with_cluster_id_stamped( monkeypatch: pytest.MonkeyPatch, embed_available: None, ) -> None: - """End-to-end (mocked): extractor emits skills → writer stamps cluster_id.""" - target = _lance_case("ac_20260517_0001", vector=[0.1] * 1024) + """End-to-end (mocked): target reconstructed from event, existing skill + comes from md, extractor emits skills → writer stamps cluster_id.""" + existing_fm = _frontmatter( + "old_skill", + agent_id="agent_42", + cluster_id="cl_xxxxxxxxxxx1", + source_case_ids=["ac_20260517_0000"], + ) supporting = [_lance_case("ac_20260517_0000")] - existing = [_lance_skill(name="old_skill", source_case_ids=["ac_20260517_0000"])] emitted = [_algo_skill(name="summarise_doc"), _algo_skill(name="batch_then_synth")] + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + monkeypatch.setattr(mod, "_reader", _reader_stub([existing_fm])) + monkeypatch.setattr(mod, "_writer", None, raising=False) + with ( patch( "everos.memory.strategies.extract_agent_skill.cluster_repo" @@ -294,17 +577,22 @@ async def test_extracts_and_persists_with_cluster_id_stamped( mock_cluster_repo.get_with_members = AsyncMock( return_value=_algo_cluster(members=["ac_20260517_0000", "ac_20260517_0001"]) ) - mock_case_repo.find_by_owner_entry = AsyncMock(return_value=target) mock_case_repo.find_by_owner_entries = AsyncMock(return_value=supporting) - # Small cluster path: count ≤ K → scalar fetch returns existing. - mock_skill_repo.count_in_cluster = AsyncMock(return_value=len(existing)) - mock_skill_repo.find_in_cluster = AsyncMock(return_value=existing) + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + side_effect=AssertionError( + "must not rank: cluster within MAX_SKILLS_IN_PROMPT" + ) + ) mock_extractor_cls.return_value.aextract = AsyncMock(return_value=emitted) mock_writer_cls.return_value.write_main = AsyncMock(return_value=None) - mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") - monkeypatch.setattr(mod, "_writer", None, raising=False) - await extract_agent_skill(_event(), FakeStrategyContext()) + event = _event( + task_intent="intent of ac_20260517_0001", + approach="approach of ac_20260517_0001", + quality_score=0.8, + case_vector=[0.1] * 1024, + ) + await extract_agent_skill(event, FakeStrategyContext()) extractor_call = mock_extractor_cls.return_value.aextract.call_args target_arg = extractor_call.args[0] @@ -333,175 +621,136 @@ async def test_extracts_and_persists_with_cluster_id_stamped( # ── _select_existing_skills routing (cluster size × vector availability) ─ -async def test_select_existing_skills_small_cluster_uses_scalar_fetch() -> None: - """``total ≤ K`` short-circuits — no ranking needed for fully-inclusive set.""" - target = _lance_case("ac_001", vector=[0.5] * 1024) - skills = [_lance_skill(name=f"s{i}") for i in range(3)] +async def test_select_existing_skills_small_cluster_returns_all_md_skills( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``total ≤ K`` → every md skill is used; LanceDB is never asked to rank.""" + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + fms = [_frontmatter(f"s{i}") for i in range(3)] + monkeypatch.setattr(mod, "_reader", _reader_stub(fms)) with patch( "everos.memory.strategies.extract_agent_skill.agent_skill_repo" - ) as mock_repo: - mock_repo.count_in_cluster = AsyncMock(return_value=3) - mock_repo.find_in_cluster = AsyncMock(return_value=skills) - mock_repo.find_topk_relevant_in_cluster = AsyncMock() + ) as mock_skill_repo: + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock() got = await _select_existing_skills( - agent_id="a", cluster_id="cl_x", target=target + agent_id="a", + cluster_id="cl_x", + app_id="default", + project_id="default", + case_vector=[0.5] * 1024, ) - assert got == skills - mock_repo.find_topk_relevant_in_cluster.assert_not_awaited() - mock_repo.find_in_cluster.assert_awaited_once_with( - owner_id="a", cluster_id="cl_x", limit=MAX_SKILLS_IN_PROMPT - ) + assert [s.name for s in got] == ["s0", "s1", "s2"] + assert all(s.content == f"body of {s.name}" for s in got) + mock_skill_repo.find_topk_relevant_in_cluster.assert_not_awaited() -async def test_select_existing_skills_large_cluster_with_vector_uses_topk() -> None: - """``total > K`` and target carries vector → cosine top-K path.""" - target = _lance_case("ac_001", vector=[0.5] * 1024) - topk_skills = [_lance_skill(name=f"s{i}") for i in range(MAX_SKILLS_IN_PROMPT)] +async def test_select_existing_skills_large_cluster_with_vector_uses_lancedb_ranking( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``total > K`` + ``case_vector`` present → LanceDB ranks, md hydrates body.""" + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + n = MAX_SKILLS_IN_PROMPT + 5 + fms = [_frontmatter(f"s{i}") for i in range(n)] + monkeypatch.setattr(mod, "_reader", _reader_stub(fms)) + + ranked_names = [f"s{i}" for i in range(MAX_SKILLS_IN_PROMPT)] with patch( "everos.memory.strategies.extract_agent_skill.agent_skill_repo" - ) as mock_repo: - mock_repo.count_in_cluster = AsyncMock(return_value=MAX_SKILLS_IN_PROMPT + 5) - mock_repo.find_topk_relevant_in_cluster = AsyncMock(return_value=topk_skills) - mock_repo.find_in_cluster = AsyncMock() + ) as mock_skill_repo: + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + return_value=[_lance_skill_row(name) for name in ranked_names] + ) got = await _select_existing_skills( - agent_id="a", cluster_id="cl_x", target=target + agent_id="a", + cluster_id="cl_x", + app_id="default", + project_id="default", + case_vector=[0.5] * 1024, ) - assert got == topk_skills - mock_repo.find_in_cluster.assert_not_awaited() - call_kwargs = mock_repo.find_topk_relevant_in_cluster.await_args.kwargs + assert [s.name for s in got] == ranked_names + assert all(s.content == f"body of {s.name}" for s in got) + call_kwargs = mock_skill_repo.find_topk_relevant_in_cluster.await_args.kwargs assert call_kwargs["query_vector"] == [0.5] * 1024 assert call_kwargs["top_k"] == MAX_SKILLS_IN_PROMPT -async def test_select_existing_skills_large_cluster_recomputes_embedding( +async def test_select_existing_skills_large_cluster_no_vector_falls_back_to_md_order( monkeypatch: pytest.MonkeyPatch, ) -> None: - """``total > K`` but case has no vector → re-embed ``task_intent`` on the fly.""" - target = _lance_case("ac_001", vector=[], task_intent="how to summarise docs") - topk_skills = [_lance_skill(name=f"s{i}") for i in range(MAX_SKILLS_IN_PROMPT)] - fresh_vec = [0.42] * 1024 + """``total > K`` + no ``case_vector`` → md ordering, capped at K, logged.""" + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + n = MAX_SKILLS_IN_PROMPT + 5 + fms = [_frontmatter(f"s{i}") for i in range(n)] + monkeypatch.setattr(mod, "_reader", _reader_stub(fms)) - mock_embedder = MagicMock() - mock_embedder.embed = AsyncMock(return_value=fresh_vec) - _install_embedder(monkeypatch, mock_embedder) - - with patch( - "everos.memory.strategies.extract_agent_skill.agent_skill_repo" - ) as mock_repo: - mock_repo.count_in_cluster = AsyncMock(return_value=MAX_SKILLS_IN_PROMPT + 5) - mock_repo.find_topk_relevant_in_cluster = AsyncMock(return_value=topk_skills) - mock_repo.find_in_cluster = AsyncMock() + with ( + patch( + "everos.memory.strategies.extract_agent_skill.agent_skill_repo" + ) as mock_skill_repo, + structlog.testing.capture_logs() as captured, + ): + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + side_effect=AssertionError("must not rank without a case_vector") + ) got = await _select_existing_skills( - agent_id="a", cluster_id="cl_x", target=target + agent_id="a", + cluster_id="cl_x", + app_id="default", + project_id="default", + case_vector=None, ) - assert got == topk_skills - mock_embedder.embed.assert_awaited_once_with("how to summarise docs") - call_kwargs = mock_repo.find_topk_relevant_in_cluster.await_args.kwargs - assert call_kwargs["query_vector"] == fresh_vec + assert [s.name for s in got] == [f"s{i}" for i in range(MAX_SKILLS_IN_PROMPT)] + warned = [ + e + for e in captured + if e.get("event") == "agent_skill_topk_no_query_vector_md_fallback" + ] + assert len(warned) == 1 + assert warned[0]["md_count"] == n -async def test_select_existing_skills_falls_back_to_scalar_when_embed_fails( +async def test_select_existing_skills_appends_md_remainder_when_lancedb_stale( monkeypatch: pytest.MonkeyPatch, ) -> None: - """``total > K`` + no vector + embedder fails → scalar fetch capped at K.""" - target = _lance_case("ac_001", vector=[], task_intent="how to summarise docs") - scalar_skills = [_lance_skill(name=f"s{i}") for i in range(MAX_SKILLS_IN_PROMPT)] + """LanceDB ranking is itself cascade-lagged: it may return a name md no + longer has (skipped), and may omit md names it does have (appended as + filler) — neither may silently drop a skill from the prompt.""" + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") + n = MAX_SKILLS_IN_PROMPT + 2 + fms = [_frontmatter(f"s{i}") for i in range(n)] + monkeypatch.setattr(mod, "_reader", _reader_stub(fms)) - mock_embedder = MagicMock() - mock_embedder.embed = AsyncMock(side_effect=EmbeddingServiceError("provider down")) - _install_embedder(monkeypatch, mock_embedder) + # Ranked list omits "s0" (stale) and includes a ghost id md no longer has. + ranked_names = [f"s{i}" for i in range(1, MAX_SKILLS_IN_PROMPT)] + ["ghost_skill"] with patch( "everos.memory.strategies.extract_agent_skill.agent_skill_repo" - ) as mock_repo: - mock_repo.count_in_cluster = AsyncMock(return_value=MAX_SKILLS_IN_PROMPT + 5) - mock_repo.find_in_cluster = AsyncMock(return_value=scalar_skills) - mock_repo.find_topk_relevant_in_cluster = AsyncMock() + ) as mock_skill_repo: + mock_skill_repo.find_topk_relevant_in_cluster = AsyncMock( + return_value=[_lance_skill_row(name) for name in ranked_names] + ) got = await _select_existing_skills( - agent_id="a", cluster_id="cl_x", target=target + agent_id="a", + cluster_id="cl_x", + app_id="default", + project_id="default", + case_vector=[0.1] * 1024, ) - assert got == scalar_skills - mock_repo.find_topk_relevant_in_cluster.assert_not_awaited() - mock_repo.find_in_cluster.assert_awaited_once_with( - owner_id="a", cluster_id="cl_x", limit=MAX_SKILLS_IN_PROMPT - ) - - -# ── _resolve_query_vector layered fallback ─────────────────────────────── - - -async def test_resolve_query_vector_prefers_persisted_vector( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """When ``target.vector`` is set, reuse it; never touch the embedder.""" - target = _lance_case("ac_001", vector=[0.3] * 1024) - exploding_embedder = MagicMock() - exploding_embedder.embed = AsyncMock( - side_effect=AssertionError("must not embed when vector is present") - ) - _install_embedder(monkeypatch, exploding_embedder) - - got = await _resolve_query_vector(target) - - assert got == [0.3] * 1024 - exploding_embedder.embed.assert_not_awaited() - - -async def test_resolve_query_vector_returns_empty_when_no_text_either( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """No persisted vector + no task_intent → ``[]`` (no policy here).""" - target = _lance_case("ac_001", vector=[], task_intent="") - exploding_embedder = MagicMock() - exploding_embedder.embed = AsyncMock( - side_effect=AssertionError("must not embed when task_intent is empty") - ) - _install_embedder(monkeypatch, exploding_embedder) - - got = await _resolve_query_vector(target) - - assert got == [] - exploding_embedder.embed.assert_not_awaited() - - -async def test_resolve_query_vector_swallows_provider_not_configured() -> None: - """Missing embedder config is a deployment issue, not a strategy fault. - - The autouse ``_reset_embedding_capability_singleton`` fixture seeds the - accessor with ``Capability(provider=None)`` for hermeticity, so - ``.require()`` raises :class:`ProviderNotConfiguredError`; the helper - catches it and returns ``[]`` instead of propagating. - """ - target = _lance_case("ac_001", vector=[], task_intent="hello") - - got = await _resolve_query_vector(target) - - assert got == [] - - -async def test_resolve_query_vector_swallows_provider_service_error( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """A live embedder raising :class:`EmbeddingServiceError` degrades to ``[]``.""" - target = _lance_case("ac_001", vector=[], task_intent="hello") - mock_embedder = MagicMock() - mock_embedder.embed = AsyncMock(side_effect=EmbeddingServiceError("provider down")) - _install_embedder(monkeypatch, mock_embedder) - - got = await _resolve_query_vector(target) - - assert got == [] - mock_embedder.embed.assert_awaited_once_with("hello") + names = [s.name for s in got] + assert len(names) == MAX_SKILLS_IN_PROMPT + assert "ghost_skill" not in names # stale lance row silently skipped + assert names[:-1] == [f"s{i}" for i in range(1, MAX_SKILLS_IN_PROMPT)] + assert names[-1] == "s0" # dropped-by-lance md skill appended, not lost # ── _select_supporting_cases ranking + cap ─────────────────────────────── @@ -509,23 +758,16 @@ async def test_resolve_query_vector_swallows_provider_service_error( async def test_select_supporting_cases_ranks_by_quality_then_timestamp() -> None: """Hydrated cases sort ``(quality_score desc, timestamp desc)``.""" - skills = [ - _lance_skill(name="s1", source_case_ids=["ac_a", "ac_b", "ac_c"]), - ] + skills = [_algo_skill(name="s1")] + skills[0].source_case_ids = ["ac_a", "ac_b", "ac_c"] case_a = _lance_case( - "ac_a", - quality_score=0.4, - timestamp=_dt.datetime(2026, 5, 1, tzinfo=_dt.UTC), + "ac_a", quality_score=0.4, timestamp=_dt.datetime(2026, 5, 1, tzinfo=_dt.UTC) ) case_b = _lance_case( - "ac_b", - quality_score=0.9, - timestamp=_dt.datetime(2026, 5, 1, tzinfo=_dt.UTC), + "ac_b", quality_score=0.9, timestamp=_dt.datetime(2026, 5, 1, tzinfo=_dt.UTC) ) case_c = _lance_case( - "ac_c", - quality_score=0.9, - timestamp=_dt.datetime(2026, 5, 10, tzinfo=_dt.UTC), + "ac_c", quality_score=0.9, timestamp=_dt.datetime(2026, 5, 10, tzinfo=_dt.UTC) ) with patch( @@ -550,7 +792,8 @@ async def test_select_supporting_cases_ranks_by_quality_then_timestamp() -> None async def test_select_supporting_cases_caps_at_max_supporting() -> None: """Hydrated set is truncated to ``MAX_SUPPORTING_CASES``.""" ids = [f"ac_{i:03d}" for i in range(MAX_SUPPORTING_CASES + 3)] - skills = [_lance_skill(name="s1", source_case_ids=ids)] + skills = [_algo_skill(name="s1")] + skills[0].source_case_ids = ids hydrated = [ _lance_case(eid, quality_score=0.5 + 0.01 * i) for i, eid in enumerate(ids) ] @@ -572,7 +815,8 @@ async def test_select_supporting_cases_caps_at_max_supporting() -> None: async def test_select_supporting_cases_skips_repo_when_no_lineage_ids() -> None: """No usable source ids → ``[]`` without a repo round trip.""" - skills = [_lance_skill(name="s1", source_case_ids=[])] + skills = [_algo_skill(name="s1")] + skills[0].source_case_ids = [] with patch( "everos.memory.strategies.extract_agent_skill.agent_case_repo" ) as mock_case_repo: @@ -615,7 +859,9 @@ def test_collect_supporting_entry_ids_handles_empty_input() -> None: async def _run_serialisation_probe( - agent_id_run_a: str, agent_id_run_b: str + monkeypatch: pytest.MonkeyPatch, + agent_id_run_a: str, + agent_id_run_b: str, ) -> list[str]: """Drive two extract_agent_skill runs and record their critical-section order. @@ -623,6 +869,7 @@ async def _run_serialisation_probe( is a tiny ``asyncio.sleep`` masquerading as the LLM call. The returned log is the strict enter/leave sequence both runs go through. """ + mod = importlib.import_module("everos.memory.strategies.extract_agent_skill") log: list[str] = [] async def mock_aextract(case, **_kwargs): @@ -631,19 +878,15 @@ async def mock_aextract(case, **_kwargs): log.append(f"leave:{case.id}") return [] - target_a = _lance_case("ac_run_a", vector=[0.1] * 1024) - target_b = _lance_case("ac_run_b", vector=[0.1] * 1024) + mock_reader = MagicMock() + mock_reader.list_by_cluster = AsyncMock(return_value=[]) + monkeypatch.setattr(mod, "_reader", mock_reader) + monkeypatch.setattr(mod, "_writer", None, raising=False) with ( patch( "everos.memory.strategies.extract_agent_skill.cluster_repo" ) as mock_cluster_repo, - patch( - "everos.memory.strategies.extract_agent_skill.agent_case_repo" - ) as mock_case_repo, - patch( - "everos.memory.strategies.extract_agent_skill.agent_skill_repo" - ) as mock_skill_repo, patch( "everos.memory.strategies.extract_agent_skill.get_llm_client", return_value=object(), @@ -656,14 +899,6 @@ async def mock_aextract(case, **_kwargs): mock_cluster_repo.get_with_members = AsyncMock( return_value=_algo_cluster(members=["ac_run_a", "ac_run_b"]) ) - mock_case_repo.find_by_owner_entry = AsyncMock( - side_effect=lambda owner, entry, **_kw: ( - target_a if entry == "ac_run_a" else target_b - ) - ) - mock_case_repo.find_by_owner_entries = AsyncMock(return_value=[]) - mock_skill_repo.count_in_cluster = AsyncMock(return_value=0) - mock_skill_repo.find_in_cluster = AsyncMock(return_value=[]) mock_extractor_cls.return_value.aextract = mock_aextract await asyncio.gather( extract_agent_skill( @@ -679,10 +914,11 @@ async def mock_aextract(case, **_kwargs): async def test_partition_lock_serialises_runs_on_same_agent( + monkeypatch: pytest.MonkeyPatch, embed_available: None, ) -> None: """Two runs sharing ``agent_id`` must not overlap critical sections.""" - log = await _run_serialisation_probe("agent_42", "agent_42") + log = await _run_serialisation_probe(monkeypatch, "agent_42", "agent_42") assert log in ( ["enter:ac_run_a", "leave:ac_run_a", "enter:ac_run_b", "leave:ac_run_b"], ["enter:ac_run_b", "leave:ac_run_b", "enter:ac_run_a", "leave:ac_run_a"], @@ -690,9 +926,126 @@ async def test_partition_lock_serialises_runs_on_same_agent( async def test_partition_lock_lets_different_agents_run_in_parallel( + monkeypatch: pytest.MonkeyPatch, embed_available: None, ) -> None: """Runs on distinct ``agent_id`` must overlap (no false serialisation).""" - log = await _run_serialisation_probe("agent_42", "agent_43") + log = await _run_serialisation_probe(monkeypatch, "agent_42", "agent_43") assert log.index("enter:ac_run_a") < log.index("leave:ac_run_b") assert log.index("enter:ac_run_b") < log.index("leave:ac_run_a") + + +# ── rename reconciliation (orphan directories) ────────────────────────── + + +def _identified_algo_skill(skill_id: str, name: str) -> AlgoAgentSkill: + """Like :func:`_algo_skill` but with an explicit id — rename + reconciliation keys off identity, so these tests must control it.""" + return AlgoAgentSkill( + id=skill_id, + cluster_id="cl1", + name=name, + description="d", + content="body", + confidence=0.8, + maturity_score=0.5, + source_case_ids=["case_a"], + ) + + +async def _write_skill(writer: AgentSkillWriter, name: str) -> Path: + fm = AgentSkillFrontmatter( + id=f"agent_42_{name}", + agent_id="agent_42", + name=name, + description="d", + confidence=0.8, + maturity_score=0.5, + cluster_id="cl1", + ) + path = await writer.write_main("agent_42", name, frontmatter=fm, body="body") + return path.parent + + +async def test_reap_removes_the_directory_a_rename_left_behind( + tmp_path: Path, +) -> None: + """An update that renames a skill must not leave its old directory. + + everalgo's ``_apply_update`` keeps ``prior.id`` while changing the + name, so the emitted skill is written under a new directory and the + old one would survive carrying the same ``cluster_id``. That is not a + cosmetic leak: since this release the next extraction's + ``existing_relevant_skills`` come from the markdown enumeration, so the + orphan returns as a duplicate of a skill the LLM already renamed, + which is how ``add``-instead-of-``update`` full-replace clobbering gets + back in. Uses a real writer on a real tmp_path — the property under + test is that the directory is gone from the filesystem. + """ + writer = AgentSkillWriter(MemoryRoot(tmp_path)) + old_dir = await _write_skill(writer, "fix_django") + new_dir = await _write_skill(writer, "fix_django_autoreload") + assert old_dir.is_dir() and new_dir.is_dir() + + await _reap_renamed_skills( + writer, + {"agent_42_fix_django": "fix_django_autoreload"}, + existing_skills=[_identified_algo_skill("agent_42_fix_django", "fix_django")], + agent_id="agent_42", + app_id="default", + project_id="default", + ) + + assert not old_dir.exists() + assert (new_dir / "SKILL.md").is_file() + + +async def test_reap_keeps_a_prior_name_another_emitted_skill_claimed( + tmp_path: Path, +) -> None: + """Never delete a directory this same batch just wrote. + + With two ops in one extraction — rename ``a`` → ``b`` while a second + op writes ``a`` — reaping ``a`` by prior name would remove a file + written moments earlier in the same loop. The claimed-name guard is + what prevents the reap from undoing its own caller. + """ + writer = AgentSkillWriter(MemoryRoot(tmp_path)) + dir_a = await _write_skill(writer, "alpha") + await _write_skill(writer, "beta") + + await _reap_renamed_skills( + writer, + {"agent_42_alpha": "beta", "other_id": "alpha"}, + existing_skills=[_identified_algo_skill("agent_42_alpha", "alpha")], + agent_id="agent_42", + app_id="default", + project_id="default", + ) + + assert dir_a.is_dir() + + +async def test_reap_ignores_newly_added_skills(tmp_path: Path) -> None: + """A fresh ``add`` carries a uuid4 id absent from the enumerated set. + + Identity is the only thing that survives a rename — ``_apply_update`` + preserves ``prior.id`` while ``_apply_add`` mints a new one — so an + id that never appeared in ``existing_skills`` cannot be a rename, and + nothing may be deleted on its account. + """ + writer = AgentSkillWriter(MemoryRoot(tmp_path)) + kept = await _write_skill(writer, "existing_skill") + + await _reap_renamed_skills( + writer, + {"3f2a9c1e4b6d47f8a0c5e9b2d7143a6f": "brand_new_skill"}, + existing_skills=[ + _identified_algo_skill("agent_42_existing_skill", "existing_skill") + ], + agent_id="agent_42", + app_id="default", + project_id="default", + ) + + assert kept.is_dir() diff --git a/tests/unit/test_memory/test_strategies/test_extract_foresight.py b/tests/unit/test_memory/test_strategies/test_extract_foresight.py index 065064ac8..876f5982f 100644 --- a/tests/unit/test_memory/test_strategies/test_extract_foresight.py +++ b/tests/unit/test_memory/test_strategies/test_extract_foresight.py @@ -5,7 +5,15 @@ import pytest import structlog.testing -from everalgo.types import ChatMessage, Foresight, MemCell +from everalgo.types import ( + ChatMessage, + Foresight, + MemCell, + ToolCall, + ToolCallFunction, + ToolCallRequest, + ToolCallResult, +) from everos.infra.ome.testing import FakeStrategyContext from everos.memory.events import UserPipelineStarted @@ -229,3 +237,105 @@ async def test_skips_when_memcell_has_no_messages( assert matching, "log line should still fire (count=0)" assert matching[0]["count"] == 0 mock_wcls.return_value.append_entries.assert_not_called() + + +# ── mixed agent/user memcells (tool calls) ────────────────────────────── + + +def _tool_call_memcell(*, with_user_message: bool) -> MemCell: + """A memcell shaped the way an agent trajectory arrives. + + ``ToolCallRequest`` carries ``sender_id`` but no ``role``; + ``ToolCallResult`` carries neither. Only ``ChatMessage`` has ``role``, + which is why a bare ``m.role`` test raised on the first tool call. + """ + items: list[object] = [ + ToolCallRequest( + id="t1", + sender_id="agent", + timestamp=1_700_000_000_000, + tool_calls=[ + ToolCall( + id="c1", + function=ToolCallFunction(name="read_file", arguments="{}"), + ) + ], + ), + ToolCallResult( + id="t2", + timestamp=1_700_000_001_000, + tool_call_id="c1", + content="file contents", + ), + ] + if with_user_message: + items.insert( + 0, + ChatMessage( + id="m1", + role="user", + content="please fix the autoreloader", + timestamp=1_699_999_999_000, + sender_id="u_alice", + ), + ) + return MemCell(items=items, timestamp=1_700_000_000_000) + + +@pytest.mark.parametrize( + ("with_user_message", "expected_senders"), + [ + pytest.param(False, [], id="pure_agent_trajectory"), + pytest.param(True, ["u_alice"], id="mixed_user_and_tool_calls"), + ], +) +async def test_tool_calls_do_not_crash_the_sender_scan( + monkeypatch: pytest.MonkeyPatch, + with_user_message: bool, + expected_senders: list[str], +) -> None: + """A memcell holding tool calls must not raise, and must not over-extract. + + Regression guard: the scan used to read ``m.role`` off every item, so + the first ``ToolCallRequest`` raised ``AttributeError`` — before any + sender was resolved, before any LLM call. That made the strategy sound + on plain user chat and guaranteed to dead-letter on agent + trajectories, which is the shape ``/add`` receives from an agent + session. everalgo contracts for the mixed case + (``user_memory/_render.chat_messages``), so the fix is to honour that + contract rather than pre-filter by hand. + + Both directions are pinned: a pure agent trajectory extracts nothing + and never reaches the LLM, and a mixed memcell extracts for the human + senders only — an implementation that merely stopped raising but + scanned tool-call ``sender_id`` values would invent ``"agent"`` as a + user. + """ + event = UserPipelineStarted( + memcell_id="mc_tool", + session_id="s1", + memcell=_tool_call_memcell(with_user_message=with_user_message), + ) + monkeypatch.setattr(mod, "_writer", None, raising=False) + + with ( + patch( + "everos.memory.strategies.extract_foresight.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.extract_foresight.ForesightExtractor" + ) as mock_cls, + patch( + "everos.memory.strategies.extract_foresight.ForesightWriter" + ) as mock_wcls, + ): + mock_cls.return_value.aextract = AsyncMock(return_value=[]) + mock_wcls.return_value.append_entries = AsyncMock(return_value=[]) + await extract_foresight(event, FakeStrategyContext()) + + called_senders = [ + call.kwargs["sender_id"] + for call in mock_cls.return_value.aextract.await_args_list + ] + assert called_senders == expected_senders diff --git a/tests/unit/test_memory/test_strategies/test_trigger_skill_clustering.py b/tests/unit/test_memory/test_strategies/test_trigger_skill_clustering.py index 0e337962a..68cba96da 100644 --- a/tests/unit/test_memory/test_strategies/test_trigger_skill_clustering.py +++ b/tests/unit/test_memory/test_strategies/test_trigger_skill_clustering.py @@ -54,11 +54,15 @@ def _event( agent_id: str = "agent_42", task_intent: str = "summarise the doc", case_timestamp_ms: int = 1_700_000_001_000, + approach: str = "", + key_insight: str | None = None, ) -> AgentCaseExtracted: return AgentCaseExtracted( memcell_id="mc_a", case_entry_id=case_entry_id, task_intent=task_intent, + approach=approach, + key_insight=key_insight, quality_score=quality_score, case_timestamp_ms=case_timestamp_ms, agent_id=agent_id, @@ -170,6 +174,62 @@ async def test_creates_new_cluster_when_no_existing( assert emitted[0].agent_id == "agent_42" +async def test_emit_passes_through_case_body_and_vector( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """1.2.3+ trigger_skill_clustering passes case body verbatim and includes + the embedding it just computed so extract_agent_skill doesn't need to + re-embed for the top-k branch. + """ + fake_vector = [0.1] * 1024 + embedder = MagicMock() + embedder.embed = AsyncMock(return_value=fake_vector) + _install_embedder(monkeypatch, embedder) + ctx = FakeStrategyContext() + + incoming = _event( + task_intent="restore replica", + approach="stop, resync, verify", + key_insight="watch oplog", + quality_score=0.8, + case_timestamp_ms=1_700_000_000_000, + agent_id="a1", + case_entry_id="c1", + ) + + with ( + patch( + "everos.memory.strategies.trigger_skill_clustering.get_llm_client", + return_value=object(), + ), + patch( + "everos.memory.strategies.trigger_skill_clustering.cluster_repo" + ) as mock_repo, + patch( + "everos.memory.strategies.trigger_skill_clustering.cluster_by_llm", + new=AsyncMock(return_value=None), + ), + patch( + "everos.memory.strategies.trigger_skill_clustering.mint_cluster_id", + return_value="cl_newxxxx0001", + ), + ): + mock_repo.list_for_owner = AsyncMock(return_value=[]) + mock_repo.upsert_with_members = AsyncMock(return_value=None) + + await trigger_skill_clustering(incoming, ctx) + + emitted = [e for e in ctx.emitted if isinstance(e, SkillClusterUpdated)] + assert len(emitted) == 1 + ev = emitted[0] + assert ev.task_intent == "restore replica" + assert ev.approach == "stop, resync, verify" + assert ev.key_insight == "watch oplog" + assert ev.quality_score == 0.8 + assert ev.case_timestamp_ms == 1_700_000_000_000 + assert ev.case_vector == fake_vector + + async def test_merges_into_existing_cluster_when_algo_matches( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/uv.lock b/uv.lock index 4f4087108..ec09b1abb 100644 --- a/uv.lock +++ b/uv.lock @@ -562,7 +562,7 @@ wheels = [ [[package]] name = "everos" -version = "1.2.2" +version = "1.2.3" source = { editable = "." } dependencies = [ { name = "aiosqlite" },