Add contributor skills at .agents/skills - #6901
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6310fc6bf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6310fc6. Configure here.
make codex is no longer needed: Codex, Cursor and Gemini CLI read .agents/skills directly. make claude now symlinks .claude/skills to it instead of the removed .ai/skills, and clean-ai only removes that symlink.
|
Both bot findings are addressed in 0d828eb: Worth noting the setup was already broken on main: @albertvillanova this is the repo-scope surface from the #6793 discussion, would like your take. |
albertvillanova
left a comment
There was a problem hiding this comment.
Thanks for opening this — this is item (4) from the #6793 split, so I'll own the fact that I asked for it, and review it on the merits rather than on whether the surface should exist.
Short version: the mechanics are right and I'd land the surface. One of the two skills earns its keep; the other doesn't yet. There's also one migration bug the bots missed, and one namespace question the PR doesn't answer.
Status
Both bot findings are genuinely fixed in 0d828eb — no make codex, and make clean-ai no longer deletes tracked content. No residual references to .ai/skills, make codex, or trl skills anywhere on the branch. Your side note is correct too: make codex/make claude were creating dangling symlinks on main because .ai/skills never existed in the repo.
I ran the commands in the skills rather than trusting them, and they hold up:
grep -rn "self._last_loaded_step" trl/trainer/ trl/experimental/→ 30 hits across 7 files.- The
diff <(sed -n '/def _generate_single_turn/,/def /p' …)recipe extracts 114 lines against a 113-line function and emits 47 diff lines. It works, and it's the right size to actually read. update-paper-index's described format matchesdocs/source/paper_index.mdexactly —##per family withPapers relating to the [...],**📜 Paper**:HF links, config snippet with the paper's hyperparameters quoted in comments.
update-paper-index: land it
This is what the surface is for. Conditional trigger (only PRs implementing a paper), so it costs nothing on the other 95% of PRs. Content that is genuinely not in AGENTS.md, and that an agent cannot infer without reading a 1000-line doc file. It also fixes an AGENTS.md imprecision: AGENTS.md says paper_index.md with no path, the skill gives docs/source/paper_index.md.
check-trainer-consistency: not as written
I diffed it sentence by sentence against .ai/AGENTS.md:
| Skill | Verbatim/near-verbatim from AGENTS.md | Genuinely new |
|---|---|---|
check-trainer-consistency |
15 of 29 sentences at ≥0.75 similarity, 7 more are headings or light paraphrase | the two shell recipes (~5 lines) |
update-paper-index |
2 sentences | essentially the whole body |
The policy content is already in AGENTS.md, which every harness loads unconditionally. So for the agent that reads AGENTS.md, this skill adds a second copy of rules it already has, and nothing else. Its trigger — Use when modifying or reviewing code in any trainer (GRPO, RLOO, SFT, DPO, ...) — fires on nearly every TRL PR, which is the definition of content that belongs in the always-loaded file rather than behind progressive disclosure.
And the two copies have already drifted, before merge: "in a dedicated PR" vs "in one dedicated PR"; "A correct-but-inconsistent codebase is harder to maintain than a consistently-wrong one" vs "A consistently-wrong codebase can be fixed in a single sweep; an inconsistent one cannot." Not a gotcha — just the maintenance cost showing up on day zero, in the file whose subject is not letting duplicated blocks drift, with no CI to catch it.
The 5 lines that are new are the valuable part. I'd move the grep and the diff recipe into the AGENTS.md "Code duplication and consistency" section as two fenced blocks and drop the skill. If you'd rather keep it as a skill, strip it to the operational steps (find every copy → propagate → diff-verify) and have it point at AGENTS.md for the policy, so no sentence exists in both files.
Migration bug: stale .agents/skills symlink blocks checkout
Anyone who followed today's CONTRIBUTING and ran make codex has .agents/skills as a dangling symlink. Reproduced:
error: The following untracked working tree files would be overwritten by checkout:
.agents/skills
Please move or remove them before you switch branches.
Aborting
Hard abort on git pull/git checkout, for exactly the flow the repo documented until this PR. And make clean-ai — the documented cleanup — no longer removes .agents/skills, so the obvious remedy doesn't work either. Either a one-line note in CONTRIBUTING (rm .agents/skills once) or keep a guarded removal in clean-ai:
clean-ai:
[ -L .agents/skills ] && rm .agents/skills || true
rm -rf .claude/skillsNamespace: the PR claims all of .agents/skills without saying how personal skills coexist
Removing /.agents/skills from .gitignore has a side effect worth being explicit about. Today TRL tracks nothing under .agents/, so git collapses that whole tree into a single ?? .agents/ line. Once files there are tracked, git must descend and enumerate — every personal skill and every private subdirectory becomes its own untracked entry:
?? .agents/.plans/
?? .agents/skills/personal-skill/
That's a per-contributor papercut for anyone using .agents/ as a scratch area, which the current ignore line quietly permitted.
I tested the two ways to fix it, judged by what happens when a maintainer adds a third skill:
| Personal skill hidden | New maintainer skill | |
|---|---|---|
/.agents/skills/* + !/.agents/skills/<each tracked one>/ |
yes | silently invisible — never shows in git status, uncommittable without -f |
/.agents/skills/local-*/ |
yes | shows as ?? .agents/skills/add-trainer/ |
The negation whitelist fails in the same direction as the bug this PR fixes: /.agents/skills was silently blocking commits, and a whitelist keeps that silent, just narrower. It also duplicates the directory listing into .gitignore, which is a second copy of a list that will drift. So I'd suggest:
.gitignore: add/.agents/skills/local-*/— one stable line, invariant under adding skills, visible failure mode. Thelocal-prefix ends up in the skill name (Cursor requires frontmatternameto match the folder), which is self-documenting in the agent's skill list.- CONTRIBUTING: say that
.agents/skills/in this repo is maintainer-owned contributor skills, that personal skills belong in~/.agents/skills(user scope in Codex, Cursor, Gemini CLI, and Copilot), and that.agents/skills/local-*/is available for personal skills that must be repo-scoped.
The enumeration of which skills are repo-owned belongs in that prose, where drift is harmless — not in an ignore rule, where drift is silent.
On the surface itself
For the record, since I pushed back hard on the path in #6793 and don't want that read as blanket opposition: committed contributor skills have solid precedent now. PyTorch ships 16 in .claude/skills/, Streamlit 19, Ray 4, vLLM 2 in .agents/skills/ plus 1 in .claude/skills/. Within HF, gradio and trackio use .agents/skills/ at the repo root; nothing in transformers, diffusers, datasets, accelerate, peft, huggingface_hub, lighteval, or smolagents — so TRL would be first in the org proper, which I think is fine given the comparison class.
Worth naming honestly: AGENTS.md at repo root is near-universal, committed skills are at maybe a quarter of comparable projects, and the two heaviest adopters chose .claude/skills rather than .agents/skills. The practice has real precedent; the path is less settled than the practice. That's an argument for keeping the count small and the bar high, not for staying out.
Tracked is also the right call over untracked, for the reason from #6793: the harnesses define a repo's .agents/ as repo/team scope explicitly shared via version control, and the untracked, user-configurable location already exists at ~/.agents/skills. More practically, co-maintenance is the whole point — if someone restructures docs/source/paper_index.md, a tracked skill breaks visibly in the same PR and gets fixed in review, while an untracked one silently starts lying to every agent that reads it.
Asks
- Land
update-paper-indexas is. - Drop
check-trainer-consistency; move its grep and diff recipes into theAGENTS.mdduplication section. (Or strip it to the operational steps with no sentence duplicated fromAGENTS.md.) - Fix the stale-symlink migration.
- Add
/.agents/skills/local-*/to.gitignoreand the scope sentence to CONTRIBUTING. - State the rule that keeps this from rotting: a skill describing a workflow must be updated in the PR that changes that workflow — same rule we already apply to
paper_index.md.
With 1–4 I'm happy to approve.

Adds repo-root contributor skills, the surface
.agents/skillsis actually for (see the discussion in #6793):check-trainer-consistency: the deliberate-duplication rule from AGENTS.md, task-shaped: how to find every copy of a duplicated block, propagate a change, and diff trainers to catch drift.update-paper-index: when and how to add a paper entry, with the exact format ofdocs/source/paper_index.md.Also removes
/.agents/skillsfrom.gitignore: it was listed under "AI agent generated artifacts" (residue of the oldtrl skills installflow) and silently blocked committing anything there.Starting with the two skills that AGENTS.md already documents.
add-trainer,run-slow-tests,prepare-releasecan follow once written and verified.Note
Low Risk
Documentation and Makefile/gitignore changes only; no runtime library or training code is affected.
Overview
Adds versioned agent skills at
.agents/skills/so Codex, Cursor, and Gemini CLI can load contributor workflows without symlinks from.ai/.check-trainer-consistencyturns the AGENTS.md duplication policy into a task guide (grep all trainer copies, propagate edits, diff regions for drift).update-paper-indexdocuments when and how to extenddocs/source/paper_index.md(HF paper links, config snippets with quoted hyperparameters)..gitignoreno longer ignores/.agents/skills, so these skills can be committed (previously treated as generated artifacts).CONTRIBUTING.mdand theMakefiledrop the oldmake codex/.ai/skillssymlink flow;make claudenow points.claude/skillsat.agents/skills, andmake clean-aionly removes that Claude symlink.Reviewed by Cursor Bugbot for commit 05e0f16. Bugbot is set up for automated code reviews on this repo. Configure here.