From 1fdedf2f6a904da57d3228ff71e65945bbe4b07a Mon Sep 17 00:00:00 2001 From: GUOGUO Date: Sat, 17 Oct 2026 21:17:09 +0800 Subject: [PATCH] fix: keep declared knowledge dependencies in bundle revisions Legacy frontmatter references and structured fields now feed bundle construction and reference resolution through the same effective metadata as sidecars. Case bundles retain their declared required resources and reject unsupported dependency declarations, and every declared packaged skill document must be present. Update evidence must be a list. Discovery failures are documented as whole-call errors. Existing item revisions are unchanged. Agent-assisted-by: Cursor Agent (Claude Opus) (implementer) Signed-off-by: GUOGUO Co-authored-by: Cursor --- docs/knowledge-operations.md | 14 ++++- src/omnirsi/knowledge_operations.py | 45 +++++++++++----- src/omnirsi/knowledge_store.py | 2 + src/omnirsi/skill_catalog.py | 15 ++++-- tests/test_knowledge_operations.py | 80 +++++++++++++++++++++++++++++ tests/test_knowledge_store.py | 2 + 6 files changed, 142 insertions(+), 16 deletions(-) diff --git a/docs/knowledge-operations.md b/docs/knowledge-operations.md index 649c0b0a..c554a330 100644 --- a/docs/knowledge-operations.md +++ b/docs/knowledge-operations.md @@ -34,6 +34,11 @@ returned item payloads, not the omission/control envelope. Each omission carries `reason` code (`unknown_identity`, `ambiguous_identity`, `invalid_identity`, `duplicate_identity`, `unavailable_revision`, `invalid`, `filters`, `query`, `output_limit`) and, for errors, a `detail` message. No-match is explicit. +Per-item omission applies after identity discovery: a checkout skill or case +record that cannot be parsed or validated during discovery (for example invalid +frontmatter or JSON, a name/directory mismatch, conflicting metadata, a missing +or duplicate case ID) fails the whole reader call explicitly, because +identities cannot be trusted. Deterministic identity order is not a learned relevance ranking. ## Identity and resources @@ -46,8 +51,15 @@ or installation rejects an old revision; it never substitutes new bytes. This reader does not itself retain history or claim standalone exports. - Local skills include their complete owner folder (except incidental - `__pycache__`, `*.pyc` and `.DS_Store`) and declared cross-skill references. Other links remain navigation in the required full checkout. + `__pycache__`, `*.pyc` and `.DS_Store`) and declared cross-skill references, + whether declared in the sidecar or in legacy frontmatter. Other links remain navigation in the required full checkout. - Local structured cases keep their own identity, observation and source limits. + A case bundle holds its record, content and any `required_resources` declared + relative to the record; missing, unsafe or escaping resources fail. Case + `references`/`dependencies` declarations are rejected rather than ignored; + external evidence stays a source pointer. +- Every declared `skill_documents` edition must be present in the retained + library; a missing target fails the read instead of leaving a dangling link. - Existing catalog records retain IDs/tiers/state. Bilingual records include the entire imported library as one conservative dependency closure: languages, manifests, licenses, helpers and transitive references are preserved. This is diff --git a/src/omnirsi/knowledge_operations.py b/src/omnirsi/knowledge_operations.py index 749819f8..97267bdd 100644 --- a/src/omnirsi/knowledge_operations.py +++ b/src/omnirsi/knowledge_operations.py @@ -271,20 +271,37 @@ def _checkout(self, identity: str) -> Bundle: if identity in cases: path = cases[identity] record = json.loads(path.read_text(encoding="utf-8")) + for field in ("references", "dependencies"): + if field in record: + raise ValueError(f"Case {field} are not supported; declare required_resources") + declared = record.get("required_resources", []) + if not isinstance(declared, list) or any(not isinstance(p, str) for p in declared): + raise ValueError("Case required_resources must be a string list") + if len(set(declared)) != len(declared): + raise ValueError("Case required_resources must not repeat a path") relative = safe_path(record["content"]) - content_path = path.parent / relative - if content_path.is_symlink() or not content_path.resolve().is_relative_to( - self.skill_root - ): - raise ValueError("Case content escapes the skills root") + required = [relative, "case.json", *(p for p in declared if p != relative)] + content = {"case.json": path.read_bytes()} + for resource in [relative, *required[2:]]: + safe_path(resource) + if resource == "case.json": + raise ValueError("Case resource collides with its case.json record") + resource_path = path.parent / resource + if resource_path.is_symlink() or not resource_path.resolve().is_relative_to( + self.skill_root + ): + raise ValueError(f"Case resource escapes the skills root: {resource}") + if not resource_path.is_file(): + raise ValueError(f"Missing case resource: {resource}") + content[resource] = resource_path.read_bytes() return Bundle( { **record, "entrypoint": relative, "sources": _sources(record["sources"]), - "required_resources": [relative, "case.json"], + "required_resources": required, }, - {relative: content_path.read_bytes(), "case.json": path.read_bytes()}, + content, {}, ) return self._skill(identity) @@ -301,6 +318,12 @@ def _knowledge(self, identity: str) -> Bundle: library, digest = self._skill_library() content.update(library) dependencies["data/skill_library"] = digest + documents = record["skill_documents"] + if not isinstance(documents, dict): + raise ValueError(f"skill_documents must map editions to paths: {identity}") + for target in documents.values(): + if not isinstance(target, str) or safe_path(target) not in library: + raise ValueError(f"Missing declared skill document: {target}") return Bundle( { "id": identity, @@ -331,14 +354,12 @@ def _knowledge(self, identity: str) -> Bundle: ) def _skill(self, identity: str) -> Bundle: - from .skill_catalog import get_skill, skill_reference + from .skill_catalog import get_skill, skill_metadata, skill_reference root = self.skill_root record = get_skill(root, identity) - directory = root / identity - content = _tree(directory, identity) - sidecar = directory / "metadata.json" - metadata = json.loads(sidecar.read_text(encoding="utf-8")) if sidecar.exists() else {} + content = _tree(root / identity, identity) + metadata = skill_metadata(root, identity) dependencies = {} for reference in metadata.get("references", []): path = skill_reference(root, identity, reference) diff --git a/src/omnirsi/knowledge_store.py b/src/omnirsi/knowledge_store.py index ce971a26..49de7848 100644 --- a/src/omnirsi/knowledge_store.py +++ b/src/omnirsi/knowledge_store.py @@ -91,6 +91,8 @@ def create(self, bundle: Bundle) -> str: def update(self, bundle: Bundle, *, expected: str, reason: str, evidence: list[dict]) -> str: if not expected or not reason.strip(): raise ValueError("Update requires expected old revision and reason") + if not isinstance(evidence, list): + raise ValueError("Update evidence must be a list; an empty list means none supplied") return self._publish(bundle, expected=expected, reason=reason, evidence=evidence) def _lineage(self, metadata: dict) -> None: diff --git a/src/omnirsi/skill_catalog.py b/src/omnirsi/skill_catalog.py index 39e3d691..e6787563 100644 --- a/src/omnirsi/skill_catalog.py +++ b/src/omnirsi/skill_catalog.py @@ -67,7 +67,7 @@ def _references(metadata: dict, name: str) -> list[str]: return references -def _metadata(directory: Path) -> dict: +def _effective(directory: Path) -> dict: # Read bytes line by line; parse only the frontmatter, never decode the body. with _entry(directory, "SKILL.md").open("rb") as stream: record = _frontmatter(stream) @@ -95,9 +95,19 @@ def _metadata(directory: Path) -> dict: or any(not isinstance(tag, str) for tag in record["tags"]) ): raise ValueError(f"Skill tags must be a string list: {directory.name}") + return record + + +def _metadata(directory: Path) -> dict: + record = _effective(directory) return {key: record[key] for key in ("name", "description", "status", "tags") if key in record} +def skill_metadata(root: Path, name: str) -> dict: + """Effective legacy frontmatter plus sidecar fields, after conflict checks.""" + return _effective(_directory(root, name)) + + def skill_records(root: Path) -> list[dict]: """Project metadata only; this projection is not an authoritative matrix.""" if not root.is_dir(): @@ -143,8 +153,7 @@ def skill_resource(root: Path, name: str, resource: str) -> Path: def skill_reference(root: Path, name: str, reference: str) -> Path: """Resolve a declared checkout reference without escaping the skills root.""" directory = _directory(root, name) - metadata = _sidecar(directory) - if reference not in _references(metadata or {}, directory.name): + if reference not in _references(_effective(directory), directory.name): raise ValueError(f"Undeclared skill reference: {reference}") if "\\" in reference or Path(reference).is_absolute(): raise ValueError("Reference must use a relative POSIX path") diff --git a/tests/test_knowledge_operations.py b/tests/test_knowledge_operations.py index a4262e63..a3bf3d0e 100644 --- a/tests/test_knowledge_operations.py +++ b/tests/test_knowledge_operations.py @@ -170,6 +170,86 @@ def test_qualified_identity_rules(tmp_path): assert package_filter["omissions"] == [{"id": "case.local-only", "reason": "filters"}] +def test_legacy_frontmatter_reference_is_a_dependency(tmp_path): + (tmp_path / "other").mkdir() + (tmp_path / "other" / "SKILL.md").write_text("---\nname: other\ndescription: t\n---\nB") + target = tmp_path / "other" / "notes.md" + target.write_text("first") + (tmp_path / "selected").mkdir() + (tmp_path / "selected" / "SKILL.md").write_text( + "---\nname: selected\ndescription: t\nreferences: [../other/notes.md]\n" + "applicability: {backend: any}\n---\nBody" + ) + reader = KnowledgeReader(skill_root=tmp_path) + original = reader.read("selected") + assert original.metadata["applicability"] == {"backend": "any"} + assert original.content["other/notes.md"] == b"first" + assert "other/notes.md" in original.dependencies + target.write_text("second") + assert reader.read("selected").revision != original.revision + target.unlink() + with pytest.raises(ValueError, match="Missing skill reference"): + reader.read("selected") + + +def _declare(root: Path, **fields) -> None: + path = root / "owner" / "cases" / "note.json" + record = json.loads(path.read_text(encoding="utf-8")) + path.write_text(json.dumps({**record, **fields}), encoding="utf-8") + + +def test_case_required_resources_are_retained_and_checked(tmp_path): + _case(tmp_path, "case.measured") + data = tmp_path / "owner" / "cases" / "measurements.csv" + data.write_text("a,1", encoding="utf-8") + plain = KnowledgeReader(skill_root=tmp_path).read("case.measured") + assert plain.metadata["required_resources"] == ["note.md", "case.json"] + _declare(tmp_path, required_resources=["measurements.csv"]) + reader = KnowledgeReader(skill_root=tmp_path) + original = reader.read("case.measured") + assert original.content["measurements.csv"] == b"a,1" + data.write_text("a,2", encoding="utf-8") + assert reader.read("case.measured").revision != original.revision + data.unlink() + with pytest.raises(ValueError, match="Missing case resource"): + reader.read("case.measured") + data.write_text("a,1", encoding="utf-8") + for fields, message in [ + ({"required_resources": ["../SKILL.md"]}, "Unsafe"), + ({"required_resources": "measurements.csv"}, "string list"), + ({"required_resources": ["measurements.csv"] * 2}, "repeat"), + ({"required_resources": ["case.json"]}, "collides"), + ({"required_resources": [], "references": ["x.md"]}, "not supported"), + ({"required_resources": [], "dependencies": {}}, "not supported"), + ]: + path = tmp_path / "owner" / "cases" / "note.json" + record = json.loads(path.read_text(encoding="utf-8")) + record.pop("references", None) + record.pop("dependencies", None) + path.write_text(json.dumps({**record, **fields}), encoding="utf-8") + with pytest.raises(ValueError, match=message): + KnowledgeReader(skill_root=tmp_path).read("case.measured") + + +def test_missing_declared_skill_document_is_explicit(): + reader = KnowledgeReader() + identity, record = next((i, r) for i, r in reader.records().items() if r.get("skill_documents")) + library, digest = reader._skill_library() + target = next(iter(record["skill_documents"].values())) + reader._library = ({k: v for k, v in library.items() if k != target}, digest) + with pytest.raises(ValueError, match="Missing declared skill document"): + reader.read(identity) + result = reader.collect(ids=[identity]) + assert result["omissions"][0]["reason"] == "invalid" + + +def test_malformed_checkout_discovery_fails_explicitly(tmp_path): + _case(tmp_path, "case.broken") + (tmp_path / "owner" / "cases" / "note.json").write_text("{", encoding="utf-8") + with pytest.raises(ValueError): + KnowledgeReader(skill_root=tmp_path).collect(ids=["owner"]) + + @pytest.mark.parametrize("path", ["../outside", "/absolute", "a\\b", "C:/outside", "a/../b"]) def test_unsafe_resource_paths_fail(path): with pytest.raises(ValueError, match="Unsafe"): diff --git a/tests/test_knowledge_store.py b/tests/test_knowledge_store.py index 9f4104e2..ecfd5c7b 100644 --- a/tests/test_knowledge_store.py +++ b/tests/test_knowledge_store.py @@ -47,6 +47,8 @@ def test_duplicate_stale_update_and_immutable_resources(tmp_path): with pytest.raises(RevisionConflict, match="Duplicate"): store.create(original) successor = changed(original, b"\nScoped interpretation amendment.\n") + with pytest.raises(ValueError, match="evidence must be a list"): + store.update(successor, expected=revision, reason="clarify", evidence="log.txt") next_revision = store.update(successor, expected=revision, reason="clarify", evidence=[]) assert next_revision != revision with pytest.raises(RevisionConflict, match="Stale"):