diff --git a/docs/skill-catalog-ledger.org b/docs/skill-catalog-ledger.org new file mode 100644 index 0000000..061d339 --- /dev/null +++ b/docs/skill-catalog-ledger.org @@ -0,0 +1,33 @@ +#+TITLE: Skill catalog candidate filtering (issue 65) +#+DATE: 2026-10-30 + +* 1. Task +Deliver and verify existing local commit c9dafdb on GitHub main. Base 52cc04f40f3858a5cf7d8063f009a4f10db6a07c has the same mirror.py bytes as that commit's parent; do not rewrite the parser or absorb issue 57's consuming-host checks. + +* 2. Existing machinery +- =sucoder/mirror.py:7399=, MirrorManager._parse_skill_catalog :: Collects file links and bare bullet candidates, deduplicates normalized paths. Its sole direct caller is _render_skill_catalog at line 7389; rendered catalog entries feed agent preludes. +- =sucoder/mirror.py:7276=, _normalize_skill_file_path and line 7307 _normalize_catalog_path :: Shared normalizers may fall back to a sibling skill/catalog. Preserve shared behavior and filter inappropriate candidates locally. +- =tests/test_mirror.py= :: Existing catalog expansion and malformed-frontmatter tests; c9dafdb adds the reported bogus-catalog regression. + +Search tier: rg/direct source and git comparison. Upstream GitNexus impact was attempted but libssl.so.3 prevents native loading. Manual risk: moderate, bounded to catalog entries in agent preludes; no sync, allocation or worker-state changes. No automated graph risk is claimed. + +* 3. Definitions +Bare bullet prose is not an explicit skill reference. Existing file links retain their semantics. A catalog must never list itself as a skill; normalized paths deduplicate entries, and skipped explicit references remain diagnosable through warnings. + +* 4. Invariants +Retain valid explicit file links, existing bare paths, extension fallback, nested skills and metadata labels. Preserve original issue reproduction. No new configuration, shared-normalizer changes or consuming-host existence policy. + +* 5. Reuse decision +Reuse the existing c9dafdb implementation and regression rather than create a second parser fix. Independent review demonstrated an extension-fallback gap in its bare-candidate check: nested/SKILL.md was discarded when nested/SKILL.org existed, whereas the existing normalizer accepts that counterpart. Match the candidate check to the normalizer's path.stem plus .org/.md, with a parametric nested-entry regression. Shared normalizers remain unchanged. Verify both the original baseline failure and this compatibility counterfactual before delivery. + +* 6. Open questions and validation +GitHub integration remains outstanding until the focused PR is reviewed/merged; deployment is separate. Local implementation c9dafdb was applied as accbe11 with identical source/test bytes and original author metadata. Its new regression passed (1), all catalog tests passed (3), and all mirror tests passed (263), importing this issue65 worktree. The same new regression on unchanged main failed, rendering the bogus SKILLS and missing-path entries. Initial verification was retained in tool outputs, not durable log files. The separate disposable baseline worktree was removed. + +Independent review demonstrated the extension fallback incompatibility above with an isolated filesystem probe. The correction uses path.stem instead of path.name for the two counterpart candidates. On accbe11 before correction, the new parametric regression had *2 failed, 1 passed*: both cross-extension references were lost, while the extensionless control already passed. After correction, *3 passed*; the complete mirror module had *266 passed* in 20.97 seconds. Independent source re-review confirms the original finding resolved and no additional concrete issue in that correction; reviewer did not run pytest. Ledger verification: OK anchored on sections 2–5, with the section 4 contradiction corrected through reuse of the existing normalizer's semantics. No shared-normalizer redesign or consuming-host validation was introduced. + +Durable private compatibility evidence: =/global/home/users/ligon/.sucoder/reviews/issue65/compatibility-RL9Z7R/=, containing =accbe11-counterfactual.log=, =patched-targeted.log=, =patched-mirror.log=, =import-path.txt= and =detect-changes.log=. Import receipts point to this branch's checkout. Commands were run from the SuCoder worktree root: +#+begin_src sh +python -m pytest -q -o showlocals=false --tb=short tests/test_mirror.py -k skill_catalog_bare_entries_preserve_extension_fallback +python -m pytest -q -o showlocals=false --tb=short tests/test_mirror.py +#+end_src +The available pytest interpreter supplied the dependencies; no shared package installation changed. GitNexus impact/change detection were attempted before edits/commits, but its native library cannot load libssl.so.3. Manual caller/diff review confines production scope to _parse_skill_catalog. The original checkout remains clean. This delivery has targeted and mirror-module coverage; no new full-suite/native-worker acceptance is claimed for the parser change. CI is checked on the published PR. diff --git a/sucoder/mirror.py b/sucoder/mirror.py index 2e55b0e..4126861 100644 --- a/sucoder/mirror.py +++ b/sucoder/mirror.py @@ -7399,23 +7399,45 @@ def _render_skill_catalog(self, catalog: Path, seen: set[Path]) -> Optional[str] def _parse_skill_catalog(self, catalog: Path, content: str) -> List[Path]: references: List[Path] = [] seen: set[Path] = set() + skipped: set[Path] = set() + own = catalog.resolve() - def add_reference(raw: str) -> None: + def target_path(raw: str) -> Optional[Path]: target = raw.strip() if not target: - return + return None target = target.strip('<>"\'') target = target.split("::", 1)[0] - path = ( + return ( Path(target).expanduser() if target.startswith("~") or target.startswith("/") else catalog.parent / target ) + + def add_reference(raw: str, *, bare: bool = False) -> None: + path = target_path(raw) + if path is None: + return + # A bare "- word" bullet is an entry only when it names something + # on disk; otherwise it is prose ("- LSMS_Library: lives in ..."). + if bare and not any( + candidate.exists() + for candidate in (path, path.parent / f"{path.stem}.org", path.parent / f"{path.stem}.md") + ): + return normalized = ( self._normalize_skill_file_path(path) or self._normalize_catalog_path(path) or path ) + if normalized.resolve() == own: + # The normalizers fall back to a sibling SKILLS file, so a path + # that names nothing here lands on this catalog, which would + # then list itself as a skill. + if path not in skipped: + skipped.add(path) + self.logger.warning("Skill catalog %s: entry %s names no skill; skipped.", catalog, path) + return if normalized not in seen: seen.add(normalized) references.append(normalized) @@ -7432,7 +7454,7 @@ def add_reference(raw: str) -> None: elif stripped.startswith("- "): candidate = stripped[2:].split()[0] if candidate and not candidate.startswith("file:") and "://" not in candidate: - add_reference(candidate) + add_reference(candidate, bare=True) return references diff --git a/tests/test_mirror.py b/tests/test_mirror.py index f51f3e6..c1dcd63 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -6543,6 +6543,59 @@ def fake_run_agent(args, **kwargs): assert not line.startswith("- SKILL"), line +def test_skill_catalog_lists_only_entries_that_name_skills( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Prose bullets and a prose ``file:`` mention used to render as a bogus + "SKILLS" entry: a path naming nothing fell back to the catalog itself.""" + manager = build_manager(tmp_path) + catalog_dir = tmp_path / "skills_catalog" + for name in ("good", "bare"): + (catalog_dir / name).mkdir(parents=True) + (catalog_dir / name / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: A {name} skill.\n---\nBody.\n", encoding="utf-8" + ) + catalog = catalog_dir / "SKILLS.md" + catalog.write_text( + "# Catalog\n\n" + "- file:good/SKILL.md — explicit entry.\n" + "- bare/SKILL.md — bare path that exists.\n" + "- LSMS_Library: `add-wave` lives with its project.\n" + "- missing/SKILL.md — bare path naming nothing.\n" + "List new skills with a `file:` link.\n", + encoding="utf-8", + ) + + with caplog.at_level(logging.WARNING, logger=manager.logger.name): + rendered = manager._render_skill_catalog(catalog, set()) + + entries = [line for line in rendered.splitlines() if line.startswith("- ")] + assert [line.split(" — ")[0] for line in entries] == ["- good", "- bare"] + # The prose file: mention is reported once instead of listed as a skill. + assert len([r for r in caplog.records if "names no skill" in r.getMessage()]) == 1 + + +@pytest.mark.parametrize( + ("reference", "actual_name"), + [ + ("nested/SKILL.md", "SKILL.org"), + ("nested/SKILL.org", "SKILL.md"), + ("nested/SKILL", "SKILL.org"), + ], +) +def test_skill_catalog_bare_entries_preserve_extension_fallback( + tmp_path: Path, reference: str, actual_name: str +) -> None: + manager = build_manager(tmp_path) + catalog = tmp_path / "skills_catalog" / "SKILLS.md" + skill = catalog.parent / "nested" / actual_name + skill.parent.mkdir(parents=True) + skill.write_text("A nested skill.\n", encoding="utf-8") + catalog.write_text(f"- {reference}\n", encoding="utf-8") + + assert manager._parse_skill_catalog(catalog, catalog.read_text()) == [skill.resolve()] + + # ---------------------------------------------------------------------- # Issue 19 option 3: never allocate a second job for a mirror that has one # ----------------------------------------------------------------------