Integrate upstream patterns and Antigravity support - #10
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (29)
Cache: Disabled due to Reviews > Disable Cache setting 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds the ChangesPatterns and packaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant SkillpackBuild
participant SkillpackInstall
participant Antigravity
Developer->>SkillpackBuild: build with --targets=antigravity
SkillpackBuild->>SkillpackInstall: provide antigravity output
SkillpackInstall->>Antigravity: install project or global skills
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
The PR successfully integrates upstream patterns and Antigravity support. The implementation is clean, properly tested, and follows existing conventions. The opt-in Antigravity build targets are correctly integrated across build/install scripts, and the wp-patterns skill includes comprehensive documentation with proper PHP 7.4+ compatibility and GPL licensing. Test coverage validates all new functionality including the theme.json corrections.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Reviewer's GuideAdds an opt-in Antigravity skillpack target to the build/install tooling, integrates a new wp-patterns skill (including docs, router wiring, eval scenarios, and CI), and tightens WordPress theme.json guidance with slug normalization and form element key corrections backed by harness contracts. Flow diagram for Antigravity skillpack build and install targetsflowchart TD
A[RepoRoot] --> B[skillpack-build.mjs]
B -->|--targets=antigravity| C[dist/antigravity/.agents/skills]
C --> D[skillpack-install.mjs --targets=antigravity]
D --> E[Project .agents/skills/]
C --> F[skillpack-install.mjs --targets=antigravity-global --dry-run]
F --> G[~/.gemini/antigravity/skills/]
Flow diagram for WordPress router decision tree updates for wp-patternsflowchart TD
A[User WordPress task] --> B{Content focus}
B -->|Block patterns / patterns/*.php / register_block_pattern| C[wp-patterns]
B -->|theme.json / Global Styles / templates / template parts| D[wp-block-themes]
B -->|Blocks / block.json / registerBlockType| E[wp-block-development]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
- Restore upstream's WordPress 6.9 / PHP 7.2.24 compatibility floor: the AI-era 7.0 floor is reserved for 7.0-only features, and upstream (WordPress#84) settled on 6.9 wording for this skill - Reference triage/theme detectors via portable ../<skill>/ paths so the commands resolve in installed skillpack layouts, matching the other skills - Add wp-patterns to the skills-ref CI validation subset Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewReviewed all 28 files. The Antigravity packaging changes are correct and well-tested (the new Issues found → fixed in 36db348
Verified
Follow-ups (pre-existing, not this PR)
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The target definitions and directory mappings for codex/vscode/claude/cursor/antigravity are duplicated across
skillpack-build.mjsandskillpack-install.mjs; consider centralizing these in a shared module to avoid drift when adding or modifying targets. - The CLI UX around global targets is slightly asymmetric (e.g.,
--globalonly maps toclaude-globalwhileantigravity-globalmust be spelled out); if Antigravity is a first-class target long-term, consider adding a shorthand or clarifying this distinction in the usage text.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The target definitions and directory mappings for codex/vscode/claude/cursor/antigravity are duplicated across `skillpack-build.mjs` and `skillpack-install.mjs`; consider centralizing these in a shared module to avoid drift when adding or modifying targets.
- The CLI UX around global targets is slightly asymmetric (e.g., `--global` only maps to `claude-global` while `antigravity-global` must be spelled out); if Antigravity is a first-class target long-term, consider adding a shorthand or clarifying this distinction in the usage text.
## Individual Comments
### Comment 1
<location path="shared/scripts/skillpack-install.mjs" line_range="237-241" />
<code_context>
function listAvailableSkills(fromDir) {
// Check all possible target sources
- const sources = ["codex", "vscode", "claude", "cursor"]
+ const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
</code_context>
<issue_to_address>
**suggestion:** Derive `listAvailableSkills` sources from existing target metadata instead of a hardcoded list.
This list duplicates information already in `VALID_TARGETS`/`getSourceDir`, so every new non‑global target must be updated in multiple places. Instead, derive `sources` from `VALID_TARGETS` by filtering out `*-global` entries and mapping through `getSourceDir`, so `listAvailableSkills` automatically stays in sync with new project‑level targets.
```suggestion
function listAvailableSkills(fromDir) {
// Derive non-global target sources from VALID_TARGETS so this stays in sync with new targets
const sources = VALID_TARGETS
.filter((t) => !t.endsWith("-global"))
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
```
</issue_to_address>
### Comment 2
<location path="shared/scripts/skillpack-install.mjs" line_range="278" />
<code_context>
- // --dest is required unless only using global targets (claude-global, cursor-global)
- const needsDest = targets.some((t) => t !== "claude-global" && t !== "cursor-global");
+ // --dest is required unless only global targets are selected.
+ const globalTargets = new Set(["claude-global", "cursor-global", "antigravity-global"]);
+ const needsDest = targets.some((t) => !globalTargets.has(t));
if (needsDest && !args.dest) {
</code_context>
<issue_to_address>
**suggestion:** Reuse the `globalTargets` knowledge in `getDestDir` to avoid divergent global-target handling.
There are now two separate definitions of which targets are global: the `globalTargets` set here, and the explicit checks in `getDestDir`/`installTarget`. This risks them drifting out of sync. Please centralize the global-target detection (e.g., an `isGlobalTarget()` helper or shared module-level set) and use it in both `getDestDir` and `main()`.
Suggested implementation:
```javascript
const GLOBAL_TARGETS = new Set(["claude-global", "cursor-global", "antigravity-global"]);
function isGlobalTarget(target) {
return GLOBAL_TARGETS.has(target);
}
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const needsDest = targets.some((t) => !isGlobalTarget(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();
```
` since they’re not visible in the snippet).
Here are the concrete edits:
```xml
<file_operations>
<file_operation operation="edit" file_path="shared/scripts/skillpack-install.mjs">
<<<<<<< SEARCH
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const globalTargets = new Set(["claude-global", "cursor-global", "antigravity-global"]);
const needsDest = targets.some((t) => !globalTargets.has(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();
=======
const GLOBAL_TARGETS = new Set(["claude-global", "cursor-global", "antigravity-global"]);
function isGlobalTarget(target) {
return GLOBAL_TARGETS.has(target);
}
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const needsDest = targets.some((t) => !isGlobalTarget(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully centralize global-target handling and avoid future drift, update the rest of the file as follows (locations not shown in the snippet):
1. In `getDestDir(target, destDir, ...)`:
- Replace any explicit checks like:
- `if (target === "claude-global" || target === "cursor-global" || target === "antigravity-global")`
- or `if (target.endsWith("-global"))`
- With:
- `if (isGlobalTarget(target)) { ... }`
- Remove any separate, local sets or arrays that enumerate the same global targets.
2. In `installTarget(target, ...)` (or any other helper that branches based on global vs non-global targets):
- Replace similar explicit checks for `"claude-global"`, `"cursor-global"`, `"antigravity-global"` (or `*-global`) with `isGlobalTarget(target)`.
- Ensure no other hard-coded global-target lists remain; they should all go through the shared `GLOBAL_TARGETS` / `isGlobalTarget` definitions.
3. If tests exist for global-target behavior (e.g., `skillpack-install` CLI tests):
- Update or add tests to cover `"antigravity-global"` and confirm `getDestDir` and `installTarget` use the shared logic (e.g., adding/removing from `GLOBAL_TARGETS` changes behavior consistently across both entry points).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| function listAvailableSkills(fromDir) { | ||
| // Check all possible target sources | ||
| const sources = ["codex", "vscode", "claude", "cursor"] | ||
| const sources = ["codex", "vscode", "claude", "cursor", "antigravity"] | ||
| .map((t) => getSourceDir(fromDir, t)) | ||
| .filter((p) => fs.existsSync(p)); |
There was a problem hiding this comment.
suggestion: Derive listAvailableSkills sources from existing target metadata instead of a hardcoded list.
This list duplicates information already in VALID_TARGETS/getSourceDir, so every new non‑global target must be updated in multiple places. Instead, derive sources from VALID_TARGETS by filtering out *-global entries and mapping through getSourceDir, so listAvailableSkills automatically stays in sync with new project‑level targets.
| function listAvailableSkills(fromDir) { | |
| // Check all possible target sources | |
| const sources = ["codex", "vscode", "claude", "cursor"] | |
| const sources = ["codex", "vscode", "claude", "cursor", "antigravity"] | |
| .map((t) => getSourceDir(fromDir, t)) | |
| .filter((p) => fs.existsSync(p)); | |
| function listAvailableSkills(fromDir) { | |
| // Derive non-global target sources from VALID_TARGETS so this stays in sync with new targets | |
| const sources = VALID_TARGETS | |
| .filter((t) => !t.endsWith("-global")) | |
| .map((t) => getSourceDir(fromDir, t)) | |
| .filter((p) => fs.existsSync(p)); |
| // --dest is required unless only using global targets (claude-global, cursor-global) | ||
| const needsDest = targets.some((t) => t !== "claude-global" && t !== "cursor-global"); | ||
| // --dest is required unless only global targets are selected. | ||
| const globalTargets = new Set(["claude-global", "cursor-global", "antigravity-global"]); |
There was a problem hiding this comment.
suggestion: Reuse the globalTargets knowledge in getDestDir to avoid divergent global-target handling.
There are now two separate definitions of which targets are global: the globalTargets set here, and the explicit checks in getDestDir/installTarget. This risks them drifting out of sync. Please centralize the global-target detection (e.g., an isGlobalTarget() helper or shared module-level set) and use it in both getDestDir and main().
Suggested implementation:
const GLOBAL_TARGETS = new Set(["claude-global", "cursor-global", "antigravity-global"]);
function isGlobalTarget(target) {
return GLOBAL_TARGETS.has(target);
}
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const needsDest = targets.some((t) => !isGlobalTarget(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();` since they’re not visible in the snippet).
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="shared/scripts/skillpack-install.mjs">
<<<<<<< SEARCH
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const globalTargets = new Set(["claude-global", "cursor-global", "antigravity-global"]);
const needsDest = targets.some((t) => !globalTargets.has(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();
=======
const GLOBAL_TARGETS = new Set(["claude-global", "cursor-global", "antigravity-global"]);
function isGlobalTarget(target) {
return GLOBAL_TARGETS.has(target);
}
const sources = ["codex", "vscode", "claude", "cursor", "antigravity"]
.map((t) => getSourceDir(fromDir, t))
.filter((p) => fs.existsSync(p));
assert(VALID_TARGETS.includes(t), `Invalid target: ${t}. Valid targets: ${VALID_TARGETS.join(", ")}`);
}
// --dest is required unless only global targets are selected.
const needsDest = targets.some((t) => !isGlobalTarget(t));
if (needsDest && !args.dest) {
process.stderr.write("Error: --dest is required for non-global targets.\n\n");
usage();
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully centralize global-target handling and avoid future drift, update the rest of the file as follows (locations not shown in the snippet):
1. In `getDestDir(target, destDir, ...)`:
- Replace any explicit checks like:
- `if (target === "claude-global" || target === "cursor-global" || target === "antigravity-global")`
- or `if (target.endsWith("-global"))`
- With:
- `if (isGlobalTarget(target)) { ... }`
- Remove any separate, local sets or arrays that enumerate the same global targets.
2. In `installTarget(target, ...)` (or any other helper that branches based on global vs non-global targets):
- Replace similar explicit checks for `"claude-global"`, `"cursor-global"`, `"antigravity-global"` (or `*-global`) with `isGlobalTarget(target)`.
- Ensure no other hard-coded global-target lists remain; they should all go through the shared `GLOBAL_TARGETS` / `isGlobalTarget` definitions.
3. If tests exist for global-target behavior (e.g., `skillpack-install` CLI tests):
- Update or add tests to cover `"antigravity-global"` and confirm `getDestDir` and `installTarget` use the shared logic (e.g., adding/removing from `GLOBAL_TARGETS` changes behavior consistently across both entry points).…, #13, WordPress#18) Brings in upstream commits aecc4d6, 20324d2, 8182820, c212346 while keeping the fork's contracts: - Kept the two-floor compatibility policy (docs/compatibility-policy.md) over upstream WordPress#70's blanket WP 7.0+/PHP 7.4.0+ bump: all SKILL.md frontmatter, eval/harness/run.mjs, scaffold-skill.mjs, and docs resolve to the fork side; CONTRIBUTING.md and README.md now state the two-floor policy explicitly instead of inheriting the flat bump - Kept the fork's fresher upstream indices (WP 7.0.2 / GB snapshot) over upstream WordPress#55's older refresh - Kept the fork's Antigravity (#13) and wp-patterns (WordPress#18) copies, both already adapted in PR #10; upstream's versions differ only in comments, usage text, and wp-patterns frontmatter - Accepted upstream's WP 7.0 example env line in wp-abilities-verify/references/runtime-harness.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
.agents/skills/and global~/.gemini/antigravity/skills/wp-patternsskill, five references, twelve eval scenarios, README/router discovery, GPL metadata, and the default WordPress 6.9+/PHP 7.2.24+ compatibility floor (matching upstream Update wp-patterns compatibility contract to WP 7.0 / PHP 7.4.0 WordPress/agent-skills#84)styles.elements.inputtostyles.elements.textInputUpstream provenance
Selectively adapted from:
wp-patternstextInputcorrectionwp-patternscompatibility wordingThis intentionally excludes unrelated upstream trunk changes. Antigravity remains opt-in; the default build targets remain Codex, VS Code, Claude, and Cursor.
Validation
node eval/harness/run.mjsgit diff --check~/.gemini/antigravity/skills/Summary by Sourcery
Add an opt-in Antigravity skillpack target and integrate the new wp-patterns skill with documentation, routing, and quality gates.
New Features:
Enhancements:
CI:
Tests: