feat: trim the node skill to its unique runtime subjects #34
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/trim-node-skill"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Skills-triage batch 1 (second half): trims
nodefrom 18 files / 3,268 lines to the seven subjects the rule corpus does not cover — streams and backpressure, caching, Pino logging, profiling, graceful shutdown (with its runnable assets), flaky tests, and stuck processes. The dropped files taught what the corpus already owns or contradicts it: type stripping framed around Node 22 (with a false Node 20 claim), barrel-file guidance the module-exports rule forbids, env-schema over Zod against parse-don't-validate, an npm-flavored node_modules layout the global virtual store removed, plus generic async/error/performance material. Rewrites the SKILL.md description around the surviving subjects, converts the one npm install command to pnpm, and moves profiling.md's five ad-hocnpxinvocations topnpx.Approach review: The trim is the right shape. Deleting whole rule files rather than editing them down keeps the skill's boundary crisp — the SKILL.md now states the split ("TypeScript execution, module style, test shape, and environment parsing are owned by the always-on rule corpus") rather than leaving readers to infer it, and the surviving seven subjects are exactly the ones the index still links. The rewritten
descriptionfollows this repo's own documented pattern fromskills/ai-facing-writing-style/SKILL.md(what it does → situations → literal triggers, one per situation), and dropping themetadata: tagsblock matches the dominant convention: only 2 of 29 skills carry one.One leftover:
skills/node/rules/stuck-processes-and-tests.mdstill links[testing.md](testing.md)under "Related rules", and that file is deleted here. SKILL.md's own links are all clean, so the gap is only in rule-to-rule cross-references — a sweep over surviving files for links to deleted paths would close it, and is worth folding into the later triage batches too.Two observations, neither blocking.
skills/fastify-zod-reference/rules/typescript.mdstill teaches type stripping and--experimental-strip-types, so the material this PR removes as corpus-owned survives in a sibling skill; that reads like a later batch, but the dedupe is incomplete until it lands. Andrules/assets/graceful-server.ts/graceful-server.test.tsare reachable from no file — pre-existing, not caused by this diff, but since the index section was rewritten anyway it was a cheap place to point at them.Approach review by Claude Code Opus (opus)
@ -52,3 +52,2 @@- ESM / modern Node: `npm i -D why-is-node-running`- older CommonJS projects: `npm i -D why-is-node-running@v2`- ESM / modern Node: `pnpm add -D why-is-node-running`This file survives the trim, but its "Related rules" section at the bottom still links
[testing.md](testing.md), which this PR deletes. An agent following the pointer spends a read on a missing path. Consider making a sweep for links into deleted rule files part of each triage batch — SKILL.md's index was updated correctly, so only rule-to-rule cross-references were missed.Summary: Found 1 medium issue.
The trim is coherent:
SKILL.mdno longer links any removed rule, the surviving seven rules cross-reference only files that still exist (except the one flagged below), the rewrittendescriptioncovers exactly the surviving subjects, and dropping themetadata:block matches the 27-of-29 skills that have none. Thenpm i -D→pnpm add -Dconversion is correct, includingwhy-is-node-running@v2:v2parses as the semver range>=2.0.0 <3.0.0-0and resolves to the published 2.3.0, so the older-CommonJS install still works. No other file in the repo references the eight deleted rules (theerror-handling.md/testing.md/performance.md/typescript.mdhits elsewhere arefastify-zod-reference's own files).Code review by Claude Code Opus (opus)
@ -1,218 +0,0 @@---🟡 Medium: Deleting this file leaves a dangling link behind.
skills/node/rules/stuck-processes-and-tests.md:123still lists- [testing.md](testing.md)under its "Related rules" section, and that file is retained by this PR (it is edited two hunks earlier for the pnpm change). An agent following the stuck-process rule will try to read a rule file that no longer ships, wasting a tool call and losing the pointer entirely.Drop that line from the "Related rules" list, leaving
flaky-tests.mdandgraceful-shutdown.md, which both still exist.Adjudication for review findings 41983 and 41988 (both slots converged on the same defect):
Accepted and fixed in
2fd49d7— the- [testing.md](testing.md)line is dropped from the "Related rules" list inskills/node/rules/stuck-processes-and-tests.md;flaky-tests.mdandgraceful-shutdown.mdremain.The suggested sweep ran over every surviving
skills/node/file against all eight deleted rule paths: line 123 was the only reference. That sweep is now part of the standing checklist for each triage batch.Two non-blocking observations from the approach review, recorded rather than actioned here: the type-stripping duplication in
fastify-zod-referenceis deleted by PR #33 in this same batch, and the unreferencedrules/assets/graceful-server.*pair is pre-existing (and pinned by thenode-test.ymlstaleness contract) — left for the estate triage rather than a drive-by edit.Summary: No actionable issues found; 1 informational note.
This is a documentation-only trim of the
nodeskill: eight rule files deleted,SKILL.mdfrontmatter/description rewritten around the seven surviving subjects, and one install command converted to pnpm.What I verified in the working tree:
rules/*.mdlink left inSKILL.md(streams, caching, logging, profiling, graceful-shutdown, flaky-tests, stuck-processes-and-tests) resolves to a file that still exists. Grepping the whole repo for the eight deleted filenames finds onlyskills/fastify-zod-reference, which has its own same-named rules — no cross-skill breakage. The only in-repo cross-reference to a deleted file (testing.mdinstuck-processes-and-tests.md) is removed by the follow-up commit, and no prose-style references ("see the X rule") remain.name+descriptionare present; the new description is 600 characters, well under the 1024-character skill-description limit. Droppingmetadata.tagsmatches the repo majority — onlyfastify-zod-referenceandtypescript-magiciancarry ametadatablock.pnpm add -D why-is-node-running@v2resolves (^2.3.0) — the leadingvis accepted as a semver range, so the version pin for CommonJS projects is preserved.skills/node/rules/assets/graceful-server.test.ts(the file thenode-testworkflow selects) still passes under Node 26.5.0: 4/4 tests green.Code review by Claude Code Opus (opus)
@ -35,3 +8,1 @@```See [rules/typescript.md](rules/typescript.md) for complete configuration and examples.Use this skill for Node.js runtime work in the subjects below. TypeScript execution, module style, test shape, and environment parsing are owned by the always-on rule corpus; this skill covers what the corpus does not.ℹ️ Info: This sentence makes the skill's completeness depend on an "always-on rule corpus" that this repository does not ship — the README states the source has no
rules/directory, and the module-exports rule cited in the PR description is not in this repo'sAGENTS.mdeither. Delivery-wise that is fine (the corpus comes from the agent's global rules), but a consumer who selectsnodein a workspace without that corpus now gets neither the TypeScript-execution/module/test/env guidance nor a pointer to where it lives. Naming the corpus source in this sentence would make the dependency recoverable; no change needed if every target agent is guaranteed to load it.Adjudication for review finding 42012 (Info):
Declined, with the rationale the finding itself anticipates. The "always-on rule corpus" is the estate's generated agent instructions (rendered into every target's global rule files), so every agent this estate deploys is guaranteed to load it — the finding's own no-change condition. Naming the source in the sentence would not make the dependency recoverable for an outside consumer either: the corpus lives in a private repository, so a pointer would name something unreachable. For such a consumer the skill remains self-contained for its seven subjects; the sentence only explains why the adjacent topics are absent, it defers no guidance a reader here needs.
The cycle also independently confirmed the 41988 fix — no dangling or prose-style references to the deleted rule files survive at this head.
Summary: No actionable issues found.
Reviewed the trim of
skills/node(8 rule files deleted,SKILL.mdrewritten, one install note converted to pnpm) against the working tree:SKILL.mdnow points only atstreams.md,caching.md,logging.md,profiling.md,graceful-shutdown.md,flaky-tests.md, andstuck-processes-and-tests.md, all of which exist. A repo-wide grep for the deleted names (typescript.md,async-patterns.md,error-handling.md,modules.md,node-modules-exploration.md,performance.md,environment.md,testing.md) returns nothing, and the survivingstuck-processes-and-tests.md"Related rules" list correctly dropstesting.mdwhile keeping the two links that still resolve.node:test(colon not followed by a space) and embedded double quotes mid-scalar, neither of which breaks YAML plain-scalar parsing. At 600 characters it is well inside the 1,024-character skill description budget, and droppingmetadata.tagsmatches every other skill inskills/, none of which sets it.pnpm add -Dis the direct equivalent ofnpm i -D, andwhy-is-node-running@v2still resolves the 2.x line under pnpm's semver range handling (a leadingvis accepted in a range). The only other package-manager invocations left in the skill arenpxruns inprofiling.md, not installs, so the body's "one npm install command" claim holds.One pre-existing observation, unrelated to these changes:
skills/node/rules/assets/graceful-server.tsandgraceful-server.test.tsare not referenced fromgraceful-shutdown.mdor any other rule file, and were not referenced by any of the deleted files either — so this PR did not orphan them, they were already unlinked.Code review by Claude Code Opus (opus)
Summary: Found 2 low-severity documentation issues; no correctness or security defects.
Verified against the working tree: no markdown link in the repository still points at any of the eight deleted rule files, every remaining link in
skills/noderesolves, no surviving rule file depends on symbols or prose defined in the removed ones, and the new frontmatter parses (plain YAML scalar, no:in the value, 600 chars) and matches thename/description-only shape every other skill here uses. The.forgejo/workflows/node-test.ymlcandidate-file expectation is unaffected — the deletions are all.md, and the one tracked test (skills/node/rules/assets/graceful-server.test.ts) is retained.Minor observation, not attached as a comment because the lines are unchanged:
rules/profiling.mdstill invokes tools vianpx(lines 15, 25, 56, 59, 62) while this PR moves install commands to pnpm, so the file is now mixed between the two package managers.Code review by Claude Code Opus (opus)
@ -35,3 +8,1 @@```See [rules/typescript.md](rules/typescript.md) for complete configuration and examples.Use this skill for Node.js runtime work in the subjects below. TypeScript execution, module style, test shape, and environment parsing are owned by the always-on rule corpus; this skill covers what the corpus does not.🟢 Low: This defers four subjects to an "always-on rule corpus" that does not travel with the skill. This repository ships skills only (no
rules/tree, per the README), so an agent that receives just this skill — the normalaxskillsdelivery for an external consumer — reads an explicit statement that TypeScript execution, module style, test shape, and environment parsing are covered elsewhere, and then gets no coverage at all. That is the failure modeskills/ai-facing-writing-style/SKILL.md:126warns about: "Keep every pointer target inside the skill directory: the directory is the unit that travels, so an outside target may not exist where the skill runs."Stating the scope positively avoids the dangling claim, e.g. "Covers Node.js runtime work in the subjects below; other Node.js and TypeScript conventions come from the project's own rules."
@ -170,3 +170,3 @@1. **Establish baseline** - Run autocannon to get initial metrics2. **Profile** - Use @platformatic/flame to identify hotspots3. **Optimize** - Fix the identified bottlenecks3. **Optimize** - Fix the identified bottlenecks; offload a hot function that is CPU-bound and cannot be made cheaper to a worker pool (`piscina`), never a hand-rolled `node:worker_threads` pool🟢 Low:
piscinais named here and in theSKILL.mdprofiling workflow, but this clause is the only mention anywhere in the skill — no install command, no pool sizing, and no teardown. Teardown is the concrete risk: aPiscinainstance keeps its worker threads alive untildestroy()is awaited, which produces exactly the "process did not exit" / open-handle symptom thatrules/stuck-processes-and-tests.md:63lists as a hang cause. Following this step and then debugging the resulting hang sends a reader between two rules in the same skill, neither of which connects them.A short example alongside the clause (construct the pool once,
await pool.destroy()in the shutdown path) would close the gap, or linkgraceful-shutdown.mdfrom this step.Adjudication for review findings 42241 and 42242, both accepted and fixed in
ecd29a4:ai-facing-writing-style/SKILL.md:126without pointing anywhere that doesn't travel with the skill.await pool.destroy()in the shutdown path, linksgraceful-shutdown.md, and names the open-handle consequence.The summary's unattached observation is also applied:
profiling.md's fivenpxinvocations are nowpnpx, matching the pnpm conversion this PR already made.Summary: Found 1 medium issue and 1 informational note. The trim itself is clean — every surviving cross-link in
skills/noderesolves, no file elsewhere in the repository references a deleted rule, dropping themetadata:block matches every otherSKILL.mdhere, and the 600-character description stays well inside the frontmatter budget.The one real defect is the new piscina sentence in
rules/profiling.md: the claim that a live pool keeps the process from exiting is false (verified against piscina 5.3.0 and 4.9.3 on Node 26 — piscina unrefs its workers and ports), anddestroy()aborts in-flight tasks where the linked graceful-shutdown workflow wants them drained viaclose().Code review by Claude Code Opus (opus)
@ -54,3 +54,3 @@```bash# Basic benchmarknpx autocannon http://localhost:3000pnpx autocannon http://localhost:3000ℹ️ Info:
pnpxis still shipped by pnpm (v11 maps thepnpxbin topnpm dlx, andflame/autocannonresolve fine), so these commands work. Two behavioural notes, no action required unless you want the corpus uniform:pnpm dlxalways fetches from the registry and never prefers a locally installed binary the waynpxdoes, sopnpx autocannonre-downloads even when autocannon is a devDependency; and the rest of this repository's skills (verify-readme,verify-unixy-cli,canonicalize-tailwind) prescribenpx -yfor runnable examples.@ -170,3 +170,3 @@1. **Establish baseline** - Run autocannon to get initial metrics2. **Profile** - Use @platformatic/flame to identify hotspots3. **Optimize** - Fix the identified bottlenecks3. **Optimize** - Fix the identified bottlenecks; offload a hot function that is CPU-bound and cannot be made cheaper to a worker pool (`piscina`), never a hand-rolled `node:worker_threads` pool. Construct the pool once and `await pool.destroy()` in the shutdown path (see [graceful-shutdown.md](graceful-shutdown.md)) — a live pool's worker threads keep the process from exiting🟡 Medium: The stated reason for the teardown is wrong, and the chosen method contradicts the shutdown rule it links to.
Piscina unrefs both the worker and its
MessagePortwhile idle (src/worker_pool/index.ts:workerInfo.init(...).workerUnref(), and_handleResponseunrefs the port oncetaskInfos.size === 0, with the comment "No more tasks running on this Worker means it should not keep the process running"). I verified this on piscina 5.3.0 and 4.9.3 under Node 26: a script that constructs a pool, awaits a task, and never callsdestroy()exits with status 0; so does one that constructs a pool withminThreads: 4and never runs anything. So "a live pool's worker threads keep the process from exiting" is false, and it directly misleads the sibling stuck-processes-and-tests.md workflow into blaming a live pool for a hang it cannot cause.Second,
destroy()"stops all Workers and rejects allPromises for pending tasks" — it aborts in-flight work, which is the opposite of the drain step in graceful-shutdown.md.close()(present in piscina 4 and 5) waits for started tasks to finish.Suggested fix:
Disposition for review 9813 (head
ecd29a4):42d92f6with the suggested line verbatim. Verified the load-bearing claim independently in a fresh sandbox: piscina 5.3.0,minThreads: 4, one completed task, noclose()/destroy()— the process exits with status 0, so "a live pool keeps the process from exiting" was false, andclose()is the drain-shaped call the linked graceful-shutdown workflow wants.pnpxvsnpx -y): declined, no change.pnpxis deliberate on a pnpm-standard fleet, and the dlx cache keeps repeat fetches cheap. Thenpx -yprescriptions inverify-readme,verify-unixy-cli, andcanonicalize-tailwindare queued for their own triage batches — corpus uniformity is better settled there than by flipping this file back now.Summary: Docs-only trim of the
nodeskill. I checked the surviving tree for fallout from the eight deleted rule files: no markdown link inskills/node(or anywhere else in the repo) still points attypescript.md,error-handling.md,async-patterns.md,modules.md,testing.md,environment.md,performance.md, ornode-modules-exploration.md, and dropping themetadata:block fromSKILL.mdmatches every other skill inskills/(none carry one). The new piscina guidance is accurate against piscina 5.3.0:close()waits for started tasks whiledestroy()rejects pending ones. Two smaller points below: one shutdown-timing mismatch in the new piscina sentence, and one note on thenpx->pnpxconversions.Code review by Claude Code Opus (opus)
@ -13,3 +13,3 @@```bashnpx @platformatic/flame app.tspnpx @platformatic/flame app.tsℹ️ Info: The PR description says it "converts the one npm install command to pnpm", but this file also converts five
npxinvocations topnpx.pnpxis still shipped by pnpm (it is in thebinmap of pnpm 11.20.0) and aliasespnpm dlx, so the commands work — butpnpm dlxalways fetches from the registry, whereasnpxprefers a binary already in the project'snode_modules/.bin. For a project that pinsautocannonor@platformatic/flameas a devDependency, these lines now silently benchmark against a freshly resolved version instead of the pinned one, and they require pnpm to be installed. Every other skill in this repo (andAGENTS.md) usesnpx -y.@ -170,3 +170,3 @@1. **Establish baseline** - Run autocannon to get initial metrics2. **Profile** - Use @platformatic/flame to identify hotspots3. **Optimize** - Fix the identified bottlenecks3. **Optimize** - Fix the identified bottlenecks; offload a hot function that is CPU-bound and cannot be made cheaper to a worker pool (`piscina`), never a hand-rolled `node:worker_threads` pool. Construct the pool once and `await pool.close()` in the shutdown path (see [graceful-shutdown.md](graceful-shutdown.md)) so in-flight tasks drain instead of being aborted by `destroy()`🟡 Medium: This points a reader at
graceful-shutdown.mdfor the pool teardown, but that file has no worker-pool section at all, and its twoclose-with-graceexamples usedelay: 10000. Piscina'scloseTimeoutdefaults to 30000 ms (piscina 5.3.0 README: "An optional time (in milliseconds) to wait for the pool to complete all in-flight tasks whenclose()is called. The default is 30000"), so anawait pool.close()placed inside that 10 s handler can still be waiting when close-with-grace force-exits the process — the in-flight tasks are killed anyway, which is exactly the outcome this sentence saysclose()avoids.Either construct the pool with a
closeTimeoutbelow the shutdown delay (e.g.new Piscina({ filename, closeTimeout: 8000 })underdelay: 10000), or say the delay must exceedcloseTimeout. Adding the pool-close step tograceful-shutdown.mdwould also make the cross-reference land on something.Disposition for review 9821 (head
42d92f6):ae3ebd8. Verified both numbers independently: piscina's dist hardcodescloseTimeout: 30000, and graceful-shutdown.md's primary examples usedelay: 10000. Took the state-the-constraint remedy — the sentence now requirescloseTimeoutbelow the shutdown handler's force-exit delay, so the constraint travels with the guidance. Deliberately did not add a pool section to graceful-shutdown.md: the cross-reference points at the shutdown ordering (drain before exit), and growing that file is out of scope for a trim PR.npx→pnpxconversions alongside the install command. The flip back tonpx -ystays declined for the reasons in the 42337 disposition; the cross-skill uniformity question is flagged for the triage batches coveringverify-readme,verify-unixy-cli, andcanonicalize-tailwind.Summary: No actionable defects found; 1 informational note.
Verified against the working tree and upstream sources:
typescript.md,async-patterns.md,error-handling.md,modules.md,node-modules-exploration.md,performance.md,testing.md) are no longer referenced fromSKILL.md, the surviving rules, any other skill in the repo, or.forgejo/workflows/node-test.yml(which only runsskills/node/rules/assets/graceful-server.test.ts, untouched here).metadata.tagsblock matches every otherskills/*/SKILL.mdin this repo, and the new 613-char description stays within the usual limits.profiling.md: checked against piscina 5.3.0's README —close()waits for started tasks whiledestroy()rejects pending ones, andcloseTimeoutdoes default to30000. The warning to keep it below the force-exit delay is consistent withgraceful-shutdown.md, whose examples use 10 s / 25 s budgets.npx→pnpx: confirmedpnpxstill ships in current pnpm (bin present in pnpm 11.20.0, delegating todlx) and thatpnpm dlxstops option parsing at the command name, sopnpx autocannon -c 100 -d 30 -p 10 ...passes-cthrough to autocannon rather than consuming it as--shell-mode. Bin namesflameandautocannonresolve from their package names.One pre-existing observation outside this diff:
rules/assets/graceful-server.tsandgraceful-server.test.tsare not linked fromgraceful-shutdown.mdorSKILL.md, so nothing routes a reader to the runnable assets the PR description highlights as retained.Code review by Claude Code Opus (opus)
@ -13,3 +13,3 @@```bashnpx @platformatic/flame app.tspnpx @platformatic/flame app.tsℹ️ Info:
pnpxis a thin alias that rewritesargvtopnpm dlx, so these examples always resolve the package from the registry into a temporary store and never reuse a locally installed copy — unlikenpx, which prefersnode_modules/.bin. That is the right behaviour for one-off profiling tools, so no change is needed; just note it diverges from the rest of the corpus, whereverify-readmeandverify-unixy-cliprescribenpx -y <tool>for documented invocations.Disposition for review 9828 (head
ae3ebd8):verify-readme/verify-unixy-clitriage batches.c6b081e—graceful-shutdown.mdnow closes with a "Runnable example" section routing readers toassets/graceful-server.tsand its CI-run test, so the assets the PR retains are discoverable instead of dead weight.Summary: No actionable issues found; 1 informational note.
This is a documentation-only trim of the
nodeskill (8 rule files deleted,SKILL.mdrewritten, 3 rule files touched). I checked the things a deletion-heavy change usually breaks:rules/*.mdlink inskills/node/SKILL.mdresolves to a file that still exists, and no file anywhere in the repo (or in the other 24 skills) references the deletedtypescript.md,error-handling.md,async-patterns.md,environment.md,modules.md,node-modules-exploration.md,performance.md, ortesting.md. Thetesting.mdentry was correctly dropped from the "Related rules" list instuck-processes-and-tests.md.SKILL.mdfrontmatter parses cleanly as YAML into exactlyname+description(600 bytes, well inside the 1,024-char skill description limit). Dropping themetadata.tagsblock matches the other 24 skills, none of which carry one.graceful-shutdown.md's added "Runnable example" paragraph is accurate:assets/graceful-server.tsdoes flip/healthto 503 and setConnection: closewhile shutting down, and.forgejo/workflows/node-test.ymldoes selectskills/node/rules/assets/graceful-server.test.ts(base64 entry decoded and confirmed), so "which CI runs" holds. No deleted file matched the workflow's tracked-test orpackage.jsonglobs, so the stale-candidate guard won't trip.close()waits for started tasks whiledestroy()aborts them, andcloseTimeoutdoes default to30000ms, so the added advice inprofiling.mdstep 3 is factually right.pnpxis real. pnpm 11.20.0 still ships apnpxbin; itsbin/pnpx.mjsrewrites argv todlxand delegates topnpm.mjs, so the five converted invocations work.Code review by Claude Code Opus (opus)
@ -13,3 +13,3 @@```bashnpx @platformatic/flame app.tspnpx @platformatic/flame app.tsℹ️ Info:
pnpxresolves differently from thenpxit replaces — pnpm'sbin/pnpx.mjssimply rewrites argv todlx, andpnpm dlxalways fetches the package into a temporary store, whereasnpxprefers a matching binary already installed in the project. For one-off tools like@platformatic/flameandautocannonthat is the intended behavior, so no change is needed; just note that a project pinning@platformatic/flameas a devDependency will profile with whatever version the registry resolves instead of the pinned one.Disposition for review 9834 (head
c6b081e):npx -yskills.No other findings — second consecutive clean review, and the Runnable example claims were verified against the assets and the CI workflow.