From 885d8eed02056bde650c859463ce03df33c686a7 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Fri, 30 Oct 2026 09:58:21 -0700 Subject: [PATCH 1/4] ledger: record existing catalog filtering implementation and invariants --- docs/skill-catalog-ledger.org | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 docs/skill-catalog-ledger.org diff --git a/docs/skill-catalog-ledger.org b/docs/skill-catalog-ledger.org new file mode 100644 index 0000000..5a1d589 --- /dev/null +++ b/docs/skill-catalog-ledger.org @@ -0,0 +1,24 @@ +#+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. Extend coverage only if a demonstrated acceptance gap remains. Verify the regression fails on base and passes on the delivered patch; run relevant mirror/prelude tests from this worktree and confirm imports. + +* 6. Open questions +No design question for delivery. GitHub integration and target-source verification remain outstanding until the focused PR is reviewed/merged; deployment is separate. From accbe114a32af38dd845c580ff2cabc4f92e0244 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Thu, 29 Oct 2026 08:19:35 -0700 Subject: [PATCH 2/4] List only catalog entries that name skills _parse_skill_catalog took the first word of every "- " line as a path, and both path normalizers fall back to a sibling SKILL/SKILLS file when a path names nothing. In a catalog's own directory that fallback is the catalog itself, so prose bullets ("- LSMS_Library: lives in ...") and a prose mention of "`file:`" each rendered as a bogus "SKILLS" skill in every agent prelude, and a bare bullet naming a missing file was listed as if it were a skill. A bare "- word" bullet is now an entry only when the word names something on disk (as given, or with .org/.md). Any reference that normalizes to the catalog itself is skipped with one warning per path, so a typo'd file: link still surfaces in the log. Explicit file: links and the shared normalizers are otherwise unchanged. Rendered against the sucoder-skills catalog as it stood before its own prose fix, the prelude now lists exactly its 38 skills. Co-Authored-By: Claude Opus 5.5 --- sucoder/mirror.py | 30 ++++++++++++++++++++++++++---- tests/test_mirror.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/sucoder/mirror.py b/sucoder/mirror.py index 2e55b0e..11f2741 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.name}.org", path.parent / f"{path.name}.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..7d3782a 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -6543,6 +6543,38 @@ 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 + + # ---------------------------------------------------------------------- # Issue 19 option 3: never allocate a second job for a mirror that has one # ---------------------------------------------------------------------- From 2dfff5fc0967b41ed3afe43c3c42873d10622808 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Fri, 30 Oct 2026 10:14:47 -0700 Subject: [PATCH 3/4] Preserve catalog extension fallback for bare references Match bare candidate checks to the existing normalizer stem fallback so .md and .org references keep resolving to the alternate existing extension. Cover both directions and the nested extensionless control. Co-Authored-By: Codex --- sucoder/mirror.py | 2 +- tests/test_mirror.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/sucoder/mirror.py b/sucoder/mirror.py index 11f2741..4126861 100644 --- a/sucoder/mirror.py +++ b/sucoder/mirror.py @@ -7422,7 +7422,7 @@ def add_reference(raw: str, *, bare: bool = False) -> None: # 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.name}.org", path.parent / f"{path.name}.md") + for candidate in (path, path.parent / f"{path.stem}.org", path.parent / f"{path.stem}.md") ): return normalized = ( diff --git a/tests/test_mirror.py b/tests/test_mirror.py index 7d3782a..c1dcd63 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -6575,6 +6575,27 @@ def test_skill_catalog_lists_only_entries_that_name_skills( 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 # ---------------------------------------------------------------------- From 4aa4fb1a386e22d256e50fb7c82f2ec7462114e8 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Fri, 30 Oct 2026 10:15:21 -0700 Subject: [PATCH 4/4] ledger: record catalog compatibility correction and verification --- docs/skill-catalog-ledger.org | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/skill-catalog-ledger.org b/docs/skill-catalog-ledger.org index 5a1d589..061d339 100644 --- a/docs/skill-catalog-ledger.org +++ b/docs/skill-catalog-ledger.org @@ -18,7 +18,16 @@ Bare bullet prose is not an explicit skill reference. Existing file links retain 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. Extend coverage only if a demonstrated acceptance gap remains. Verify the regression fails on base and passes on the delivered patch; run relevant mirror/prelude tests from this worktree and confirm imports. +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 -No design question for delivery. GitHub integration and target-source verification remain outstanding until the focused PR is reviewed/merged; deployment is separate. +* 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.