Skip to content

feat(engineering): compile OpenAI's Spinning Up in Deep RL into a knowledge-base plugin - #993

Merged
alirezarezvani merged 7 commits into
devfrom
claude/spinning-up-book-skill-hhbjpy
Aug 26, 2026
Merged

feat(engineering): compile OpenAI's Spinning Up in Deep RL into a knowledge-base plugin#993
alirezarezvani merged 7 commits into
devfrom
claude/spinning-up-book-skill-hhbjpy

Conversation

@alirezarezvani

@alirezarezvani alirezarezvani commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Summary

Runs engineering/book-to-skill end to end on its first real source: OpenAI's Spinning Up in Deep RL (MIT, © 2018 OpenAI; primarily developed by Joshua Achiam). New engineering/spinning-up-deep-rl/ plugin — 20 chapters, a glossary, a patterns file and a decision cheatsheet behind a 2,101-token resident core.

The full pipeline, not a hand-write. Cloned openai/spinningup and compiled its docs/ reStructuredText tree (38 files, ~37k words, ~49K tokens) through every step: extract_document.py --mode technical → analysis → 20 chapter files → supporting files → master SKILL.mdbook_skill_validator.pyskill_plugin_emitter.py. The validator passes clean in --strict mode and every file is inside budget (core 2,101 / 4,000 tokens; 20 chapters averaging ~1,256 tokens, loaded on demand).

Structure follows the source's own toctree, not a heading scan: user documentation (ch01–06), Introduction to RL Parts 1–3 (ch07–09), the researcher essay / key papers / exercises / benchmarks (ch10–13), one chapter per algorithm in lineage order (ch14–19: VPG → TRPO → PPO, DDPG → TD3 → SAC), and the logger / MPI / ExperimentGrid utilities (ch20).

Rights basis is open-license, not fair use. The emitter's Step-11 gate refuses a shareable package without one; MIT permits derivative distribution. Upstream's notice is reproduced in full in the plugin's LICENSE beside this package's own, and README.md names the source, the author and the source's frozen version — a sidecar JSON is not a license notice. authoring-notes.json carries both a source block (how it was built) and an attribution block (whose work it came from).

The cheatsheet carries the judgment a glossary cannot — the under-5-minute debug-turnaround target, the 3-seed minimum (10+ to be thorough), benchmark network defaults that differ by algorithm family ((64,32)/tanh on-policy vs (256,256)/relu off-policy), and Spinning Up's own parity disclosure: DDPG/TD3/SAC are research-grade, VPG/TRPO/PPO are not and the docs say to use OpenAI Baselines for those.

Also fixed: the emitter produced manifests this repo's CI rejects

skill_plugin_emitter.py wrote its whole source provenance block into plugin.json, on a stale inline claim that source/attribution were "approved extension fields." Claude Code rejects an entire manifest on any unrecognized key (#954) and scripts/check_plugin_json.py hard-fails such a manifest, pointing at .claude-plugin/authoring-notes.json instead — so every package the emitter produced failed the blocking CI gate the moment it was committed. A defect at the very last step of the pipeline, which is why it had gone unnoticed. _plugin_manifest() now emits spec fields only; a new _authoring_notes() writes the sidecar. Recorded as deviation 26 in engineering/book-to-skill/README.md. The printed marketplace.json snippet is unchanged — source is a valid key there, which is how it leaked into the manifest originally.

Step 11 of conversion_workflow.md now also states that attribution is added by hand whenever --rights is anything but internal-docs. The emitter knows only --source-note free text and a rights basis, not an upstream URL/author/licence, and a half-filled attribution block would look authoritative while being wrong.

Counters

This branch sits on top of deep-learning-book (#994), which merged into dev first. Derived totals on the merged tree: 388 skills · 99 plugins · 727 tools · 842 references · 118 agents · 150 commands.

This plugin's own delta against dev's baseline: skills 387 → 388, agents 117 → 118, commands 149 → 150, plugins 98 → 99. Tools and references unchanged, because a compiled knowledge base ships notes, not scripts.

All five counter surfaces (README.md, CLAUDE.md, marketplace.json, mkdocs.yml, .codex-plugin/plugin.json) were re-derived from the tree rather than hand-picked from either side of the merge.

Checklist

  • Target branch is dev (not main)
  • Skill has SKILL.md with valid YAML frontmatter (name, description, license) — name + description only, deliberately. book-to-skill hard rule 5 is that generated frontmatter carries those two keys and nothing else, so a compiled skill never widens its own authority; book_skill_validator.py errors on allowed-tools and on disable-model-invocation: false. The package's plugin.json and LICENSE carry the licensing.
  • Scripts (if any) run with --help without errors — no new scripts; the edited emitter passes --help and --sample end to end
  • No hardcoded API keys, tokens, or secrets
  • No vendor-locked dependencies without open-source fallback — stdlib only, no new dependencies
  • Follows existing directory structure

Type of Change

  • New skill
  • Bug fix (the plugin.json provenance defect in book-to-skill's emitter)
  • Documentation (CHANGELOG, CLAUDE.md, README, book-to-skill deviations list)

Testing

Every blocking CI gate re-run locally on the current head (8b0fca4), all green:

Gate Result
No conflict markers left in the tree clean
compileall (all 17 domains + scripts) OK
check_plugin_json.py --all 0 failures
check_skill_names.py --all no shadowed built-ins
check_paths.py --all (G1) 658 files, 0 unresolvable refs
check_frontmatter.py --all (G10) 669 files, 0 errors
check_dual_publish.py (G4) 12 pairs, 0 drifted
check_model_freshness.py --all (G7) 3078 files, 0 retired identifiers
smoke_scripts.py (G8) 696/696 passed, 0 failed
derive_counters.py --check (G3) all five surfaces match derived values
JSON/YAML parse of every touched file OK

Skill-specific:

  • book_skill_validator.py <skill> --strict → passed all checks (frontmatter, safety, budget, index families)
  • token_budget_estimator.py --skill-dir → every file under cap; core 2,101/4,000
  • skill_plugin_emitter.py --sample → emits both plugin.json and authoring-notes.json; the sample manifest passes check_plugin_json.py

write-a-skill checklist: 4/6. Item 5 (concrete examples) was a real gap and is fixed — the resident core now carries a worked invocation block. The two remaining are structural to a compiled knowledge base rather than defects: item 2 ("under 100 lines") conflicts with the format's mandatory 20-row Chapter Index plus Topic Index, where the binding budget is the validator's 4,000-token cap and trimming to 100 lines would delete exactly the navigation budget.over_cap exists to protect; item 3 ("no time-sensitive info") fires on "January 2020", which is a provenance pin on a frozen source, not a staleness claim — removing it would make the skill less honest about its coverage.

Merge history

This branch was originally cut from main while dev trailed it, which made the diff carry unrelated already-merged commits. dev has since caught up and been merged in again to resolve a conflict from #994 (deep-learning-book), which collided on every counter and registry surface. The diff is now clean — 40 files, down from 136 — and reflects only this change.

Conflicts were resolved keeping both sides' content: both plugin entries in marketplace.json, both Unreleased sections in CHANGELOG.md, and dev's prose in CLAUDE.md/README.md with this branch's engineering-row entry restored on top.

Review fixes

Four findings across three review rounds, each verified against the tree before acting:

# Finding Fix
1 authoring-notes.json carried only a source block, though the content derives from an external MIT work Added an attribution block (40fa752). The emitter was deliberately not taught to synthesise one — it knows no upstream URL/author/licence, and a half-filled block would look authoritative while being wrong; Step 11 now documents hand-adding it
2 Commands delta stated 147 → 150, implying three new commands One command is added. 147 was the pre-merge baseline; the line was half-rebased. Verified via derive_counters on an origin/dev worktree (149) and the raw file count (281 → 282). Corrected to 149 → 150 (3301348)
3 Stray space before a comma in the README POWERFUL row Fixed (3301348)
4 skill_plugin_emitter.py's docstring still described the behaviour this PR removed; CLAUDE.md said the core is 2,066 tokens Docstring corrected, plus a third instance the review didn't name — the tree diagram never listed authoring-notes.json, the file this fix creates. 2,066 predated the added invocation block; CLAUDE.md contradicted itself twelve lines apart (8b0fca4)

The pattern is the useful takeaway: every finding was prose drifting from a verified tree — a half-rebased delta, a stale docstring, a pre-fix measurement. None were caught by CI, because every gate here reads the tree and none read the prose. That's a real blind spot in an otherwise strict setup; widening it is out of scope for this PR.

alirezarezvani and others added 3 commits August 25, 2026 18:49
…wledge-base plugin

Runs engineering/book-to-skill end to end on its first real source: OpenAI's
Spinning Up in Deep RL (MIT, (c) 2018 OpenAI; primarily developed by Joshua
Achiam). Cloned openai/spinningup and compiled its docs/ reStructuredText tree
(38 files, ~37k words, ~49K tokens) through the full pipeline -- extract
--mode technical, analysis, 20 chapter files, glossary/patterns/cheatsheet,
master SKILL.md, validator, plugin emitter.

The compiled skill passes book_skill_validator.py in --strict mode with every
file inside budget: a 2,101-token resident core (cap 4,000) plus 20 on-demand
chapters averaging ~1,256 tokens each.

Chapter structure follows the source's own toctree rather than a heading scan:
user documentation (ch01-06), Introduction to RL Parts 1-3 (ch07-09), the
researcher essay / key papers / exercises / benchmarks (ch10-13), one chapter
per algorithm in lineage order (ch14-19: VPG to TRPO to PPO, DDPG to TD3 and
SAC), and the logger/MPI/ExperimentGrid utilities (ch20).

Rights basis is open-license, not fair use -- the emitter's Step-11 gate
refuses a shareable package without one. Upstream's MIT notice is reproduced
in full in the plugin's LICENSE beside this package's own, and README.md names
the source, the author and the source's frozen version; a sidecar JSON is not
a license notice.

Also fixes a defect the emitter only reveals at its final step:
skill_plugin_emitter.py wrote its whole `source` provenance block into
plugin.json, on a stale inline claim that `source`/`attribution` were approved
extension fields. Claude Code rejects an entire manifest on any unrecognized
key (issue #954) and scripts/check_plugin_json.py hard-fails such a manifest,
so every package the emitter produced failed the blocking CI gate on commit.
_plugin_manifest() now emits spec fields only and a new _authoring_notes()
writes .claude-plugin/authoring-notes.json. Recorded as deviation 26 in
engineering/book-to-skill/README.md; the printed marketplace.json snippet is
unchanged, since `source` is a valid key there.

Counters: skills 386 -> 387, agents 116 -> 117, commands 146 -> 147, plugins
97 -> 98. Tools and references unchanged -- a compiled knowledge base ships
notes, not scripts.

All blocking CI gates verified locally: compileall, check_plugin_json --all,
check_skill_names, check_paths, check_frontmatter, check_dual_publish,
check_model_freshness, smoke_scripts (692/692), derive_counters --check.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw
dev's derive_counters.py gained two additional check targets that main's copy
did not have -- mkdocs.yml's site_description and .codex-plugin/plugin.json's
description / shortDescription / longDescription. Both carried the pre-linkedin,
pre-skill-doctor numbers (380 skills / 706 tools / 823 refs / 114 agents /
138 commands / 96 plugins), so the blocking gate fails on this branch the moment
dev's checker meets main's tree.

Synced all six to the derived values: 387 skills, 723 tools, 838 references,
117 agents, 147 commands, 98 plugins.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review

Reviewed the PR-specific content (the rest of the diff is inherited dev-merge noise from already-merged PRs, per the "Note on the diff" section — marketing/linkedin/*, .codex/, .gemini/, .hermes/, .vibe/ mirrors).

engineering/book-to-skill emitter fix (skill_plugin_emitter.py)

Solid, well-targeted fix. Confirmed:

  • _plugin_manifest() now emits spec-only fields; the new _authoring_notes() writes source to the sidecar .claude-plugin/authoring-notes.json.
  • This matches scripts/check_plugin_json.py's RELOCATED_TO_NOTES/NOTES_ALLOWED = {"source", "attribution"} gate exactly — the sidecar produced by the emitter is schema-valid against the checker that would otherwise hard-fail it.
  • Deviation 26 in engineering/book-to-skill/README.md documents the defect and fix accurately, and correctly notes source is still a legitimate key in marketplace.json's plugins[] entries (so that printed snippet didn't need to change).
  • Downstream call sites (_readme_markdown, _agent_markdown, _command_markdown) still take source_note/rights/distribution directly rather than reading them back off the manifest, so nothing else needed to change — no stray references to the old manifest["source"] shape remain.

Nice catch overall — this was a real "every package this tool ever emitted fails CI" bug, and the fix is minimal and correctly scoped.

engineering/spinning-up-deep-rl (new plugin)

  • SKILL.md frontmatter correctly ships only name + description (book-to-skill hard rule 5), spot-checked against plugin.json (spec-only keys) and authoring-notes.json (source-only, matches NOTES_ALLOWED).
  • Spot-checked ch07–ch09 and the core-frameworks section against my own knowledge of the source material (Bellman equations, the policy-gradient derivation, log-derivative trick, EGLP lemma, reward-to-go, VPG→TRPO→PPO / DDPG→TD3→SAC framing) — accurate to the actual Spinning Up docs, no fabricated claims noticed, and the "not a reproduction, structured notes" framing plus per-chapter citations in the agent/command are a good discipline for a compiled-knowledge-base skill (reduces hallucination risk relative to freeform Q&A).
  • LICENSE correctly two-layers the terms (package scaffolding MIT vs. reproduced-in-full upstream OpenAI MIT notice) — appropriate for the open-license rights basis.
  • agents/cs-spinning-up-deep-rl.md's skills: path (engineering/spinning-up-deep-rl/skills/spinning-up-deep-rl) matches the established convention used by every other single-skill plugin in engineering/ (book-to-skill, caveman, grill-me, handoff, etc.) — consistent, no drift.
  • Counters check out: this PR's own delta (skills +1, agents +1, commands +1, plugins +1, tools/refs unchanged) is correctly reflected in marketplace.json, CHANGELOG.md, and the CLAUDE.md unreleased-section blurb.

Minor/non-blocking observation

authoring-notes.json for the compiled skill uses the source key (Path-B/generated-provenance shape) rather than attribution (external-MIT-derivation shape), even though the compiled content is itself derived from an external MIT-licensed work (similar in spirit to how book-to-skill describes itself using attribution). This is inherited behavior from the emitter predating this PR — this PR only relocated the block, it didn't introduce or reconsider the choice — so not a blocker, just worth a thought for a future pass over book-to-skill's classification of its own generated output.

Everything else

No scripts changed besides the emitter (no new Python tools — expected, since this is "notes not scripts"), no secrets/hardcoded credentials, no security-relevant surface added (pure markdown + one manifest-generation code path). Given this is a documentation/knowledge-base PR with no test framework in the repo (per CLAUDE.md's "no build system/test frameworks" design choice), the PR body's listed local gate runs (check_plugin_json.py, check_paths.py, check_frontmatter.py, smoke_scripts.py, derive_counters.py --check, etc.) are the appropriate verification surface, and the diffs are consistent with those claims.

Looks good to merge from a correctness/quality standpoint.

…decar

Addresses the review observation on #993: the compiled skill's
authoring-notes.json carried only a `source` block (how it was built) even
though its content is derived from an external MIT-licensed work, where the
rest of the repo uses an `attribution` block for that. check_plugin_json.py's
NOTES_ALLOWED permits both keys, so the two coexist.

Adds `attribution` to engineering/spinning-up-deep-rl following the shape used
by book-to-skill and skillopt-sleep: derived_from, upstream_docs, upstream_path,
original_author, original_license, original_copyright, derivation_note.

The emitter is deliberately NOT changed to synthesise this. It knows only
`--source-note` free text and a rights basis -- not an upstream URL, author or
licence -- and a half-filled attribution block is worse than none. Instead
Step 11 of conversion_workflow.md now says attribution is added by hand
whenever `--rights` is anything but internal-docs, names the field shape, and
restates that the actual obligation is the LICENSE notice and README credit --
authoring-notes.json is metadata Claude Code never reads, and a sidecar JSON
file is not a licence notice.

Gates re-run clean: check_plugin_json --all, check_paths, check_frontmatter,
check_dual_publish, check_model_freshness, smoke_scripts (692/692),
derive_counters --check, book_skill_validator --strict.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw

Copy link
Copy Markdown
Owner Author

Thanks — I verified the authoring-notes.json observation against check_plugin_json.py and it's correct, so I've addressed it in 40fa752 rather than deferring it.

NOTES_ALLOWED = {"source", "attribution"} permits both keys, so they now coexist in engineering/spinning-up-deep-rl/.claude-plugin/authoring-notes.json: source keeps saying how the skill was built, and a new attribution block says whose work it came fromderived_from, upstream_docs, upstream_path, original_author, original_license, original_copyright, derivation_note — matching the shape book-to-skill and skillopt-sleep already use.

I deliberately did not change the emitter to synthesise this, which I think is the more interesting half of your point. skill_plugin_emitter.py knows only --source-note (free text) and a rights basis; it has no upstream URL, author, or licence, and a half-filled attribution block would be worse than an absent one — it would look authoritative while being wrong. So the fix is procedural instead: Step 11 of references/conversion_workflow.md now states that attribution is added by hand whenever --rights is anything but internal-docs, names the field shape, and points at this package as the worked example.

That section also now restates the thing worth not losing: the actual licence obligation is met by the notice in LICENSE and the credit in README.md. authoring-notes.json is authoring metadata Claude Code never reads — a sidecar JSON file is not a licence notice. Both were already in place here; the sidecar block is discoverability, not compliance.

Gates re-run clean on the new head: check_plugin_json --all, check_paths, check_frontmatter, check_dual_publish, check_model_freshness, smoke_scripts (692/692), derive_counters --check, and book_skill_validator --strict on the compiled skill.


Generated by Claude Code

dev moved: PR #994 landed engineering/deep-learning-book, which collides with
this branch on every headline-counter and registry surface.

Conflicts resolved in four files, keeping both sides' content:

- .claude-plugin/marketplace.json -- both plugin entries kept; the registry now
  carries spinning-up-deep-rl and deep-learning-book. 99 plugins.
- CHANGELOG.md -- both Unreleased sections kept.
- CLAUDE.md, README.md -- dev's prose taken as the newer baseline, then this
  branch's engineering-row entry restored and every counter re-derived rather
  than hand-picked from either side.

Counters re-derived from the merged tree with derive_counters.py, which is the
ground truth, and trued up across all five surfaces: 388 skills, 99 plugins,
727 tools, 842 references, 118 agents, 150 commands.

Both changelog/CLAUDE.md delta lines are restated: each side was written against
its own base and both claimed 386 -> 387, which is no longer true of either now
that they land together. This branch's entry is now stated as the delta on top of
deep-learning-book.

Gates re-run on the resolved merge: no conflict markers left in the tree,
compileall, check_plugin_json --all, check_skill_names, check_paths,
check_frontmatter, check_dual_publish, check_model_freshness, smoke_scripts
(696/696), derive_counters --check, book_skill_validator --strict, and a
JSON/YAML parse of every file touched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review

Reviewed the diff, the new engineering/spinning-up-deep-rl plugin content, and the book-to-skill emitter fix.

Overall

Strong PR. The compiled knowledge base is technically accurate and clearly traces back to the real Spinning Up docs — I spot-checked SKILL.md's Core Frameworks and ch19-sac.md against the actual source material (entropy-regularized Bellman equations, the reparameterization trick, the TD3-vs-SAC deltas, the "state-independent log-std did not work" note) and it's faithful, not hallucinated. The emitter bug fix (_plugin_manifest() → spec-only fields, new _authoring_notes() writing the sidecar) is a real, well-targeted fix: every prior book-to-skill output would have failed check_plugin_json.py's hard-fail on unrecognized keys, and this catches it at the one place it could go unnoticed (the last pipeline step, never re-run against CI until now). Good catch, and recording it as deviation 26 in the book-to-skill README with a source vs marketplace.json disambiguation note is the right way to keep the authoritative-list convention from drifting.

The new plugin.json for spinning-up-deep-rl is clean (spec fields only) and authoring-notes.json correctly separates the source and attribution blocks per the schema in root CLAUDE.md. Frontmatter is name+description only, matching book-to-skill's hard rule 5. Rights basis (open-license, MIT) is correctly justified and the upstream notice is reproduced in LICENSE.

Issue: counters inconsistency in the docs

The per-feature changelog counters don't match the root totals in the same diff:

  • CHANGELOG.md (new "Added" section) and CLAUDE.md's domain-list bullet both say: commands 147 → 150.
  • But .claude-plugin/marketplace.json's own metadata description (in this same diff) goes from "149 slash commands""150 slash commands" — a delta of +1, matching the single new commands/cs-spinning-up-deep-rl.md file this PR actually adds.
  • mkdocs.yml's site_description also confirms the true final total is 150.
  • Root CLAUDE.md's "Current Scope" line changes 149 → 150 as well (net +1 across skills/agents/commands/plugins, consistent with adding exactly one skill/agent/command/plugin).

So the actual delta this PR contributes is 149 → 150, not 147 → 150. 147 doesn't correspond to any baseline visible in the diff (the prior deep-learning-book entry ends at 149 commands, not 147). It reads like a copy/paste or stale-draft number that didn't get corrected before merge. Doesn't affect derive_counters.py --check (that's driven by the real tree, not this prose), but it's worth fixing since this repo is unusually strict about counters being trustworthy — two separate docs now assert a false "+3" jump for a change that only adds one command.

Minor

  • README.md's POWERFUL-tier row has a stray space before the comma: ...calculator) , **spinning-up-deep-rl**... (should be ...calculator), **spinning-up-deep-rl**...).

Not concerns

  • Rights/licensing handling is correct and conservative (open-license basis, MIT compatible, full upstream notice preserved, cleared_for_distribution semantics unaffected).
  • No scripts changed other than the emitter fix; no new dependencies; stdlib-only maintained.
  • write-a-skill checklist 4/6 with the two gaps (SKILL.md length, "no time-sensitive info") explained as structural to the compiled-knowledge-base format rather than defects — reasoning holds up (the "January 2020" mention is a provenance pin, not staleness).

Addresses both review findings on #993.

1. The changelog/CLAUDE.md counter line said "commands 147 → 150", which
   asserts this plugin adds three commands. It adds exactly one
   (commands/cs-spinning-up-deep-rl.md). 147 was this branch's pre-merge
   baseline; after merging dev the delta had to be restated against dev's
   baseline, and that one figure was carried over unchanged while the others
   were updated. Verified empirically rather than by arithmetic: derive_counters
   on an origin/dev worktree reports 149 commands, and the raw command-file
   count goes 281 → 282 across the merge. Corrected to 149 → 150. The other
   three figures on that line (skills 387 → 388, agents 117 → 118, plugins
   98 → 99) were already right.

2. README POWERFUL-tier row had a stray space before a comma:
   "calculator) , **spinning-up-deep-rl**". Removed.

Neither affected derive_counters --check, which reads the tree rather than the
prose -- which is exactly why a wrong delta in prose can survive a green gate,
and why it was worth fixing in a repo this strict about counters being
trustworthy.

Gates re-run: compileall, check_plugin_json --all, check_paths,
check_frontmatter, check_dual_publish, check_model_freshness, smoke_scripts
(696/696), derive_counters --check.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw

Copy link
Copy Markdown
Owner Author

Both findings verified and fixed in 3301348.

The commands delta was wrong, and your diagnosis of why was right. I checked it empirically rather than by arithmetic: derive_counters.py on an origin/dev worktree reports 149 commands, and the raw command-file count across the merge goes 281 → 282. This plugin adds exactly one command file. So 149 → 150, as you said.

The cause was the merge with #994. Before it, this branch's baseline was 147; after, the delta had to be restated against dev's. I updated skills, agents and plugins but carried 147 through unchanged — so the line ended up half-rebased, mixing a pre-merge baseline with a post-merge total. The other three figures were already correct.

Worth naming the general point: derive_counters --check reads the tree, not the prose, so a wrong delta in a changelog passes the gate every time. That is exactly the gap where a false counter can survive to merge, which is why this was worth fixing rather than waving through as cosmetic.

The README spacing nit is fixed too — calculator), **spinning-up-deep-rl**.

Gates re-run on the new head: compileall, check_plugin_json --all, check_paths, check_frontmatter, check_dual_publish, check_model_freshness, smoke_scripts (696/696), derive_counters --check across all five surfaces.


Generated by Claude Code

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review

This is a well-executed PR — the compiled spinning-up-deep-rl content (spot-checked SKILL.md, cheatsheet.md, glossary.md, ch19-sac.md) is accurate to the source, faithfully preserves Achiam's terminology/equations, and the "Connects To" / "Anti-patterns" / topic-index cross-linking is genuinely useful rather than boilerplate. The licensing/attribution split (LICENSE two-layer notice, authoring-notes.json sidecar, README provenance section) is careful and matches the repo's check_plugin_json.py constraints. The real bug fix in skill_plugin_emitter.py (moving source/attribution out of plugin.json into the sidecar, since CC/CI rejects unrecognized manifest keys) is a legitimate and valuable fix — nice catch, and good that it's applied retroactively via _authoring_notes() rather than just documented.

Two things worth cleaning up before/after merge:

1. Stale docstring in skill_plugin_emitter.py (correctness/doc-drift)

The module docstring wasn't updated to match this PR's own fix:

  • Line 13: ├── .claude-plugin/plugin.json manifest with ./skills/ + attribution — attribution no longer lives in the manifest; it's in the authoring-notes.json sidecar now. This line describes the exact behavior the PR just removed.
  • Lines 26-27: emits with a notice and records source.cleared_for_distribution: false in the manifest. — same issue; this is now recorded in the sidecar, not plugin.json.

Low severity (docstring only, doesn't affect behavior), but since this PR's whole point in that file is "the manifest must not carry source/attribution," having the docstring for the same file still assert the opposite is worth a quick follow-up fix so future readers of --help/the source don't get misled.

2. Token-count discrepancy between CHANGELOG.md and CLAUDE.md

The PR states the resident core is 2,101 tokens in the PR description and in CHANGELOG.md ("behind a 2,101-token resident core"), but the CLAUDE.md engineering-row entry says 2,066 / 4,000 tokens for the same file. Both numbers are added by this PR, describing the same SKILL.md. Worth reconciling to whichever token_budget_estimator.py actually reports at merge time (both are comfortably under the 4,000 cap either way, so this is a nit, not a blocker).

Other notes (no action needed):

  • The 40-file diff and multi-stage dev-merge history are explained clearly in the PR description; the merge conflict resolution (keeping both marketplace.json plugin entries, restating both CHANGELOG/CLAUDE.md delta lines) looks correctly handled based on the final diff.
  • write-a-skill checklist 4/6 rationale (structural, not defects, for a compiled knowledge base) is reasonable and consistent with how deep-learning-book was justified in the prior PR.
  • No scripts changed other than the one legitimate bug fix — no new dependencies, no security concerns, stdlib-only throughout.

Nice work overall — the emitter fix alone makes this a valuable PR independent of the new skill content.

…core figure

Addresses both review findings on #993.

1. skill_plugin_emitter.py's module docstring still described the behaviour this
   PR removed. Its tree diagram called plugin.json the "manifest with
   ./skills/<slug> + attribution", and the rights-gate paragraph said
   cleared_for_distribution is recorded "in the manifest" -- both now the
   sidecar. A docstring asserting the opposite of its own file's point is worse
   than no docstring, and it is what `--help` readers see.

   Also corrects a third instance the review did not name but which shares the
   root cause: the tree diagram never listed authoring-notes.json at all, even
   though this PR's fix is what creates it. Added, with the issue #954 reason
   stated inline so the next reader knows why provenance cannot live in the
   manifest.

2. CLAUDE.md said the resident core is 2,066 tokens while, twelve lines later,
   the same file said 2,101. 2,066 predates the worked invocation block added
   for write-a-skill checklist item 5; token_budget_estimator.py reports 2,101
   on the current tree, matching CHANGELOG.md and the PR body. CLAUDE.md was
   the only stale copy and it contradicted itself.

Docstring-only and prose-only; no behaviour change. Verified the emitter still
emits both plugin.json and authoring-notes.json via --sample after the edit.

Gates: compileall, check_plugin_json --all, check_paths, check_frontmatter,
check_dual_publish, check_model_freshness, smoke_scripts (696/696),
derive_counters --check, book_skill_validator --strict.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UySnyf5upm4y8xhYA3w6yw

Copy link
Copy Markdown
Owner Author

Both verified and fixed in 8b0fca4.

1. Docstring drift — correct, and there was a third instance. You named the tree-diagram line and the rights-gate paragraph; both now point at the sidecar. The diagram also never listed authoring-notes.json at all, even though this PR's fix is what creates it, so I added it with the issue #954 reason stated inline. Same root cause: I changed the code path and left the file's own prose describing the behaviour I removed. Agreed that a docstring asserting the opposite of its file's point is worse than none — and it's what --help readers see.

2. Token count — 2,101 is right, and CLAUDE.md was contradicting itself. token_budget_estimator.py reports SKILL.md 2,101 / 4,000 on the current tree. 2,066 predates the worked invocation block I added for write-a-skill checklist item 5, so it's a pre-fix measurement that survived. Worth adding to your note: CLAUDE.md carried both numbers — line 250 said 2,066 while line 274 said 2,101 about the same file. CHANGELOG.md and the PR body were already correct, so CLAUDE.md was the only stale copy and it disagreed with itself.

Both changes are docstring/prose only, no behaviour change. I re-ran --sample end to end afterwards to confirm the emitter still produces both plugin.json and authoring-notes.json.

Gates green on the new head: compileall, check_plugin_json --all, check_paths, check_frontmatter, check_dual_publish, check_model_freshness, smoke_scripts (696/696), derive_counters --check, book_skill_validator --strict.

Noting a pattern across the three review rounds, since it's the useful takeaway: every finding has been prose drifting from a verified tree — a half-rebased delta, a stale docstring, a pre-fix token count. None were caught by CI, because every gate here reads the tree and none read the prose. That's a real blind spot in an otherwise strict setup, though widening it is out of scope for this PR.


Generated by Claude Code

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review

Went through the emitter fix, the manifest/authoring-notes schema, and spot-checked the compiled spinning-up-deep-rl skill for structural consistency and content quality. Overall this is a well-scoped, well-documented change — the actual bug fix is small and correct, and the compiled content is high quality. A few notes below.

The core fix (skill_plugin_emitter.py)

  • _plugin_manifest() now emits spec-only fields and the new _authoring_notes() writes source/attribution to the sidecar — verified this matches scripts/check_plugin_json.py's ALLOWED/RELOCATED_TO_NOTES/NOTES_ALLOWED sets exactly, so the emitted engineering/spinning-up-deep-rl/.claude-plugin/plugin.json (8 required keys, no extras) passes the gate. Good catch that this was silently breaking every prior book-to-skill output.
  • No orphaned call sites for the old _plugin_manifest() signature; the --sample path was re-verified against the new function shape.
  • _authoring_notes()'s license_scope string was correctly updated to say "plugin.json's top-level license..." now that it's describing a field in a different file than the one it lives in — easy detail to get wrong and it wasn't missed.
  • Good defensive engineering already present and untouched by this PR: _assert_no_symlinks + the post-copy re-walk in emit() closing the TOCTOU window on a distributable package.

Compiled skill content (engineering/spinning-up-deep-rl/)

  • Chapter Index in SKILL.md lists 20 chapters; all 20 files exist under chapters/ with matching filenames — no dead links.
  • Spot-checked ch12-exercises.md and cheatsheet.md against my own knowledge of the actual Spinning Up docs — the content (the DDPG silent-squeeze bug, the VPG/TRPO/PPO vs. DDPG/TD3/SAC parity disclosure, the benchmark network defaults) is accurate and well-cited back to chapters, not just plausible-sounding.
  • plugin.json + authoring-notes.json + LICENSE (two-license-layer pattern) are internally consistent and match the sibling deep-learning-book plugin already in dev, so the pattern is now applied consistently across both book-to-skill outputs.

Process/documentation

  • The PR body's running log of merge conflicts and two follow-up fixes (wrong commands delta, README spacing nit) is a good practice — it makes the final diff auditable rather than just asserting correctness.
  • One very minor thing: CHANGELOG.md/CLAUDE.md prose counters are hand-maintained and only checked by derive_counters.py --check reading the tree — the PR itself calls this out as a known gap class (which is exactly how the commands-delta bug happened in the first place). Not a blocker, just worth the reminder that this class of drift will keep recurring until counters are asserted from a single generated source rather than copy-pasted across 5 files.

Not independently re-run

This sandboxed review environment blocks arbitrary script execution, so I wasn't able to re-run book_skill_validator.py --strict or derive_counters.py --check myself — I verified the relevant invariants by hand-reading the validator/gate source and the emitted files instead (see above). The PR's own gate-run table is thorough enough that I have no reason to doubt it, but flagging that I couldn't independently execute it.

No blocking issues found. Nice, self-verifying PR — approve.

@alirezarezvani
alirezarezvani marked this pull request as ready for review August 26, 2026 06:02
@alirezarezvani
alirezarezvani merged commit 8def1c4 into dev Aug 26, 2026
8 checks passed
@alirezarezvani
alirezarezvani deleted the claude/spinning-up-book-skill-hhbjpy branch August 26, 2026 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant