Skip to content

Commit 85f0c50

Browse files
Kendrick-Songclaude
andcommitted
fix(md): return skill bodies from list_by_cluster; correct docs
Security review of 1b8cf11 + add842d found the list_by_cluster fix was incomplete: it stopped its own enumeration from dropping a skill whose directory suffix wasn't a sanitizer fixpoint, but the caller (_hydrate_algo_skills) still re-read each selected skill by fm.name via read_main, which re-derives (and re-sanitizes) a path from that name and drops it there instead. Reproduced on a real filesystem: skill_My Skill/ enumerates fine, but read_main("My Skill") re-derives to skill_My_Skill/ and misses. The drop moved one layer downstream; existing_relevant_skills was empty before and after the prior fix. Fix: list_by_cluster now returns (frontmatter, body) pairs instead of frontmatter alone, so the caller never needs a second, name-based read. _select_existing_skills / _rank_skills_by_relevance updated to carry (fm, body) tuples through selection; _hydrate_algo_skills is deleted — the body is already in hand, so there's nothing left for it to do. This closes the drop for real, removes the second disk read (the 2n-read concern carried since Task 4), and makes "the reader never derives a path" true end-to-end rather than true only for list_by_cluster's own enumeration step. New end-to-end regression test (test_select_existing_skills... / test_existing_skills_reaches_llm_for_skill_whose_directory_has_a_space) seeds a skill_My Skill/ directory directly on disk (bypassing the writer) and runs the real extract_agent_skill strategy against it, asserting the skill reaches existing_relevant_skills with non-empty content — the property the previous commit's test docstring claimed but the code didn't yet deliver. The reader-level regression test gained the same body assertion. Also, per review: - path_safety.py: corrected the NFC docstring claim, which was wrong for the ~1,082 Unicode composition-exclusion codepoints (e.g. Devanagari क़/ख़, U+0958/U+0959) — NFC decomposes an already-precomposed exclusion character instead of preserving it, so the combining mark is stripped either way. Scoped the claim to "best-effort for the common case", not a guarantee for every script. New test pins this directly. - Dropped the e2e test's unused _reset_rerank_capability_singleton fixture parameter: the reviewer adjudicated the earlier instruction conflict the other way — ordering is only meaningful between fixtures that touch the same state, and this fixture never reads or writes the rerank capability at all. - Widened the skill-name collision documentation (SkillPathMixin.sanitize_skill_name, CHANGELOG) beyond dropped-punctuation / space-collapse / truncation to the larger case: every combining mark is non-\w and is stripped regardless of script, so e.g. Devanagari "किताब" and "कताब" both collapse to "कतब" (same for Thai tone marks, Hebrew niqqud, Arabic harakat). - Corrected the collision justification: because _persist_skill sanitizes before frontmatter construction, the LLM sees the already-sanitized name in existing_relevant_skills, so a colliding raw name is an *affirmative* decision that two skills are different, not a probable intended update. The decision to accept collisions still stands, but on its real grounds: a disambiguating suffix would break the fm.name == directory-suffix identity the reader/writer seam relies on, and detecting-and-raising would reintroduce the dead-letter DoS. - Merged two consecutive "# -- Internals --" banners in agent_skill_reader.py into one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1aa6b9f commit 85f0c50

9 files changed

Lines changed: 267 additions & 146 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
6969
there is no legacy skill corpus whose directory names would change
7070
under the new sanitizer. Sanitizing is lossy: skills whose raw names
7171
differ only in characters the sanitizer drops or replaces (e.g.
72-
`"fix django"` vs. `"fix_django"`) now share one `SKILL.md`, and the
73-
later write wins — the earlier skill's `source_case_ids`,
74-
`maturity_score`, and body are silently lost, not merged. This is
75-
accepted, not mitigated: the LLM's add/update decision is keyed on the
76-
name it sees, so a collision usually reads as an intended update
77-
anyway.
72+
`"fix django"` vs. `"fix_django"`) now share one `SKILL.md`, and so do
73+
names differing only in a combining mark regardless of script (e.g.
74+
Devanagari `"किताब"` vs. `"कताब"` — a combining mark alone is not `\w`
75+
and is stripped either way; same for Thai tone marks, Hebrew niqqud,
76+
Arabic harakat). The later write wins — the earlier skill's
77+
`source_case_ids`, `maturity_score`, and body are silently lost, not
78+
merged. This is accepted, not mitigated, on two grounds: a
79+
disambiguating suffix would break the `name` ≡ directory-suffix
80+
identity the reader/writer relies on, and detecting a collision and
81+
raising would reintroduce the dead-letter DoS the sanitizer was built
82+
to avoid.
7883

7984
### Added
8085

‎src/everos/core/persistence/markdown/frontmatter.py‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -269,20 +269,34 @@ def sanitize_skill_name(cls, skill_name: str) -> str:
269269
name rather than merely idempotent-if-resanitized.
270270
271271
This is lossy: distinct raw names can collapse onto the same
272-
sanitized name (``"fix django"`` and ``"fix_django"`` both become
273-
``"fix_django"``; ``"fix!django"`` and ``"fixdjango"`` both become
274-
``"fixdjango"``; two names differing only past the 50-character cap
275-
also collide). Because ``AgentSkillWriter.write_main`` is a full-file
276-
replace and the LanceDB primary key is
277-
``f"{agent_id}_{sanitized_name}"``, a collision means the later skill
278-
silently overwrites the earlier one — its accumulated
279-
``source_case_ids``, ``maturity_score``, and body are lost, not
280-
merged. This is deliberate, not an oversight: the LLM's add/update
281-
decision for a skill is keyed on the name it sees, so a collision
282-
usually reads as an intended update anyway; and disambiguating
283-
colliding names (e.g. with a suffix) would break the
272+
sanitized name. Dropped punctuation, space/underscore collapse, and
273+
the 50-character cap are the visible cases (``"fix django"`` and
274+
``"fix_django"`` both become ``"fix_django"``; ``"fix!django"`` and
275+
``"fixdjango"`` both become ``"fixdjango"``). The larger case is
276+
every combining mark: a combining mark alone is not ``\\w``, so it
277+
is stripped regardless of script, and two names that differ only in
278+
their marks collide — e.g. Devanagari ``"किताब"`` and ``"कताब"``
279+
both sanitize to ``"कतब"``; the same holds for Thai tone marks,
280+
Hebrew niqqud, and Arabic harakat.
281+
282+
Because ``AgentSkillWriter.write_main`` is a full-file replace and
283+
the LanceDB primary key is ``f"{agent_id}_{sanitized_name}"``, a
284+
collision means the later skill silently overwrites the earlier
285+
one — its accumulated ``source_case_ids``, ``maturity_score``, and
286+
body are lost, not merged.
287+
288+
This is deliberate, not an oversight — but not because a collision
289+
"usually reads as an intended update". ``_persist_skill`` sanitizes
290+
*before* constructing the frontmatter, so the LLM is shown the
291+
already-sanitized name in ``existing_relevant_skills``; when it
292+
then emits a raw name like ``"fix django"`` after having just been
293+
shown ``"fix_django"``, it has affirmatively treated them as two
294+
different skills, and the write silently merges them anyway. The
295+
decision to accept this is justified on two other grounds instead:
296+
a disambiguating suffix on a collision would break the
284297
``frontmatter.name`` ≡ directory-suffix identity the reader/writer
285-
seam relies on.
298+
seam relies on, and detecting a collision and raising would
299+
reintroduce the dead-letter DoS this sanitizer was built to avoid.
286300
"""
287301
return sanitize_dirname(skill_name, fallback="unnamed")
288302

‎src/everos/core/persistence/markdown/path_safety.py‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,18 @@
3838
def sanitize_dirname(raw: str, fallback: str) -> str:
3939
"""Produce a safe directory/file name segment from free-text input.
4040
41-
* NFC-normalize first, so a decomposed (NFD) accented character (e.g. an
42-
``"e"`` + combining acute accent) collapses to its precomposed form
43-
before the character filter runs, rather than losing the accent
44-
entirely (a combining mark is not ``\\w``).
41+
* NFC-normalize first. For an ordinary decomposed (NFD) input — a base
42+
letter plus a combining mark, e.g. ``"e"`` + combining acute accent —
43+
this collapses to the precomposed form before the character filter
44+
runs, so the accent survives (a combining mark alone is not ``\\w``
45+
and would otherwise be silently stripped). This is best-effort, not a
46+
guarantee: for the ~1,082 Unicode *composition exclusion* codepoints
47+
(e.g. Devanagari ``क़``/``ख़``, U+0958/U+0959), NFC does the
48+
opposite — it *decomposes* an already-precomposed exclusion
49+
character, because recomposing it is explicitly excluded from the
50+
NFC algorithm, and the resulting combining mark is then stripped just
51+
the same. Normalizing here improves fidelity for the common case; it
52+
does not make every Unicode script round-trip losslessly.
4553
* Replace spaces with underscores.
4654
* Strip characters outside ``[a-zA-Z0-9_\\-.]`` (``\\w`` is Unicode-aware,
4755
so CJK and other non-ASCII scripts survive readably). Note that ``.``

‎src/everos/infra/persistence/markdown/readers/agent_skill_reader.py‎

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@
66
:class:`AgentSkillFrontmatter` subclass instance + the Tier-2 body, so
77
the caller never deals with raw dicts.
88
- :meth:`list_by_cluster` walks every ``skill_*/SKILL.md`` under an agent
9-
and returns the ones whose parsed ``cluster_id`` matches. This is the
10-
strong-consistency source of truth for cluster membership — LanceDB is
11-
cascade-lagged and must not be used for that check.
9+
and returns ``(frontmatter, body)`` for the ones whose parsed
10+
``cluster_id`` matches. This is the strong-consistency source of truth
11+
for cluster membership — LanceDB is cascade-lagged and must not be used
12+
for that check.
1213
- :meth:`read_reference` / :meth:`read_script` are plain text reads;
1314
no frontmatter, no schema.
1415
@@ -24,13 +25,18 @@
2425
directory segment. ``read_main`` / ``read_reference`` / ``read_script``
2526
take a caller-supplied ``skill_name`` and re-derive the path from it, so
2627
the reader and writer must never diverge on how a ``skill_name`` maps to
27-
a directory. ``list_by_cluster`` avoids that class of risk entirely for
28-
its own enumeration: it reads each globbed ``SKILL.md`` path directly
29-
rather than recovering a name from the directory and re-deriving a path
30-
from it — the reader never derives a path at all on that route, so a
31-
directory whose suffix happens not to be a sanitizer fixpoint (e.g. one
32-
containing a space) can never be silently dropped by a re-sanitization
33-
mismatch.
28+
a directory. ``list_by_cluster`` never derives a path at all: it reads
29+
each globbed ``SKILL.md`` path directly and hands back the body it
30+
already read, rather than recovering a name from the directory and
31+
leaving the caller to re-derive a path from that name for a second read.
32+
A caller that discarded the body and re-read by name would recreate
33+
exactly the re-sanitization risk this method exists to avoid — a
34+
directory whose suffix isn't itself a sanitizer fixpoint (e.g. one
35+
containing a raw space) would silently miss on that second, name-based
36+
read even though the first, path-based read found it just fine. Returning
37+
the body is what makes "the reader never derives a path" a property of
38+
the full ``list_by_cluster`` → caller flow, not just of the enumeration
39+
step in isolation.
3440
"""
3541

3642
from __future__ import annotations
@@ -88,11 +94,11 @@ async def list_by_cluster(
8894
*,
8995
app_id: str = "default",
9096
project_id: str = "default",
91-
) -> list[AgentSkillFrontmatter]:
97+
) -> list[tuple[AgentSkillFrontmatter, str]]:
9298
"""Enumerate this agent's ``SKILL.md`` files whose ``cluster_id`` matches.
9399
94100
Walks ``skills/skill_*/SKILL.md`` under the agent's memory root and
95-
returns fully-parsed frontmatter for each match. Skills whose
101+
returns ``(frontmatter, body)`` for each match. Skills whose
96102
frontmatter has ``cluster_id is None`` (or a different cluster) are
97103
filtered out.
98104
@@ -105,7 +111,11 @@ async def list_by_cluster(
105111
derives a path at all on this route, so this enumeration cannot drop
106112
a skill whose on-disk directory suffix is not itself a sanitizer
107113
fixpoint (e.g. one written with a raw, unsanitized name containing a
108-
space).
114+
space). Returning the body here (rather than frontmatter alone) is
115+
load-bearing, not a convenience: a caller that discarded it and
116+
re-read by ``frontmatter.name`` would reintroduce the same
117+
name-based re-derivation this method exists to avoid, one call
118+
later.
109119
110120
Args:
111121
agent_id: Owning agent.
@@ -114,7 +124,7 @@ async def list_by_cluster(
114124
project_id: Project scope; defaults to ``"default"``.
115125
116126
Returns:
117-
``AgentSkillFrontmatter`` list sorted by skill directory path.
127+
``(frontmatter, body)`` pairs sorted by skill directory path.
118128
Empty if the agent has no skill directory yet, or none match.
119129
"""
120130
skills_dir = self._skills_root(agent_id, app_id, project_id)
@@ -125,14 +135,14 @@ async def list_by_cluster(
125135
f"/{AgentSkillFrontmatter.SKILL_MAIN_FILENAME}"
126136
)
127137
paths = await anyio.to_thread.run_sync(lambda: sorted(skills_dir.glob(pattern)))
128-
matches: list[AgentSkillFrontmatter] = []
138+
matches: list[tuple[AgentSkillFrontmatter, str]] = []
129139
for path in paths:
130140
parsed = await self._read_path(path, schema=AgentSkillFrontmatter)
131141
if parsed is None:
132142
continue
133-
frontmatter, _body = parsed
143+
frontmatter, body = parsed
134144
if frontmatter.cluster_id == cluster_id:
135-
matches.append(frontmatter)
145+
matches.append((frontmatter, body))
136146
return matches
137147

138148
async def read_reference(
@@ -177,7 +187,7 @@ async def read_script(
177187
text = await apath.read_text(encoding="utf-8")
178188
return text.rstrip("\n")
179189

180-
# ── Internals ─────────────────────────────────────────────────────────
190+
# ── Internals — same shape as AgentSkillWriter ────────────────────────────
181191

182192
async def _read_path(self, path: Path, *, schema: type[T]) -> tuple[T, str] | None:
183193
"""Read + parse an already-resolved ``SKILL.md`` path.
@@ -193,8 +203,6 @@ async def _read_path(self, path: Path, *, schema: type[T]) -> tuple[T, str] | No
193203
body = parsed.body.rstrip("\n")
194204
return frontmatter, body
195205

196-
# ── Internals — same shape as AgentSkillWriter ────────────────────────────
197-
198206
def _skills_root(self, agent_id: str, app_id: str, project_id: str) -> Path:
199207
return (
200208
self._root.agents_dir(app_id, project_id)

‎src/everos/memory/strategies/extract_agent_skill.py‎

Lines changed: 23 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,15 @@ async def _select_existing_skills(
245245
count exceeds ``MAX_SKILLS_IN_PROMPT`` and ``case_vector`` is
246246
available; when it isn't (pre-1.2.3 event, or embedding was
247247
unavailable upstream), fall back to md ordering.
248+
249+
``list_by_cluster`` returns each skill's frontmatter *and* body
250+
together, so there is no second, name-based read to hydrate
251+
``content`` — a prior version re-read each selected skill by
252+
``fm.name`` via ``read_main``, which re-derives (and re-sanitizes) a
253+
path from that name and would silently miss a skill whose on-disk
254+
directory suffix isn't itself a sanitizer fixpoint, even though the
255+
first, path-based enumeration found it. See
256+
``AgentSkillReader.list_by_cluster``'s docstring.
248257
"""
249258
reader = _get_reader()
250259
md_skills = await reader.list_by_cluster(
@@ -254,9 +263,9 @@ async def _select_existing_skills(
254263
return []
255264

256265
if len(md_skills) <= MAX_SKILLS_IN_PROMPT:
257-
selected_fms = md_skills
266+
selected = md_skills
258267
elif case_vector is not None:
259-
selected_fms = await _rank_skills_by_relevance(
268+
selected = await _rank_skills_by_relevance(
260269
md_skills,
261270
agent_id=agent_id,
262271
cluster_id=cluster_id,
@@ -269,20 +278,18 @@ async def _select_existing_skills(
269278
cluster_id=cluster_id,
270279
md_count=len(md_skills),
271280
)
272-
selected_fms = md_skills[:MAX_SKILLS_IN_PROMPT]
281+
selected = md_skills[:MAX_SKILLS_IN_PROMPT]
273282

274-
return await _hydrate_algo_skills(
275-
selected_fms, agent_id=agent_id, app_id=app_id, project_id=project_id
276-
)
283+
return [_md_to_algo_skill(fm, body) for fm, body in selected]
277284

278285

279286
async def _rank_skills_by_relevance(
280-
md_skills: list[AgentSkillFrontmatter],
287+
md_skills: list[tuple[AgentSkillFrontmatter, str]],
281288
*,
282289
agent_id: str,
283290
cluster_id: str,
284291
case_vector: list[float],
285-
) -> list[AgentSkillFrontmatter]:
292+
) -> list[tuple[AgentSkillFrontmatter, str]]:
286293
"""Ask LanceDB to rank the md skills by cosine relevance, capped at K.
287294
288295
LanceDB is used purely as a ranking index here, never as the
@@ -291,59 +298,27 @@ async def _rank_skills_by_relevance(
291298
skill LanceDB didn't return (also stale index) is appended in
292299
arbitrary order so no skill is silently dropped from the prompt.
293300
"""
294-
md_by_name = {fm.name: fm for fm in md_skills}
301+
md_by_name = {fm.name: (fm, body) for fm, body in md_skills}
295302
ranked_lance = await agent_skill_repo.find_topk_relevant_in_cluster(
296303
owner_id=agent_id,
297304
cluster_id=cluster_id,
298305
query_vector=case_vector,
299306
top_k=MAX_SKILLS_IN_PROMPT,
300307
)
301-
selected: list[AgentSkillFrontmatter] = []
308+
selected: list[tuple[AgentSkillFrontmatter, str]] = []
302309
seen_names: set[str] = set()
303310
for lance_row in ranked_lance:
304-
fm = md_by_name.get(lance_row.name)
305-
if fm is not None and fm.name not in seen_names:
306-
selected.append(fm)
307-
seen_names.add(fm.name)
308-
for fm in md_skills:
311+
pair = md_by_name.get(lance_row.name)
312+
if pair is not None and pair[0].name not in seen_names:
313+
selected.append(pair)
314+
seen_names.add(pair[0].name)
315+
for fm, body in md_skills:
309316
if fm.name not in seen_names and len(selected) < MAX_SKILLS_IN_PROMPT:
310-
selected.append(fm)
317+
selected.append((fm, body))
311318
seen_names.add(fm.name)
312319
return selected
313320

314321

315-
async def _hydrate_algo_skills(
316-
frontmatters: list[AgentSkillFrontmatter],
317-
*,
318-
agent_id: str,
319-
app_id: str,
320-
project_id: str,
321-
) -> list[AlgoAgentSkill]:
322-
"""Re-read each selected skill's body and project it onto the algo type.
323-
324-
A second read (rather than reusing the frontmatter-only rows from
325-
``list_by_cluster``) is needed because ``content`` must carry the
326-
real skill body — see :func:`_md_to_algo_skill`. A skill deleted
327-
between the enumeration and this read is skipped rather than
328-
raising; the next run will simply not see it.
329-
"""
330-
reader = _get_reader()
331-
algo_skills: list[AlgoAgentSkill] = []
332-
for fm in frontmatters:
333-
result = await reader.read_main(
334-
agent_id,
335-
fm.name,
336-
schema=AgentSkillFrontmatter,
337-
app_id=app_id,
338-
project_id=project_id,
339-
)
340-
if result is None:
341-
continue
342-
fresh_fm, body = result
343-
algo_skills.append(_md_to_algo_skill(fresh_fm, body))
344-
return algo_skills
345-
346-
347322
async def _select_supporting_cases(
348323
skills: list[AlgoAgentSkill],
349324
*,

‎tests/e2e/test_add_flush_agent_pipeline_e2e.py‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@
7878
@pytest.fixture(autouse=True)
7979
def _opt_in_real_embedding(
8080
_reset_embedding_capability_singleton: None,
81-
_reset_rerank_capability_singleton: None,
8281
) -> Iterator[None]:
8382
"""Opt this module's test into a real embedding capability.
8483
@@ -93,15 +92,13 @@ def _opt_in_real_embedding(
9392
Scoped to this file only (not the global fixture) so every other
9493
test keeps its hermetic default.
9594
96-
Requesting both ``_reset_embedding_capability_singleton`` and
97-
``_reset_rerank_capability_singleton`` as parameters — rather than
98-
relying on collection/declaration order between this file's conftest
99-
chain and the root conftest — makes pytest's dependency graph
100-
guarantee this fixture's setup runs after both and its teardown before
101-
either. This fixture does not touch the rerank capability's value
102-
(see the module docstring for why); the rerank fixture is requested
103-
purely for that ordering guarantee, not because this fixture opts
104-
rerank in.
95+
Requesting ``_reset_embedding_capability_singleton`` as a parameter
96+
— rather than relying on collection/declaration order between this
97+
file's conftest chain and the root conftest — makes pytest's
98+
dependency graph guarantee this fixture's setup runs after it and its
99+
teardown before it. Rerank is left untouched (see the module
100+
docstring): this fixture reads and writes only the embedding
101+
capability, so there is nothing to order it against.
105102
106103
Setting ``_capability = None`` (rather than constructing a capability
107104
object directly) makes the accessor rebuild lazily from

‎tests/unit/test_core/test_persistence/test_markdown/test_path_safety.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,28 @@ def test_nfc_normalizes_decomposed_accents() -> None:
9393
assert sanitized_nfc == sanitized_nfd == "café"
9494

9595

96+
def test_nfc_does_not_help_composition_exclusions() -> None:
97+
"""Pins the documented exception: for Unicode "composition exclusion"
98+
codepoints, NFC normalization does not help — it decomposes an
99+
already-precomposed character, and the resulting combining mark is
100+
stripped either way.
101+
102+
U+0958 / U+0959 (Devanagari letters formed from a base letter + nukta)
103+
are composition exclusions: their canonical decomposition is excluded
104+
from NFC recomposition, so ``normalize("NFC", precomposed)`` yields the
105+
*decomposed* form, not the precomposed one.
106+
"""
107+
precomposed = "क़ख़"
108+
assert unicodedata.normalize("NFC", precomposed) != precomposed
109+
110+
sanitized = sanitize_dirname(precomposed, fallback="unnamed")
111+
112+
# The nukta (combining mark, U+093C) is lost: NFC decomposes the
113+
# precomposed input into base + nukta, and the nukta is then stripped
114+
# (not \w) -- the opposite of what NFC does for an ordinary NFD accent.
115+
assert sanitized == "कख"
116+
117+
96118
def test_cjk_and_space_input_preserved_readably() -> None:
97119
raw = "修复 Django 自动重载问题"
98120
sanitized = sanitize_dirname(raw, fallback="unnamed")

0 commit comments

Comments
 (0)