feat(fx-dev)!: standardize on AGENTS.md and REVIEW.md - #18
Merged
Conversation
Every skill that read or wrote project conventions used a different file: resolvers wrote recurrence rules to .github/copilot-instructions.md, readers looked at CLAUDE.md, and Codex was unmodelled entirely. Consolidate on two canonical files with pointers for the tools that cannot read them. Canonical: - AGENTS.md — project conventions (how code is written) - REVIEW.md — review conventions (what reviewers flag) Pointers, one per tool that needs a bridge: - CLAUDE.md is a single `@AGENTS.md` import (Claude Code does not read AGENTS.md) - .github/copilot-instructions.md is a symlink to ../REVIEW.md (Copilot code review does not follow file references, so the path it looks for must be the file itself) - AGENTS.md carries a `## Code Review Rules` section pointing at REVIEW.md (Codex reads only AGENTS.md) - .coderabbit.yaml lists **/REVIEW.md (not in CodeRabbit's defaults) All feedback resolvers now write INCORRECT-category rules to REVIEW.md, so suppressing one false positive suppresses it for Copilot, CodeRabbit, Codex, and Claude Code Review at once. Adds skills/setup/references/instruction-files.md as the single source of truth for the layout, the per-tool support matrix, and the migration path. The setup skill scaffolds and migrates it; this repo is converted to it as well. BREAKING CHANGE: skills now read AGENTS.md and write REVIEW.md instead of CLAUDE.md and .github/copilot-instructions.md. Run fx-dev:setup in existing projects to migrate.
- codex-review no longer invokes fx-dev:setup; a missing AGENTS.md pointer is reported, keeping the pre-PR review read-only instead of scaffolding docs/ and CodeRabbit config onto the branch mid-review - resolve-pr-feedback aggregates REVIEW.md rules from parallel resolvers and applies them in one serialized edit; re-read-before-write is not locking and concurrent sub-agents could silently drop each other's rules - the core.symlinks=false mirror is now a standing sync obligation with a regeneration snippet, wired into both feedback resolvers; a stale mirror enforces retracted rules - instruction-file migration falls back to plain mv when the source file is untracked, since git mv aborts on untracked paths during first-time setup
…aging edge cases - setup checks `test -L CLAUDE.md` before any other case: writing through a symlinked CLAUDE.md would edit AGENTS.md and produce a self-import - setup readlinks an existing .github/copilot-instructions.md symlink and merges a non-REVIEW.md target before unlinking, instead of orphaning it - symlink verification no longer runs `git add`; staging is the caller's job and must cover the whole migration, so the symlink cannot land without its target - resolve-pr-feedback verifies aggregated rules with `git diff -- REVIEW.md` rather than a total rule count, which always fails on an established file
- setup canonicalizes any legacy instruction symlink with readlink -f and refuses to copy targets outside the checkout. A CLAUDE.md symlinked to ~/.claude/CLAUDE.md would otherwise inline the user's private machine-wide instructions into a tracked, possibly public, AGENTS.md - codex-review regenerates the generated-mirror fallback after writing REVIEW.md, matching the Copilot and CodeRabbit resolvers - resolve-pr-feedback regenerates the mirror in the root session, since the aggregated write means the resolvers' own regeneration steps never run
- setup captures and validates a legacy CLAUDE.md symlink target before removing the link; unlinking first made in-repo conventions unrecoverable and left external targets unreportable - canonicalization uses a portable realpath -> readlink -f -> python3 chain. macOS readlink has no -f, so the previous check misreported every target as unresolvable and declined to migrate in-repo conventions - Step 9 no longer assumes the symlink exists: on the generated-mirror fallback path, CodeRabbit defaults never reach root REVIEW.md, so setup now writes a .coderabbit.yaml listing it explicitly
…usive setup runs automatically on every spec-writer and project-management invocation, so a non-idempotent migration path recurs every single run. - 6.1 opens with a guard that short-circuits when AGENTS.md is a regular file and CLAUDE.md already holds the @AGENTS.md pointer. Without it, the pointer was classified as content to merge and appended into AGENTS.md, making the file import itself on every run - AGENTS.md is now checked for being a symlink before any merge or append, with the same repo-boundary rule as CLAUDE.md; writing through it would edit the target and could pull external content into the repo - the CLAUDE.md-only migration path splits out Claude-only rules instead of publishing them wholesale to Codex, Copilot, and CodeRabbit - the mirror fallback deletes the symlink it replaces. core.symlinks=false on a filesystem that still allows ln -s left both in place, and the mirror regeneration snippet is guarded on the symlink's absence, so refreshes were silently disabled
…light Each review round found the same symlink-boundary rule missing on one more path. Fix the class instead of the instances: new Step 5.5 classifies AGENTS.md, CLAUDE.md, REVIEW.md, and .github/copilot-instructions.md in one pass before Steps 6-8 touch anything, so every path they see is a regular in-repo file or absent. The preflight covers the two failure modes uniformly: - symlinks escaping the checkout (classically ~/.claude/CLAUDE.md): never read or copied, deleted and reported instead - materialized pointers, the plain-text file core.symlinks=false produces whose entire content is a path: deleted, never merged as a rule Also branches the CodeRabbit config decision on whether the symlink exists; on the generated-mirror fallback the defaults never reach root REVIEW.md, so the resolver now writes an explicit .coderabbit.yaml.
There was a problem hiding this comment.
Pull request overview
This PR standardizes instruction-file handling across fx-dev skills by establishing AGENTS.md (project conventions) and REVIEW.md (review conventions) as the canonical sources, with tool-specific pointers (a CLAUDE.md import pointer and a Copilot instructions symlink) and updated resolver/reader behavior to consistently target the canonical files.
Changes:
- Introduces canonical root instruction files (
AGENTS.md,REVIEW.md) and converts rootCLAUDE.mdinto an@AGENTS.mdpointer; updates repo entry points (README, index.html) accordingly. - Updates
fx-dev:setupto preflight symlink/materialized-pointer edge cases and to scaffold/migrate the new instruction-file layout. - Aligns fx-dev skills/resolvers/readers to read conventions from
AGENTS.mdand to write recurrence-prevention rules toREVIEW.md, with plugin/marketplace version bumps.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| REVIEW.md | Adds repo-wide PR review conventions as the canonical review rules. |
| README.md | Updates development docs to point to AGENTS.md / REVIEW.md. |
| plugins/fx-meta/skills/learn/SKILL.md | Updates the skill to read AGENTS.md (and clarifies CLAUDE.md / REVIEW.md roles). |
| plugins/fx-meta/.claude-plugin/plugin.json | Bumps fx-meta plugin version for the updated skill instructions. |
| plugins/fx-dev/skills/upstream-contrib/SKILL.md | Updates upstream contribution flow to prefer AGENTS.md with a fallback to CLAUDE.md. |
| plugins/fx-dev/skills/spec-writer/SKILL.md | Updates setup expectations and references from legacy instruction files to the new canonical ones. |
| plugins/fx-dev/skills/setup/SKILL.md | Implements the instruction-file standard migration/scaffolding, including new Step 5.5 preflight. |
| plugins/fx-dev/skills/setup/references/instruction-files.md | Adds the detailed instruction-file standard and tool matrix (including symlink fallback guidance). |
| plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md | Documents shared convention that all resolvers write INCORRECT recurrence rules to REVIEW.md and discusses parallel-write avoidance. |
| plugins/fx-dev/skills/resolve-codecov-feedback/SKILL.md | Updates coverage-requirement source from CLAUDE.md to AGENTS.md. |
| plugins/fx-dev/skills/requirements-analyzer/SKILL.md | Updates conventions sources to AGENTS.md / REVIEW.md (and clarifies CLAUDE.md usage). |
| plugins/fx-dev/skills/rabbit-feedback-resolver/SKILL.md | Updates CodeRabbit configuration guidance to ensure REVIEW.md/AGENTS.md are read and recurrence rules go to REVIEW.md. |
| plugins/fx-dev/skills/project-management/SKILL.md | Updates external task-tracking guidance references from CLAUDE.md to AGENTS.md. |
| plugins/fx-dev/skills/project-management/references/tasks-md-template.md | Updates template wording to reference AGENTS.md for external tracking preferences. |
| plugins/fx-dev/skills/pr-reviewer/SKILL.md | Updates required pre-review reads to AGENTS.md + REVIEW.md (and CLAUDE.md for Claude-only additions). |
| plugins/fx-dev/skills/pr-preparer/SKILL.md | Updates compliance checks to use AGENTS.md. |
| plugins/fx-dev/skills/planner/SKILL.md | Updates planning inputs to use AGENTS.md. |
| plugins/fx-dev/skills/copilot-feedback-resolver/SKILL.md | Redirects INCORRECT recurrence-prevention updates from copilot-instructions to canonical REVIEW.md (including mirror fallback guidance). |
| plugins/fx-dev/skills/codex-review/SKILL.md | Documents Codex’s AGENTS.md-only behavior and the pointer requirement to REVIEW.md. |
| plugins/fx-dev/skills/coder/SKILL.md | Updates coder standards to follow AGENTS.md. |
| plugins/fx-dev/README.md | Updates fx-dev plugin docs to explain the AGENTS.md / REVIEW.md split and resolver write target. |
| plugins/fx-dev/.claude-plugin/plugin.json | Bumps fx-dev major version for the breaking instruction-file behavior change. |
| index.html | Updates marketplace landing links to point to AGENTS.md and adds REVIEW.md. |
| CLAUDE.md | Converts legacy full guidelines into a single @AGENTS.md pointer. |
| AGENTS.md | Adds canonical project-conventions file (migrated from legacy CLAUDE.md) and includes REVIEW.md pointer section. |
| .githooks/pre-commit | Updates top-level-file detection comment to reflect AGENTS.md/REVIEW.md replacing CLAUDE.md. |
| .claude-plugin/marketplace.json | Bumps marketplace metadata version due to top-level and plugin changes. |
Comments suppressed due to low confidence (1)
plugins/fx-dev/skills/setup/SKILL.md:274
- In Step 8.1,
test -f .github/copilot-instructions.mdis true for both a regular file and a symlink to an existing file, so the echo text "is a regular file" can be wrong and the preflight can’t reliably distinguish the “correct symlink” exception called out just above. Usetest -L(or-f+! -L) so the migration logic branches correctly.
```bash
test -f REVIEW.md && echo "REVIEW.md exists" || echo "REVIEW.md missing"
test -f .github/copilot-instructions.md && echo "copilot-instructions is a regular file" || echo "copilot-instructions absent"
</details>
GitHub reflows markdown to the reader's viewport, so hard-wrapping a PR description at 80 columns only renders ragged and re-wraps badly on narrow screens. Nothing in the repo actually asked for it, but nothing forbade it either, and PR #18's body was written that way unprompted. The rule follows the renderer, not the content: commit bodies stay wrapped at ~72 columns because git renders them as plain text; anything GitHub renders as markdown gets one long line per paragraph. Documented in the github skill's messaging conventions and referenced from pr-preparer, which is where PR bodies are authored.
…reflight Step 5.5 said its symlink handling applied identically to all four paths, and the action table sent every in-repo symlink to 'replace with a regular file'. Applied to .github/copilot-instructions.md -> ../REVIEW.md that deletes the exact link Step 8.3 exists to create, and it contradicted Step 8.1, which already documented that symlink as a valid end state. Because setup runs automatically on every spec-writer and project-management call, this would have churned the symlink on every invocation. The preflight now checks for the correct symlink first and excludes it, and the table carries the exception explicitly. Also replaces the plausible-looking org and service names in the AGENTS.md 'bad examples' block with angle-bracket placeholders. A don't-do-this illustration should not itself contain a realistic-looking private name.
The file-review step named a 'global user-level' CLAUDE.md as something to examine. That resolves to ~/.claude/CLAUDE.md, which is private, machine-wide, and routinely holds internal identifiers — reading it risks quoting that content into a spec, issue, or PR artifact. Scoped the list to repo files and added an explicit prohibition on reading or quoting home-level instructions.
…d natively GitHub shipped native REVIEW.md support for Copilot code review on 2026-07-17: 'Copilot code review now reads REVIEW.md, GEMINI.md, and CLAUDE.md files from your repository.' That postdates the docs support-matrix page this standard was built against, and it removes the only reason the bridge existed. Deleted, because Copilot was their sole consumer: - the .github/copilot-instructions.md -> ../REVIEW.md symlink - the generated-mirror fallback for core.symlinks=false checkouts, its regeneration snippet, and the resolver wiring that kept it in sync - the preflight's materialized-pointer detection and its 'one symlink that is supposed to exist' exception The preflight keeps its real job: classifying AGENTS.md, CLAUDE.md, and REVIEW.md so a legacy symlink is not written through, and refusing to copy a target that escapes the checkout. .coderabbit.yaml becomes mandatory rather than conditional. It was previously optional because CodeRabbit could reach REVIEW.md through the symlink via its default copilot-instructions pattern; without the symlink, the explicit **/REVIEW.md pattern is the only path. Added one to this repo accordingly. Minor rather than patch: the scaffolded layout changes shape.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
plugins/fx-dev/.claude-plugin/plugin.json:4
- The fx-dev plugin version is set to 3.1.0 here, but the PR description/testing notes say the shipped fx-dev version is 3.0.3. Please align the PR description with the actual version being released (or adjust this version if 3.0.3 is the intended release).
"name": "fx-dev",
"version": "3.1.0",
"description": "Complete development workflow including SDLC, pull requests, git utilities, and GitHub integration"
.claude-plugin/marketplace.json:11
- marketplace.json metadata.version is 0.12.0, but the PR description/testing notes state the marketplace version bump is 0.10.0 → 0.11.3. Please reconcile the PR description with the actual manifest version (or adjust this version if 0.11.3 is the intended release).
"metadata": {
"description": "Personal Claude Code plugins, skills, and subagents",
"version": "0.12.0",
"homepage": "https://cc.fx.gd"
},
plugins/fx-dev/skills/rabbit-feedback-resolver/SKILL.md:124
- This “Minimal config when you have no custom
filePatterns” section is internally inconsistent: it showsenabled: truealone, but CodeRabbit won’t readREVIEW.mdunless afilePatternsentry explicitly includes it. As written, readers may apply the minimal config and still miss the canonical review conventions.
**Minimal config when you have no custom `filePatterns`:**
```yaml
knowledge_base:
code_guidelines:
The state table still said 'enabled: true, no custom filePatterns -> no action needed', which was true only while the copilot-instructions symlink existed to carry CodeRabbit to REVIEW.md via its default patterns. With the symlink gone that state leaves CodeRabbit reviewing without the review conventions, directly contradicting the paragraph above it. Now the only no-action state is one where **/REVIEW.md is already present, and the 'minimal config' example that showed enabled: true alone is removed rather than annotated as insufficient.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
plugins/fx-dev/.claude-plugin/plugin.json:4
- PR description says fx-dev is bumped to 3.0.3, but the manifest version here is 3.1.1. Please align the PR description/testing notes with the shipped version (or adjust this version if 3.0.3 is the intended release).
"name": "fx-dev",
"version": "3.1.1",
"description": "Complete development workflow including SDLC, pull requests, git utilities, and GitHub integration"
.claude-plugin/marketplace.json:11
- PR description says marketplace metadata.version is bumped to 0.11.3, but the manifest has 0.12.1. Please align the PR description/testing notes with the shipped version (or adjust this value if 0.11.3 is the intended release).
"metadata": {
"description": "Personal Claude Code plugins, skills, and subagents",
"version": "0.12.1",
"homepage": "https://cc.fx.gd"
},
The AGENTS.md read suppressed stderr but its CLAUDE.md fallback did not, so a repo with neither file printed a raw 'No such file or directory' after an apparently successful command. Suppressed it and added an explicit message.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
plugins/fx-dev/.claude-plugin/plugin.json:3
- The PR description/testing notes state the shipped fx-dev version bump is 2.2.0 → 3.0.3, but this manifest sets "version": "3.1.2". Please align the PR description with the actual version being released, or adjust this version if 3.0.3 is the intended target.
"version": "3.1.2",
.claude-plugin/marketplace.json:10
- The PR description/testing notes state the marketplace metadata.version bump is 0.10.0 → 0.11.3, but this file sets "metadata.version": "0.12.2". Please align the PR description with the actual version being released, or adjust this version if 0.11.3 is the intended target.
"metadata": {
"description": "Personal Claude Code plugins, skills, and subagents",
"version": "0.12.2",
"homepage": "https://cc.fx.gd"
setup is invoked automatically by spec-writer and project-management on every call, and it was also doing the destructive half of the work: git mv, content merges, symlink resolution, stale-section deletion. An unattended skill that rewrites a user's files is the shape that produced the AGENTS.md self-import bug earlier in this branch. The two now split on one line: who is allowed to destroy something. setup (automatic) may create missing files and append a missing marker block. It may not move, merge, overwrite, delete, or resolve a symlink. On finding a legacy layout it reports the specific findings and defers. Notably it refuses to seed AGENTS.md when CLAUDE.md holds real conventions, which would split the project's conventions across two files. upgrade (explicit only) owns all of it: detect, build a concrete per-file plan, confirm via AskUserQuestion, apply, then call setup to seed what is still missing. It never commits, so every change is reviewable in git diff. It is structured as a numbered migration registry — M1 is the instruction-file migration, and future convention changes get added alongside it. Carried over intact: the repo-escape rule that refuses to copy a symlink target outside the checkout, and the Claude-only rule split so a wholesale CLAUDE.md move does not publish Claude Code mechanics to Codex, Copilot, and CodeRabbit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
plugins/fx-dev/.claude-plugin/plugin.json:4
- The PR description/testing notes state the shipped fx-dev version bump is 2.2.0 → 3.0.3, but this manifest sets fx-dev to 3.2.0. Please align the PR description with the actual version being released, or adjust this version if 3.0.3 is the intended release.
"name": "fx-dev",
"version": "3.2.0",
"description": "Complete development workflow including SDLC, pull requests, git utilities, and GitHub integration"
.claude-plugin/marketplace.json:11
- The PR description/testing notes state the marketplace metadata.version bump is 0.10.0 → 0.11.3, but this manifest sets metadata.version to 0.13.0. Please reconcile the PR description and the released marketplace version (either update the description or adjust this version).
"metadata": {
"description": "Personal Claude Code plugins, skills, and subagents",
"version": "0.13.0",
"homepage": "https://cc.fx.gd"
},
Codex found the detector violating the privacy rule the skill documents. The pointer checks grepped CLAUDE.md and AGENTS.md unconditionally, and grep follows symlinks — so a CLAUDE.md -> ~/.claude/CLAUDE.md link had its target read during detection, before the user approved anything. Content inspection is now gated on is_plain(), a regular file that is not a symlink; symlinked paths are already classified by the block above and are skipped. Three more from the same pass: - M1 detection missed a canonical file being simply absent. With pointers otherwise current, upgrade reported 'already current' and stopped before Step 5 could run setup to recreate it. Missing AGENTS.md or REVIEW.md is now a detected condition (M1.0). - The .coderabbit.yaml detector grepped for the filename only, so a config listing **/REVIEW.md under enabled: false read as current. It now prints the file, and M1.5 applies unless enabled is true AND the pattern is present. - setup rewrote an explicit code_guidelines.enabled: false to true. setup runs unattended during unrelated spec work, so that silently reverses a deliberate opt-out; it now reports and defers. Added to setup's contract: adding a missing key is creation, changing a value that is already set is not.
Codex found three holes in setup's create-only contract, all in the same family: a step that writes a file the preflight had already condemned. - Step 8.3 appended the Codex pointer to AGENTS.md unconditionally. In the primary legacy layout (CLAUDE.md holds conventions, AGENTS.md absent) Step 6 correctly declined to create AGENTS.md and 8.3 then created it anyway — a stub containing only review rules, which every non-Claude agent would read as the project's complete conventions. - [ -e ] follows symlinks, so a DANGLING .github/copilot-instructions.md read as absent. The old fx-dev bridge points at ../REVIEW.md and dangles exactly when REVIEW.md is missing, so setup created REVIEW.md, left the stale bridge in place, and reported success. - .coderabbit.yaml was never classified, so setup would create or update through a symlink and edit its target, possibly outside the repo. The preflight now emits three flags (legacy_agents, legacy_review, legacy_rabbit), each gating the steps that write the affected file, with exists() pairing -e with -L and is_plain() guarding every content inspection.
Four from Codex, all about a skill doing more than its caller asked for. - Both feedback resolvers ran fx-dev:setup when REVIEW.md was absent. They operate on an open PR and push their changes, so resolving one incorrect comment could add docs/specs, docs/changes, task indexes, AGENTS.md, CLAUDE.md and .coderabbit.yaml to someone's diff. They now create REVIEW.md directly and mention that setup completes the layout later. - rabbit-feedback-resolver flipped code_guidelines.enabled: false to true mid-PR, contradicting setup and upgrade, which both treat that value as a deliberate decision. It now reports and defers. - setup's disabled-check grepped 'enabled:\s*false' unscoped, so an unrelated reviews.auto_review.enabled: false wrongly skipped Step 9 and left CodeRabbit without **/REVIEW.md. Replaced with a parser scoped to the code_guidelines block, verified against five configs including that exact false positive. - upgrade's frontmatter triggered on a bare 'upgrade', which skill routing would match for 'upgrade React' or 'upgrade the database' — routing ordinary dependency work into a skill that rewrites instruction files. Narrowed to convention/instruction-file wording, with an explicit NOT-for-dependencies clause and an instruction to ask when ambiguous.
…olding docs Five from Codex, led by a data-loss bug. M1.6 told upgrade to remove any AGENTS.md section referencing PROJECT.md or - [x]. A '## Conventions' heading holding one obsolete task rule alongside security, testing and deployment rules would have been deleted whole. Cleanup is now clause-level with a keep/remove table, an instruction to edit lines and leave the heading when a section is mixed, and a bias to keep: a stale rule is recoverable, a deleted one is not. - upgrade Step 5 invoked fx-dev:setup, which scaffolds docs/specs, docs/changes and the task indexes — a documentation system the user never asked for, in the same diff as an instruction-file migration. It now seeds only the instruction files, using setup's exact seed blocks. - M1 detection missed stale task language, so a repo where setup had appended the current marker beside an old task section reported 'nothing to migrate' and M1.6 never ran. Added a detector. - resolve-pr-feedback still told resolvers to run setup mid-PR, contradicting both resolver skills after the last commit fixed them. - resolve-codecov-feedback read only AGENTS.md, so an unmigrated repo with its coverage threshold in CLAUDE.md silently got the 100% default. It now falls back to CLAUDE.md before defaulting.
Follow-ups to the previous two commits — gates I added but did not carry through to every affected step. - Step 7 inspected CLAUDE.md for the @AGENTS.md pointer without checking legacy_agents. That flag is set when CLAUDE.md is a symlink, and the check follows the link, so an unattended run read ~/.claude/CLAUDE.md. Step 7 now skips entirely on that flag. - Step 8.3 was gated on legacy_agents but not on REVIEW.md existing. In a repo with .github/copilot-instructions.md and no REVIEW.md, legacy_review blocks Step 8 while 8.3 still wrote a pointer whose entire content is 'read REVIEW.md' — a dangling instruction for Codex. - Step 6.2 treated the presence of /project-management as proof the file was current, so obsolete PROJECT.md and checkbox rules surviving alongside it were never reported. - upgrade's stale detector missed task-placement language (tasks under docs/specs/), which M1.6 explicitly removes, so that state reported 'nothing to migrate'. - M1.0 still said Step 5 runs fx-dev:setup, which Step 5 now forbids.
…contain cb1e220's message listed five fixes; a failed string match meant only the three setup ones landed, and the script's error did not stop the commit. The two upgrade fixes are here: - the stale-language detector now matches task-placement wording (tasks under docs/specs/), which M1.6 removes but the detector never flagged - M1.0 no longer says Step 5 runs fx-dev:setup, which Step 5 forbids
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Standardizes every fx-dev skill on two canonical instruction files, with a pointer for each tool that cannot read them natively.
AGENTS.mdREVIEW.mdCopilot code review and Claude Code Review both read
REVIEW.mdnatively, so only two pointers are needed:CLAUDE.mdis a single@AGENTS.mdimport — Claude Code CLI does not readAGENTS.md.coderabbit.yamllists**/REVIEW.md— not in CodeRabbit's defaultsPlus one section, not a file:
AGENTS.mdcarries## Code Review Rulespointing atREVIEW.md, because Codex reads onlyAGENTS.md.Why
The resolvers each wrote recurrence-prevention rules to a different place:
copilot-feedback-resolverandrabbit-feedback-resolverboth targeted.github/copilot-instructions.md, readers looked atCLAUDE.md, and Codex had no instruction-file model at all. Now every resolver writes INCORRECT-category rules toREVIEW.md, so suppressing one false positive suppresses it for Copilot, CodeRabbit, Codex, and Claude Code Review at once.Per-tool support was verified against current vendor docs, not assumed — the matrix lives in
skills/setup/references/instruction-files.mdalongside the migration path for existing repos.Changes
skills/upgrade/— the intrusive migration skill; plans, confirms, then rewritesskills/setup/references/instruction-files.md— the standard and the per-tool matrixsetupreduced to a create-only contract; detects legacy layouts and defers toupgradecopilot-feedback-resolver,rabbit-feedback-resolver,resolve-pr-feedback) targetREVIEW.mduniformlycodex-reviewchecks for theAGENTS.mdpointer and reports when missingpr-reviewer,planner,coder,spec-writer,project-management,requirements-analyzer,resolve-codecov-feedback,pr-preparer,upstream-contrib) readAGENTS.mdCLAUDE.mdtoAGENTS.md,.github/copilot-instructions.mdtoREVIEW.md, plus a.coderabbit.yamlReview notes
Five rounds of
codex reviewran against this branch; 18 findings were fixed, including four that mattered:setupruns automatically on every/spec-writerand/project-managementcall. The migration path classified the@AGENTS.mdpointer as content to merge, appending it intoAGENTS.mdand making the file import itself — on every single run.CLAUDE.mdsymlinked to~/.claude/CLAUDE.mdwould have had the user's private machine-wide instructions inlined into a tracked, possibly public,AGENTS.md. Symlink targets are now canonicalized and targets outside the checkout are never read.REVIEW.mdcould silently discard each other's rules; they now return proposed rules and the root session applies them in one serialized edit.git add, which could land the symlink in a commit without its target. It no longer stages anything.The sixth round could not run — Codex hit its usage limit (resets Jul 28), so the final commits are unverified by Codex. The preceding round's findings were a long tail of the same symlink edge case, which is why they were fixed centrally in the Step 5.5 preflight rather than one path at a time.
Late change: the Copilot bridge is gone
This PR originally symlinked
.github/copilot-instructions.mdto../REVIEW.md, because GitHub's custom-instructions docs say Copilot code review will not follow file references. That was correct when written and is now obsolete: the 2026-07-17 changelog added nativeREVIEW.mdsupport, postdating the docs page it was verified against.Copilot was the symlink's only consumer, so removing it also removed the fallback machinery built to make it robust — the generated mirror for
core.symlinks=falsecheckouts, its regeneration snippet, the resolver wiring that kept it in sync, and the preflight's materialized-pointer handling. That is a large net simplification.Copilot itself surfaced this, flagging the symlink as a missing file while reviewing from
REVIEW.mdit had read natively..coderabbit.yamlis now mandatory rather than optional: CodeRabbit previously reachedREVIEW.mdthrough the symlink via its defaultcopilot-instructionspattern, and the explicit**/REVIEW.mdpattern is now the only path.Migration: new
fx-dev:upgradeskillExisting projects: run
/fx-dev:upgrade. It detects a legacyCLAUDE.mdor.github/copilot-instructions.md, shows a concrete per-file plan, asks for confirmation, then moves the content and creates the pointers. It never commits, so the whole migration is reviewable ingit diff.Migration was originally part of
setup, which is wrong:setupis invoked automatically byspec-writerandproject-managementon every call, and it was doinggit mv, content merges, and symlink deletion unattended. That is the same shape as theAGENTS.mdself-import bug found earlier in this branch — an auto-running skill rewriting files nobody reviewed.The two now split on one line, who is allowed to destroy something:
setup(automatic)upgrade(explicit only)setupnotably refuses to seed anAGENTS.mdwhenCLAUDE.mdstill holds real conventions — that would split the project's conventions across two files and leave neither complete.upgradeis structured as a numbered migration registry so future convention changes are added alongside M1 rather than needing their own skill. Both the repo-escape rule (never copy a symlink target outside the checkout) and the Claude-only rule split carry over intact.Testing
jq emptypasses on all three changed manifests.github/copilot-instructions.mdanywhere;REVIEW.md,AGENTS.mdandCLAUDE.mdare all regular filesALREADY MIGRATED. Note this covers theAGENTS.md/CLAUDE.mdguard only — Step 8.2'sdocs/changes/marker is absent from this repo'sREVIEW.md, so setup would seed the Task Cross-Reference block if it were run here. It isn't: fx-cc does not use the spec-drivendocs/workflow. In a repo that does, Step 2 createsdocs/changes/before Step 8.2 is reached, so the seeded block is coherent there2.2.0to3.0.3(major is the breaking change — skills read and write different files; the patch increments are review-feedback fixes on this branch), fx-meta0.1.2to0.1.3, marketplace0.10.0to0.11.3