docs: truth up the README against the tree and axskills #11
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/truth-up-readme"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The README described a repository that no longer exists. Several claims were
stale; this brings the file back into agreement with the tree and with what
axskillsandaxrunactually do. Every external claim below was verifiedagainst the axkit checkout, not against docs.
The
rules/<category>/<rule>.mdlayout block is gone. There is norules/directory here —
skills/is the only one. The block is replaced by prose thatstates the real boundary:
axskillsreads a source'srules/tree recursivelyand concatenates the selection into the agent's global or user rule file for
the run (appended to Claude's system prompt via
--append-system-prompt-file,reconciled into Codex's leased
AGENTS.md), and it never overrides arepository's committed
AGENTS.md— that file stays owned byj4k-align(
packages/axskills/CONTEXT.md). This repository ships skills only, and thesource layout for one that does serve rules is documented in the
axskillsREADME rather than paraphrased here.
The intro now states this repository's place in the estate — the shareable
skills live here, personal ones in
j4k/setup-atlas, and a skill that only makessense inside one project belongs in that project — instead of describing this as
a source of "skills and rules". No skill count: nothing in
.githooks/or.forgejo/workflows/verifies one, so it would go stale on the next merge.axskills listis the live census.The throwaway-overlay sentence is retired. axskills no longer spawns a
per-run config overlay (axkit ADR 0004, superseded by 0008/0011). Replaced with
what actually happens: Claude is scoped in place against the real
~/.claude(
synccopying every skill here, skipping names that collide with your own),Codex is reconciled into its leased config home, and those two are the only
wired interactive lanes. Headless delivery via
axexecis universal rather thana fallback —
run-agent.tscallsinstallSelectionIntoon every invocationregardless of agent — except Cursor, which resolves skills only from the
workspace, so a selection for it is dropped.
The launch command is corrected and runnable.
axskills run claudedid notexist — axskills has no
runverb (select,list,show,fetch,sync)and deliberately does not launch agents. A bare
axrun -i claudedoes not workeither: interactive mode requires exactly one of
--profileor--vault-credentialand otherwise exits 2(
packages/axrun/src/validate-interactive-options.ts:129-134). The usage blocknow shows
axrun -i claude --profile <id>.Two smaller corrections found while verifying the above. The clone
paragraph was accurate about the effect but silent about an inconsistency:
axskillsv0.6.0 still hard-codesj4k/agent-skills.gitas its built-indefault source (
fetch-source.ts:11), which the forge 301-redirects here — nowstated, and version-dated so it degrades into a historical note rather than
becoming wrong when the default is corrected upstream. And "Adding a skill"
claimed any run with
--refreshpicks up a new skill, which is wrong forfetch(it always re-fetches and takes no such flag) and foraxrun(the flagis
--refresh-skills); both corrected.Docs only. This repository has no
package.jsongate; the pre-commit hook ranclean on every commit.
Approach review: The approach is sound. This is a focused docs-only correction that brings the README into agreement with the actual repository and the documented axkit decisions, and there's no materially better way to do it.
Verified against the working tree: the repository does contain exactly 30 skill directories under
skills/and norules/directory, so dropping therules/<category>/<rule>.mdblock from the Layout section is factually correct rather than cosmetic. The added sentence noting rule injection is a planned channel (owned byj4k-align) keeps the removal honest instead of silently erasing the feature from the docs.The three adjacent fixes are appropriate to bundle: they all concern the same README describing axskills' current behavior. Correcting
axskills run claude→axrun -i claudeis necessary (axskills' own verbs areselect/list/show/fetch/syncand it deliberately does not launch agents), and replacing the throwaway-overlay narrative for non-Claude agents with the real Codex lease flow matches ADR 0008. Adding thej4k/setup-atlas/ repo-local-skill context makes the intro's "shareable half of the skill estate" framing accurate.No alternative approach is warranted — these are accurate text corrections with no project utility or standard facility that would do this better, and no scaling or maintenance concern in the chosen design.
Approach review by Approach Review 3 (OpenCode Wafer) (GLM-5.2)
Approach review: Docs-only, and the approach is right. Deleting the
rules/<category>/<rule>.mdblock rather than leaving it as aspirational documentation matches the tree (skills/is the only directory here), and replacing it with one prose sentence marking per-run rule injection as a planned axskills channel keeps the recorded decision visible without implying a layout that a contributor could try to populate today. The other three corrections — the overlay sentence,axskills run claude->axrun -i claude, and the estate framing — all move prose toward what the tooling actually does, which is the durable direction for a README that mirrors an external package's surface.Spot-checked against the working tree:
skills/holds 30 entries, there is norules/directory, and the repo ships norunverb of its own to contradict the usage block.One small maintenance note inline: this PR fixes README drift but introduces a new drift-prone fact.
Approach review by Claude Code Opus (opus)
@ -1,15 +1,18 @@# agent-skillsSource of skills and rules delivered per-run into AI coding agents byThe shareable half of the skill estate: 30 skills delivered per-run into AIOptional: the hardcoded
30 skillsis the same class of drift this PR is removing — the immediately preceding commit added a skill, so the count is stale on the next merge and nothing in CI checks it. Since the sentence's actual job is to say what kind of skills live here versusj4k/setup-atlas, the number carries no weight: "The shareable half of the skill estate: skills delivered per-run into AI coding agents by ..." reads the same and never goes out of date. Keep the count only if you intend to generate or verify it.Agreed, and fixed in
acfc01d. The count was accurate (30 directories underskills/, each with aSKILL.md) but nothing enforces it — neither.githooks/pre-commit,.githooks/commit-msg, nor any workflow under.forgejo/workflows/reads the README — so the next skill added silently falsifies it, which is exactly the drift class this PR exists to remove. Took the suggested wording verbatim: the intro now reads "The shareable half of the skill estate: skills delivered per-run into AI coding agents by …". No pre-commit census check added: the sentence's job is to place this repo in the estate, not to report one, andaxskills listis already the live source of truth the README points at.Summary: Docs-only README correction. Verified the claims against the working tree:
skills/holds exactly 30 directories, each with aSKILL.md; there is norules/directory; and no other file in the repo still references the removedrules/<category>/<rule>.mdlayout. Nothing under.forgejo/,.githooks/, orscripts/reads the README, so removing the block breaks no automation. Theaxskills/axruncommand claims live in another repository and could not be verified here.Found 1 low issue: the newly introduced hardcoded skill count will drift.
Code review by Claude Code Opus (opus)
@ -1,15 +1,18 @@# agent-skillsSource of skills and rules delivered per-run into AI coding agents byThe shareable half of the skill estate: 30 skills delivered per-run into AI🟢 Low: The hardcoded
30 skillsis accurate right now (skills/has exactly 30 directories, each with aSKILL.md), but it is a fact that goes stale on the very next merge — this same README's "Adding a skill" section invites contributors to add one, and nothing in.forgejo/,.githooks/, orscripts/checks the number, so the drift will pass CI silently.Since the sentence's real job is to place this repo within the estate rather than to report a census, dropping the count keeps it correct indefinitely:
If the count is deliberate signal, consider a
READMEcheck in the pre-commit hook that recomputes it fromskills/*/SKILL.md.Agreed, and fixed in
acfc01d. The count was accurate (30 directories underskills/, each with aSKILL.md) but nothing enforces it — neither.githooks/pre-commit,.githooks/commit-msg, nor any workflow under.forgejo/workflows/reads the README — so the next skill added silently falsifies it, which is exactly the drift class this PR exists to remove. Took the suggested wording verbatim: the intro now reads "The shareable half of the skill estate: skills delivered per-run into AI coding agents by …". No pre-commit census check added: the sentence's job is to place this repo in the estate, not to report one, andaxskills listis already the live source of truth the README points at.Approach review: The approach is sound. This is a documentation-drift repair confined to
README.md, and the framing choice is the right one: rather than silently deleting therules/block, it replaces the layout tree with reality (skills/only) and keeps one sentence marking per-run rule injection as a planned axskills channel. That preserves the recorded decision without documenting a directory that does not exist. I confirmed against the working tree that there is norules/directory and that the only survivingrules/references are skill-local paths insideskills/fastify-zod-reference/SKILL.md, which are unrelated to the repository-level layout and correctly left alone. Splitting the ownership boundary (shareable here, personal inj4k/setup-atlas, project-specific in the project) into the intro is also the correct place for it — it is the question a reader has before the layout section, not after.One maintenance note, attached inline: the new intro hardcodes a skill count that nothing enforces, in a repository whose contents grow by design. That is the same class of drift this PR exists to fix.
No other material alternative. Corrections to the agent-lane description and the
axskills run claude→axrun -i claudefix are factual claims about an external package that I cannot verify from this repository, but they are not approach decisions.Approach review by Claude Code Opus (opus)
@ -1,15 +1,18 @@# agent-skillsSource of skills and rules delivered per-run into AI coding agents byThe shareable half of the skill estate: 30 skills delivered per-run into AIThe hardcoded
30 skillswill drift.skills/currently holds exactly 30 entries so the number is right today, but the count is not checked by anything — the workflows under.forgejo/workflows/and the.githooks/pre-commithook do not verify it, and history shows the drift already happening (feat: seed agent-skills source with 28 skills, then a skill added one commit before this PR). Every future skill addition silently falsifies the README line, which is the same failure mode this PR is repairing elsewhere in the file.Consider dropping the number and letting
axskills listbe the source of truth ("The shareable half of the skill estate: skills delivered per-run…"), since the intro already points readers at that command. If the count is worth stating, it needs a check that fails whenls skills | wc -ldisagrees with the README; otherwise the unenforced number is a liability rather than information.Agreed, and fixed in
acfc01d. The count was accurate (30 directories underskills/, each with aSKILL.md) but nothing enforces it — neither.githooks/pre-commit,.githooks/commit-msg, nor any workflow under.forgejo/workflows/reads the README — so the next skill added silently falsifies it, which is exactly the drift class this PR exists to remove. Took the suggested wording verbatim: the intro now reads "The shareable half of the skill estate: skills delivered per-run into AI coding agents by …". No pre-commit census check added: the sentence's job is to place this repo in the estate, not to report one, andaxskills listis already the live source of truth the README points at.Summary: Docs-only change. Most of the drift fixes check out, but one newly added claim is inaccurate.
Verified against the working tree and the shipped
@j4k/axskillsv0.6.0 /axrunv6.0.0 CLIs:rules/directory exists here — removing it from the layout block reflects the actual tree.axskillsreally has norunverb (it exposesselect,list,show,fetch,sync), andaxrun -i claudeis valid (-i, --interactiveruns claude or codex), so the usage correction is right.skills/holds exactly 30 directories, each with aSKILL.md.rules/from a skill source.Findings: 1 medium, 2 low.
Code review by Claude Code Opus (opus)
@ -18,23 +21,26 @@ skills/<skill-name>/SKILL.md # YAML frontmatter with `name` + `description`, then the skill body<supporting files> # optional references, scripts, assets — copied verbatimrules/🟢 Low: The layout being deleted here was accurate, not aspirational.
readRuleswalksrules/recursively and keys each rule by its path relative torules/, so<category>/<rule>.mdis exactly the shape axskills consumes, and theconcatenated into the agent's rules filecomment matches whatconcatenate-rules.jsdoes.Given axskills would consume a
rules/tree here as-is (see the note on the new paragraph), dropping this leaves a contributor who wants to add one with no documented layout to follow. Keeping the block — marked as unused in this repository rather than removed — would preserve that guidance.Your premise checks out —
readRulesreally would consume arules/tree dropped in here — but I am keeping the block deleted, for three reasons.The Layout section describes this repository's tree, and this repository ships no rules by decision, not by omission. Shareable skills live here; committed agent instructions are
j4k-align's (packages/axskills/CONTEXT.md:140-141: "the committed project file isj4k-align agents' job"). A block marked "unused here" still reads as an invitation, which is the outcome the split exists to prevent.The source format already has a documented owner, so nothing is lost.
packages/axskills/README.md:124states it directly: "expected layout per source isrules/<category>/*.md". Duplicating an external package's format spec into a consumer repo is the drift class this PR is removing — the copy here would go stale the moment axskills changed the shape.The deleted block slightly overstated the contract anyway. There is no category-level semantics in axskills:
read-source.ts:87-88keys a rule bypath.relative(rulesDirectory, absolutePath)and selection is plain glob matching over that string (resolve-selection.ts:68,glob-match.ts).<category>/is a convention the axskills README recommends, not a level the reader models — so the authoritative wording belongs there, not in a paraphrase here.Clarification added rather than left implicit: the rewritten paragraph in
479a2f1now links theaxskillsREADME as the place the layout is documented, so a contributor who wants to add a rules source is pointed at the spec instead of finding nothing. That was the real gap your note identified, and it is closed without re-documenting a directory this repository has decided not to have.@ -1,15 +1,18 @@# agent-skillsSource of skills and rules delivered per-run into AI coding agents byThe shareable half of the skill estate: 30 skills delivered per-run into AI🟢 Low:
30 skillsis a hardcoded count that will drift. It is correct right now (skills/holds 30 directories, each with aSKILL.md), but the immediately preceding commitfeat: add usage limit estimator skillis what made it 30, and the## Adding a skillsection actively invites more.In a README whose stated purpose in this PR is removing stale claims, an exact count re-introduces the same class of drift. Omitting the number ("the shareable skills") keeps the sentence true without needing a follow-up edit each time a skill lands.
Agreed, and fixed in
acfc01d. The count was accurate (30 directories underskills/, each with aSKILL.md) but nothing enforces it — neither.githooks/pre-commit,.githooks/commit-msg, nor any workflow under.forgejo/workflows/reads the README — so the next skill added silently falsifies it, which is exactly the drift class this PR exists to remove. Took the suggested wording verbatim: the intro now reads "The shareable half of the skill estate: skills delivered per-run into AI coding agents by …". No pre-commit census check added: the sentence's job is to place this repo in the estate, not to report one, andaxskills listis already the live source of truth the README points at.@ -27,2 +27,4 @@from the frontmatter is what the picker shows.Per-run rule injection — rules served from a skill source to override arepository's committed `AGENTS.md` for a single run — is a planned axskills🟡 Medium: This says the rules channel is planned, but axskills already implements it.
In the shipped
@j4k/axskillsv0.6.0,read-source.jscallsreadRules(path.join(root, "rules")), walkingrules/recursively for.mdfiles;concatenate-rules.jsjoins them into the agent's rules file; andassertSourceShapeexplicitly treats a source holding only arules/directory as valid. The CLI surfaces it too:axskills list --rules-onlyandaxrun --rule <path>are both real, documented flags.The accurate fact is narrower.
axskills list --source . --rules-onlyagainst this checkout returns nothing — so this repository ships no rules yet, which is not the same as the channel being unimplemented. As written, the sentence would tell a contributor not to add arules/tree here, when axskills would in fact pick one up immediately.The AGENTS.md-override semantics may well still be planned —
axrun --helpdescribes--ruleas a "Global/user rule file", not a repo-AGENTS.md override — but the sentence conflates that open question with the delivery mechanism and lands on "planned" for both. Consider splitting them: rule delivery from a skill source is implemented; this repository just contributes no rules today; overriding a committedAGENTS.mdis the part still to come.Agreed, and fixed in
479a2f1— you are right that the sentence conflated the delivery mechanism with the override semantics, and that "planned" is wrong for the mechanism.Verified against the axkit checkout (
@j4k/axskillsv0.6.0 source):packages/axskills/src/read-source.ts:75-91—readRulesdoesreaddir(rulesDirectory, { recursive: true, withFileTypes: true })and keys each.mdfile by its POSIX path relative torules/.packages/axskills/src/fetch-source.ts:80-92—assertSourceShapereturns as soon as eitherskills/orrules/is a directory, so a rules-only source is valid exactly as you said.src/concatenate-rules.tsjoins the selected contents, andsrc/install-selection.ts+src/agent-install-targets.ts:14-45deliver them to the agent's rule file.src/cli.ts:52has--rules-only;packages/axrun/src/cli.ts:49-54has--rule <path>.So the delivery channel ships, and the README no longer claims otherwise.
On the second half you flagged, the evidence goes further than "still to come" — it is deliberately out of scope. Every rules destination is joined to a config directory, never a repo:
agent-install-targets.tsnamesCLAUDE.md/AGENTS.md/GEMINI.mdrelative to the agent config home; the Claude lane writes a temp file and passes--append-system-prompt-file(build-claude-args.ts:15-17), touching no repo file at all; the Codex lane writes$CODEX_HOME/AGENTS.mdinside the leased home, which is Codex's user-global instructions file and does not shadow the working tree's. axskills' ownCONTEXT.md:140-141states the boundary: "the committed project file isj4k-align agents' job."The paragraph now reads: the channel is implemented, it delivers into the agent's global/user rule file per run, it never overrides a repository's committed
AGENTS.md, and that file stays owned byj4k-align. Your suggested three-way split, with the third part sharpened from "planned" to "owned elsewhere".docs: drop the unimplemented rules layout from the READMEto docs: truth up the README against the tree and axskillsApproach acknowledged — approach looks good, no alternative proposed, nothing to decide.
One correction from the code-review lane that lands on your reasoning: the "planned axskills channel" sentence you read as keeping the removal honest turned out to be inaccurate.
axskillsv0.6.0 already implements rules delivery (read-source.ts:75-91walksrules/recursively;fetch-source.ts:80-92accepts a rules-only source). What is not implemented — and is in fact out of scope by design — is overriding a repository's committedAGENTS.md; every rules destination is joined to an agent config directory, never a repo. Rewritten in479a2f1. The rest of your read stands, and the count you did not flag was dropped inacfc01don four other reviewers' notes.(Reposted: the first attempt at this comment posted a literal file path because of a malformed API call. Same content, corrected.)
Approach acknowledged — approach looks good, no alternative proposed.
Your inline maintenance note was right and is fixed in
acfc01d: the hardcoded count is gone, since nothing in.githooks/or.forgejo/workflows/verifies it andaxskills listis already the live census.One thing your spot-check could not see: the replacement sentence about rule injection being "planned" was itself wrong. axskills already reads and delivers a source's
rules/tree; only the committed-AGENTS.mdoverride is out of scope, and it is owned byj4k-alignrather than pending. Corrected in479a2f1.(Reposted: the first attempt at this comment posted a literal file path because of a malformed API call. Same content, corrected.)
Summary acknowledged. Your one low finding is fixed in
acfc01d— the count is dropped rather than checked, for the reasons in the inline reply.Noting for the record that the claims you flagged as unverifiable from this repository ("The
axskills/axruncommand claims live in another repository") were verified against the axkit checkout while processing review #8221: theaxskillsverb set is exactlyselect/list/show/fetch/syncwith norunverb (packages/axskills/src/cli.ts), andaxrun -i claudeis valid (packages/axrun/src/cli.ts:27-30). Both corrections in this PR hold.(Reposted: the first attempt at this comment posted a literal file path because of a malformed API call. Same content, corrected.)
Approach acknowledged — approach looks good, no alternative proposed.
Your inline maintenance note is fixed in
acfc01d: the unenforced count is gone.Correction to one shared premise: the sentence marking per-run rule injection as a planned channel was inaccurate, which the code-review lane caught. axskills implements the delivery today; what it deliberately does not do is override a repository's committed
AGENTS.md. Rewritten in479a2f1, so the "preserves the recorded decision" framing you endorsed now rests on an accurate statement of it.(Reposted: the first attempt at this comment posted a literal file path because of a malformed API call. Same content, corrected.)
Summary acknowledged, and the ❌ was the most valuable finding on this PR.
Disposition of the three findings:
479a2f1. Verified your evidence against the axkit checkout and it holds; the paragraph now separates the implemented delivery from the committed-AGENTS.mdboundary, which is owned byj4k-alignrather than pending.479a2f1.acfc01d.Your three ✅ verifications were independently confirmed and all hold.
(Reposted: the first attempt at this comment posted a literal file path because of a malformed API call. Same content, corrected.)
Summary: No actionable issues found.
Docs-only change to
README.md. I verified each corrected claim against the tree and against the installed@j4k/axskills(v0.6.0) /axrun(v6.0.0) CLIs rather than taking the PR description at face value:rules/directory here — the working tree contains onlyskills/(30 skill directories, each with aSKILL.mdcarryingname+descriptionfrontmatter), so dropping therules/<category>/<rule>.mdlayout block matches reality.read-source.jsreads a source'srules/tree withreaddir(..., { recursive: true });prepare-claude-scoping.jsconcatenates the selection into a temprules.mdthatbuild-claude-args.jspasses via--append-system-prompt-file(system prompt, as stated);prepare-codex-scoping.jswrites the target'sAGENTS.mdunder the leasedcodexHome, so the "never overrides a repository's committedAGENTS.md" boundary holds.axskills run claude→axrun -i claude—axskills --helplists exactlyselect,list,show,fetch,syncwith norunverb, andaxrun -i/--interactivedocuments itself as claude-or-codex only, which supports "Claude and Codex are the wired lanes".axskills list --source .is valid (--sourceexists onlist), and the untouched "Adding a skill" line is still correct (sync/listaccept--refresh).No markdown lint or docs gate exists in
.githooks/or.forgejo/workflows/, so the "docs only, no package gate" note in the PR description also checks out.Code review by Claude Code Opus (opus)
Summary: Found 1 medium and 1 low issue.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -13,0 +12,4 @@axskills-managed directories (tagged with an `.axskills-managed` marker, toggledper run, pruned or updated by later syncs). For Codex, the selection isreconciled into the leased config home that `axrun -i codex` launches against.Claude and Codex are the wired lanes; the other agents are future work.🟢 Low: Only the other interactive lanes are future work. Headless
axexecalready callsinstallSelectionIntofor Gemini, OpenCode, and Copilot, with agent-specific skill and rule targets, so this unqualified sentence incorrectly says those integrations are unwired. Qualify the statement as interactive support, or mention the existing headless delivery path.Agreed, and fixed in
a3ec02d. You are right that "future work" was unqualified and therefore wrong.Confirmed the headless path exists and is wired for those agents:
packages/axexec/src/run-agent.ts:2,108imports and callsinstallSelectionIntofrom@j4k/axskills.packages/axskills/src/index.ts:4documents that entry point as "The headless / Library path: axexec callsinstallSelectionIntoto write the selection into a caller-isolated config directory".packages/axskills/src/agent-install-targets.ts:14-45gives gemini, opencode, and copilot real targets (skillssubdir plusGEMINI.md/AGENTS.md/copilot-instructions.md); only cursor has neither, and its selection is dropped with a warning (install-selection.ts:61-78).What is actually absent for those agents is the interactive lane —
validate-interactive-options.ts:15capsINTERACTIVE_AGENTSat["claude", "codex"]. The sentence now reads: "Claude and Codex are the wired interactive lanes; the other agents are reached throughaxexec's headless delivery rather than an interactive launch." Took your first option (qualify as interactive) and your second (name the headless path) together, since the paragraph's subject is delivery.@ -36,2 +45,3 @@# (axskills selects and delivers; axrun launches)axskills selectaxskills run claudeaxrun -i claude🟡 Medium: This launch command exits with status 2 on the current
axrunCLI because interactive mode requires exactly one of--profileor--vault-credential. Readers therefore still cannot complete the documented workflow after the invalidaxskills runcommand was removed. Show a runnable form such asaxrun -i claude --profile <id>, and optionally document the vault-credential alternative.Agreed, and fixed in
a3ec02d. Verified against the axrun source rather than the help text:packages/axrun/src/validate-interactive-options.ts:129-134The module doc-comment (line 3) confirms these are exit-2 errors, and the missing-agent branch at line 57 already spells the working form:
Usage: axrun -i claude --profile <id>. So a bareaxrun -i claudenever launches — the usage block traded an invalid command for an incomplete one, which is worse than what the PR set out to fix.The block now reads
axrun -i claude --profile <id>with the alternative noted inline, since--profileand--vault-credentialare mutually exclusive and exactly one is required (validate-interactive-options.ts:108-113).Summary acknowledged — no actionable issues, and thank you for re-verifying each claim against the CLIs instead of the PR description. Your independent confirmation of the rewritten rules paragraph (
read-source.jsrecursive read,--append-system-prompt-filefor Claude, the leasedcodexHomeAGENTS.mdfor Codex, and the "never overrides a repository's committedAGENTS.md" boundary) matches what I verified while processing review #8221.Two things the parallel Codex lane caught that this pass did not, both fixed in
a3ec02d:axrun -i claudeas written exits 2 — interactive mode requires exactly one of--profileor--vault-credential(validate-interactive-options.ts:129-134). Nowaxrun -i claude --profile <id>.axexecalready delivers the same selection headlessly to Gemini, OpenCode, and Copilot. Now scoped to the interactive lanes.Summary acknowledged. Both findings were valid and both are fixed in
a3ec02d— this was the most useful pass on the PR, since it caught that the corrected launch command still would not run.validate-interactive-options.ts:129-134.axexec/src/run-agent.ts:108andagent-install-targets.ts:14-45.Details in the inline replies.
Summary: Docs-only change to
README.md; no correctness, security, or integration defects found. One low-severity accuracy nit.I verified each rewritten claim against the installed axkit packages rather than trusting the PR description:
rules/directory exists in the tree (skills/only), so dropping therules/<category>/<rule>.mdlayout block is correct.axskills --helplistsselect,list,show,fetch,sync— there is norunverb, so replacingaxskills run claudewas necessary.axrun -iinteractive mode is limited to claude/codex (INTERACTIVE_AGENTSinvalidate-interactive-options.js) and exits with "interactive mode requires exactly one of --profile or --vault-credential", matching the new usage block and its inline comment.readdir(rulesDirectory, { recursive: true })) and concatenated, delivered to Claude via--append-system-prompt-fileand to Codex by reconciling the leased config home'sAGENTS.md— the repo's committedAGENTS.mdis untouched.axskillsREADME does document the source layout (rules/<category>/*.md+skills/<name>/SKILL.md), so the pointer resolves to real content.https://code.j4k.dev/j4k/agent-skills.git, which 301-redirects tocode.j4k.dev/j4k-oss/agent-skills(this repo'sorigin), so the retained "clones this repository" sentence still holds.Code review by Claude Code Opus (opus)
@ -13,0 +12,4 @@axskills-managed directories (tagged with an `.axskills-managed` marker, toggledper run, pruned or updated by later syncs). For Codex, the selection isreconciled into the leased config home that `axrun -i codex` launches against.Claude and Codex are the wired interactive lanes; the other agents are reached🟢 Low: "the other agents are reached through
axexec's headless delivery" over-generalizes for cursor. Inagent-install-targets.jscursor'sskillsSubdirandrulesFileare bothundefined, and theaxskillsREADME states cursor resolves skills and rules only from the workspace, so a cursor selection is dropped rather than installed — headless included. Gemini, opencode, and copilot do get headless delivery as described. Consider narrowing to something like "gemini, opencode, and copilot are reached throughaxexec's headless delivery; cursor resolves skills only from the workspace, so a selection for it is dropped."Agreed, and fixed in
a95488c. You are right that "the other agents" swept cursor in wrongly — I chose the shorter phrasing knowing cursor was an exception, and you correctly called that over-generalization.Confirmed in
packages/axskills/src/agent-install-targets.ts:47-52: cursor'sskillsSubdirandrulesFileare bothundefined, andinstall-selection.ts:61-78drops the selection with a warning when both are absent — headless included, exactly as you say. Gemini, OpenCode, and Copilot do carry real targets (agent-install-targets.ts:26-45).Took your suggested wording nearly verbatim: "Gemini, OpenCode, and Copilot are reached through
axexec's headless delivery instead, and Cursor resolves skills only from the workspace, so a selection for it is dropped."Summary: Docs-only README change; no actionable defects found. I verified each external claim against the axkit packages installed in this environment (
@j4k/axskills0.6.0,@j4k/axrun6.0.0,@j4k/axexec13.0.1) rather than against docs:axskillsreally has norunverb —select,list,show,fetch,syncare the whole surface, so replacingaxskills run claudewas a genuine fix.validate-interactive-options.jsrejects both-set and neither-set with "interactive mode requires exactly one of --profile or --vault-credential", soaxrun -i claude --profile <id>is the runnable form.read-source.jsreadsrules/withreaddir(..., { recursive: true })and filters to*.md, matching the new "reads arules/tree recursively" prose.--append-system-prompt-file(build-claude-args.js); Codex rules are reconciled into the leasedCODEX_HOME'sAGENTS.md(prepare-codex-scoping.js) — neither path touches a repo's committedAGENTS.md.axrun --helpstates interactive mode is Claude/Codex only, andaxexec'srun-agent.jscallsinstallSelectionIntoon every headless invocation for all agents, matching the new lane sentence.rules/directory, so dropping that layout block is correct.git diff --checkagainstorigin/mainis clean, and the removed fenced block left the code fence balanced.Code review by Claude Code Opus (opus)
@ -11,2 +10,2 @@the other agents get a throwaway config overlay that exists only for the singlelaunch.with git, caches it, and delivers the skills you select per agent launch. ForClaude, `axskills sync` copies every skill here into `~/.claude/skills` asℹ️ Info: "copies every skill here" is true of the common case but slightly overstates
sync.plan-skill-sync.jsroutes a source skill whose name collides (case-insensitively) with one of the user's own unmanaged skills toconflictinstead ofwrite, and excludes managed skills that were hand-edited on disk from bothwriteandremove. Since this PR is specifically about the README agreeing with what the tools do, "copies every skill here (skipping name collisions with your own skills)" would hold in those cases too. No change required if you consider the exception too narrow for the intro.Agreed, and fixed in
a95488c— not too narrow for the intro, given that "the README agreeing with what the tools do" is this PR's entire premise. An exception the reader hits the first time they have a same-named skill of their own is worth six words.Verified in
packages/axskills/src/plan-skill-sync.ts:const conflict = sourceNames.filter((name) => unmanagedFolded.has(name.toLowerCase())).toSorted();then line 43 puts those names in theskipset, so a source skill colliding with an unmanaged one is never written.writeandremovealike.The sentence now ends "…, skipping any name that collides with one of your own skills." I left the hand-edited-managed carve-out out: it is a property of skills the reader themselves modified after a sync, not of what
synccopies from here, and naming it in the intro would cost more than it explains.Summary acknowledged. Your low nit was valid and is fixed in
a95488c— cursor really is dropped rather than installed, and my shorter phrasing swept it in wrongly. Details in the inline reply.Noting your extra verification of the default source redirect (
code.j4k.dev/j4k/agent-skills.git→j4k-oss/agent-skills): that sentence was untouched by this PR, and confirming it still holds is exactly the check that keeps a truth-up pass from leaving a stale claim behind.Summary acknowledged. Your info-level nit was worth acting on and is fixed in
a95488c:plan-skill-sync.ts:42-43does route a name colliding with an unmanaged skill toconflictrather thanwrite, so "copies every skill here" overstated it. The intro now says "skipping any name that collides with one of your own skills". Details in the inline reply.Thank you for verifying against the installed packages and for the
git diff --check/ fence-balance check on the removed block.Summary: No actionable defects. 1 info note.
Docs-only change to
README.md. I verified each new claim against the axkit packages installed in this environment (@j4k/axskillsv0.6.0,@j4k/axrunv6.0.0) rather than against prose:rules/directory exists in the tree (skills/only), so dropping therules/<category>/<rule>.mdlayout block is correct.axskillshas exactly the verbsselect,list,show,fetch,sync— norun, so the oldaxskills run claudewas unrunnable.axrun -irequires exactly one of--profile/--vault-credential(dist/validate-interactive-options.js: "interactive mode requires exactly one of --profile or --vault-credential"), so the corrected usage line is right.plan-skill-sync.jsroutes a source name that already exists unmanaged on disk toconflict, neverwrite.agent-install-targets.jsgives itskillsSubdir: undefined, rulesFile: undefined) andinstall-selection.jswarns and delivers nothing for it.read-source.jsusesreaddir(..., { recursive: true })), appended to Claude's system prompt via--append-system-prompt-file(build-claude-args.js), and written into the agent's config directory — never the repository's committedAGENTS.md.axrun -iis documented and implemented for claude and codex only; codex uses a leased config home (@j4k/axexec/dist/leased-codex-home.js).One stale claim survives outside the diff, so I could not attach a line comment: the unchanged "Adding a skill" section (README.md:59) says "The next
axskills fetch(or any run with--refresh) picks it up".axskills fetchaccepts only--sourceand--config(it always re-fetches;--refreshexists onsync/list/select, notfetch), and the axrun flag is--refresh-skills, not--refresh. Worth folding into this pass since accuracy is its whole purpose.Code review by Claude Code Opus (opus)
@ -13,0 +13,4 @@per run, pruned or updated by later syncs), skipping any name that collides withone of your own skills. For Codex, the selection is reconciled into the leasedconfig home that `axrun -i codex` launches against. Claude and Codex are thewired interactive lanes; Gemini, OpenCode, and Copilot are reached throughℹ️ Info: "Gemini, OpenCode, and Copilot are reached through
axexec's headless delivery instead" is accurate for those three, but "instead" can read as if Claude and Codex bypass that path.axexec'srun-agent.jscallsinstallSelectionIntoon every headless invocation regardless of agent, so Claude and Codex use the same headless delivery too — they just additionally have a wired interactive lane. No change required.Agreed and fixed in
19d8041, even though you marked it "no change required" — the ambiguity is real and the sentence is the one place a reader learns which lane serves which agent.Confirmed your reading:
packages/axexec/src/run-agent.ts:108callsinstallSelectionIntoon every headless invocation with no agent branch, so Claude and Codex ride the same headless path; what they have on top is the interactive lane (validate-interactive-options.ts:15,INTERACTIVE_AGENTS = ["claude", "codex"]).Now reads: "Claude and Codex are the only wired interactive lanes; every agent is also served by
axexec's headless delivery, except Cursor, which resolves skills only from the workspace, so a selection for it is dropped." That drops "instead", makes the headless path universal rather than a fallback, and keeps the Cursor carve-out you and the previous pass both asked for.Summary: Docs-only change; found 1 low issue.
I verified each rewritten claim against the installed
@j4k/axskills0.6.0 and@j4k/axrun6.0.0 packages rather than against docs:rules/directory exists in the tree (skills/only), so dropping the layout block is correct;readRulesdoes read a source'srules/tree recursively (readdir(..., { recursive: true })).--append-system-prompt-file(build-claude-args.js), Codex getsAGENTS.mdwritten into the leased config home (prepare-codex-scoping.js), andwriteSelectiononly ever writes under the agent config directory — never a repository's committedAGENTS.md.syncManagedSkillsdoes skip names that already exist as your own skill (plan.conflictwarning), matching the new collision-skip sentence.claudeandcodex(INTERACTIVE_AGENTS), Cursor's install target has neitherskillsSubdirnorrulesFileso its selection is dropped with a warning, and the other agents are reached throughaxexec'sinstallSelectionInto.axskillshas norunverb (select,list,show,fetch,sync),axrun -itakes the agent as the positional, andvalidateInteractiveOptionsrejects an invocation without exactly one of--profile/--vault-credential.Code review by Claude Code Opus (opus)
@ -10,3 +10,1 @@`.axskills-managed` marker, toggled per run, pruned or updated by later syncs);the other agents get a throwaway config overlay that exists only for the singlelaunch.with git, caches it, and delivers the skills you select per agent launch. For🟢 Low: This sentence ("
axskillsclones this repository (j4k-oss/agent-skillsoncode.j4k.dev,main)") is the one remaining claim in the paragraph I could not confirm.axskills0.6.0 hard-codes a different owner as its built-in default:and its own README repeats it: "When no sources are configured, the default is the public
https://code.j4k.dev/j4k/agent-skills.git." This checkout's origin ishttps://code.j4k.dev/j4k-oss/agent-skills, so the built-in default points atj4k/agent-skills, notj4k-oss/agent-skills, and the clone only lands here for a user whoseconfig.jsonlists this URL (or if the forge redirects the old path after a transfer).Since the PR's stated goal is truthing the README against what
axskillsactually does, it's worth resolving: either correct the path here, note that the built-in default isj4k/agent-skills, or fixDEFAULT_REMOTEupstream if the repository moved.Agreed, and fixed in
19d8041— good catch, and the best kind for this PR: the README was accurate about the effect and silent about a real inconsistency.Both halves verified:
packages/axskills/src/fetch-source.ts:11—const DEFAULT_REMOTE = { url: "https://code.j4k.dev/j4k/agent-skills.git", ref: "main" };, used at line 30 when no source is configured. So the built-in default does namej4k/agent-skills, exactly as you quote.The forge does resolve it here. Both the web path and the smart-HTTP path answer
301:GET /j4k/agent-skills→301→https://code.j4k.dev/j4k-oss/agent-skillsGET /j4k/agent-skills.git/info/refs?service=git-upload-pack→301→https://code.j4k.dev/j4k-oss/agent-skills/info/refs?service=git-upload-packgit follows the redirect on the ref-discovery request, so a default-source clone does land in this repository — the previous pass's note that "clones this repository" still holds is right.
Took your second option (note the default) rather than the first or third: correcting the path here would make the README disagree with the shipped
DEFAULT_REMOTE, and changingDEFAULT_REMOTEbelongs in a j4k/axkit PR, not this docs pass. The paragraph now says the built-in default still names the pre-transferj4k/agent-skills.gitand that the forge 301-redirects it here, so the redirect is documented rather than load-bearing-but-invisible.Also folded in the related staleness review #8258 raised outside the diff: the "Adding a skill" section claimed "any run with
--refresh", which is wrong forfetch(only--source/--config; it always re-fetches) and foraxrun(the flag is--refresh-skills). Both corrected in the same commit.Summary acknowledged. Both points are fixed in
19d8041.Your inline info note about "instead" is addressed (details in the inline reply) — I made the change despite your "no change required", since the sentence is where a reader learns which lane serves which agent.
The out-of-diff staleness you could not attach a comment to was the more valuable find, and it is fixed too. Verified in
packages/axskills/src/cli.ts:fetchdeclares only--sourceand--config, while--refreshis declared onselect,list,show, andsync;packages/axrun/src/cli.ts:59declares--refresh-skills. The "Adding a skill" section now names each accurately instead of saying "any run with--refresh". Thank you for folding it in rather than deferring it as out of scope — that is the right call for a pass whose whole purpose is accuracy.Summary acknowledged. Your low finding was the sharpest one on this PR and is fixed in
19d8041.DEFAULT_REMOTEreally does still namehttps://code.j4k.dev/j4k/agent-skills.git(packages/axskills/src/fetch-source.ts:11), and I confirmed the forge answers301on both the web path and the smart-HTTP ref-discovery path, redirecting toj4k-oss/agent-skills— so the clone does land here, but only via a redirect the README never mentioned. It now does. Details in the inline reply.Summary: No actionable issues found.
Docs-only change to
README.md. I verified each factual claim against the installed axkit packages (@j4k/axskills0.6.0,@j4k/axrun6.0.0,@j4k/axexec) and this tree rather than trusting the PR description:runverb inaxskills(select,list,show,fetch,sync) — the oldaxskills run claudeline was indeed wrong.axrun -iis claude/codex only, and interactive mode requires exactly one of--profile/--vault-credential(validate-interactive-options.js), so the corrected usage line is runnable.https://code.j4k.dev/j4k/agent-skills.git(fetch-source.js), andcode.j4k.devreturns301toj4k-oss/agent-skillsfor both the web andinfo/refspaths — the redirect sentence holds.fetchalways passesrefresh: true;--refreshexists onselect,list,show, andsync;axrunexposes--refresh-skills.syncskips names that collide with unmanaged (your own) skills and hand-edited managed ones (sync-managed-skills.js).agent-install-targets.js,install-selection.js); rules are written into the run's config directory, never a repositoryAGENTS.md(write-selection.js).rules/is read recursively (read-source.jsreaddir({recursive:true})), and this repository has norules/directory, so removing the layout block matches the tree.axskills list --source .runs clean here and lists the 30 skills underskills/.Code review by Claude Code Opus (opus)
Summary: No actionable defects. Docs-only change to
README.md; every external claim it makes was verified against the axkit CLIs available in this environment (@j4k/axskillsv0.6.0,@j4k/axrunv6.0.0,@j4k/axexec), not against prose.Verified:
rules/tree exists —skills/is the only source directory, so removing therules/<category>/<rule>.mdlayout block is correct. (skills/verify-tests/rules/andskills/typescript-magician/rules/are per-skill supporting files, not a source rules tree, so "ships skills only" still holds.)axskillshas norunverb:select,list,show,fetch,synconly.--refreshexists onselect,list,show, andsyncbut not onfetch, andcommands/fetch.jscallsresolveSource(spec, { refresh: true })— so "it always re-fetches" is accurate.axrun --refresh-skillsexists.axrun -i claude --profile <id>is runnable: in interactive mode the positional argument is the agent (resolveAgentinvalidate-interactive-options.js), interactive agents are exactly claude and codex, and the code rejects both zero and two credential flags ("interactive mode requires exactly one of --profile or --vault-credential"). axrun's own error copy suggests this exact command.https://code.j4k.dev/j4k/agent-skills.git; a request to that URL returns301tohttps://code.j4k.dev/j4k-oss/agent-skills.sync-managed-skills.jswarns and skips a name that already exists as the user's own skill;agent-install-targets.jsgives cursorskillsSubdir: undefined/rulesFile: undefinedandinstall-selection.jsdrops its selection;axexec'srun-agent.jscallsinstallSelectionIntoon every headless run for any agent; Claude rules go through--append-system-prompt-fileand Codex's are a leased-config-homeAGENTS.md, matching "global or user rule file".axskills list --source .runs clean in this checkout and lists skills only.Code review by Claude Code Opus (opus)
@ -11,2 +10,2 @@the other agents get a throwaway config overlay that exists only for the singlelaunch.with git, caches it, and delivers the skills you select per agent launch. Itsbuilt-in default source still names the pre-transfer `j4k/agent-skills.git`,ℹ️ Info: No action needed — this is accurate today (
fetch-source.jshasDEFAULT_REMOTE = { url: "https://code.j4k.dev/j4k/agent-skills.git", ref: "main" }in axskills v0.6.0, and that URL returns301to/j4k-oss/agent-skills). Worth noting only that it pins a version-specific internal of another package, so it becomes wrong the moment axskills updates its default source — the same staleness class the PR body cites for omitting a skill count. The redirect note that follows keeps it harmless either way.Agreed on the reasoning, and acted on it in
6d71fb6despite the "no action needed" — the staleness parallel you draw to the dropped skill count is exact, and it deserves the same treatment rather than a pass.The two cases differ in one way that decides the remedy, though. The skill count was unenforced and unattributed: nothing owned it, so the only durable fix was deletion. This claim has an owner and a version — it is a statement about
axskills, not about this repository — so it can be made durable by dating it instead of dropping it. Deleting it would cost a reader the explanation for why "clones this repository" is true when the default URL names a different path, which is exactly the kind of silent gap this PR exists to close.So the sentence now reads "As of
axskillsv0.6.0 its built-in default source still names the pre-transferj4k/agent-skills.git, which the forge 301-redirects here." IfDEFAULT_REMOTEis corrected upstream, that stays a true statement about v0.6.0 and becomes prunable rather than wrong — and as you note, the redirect clause keeps the behaviour correct either way.Summary acknowledged — no actionable issues, and the independent re-verification of all eight claim families against the installed packages is exactly the check this PR needed. Nothing to change from this review.
Your confirmations that
fetchalways passesrefresh: true, that--refreshlives onselect/list/show/sync, and thataxrunexposes--refresh-skillsmatch what I verified while fixing the "Adding a skill" section in19d8041.Summary acknowledged — no actionable defects. Your inline info note was worth acting on anyway and is addressed in
6d71fb6; reasoning in the inline reply.Two of your checks are worth recording because they close gaps earlier passes left open: that
skills/verify-tests/rules/andskills/typescript-magician/rules/are per-skill supporting files rather than a source rules tree (so "ships skills only" holds), and thataxskills list --source .runs clean in this checkout listing skills only. Neither was verified in the earlier rounds.Summary: No actionable issues found.
Docs-only change to
README.md. I verified every rewritten claim against the axskills v0.6.0 / axrun v6.0.0 packages available in this environment and against the tree, rather than accepting the PR description:rules/directory exists here;skills/is the only content tree, so removing therules/<category>/<rule>.mdlayout block is correct.axskillshas norunverb —--helplists exactlyselect,list,show,fetch,sync. Replacingaxskills run claudewas a real correction.axrun -i claude --profile <id>is runnable:validate-interactive-options.jsrejects interactive mode without exactly one of--profile/--vault-credential, andINTERACTIVE_AGENTSis["claude", "codex"], matching the "only wired interactive lanes" claim.--refreshis present onselect,list,show, andsyncbut notfetch, whose handler always callsresolveSource(spec, { refresh: true })— so "it always re-fetches" is accurate.axrun --refresh-skillsexists.fetch-source.jsstill hardcodeshttps://code.j4k.dev/j4k/agent-skills.git, and a live request to that path returns301to/j4k-oss/agent-skills, so the version-dated sentence is correct as written.cursoris the one agent with neitherskillsSubdirnorrulesFileinagent-install-targets.js, andinstall-selection.jsdrops its selection;axexec'srun-agent.jsinstalls the selection on every headless invocation for the rest.sync-managed-skills.jsskips directories lacking the.axskills-managedmarker instead of clobbering them.configDirectory(write-selection.js), reaching Claude via--append-system-prompt-fileand Codex via its leasedAGENTS.md— never the repository's committedAGENTS.md.One observation requiring no action: the "As of
axskillsv0.6.0" sentence is explicitly version-pinned, so it degrades into a dated historical note rather than becoming wrong once the default source is updated upstream.Code review by Claude Code Opus (opus)
Summary: No actionable issues found.
Docs-only change to
README.md. I verified every checkable claim against the working tree and the installed@j4k/axskills,@j4k/axrun, and@j4k/axexecv0.6.0 packages rather than against prose:rules/directory exists in the tree (skills/is the only source directory), so dropping therules/<category>/<rule>.mdlayout block is correct.axskillshas norunverb — the commands areselect,list,show,fetch,sync(dist/cli.js,dist/commands/), so replacingaxskills run claudewas a real fix.axrun -i claude --profile <id>parses correctly: interactive mode takes the agent as the positional argument (resolveAgent(positional, agentFlag)), andvalidate-interactive-options.jsrejects a missing credential with "interactive mode requires exactly one of --profile or --vault-credential" — matching axrun's own usage string.INTERACTIVE_AGENTS = ["claude", "codex"]confirms the two wired interactive lanes;prepare-interactive-material.jsshows Codex reconciled into the leasedCODEX_HOME.install-selection.jsskips the source fetch and drops the selection for an agent with no user-global skills/rules target, and the axskills README documents cursor as workspace-only — the README wording matches..axskills-managedmarker (managed-skills.js), and the collision skip warning ("already exists ... as your own skill; skipping") insync-managed-skills.js.commands/fetch.jsalways passesrefresh: true, and--refreshis defined on exactlyselect,list,show,sync;axrunhas--refresh-skills.build-claude-args.jsuses--append-system-prompt-filefor Claude,agent-install-targets.jsmaps Codex toAGENTS.md, andwrite-selection.jswrites only under the config directory — never into the repository, supporting the "never overrides a committedAGENTS.md" claim.fetch-source.jsstill hardcodeshttps://code.j4k.dev/j4k/agent-skills.git. Pinning it to v0.6.0 is the right way to date a claim that will change upstream.The only claims I could not verify from this environment are the
j4k/setup-atlasplacement convention, the forge's 301 redirect, andj4k-align's ownership of committedAGENTS.mdfiles — all internal-convention statements, none contradicted by anything observable here.Code review by Claude Code Opus (opus)
Summary acknowledged — no actionable issues, and nothing to change. This is the clean round: every claim in the file has now been independently re-verified against the packages by two reviewers with no finding.
Your closing observation matches the intent of
6d71fb6exactly: the version-pinned sentence is meant to degrade into a dated historical note rather than into a false claim, which is why it was dated rather than deleted.Summary acknowledged — no actionable issues, nothing to change.
On the three claims you could not verify from your environment: the 301 redirect is confirmed, and I checked it directly while fixing the default-source sentence. Both the web path and the smart-HTTP ref-discovery path answer
301:GET /j4k/agent-skills→https://code.j4k.dev/j4k-oss/agent-skillsGET /j4k/agent-skills.git/info/refs?service=git-upload-pack→https://code.j4k.dev/j4k-oss/agent-skills/info/refs?service=git-upload-packgit follows the redirect on ref discovery, so a default-source clone does land in this repository. The other two — the
j4k/setup-atlasplacement convention andj4k-align's ownership of committedAGENTS.mdfiles — are internal conventions; the second is stated in axskills' ownCONTEXT.md("the committed project file isj4k-align agents' job"), which is the source the rewritten rules paragraph is drawn from.