fix(node): remove fabricated APIs and duplicated scaffolding #46

Merged
jercik merged 6 commits from align/node into main 2026-08-07 07:53:00 +00:00
Owner

Review of the trimmed skill found five technical claims that fail against the real tools, each verified by probe:

  • node:test has no { retry } option — it is silently ignored — and --test-only alone isolates nothing; the flaky-test steps now use --test-name-pattern. node --test --parallel is not a real invocation either.
  • The Fastify graceful-shutdown section claimed Fastify handles signals itself; it registers no signal handlers, so the documented hooks never ran on SIGTERM. The section now wires close-with-grace to app.close(), matching what fastify-cli generates.
  • mnemoist is an npm 404 (the package is mnemonist), and async-cache-dedupe has no invalidate(name, arg) — the call is clear.
  • The @platformatic/flame section documented a bare invocation, an --output markdown mode, and a profile() export that don't exist; corrected against the published v1.7.0 CLI.
  • The close-with-grace link pointed at fastify/close-with-grace, which 404s; the repo is mcollina/close-with-grace.

The rest is deletion: the activation checklist and trigger sections that restate rule-file content, --help transcriptions, a circular async-leak example, and three near-verbatim restatements of the stuck-processes checklist.

Review of the trimmed skill found five technical claims that fail against the real tools, each verified by probe: - node:test has no `{ retry }` option — it is silently ignored — and `--test-only` alone isolates nothing; the flaky-test steps now use `--test-name-pattern`. `node --test --parallel` is not a real invocation either. - The Fastify graceful-shutdown section claimed Fastify handles signals itself; it registers no signal handlers, so the documented hooks never ran on SIGTERM. The section now wires close-with-grace to `app.close()`, matching what fastify-cli generates. - `mnemoist` is an npm 404 (the package is `mnemonist`), and async-cache-dedupe has no `invalidate(name, arg)` — the call is `clear`. - The `@platformatic/flame` section documented a bare invocation, an `--output markdown` mode, and a `profile()` export that don't exist; corrected against the published v1.7.0 CLI. - The close-with-grace link pointed at `fastify/close-with-grace`, which 404s; the repo is `mcollina/close-with-grace`. The rest is deletion: the activation checklist and trigger sections that restate rule-file content, `--help` transcriptions, a circular async-leak example, and three near-verbatim restatements of the stuck-processes checklist.
fix(node): remove fabricated APIs and duplicated scaffolding
Some checks failed
Node tests / node:test (pull_request) Successful in 16s
commit-msg / commitlint (pull_request) Successful in 30s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m1s
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Failing after 49s
PR Review / forgejo-review-approach-3 generator (pull_request_target) Failing after 49s
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 48s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Successful in 3m53s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 6m14s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 6m30s
ab390a3654
forgejo-actions left a comment

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.0 bin/flame.js does expose run <script> and generate <pprof-file> with --md-format (summary | detailed | adaptive), and no profile() export — the rewritten section matches the published CLI. Its own help text spells the generate argument profile.pb.gz.
  • async-cache-dedupe has both clear(name, value) and invalidate(name, references); the latter is reference-based, so clear('getUser', '123') is the right call for a single entry, and it stays consistent with the existing reference-based section that uses invalidateAll.
  • node:test on 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.yml still selects skills/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)

**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.0` `bin/flame.js` does expose `run <script>` and `generate <pprof-file>` with `--md-format` (`summary` | `detailed` | `adaptive`), and no `profile()` export — the rewritten section matches the published CLI. Its own help text spells the generate argument `profile.pb.gz`. - `async-cache-dedupe` has both `clear(name, value)` and `invalidate(name, references)`; the latter is reference-based, so `clear('getUser', '123')` is the right call for a single entry, and it stays consistent with the existing reference-based section that uses `invalidateAll`. - `node:test` on 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.yml` still selects `skills/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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6ImFiMzkwYTM2NTQ0MjMwMzI1Y2JjMjc1MjljNGYxYjBhZjVkZjBiOWMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtc21hcnQtMSIsInNvdXJjZVdvcmtmbG93UnVuSWQiOiIxNzczMiIsInNvdXJjZUdlbmVyYXRvckF0dGVtcHQiOiIxIiwicmVzdWx0SWQiOiIwYmRiZmZmMS0xZjcxLTQyMzQtODU1Mi04Zjk1MTFmZGU1MjcifQ== -->
@ -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, backpressure

Enriching this bullet compensates for the deleted streams/caching checklist in the link list, but Common Workflows above 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.

Enriching this bullet compensates for the deleted streams/caching checklist in the link list, but `Common Workflows` above 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.
Author
Owner

Accepted — added in 77c1e89 as 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.

Accepted — added in 77c1e89 as 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.
jercik marked this conversation as resolved
@ -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 mnemonist

Worth 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 job lru-cache already covers — its get/set/has work synchronously, so the memoization example holds verbatim with the library the guide already recommends. A reader following the guide has no rule for when mnemonist wins.

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.

Worth 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 job `lru-cache` already covers — its `get`/`set`/`has` work synchronously, so the memoization example holds verbatim with the library the guide already recommends. A reader following the guide has no rule for when `mnemonist` wins. 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.
Author
Owner

Agreed — deleted the whole section in 77c1e89 instead of keeping the name fix. lru-cache's get/set/has are synchronous, so the memoization example holds verbatim with a library the quick guide already recommends, and the file offered no criterion for when mnemonist would 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.

Agreed — deleted the whole section in 77c1e89 instead of keeping the name fix. `lru-cache`'s `get`/`set`/`has` are synchronous, so the memoization example holds verbatim with a library the quick guide already recommends, and the file offered no criterion for when `mnemonist` would 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.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 1 medium issue.

Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)

**Summary:** Found 1 medium issue. _Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6ImFiMzkwYTM2NTQ0MjMwMzI1Y2JjMjc1MjljNGYxYjBhZjVkZjBiOWMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3NzMyIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjIxM2I2ZTk4LTc2NmItNDgwNC1hMDBlLTZiMWZmMjRlYTVhYyJ9 -->
@ -13,3 +13,3 @@
```bash
pnpx @platformatic/flame app.ts
pnpx @platformatic/flame run app.ts

🟡 Medium: This corrected invocation still uses a .ts entry 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 as const value: number = 1 is parsed as JavaScript and the command exits with a syntax error. Even newer built-in type stripping does not honor tsconfig transforms. Since the skill declares TypeScript execution out of scope, use app.js here, or explicitly pass the project's loader (for example --node-options="--import tsx").

🟡 **Medium:** This corrected invocation still uses a `.ts` entry 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 as `const value: number = 1` is parsed as JavaScript and the command exits with a syntax error. Even newer built-in type stripping does not honor `tsconfig` transforms. Since the skill declares TypeScript execution out of scope, use `app.js` here, or explicitly pass the project's loader (for example `--node-options="--import tsx"`).
Author
Owner

Fixed in 77c1e89: the example now reads pnpx @platformatic/flame run app.js. Since the skill declares TypeScript execution out of scope, a .js entry point keeps the example valid on every Node release flame supports without smuggling a loader decision into a profiling rule — and it matches the app.js the rest of the file already uses. Preferred over the --node-options="--import tsx" alternative for the same scope reason.

Fixed in 77c1e89: the example now reads `pnpx @platformatic/flame run app.js`. Since the skill declares TypeScript execution out of scope, a `.js` entry point keeps the example valid on every Node release flame supports without smuggling a loader decision into a profiling rule — and it matches the `app.js` the rest of the file already uses. Preferred over the `--node-options="--import tsx"` alternative for the same scope reason.
jercik marked this conversation as resolved
fix(node): apply review feedback on streaming workflow, caching, profiling
Some checks failed
Node tests / node:test (pull_request) Successful in 16s
commit-msg / commitlint (pull_request) Successful in 34s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m25s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 43s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 4m36s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 4m54s
77c1e89b7c
- add the streaming-pipeline workflow line to Common Workflows, preserving
  the ordered pipeline shape the deleted checklist alone carried
- drop the mnemonist memoization section: third overlapping package with
  no selection criterion beside the two the quick guide recommends
- use a .js entry point in the flame example; TypeScript execution is
  declared out of scope by this skill
- restore the CI-run guarantee for graceful-server.test.ts

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Review feedback for head ab390a36 processed in 77c1e89.

On the regression the approach review's summary flagged (no inline thread, so answering here): confirmed against .forgejo/workflows/node-test.yml, which still selects skills/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 .js entry point.

Review feedback for head ab390a36 processed in 77c1e89. On the regression the approach review's summary flagged (no inline thread, so answering here): confirmed against `.forgejo/workflows/node-test.yml`, which still selects `skills/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 `.js` entry point.
forgejo-actions left a comment

Summary: Found 1 medium documentation issue.

Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)

**Summary:** Found 1 medium documentation issue. _Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6Ijc3YzFlODliN2M1ZWFiMWQ0N2RhYmE4YTk5ZWFlOWQ0NzMxNTc5MmEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3ODA5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjcwMzc2NmJjLTFlNDMtNDg4Yy05MWMzLWIyNTFkZGNhOGRkMSJ9 -->
@ -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 createReadStream and 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 an async-cache-dedupe dependency; the removed guidance correctly made caching conditional on repeated work. Make the source and enrichment stages conditional (for example, source stream and optional cached enrichment when repeated lookups exist) so the skill does not generate unrelated scaffolding.

🟡 **Medium:** This generic workflow now requires both `createReadStream` and 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 an `async-cache-dedupe` dependency; the removed guidance correctly made caching conditional on repeated work. Make the source and enrichment stages conditional (for example, `source stream` and optional cached enrichment when repeated lookups exist) so the skill does not generate unrelated scaffolding.
Author
Owner

Accepted — fixed in d123cd3. The line now reads: source stream (createReadStream for 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-dedupe dependency.

Accepted — fixed in d123cd3. The line now reads: source stream (`createReadStream` for 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-dedupe` dependency.
jercik marked this conversation as resolved
fix(node): condition streaming workflow source and enrichment stages
Some checks failed
Node tests / node:test (pull_request) Successful in 15s
commit-msg / commitlint (pull_request) Successful in 27s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m1s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 30s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 4m37s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 4m58s
d123cd399d
The workflow line prescribed createReadStream and async-cache-dedupe for
every ingestion shape; caching.md itself gates enrichment on repeated
lookups, and non-file sources have no file stream.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
forgejo-actions left a comment

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 } on it() is silently ignored on Node 26 (a test that needs 3 attempts still fails on the first), and node --test --parallel exits with node: bad option: --parallel, so both removals are correct.
  • The reworked "Test Order Dependencies" symptom reproduces: with --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.0 clear(name, value) removes the single entry for that argument (confirmed by call counting); mnemoist is a genuine npm 404.
  • @platformatic/flame@1.7.0 ships flame run <script> and flame generate <pprof> with --md-format accepting summary|detailed|adaptive, and preload.js writes <profile>.md alongside the .pb and .html on every profiled exit — so the rewritten section matches the published CLI, and the deleted --output markdown flag and profile() export are absent from the package.
  • mcollina/close-with-grace is the real repo; Fastify core registers no signal handlers.
  • CI does run skills/node/rules/assets/graceful-server.test.ts (.forgejo/workflows/node-test.yml), so the graceful-shutdown wording is accurate.
  • No dangling links remain to the removed sections anywhere in the repo.

Code review by Claude Code Opus (opus)

**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 }` on `it()` is silently ignored on Node 26 (a test that needs 3 attempts still fails on the first), and `node --test --parallel` exits with `node: bad option: --parallel`, so both removals are correct. - The reworked "Test Order Dependencies" symptom reproduces: with `--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.0` `clear(name, value)` removes the single entry for that argument (confirmed by call counting); `mnemoist` is a genuine npm 404. - `@platformatic/flame@1.7.0` ships `flame run <script>` and `flame generate <pprof>` with `--md-format` accepting `summary|detailed|adaptive`, and `preload.js` writes `<profile>.md` alongside the `.pb` and `.html` on every profiled exit — so the rewritten section matches the published CLI, and the deleted `--output markdown` flag and `profile()` export are absent from the package. - `mcollina/close-with-grace` is the real repo; Fastify core registers no signal handlers. - CI does run `skills/node/rules/assets/graceful-server.test.ts` (`.forgejo/workflows/node-test.yml`), so the graceful-shutdown wording is accurate. - No dangling links remain to the removed sections anywhere in the repo. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6ImQxMjNjZDM5OWRkMDBiZGY2ZDQ2MzFkZGIwZDJhYTdjYmY5OWI3OGIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3ODM5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImQ4ZDk4Y2U3LWJiNTEtNGIwYS1iYzVkLTY2MGNmY2Y2NTQ4OCJ9 -->
@ -151,3 +128,3 @@
```typescript
// Invalidate single entry
await cache.invalidate('getUser', '123');
await cache.clear('getUser', '123');

🟡 Medium: The clear fix 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 with storage: { type: 'memory' } and no options.invalidation, and reference invalidation is disabled by default. cache.invalidateAll('user:123') then logs invalidation is disabled, references are useless at warn level and returns without evicting anything.

Verified against async-cache-dedupe@3.4.0 — with storage: { type: 'memory' } the underlying function is called once across get → invalidateAll → get; with storage: { type: 'memory', options: { invalidation: true } } it is called twice.

Fix: storage: { type: 'memory', options: { invalidation: true } } in that example (Redis takes options: { 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 — so clear is the correct call for evicting one argument's entry and this line shouldn't be "corrected" back later.

🟡 **Medium:** The `clear` fix 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 with `storage: { type: 'memory' }` and no `options.invalidation`, and reference invalidation is disabled by default. `cache.invalidateAll('user:123')` then logs `invalidation is disabled, references are useless` at warn level and returns without evicting anything. Verified against `async-cache-dedupe@3.4.0` — with `storage: { type: 'memory' }` the underlying function is called once across `get → invalidateAll → get`; with `storage: { type: 'memory', options: { invalidation: true } }` it is called twice. Fix: `storage: { type: 'memory', options: { invalidation: true } }` in that example (Redis takes `options: { 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 — so `clear` is the correct call for evicting one argument's entry and this line shouldn't be "corrected" back later.
Author
Owner

Confirmed by probe against async-cache-dedupe@3.4.0, matching yours exactly: with storage: { type: 'memory' } the underlying function runs once across get → invalidateAll → get; with options: { invalidation: true } it runs twice. Fixed in 1704bcf — the example now enables invalidation, with a one-line comment pinning the default-off gotcha, since a silently inert invalidateAll is 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, so clear('getUser', '123') remains the correct single-entry eviction and won't be "corrected" back.

Confirmed by probe against `async-cache-dedupe@3.4.0`, matching yours exactly: with `storage: { type: 'memory' }` the underlying function runs once across `get → invalidateAll → get`; with `options: { invalidation: true }` it runs twice. Fixed in 1704bcf — the example now enables invalidation, with a one-line comment pinning the default-off gotcha, since a silently inert `invalidateAll` is 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, so `clear('getUser', '123')` remains the correct single-entry eviction and won't be "corrected" back.
jercik marked this conversation as resolved
@ -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 is pnpx @platformatic/flame run app.js — nothing has installed flame on PATH, so a reader following this sequence hits command not found. Suggest pnpx @platformatic/flame generate <profile.pb> for consistency with the invocation style used in the rest of the file.

🟢 **Low:** `flame generate <profile.pb>` invokes a bare binary, but the command directly above it is `pnpx @platformatic/flame run app.js` — nothing has installed `flame` on `PATH`, so a reader following this sequence hits `command not found`. Suggest `pnpx @platformatic/flame generate <profile.pb>` for consistency with the invocation style used in the rest of the file.
Author
Owner

Fixed in 1704bcfpnpx @platformatic/flame generate <profile.pb>, matching the invocation style used everywhere else in the file.

Fixed in 1704bcf — `pnpx @platformatic/flame generate <profile.pb>`, matching the invocation style used everywhere else in the file.
jercik marked this conversation as resolved
fix(node): enable invalidation in reference example, qualify flame generate
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 17s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m31s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 37s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 4m48s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 5m9s
1704bcf371
Author
Owner

Review feedback for head d123cd39 processed in 1704bcf:

  • 🟡 Medium (Reference-Based Invalidation example inert without options: { invalidation: true }): fixed in 1704bcf — reproduced the reviewer's probe against async-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.
  • 🟢 Low (bare flame generate vs the file's pnpx invocation style): fixed in 1704bcf — now pnpx @platformatic/flame generate <profile.pb>.
Review feedback for head d123cd39 processed in 1704bcf: - 🟡 Medium (Reference-Based Invalidation example inert without `options: { invalidation: true }`): **fixed** in 1704bcf — reproduced the reviewer's probe against `async-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. - 🟢 Low (bare `flame generate` vs the file's `pnpx` invocation style): **fixed** in 1704bcf — now `pnpx @platformatic/flame generate <profile.pb>`.
forgejo-actions left a comment

Summary: No actionable issues found.

Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)

**Summary:** No actionable issues found. _Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6IjE3MDRiY2YzNzFjZWVkZjNhZjJlYTBjYzljMmFiZjRmODZmMTA1Y2IiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3ODY5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImUzNjg5NGZiLTU1MGMtNDY4MS05NmRhLTY5YzljYjNmMDI0MyJ9 -->
fix(node): restore mnemonist section, document real invalidate semantics
Some checks failed
Node tests / node:test (pull_request) Successful in 8s
commit-msg / commitlint (pull_request) Successful in 18s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m49s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 51s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 5m24s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 5m45s
0b2f80ecf7
The mnemonist memoization section was dropped during review as a third
overlapping package, but its content was verified correct (import,
constructor, LRU semantics, strict typecheck) — the only defect was the
prose spelling, fixed here. A quick-guide bullet now states when to pick
it over lru-cache, answering the redundancy objection.

cache.invalidate(name, references) is a real async-cache-dedupe API the
earlier review cycle declared nonexistent. Probes against the shipped
3.4.0 source show it evicts by reference through the named function's
storage — global with the default shared storage, function-scoped only
under a per-define storage — so the section documents exactly that
instead of the function-scoping its signature suggests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Context for the 0b2f80e review 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 shipped node_modules source and live execution):

  • mnemonist section restored. The cycle-2 removal rationale was "third overlapping package with no selection criterion" — a redundancy call, not an API finding. The section's content verifies clean: the named ESM export exists in the shipped tarball, the example's LRU semantics reproduce live, and the snippet type-checks verbatim under strict TS 7. The new quick-guide bullet supplies the missing selection criterion (fixed-capacity LRU, no TTL/disposal, leaner than 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), so clear('getUser', '123') stays as the single-entry eviction. The new paragraph documents what invalidate actually does, probe-verified both ways: with the default shared storage it evicts matching references across every defined function (identical to invalidateAll); it narrows to one function only under a per-define storage.

Both additions were verified today against the latest published packages; please review the wording, not the APIs' existence.

Context for the 0b2f80e review 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 shipped `node_modules` source and live execution): - **mnemonist section restored.** The cycle-2 removal rationale was "third overlapping package with no selection criterion" — a redundancy call, not an API finding. The section's content verifies clean: the named ESM export exists in the shipped tarball, the example's LRU semantics reproduce live, and the snippet type-checks verbatim under strict TS 7. The new quick-guide bullet supplies the missing selection criterion (fixed-capacity LRU, no TTL/disposal, leaner than `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), so `clear('getUser', '123')` stays as the single-entry eviction. The new paragraph documents what `invalidate` actually does, probe-verified both ways: with the default shared storage it evicts matching references across every defined function (identical to `invalidateAll`); it narrows to one function only under a per-define `storage`. Both additions were verified today against the latest published packages; please review the wording, not the APIs' existence.
forgejo-actions left a comment

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 --test on 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). invalidateAll is a silent no-op without storage.options.invalidation: true (1 call vs 2 with it enabled). And invalidate(name, references) does evict across every defined function on the shared _default storage — a probe with getUser/getUserPosts sharing user:1 showed invalidate('getUser', 'user:1') evicting both. The new paragraph at caching.md:184 is accurate, including the per-define storage narrowing.
  • @platformatic/flame v1.7.0's CLI confirms run <script>, generate <pprof-file>, and --md-format with summary|detailed|adaptive; preload.js writes cpu-profile-<ts>.pb alongside the .md and HTML. The old --output markdown flag and profile() export do not exist.
  • mnemonist 0.40.4 exports LRUCache and LRUCacheWithDelete from the root, LRUCache.prototype.has exists, and the typings carry no TTL or disposal options — the new selection-guide bullet is right, and mnemoist is indeed not a package.
  • Signal listeners are unref'd in Node, so the Fastify example's lack of 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 Comparison all grep clean), and section numbering in flaky-tests.md and stuck-processes-and-tests.md stays contiguous after the deletions.

Code review by Claude Code Opus (opus)

**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 --test` on 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). `invalidateAll` is a silent no-op without `storage.options.invalidation: true` (1 call vs 2 with it enabled). And `invalidate(name, references)` does evict across every defined function on the shared `_default` storage — a probe with `getUser`/`getUserPosts` sharing `user:1` showed `invalidate('getUser', 'user:1')` evicting both. The new paragraph at `caching.md:184` is accurate, including the per-`define` `storage` narrowing. - `@platformatic/flame` v1.7.0's CLI confirms `run <script>`, `generate <pprof-file>`, and `--md-format` with `summary|detailed|adaptive`; `preload.js` writes `cpu-profile-<ts>.pb` alongside the `.md` and HTML. The old `--output markdown` flag and `profile()` export do not exist. - `mnemonist` 0.40.4 exports `LRUCache` and `LRUCacheWithDelete` from the root, `LRUCache.prototype.has` exists, and the typings carry no TTL or disposal options — the new selection-guide bullet is right, and `mnemoist` is indeed not a package. - Signal listeners are unref'd in Node, so the Fastify example's lack of `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 Comparison` all grep clean), and section numbering in `flaky-tests.md` and `stuck-processes-and-tests.md` stays contiguous after the deletions. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6IjBiMmY4MGVjZjcxOGQ5ZmRiOGUyNWZlMjAyOThmNWRkMjk4Y2NiMmYiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3OTM0Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImZmMTY4ZGRiLTFmM2EtNDU0Yy05YzNmLTM4NjQxYjI4ZjkyMyJ9 -->
@ -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.ts never imports graceful-server.ts. It declares its own isShuttingDown and a copy-pasted createHandler() (lines 7–29 of the test), so it pins a duplicate of the handler, not the runnable example. graceful-server.ts could regress its 503 or Connection: close behavior and .forgejo/workflows/node-test.yml would still be green.

Either import the real thing — export createHandler from graceful-server.ts and 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.

🟢 **Low:** This rewrite keeps a claim the asset does not support: `graceful-server.test.ts` never imports `graceful-server.ts`. It declares its own `isShuttingDown` and a copy-pasted `createHandler()` (lines 7–29 of the test), so it pins a duplicate of the handler, not the runnable example. `graceful-server.ts` could regress its 503 or `Connection: close` behavior and `.forgejo/workflows/node-test.yml` would still be green. Either import the real thing — export `createHandler` from `graceful-server.ts` and 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.
jercik marked this conversation as resolved
fix(node): stop claiming the shutdown test pins the asset's behavior
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 11s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m33s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 55s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 10m25s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 10m46s
80220ef086
The test exercises its own dependency-free copy of the handler —
importing the asset would pull close-with-grace into a package-less
repo and turn CI red — so "pins that behavior" overclaimed what a
green run proves. Say what the test does: demonstrates the same
behavior against its copy, run by CI.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ -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.
Author
Owner

Fixed in 80220ef with your second remedy. This was a regression of an earlier approved fix: the original overclaim was removed in ab390a3, 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 and node-test.yml runs the test as a direct entrypoint, so importing the asset makes close-with-grace unresolvable 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.

Fixed in 80220ef with your second remedy. This was a regression of an earlier approved fix: the original overclaim was removed in ab390a3, 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 and `node-test.yml` runs the test as a direct entrypoint, so importing the asset makes `close-with-grace` unresolvable 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.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 1 low issue. Every factual correction in this PR verified against the real tools.

Probes run against the published artifacts:

  • @platformatic/flame v1.7.0flame --help confirms run <script> and generate <pprof-file> subcommands and --md-format (summary/detailed/adaptive), accepted by both subcommands. Actually running flame run app.js and flame run server.js + SIGINT both wrote cpu-profile-<ts>.pb, .html, and a non-empty .md hotspot report, so the replacement text on profiling.md:15-20 is accurate. The removed --output markdown flag and profile() export do not exist in the package.
  • async-cache-dedupe v3.4.0 — runtime probe confirms cache.clear('getUser', '123') evicts the single entry, that invalidateAll is a no-op without storage: { type: 'memory', options: { invalidation: true } } (memory storage returns early, logging through the default no-op logger), and source confirms invalidate(name, references) dispatches to the named wrapper's storage — which is the shared _default instance unless define supplies its own. caching.md:150, 164-165, and 184 are all correct.
  • mnemonist v0.40.4 — package exists (mnemoist is indeed a 404); LRUCache is a named export, has() works, delete is undefined on LRUCache, and LRUCacheWithDelete exists with delete. caching.md:13 and 17-19 check out.
  • close-with-grace v2.5.0 — npm repository.url is github.com/mcollina/close-with-grace; the old fastify/ path is wrong.
  • fastify v5.11.2 — no process.on('SIG…') anywhere in fastify.js or lib/, so the previous "built-in close-with-grace support" claim was false and the new app.close() wiring is the right fix.
  • node:test on Node 26.5 — with two order-dependent top-level tests, node --test file passes and node --test --test-name-pattern="test 2: find user" file fails, so the rewritten symptom on flaky-tests.md:203 is accurate. There is no --parallel flag and no { retry } test option.
  • CI.forgejo/workflows/node-test.yml does select and run skills/node/rules/assets/graceful-server.test.ts, and that test defines its own handler rather than importing the asset, so graceful-shutdown.md:213 describes it correctly.

No dangling links to the deleted sections remain anywhere under skills/.

Code review by Claude Code Opus (opus)

**Summary:** Found 1 low issue. Every factual correction in this PR verified against the real tools. Probes run against the published artifacts: - **`@platformatic/flame` v1.7.0** — `flame --help` confirms `run <script>` and `generate <pprof-file>` subcommands and `--md-format` (`summary`/`detailed`/`adaptive`), accepted by both subcommands. Actually running `flame run app.js` and `flame run server.js` + SIGINT both wrote `cpu-profile-<ts>.pb`, `.html`, and a non-empty `.md` hotspot report, so the replacement text on `profiling.md:15-20` is accurate. The removed `--output markdown` flag and `profile()` export do not exist in the package. - **`async-cache-dedupe` v3.4.0** — runtime probe confirms `cache.clear('getUser', '123')` evicts the single entry, that `invalidateAll` is a no-op without `storage: { type: 'memory', options: { invalidation: true } }` (memory storage returns early, logging through the default no-op logger), and source confirms `invalidate(name, references)` dispatches to the named wrapper's storage — which is the shared `_default` instance unless `define` supplies its own. `caching.md:150`, `164-165`, and `184` are all correct. - **`mnemonist` v0.40.4** — package exists (`mnemoist` is indeed a 404); `LRUCache` is a named export, `has()` works, `delete` is `undefined` on `LRUCache`, and `LRUCacheWithDelete` exists with `delete`. `caching.md:13` and `17-19` check out. - **`close-with-grace` v2.5.0** — npm `repository.url` is `github.com/mcollina/close-with-grace`; the old `fastify/` path is wrong. - **`fastify` v5.11.2** — no `process.on('SIG…')` anywhere in `fastify.js` or `lib/`, so the previous "built-in close-with-grace support" claim was false and the new `app.close()` wiring is the right fix. - **node:test on Node 26.5** — with two order-dependent top-level tests, `node --test file` passes and `node --test --test-name-pattern="test 2: find user" file` fails, so the rewritten symptom on `flaky-tests.md:203` is accurate. There is no `--parallel` flag and no `{ retry }` test option. - **CI** — `.forgejo/workflows/node-test.yml` does select and run `skills/node/rules/assets/graceful-server.test.ts`, and that test defines its own handler rather than importing the asset, so `graceful-shutdown.md:213` describes it correctly. No dangling links to the deleted sections remain anywhere under `skills/`. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjQ2IiwiaGVhZFNoYSI6IjgwMjIwZWYwODZjNDQ0MTI3YzYwZDE5ZGZjNmJkNWU2NTJmYzVmNmIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3OTUxIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjMyYzZkYmMyLTAwNDQtNDcyMi05NTI0LTFkMGFmZGRkYWQzMyJ9 -->
@ -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.debug and logger.warn and 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 demonstrates info and error only, 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, --help transcriptions, 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. debug for per-step tracing, info for operational events, warn for handled anomalies, error for conditions needing attention.

🟢 **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.debug` and `logger.warn` and 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 demonstrates `info` and `error` only, 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, `--help` transcriptions, 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. `debug` for per-step tracing, `info` for operational events, `warn` for handled anomalies, `error` for conditions needing attention.
jercik marked this conversation as resolved
@ -40,4 +25,1 @@
logger.error({ err, orderId: order.id }, 'Failed to process payment');
```
## Transports
Author
Owner

Declining 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.

Declining 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.
jercik marked this conversation as resolved
jercik merged commit 6c817eeea3 into main 2026-08-07 07:53:00 +00:00
jercik deleted branch align/node 2026-08-07 07:53:00 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/agent-skills!46
No description provided.