refactor(fx-dev): define background waits once and record three skill learnings - #26
Merged
Conversation
…tion step The "run it backgrounded, never sleep-poll" rule was restated in five skills, and the new pgrep self-match learning was about to be written twice more. Move the whole rule to fx-dev/skills/dev/references/background-waits.md and reduce every consumer to a pointer plus its own invocation specifics, per the define-once rule the review skill already states for scope definitions. The new reference carries the loop that deadlocked a run for 49 minutes: pgrep -f matches full command lines, so a watcher whose own command line contains the pattern matches itself and never exits. team gains the idle-is-not-completed rule: an idle teammate has stopped and will not wake the coordinator, so filing an idle notification and going quiet produces a mutual stall. learn gains Step 0 — resolve which tree actually provides the skill before editing, since the copy that runs is often not the fx-cc marketplace. Its hardcoded marketplace paths become $FXCC so the rest of the skill operates on the tree Step 0 resolved. Claude-Session: https://claude.ai/code/session_01CvJkjtA5q2GDCLH2MorimY
Codex convergence pass 1 returned five findings, all valid. learn used $FXCC as a shell variable across separate commands. The shell does not persist between tool calls, so every later cd would have run on an empty value, and `FXCC=<path>` is not even valid Bash — the angle brackets parse as a redirection. It is now [TREE], a literal placeholder substituted into each command, matching the [SKILL_BASE_DIR] convention used elsewhere. learn's Step 0 sends the fix to whichever tree actually provides the skill, but every later step assumed the fx-cc plugins/ layout, so a flat catalog stalled before reaching the file. Step 0 now records the layout, and the marketplace-only steps — hook wiring, plugin-cache sync, version bumps — are marked as such. background-waits.md offered `pgrep -x` and `pgrep -f | grep -v $$` as safe process checks. Neither is: -x cannot tell one codex run from another, and grep -v $$ filters PID text by substring and drops only the current shell, leaving a parent whose command line matches. Both are now documented as traps. Three more copies of the foreground-cap rationale and the no-mode-selection rule survived the first pass in dev, team, copilot-review and coderabbit-review; they are now pointers. One cross-reference used the bare filename instead of the canonical fx-dev/skills/dev/references/ form. Separately, pr-preparer carried a verbatim copy of the github skill's 30-line Mechanical body check, justified by never loading that skill. It now loads it, as coder and team already do, and points at the canonical block. Claude-Session: https://claude.ai/code/session_01CvJkjtA5q2GDCLH2MorimY
… dedup Codex convergence pass 2 returned three findings, all valid. Step 0 listed the Claude plugin cache as a tree a skill can load from, then let [TREE] be that path — but the cache is a build artifact, so an edit there is overwritten on the next sync and never reaches a repo. It is now called out explicitly, with the mapping back to the marketplace checkout the cache was copied from. Every [TREE] shell operand is now quoted; the placeholder can be an arbitrary resolved path, and an unquoted one word-splits on whitespace. Two more restatements of the wait rule survived in team. One of them spelled the foreground cap as "600 s" where the reference says "600 000 ms", so the pass-1 sweep grep did not match it. Both are now pointers, and the dev heading no longer half-states the rule it points at. Claude-Session: https://claude.ai/code/session_01CvJkjtA5q2GDCLH2MorimY
Codex convergence pass 3 returned two findings, both valid, both introduced by this branch. background-waits.md claimed to govern waiting on "anything slow". It does not: verify-web-change gates Docker, Compose health and dev-server readiness with bounded in-command poll loops that have no external completion signal and whose next step depends on them. Backgrounding those would break the sequence they exist to order, and team carries this rule into spawned verification agents, so the overclaim could have restructured working readiness checks. The reference is now scoped to external completions and states the distinguishing test: whether anything outside your command will signal that it finished. pr-preparer's new Step 0 said the github skill is canonical for "every title and body rule" and not to restate them, while the skill's own blocking title gate restates them deliberately. Step 0 now claims only the body material that was actually deduplicated. Claude-Session: https://claude.ai/code/session_01CvJkjtA5q2GDCLH2MorimY
There was a problem hiding this comment.
🟢 Approval recommended
The refactor appears consistent and complete, with only a minor grammatical nit identified in the new reference doc.
Pull request overview
This PR consolidates “backgrounded waits, never sleep/poll” guidance into a single canonical reference and updates multiple fx-dev skills to point at it, while also recording operational learnings about process self-matching, agent idle semantics, and editing the correct skill source tree.
Changes:
- Add a new canonical reference file for background wait policy and link existing reviewer/CI wait instructions to it.
- Update
fx-meta:learnto first resolve the authoritative skill source tree (and avoid editing cached/incorrect copies). - Reduce duplicated PR-body-check prose in
fx-dev:pr-preparerby referencing the canonicalfx-dev:githubconventions block instead.
File summaries
| File | Description |
|---|---|
| plugins/fx-meta/skills/learn/SKILL.md | Adds mandatory “resolve the authoritative skill tree” step and adapts workflow to marketplace vs flat-catalog layouts. |
| plugins/fx-meta/.claude-plugin/plugin.json | Bumps fx-meta version for the updated learn skill content. |
| plugins/fx-dev/skills/team/SKILL.md | Repoints coordinator wait policy to the canonical background-waits reference and records the idle vs completed learning. |
| plugins/fx-dev/skills/pr-preparer/SKILL.md | Removes duplicated mechanical PR-body check and requires loading fx-dev:github for canonical conventions. |
| plugins/fx-dev/skills/dev/SKILL.md | Replaces repeated waiter rationale with references to the shared background-waits doc. |
| plugins/fx-dev/skills/dev/references/background-waits.md | New canonical documentation for external-completion waits (backgrounded + logged) and failure modes of hand-rolled waits. |
| plugins/fx-dev/skills/copilot-review/SKILL.md | Links to canonical background-waits policy instead of restating mode-selection/backgrounding rationale. |
| plugins/fx-dev/skills/codex-review/SKILL.md | Links to canonical background-waits policy, including the self-matching pgrep -f pitfall. |
| plugins/fx-dev/skills/coderabbit-review/SKILL.md | Links to canonical background-waits policy instead of repeating foreground-timeout rationale. |
| plugins/fx-dev/.claude-plugin/plugin.json | Bumps fx-dev version for the refactor/documentation updates across skills. |
| .claude-plugin/marketplace.json | Bumps marketplace metadata version to reflect plugin changes. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Why
Three behavioural learnings needed recording, and writing them the obvious way would have added the same prose to two more skills. The "run it backgrounded, never sleep-poll" rule was already restated in five skills, so this defines it once and points the consumers at it.
The learnings
A self-matching process pattern never exits.
until ! pgrep -f 'codex review -c'; do sleep 15; donedeadlocked a run for 49 minutes:pgrep -fmatches full command lines, so the watching shell matched itself and the condition was never true, long after the real process had exited and written its output. Nothing errored and nothing was logged.An
idleteammate has stopped. Only a completed teammate sends a notification. Reading anidle_notificationas "still working" produces a mutual stall — the coordinator waits for a completion that will never fire while the teammate waits for a message that will never be sent.The skill you are correcting is often not in the tree you are editing.
/learnnow resolves which tree actually provided the skill before touching anything, because a fix applied to the wrong copy reports success and changes nothing.The consolidation
plugins/fx-dev/skills/dev/references/background-waits.mdis new and holds the wait rule once.codex-review,copilot-review,coderabbit-review,devandteamkeep their own invocation details — script, log path,STATUStable — and point at it for the rule and its rationale. This follows the define-once conventionreview/SKILL.mdalready states for scope definitions.Separately,
pr-preparercarried a verbatim copy of the github skill's 30-line Mechanical body check, justified by never loading that skill. It now loads it, ascoderandteamalready do, and points at the canonical block. Its title rules stay duplicated on purpose — that block exists to be carried into delegated prompts, and the self-check is this skill's blocking gate.Net effect across the skills is smaller than before this branch, with each rule in one place.
Review
Codex converged over four passes; ten findings, all fixed. The two worth calling out were self-inflicted by the consolidation itself.
$FXCCwas used as a shell variable across separate commands, which cannot work — the shell does not persist between tool calls, andFXCC=<path>is not even valid Bash. It is now"[TREE]", a literal placeholder. And the reference first claimed to govern waiting on "anything slow", which would have forbiddenverify-web-change's bounded in-command readiness loops for Docker, Compose health and dev-server startup; those have no external completion signal and the next step depends on them. It is now scoped to external completions, with the distinguishing test written down.Known follow-up, deliberately not in this PR
The
reviewThreadsGraphQL query is inlined in six skills whilegithub/references/graphql-patterns.mdis the canonical home that onlygithub/SKILL.mdpoints at. That is the largest remaining duplication class in the catalogue and wants its own PR.https://claude.ai/code/session_01CvJkjtA5q2GDCLH2MorimY