fix(symlink-lint): enforce relay completeness and sync missing self-adoption symlinks - #1143
fix(symlink-lint): enforce relay completeness and sync missing self-adoption symlinks#1143onlyarnav wants to merge 1 commit into
Conversation
dpol1
left a comment
There was a problem hiding this comment.
Thanks for picking #1137 up so fast — the five links are right (mode 120000, canonical → ../../skills/…, relays → ../../.agents/skills/magpie-…), and rule 3 run against main reports exactly the five missing links and nothing else. Skill discovery mirrors the validator (is_dir() + no leading dot + PRUNE_DIR_NAMES), so skills/pyproject.toml and the cache dirs are ignored, and agent-dir discovery stays convention-based like rules 1–2. Locally on the PR head: symlink-lint and --archive exit 0, ruff check / mypy / pytest (27) green. Branch is clean against main (MERGEABLE); merge is BLOCKED only by the red prek check.
Two things need to land before this can merge, plus a few smaller ones.
CI is red — workspace-ruff-format (__init__.py:145-147)
Confirmed from run 33528122998, not guessed: ruff format (workspace)...Failed, 1 file would be reformatted. The three-line find_missing_relays(...) signature fits the tool's 110-column limit, so ruff collapses it to one line. uv run --directory tools/symlink-lint --project . --group dev ruff format src and recommit. (AGENTS.md § Local setup: "Before opening or updating a PR, run prek run --all-files … as a hard pre-flight gate.")
The hook never fires on the case rule 3 exists for (.pre-commit-config.yaml:234)
The symlink-lint hook is types: [symlink]. Rule 3's failure mode — a new skills/<x>/ committed with no links — stages no symlink, so locally the hook does not run on exactly that commit; only CI's --all-files catches it, which is how #1137's drift landed in the first place. The archive hook at :250-251 already has the right shape: add a files: pattern covering skills/.* and the agent skills/ trees with types_or: [symlink, file], and update the comment at :224-227 and README.md:106-107 ("fired on any staged symlink"). (AGENTS.md § Local setup: "fix the underlying issue or update the hook config in the same PR"; README.md:53-54 promises rule 3 "catches new skills that landed without their self-adoption symlinks".)
Smaller points
- Scoping heuristic (
__init__.py:162-164). "Framework checkout" is inferred fromroot/skillsbeing any directory. An adopter repo with its own top-levelskills/gets told to add.agents/skills/magpie-<subdir>for every subdirectory (reproduced on a scratch tree). Rule 2 makes a similar assumption already, so this is not new exposure — but the README/docstring now claim adopters are exempt, which overstates it. Either gate on a framework marker (e.g.skills/setup/SKILL.md), or reword the exemption. - Trusted-source pointer dirs (
__init__.py:166-170). Askills/<name>/source.mdpointer dir (noSKILL.md) is treated as a skill, so rule 3 demands a canonical link →../../skills/<name>. Perskills/setup/skill-sources.md:222-231that link's real target is../../.apache-magpie-sources/<id>/skills/<name>/and it is gitignored — so a fresh clone with any pointer dir fails the lint (reproduced). None exist in-tree today, so this is latent; skip dirs withoutSKILL.mdthe way the validator'sis_skill_source_pointerdoes, and add the test case. - Tests (
tests/test_symlink_lint.py:192-197).test_completeness_unwired_agent_dir_not_requiredbuilds the same tree as the clean test — no agent dir without askills/child is ever created, so the behaviour in its name is untested;(tmp_path / ".github").mkdir()before the assert fixes it. Missing cases: pointer dir (above),skills/pyproject.toml. - Out-of-scope changes.
os.path→posixpath(:53,:252-253),_relreturningstr(:262-266), and the module-widepytestmark = skipif(not _can_symlink())(tests:53-55) are not in #1137 or the commit message. The skipif in particular silently skips the whole suite — archive andmain()tests included — whereveros.symlinkfails. Drop them here or split into their own PR with the rationale. (AGENTS.md § Before submitting: "Re-read the diff and check that every change is intentional.") - Doc drift to three rules.
README.md:78("Unlike rules 1–2"),docs/adapters/add-a-harness.md:76("enforces both rules"),.pre-commit-config.yaml:224-225comment. - Wording.
README.md:54-55/__init__.py:41-42conflate the dangling-skip (applies everywhere) with rule 3'sskills/-absent exemption.docs/labels-and-capabilities.md:305and the spec edit at:43-44now have "(canonical/relay target-correctness)" trailing "incomplete … sets" — it belongs after "misdirected skill relays". - Nits.
__init__.py:185-190: every target inskills/setup/agents.mdis a dot-dir, soentry.name.startswith(".")tightens the wired-dir scan cheaply. PR body:Fixes #1137(orRefs) instead of the## possible fix for:heading, so the link is machine-readable.tools/spec-loop/.last-syncwas not bumped alongside the spec edit — though it is already behindmainfor unrelated reasons, so that is a judgment call.
This review was drafted by an AI-assisted tool and posted by
a contributor who does not have confirmed Apache Magpie maintainer
access. The findings below are this tool's analysis only, not
a maintainer sign-off; an Apache Magpie maintainer will still need
to look at the PR before it moves forward. If you think a
finding is mis-applied, please reply on the PR.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.
5216692 to
bb8bdeb
Compare
…doption symlinks The self-adoption relay directories drifted out of sync with skills/: - dependency-license-audit was missing from .agents/skills/ (canonical), .claude/skills/, .github/skills/, and .kiro/skills/ (relays). - report-framework-issue was missing from .kiro/skills/. Add Rule 3 (completeness) to symlink-lint to verify that every skill under skills/ has a canonical entry in .agents/skills/ and that every wired agent directory relays the full canonical set (scoped to framework checkouts). Also wire the missing symlinks, update the pre-commit hook filter, and add corresponding test coverage. Fixes apache#1137 Generated-by: Antigravity (Gemini 3.7 Flash)
bb8bdeb to
94c1e38
Compare
Summary
skills/(71 skill directories):.agents/skills/(canonical),.claude/skills/,.github/skills/, and.kiro/skills/were missingmagpie-dependency-license-audit..kiro/skills/was missingmagpie-report-framework-issue.symlink-lint: in the framework checkout (whereskills/lives), every skill directory underskills/must have its canonical entry under.agents/skills/and every wired agent directory (.claude/,.github/,.kiro/, etc.) must relay the full canonical set. Adopter checkouts (where rootskills/is absent) remain exempt.tools/symlink-lint/tests/test_symlink_lint.py.tools/symlink-lint/README.md,docs/labels-and-capabilities.md, andtools/spec-loop/specs/meta-and-quality-tooling.md.Type of change
tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)prek, workflows, validators)Test plan
uv run --directory tools/symlink-lint --project . ruff check src testspassesuv run --directory tools/symlink-lint --project . mypy src testspassesuv run --project tools/symlink-lint pytest tools/symlink-lint/testspassespython tools/symlink-lint/src/symlink_lint/__init__.py --archivepasses (exit code 0)120000RFC-AI-0004 compliance
possible fix for: #1137
Notes for reviewers (optional)
Generated-by: Antigravity (Gemini 3.7 Flash)