fix(memory): rescue skill extraction, disable foresight, tighten APIs

[Kendrick-Song]

Summary

Agent skill extraction had never once succeeded. extract_agent_skill read the freshly-written agent case from LanceDB, which the cascade daemon writes asynchronously; the Immediate trigger chain fires within ~200ms of the markdown write, so the read always missed. The code raised a retry-class _CaseNotYetIndexedError — but the OME retry loop had zero backoff, so all attempts burned in milliseconds and the run dead-lettered. Measured 4/4 failures against 1.2.2; the .skills/ directory was never created.

This PR moves the strategy's data sources off the eventually-consistent index: the target case now rides the event payload, and the cluster's existing skills come from markdown (strongly consistent). LanceDB is demoted to a pure relevance-ranking index. Three latent defects behind the first one are fixed in the same pass, plus a path-traversal hole found while verifying the fix on a live server.

extract_foresight turned out to be dead-lettering on every run for an unrelated reason, and is disabled by default here as a stop-gap — same symptom, different cause, so it is described in full below rather than folded into the narrative above.

Fixed

1. Agent skill extraction (P0). Target case travels on SkillClusterUpdated; _load_target_case and _CaseNotYetIndexedError are deleted. Existing cluster skills come from a new AgentSkillReader.list_by_cluster. No code path derives a skill path from a name any more.
2. A second, latent defect behind the first. Once (1) works, the read of existing cluster skills would hit the same lag → the LLM sees no existing skill → emits add instead of update → write_main full-replaces → accumulated source_case_ids / maturity_score / body silently lost. The markdown-first read closes this.
3. OME retries now back off (1s → 2s → 4s, capped at 10s, plus jitter). The old loop claimed to wait for eventually-consistent state and waited zero milliseconds. Generic — every retry-class error benefits.
4. POST /ome/trigger no longer masks strategy state. status gains not_dispatched; new dispatched and runs fields surface dead-lettered runs that previously had no HTTP surface at all.
5. Agentic search on agent memory no longer returns HTTP 500 when a skill has an empty description (a legal everalgo output — skill_ops.py:294 guards with an and), and now uses the kind-shaped rerank passage instead of the generic one, matching the HYBRID lane.
6. A single unparseable SKILL.md disabled skill extraction for its whole cluster. list_by_cluster propagated any frontmatter ValidationError, aborting the enumeration that feeds extract_agent_skill its existing skills — so one bad file dead-lettered that cluster's extraction on every subsequent run, the exact permanent-failure mode (1) exists to eliminate. The write path already pre-sanitized to avoid it; the read path was left open. The trigger surface is the whole schema, not just the traversal validator this PR adds — _read_path validates all of AgentSkillFrontmatter, so a field a later revision makes required would take out every existing file at once. Reproduced both ways on a real filesystem.
7. CWE-22 path traversal via LLM-generated skill names. skill.name is LLM output — its input is user conversation content, so prompt-injectable — and was concatenated straight into a filesystem path. Measured: "../" * 8 + "tmp/pwned" resolved outside the memory root. The repo already had a sanitizer from the knowledge-upload CWE-22 fix; it is now shared rather than duplicated, and applied at the single path-building point both the reader and writer derive from.

Changed

• extract_foresight now ships disabled (enabled=False), temporarily. It reads m.role off every memcell item, but only ChatMessage carries that attribute — ToolCallRequest has sender_id and no role, ToolCallResult has neither — so any memcell holding a tool call raises AttributeError before the first sender is resolved. That makes it correct on plain user chat and guaranteed to fail on agent trajectories, where it burns max_retries and dead-letters every time, on output nothing downstream consumes today. The decorator default is what changed, not default_ome.toml: everos init skips an existing ~/.everos/ome.toml (init_cmd.py:85), so a template edit would have reached new installs only. The opt-in is left working on purpose, since a chat-only deployment does get correct foresights. Stop-gap, not the fix — the real change is per-episode extraction (as atomic_fact does) instead of per-memcell, which needs an everalgo entry point that does not exist yet.

Upgrade notes

• extract_foresight stops running unless you opt in. If your deployment ingests plain user chat only, restore it in ome.toml (hot-reloaded, no restart):

  [strategies.extract_foresight]
  enabled = true

• If your client matches TriggerResponse.status exhaustively (Python Literal, TypeScript union), add a not_dispatched branch.
• AgentSkillFrontmatter.name and the agent_skill LanceDB primary key now hold the sanitized name, not the raw LLM output. Sanitizing is lossy, so names differing only in stripped or replaced characters now share one SKILL.md, last write winning — see SkillPathMixin.sanitize_skill_name for why that is accepted rather than mitigated.
• OfflineEngine.trigger_manual returns tuple[BaseEvent, list[tuple[StrategyMeta, str]]] instead of None.

Worth knowing for reviewers

• The retry backoff in (3) was originally motivated by _CaseNotYetIndexedError — which (1) then deleted. Its only remaining consumer is _ClusterMissingError, a same-transaction SQLite race that resolves in milliseconds, so the 1s/2s/4s defaults are oversized for the one caller left. Kept because the next retry-class error will need them, but the defaults were tuned for a case that no longer exists.
• The no-migration argument holds for skills only, not for the whole sanitization change. For agent skills the reason is the defect this PR fixes: extraction never succeeded, so .skills/ was never created and there is no legacy corpus. Knowledge upload, which shares the sanitizer, does have a corpus — and two inputs now resolve elsewhere than the directory already on disk: a decomposed (NFD) topic keeps its combining marks ("Résumé" no longer degrades to "Resume", since the shared helper NFC-normalizes first), and a topic or category of exactly . / .. falls back instead of resolving onto the parent (that one is the fix, not a regression). Precomposed input, CJK included, is byte-identical before and after — the character class is unchanged from the private copy it replaced, which was already [^\w\-.] with re.UNICODE. Practical exposure is small (category_id is one of 20 fixed ASCII categories; LLM-emitted topics are normally precomposed) but it is not zero, and the earlier framing implied it was.
• SkillClusterUpdated now persists a 1024-dim vector, growing run_record. Measured: ~0.8 KB → ~14 KB per record, so ~14 MB instead of ~0.8 MB for this strategy at the default max_records_per_strategy = 1000. Operators sizing ome.db need that number. Not trimmed here because it is not a local change: crash recovery replays event_payload to rebuild the event (engine.py:749, crash_recovery.py:63), so a trimmed persisted copy would send the recovered run down the md-ordering branch while the original took the ranked branch — a silent divergence. The vector is only read when a cluster exceeds MAX_SKILLS_IN_PROMPT, so it usually rides along unused. Follow-up filed.
• ome.toml overrides are not applied by the time engine.start() returns, which the foresight change surfaced rather than introduced. ConfigReloader.start() fires its initial load as a task (config_reloader.py:227-229), so an event emitted in that window is judged against the coded defaults and dropped by the enabled gate, with no redelivery. Harmless for a server (real events arrive much later), but the one integration test that opts foresight back in had to wait for the override to reach the registry before emitting — the reason is in the test, not just in this description. Pre-existing OME behavior; out of scope here.
• The radius plumbing an earlier commit added to the agentic path was reverted before merge. radius is cosine-scale but ahybrid_retrieve applies min_score to RRF-fused scores (max ≈ 0.0328), so the default radius=0.5 emptied every agentic agent search. RankInput.radius is never read anywhere in everalgo either, so radius was already inert on every lane — restoring "inert" is the patch-appropriate outcome. Tracked as a follow-up.

Area

• Architecture method
• Benchmark
• Use case
• Documentation
• Developer experience
• CI, build, or release

Verification

make ci  — green on the final tree
  ruff check + format        clean
  import-linter              3/3 contracts kept
  repo hygiene               assets / file-size / deprecated-names / github-docs / datetime / openapi-drift
  unit                       1984 passed
  integration                182 passed, 7 deselected
  package                    sdist + wheel build, install + import smoke (everos 1.2.3)

Live end-to-end verification (real LLM + real embedder, one SWE-bench django trajectory
through HTTP /add + /flush, OME + cascade drained):

  OME run_record
    extract_agent_case         success  1
    trigger_skill_clustering   success  1
    extract_agent_skill        success  1     <- one attempt, no retries, no dead-letter
    extract_atomic_facts       success  1
    extract_user_profile       success  1
    trigger_profile_clustering success  1

  On disk
    agents/agent_django/skills/skill_修复_Django_自动重载模块路径问题/SKILL.md

Same run pre-sanitization produced `skill_修复 Django 自动重载问题` (raw space), confirming
the sanitizer is active on the real write path.

Security fixes verified independently (path resolution only, nothing written):
  sanitize_dirname('../')                                   -> 'unnamed'   (was '..')
  AgentSkillFrontmatter(name=sanitize_skill_name('../'))     -> constructs  (was ValidationError)
  Path('/root/knowledge')/sanitize_dirname('../','Others')   -> stays under /root/knowledge
  NFD 'café'                                                 -> 'café'      (was 'cafe')

Sanitizer swept exhaustively over all 1,114,112 Unicode codepoints: NFC introduces no new
degenerate or dangerous result, and truncation cannot manufacture '.' or '..'.

Every new regression test verified red against the code it guards:
  list_by_cluster skip (both parametrized cases)  -> ValidationError propagates without it
  knowledge NFD + dot-topic fallback              -> fail against the 1.2.2 sanitizer

extract_foresight, same live run, before being disabled:
  extract_foresight          failed 2 + dead_letter 1
    AttributeError: 'ToolCallRequest' object has no attribute 'role'

Review history: 8 per-task reviews, one whole-branch review, one live verification pass,
three scoped security reviews, and one post-green review round. Each found something the
previous had passed — the whole-branch review caught a scale mismatch no per-task review
could see, the live run caught a test that structurally could not cover the code it claimed
to, the security reviews caught two "fixes" that had not closed the hole they targeted, and
the last round caught the read-path counterpart to a write-path fix (finding 6) plus three
docstring claims that did not match behavior. The foresight change then arrived separately
and turned up the engine.start() config window noted above.

One reviewer finding was verified and then not acted on as reported: moving this route's
OfflineEngine import under TYPE_CHECKING was described as saving ~750ms of app startup.
The import is genuinely type-only and the move is correct (an eager import there contradicted
the deferred _get_engine import a few lines below), but it saves nothing today —
service.memorize:37 imports the engine eagerly to construct it, so measurement after the
change still shows apscheduler loaded on any app import. The change is kept for consistency;
the cost claim is not.

Checklist

• I kept the change scoped to the relevant area.
• I am opening this from a separate branch, not pushing directly to main.
• I updated docs, examples, or setup notes when behavior changed.
• I added or updated tests when the change affects behavior.
• I did not commit secrets, .env files, dependency folders, or generated output.

🤖 Generated with Claude Code

[gloryfromca]

Adversarial review. Scope note: production code is only ~+900/−216 across 21 files (tests +2150, docs/CHANGELOG/openapi +665), but two of those files are engine-level and affect every strategy, not just the agent-skill chain.

The write-back loop is where I'd focus. This PR is the first one under which extract_agent_skill actually succeeds — .skills/ was never created before — so defects in _persist_skill that were previously unreachable become reachable on merge. Two of them are, I believe, live.

---

Blocking — implementation

1. An update that renames a skill leaves an orphan directory, and the orphan feeds back into the next run

everalgo's _apply_update treats a name change as a first-class update (skill_ops.py):

name_changed = bool(new_name) and new_name != (prior.name or "")
...
eff_name = new_name if new_name else (prior.name or "")

_persist_skill is write-only:

sanitized_name = AgentSkillFrontmatter.sanitize_skill_name(skill.name)
frontmatter = AgentSkillFrontmatter(id=f"{agent_id}_{sanitized_name}", ..., name=sanitized_name, ...)
await writer.write_main(agent_id, sanitized_name, ...)

So when the LLM renames fix_django → fix_django_autoreload, a new skill_fix_django_autoreload/ is written and skill_fix_django/ stays on disk with the same cluster_id. Consequences compound:

1. The next run's list_by_cluster returns both, so the LLM is shown its own pre-rename duplicate.
2. Cascade indexes md → two LanceDB rows for one logical skill.
3. Each rename adds another orphan; nothing ever reaps them.

Note this interacts directly with the PR's central change. Before, existing skills were read from LanceDB; now the md enumeration is the input, so an orphan directory doesn't just sit there — it pollutes the next extraction's prompt, which is a new source of exactly the "LLM emits add for something that already exists" failure this PR set out to eliminate.

The information needed to fix it is already present and is being discarded. _apply_update returns prior.model_copy(...), which preserves prior.id, while _apply_add mints uuid.uuid4().hex. _md_to_algo_skill passes id=fm.id (i.e. f"{agent_id}_{old_sanitized_name}") in, so a renaming update comes back carrying the old identity — and _persist_skill overwrites it with f"{agent_id}_{sanitized_name}" before it can be used.

Minimal fix: compare skill.id against f"{agent_id}_{sanitized_name}"; when they differ and skill.id matches an entry from list_by_cluster, delete the old skill directory after the new one is written. (Or, if renames aren't wanted at all, pin eff_name by passing the prior name through — but that decision belongs to whoever owns the prompt contract.)

2. retire is a no-op on the EverOS side

AgentSkillExtractor.aextract returns a flat list[AgentSkill] with no op discriminator. The retire branch of _apply_update returns an ordinary skill whose only marker is confidence < retire_confidence (default 0.1; extract_agent_skill doesn't override it). Step 6 writes everything back indiscriminately:

for skill in emitted_skills:
    await _persist_skill(writer, skill, ...)

There is no delete, no tombstone, no removal from retrieval. A retired skill just gets a lower confidence, stays in md, stays in the next prompt, and stays searchable. The module docstring's step 4 says "→ add / update / retire skill operations", but retire isn't implemented — and as of this PR it's reachable for the first time.

At minimum this needs an explicit branch (confidence < retire_confidence → remove the directory, or stamp a retired: true field that list_by_cluster filters on) or an honest "not implemented" note in the docstring plus a follow-up issue. Silently persisting retirements as normal skills is the worst of the three options.

---

Blocking — engine

3. The new backoff sleeps while holding the engine semaphore, and none of it is tunable in production

Runner.run acquires engine_sem around the whole retry chain — its own docstring says so ("Holds engine_sem for the full retry chain so concurrency cap applies end-to-end") — and await self._sleep_backoff(attempt) is now inside that block.

With max_concurrent_runs = 20 and extract_agent_skill at max_retries=3, one persistently failing run now occupies a concurrency slot for 1 + 2 + 4 ≈ 7s of pure sleep, where it previously occupied it for milliseconds. Under a sweeping failure (LLM provider wobble, embedder outage) all 20 slots can be asleep simultaneously.

The part that makes this hard to live with: production builds OMEConfig with only jobstore_path and config_path (service/memorize.py:167), so retry_backoff_* and max_concurrent_runs are all code-only. The CHANGELOG is honest about this ("not currently exposed via everos.toml or ome.toml"), but it means operators have no lever when the backoff misbehaves.

Backpressure during an outage is arguably desirable, so I'm not asking for the backoff to go — but it should either release the semaphore across the sleep (and say explicitly that the end-to-end cap semantics change), or expose the three knobs through Settings. Right now the PR description characterises the defaults only as "oversized for the one caller left"; it doesn't mention they hold a concurrency slot.

4. extract_foresight is disabled globally, but a one-line change fixes it — and everalgo already supports mixed memcells

The crash is EverOS-side, on one line:

sender_ids = sorted({m.sender_id for m in memcell.items if m.role == "user"})

everalgo explicitly contracts for the mixed case:

• ForesightExtractor class docstring: "Non-ChatMessage items in memcell.items are silently skipped (agent → user-memory contract)."
• _render.chat_messages() filters isinstance(item, ChatMessage); both _resolve_user_name and _render_conversation go through it.
• memcell.items[0].timestamp is safe too — ToolCallRequest carries timestamp.

Switch that comprehension to isinstance(m, ChatMessage) and: a pure agent trajectory yields an empty sender_ids → clean no-op with no LLM call; a mixed memcell extracts from the user messages only, which is precisely the contract everalgo documents.

The PR's stated reason for disabling is that "the real change is per-episode extraction, which needs an everalgo entry point that does not exist yet" — but that's about extraction granularity, not about the crash. Turning a feature off for every deployment to avoid a one-line defensive change is a lopsided trade. It's also the weakest part of the case for 1.2.3: additive response fields are fine in a patch, a feature disappearing is less so. Fixing it the short way makes the version question go away too.

---

Non-blocking

• The stale-index clobber (fix #2) is only closed for clusters ≤ 10. Above MAX_SKILLS_IN_PROMPT, _rank_skills_by_relevance orders md candidates by a LanceDB top-10. The md backfill is guarded by len(selected) < MAX_SKILLS_IN_PROMPT, so if all 10 LanceDB rows hit md, it never runs — and the skill missing from a lagging index is exactly last run's freshly written one, i.e. the one most likely to need update. It gets squeezed out of the prompt → add → full-replace clobber. test_select_existing_skills_appends_md_remainder_when_lancedb_stale covers the under-10 case, not this one. Suggest backfilling by md updated_at descending rather than path order, and reserving a slot or two for md skills LanceDB didn't return. Worth stating in the PR description that fix #2 is bounded by cluster size.

• reference_name / script_filename are not sanitized. _reference_path does / f"{reference_name}.md" and _script_path does / script_filename, both appended after _skill_dir — so skill_dir_name doesn't protect them. There are currently zero callers in src/, so this isn't reachable today, but they're public methods and the PR describes the sanitizer as living at "the single path-building point both the reader and writer derive from", which isn't true for these two segments. Whoever wires up progressive disclosure next will walk straight into it.

• The embedding body-guard is now stale. extract_agent_skill no longer embeds anything (case_vector arrives on the event), yet still returns early on not get_embedding_capability().available. Harmless in practice — trigger_skill_clustering gates on the same capability, so no SkillClusterUpdated exists without an embedder — but the comment's "defensive degradation" rationale no longer holds and will mislead.

• list_by_cluster is a per-agent full scan. It globs and parses every SKILL.md for the agent across all clusters, then filters by cluster_id, all inside the partition lock. Fine at today's volumes, but there's no ceiling and no log making the cost observable.

• Empty-passage ValueError is narrowed, not closed. _to_everalgo_doc_metadata now sets episode unconditionally; if both source fields are empty the passage is "" and _format_docs raises. The skill side is safe (sanitized name falls back to "unnamed"), the case side less so if task_intent is ever empty. An or "(empty)" closes it.

• Docstring placement. path_safety.py is 83 lines of which ~60 are prose; sanitize_skill_name's docstring runs 60+ lines on a collision trade-off; one CHANGELOG entry is 60 lines. The content is high quality but it's design rationale, which belongs in docs/ or an ADR — in a docstring it drifts from the code and nobody reads it on IDE hover.

---

What holds up well

The tests are real, which is not something I say often:

• The e2e assertion went from assert len(pytest_skills) >= 0 to a genuine floor, and asserts all_skill_runs is non-empty before asserting no dead-letters — explicitly closing the "strategy never ran, so the dead-letter check passes vacuously" trap.
• The backoff tests assert on actual sleep arguments (sleeps == [1.0, 2.0, 3.0, 3.0, 3.0, 3.0] for the cap, jittered ranges for the exponent), not merely that sleep was called.
• The integration test re-enables foresight through the real ome.toml opt-in path, covering the opt-in itself rather than routing around the new default.
• The only place behaviour is switched off in tests is retry_backoff_base_seconds=0.0 in the OME harness, and dedicated unit tests cover backoff separately. That's an acceptable split.

The _read_path distinction — reading an already-resolved path versus re-deriving one from a name — and having list_by_cluster return the body so callers can't re-derive a path one call later, is the sharpest structural idea in the PR and worth keeping exactly as written.

[gloryfromca]

Two smaller points that didn't make it into the review above.

A cheap mitigation exists for the case-insensitive-filesystem shadowing. The sanitize_skill_name docstring analyses this precisely and I agree with the conclusion that a disambiguating suffix needs a deliberate design pass rather than being bolted on here. But "accept it entirely" isn't the only alternative to "fix it properly": a casefolded existence probe on the skill directory before write_main, logging a warning on a hit, costs one Path.exists() and no design decisions. That turns the failure from silent data loss into observable data loss, which matters because the split state it produces — the LanceDB index advertising a name whose SKILL.md content was overwritten by a case-variant — is otherwise invisible until someone notices a search hit resolving to the wrong skill. Worth noting that macOS APFS and Windows NTFS defaults are both affected, so for an open-source project this is the developer's machine, not an exotic deployment.

OfflineEngine.trigger_manual's signature change is a breaking change to a public engine API. None → tuple[BaseEvent, list[tuple[StrategyMeta, str]]] is correctly called out in the upgrade notes, and OME is internal enough that I don't think it needs to gate the release. Flagging it only because it's the second contract change riding in a patch version alongside the status enum widening — if extract_foresight ends up staying disabled rather than getting the one-line fix, the three together make a stronger case for 1.3.0 than any one of them does alone.

[Kendrick-Song]

Thank you — this found real defects, and the framing on (1) in particular is what made it land: the issue isn't that a rename leaks a directory, it's that this PR is what makes the leak reachable and harmful, because the enumeration it introduces is now the input to the next run. I'd have filed that as cleanup without your second paragraph.

I reproduced all four blocking items and all six non-blocking ones before acting. Six are fixed, one is fixed with a different remedy than proposed, one is declined with reasoning below.

---

Blocking

1. Rename orphan — fixed (dbb4be0)

Confirmed end to end: _apply_update preserves prior.id (skill_ops.py:524) while _apply_add mints uuid.uuid4().hex (:318); _md_to_algo_skill passes id=fm.id; _persist_skill overwrote it with f"{agent_id}_{sanitized_name}".

Took your minimal fix. _reap_renamed_skills runs after the write loop and keys on skill.id — the id-vs-uuid asymmetry you pointed at is exactly what makes "is this a rename" decidable without heuristics, so that is what identifies the target rather than a name comparison.

Two things beyond the sketch:

• AgentSkillWriter.delete_skill fails closed. A directory it cannot resolve by the writer's own path rule is left alone rather than located by anything looser. An orphan is a bounded cost; deleting the wrong directory is not, and md is the source of truth here.
• A prior name another emitted skill just claimed is never deleted. With two ops in one batch (rename a→b while a second op writes a), the naive reap removes a file written moments earlier in the same loop. Pinned by a test.

Three tests on a real filesystem, all verified red against a no-op reap.

2. retire is a no-op — documented, not implemented

Confirmed: skill_ops.py:430-450 returns an ordinary skill whose only marker is the lowered confidence, and step 6 writes everything back. grep -n "retire\|delete\|remove\|unlink" over the strategy returns nothing, and AgentSkillWriter had no delete method at all before this PR.

Took your third option, deliberately. The two implementations are not equivalent in kind:

• Delete the directory hands an LLM-produced confidence score the authority to destroy the source of truth in an md-first system. delete_skill now exists, so this is cheap — which is exactly why it shouldn't be done as a side effect of a bug-fix PR.
• A retired flag only works if the enumeration, cascade, and search all learn to filter on it. Filtering just the prompt leaves retired skills searchable, which is arguably worse than today because the state becomes inconsistent between the two readers.

So the module docstring now states it is unimplemented and why the choice is deferred, rather than listing three ops when two are handled. Agreed that silently persisting retirements as normal skills is the worst of the three — that part is what the docstring change is for.

3. Backoff holds the semaphore — fixed (af596fa), but not by either remedy you offered

The numbers check out (max_concurrent_runs = 20, extract_agent_skill max_retries=3, and retry_backoff_* / max_concurrent_runs are both absent from Settings — production passes only jobstore_path and config_path).

I'd initially classified this Minor and filed it as #396. That was wrong, but for a different reason than "operators have no lever". The real problem is amplification:

Before this PR a failing run burned its retries in milliseconds and released the slot. With backoff it holds a slot for ~7s of sleep. When enough slots are asleep, strategies that would have succeeded are starved — a partial outage becomes a total stall.

"Backpressure during an outage is desirable" holds for the failing work. It does not hold for everything sharing the semaphore with it.

That's why I didn't take either branch of your or: exposing the knobs doesn't remove the amplification, it delegates it to an operator who now has to reason about max_concurrent_runs × backoff interaction during an incident. Releasing the slot removes it:

for attempt in range(max_retries_snapshot + 1):
    if attempt > 0:
        await self._sleep_backoff(attempt)   # outside the semaphore
        current_run_id = uuid4().hex
    async with self._sem:
        terminated = await self._run_one_attempt(...)

The cap exists to bound concurrent strategy work — LLM calls, embeddings, storage IO — and a sleeping coroutine consumes none of it, so this is arguably what it should always have meant. The semantics change is deliberate and stated in the docstring and CHANGELOG: the cap still applies to execution, no longer to waiting.

Guard uses a single-permit semaphore so locked() is unambiguous, and asserts a second waiter actually acquires — locked() alone passes on an implementation that frees the slot but leaves waiters unable to take it. Verified red against the previous structure.

Config exposure is a separate question and stays open.

4. extract_foresight — crash fixed (ff9d2fe), strategy stays disabled (4602979)

You're right about the crash, and the evidence is stronger than what you cited. Beyond the _render.chat_messages contract, everalgo's own profile code writes the exact expression you proposed:

# everalgo/user_memory/profile.py:259
{m.sender_id for cell in memcells for m in chat_messages(cell) if m.role == "user"}

And across EverOS:

$ grep -rn "m.role\|\.role ==" src/everos/memory/strategies/*.py
src/everos/memory/strategies/extract_foresight.py:78

One hand-rolled filter in the whole tree, and it was hand-rolled wrong. Fixed with isinstance(m, ChatMessage); a pure agent trajectory now yields no senders and never reaches the LLM. The guard pins both directions — an implementation that merely stopped raising but scanned tool-call sender_id values would invent "agent" as a user.

You were also right that my stated reason was wrong: I had conflated extraction granularity (per-memcell vs per-episode, which does need an everalgo entry point) with the crash (one line). The docstring, default_ome.toml, and CHANGELOG all said so and are corrected.

The strategy still ships disabled, on a different basis: nothing in EverOS reads foresights today — no search route surfaces them, no prompt slot consumes them — so it is one LLM call per sender per memcell producing write-only data. That is a product call, made by the maintainer, not a workaround for the defect. It will be re-enabled when something consumes the output. Your point about a feature disappearing in a patch release stands and is the cost we're accepting knowingly; fixing the crash is what makes the documented ome.toml opt-in actually usable in the meantime rather than a trap.

---

Non-blocking

Stale-index clobber bounded by cluster size — stated, not fixed. Confirmed: the backfill is guarded by len(selected) < MAX_SKILLS_IN_PROMPT, so with 10 LanceDB rows all mapping to md it never runs, and the omitted skill is by definition the most recently written one. Now stated explicitly in the CHANGELOG (window: cluster > 10 and an index that hasn't caught up; consequence is the pre-existing full-replace, not a new failure mode). Not fixed here because reserving slots or ordering by updated_at changes ranking behaviour, which wants its own change and its own test.

reference_name / script_filename unsanitized — fixed (dbb4be0). Both now route through sanitize_dirname, on the reader as well as the writer. Sanitizing one side only would have been worse than not sanitizing: the write lands on the safe path while the read looks at the raw one and reports the file missing. Parametrized over traversal, .., empty, and embedded separators, plus a round-trip test pinning that both sides resolve identically. You're right that the PR's "single path-building point both reader and writer derive from" was inaccurate for these two segments.

Stale embedding body-guard — fixed (dbb4be0). The comment no longer claims to protect a local embed call. The guard stays, on the accurate rationale: it keeps the whole agent-skill track consistent with the tier the deployment is running, so a direct emit degrades cleanly instead of producing skills the ranking half of the pipeline cannot serve.

Empty passage ValueError — fixed (dbb4be0). Falls back to a placeholder. One malformed row shouldn't 500 a result set it merely happens to appear in.

list_by_cluster full scan — acknowledged, not changed. The cost is real and the missing ceiling is a fair criticism. But md being the sole authority for cluster membership is the deliberate design of this PR; reintroducing an index for the existence check is precisely the eventual-consistency dependency it removes. Observability for it belongs with the other follow-ups.

Docstring placement — declining, with a repo-convention citation. .claude/rules/module-docstring.md asks for exactly this:

The load-bearing invariants — the rules a reader must know to change it safely […] Prefer prose that would save the next engineer a debugging session over boilerplate.

The collision trade-off, why the sanitizer must reject . / .., and why list_by_cluster must return bodies are invariants whose violation reintroduces the bugs this PR fixes — not background material. And this review is evidence they work where they are: your blocking (1) argument quotes _persist_skill's docstring back at me. If the convention should change, that's worth its own discussion — I don't think this PR should be the place it's overturned unilaterally.

One concession: I agree the volume is at the edge. If a specific one reads as narrative rather than invariant, name it and I'll cut it.

---

Also in this push

#392 merged to main while this was in review. Since 1.2.3 ships that code, its entries have been folded from [Unreleased] into [1.2.3] section by section and the date moved to the actual release day — leaving them under [Unreleased] would have had the release notes disclaim work the release contains. Full suite re-run on the merged tree: 2020 unit, 182 integration.

Every new guard was verified red against the code it protects:

engine_sem release        -> held_during_sleep[0] == True under the old structure
rename reap               -> skill_fix_django/ survives when the reap is a no-op
foresight tool calls      -> AttributeError: 'ToolCallRequest' object has no attribute 'role'
reference/script sanitize -> paths escape the skill dir without it