fix(node): remove fabricated APIs and duplicated scaffolding #46
Loading…
Reference in a new issue
No description provided.
Delete branch "align/node"
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?
Review of the trimmed skill found five technical claims that fail against the real tools, each verified by probe:
{ retry }option — it is silently ignored — and--test-onlyalone isolates nothing; the flaky-test steps now use--test-name-pattern.node --test --parallelis not a real invocation either.app.close(), matching what fastify-cli generates.mnemoistis an npm 404 (the package ismnemonist), and async-cache-dedupe has noinvalidate(name, arg)— the call isclear.@platformatic/flamesection documented a bare invocation, an--output markdownmode, and aprofile()export that don't exist; corrected against the published v1.7.0 CLI.fastify/close-with-grace, which 404s; the repo ismcollina/close-with-grace.The rest is deletion: the activation checklist and trigger sections that restate rule-file content,
--helptranscriptions, a circular async-leak example, and three near-verbatim restatements of the stuck-processes checklist.Approach review: The approach is sound: correct the claims that fail against the real tools, and delete the scaffolding that restates content the rule files (or the frontmatter triggers) already carry. Splitting the change that way keeps each corrected claim visible in the diff instead of burying it in a rewrite.
I spot-checked the substantive corrections against the actual artifacts rather than taking the description on trust:
@platformatic/flame@1.7.0bin/flame.jsdoes exposerun <script>andgenerate <pprof-file>with--md-format(summary|detailed|adaptive), and noprofile()export — the rewritten section matches the published CLI. Its own help text spells the generate argumentprofile.pb.gz.async-cache-dedupehas bothclear(name, value)andinvalidate(name, references); the latter is reference-based, soclear('getUser', '123')is the right call for a single entry, and it stays consistent with the existing reference-based section that usesinvalidateAll.node:teston v26.5.0 silently ignores{ retry: 3 }— a probe with an always-throwing test executed the body exactly once — so deleting that section rather than rewording it is correct.Two structural notes are attached as comments. Separately, one small factual regression rides along: the graceful-shutdown asset sentence dropped "which CI runs", but
.forgejo/workflows/node-test.ymlstill selectsskills/node/rules/assets/graceful-server.test.ts, so the weaker "demonstrates testing" phrasing understates a guarantee the repo actually holds.No material alternative to the overall approach.
Approach review by Claude Code Opus (opus)
@ -48,3 +23,2 @@- [rules/streams.md](rules/streams.md) - Working with Node.js streams- [rules/caching.md](rules/caching.md) - Caching patterns and libraries- [rules/streams.md](rules/streams.md) - `pipeline()` composition, async-generator transforms, backpressureEnriching this bullet compensates for the deleted streams/caching checklist in the link list, but
Common Workflowsabove now has a step sequence for shutdown, flaky tests, stuck processes, and profiling — and none for the stream-pipeline path, which the frontmatter still advertises as a headline trigger ("backpressure", "stream pipeline", CSV/ETL/ingestion).An alternative that keeps the deletion but preserves symmetry: fold the checklist into one workflow line next to the other four, e.g.
**Streaming a large dataset**: createReadStream → async function* transform → cached enrichment (async-cache-dedupe) → await pipeline() to the destination. That is the part of the removed section that was not a restatement of rule-file content — the numbered checklist and the trigger-word list were, but the ordered pipeline shape was not stated anywhere else at this level.Accepted — added in
77c1e89as the first Common Workflows entry, since streams lead the skill description:**Streaming a large dataset** (CSV, ETL, ingestion): createReadStream → async function* transform → cached enrichment (async-cache-dedupe) → await pipeline() to the destination, linking rules/streams.md and rules/caching.md.Agreed on the split you drew: the ordered end-to-end pipeline shape was the one part of the deleted section not restated elsewhere at this level, while the trigger-word lists and the numbered checklist duplicated the frontmatter and the rule files — those stay deleted.
@ -18,3 +14,3 @@- In stream/ETL scenarios, prefer `async-cache-dedupe` for enrichment calls inside an `async function*` transform.## Memoization with mnemoist## Memoization with mnemonistWorth considering deletion here rather than a name fix. The quick guide directly above names exactly two options (
lru-cache,async-cache-dedupe), and this section then introduces a third package for a joblru-cachealready covers — itsget/set/haswork synchronously, so the memoization example holds verbatim with the library the guide already recommends. A reader following the guide has no rule for whenmnemonistwins.Since the PR's stated aim is removing content that duplicates what is stated elsewhere, folding this example into the LRU Cache section would leave one recommendation per job instead of two libraries with overlapping scope and no selection criterion.
Agreed — deleted the whole section in
77c1e89instead of keeping the name fix.lru-cache'sget/set/hasare synchronous, so the memoization example holds verbatim with a library the quick guide already recommends, and the file offered no criterion for whenmnemonistwould win. That leaves one recommendation per job, which is also where the PR's deletion-first aim points. No references to mnemonist remain anywhere in the repo.Summary: Found 1 medium issue.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -13,3 +13,3 @@```bashpnpx @platformatic/flame app.tspnpx @platformatic/flame run app.ts🟡 Medium: This corrected invocation still uses a
.tsentry point without configuring TypeScript execution. Flame 1.7.0 launches the script with Node and supports Node >=22.6; on supported releases before 22.18, a normal annotation such asconst value: number = 1is parsed as JavaScript and the command exits with a syntax error. Even newer built-in type stripping does not honortsconfigtransforms. Since the skill declares TypeScript execution out of scope, useapp.jshere, or explicitly pass the project's loader (for example--node-options="--import tsx").Fixed in
77c1e89: the example now readspnpx @platformatic/flame run app.js. Since the skill declares TypeScript execution out of scope, a.jsentry point keeps the example valid on every Node release flame supports without smuggling a loader decision into a profiling rule — and it matches theapp.jsthe rest of the file already uses. Preferred over the--node-options="--import tsx"alternative for the same scope reason.Review feedback for head
ab390a36processed in77c1e89.On the regression the approach review's summary flagged (no inline thread, so answering here): confirmed against
.forgejo/workflows/node-test.yml, which still selectsskills/node/rules/assets/graceful-server.test.ts— the "which CI runs" guarantee was real and the cut weakened it. Restored as "pins that shutdown-state behavior with node:test, and CI runs it", keeping the two-sentence structure this PR introduced.The three inline findings are each answered and resolved in their conversations: the streaming workflow line is added to
SKILL.md, the mnemonist section is deleted outright, and the flame example now uses a.jsentry point.Summary: Found 1 medium documentation issue.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -11,3 +9,4 @@For multi-step processes, follow these high-level sequences before consulting the relevant rule file:**Streaming a large dataset** (CSV, ETL, ingestion): `createReadStream` → `async function*` transform → cached enrichment (`async-cache-dedupe`) → `await pipeline()` to the destination. See [rules/streams.md](rules/streams.md) and [rules/caching.md](rules/caching.md).🟡 Medium: This generic workflow now requires both
createReadStreamand cached enrichment for every CSV/ETL/ingestion task. Database or network ingestion may have no file stream, and transforms without repeated remote lookups should not acquire anasync-cache-dedupedependency; the removed guidance correctly made caching conditional on repeated work. Make the source and enrichment stages conditional (for example,source streamand optional cached enrichment when repeated lookups exist) so the skill does not generate unrelated scaffolding.Accepted — fixed in
d123cd3. The line now reads: source stream (createReadStreamfor files) →async function*transform → cached enrichment (async-cache-dedupe) when rows repeat remote lookups →await pipeline()to the destination.The unconditional form contradicted the guidance it links: caching.md already gates stream enrichment on rows repeatedly referencing the same key, so the workflow line was over-prescribing relative to the skill's own rule file. The ordered pipeline shape itself stays — it was added last cycle as the one part of the deleted streams/caching checklist not restated elsewhere at this level — but the source stage is now generic and the enrichment stage carries its condition, so a DB- or network-fed ingestion, or a transform with no repeated lookups, no longer reads as requiring a file stream or an
async-cache-dedupedependency.Summary: Found 1 medium and 1 low issue. The corrections in this PR check out against the real tools — I verified each by probe rather than by recall.
Verified accurate:
{ retry: 3 }onit()is silently ignored on Node 26 (a test that needs 3 attempts still fails on the first), andnode --test --parallelexits withnode: bad option: --parallel, so both removals are correct.--test-name-pattern="test 2", the non-matching first test does not run, so the order-dependent test fails exactly as described.async-cache-dedupe@3.4.0clear(name, value)removes the single entry for that argument (confirmed by call counting);mnemoistis a genuine npm 404.@platformatic/flame@1.7.0shipsflame run <script>andflame generate <pprof>with--md-formatacceptingsummary|detailed|adaptive, andpreload.jswrites<profile>.mdalongside the.pband.htmlon every profiled exit — so the rewritten section matches the published CLI, and the deleted--output markdownflag andprofile()export are absent from the package.mcollina/close-with-graceis the real repo; Fastify core registers no signal handlers.skills/node/rules/assets/graceful-server.test.ts(.forgejo/workflows/node-test.yml), so the graceful-shutdown wording is accurate.Code review by Claude Code Opus (opus)
@ -151,3 +128,3 @@```typescript// Invalidate single entryawait cache.invalidate('getUser', '123');await cache.clear('getUser', '123');🟡 Medium: The
clearfix here is right, but the Reference-Based Invalidation example ~15 lines below is the same class of defect this PR is removing: it builds the cache withstorage: { type: 'memory' }and nooptions.invalidation, and reference invalidation is disabled by default.cache.invalidateAll('user:123')then logsinvalidation is disabled, references are uselessat warn level and returns without evicting anything.Verified against
async-cache-dedupe@3.4.0— withstorage: { type: 'memory' }the underlying function is called once acrossget → invalidateAll → get; withstorage: { type: 'memory', options: { invalidation: true } }it is called twice.Fix:
storage: { type: 'memory', options: { invalidation: true } }in that example (Redis takesoptions: { client, invalidation: true }).Separately, for the record:
cache.invalidate(name, references)does exist in the API, it just takes references rather than the call argument — soclearis the correct call for evicting one argument's entry and this line shouldn't be "corrected" back later.Confirmed by probe against
async-cache-dedupe@3.4.0, matching yours exactly: withstorage: { type: 'memory' }the underlying function runs once acrossget → invalidateAll → get; withoptions: { invalidation: true }it runs twice. Fixed in1704bcf— the example now enables invalidation, with a one-line comment pinning the default-off gotcha, since a silently inertinvalidateAllis precisely the class of defect this PR exists to remove.I weighed deleting the section instead (deletion-first): it stays because it is the third of the file's three invalidation patterns and the enable-flag trap is non-obvious knowledge the fixed example now carries — the un-fixed version of this example is evidence the trap is easy to hit.
On your closing note: agreed and noted —
invalidate(name, references)is real but takes references, soclear('getUser', '123')remains the correct single-entry eviction and won't be "corrected" back.@ -44,3 +20,1 @@await stop();```Every run also writes an LLM-friendly markdown hotspot report beside the .pb profile and HTML flamegraph; regenerate outputs from a saved profile with `flame generate <profile.pb>`, passing `--md-format=detailed` for a fuller report.🟢 Low:
flame generate <profile.pb>invokes a bare binary, but the command directly above it ispnpx @platformatic/flame run app.js— nothing has installedflameonPATH, so a reader following this sequence hitscommand not found. Suggestpnpx @platformatic/flame generate <profile.pb>for consistency with the invocation style used in the rest of the file.Fixed in
1704bcf—pnpx @platformatic/flame generate <profile.pb>, matching the invocation style used everywhere else in the file.Review feedback for head
d123cd39processed in1704bcf:options: { invalidation: true }): fixed in1704bcf— reproduced the reviewer's probe againstasync-cache-dedupe@3.4.0(1 underlying call without the option, 2 with it), enabled invalidation in the example, and kept the section rather than deleting it; details in the thread.flame generatevs the file'spnpxinvocation style): fixed in1704bcf— nowpnpx @platformatic/flame generate <profile.pb>.Summary: No actionable issues found.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
Context for the
0b2f80ereview cycle — this commit restores content a cycle-2 review removed, on maintainer instruction after an adversarial re-verification of every removed-API claim (9 independent probes against shippednode_modulessource and live execution):lru-cache), which was the objection's substance.cache.invalidate(name, references)documented. An earlier cycle's claim that this method does not exist is false — it ships in async-cache-dedupe 3.4.0's source and public typings. The original skill line was still broken (it passed a call argument where a reference belongs), soclear('getUser', '123')stays as the single-entry eviction. The new paragraph documents whatinvalidateactually does, probe-verified both ways: with the default shared storage it evicts matching references across every defined function (identical toinvalidateAll); it narrows to one function only under a per-definestorage.Both additions were verified today against the latest published packages; please review the wording, not the APIs' existence.
Summary: Found 1 low issue. The five technical corrections all hold up against the real tools — I re-probed each rather than taking the PR description on trust:
node --teston v26.5.0 silently ignores{ retry: 3 }(test failed on the first attempt, no retries), so removing that section is correct.async-cache-dedupe:cache.clear('getUser', '123')does evict a single entry (probe: 2 backing calls across 3 gets).invalidateAllis a silent no-op withoutstorage.options.invalidation: true(1 call vs 2 with it enabled). Andinvalidate(name, references)does evict across every defined function on the shared_defaultstorage — a probe withgetUser/getUserPostssharinguser:1showedinvalidate('getUser', 'user:1')evicting both. The new paragraph atcaching.md:184is accurate, including the per-definestoragenarrowing.@platformatic/flamev1.7.0's CLI confirmsrun <script>,generate <pprof-file>, and--md-formatwithsummary|detailed|adaptive;preload.jswritescpu-profile-<ts>.pbalongside the.mdand HTML. The old--output markdownflag andprofile()export do not exist.mnemonist0.40.4 exportsLRUCacheandLRUCacheWithDeletefrom the root,LRUCache.prototype.hasexists, and the typings carry no TTL or disposal options — the new selection-guide bullet is right, andmnemoistis indeed not a package.closeListeners.uninstall()is not an open-handle leak; no finding there.No dangling links to the deleted sections remain (
--test-only,--parallel,fastify/close-with-grace,retry,pino-datadog,Tool Comparisonall grep clean), and section numbering inflaky-tests.mdandstuck-processes-and-tests.mdstays contiguous after the deletions.Code review by Claude Code Opus (opus)
@ -206,3 +211,3 @@## Runnable example[assets/graceful-server.ts](assets/graceful-server.ts) is a complete close-with-grace HTTP server — its health endpoint flips to 503 during shutdown and keep-alive is disabled to drain connections — with the behavior pinned by [assets/graceful-server.test.ts](assets/graceful-server.test.ts), which CI runs.[assets/graceful-server.ts](assets/graceful-server.ts) is a complete close-with-grace HTTP server — its health endpoint flips to 503 during shutdown and keep-alive is disabled to drain connections. [assets/graceful-server.test.ts](assets/graceful-server.test.ts) pins that shutdown-state behavior with node:test, and CI runs it.🟢 Low: This rewrite keeps a claim the asset does not support:
graceful-server.test.tsnever importsgraceful-server.ts. It declares its ownisShuttingDownand a copy-pastedcreateHandler()(lines 7–29 of the test), so it pins a duplicate of the handler, not the runnable example.graceful-server.tscould regress its 503 orConnection: closebehavior and.forgejo/workflows/node-test.ymlwould still be green.Either import the real thing — export
createHandlerfromgraceful-server.tsand have the test build its server from it — or soften the sentence to say the test demonstrates the same shutdown-state behavior rather than pinning the example's.@ -206,3 +211,3 @@## Runnable example[assets/graceful-server.ts](assets/graceful-server.ts) is a complete close-with-grace HTTP server — its health endpoint flips to 503 during shutdown and keep-alive is disabled to drain connections — with the behavior pinned by [assets/graceful-server.test.ts](assets/graceful-server.test.ts), which CI runs.[assets/graceful-server.ts](assets/graceful-server.ts) is a complete close-with-grace HTTP server — its health endpoint flips to 503 during shutdown and keep-alive is disabled to drain connections. [assets/graceful-server.test.ts](assets/graceful-server.test.ts) demonstrates testing the same shutdown-state behavior with node:test against its own dependency-free copy of the handler, and CI runs it.Fixed in
80220efwith your second remedy. This was a regression of an earlier approved fix: the original overclaim was removed inab390a3, then partially reinstated when a later cycle asked for the CI mention back. The import route is deliberately off the table — this repo is package-less andnode-test.ymlruns the test as a direct entrypoint, so importing the asset makesclose-with-graceunresolvable and turns CI red; the dependency-free copy is what lets the test run anywhere the skill travels. The sentence now says the test demonstrates the same behavior against its own copy, which is exactly what a green run proves.Summary: Found 1 low issue. Every factual correction in this PR verified against the real tools.
Probes run against the published artifacts:
@platformatic/flamev1.7.0 —flame --helpconfirmsrun <script>andgenerate <pprof-file>subcommands and--md-format(summary/detailed/adaptive), accepted by both subcommands. Actually runningflame run app.jsandflame run server.js+ SIGINT both wrotecpu-profile-<ts>.pb,.html, and a non-empty.mdhotspot report, so the replacement text onprofiling.md:15-20is accurate. The removed--output markdownflag andprofile()export do not exist in the package.async-cache-dedupev3.4.0 — runtime probe confirmscache.clear('getUser', '123')evicts the single entry, thatinvalidateAllis a no-op withoutstorage: { type: 'memory', options: { invalidation: true } }(memory storage returns early, logging through the default no-op logger), and source confirmsinvalidate(name, references)dispatches to the named wrapper's storage — which is the shared_defaultinstance unlessdefinesupplies its own.caching.md:150,164-165, and184are all correct.mnemonistv0.40.4 — package exists (mnemoistis indeed a 404);LRUCacheis a named export,has()works,deleteisundefinedonLRUCache, andLRUCacheWithDeleteexists withdelete.caching.md:13and17-19check out.close-with-gracev2.5.0 — npmrepository.urlisgithub.com/mcollina/close-with-grace; the oldfastify/path is wrong.fastifyv5.11.2 — noprocess.on('SIG…')anywhere infastify.jsorlib/, so the previous "built-in close-with-grace support" claim was false and the newapp.close()wiring is the right fix.node --test filepasses andnode --test --test-name-pattern="test 2: find user" filefails, so the rewritten symptom onflaky-tests.md:203is accurate. There is no--parallelflag and no{ retry }test option..forgejo/workflows/node-test.ymldoes select and runskills/node/rules/assets/graceful-server.test.ts, and that test defines its own handler rather than importing the asset, sograceful-shutdown.md:213describes it correctly.No dangling links to the deleted sections remain anywhere under
skills/.Code review by Claude Code Opus (opus)
@ -22,24 +22,6 @@ logger.info({ userId: user.id }, 'User created');logger.error({ err, orderId: order.id }, 'Failed to process payment');```## Log Levels🟢 Low: Unlike the other deletions in this PR, this block was not duplicated content — it is the only place in the skill that shows
logger.debugandlogger.warnand says when to reach for each level. After the removal,grep -rn "logger.warn\|logger.debug" skills/returns nothing: the surviving example at lines 21-22 demonstratesinfoanderroronly, and no other rule file covers level selection. That is a real loss of guidance rather than a de-duplication.The PR body justifies the deletions as "activation checklist and trigger sections that restate rule-file content,
--helptranscriptions, a circular async-leak example, and three near-verbatim restatements of the stuck-processes checklist" — this section fits none of those categories. Consider keeping a compressed one-liner in its place, e.g.debugfor per-step tracing,infofor operational events,warnfor handled anomalies,errorfor conditions needing attention.@ -40,4 +25,1 @@logger.error({ err, orderId: order.id }, 'Failed to process payment');```## TransportsDeclining per the approved triage spec. You're right that the PR body's category list doesn't cover this deletion — its recorded ground was different: the no-op test, not de-duplication. A debug/info/warn/error legend is textbook knowledge every competent agent already carries; this repo's triage standard is that skill content earns its tokens by stating what an agent doesn't know, and level semantics are the paradigm case of what it does know. The information-bearing part — pino's call shape with the merging object first — survives at lines 21-22. Restoring a compressed legend would restore the same no-op content at lower cost, not fix a loss. Position final absent a maintainer overrule.