Skip to content

Add wordpress-delegate: route WordPress tasks to the best-suited implementer - #40

Open
Amrmohamed1934 wants to merge 5 commits into
amElnagdy:masterfrom
Amrmohamed1934:feat/wordpress-delegate
Open

Add wordpress-delegate: route WordPress tasks to the best-suited implementer#40
Amrmohamed1934 wants to merge 5 commits into
amElnagdy:masterfrom
Amrmohamed1934:feat/wordpress-delegate

Conversation

@Amrmohamed1934

@Amrmohamed1934 Amrmohamed1934 commented Aug 1, 2026

Copy link
Copy Markdown

Claim: none — no implementer is claimed here. This is not a new implementer skill, and that is the
one thing worth deciding before the code is read.

This proposes a second kind of skill

wordpress-delegate launches no implementer CLI. It classifies a WordPress brief, prepends a
WordPress engineering preamble, and dispatches through a sibling's relay.mjs.

The four invariants hold, transitively — a separate CLI still edits a real working tree and the diff
is still the deliverable, nothing commits, Node built-ins only, and autonomy is still stated in the
chosen CLI's own terms because the sibling states it. But "one skill per implementer CLI" does not
describe it, so CONTRIBUTING.md gains a Domain skills section with three extra rules:

  • It launches no implementer CLI. It shells out to node (the sibling) and git. A domain relay
    that grew its own version preflight, stream parser, or process-tree kill has forked the loop.
  • The sibling's result is the authority. Contract fields are lifted, the sibling's own result is
    embedded verbatim, and the verdict is overridden only where the domain relay did the killing.
  • Routing is a table, and the table is the extension point. One row per lane.

If you would rather this repo stayed one-skill-per-CLI, say so and I will close it — the skill is
self-contained and drops out cleanly. Everything below assumes the category is welcome.

What ran

Native Windows 11, codex 0.144.1, through codex-delegate — a write run against a throwaway
plugin repo containing a deliberately unauthenticated, injectable AJAX handler. Routed by the table
(no --implementer), dispatched through the sibling, and the diff came back with
check_ajax_referer plus current_user_can, wp_unslash/absint, $wpdb->prepare() with
%i/%d/%s, esc_html at output, a text domain with a translator comment, and the
wp_ajax_nopriv registration dropped. None of that was in the brief — it is the preamble arriving at
the implementer. The thread id was lifted for resume, and the relay made no commit (the repo still
had one commit and a dirty tree afterwards).

node test/relay-smoke.mjs — all green on native Windows. wordpress enters the shared matrix
like every sibling (package shape, registration, syntax, atomic publish, the full --timeout
rejection matrix plus its positive control, and the whole-process-tree timeout kill). Routing gets
its own coverage, since routing is what this relay uniquely does:

  • every shipped lane's decision, asserted on a brief phrased the way the task actually arrives
  • the read-only demotion rule, and its two-signal counter-case
  • the fallback's confidence: "none" and its clarification string
  • --strict-routing refusing at exit 3 before any artifact exists
  • --read-only and --session translated for all ten implementers, and refused for kimi/agy
  • opencode's missing --model reported as a blocker rather than a crash
  • the preamble and the detected domain notes present in the composed brief
  • a resumed run sending the delta brief without the preamble
  • a missing sibling relay as implementer_unavailable at exit 127, with a result file
  • a full dispatch with the sibling's result lifted intact

Untested, and stated in the README's Verification status line: only codex has been driven live
grok, kimi, and opencode are contract-tested. The aborted path is POSIX-only in the suite and
has not been driven for this relay on any platform. No run against a real production WordPress site.

Routing

Two tables at the top of scripts/relay.mjs.

LANES — what kind of work this is, and who gets it. Lanes score by distinct signal matches;
highest wins; lane order breaks ties, so a brief tripping both security and performance routes as
security.

Lane Why
security-audit grok (read-only) adversarial reading, not editing; findings are the deliverable
research grok (read-only) breadth and a defended recommendation, not a diff
documentation kimi high-volume, low-branching prose
refactor-sweep opencode sustained context across files beats depth on any one
plugin-architecture codex structural decisions compound
elementor-widget codex the control/render/editor contract fails silently when wrong
performance codex measurement-driven, easy to get plausibly wrong
implementation (fallback) codex no lane signal dominated

DOMAINS — what the work is about (WooCommerce, Elementor, ACF, WPForms, database, security,
integrations, hosting, deployment). Domains do not route. They select the preamble notes and are
recorded in the result so a reviewer can see what the router thought it was looking at.

One rule overrides the score, and it came out of a run that failed in review rather than from
design: a read-only lane needs two signals to win. "Fix the cart hook, it's missing a nonce
check" was routing to a read-only audit on the word nonce and would have returned findings with an
empty diff to someone who asked for a fix. It now demotes to implementation, keeps its writes, and
records security-audit in alternates.

Unknown tasks fall through with confidence: "none" and a clarification string;
--strict-routing turns that into exit 3 with the question to put to the user.

Reading the relay

For the line-by-line read, the shape is: two data tables and a preamble constant at the top, then
classify(), then a dispatch path that is deliberately thin. Worth a second look:

  • killChild / the SIGTERM handler — the cascade is wordpress → sibling → CLI → tools, each detached
    into its own group, and the grace window is longer than the sibling's own 2s touched-files refresh
    so a forwarded kill still publishes a complete snapshot.
  • parseDuration — the accepted range is exactly a sibling's, so a duration this relay forwards is
    never one the sibling then rejects; the backstop's grace is clamped at schedule time instead.
  • -- passthrough — only flags meaning the same thing across implementers are modelled; anything
    CLI-specific goes after -- and is that CLI's business.

Second commit: .gitignore

AGENTS.md asks you to validate with npx skills add . before publishing; doing that inside the
repo drops a .claude/ directory and a skills-lock.json that then show up as untracked forever.
Kept as its own commit (ae0942c) so it can be dropped or cherry-picked independently of the skill.
Only the two paths that actually appear are listed — the agent directory depends on which agent the
CLI detects, so the comment says to add yours rather than guessing at .cursor/, .codex/ and the
rest. Say the word if you'd rather have it as its own PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added WordPress task delegation with domain-aware routing to suitable implementation tools.
    • Added route inspection, dry-run execution, read-only audits, session resumption, and controlled dispatch options.
    • Added support for sequential multi-task workflows with review and commit guidance.
  • Documentation

    • Expanded project and contributor documentation for domain skills, routing conventions, usage, verification, and review workflows.
  • Tests

    • Added coverage for WordPress routing, dispatch behavior, flag handling, resumption, missing tools, and timeout scenarios.

…ementer

wordpress-delegate is the first domain skill here: it launches no implementer
CLI of its own. It classifies the brief, prepends the WordPress engineering
preamble, and dispatches through a sibling's relay.mjs, which keeps sandboxes,
sessions, stream parsing, process-tree kills, and the result contract owned by
exactly one file per implementer.

Routing is two tables at the top of scripts/relay.mjs. LANES decides what kind
of work a brief is and who gets it - security-audit and research to grok
read-only, documentation to kimi, refactor-sweep to opencode,
plugin-architecture, elementor-widget and performance to codex, with
implementation as the fallback. Lanes score by distinct signal matches, and lane
order breaks ties, so a brief that trips both security and performance routes as
security. DOMAINS decides what the brief is about - WooCommerce, Elementor, ACF,
WPForms, database, security, integrations, hosting, deployment - and selects the
preamble notes without touching the routing decision.

One rule overrides the score: a read-only lane needs two signals to win. A
bugfix brief that mentions a missing nonce check routed to a read-only audit on
the word nonce alone, and would have returned findings with an empty diff to
someone who asked for a fix. It now demotes to implementation and keeps its
writes, with the audit lane recorded in alternates.

Adding a lane is one row; nothing else in the file knows the lane names.
--dry-run prints the decision without dispatching, --list-routes prints both
tables, --print-preamble prints the standing instruction so the docs cannot
drift from what is actually sent, and --strict-routing exits 3 with the question
to put to the user rather than letting the fallback decide.

The portable --read-only and --session are translated into each CLI's own terms
(--plan-only for Vibe, --permission-mode plan and --resume for Qoder,
--conversation for Antigravity) and refused up front for kimi and agy, which
have no read-only mode. Anything specific to one CLI goes after -- and is
forwarded verbatim.

result.json speaks delegate-relay.result.v1 with the contract fields lifted from
the sibling, the sibling's own result embedded verbatim under implementerResult,
and a routing block recording the lane, confidence, matched signals, domains and
runners-up so a reviewer can see what the router thought it was looking at.

Registered in the smoke matrix alongside every sibling, with routing-specific
coverage: each shipped lane's decision, the demotion rule, the strict refusal,
the flag translation for all ten implementers, the preamble in the composed
brief, a resumed run sending the delta without it, a missing sibling relay, and
a full dispatch with the sibling's result lifted intact.

Verified on native Windows through codex-delegate with codex 0.144.1: a write
run on a throwaway plugin repo produced check_ajax_referer plus
current_user_can, wp_unslash/absint, $wpdb->prepare() with %i/%d/%s, esc_html at
output, a text domain with a translator comment, and the wp_ajax_nopriv
registration dropped - and made no commit. The other three lane targets are
contract-tested only, and the aborted path is POSIX-only in the suite and
undriven for this relay.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e84e32f5-7667-40e5-bacb-49ab49a5f539

📥 Commits

Reviewing files that changed from the base of the PR and between ef9cd1f and 7a2379d.

📒 Files selected for processing (3)
  • skills/wordpress-delegate/references/dispatch-and-poll.md
  • skills/wordpress-delegate/scripts/relay.mjs
  • test/relay-smoke.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
  • skills/wordpress-delegate/references/dispatch-and-poll.md
  • test/relay-smoke.mjs
  • skills/wordpress-delegate/scripts/relay.mjs

Walkthrough

Changes

WordPress delegation

Layer / File(s) Summary
Domain skill contract and registration
.gitignore, AGENTS.md, CONTRIBUTING.md, README.md, skills.sh.json, skills/wordpress-delegate/SKILL.md
Registers wordpress-delegate and defines its domain routing contract, workflow, boundaries, and repository conventions.
Routing and WordPress brief composition
skills/wordpress-delegate/references/writing-the-brief.md, skills/wordpress-delegate/scripts/relay.mjs
Adds WordPress domain detection, implementer lanes, engineering preambles, CLI controls, and brief composition.
Sibling dispatch and result lifecycle
skills/wordpress-delegate/references/dispatch-and-poll.md, skills/wordpress-delegate/scripts/relay.mjs
Adds sibling relay resolution, flag translation, process control, timeout handling, result persistence, fallback reporting, and execution summaries.
Review workflow and smoke coverage
skills/wordpress-delegate/references/multi-task-queues.md, skills/wordpress-delegate/references/review-and-land.md, test/relay-smoke.mjs
Documents queue and review procedures and tests routing, resume handling, result embedding, missing relays, and timeout behavior.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant WordPressRelay
  participant ImplementerRelay
  participant ResultFile
  User->>WordPressRelay: Submit WordPress brief
  WordPressRelay->>WordPressRelay: Classify domain and select implementer
  WordPressRelay->>ImplementerRelay: Dispatch composed brief
  ImplementerRelay-->>WordPressRelay: Return result and session data
  WordPressRelay->>ResultFile: Write atomic result artifact
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the new WordPress delegate and its primary routing behavior.
Description check ✅ Passed The description covers the claim, test environment, verification results, untested paths, routing design, and domain-skill rationale.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

AGENTS.md asks you to validate the package locally with `npx skills add .`
before publishing a change. Doing that inside the repo drops a `.claude/`
directory of installed skill copies and a `skills-lock.json` beside them, both
of which then show up as untracked in every subsequent `git status` and are easy
to stage by accident alongside a real change.

Only the two paths that actually appear are listed. The agent directory depends
on which agent the CLI detects, so the comment says to add yours if it differs
rather than guessing at `.cursor/`, `.codex/` and the rest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
skills/wordpress-delegate/SKILL.md (1)

44-48: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Pin the install reference in the copy-pasteable npx command.

npx skills add amElnagdy/delegate-skills installs directly from a branch reference with no tag or commit pin. If the upstream default branch is ever compromised, every reader who copies this exact command installs whatever is currently on that branch, not a reviewed version. Pin to a released tag or commit.

🔒️ Proposed fix
-1. At least the routed sibling is installed — the four shipped routing targets are
-   `codex-delegate`, `grok-delegate`, `kimi-delegate`, and `opencode-delegate`. Install the whole
-   package (`npx skills add amElnagdy/delegate-skills`) and all ten are there.
+1. At least the routed sibling is installed — the four shipped routing targets are
+   `codex-delegate`, `grok-delegate`, `kimi-delegate`, and `opencode-delegate`. Install the whole
+   package (`npx skills add amElnagdy/delegate-skills#<tag>`) and all ten are there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/wordpress-delegate/SKILL.md` around lines 44 - 48, Update the
copy-pasteable npx skills add command in the Prerequisites section to reference
a reviewed released tag or commit instead of the unpinned default branch, while
preserving installation of the complete delegate-skills package.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/wordpress-delegate/scripts/relay.mjs`:
- Around line 799-809: Reconcile the watchdog tree-kill ownership across all
affected sites: in skills/wordpress-delegate/scripts/relay.mjs, either remove or
stop calling killChild(), or explicitly retain it as an owned watchdog
exception; update AGENTS.md lines 67-76 and CONTRIBUTING.md lines 22-42
consistently with that choice, documenting the exception and verdict-override
behavior if retained. Ensure the relay behavior and both documents define the
same process-tree kill invariant.

---

Nitpick comments:
In `@skills/wordpress-delegate/SKILL.md`:
- Around line 44-48: Update the copy-pasteable npx skills add command in the
Prerequisites section to reference a reviewed released tag or commit instead of
the unpinned default branch, while preserving installation of the complete
delegate-skills package.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dd4f3d32-40d4-44bb-b43f-dfd4eace419c

📥 Commits

Reviewing files that changed from the base of the PR and between 347f2fa and ae0942c.

📒 Files selected for processing (12)
  • .gitignore
  • AGENTS.md
  • CONTRIBUTING.md
  • README.md
  • skills.sh.json
  • skills/wordpress-delegate/SKILL.md
  • skills/wordpress-delegate/references/dispatch-and-poll.md
  • skills/wordpress-delegate/references/multi-task-queues.md
  • skills/wordpress-delegate/references/review-and-land.md
  • skills/wordpress-delegate/references/writing-the-brief.md
  • skills/wordpress-delegate/scripts/relay.mjs
  • test/relay-smoke.mjs

Comment thread skills/wordpress-delegate/scripts/relay.mjs
amrmohammed1934 and others added 2 commits August 1, 2026 07:40
Gutenberg work has the same shape as the Elementor lane's: an invalid block is a
validation error in the editor, not a test failure, so the registration and
edit/save contract is the constraint that bites. Fourteen signals covering
block.json, registerBlockType and register_block_type, InnerBlocks, block
attributes and deprecations and variations, the @WordPress packages,
useBlockProps, theme.json, FSE and block themes, the Interactivity API, and
render_callback. Routed to codex for the same reason elementor-widget is.

Placed between elementor-widget and performance. Lane order breaks ties, so
"block editor performance" routes as block work, which mirrors why
elementor-widget already sits above performance.

Writing its signals surfaced a bug in the matcher: classify() folded the brief
to lower case before testing, so any signal containing a capital could never
fire. The Elementor lane's /\b\\?Elementor\\/ row was dead on arrival, and a
block-editor lane would have needed registerblocktype spelled wrong to work.
Signals are now normalized to case-insensitive once at load and matched against
the brief as written, so a row can be written in whatever casing its ecosystem
uses and matchedSignals echoes back what the brief actually said. A regression
check pins it, and the extension guide in dispatch-and-poll.md says so.

The guide's worked example used block-editor as its hypothetical new lane, which
would now be confusing; it shows a multisite lane instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The domain-skill rules said a relay that grew its own process-tree kill had
forked the loop, and then wordpress-delegate shipped a killChild(). Both cannot
be true, and the rule as written was the wrong half: a relay that owns a
watchdog backstop and forwards SIGTERM/SIGINT/SIGHUP cannot honour either
without terminating the process it spawned.

So the rule now names the exception and bounds it. The domain relay kills
exactly one thing, the sibling relay. On POSIX the sibling spawns the
implementer CLI detached into a process group of its own, so the signal does not
reach past the sibling at all -- the sibling's own handler is what fells the CLI
and its tools, which is the behaviour the smoke suite already drives. Windows is
the usual exception: with no process groups to signal, the taskkill /t that
fells the sibling fells the tree under it and the sibling's handler never runs.

What a domain relay still must not re-implement is the per-CLI machinery: the
version preflight, the stream parser, and the argv and sandbox translation.
Those are what "one file per implementer" is protecting, and the previous
wording buried them behind the kill.

This also explains the neighbouring rule rather than leaving it to be inferred.
The sibling's result is the authority except where the domain relay did the
killing -- which is exactly, and only, the case this exception creates.

Reconciled in all four places that stated it: AGENTS.md, CONTRIBUTING.md, the
skill's own dispatch-and-poll reference, and the comment on killChild itself.
No behaviour change; the relay already did what is now described.

Raised by CodeRabbit on amElnagdy#40.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Amrmohamed1934

Copy link
Copy Markdown
Author

Good catch on the first one — that was a real contradiction, and the rule as written was the wrong half.

Kill ownership (major): fixed in ef9cd1f. A relay that owns a watchdog backstop and forwards SIGTERM/SIGINT/SIGHUP cannot honour either without terminating the process it spawned, so the honest rule names the exception and bounds it rather than banning it.

What the relay actually does, and what the docs now say:

  • It kills exactly one thing — the sibling relay.
  • On POSIX the sibling spawns the implementer CLI detached, into a process group of its own, so process.kill(-child.pid, …) does not reach past the sibling. The sibling's own handler is what fells the CLI and its tools. That cascade is what wordpress timeout: the implementer's own subprocess is dead (whole tree felled) already drives in the smoke suite.
  • Windows is the usual exception: no process groups to signal, so the taskkill /t that fells the sibling fells the tree under it and the sibling's handler never runs. Same asymmetry every relay here already carries.

What a domain relay still must not re-implement is the per-CLI machinery — the version preflight, the stream parser, and the argv and sandbox translation. Those are what "one file per implementer" is protecting, and the old wording buried them behind the kill.

It also makes the neighbouring rule follow instead of having to be inferred: the sibling's result is the authority except where the domain relay did the killing, which is exactly and only the case this exception creates.

Reconciled in all four places that stated it — AGENTS.md, CONTRIBUTING.md, the skill's own dispatch-and-poll.md, and the comment on killChild itself. No behaviour change; the relay already did what is now described. Smoke suite still green.

Pinned install (nitpick): skipping, with a reason. Every install command in this repo — the README's four, and all ten sibling SKILL.mds — uses the unpinned npx skills add amElnagdy/delegate-skills, and the repo has no tags to pin to. Pinning this one line would make the new skill the only one whose documented install differs from its siblings, and point at a ref that doesn't exist. If pinning is wanted it's a repo-wide change against a real tag, and the maintainer's call — happy to open it separately.

Also in this push, unrelated to the review: 0834b9b adds a block-editor lane for Gutenberg work, and fixes a matcher bug it surfaced — classify() folded the brief to lower case before testing, so any signal containing a capital could never fire. The Elementor lane's /\b\\?Elementor\\/ row was dead on arrival. Signals are now case-insensitive, with a regression check.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
skills/wordpress-delegate/SKILL.md (1)

80-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use an executable and portable relay path.

node .../relay.mjs is not a runnable path. The find ~ fallback is local-machine-specific and is not portable to native PowerShell or cmd. Use the documented <skill-dir>/scripts/relay.mjs path and rely on the Skills CLI's reported installation path.

As per coding guidelines, user-facing documentation must not reference local machines, and commands must match the installed relay.

Proposed documentation fix
-# see all options and the lane table:             node .../relay.mjs --help   ·   --list-routes
+# see all options:                                node <skill-dir>/scripts/relay.mjs --help
+# see the lane table:                             node <skill-dir>/scripts/relay.mjs --list-routes
...
-If unsure where it landed, run
-`find ~ -name relay.mjs -path '*wordpress-delegate*'` and substitute the directory above it.
+Use the installed skill directory reported by the Skills CLI and substitute it for `<skill-dir>`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/wordpress-delegate/SKILL.md` around lines 80 - 86, Update the relay
usage documentation near the `--help` and `--list-routes` examples to invoke the
executable `<skill-dir>/scripts/relay.mjs` path. Remove the `find ~` fallback
and any local-machine-specific guidance, relying instead on the Skills CLI’s
reported “Base directory for this skill” installation path.

Source: Coding guidelines

skills/wordpress-delegate/references/dispatch-and-poll.md (1)

158-160: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the controlled delegation vocabulary.

The heading uses other agents for a dispatch path. Replace it with Plain shell and use orchestrator only for orchestrator-specific instructions.

Proposed fix
- - **Plain shell / other agents:** foreground for short tasks, or background and poll.
+ - **Plain shell:** run short tasks in the foreground, or run the command in the background and poll.

As per coding guidelines: user-facing documentation must use the controlled delegation vocabulary and must not invent synonyms or coined umbrella terms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/wordpress-delegate/references/dispatch-and-poll.md` around lines 158 -
160, Update the dispatch-path heading in the documentation to replace “other
agents” with “Plain shell,” and reserve “orchestrator” exclusively for
orchestrator-specific instructions. Keep the surrounding foreground/background
execution guidance unchanged.

Source: Coding guidelines

skills/wordpress-delegate/scripts/relay.mjs (1)

830-844: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle SIGINT/SIGHUP and failed taskkill under Windows.

killChild(child) is used by the backstop and the SIGTERM/SIGINT/SIGHUP handlers. On Windows, anything other than that default SIGTERM call returns directly, so SIGINT and watchdog-backed SIGTERM stop forwarding. A failed taskkill path also does nothing; use child.kill() with the requested signal here for every supported termination path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/wordpress-delegate/scripts/relay.mjs` around lines 830 - 844, Update
killChild to handle every supported signal on Windows, including SIGINT and
SIGHUP, instead of returning for non-SIGTERM calls. Attempt the existing
taskkill flow where appropriate, but when taskkill fails or is not used, fall
back to child.kill(signal) so the requested termination signal is always
forwarded.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@AGENTS.md`:
- Line 80: Update the Windows process-tree description in AGENTS.md by replacing
both occurrences of “fells” with “kills,” preserving the rest of the sentence.

In `@skills/wordpress-delegate/references/dispatch-and-poll.md`:
- Around line 7-16: Update the subprocess inventory in the relay documentation
to include the Windows taskkill /t /f invocation. Distinguish the
normal-dispatch commands (node and git) from this Windows-only termination
exception, and ensure the documented commands match the implementer CLI and
relay.mjs behavior.

In `@skills/wordpress-delegate/scripts/relay.mjs`:
- Line 318: The `@wordpress` signal regex in relay.mjs currently uses a leading
word boundary that prevents matching plain-text mentions beginning with “@”.
Update the pattern to remove that leading \b or replace it with a boundary
compatible with @, while preserving the existing package alternatives.

---

Outside diff comments:
In `@skills/wordpress-delegate/references/dispatch-and-poll.md`:
- Around line 158-160: Update the dispatch-path heading in the documentation to
replace “other agents” with “Plain shell,” and reserve “orchestrator”
exclusively for orchestrator-specific instructions. Keep the surrounding
foreground/background execution guidance unchanged.

In `@skills/wordpress-delegate/scripts/relay.mjs`:
- Around line 830-844: Update killChild to handle every supported signal on
Windows, including SIGINT and SIGHUP, instead of returning for non-SIGTERM
calls. Attempt the existing taskkill flow where appropriate, but when taskkill
fails or is not used, fall back to child.kill(signal) so the requested
termination signal is always forwarded.

In `@skills/wordpress-delegate/SKILL.md`:
- Around line 80-86: Update the relay usage documentation near the `--help` and
`--list-routes` examples to invoke the executable
`<skill-dir>/scripts/relay.mjs` path. Remove the `find ~` fallback and any
local-machine-specific guidance, relying instead on the Skills CLI’s reported
“Base directory for this skill” installation path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d3a41723-e1a5-4b6b-8366-8ea0df2f5c76

📥 Commits

Reviewing files that changed from the base of the PR and between ae0942c and ef9cd1f.

📒 Files selected for processing (6)
  • AGENTS.md
  • CONTRIBUTING.md
  • skills/wordpress-delegate/SKILL.md
  • skills/wordpress-delegate/references/dispatch-and-poll.md
  • skills/wordpress-delegate/scripts/relay.mjs
  • test/relay-smoke.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/relay-smoke.mjs
  • CONTRIBUTING.md

Comment thread AGENTS.md
backstop and forwards `SIGTERM`/`SIGINT`/`SIGHUP`, and neither is possible without killing the
process it spawned. It kills exactly one thing — the sibling relay — and the sibling's own handler
is what reaches the implementer CLI, which leads a process group of its own. (On Windows there are
no process groups to signal, so the `taskkill /t` that fells the sibling fells the tree under it

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use kills in the Windows process-tree description.

fells makes the sentence unclear. Replace both occurrences with kills so the wording matches taskkill /t behavior.

Based on static analysis, this wording needs correction.

🧰 Tools
🪛 LanguageTool

[grammar] ~80-~80: Ensure spelling is correct
Context: ...ps to signal, so the taskkill /t that fells the sibling fells the tree under it t...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~80-~80: Ensure spelling is correct
Context: ...he taskkill /t that fells the sibling fells the tree under it too; the sibling's ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@AGENTS.md` at line 80, Update the Windows process-tree description in
AGENTS.md by replacing both occurrences of “fells” with “kills,” preserving the
rest of the sentence.

Source: Linters/SAST tools

Comment thread skills/wordpress-delegate/references/dispatch-and-poll.md
Comment thread skills/wordpress-delegate/scripts/relay.mjs Outdated
`/\b@wordpress\/(blocks|...)\b/` matched nothing anyone would write. A word
boundary has to sit between a word character and a non-word one, and `@` is
non-word, so `\b@` only matches when the mention is glued to a word --
`x@wordpress`. Every real form (`import from '@wordpress/blocks'`, "the
@wordpress/block-editor package") failed it. Dropping the leading `\b` fixes it.

The failure mode is why this one matters more than its size suggests: a signal
that can never fire is invisible. Nothing errors; the lane just scores one lower
than it should, which quietly moves a brief from `high` confidence to `low`, and
a `low` on a read-only lane now demotes it out of the lane entirely. A smoke
check pins the package mention.

Also corrected the subprocess inventory. Both the relay header and
dispatch-and-poll said it shells out to `node` and `git` "and nothing else",
which is not true on Windows: killChild runs `taskkill /t /f` there, because
Windows has no process groups to signal. Now stated the way the README's trust
section already states it for every sibling -- the two commands, plus the
platform termination utility where felling a tree needs one.

Both raised by CodeRabbit on amElnagdy#40.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Amrmohamed1934

Copy link
Copy Markdown
Author

Two of the six are real and are fixed in 7a2379d. Four I'm declining, with reasons — three of them because the change would make this skill the only one in the repo that does it differently.

Fixed

@wordpress signal boundary (major) — correct, and worse than it looks. \b@ requires the boundary to sit between a word character and @, which isn't a boundary, so the pattern only matched glued to a word (x@wordpress). Every real form failed it:

/\b@wordpress\/blocks\b/i.test("import x from '@wordpress/blocks'")  // false
/@wordpress\/blocks\b/i.test("import x from '@wordpress/blocks'")    // true

Worth flagging why this one matters beyond its size: a signal that can never fire is silent. Nothing errors — the lane just scores one lower than it should, which moves a brief from high to low confidence, and a low on a read-only lane now demotes it out of the lane entirely. Smoke check added pinning the package mention.

Subprocess inventory (minor) — correct. The relay header and dispatch-and-poll.md both said node and git "and nothing else", which isn't true on Windows, where killChild runs taskkill /t /f. Fixed, using the phrasing the README's trust section already uses for every sibling — the two commands, plus the platform termination utility where felling a tree needs one — rather than a wording unique to this skill.

Declined

killChild and SIGINT/SIGHUP on Windows — this is a misread. killChild is only ever called two ways: with no signal argument (defaulting to SIGTERM) or explicitly with SIGKILL. The SIGTERM/SIGINT/SIGHUP handler calls killChild(child) — it does not forward sig. So SIGINT on Windows runs taskkill /t /f exactly as SIGTERM does. The early return only skips the second call, the SIGKILL escalation, which is redundant after /f has already force-killed the tree.

The proposed child.kill(signal) fallback would also be a regression: on Windows Node ignores the signal and terminates only that process, leaving the tree the /t was there to reach. This is identical code to all ten sibling relays.

node .../relay.mjs --help and the find ~ fallback. Both are established repo idioms, not inventions here — agy, codex, grok, opencode, pi, and others carry the same two lines, including find ~ -name relay.mjs -path '*codex-delegate*'. find ~ is also generic rather than machine-specific; the guideline bans references to the author's machine or config. Changing it in this one skill makes the newcomer inconsistent for no gain. Worth doing repo-wide if the maintainer wants it — happy to open that separately.

"Plain shell / other agents". Verbatim from the sibling references, and consistent with the vocabulary table: orchestrator is the driving agent, and this bullet is addressed to orchestrators other than Claude Code, which is what the heading distinguishes.

"fells" → "kills". fell (to bring down) is the repo's own term for this — test/relay-smoke.mjs uses "the watchdog fells the whole tree", "must still fell the survivor", and "whole tree felled" in the assertion names this relay is registered under. The LanguageTool hit is a false positive on a real verb. Matching the surrounding vocabulary is the guideline that applies here.

Smoke suite green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants