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"):