feat(cli): mirror skills to .agents/skills/ when .agents/ exists#2488
feat(cli): mirror skills to .agents/skills/ when .agents/ exists#2488ArgonarioD wants to merge 6 commits into
Conversation
Some agents prefer repo-local .agents/skills/ over the global install. When .agents/ is present, mirror the standard and generated skills written to .claude/skills/ so those agents serve up-to-date copies. Opt-in via .agents/; absent directory leaves the layout untouched. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ArgonarioD is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for your contribution! We appreciate the time and effort you put into this but there's one ready PR in the queue already for this #2434 |
@magyargergo Thanks for pointing me to #2434. I've read through it — it flattens .claude/skills/ (standard skills as direct children, community skills under a gitnexus-area-* namespace), which fixes discoverability for Claude Code. That covers the Claude side. My PR targets a different gap #2434 doesn't touch: .agents/. Concretely, the current Codex app asks users to migrate repository-level skills from .claude/ into .agents/, then reads only .agents/ afterward. So when gitnexus analyze updates skills under .claude/skills/ (including #2434's new community skills), Codex never picks up those updates — it keeps serving the migrated stale copies. Mirroring the writes to .agents/skills/ on every analyze is what keeps Codex in sync. #2434 has zero references to .agents/, so this isn't covered. Is the .agents/ mirroring intentionally out of scope, or should it land as a follow-up? |
# Conflicts: # gitnexus/src/cli/ai-context.ts # gitnexus/src/cli/skill-gen.ts
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
Review: mirror skills to .agents/skills/ when .agents/ exists
Reviewed at head 7ba5483f against merge-base 91955e65, in a dedicated worktree with a fresh GitNexus index. Evidence: full diff read; detect_changes (exactly the 5 changed symbols, zero affected execution flows); tsc --noEmit clean; all 67 tests in the two touched files pass locally; CI green (fork-level Vercel auth noise aside); a combined domain + cross-cutting expert pass; and the headline finding reproduced empirically through this repo's own test harness.
The feature itself is sound and well-scoped: the opt-in gate is a single stat, canonical writes always precede mirror writes, the gitnexus-area-* mirror cleanup is correctly namespace-scoped (standard and user-authored skills survive it, symmetric with the canonical cleanup), the mirrored content is byte-identical and contains no .claude-specific references, and the skill-gen→ai-context import is safe. Flag interplay (--skills / --skip-skills / --index-only) produces no state where the mirror is written but canonical is skipped.
Two defects need fixing before merge, both cheap:
Empirical repro for the headline finding
With .agents/ present but .agents/skills occupied by a file (harness-driven, this repo's own test helpers):
threw: Error: EEXIST: file already exists, mkdir '…/.agents/skills'
canonicalExists: false ← zero .claude/skills/gitnexus-area-* written
And because the canonical gitnexus-area-* cleanup at skill-gen.ts:89-98 runs before the throwing mirror mkdir, a rerun in that state deletes previously generated canonical skills and writes nothing back — all swallowed silently by analyze.ts's pre-existing catch { /* best-effort */ }. This contradicts the PR body's own contract ("Mirror writes are best-effort and per-skill: a failed mirror copy logs a warning and continues"), which the ai-context.ts half honors and the skill-gen.ts half does not. Details inline.
Findings
- MEDIUM (inline,
skill-gen.ts:163) — unwrapped mirrormkdir/writeFilesilently abort canonical community-skill generation and destroy prior output - MEDIUM (inline,
ai-context.ts:392) —.agentsmissing from the up-to-date fast-path dirty-check exclusion list; repos with a tracked.agents/lose the fast path permanently - LOW (inline,
ai-context.ts:372) — the gate fires on.agents/directories that exist for non-skill purposes (e.g. Codex plugin marketplaces — this repo itself carries.agents/plugins/marketplace.json) - LOW — docs/help staleness and PR-body drift:
README.md:184("6 agent skills installed to.claude/skills/"),README.md:392/:448and the--skip-skillshelp text (src/cli/index.ts:93-94) all omit the mirror (--skip-skillsnow also suppresses the standard-skill mirror;--skillscommunity skills also land in.agents/skills/). The PR body also describes a nested.agents/skills/gitnexus/+generated/layout and manual verification of those paths — the shipped code writes the flat.agents/skills/<name>/layout and clears thegitnexus-area-*namespace, so the description and the recorded manual verification predate the final revision; please re-verify against head and update the body. DoD.md line 62 asks for the doc updates in the same change.
Coverage and residual risk
The 4 new tests cover the mirror-present/mirror-absent happy paths for both write sites, with no conditionals around assertions. Missing: any failure-path test (.agents/skills as a file — the headline trigger; the ai-context.ts warn path), and a fast-path regression test with .agents/ present (the existing #1233 test never creates it). CHANGELOG correctly untouched. Verified clean: architectural fit (repo-local analyze-time injection already lives in these two files; editor-targets.ts remains the global setup/uninstall source of truth, untouched), TypeScript conformance (no any, unknown-typed catches, house test idioms), and YAGNI (the per-site stat and the {skills, agentsMirror} return-shape change are both fine).
Verdict
REQUEST CHANGES — both MEDIUMs have small, mechanical fixes: a shared per-skill try/catch mirror helper used by both sites (which also restores the PR body's stated contract), and two :(exclude) pathspecs in run-analyze.ts. Evidence is valid for head 7ba5483f; re-verify if the head moves.
| // Step 4: Ensure the shared project-skill root exists. Never clear it: it | ||
| // also contains user-authored and standard GitNexus skills. | ||
| await fs.mkdir(outputDir, { recursive: true }); | ||
| if (mirrorToAgents) { |
There was a problem hiding this comment.
MEDIUM — unwrapped mirror writes silently abort canonical community-skill generation and destroy prior output (empirically reproduced).
This mkdir — and the per-skill mirror mkdir/writeFile at lines 218-222 — are the only unwrapped filesystem ops in the mirror feature. Reproduced through this repo's own test harness with .agents/ present but .agents/skills occupied by a file:
generateSkillFilesthrowsEEXISThere, after the canonicalgitnexus-area-*cleanup (lines 89-98) already ran → previously generated canonical skills are deleted and nothing is written back (canonicalExists: falsefor every community skill);- a mid-loop failure at line 219-221 aborts canonical writes for all remaining skills, leaving a partial canonical set;
- the caller (
analyze.ts~1508) swallows the rejection with a silentcatch { /* best-effort */ }, so the user gets no community skills and no message.
The PR body's stated contract — "Mirror writes are best-effort and per-skill: a failed mirror copy logs a warning and continues … it never fails the whole analyze run" — is implemented in ai-context.ts (per-skill try/catch + logger.warn) but not here. No test covers any mirror failure path.
Remediation: wrap the mirror ops (this mkdir and the loop write) in try/catch + warn, mirroring ai-context.ts's pattern — simplest shape is a shared mirrorSkill(dir, name, content) helper used by both sites, which also prevents the two implementations drifting again. A regression test with .agents/skills as a file would pin the contract.
| const skillsDir = path.join(repoPath, '.claude', 'skills'); | ||
| const legacySkillsDir = path.join(skillsDir, 'gitnexus'); | ||
| const installedSkills: string[] = []; | ||
| const agentsMirror = await shouldMirrorSkillsToAgents(repoPath); |
There was a problem hiding this comment.
MEDIUM — .agents is missing from the up-to-date fast-path dirty-check exclusion list, permanently defeating "Already up to date" for exactly the repos this feature targets.
run-analyze.ts:960-977 runs git status --porcelain with :(exclude) pathspecs for every path GitNexus itself writes during analyze — .gitnexus, .claude/**, .cursor/**, AGENTS.md, CLAUDE.md — with the contract spelled out in the comment above it: counting self-written paths as dirty "would perpetually defeat the up-to-date fast path" (#1233 regression guard). This PR adds a new analyze-time write surface (.agents/skills/**, from this call plus generateSkillFiles on --skills) without adding it to that list.
Failing scenario: a repo with a tracked .agents/ (e.g. a committed plugin marketplace) and no .agents/* ignore rule. Run 1 writes untracked .agents/skills/gitnexus-*/SKILL.md mirrors; every subsequent same-commit analyze sees ?? .agents/skills/…, reports dirty, and falls through to the full incremental pipeline (lbug write lock, parse-cache diff, context rewrite — which rewrites the mirror, keeping the loop alive) instead of the instant early return. Permanent until the user commits or ignores the files, and amplified by hook-triggered analyzes contending for the lock. The PR body's "strictly additive… no index refresh required" doesn't hold for this population. The existing fast-path test (#1233) never creates .agents/, so nothing catches it.
Remediation: add ':(exclude).agents' + ':(exclude).agents/**' to the pathspec array at run-analyze.ts:969-976, and a fast-path test with .agents/ present.
| * an `.agents/` directory, skills written to `.claude/skills/` are mirrored | ||
| * there too so those agents serve the up-to-date copies. | ||
| */ | ||
| export async function shouldMirrorSkillsToAgents(repoPath: string): Promise<boolean> { |
There was a problem hiding this comment.
LOW — the opt-in gate fires on .agents/ directories that exist for non-skill purposes.
.agents/ is also the Codex plugin-marketplace convention — this repository itself carries a checked-in .agents/plugins/marketplace.json (see CONTRIBUTING.md:179, with the rest of .agents/ gitignored). A repo carrying .agents/plugins/ only — no agent reading repo-local skills — gets 6 (up to ~26 with --skills) unrequested skill directories injected, and absent an ignore rule, the fast-path defeat from the other comment. Harmless in this repo only because its .gitignore happens to swallow the rest of .agents/.
Cheap option: gate on .agents/skills/ existing rather than .agents/ (a repo that wants the mirror creates the directory once), or document the trigger in the README so plugin-only repos know how to avoid it. Flagged as a design cost with a concrete carrying scenario, not a blocker — author's call.
|
Thanks for the update. I rechecked the current head against current I agree with the concern that this overlaps with the already-ready skill-layout work in #2434, so I would not merge this as-is unless the maintainers explicitly want this narrower There is also still a correctness gap in the generated-skill mirror path. Standard skill mirroring in If this PR continues, generated skill mirroring should match the standard-skill behavior: canonical |
azizur100389
left a comment
azizur100389
left a comment
There was a problem hiding this comment.
Requesting changes.
gitnexus/src/cli/skill-gen.ts should treat generated-skill mirroring to .agents/skills/generated/ as best-effort, the same way standard skill mirroring is handled. Right now a mirror mkdir/write failure can escape after canonical cleanup and leave .claude/skills/generated/ missing or partial. Please wrap the mirror path with a warning-only error boundary and add a regression test where .agents/skills fails but canonical .claude output still succeeds.
Please also exclude .agents / .agents/** from the analyze dirty-check pathspecs in gitnexus/src/core/run-analyze.ts, otherwise repos that track .agents/ can lose the up-to-date fast path after mirror writes.
Since this changes user-visible analyze output, the README/help text should mention the .agents mirror behavior and how --skip-skills affects it.
3 participants
Some agents prefer repo-local .agents/skills/ over the global install. When .agents/ is present, mirror the standard and generated skills written to .claude/skills/ so those agents serve up-to-date copies. Opt-in via .agents/; absent directory leaves the layout untouched.
Summary
analyze writes skills to .claude/skills/. Some agents instead read skills from a repo-local .agents/skills/ directory and prefer it over the global ~/.agents/skills/ install. This PR mirrors the same SKILL.md files into .agents/skills/ when the repo already contains an .agents/ directory, so those agents serve the up-to-date repo-specific skills instead of stale global copies.
Motivation / context
setup already installs the bundled GitNexus skills globally to ~/.agents/skills/ for Codex (editor-targets.ts). But analyze generates repo-specific skills (community skills from --skills, plus the standard skill set) only under .claude/skills/. Agents that resolve skills from a repo-local .agents/skills/ never see those repo-specific skills and keep serving the stale global set. Mirroring — gated on the presence of .agents/ — closes that gap without changing behavior for repos that haven't opted in.
No linked issue; surfaced while investigating where analyze writes repo-level skills.
Areas touched
Scope & constraints
In scope
Explicitly out of scope / not done here
Implementation notes
Testing & verification
Risk & rollout
Checklist