From 783dd37251ab1489edd01c8d53efe65b25f999ee Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Fri, 18 Sep 2026 16:24:56 +0700 Subject: [PATCH] docs(plans): record the JavaScript migration, parity, and review Adds the migration plan and its phase files, the parity report, and the code review. Annotates the two records this work supersedes rather than rewriting them: the 2026-09-18 language-choice research, whose first recommendation was to keep the engine in Go and was overridden the same day, and the 2026-08-18 plan that produced that engine. Both bodies are left as written, since erasing them would destroy the reasoning a future reader needs. Six success criteria stay unticked with the reason inline: they need a push, a GitHub token, or a Codex run to verify. --- .../plan.md | 1 + .../phase-01-port-engine-to-javascript.md | 216 ++++++++++++ .../phase-02-parity-test-suite.md | 164 ++++++++++ ...phase-03-skills-restructure-and-cutover.md | 191 +++++++++++ .../phase-04-retire-go-toolchain.md | 125 +++++++ .../phase-05-unify-module-extensions.md | 124 +++++++ .../plan.md | 238 ++++++++++++++ ...e-review-260918-newsletter-js-migration.md | 308 ++++++++++++++++++ ...y-260918-newsletter-js-migration-report.md | 174 ++++++++++ ...arch-260918-1139-script-language-choice.md | 15 + 10 files changed, 1556 insertions(+) create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-01-port-engine-to-javascript.md create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-02-parity-test-suite.md create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-03-skills-restructure-and-cutover.md create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-04-retire-go-toolchain.md create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-05-unify-module-extensions.md create mode 100644 plans/260918-0647-migrate-newsletter-engine-to-javascript/plan.md create mode 100644 plans/reports/code-review-260918-newsletter-js-migration.md create mode 100644 plans/reports/parity-260918-newsletter-js-migration-report.md diff --git a/plans/260818-2056-migrate-newsletter-scripts-to-go/plan.md b/plans/260818-2056-migrate-newsletter-scripts-to-go/plan.md index 3b07052..a410645 100644 --- a/plans/260818-2056-migrate-newsletter-scripts-to-go/plan.md +++ b/plans/260818-2056-migrate-newsletter-scripts-to-go/plan.md @@ -1,6 +1,7 @@ # Migrate Newsletter Scripts from Node.js to Go **Status:** COMPLETED (2026-08-18) — all 3 phases done; parity report in `plans/reports/parity-260818-newsletter-go-migration-report.md` +**Superseded:** 2026-09-18 — the Go engine this plan delivered was ported back to JavaScript by `plans/260918-0647-migrate-newsletter-engine-to-javascript/`. Body left unchanged; it records the reasoning as it stood. **Created:** 2026-08-18 **Scope:** Port the 8-file shared engine in `scripts/newsletter/` (~740 LoC JS, stdlib-only) to a single Go package with 6 subcommands, verify behavior parity, cut over all skill/doc call sites, delete the JS. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-01-port-engine-to-javascript.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-01-port-engine-to-javascript.md new file mode 100644 index 0000000..b6080df --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-01-port-engine-to-javascript.md @@ -0,0 +1,216 @@ +--- +phase: 1 +title: "Port the engine to JavaScript" +status: done +priority: P1 +effort: "1.5d" +dependencies: [] +--- + +# Phase 1: Port the engine to JavaScript + +## Overview + +Translate the 1168-LOC Go engine into Node ESM modules under +`scripts/newsletter/`, one JS file per Go file, replacing hand-rolled HTML/XML +regexes with cheerio. Go and JS coexist at the end of this phase — nothing is +deleted until phase 4. + +## Requirements + +**Functional** +- `node scripts/newsletter [args]` dispatches all seven subcommands from + the repo root, with the same usage text and the same argument shapes. +- Output is byte-identical to the Go engine except where this phase names a + deliberate difference. +- Repo-relative paths (`content/post`) resolve from `process.cwd()`, preserving + the "invoked from the repo root" contract. +- `config/substack-publications.json` stays the editable source of truth, read at + runtime (no `go:embed` equivalent needed), with the same + `["blog.bytebytego.com"]` fallback on read or parse failure. + +**Non-functional** +- ESLint clean under the existing `eslint.config.mjs` (`eqeqeq`, `prefer-const`, + `no-unused-vars` with `^_` escape). +- JSDoc type annotations on every exported function, per repo policy — types + without TypeScript. +- Exactly three runtime dependencies. No dev dependencies. + +## Architecture + +``` +scripts/newsletter/ + index.js subcommand dispatch, usage, printJson, dep-guard + url-utils.js cleanUrl, bareUrl, isSubstackImage, substackImageUuid, + fetchTextOk, checkAccessibility, collectMarkdown, + checkDuplicate, classifyType, boundary helpers + html-text.js stripTags, itemTitle, itemLink, extractCandidates, + captionForUuid, postTitleFromHtml [cheerio] + add-url.js add-url + detect-image-source.js detect-image-source + find-substack-post.js find-substack-post [cheerio] + find-newsletter-number.js find-newsletter-number + list-existing-tags.js list-existing-tags + fetch-via-defuddle.js fetch-via-defuddle [defuddle] + post-stats.js post-stats + config/substack-publications.json (unchanged) +``` + +Each command module exports a single `run*(args)` function; `index.js` owns +dispatch and the missing-dependency guard. + +**Deviation, made during review:** the shared JSON printer moved out of +`index.js` into its own `json-out.js`. Keeping it in `index.js` meant every +JSON-emitting command imported the dispatcher, and `index.js` runs `main()` at +module scope — so importing a command module executed the CLI against the host +argv, and the pure functions this phase exports for testing could not be imported +at all. One extra module removes the cycle outright. + +`printJson(v)` = `process.stdout.write(JSON.stringify(v, null, 2) + "\n")`. +This matches Go's `SetEscapeHTML(false)` + `SetIndent("", " ")` + `Encode` +exactly: JS does not HTML-escape, and `Encode` appends the newline. + +## Related code files + +- Create: `scripts/newsletter/index.js` and the nine modules above +- Modify: `package.json` (dependencies, `test` script) +- Modify: `package-lock.json` (via `npm install`) +- Unchanged: `scripts/newsletter/config/substack-publications.json` +- Untouched this phase: all `scripts/newsletter/*.go` + +## The seven parity traps + +Each is a place where the idiomatic JS differs from what the Go does. These are +the reason phase 2 exists; get them right here. + +**T1 — `cleanUrl` must not re-encode the query.** `url_utils.go` splits +`u.RawQuery` on `&` and rejoins surviving pairs verbatim, with a comment +explaining that Go's `url.Values` would sort keys and that sorted output must not +leak into stored `clean_url` values. JS `URLSearchParams.delete()` preserves +order but *normalizes percent-encoding* (e.g. `%7E` → `~`, `+` → `%20`), which is +the same class of bug. Split `url.search.slice(1)` on `&` and rejoin the kept +pairs as raw strings. Filter rule: drop any key whose lowercase form starts with +`utm_`, or is in the exact-match set (`fbclid gclid msclkid mc_eid aid ref +ref_src ref_url source s ck_subscriber_id igshid yclid vero_id`). + +**T2 — keep `hasBoundaryMatch` as an index loop.** Go hand-rolls it because RE2 +has no lookahead. JS regex *does* have lookahead, so the tempting move is to +collapse it into one pattern. Do not: the byte-offset boundary checks +(`uuidBoundaryOk`, `urlBoundaryOk`, including the optional trailing-slash skip +and the `)]"'?#<>_&,` delimiter set) are what make a prefix URL not a false +duplicate. Port the loop literally. A regex rewrite is a separate, separately +verified change. + +**T3 — `omitempty` fields must be absent, not empty.** Go omits `title`, +`author` (add-url) and `uuid`, `innerUrl` (detect-image-source) when empty. +Build those objects conditionally; never emit `"title": ""`. + +**T4 — the deep-miss `cutoff` is `null`, not omitted.** `find-substack-post +--deep` on a total miss emits `{found, source, scanned, budget, cutoff}` where +`cutoff` is a `*string` — JSON `null` when no sitemap could be fetched. Emit +`null` explicitly. + +**T5 — count runes, not UTF-16 units.** `extractCandidates` filters on +`utf8.RuneCountInString(text) < 6 || > 70`. JS `.length` counts UTF-16 code +units, so any non-BMP character (emoji in a TOC bullet) counts double and can +push a valid title past 70. Use `[...text].length`. + +**T6 — `list-existing-tags` output is `%6d %s`.** Right-align the count in a +6-character field: `String(count).padStart(6) + " " + tag`. It is plain text, not +JSON, and the skill reads it. + +**T7 — `extractInnerUrl` swallows decode errors.** Go's `url.PathUnescape` +returns an error on a malformed `%` sequence and the code falls back to the raw +substring. JS `decodeURIComponent` *throws*. Wrap in try/catch and return the +raw substring on failure. + +Two more that are free in JS but worth confirming: `extractCandidates` must +marshal as `[]` not `null` when empty (natural in JS; Go needed an explicit +non-nil slice), and `URL` already serializes an empty path as `/` (Go had to add +that by hand). + +## Implementation steps + +1. `npm install cheerio defuddle linkedom` — confirm the lockfile records exactly + these three plus their transitive deps, and that `npm ci` works offline + afterward on this ARM64 box. +2. Add to `package.json`: `"test": "node --test scripts/newsletter/*.test.js"`. + Leave `lint` and `projects:refresh` alone. +3. Write `index.js`: usage text (updated to `node scripts/newsletter …`), + `switch` dispatch over `process.argv[2]`, `printJson`, and the R1 dependency + guard — wrap the dynamic import of each command module and translate + `ERR_MODULE_NOT_FOUND` into + `newsletter engine: dependencies missing — run 'npm ci' from the repo root` + on stderr with exit 1. +4. Port `url-utils.js`. No dependencies — pure stdlib (`node:fs`, `node:path`, + `URL`, global `fetch`). Apply T1 and T2. `fetchTextOk` and + `checkAccessibility` use `AbortSignal.timeout(ms)`; `checkAccessibility` + returns the literal string `"000"` on any network error. +5. Port `html-text.js` using cheerio. This is the phase's real work: + - `stripTags` → `cheerio.load(s).text()` then collapse whitespace and trim. + Note cheerio decodes entities, replacing Go's `html.UnescapeString`. + - `itemTitle` / `itemLink` → `$('title').first()` / `$('link').first()` under + `xmlMode: true`, which handles CDATA natively. + - `extractCandidates` → `$('li')` with the T5 rune filter, case-insensitive + dedupe, first-seen order preserved. + - `captionForUuid` → find the element whose serialized subtree contains the + uuid, walk up with `.closest('figure')`, read `figcaption`. This replaces + the `LastIndex("")` string surgery; expect + *better* results on nested figures and record any difference under R3. + - `postTitleFromHtml` → `meta[property="og:title"]` → `h1` → `title`, same + precedence. +6. Port the five straightforward commands: `add-url.js`, `detect-image-source.js` + (T7), `find-newsletter-number.js` (keep the per-year early exit: scan a whole + year's months and days, then break only if that year yielded a number), + `list-existing-tags.js` (T6), `post-stats.js`. +7. Port `find-substack-post.js`: `node:util`'s `parseArgs` for `--uuid` / + `--deep`; cheerio `xmlMode` for RSS `` and sitemap `//`; + the 3-month cutoff; newest-first stable sort; the shared `deepFetchBudget = 40` + spanning all publications; T4 on the miss output. Read the publications JSON + at runtime with the documented fallback. +8. Port `fetch-via-defuddle.js` with the two-tier design from `plan.md`: + local `defuddle/node` first, then `https://defuddle.md/`. Exit 2 on bad + args, 1 on both tiers failing, 0 with the body on stdout. Stderr names which + tier failed and why. +9. `npx eslint scripts/newsletter/` and fix. Add JSDoc to every exported + function. +10. Smoke every subcommand once by hand against a live URL before handing off to + phase 2. + +## Success criteria + +- [x] All seven subcommands run under `node scripts/newsletter ` without a + stack trace. +- [x] `npx eslint scripts/newsletter/` is clean. +- [x] Every exported function carries a JSDoc type annotation. +- [x] `package.json` lists exactly `cheerio`, `defuddle`, `linkedom` as new + dependencies; no new devDependencies. +- [x] Each of T1-T7 is addressed, with a code comment at the site explaining the + constraint (explain the invariant, not the trap number). +- [x] Deleting `node_modules` and running any subcommand prints the `npm ci` + message, not a module-resolution stack trace. +- [x] No `.go` file modified or deleted. + +## Risk assessment + +**Cheerio's whitespace and entity handling differs from `html.UnescapeString` + +regex.** `stripTags` is used for captions and candidate titles that land verbatim +in published posts. +*Signal:* phase-2 fixture mismatches concentrated in `extractCandidates` / +`captionForUuid`. +*Response:* adjust the cheerio text pipeline to match Go on the fixtures. If a +difference is cheerio being correct on malformed markup, accept and record under +R3 — do not contort the parser back into regex behavior. + +**`defuddle/node` may not work on headless ARM64 Linux.** linkedom is pure JS so +it should, but this is unverified. +*Signal:* step 1 or step 8 fails on import or on a real extraction. +*Response:* ship `fetch-via-defuddle` with the `defuddle.md` proxy tier only, +drop `defuddle` and `linkedom` from `package.json`, and note the reduction — the +command keeps exactly its current Go behavior, so nothing regresses. + +**`parseArgs` is stricter than Go's `flag`.** Go's package accepts `-uuid` as +well as `--uuid`; `parseArgs` accepts only `--uuid`. +*Signal:* a skill or a user invoking the single-dash form. +*Response:* accept the narrowing — every documented call site uses `--uuid` — +and state it in the phase-2 report as a deliberate CLI difference. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-02-parity-test-suite.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-02-parity-test-suite.md new file mode 100644 index 0000000..dbc6fee --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-02-parity-test-suite.md @@ -0,0 +1,164 @@ +--- +phase: 2 +title: "Golden-output parity suite" +status: done +priority: P1 +effort: "0.5d" +dependencies: [1] +--- + +# Phase 2: Golden-output parity suite + +## Overview + +Capture the Go engine's stdout for a fixed case matrix, commit those outputs as +fixtures, and assert the JS engine reproduces them using `node:test`. This is +both the migration's proof of correctness and the regression net the engine has +never had. It must run while the Go engine still exists. + +## Requirements + +**Functional** +- Every subcommand covered; every parity trap T1-T7 from phase 1 covered by at + least one case that would fail if the trap were mishandled. +- Fixtures are generated from the Go engine, never hand-written — a hand-written + fixture proves nothing about parity. +- Network-dependent cases are separated from deterministic ones so `npm test` + stays runnable offline. + +**Non-functional** +- `node:test` + `node:assert` only. No test framework dependency. +- The suite runs in under 30 seconds offline. + +## Architecture + +``` +scripts/newsletter/ + parity.test.js deterministic cases, offline, runs in npm test + network.test.js live-network cases, skipped unless NEWSLETTER_NET=1 + __fixtures__/ + golden/.json or .txt — captured Go stdout, committed + repo/content/post/ a small synthetic content tree; __fixtures__/repo + stands in as a repo root via the subprocess cwd +``` + +**Two test tiers.** Deterministic cases (URL cleaning, classification, dedup +against a fixture content tree, post-stats, tag listing, newsletter numbering, +HTML/RSS extraction from saved markup) run always. Live-network cases (YouTube +oEmbed, Substack RSS and sitemap, defuddle) run only with `NEWSLETTER_NET=1`, +because upstream content changes and a CI-red-on-someone-else's-outage test +teaches people to ignore red. + +**Fixture content tree.** Dedup and the content-scanning commands must not read +the real `content/post` — it grows every day, so a fixture captured today fails +tomorrow. Build a small synthetic tree under `__fixtures__/repo/content/post/` +covering: +a post with a `Newsletter #N` heading, a post with a stored bare URL, a post with +a stored URL that is a strict *prefix* of the test URL (the T2 false-duplicate +case), a post with a Substack uuid in `
` form, a post with a uuid in +markdown-autolink form, and a post with a frontmatter `tags:` array. + +Tests point the engine at that tree by **setting the subprocess working +directory**, not by adding a config knob. Both engines already resolve +`content/post` relative to the process CWD — that is the documented repo-root +invocation contract — so `execFileSync(…, { cwd: fixtureRoot })` redirects every +content-scanning command with no change to either engine. The fixture tree +therefore lives at `__fixtures__/repo/content/post/…` so that `__fixtures__/repo` +can stand in as a repo root. + +**Saved markup.** RSS and sitemap parsing cases use saved upstream responses in +`__fixtures__/`, captured once. This is what lets cheerio-vs-regex differences be +compared deterministically. + +## Related code files + +- Create: `scripts/newsletter/parity.test.js` +- Create: `scripts/newsletter/network.test.js` +- Create: `scripts/newsletter/__fixtures__/` (golden outputs, `repo/content/post` tree, saved markup) +- Create: `plans/reports/parity-260918-newsletter-js-migration-report.md` +- Modify: `scripts/newsletter/*.js` — only if a fixture mismatch reveals a port bug +- Modify: `.gitignore` if any capture artifact must be excluded (prefer not to) + +## Case matrix + +Capture from Go, assert against JS. Minimum coverage: + +| Subcommand | Cases | +|---|---| +| `add-url` | article; article with `utm_*` + `fbclid` + `ref` params (T1); article with a param that must survive; `youtube.com/watch?v=`; `youtu.be/`; `youtube.com/shorts/`; a YouTube playlist URL (must *not* route youtube); substackcdn image URL; raw S3 substack image; `.pdf`; `.mp4`; `.avif` with query string; a URL that is a prefix of one already stored (T2, expect `duplicate:false`); a URL genuinely stored (expect `duplicate:true`); a non-YouTube URL (T3, expect no `title` key at all) | +| `detect-image-source` | substackcdn `/image/fetch/` wrapper; raw S3; non-Substack image (T3, no `uuid`/`innerUrl` keys); a wrapper with a malformed `%` sequence (T7) | +| `find-substack-post` | saved RSS containing the uuid; saved RSS missing it (`{found:false}`); `--deep` total miss against a saved sitemap (T4, `cutoff` present and `null` when the sitemap fetch fails); a saved RSS item whose TOC bullets include a non-BMP character near the 70-rune limit (T5) | +| `find-newsletter-number` | fixture tree with newsletters; empty tree (expect `1`); a tree where the newest year has no newsletter but an older year does (per-year early exit) | +| `list-existing-tags` | fixture tree — assert the exact `%6d %s` text including alignment (T6); tie-break ordering between two tags with equal counts | +| `post-stats` | a post with articles, all three Bonus subsections, and a `Newsletter #N` heading; a post with an empty Bonus; a post with no Bonus at all | +| `fetch-via-defuddle` | bad args → exit 2; unreachable host → exit 1; success → exit 0 with a body (network tier) | + +## Implementation steps + +1. Build `__fixtures__/repo/content/post/` by hand — six or so small `index.md` + files, each documented with the case it exists for. +2. Confirm both engines resolve content paths from CWD with no env override, so + the same fixture root drives the Go capture and the JS assertion. +3. Save the upstream RSS and sitemap responses used by the extraction cases. +4. Write a capture script in the scratchpad (not the repo) that runs every + deterministic case through `go run ./scripts/newsletter …` — with the + content-scanning cases run from `__fixtures__/repo` as CWD, using an absolute + path to the Go package — and writes + `__fixtures__/golden/`. Record exit codes alongside stdout. +5. Review every captured fixture by eye before committing. A fixture that + encodes a Go bug becomes a requirement the JS must match forever — catch that + now, and if one is found, record it as a known-bug fixture with a comment + rather than silently "fixing" it in the port. +6. Write `parity.test.js`: for each case, invoke the JS engine as a subprocess + (`node:child_process` `execFileSync`) so the test exercises the real CLI + surface including exit codes, passing `cwd: __fixtures__/repo` for the + content-scanning cases, and compare stdout to the golden file. +7. Write `network.test.js` with the same shape, guarded by + `{ skip: process.env.NEWSLETTER_NET !== "1" }`. +8. Run `npm test`. Every mismatch is triaged into exactly one of: **port bug** + (fix the JS), **accepted improvement** (cheerio extracting better text; update + the fixture and record the reason), or **accepted CLI difference** (e.g. + `parseArgs` rejecting `-uuid`). +9. Run the network tier once manually with `NEWSLETTER_NET=1`. +10. Write `plans/reports/parity-260918-newsletter-js-migration-report.md`: the + matrix, pass/fail per case, and every accepted difference with its + justification. Mirror the structure of the existing + `parity-260818-newsletter-go-migration-report.md`. + +## Success criteria + +- [x] `npm test` passes offline from a clean `npm ci`. +- [x] `NEWSLETTER_NET=1 npm test` passes against live upstreams. +- [x] Every subcommand has at least one case; T1-T7 each have a case that fails + if the trap is mishandled (verify by temporarily reintroducing the naive + implementation and watching the test go red). +- [x] Every fixture was generated from the Go engine and eyeballed before commit. +- [x] Zero unexplained differences. Every accepted difference is in the report + with a reason. +- [x] Tests read only `__fixtures__/`, never the real `content/post` — verify by + running the suite with the real `content/post` temporarily renamed. +- [x] Neither engine gained a test-only config knob; redirection is CWD alone. + +## Risk assessment + +**Fixtures that encode Go bugs.** Capturing blindly turns any existing bug into a +permanent contract. +*Signal:* step 5's review finds output that is wrong on its own terms. +*Response:* keep the fixture as-is with a comment naming the wrong behavior, so +the port is provably faithful; fix it as a separate change after cutover. + +**Live-network cases rot.** ByteByteGo's RSS window moves; a uuid that resolves +today will not in a month. +*Mitigation:* the network tier is opt-in and never gates `npm test`. +*Response:* if a network case goes stale, re-capture the saved markup and move +the assertion into the deterministic tier rather than chasing live content. + +**The synthetic content tree misses a real-world markdown shape.** Dedup +boundary handling is the highest-consequence logic in the engine — a false +negative publishes a duplicate, a false positive silently drops a URL the user +asked for. +*Mitigation:* seed the fixture tree by grepping the real `content/post` for the +distinct shapes stored URLs actually take (bare, trailing slash, autolink +`<…>`, inside a markdown link, inside an image) and cover each. +*Response:* if a shape is found that neither engine handles, that is a +pre-existing bug — record it, do not fix it inside the migration. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-03-skills-restructure-and-cutover.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-03-skills-restructure-and-cutover.md new file mode 100644 index 0000000..c8bf844 --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-03-skills-restructure-and-cutover.md @@ -0,0 +1,191 @@ +--- +phase: 3 +title: "Skill restructure and call-site cutover" +status: done +priority: P2 +effort: "0.5d" +dependencies: [2] +--- + +# Phase 3: Skill restructure and call-site cutover + +## Overview + +Switch every `go run ./scripts/newsletter` call site to `node scripts/newsletter` +and, in the same pass, fix the three structural problems in the skill layer: +duplicated Codex adapters, a shared reference file owned by one skill, and the +engine's command list spelled out in five different files. + +## Requirements + +**Functional** +- No `go run` anywhere outside `plans/` and `scripts/newsletter/*.go` (the Go + sources document their own invocation in doc comments and are deleted whole in + phase 4; `plans/` are historical records and stay as written). +- The seven engine commands are enumerated in exactly one file. Every other + mention is a link, except a skill inlining the one or two commands it actually + invokes. +- Shared skill prose is owned by no individual skill. +- `.agents/` carries no content that can drift from `.claude/`. +- Setup docs state the `npm ci` prerequisite (R1) once, where a new user will + hit it. + +**Non-functional** +- Both Claude Code and Codex still discover and run all six skills afterward. +- Skill frontmatter `description` fields keep their trigger wording — they are + how the runtimes decide to invoke a skill, and rewording them changes routing + behavior. + +## Architecture + +**Shared prose moves to `docs/newsletter/`,** not into another skill's directory. +`docs/` already exists and is tool-neutral, which is the point: `.claude/` and +`.agents/` both reference it by repo-root-relative path, so nothing breaks under +symlinks and no skill owns another skill's documentation. + +``` +docs/newsletter/ + engine-commands.md the ONE enumeration of the seven subcommands + post-mechanics.md moved from .claude/skills/mt-add-url/references/ +``` + +**`.agents/skills/mt-*/SKILL.md` become symlinks** to their `.claude` +counterparts, eliminating the parallel frontmatter. Verify Codex follows symlinks +before committing; if it does not, keep the current thin pointers (they are +already only 10 lines) and settle for centralizing the command table. + +**Renames — confirmed 2026-09-18.** The scheme is uniform +`mt--`; four of the six already conform, so only two move: +`mt-add-post` → `mt-add-article` (it adds an article *to* a post) and +`mt-webfetch` → `mt-fetch-url`. `mt-add-url`, `mt-add-video`, `mt-add-image` and +`mt-add-tags` keep their names — renaming a conforming skill is churn. Directory +name and frontmatter `name:` change together, and every cross-reference in the +other skills, `AGENTS.md`, and `README.md` follows. + + + +## Related code files + +- Create: `docs/newsletter/engine-commands.md` +- Create: `docs/newsletter/post-mechanics.md` (git mv from + `.claude/skills/mt-add-url/references/newsletter-post-mechanics.md`) +- Delete: `.claude/skills/mt-add-url/references/` (now empty) +- Rename: `.claude/skills/mt-add-post/` → `mt-add-article/`; + `.claude/skills/mt-webfetch/` → `mt-fetch-url/`; same under `.agents/skills/` +- Modify: all six `.claude/skills/*/SKILL.md` +- Replace: all six `.agents/skills/*/SKILL.md` (symlinks) +- Modify: `AGENTS.md` (lines 42-51 and the routing section), `README.md` (line 42), + `docs/multi-tool-usage.md` (lines 3, 13, 26, 27, 29) +- Modify: `opencode.json` — it already allows `node *` and never allowed `go *`; + confirm no change is needed rather than assuming + +## Call-site inventory + +Verified by grep at plan time. Every `go run` occurrence outside `plans/`: + +| File | Lines | Commands | +|---|---|---| +| `AGENTS.md` | 42, 45-51 | all seven + the prose describing the engine as Go | +| `.claude/skills/mt-add-url/SKILL.md` | 8, 23, 65 | `add-url`, `post-stats` | +| `.claude/skills/mt-add-url/references/newsletter-post-mechanics.md` | 4, 29, 91 | `find-newsletter-number`, `post-stats` | +| `.claude/skills/mt-add-post/SKILL.md` | 10, 16 | `add-url` | +| `.claude/skills/mt-add-video/SKILL.md` | 12, 22 | `add-url` | +| `.claude/skills/mt-add-image/SKILL.md` | 10, 24, 30, 40, 44, 71 | `detect-image-source`, `add-url`, `find-substack-post` (+`--deep`) | +| `.claude/skills/mt-add-tags/SKILL.md` | 53 | `list-existing-tags` | +| `.claude/skills/mt-webfetch/SKILL.md` | 31, 82 | `fetch-via-defuddle` | +| `README.md` | 42 | prose: "Go via `go run`" | +| `docs/multi-tool-usage.md` | 3, 13, 26, 27, 29 | prose + the shared-engine invocation | + +## Implementation steps + +1. Re-read the confirmed rename scheme above before touching anything — every + later step bakes the names in. +2. Write `docs/newsletter/engine-commands.md`: the seven commands, their + arguments, their output shapes, exit codes (including + `fetch-via-defuddle`'s now-meaningful 2-vs-1), the repo-root invocation + contract, and the `npm ci` prerequisite. +3. `git mv` the shared reference to `docs/newsletter/post-mechanics.md`. Update + its own line 4 (`All shared scripts live in scripts/newsletter/`) and its two + command blocks. Remove the now-empty `references/` directory. +4. Rename the two skill directories with `git mv`, under both `.claude/` and + `.agents/`, and update each `SKILL.md` frontmatter `name:`. +5. Update all six `.claude` skills: `go run ./scripts/newsletter X` → + `node scripts/newsletter X`; `../mt-add-url/references/newsletter-post-mechanics.md` + → `docs/newsletter/post-mechanics.md`; `mt-add-post` → `mt-add-article` and + `mt-webfetch` → `mt-fetch-url` in every cross-reference; add a link to + `docs/newsletter/engine-commands.md` where a skill currently explains the + engine. Leave the `description:` frontmatter wording alone except for the two + renamed names. +6. Update `mt-fetch-url`'s tier list for the new two-stage tier 1 (local + defuddle, then the `defuddle.md` proxy) from phase 1 — the current text + describes a single defuddle step. The chain becomes: WebFetch → local defuddle + → defuddle.md → reader proxy → report failure. +7. Test the symlink approach: replace one `.agents` adapter with a symlink, run + Codex, confirm it discovers and follows the skill. On success convert the + other five; on failure revert to thin pointers and note the reason in + `docs/multi-tool-usage.md`. +8. Rewrite `AGENTS.md` lines 40-52: describe the engine as Node, replace the + seven-command block with a one-line pointer to + `docs/newsletter/engine-commands.md` plus the `npm ci` prerequisite, and + update the skill names in the routing section and the Codex `$mt-*` list. +9. Update `README.md` line 42 and `docs/multi-tool-usage.md` lines 3, 13, 26, 27, + 29 — Node not Go, link the command reference rather than restating it, and + keep `docs/newsletter/` in the "what you must keep" list in the + pick-one-tool section. +10. Verify: `grep -rn "go run\|mt-add-post\|mt-webfetch" .claude .agents AGENTS.md + README.md docs` returns nothing. Do not include `scripts/` — the Go sources + still exist and reference themselves. +11. Verify the enumeration rule: `grep -rln "find-newsletter-number" .claude + .agents AGENTS.md README.md docs` should show `docs/newsletter/` files and + only those skills that actually invoke that command. +12. Run one real `mt-add-url` flow end to end — an article, a YouTube link, and a + Substack image into a scratch post — under Claude Code, then repeat the + dispatch step under Codex. + +## Success criteria + +- [x] `grep -rn "go run" .claude .agents AGENTS.md README.md docs` → empty + (`scripts/` is excluded until phase 4: the 10 Go sources carry 16 `go run` + occurrences in their own doc comments) +- [x] `grep -rn "mt-add-post\|mt-webfetch" .claude .agents AGENTS.md README.md docs` → empty +- [x] All seven commands enumerated in `docs/newsletter/engine-commands.md` and + nowhere else +- [x] No skill references a path inside another skill's directory +- [x] `.agents/skills/` contains no prose that can drift from `.claude/skills/` + (symlinks), or the fallback is documented with the reason +- [x] Claude Code lists and runs all six skills, two under new names +- [x] All six skill directory names satisfy `mt--` +- [ ] Codex discovers all six skills and `$mt-add-url` dispatches correctly — deferred: the user will verify on their next Codex run (symlinks were chosen over thin pointers at their direction) +- [x] One real end-to-end `mt-add-url` flow produces the expected markdown +- [x] The `npm ci` prerequisite appears in `AGENTS.md`, `README.md`, and + `docs/newsletter/engine-commands.md` + +## Risk assessment + +**Renaming skills breaks the user's muscle memory and any saved invocation.** +`mt-add-post` and `mt-webfetch` are typed by hand today. +*Signal:* the user or a runtime invoking an old name after cutover. +*Response:* the renames are the user's request, so carry them out — but call out +in the final report that `$mt-add-post` and `/mt-webfetch` no longer resolve. +Do not leave alias directories; two names for one skill is the duplication this +phase exists to remove. + +**Frontmatter `description` is routing logic, not documentation.** These strings +are how Claude Code and Codex decide whether to invoke a skill. An edit that +looks like tidying can stop a skill from triggering. +*Mitigation:* change only the skill names inside them; leave every trigger phrase +untouched. +*Signal:* step 12's end-to-end run failing to auto-dispatch. +*Response:* restore the original wording verbatim and change only the name token. + +**Symlinked skills may break Codex discovery, or break on a Windows checkout.** +*Signal:* step 7. +*Response:* thin pointers, as today. This is a nice-to-have; the command-table +centralization is the part that actually reduces future edit cost. + +**Moving `post-mechanics.md` out of `.claude/skills/` could put it outside what a +runtime packages with a skill.** Claude Code reads repo-relative paths fine, but +this changes the file's relationship to the skill. +*Signal:* a skill run that cannot read the referenced file. +*Response:* move it back under `.claude/skills/` in a shared, non-skill +subdirectory and have `.agents` reference it by repo-root-relative path instead. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-04-retire-go-toolchain.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-04-retire-go-toolchain.md new file mode 100644 index 0000000..19dc7b5 --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-04-retire-go-toolchain.md @@ -0,0 +1,125 @@ +--- +phase: 4 +title: "Retire the Go toolchain" +status: done +priority: P2 +effort: "2h" +dependencies: [2, 3] +--- + +# Phase 4: Retire the Go toolchain + +## Overview + +Delete the Go engine and remove Go from the repo, GitHub Actions, and Netlify. +The newsletter engine is Go's only consumer here — Hugo vendors its theme in +`themes/` and `config/_default/` has no `[module]` section, so nothing else +needs a Go toolchain. This is the only irreversible phase; it runs last and only +after phase 2 is green. + +## Requirements + +**Functional** +- No `.go` file, no `go.mod`, no Go step in CI, no `GO_VERSION` anywhere. +- The Hugo build still succeeds on GitHub Actions and on Netlify. +- The superseded research recommendation is annotated, not left contradicting + the repo. + +**Non-functional** +- One commit per concern (engine deletion, CI, docs/report) so a bisect can + isolate a Hugo build break to the CI change. + +## Architecture + +Go's footprint, verified by grep at plan time: + +| File | What to remove | +|---|---| +| `scripts/newsletter/*.go` | all 10 files | +| `go.mod` | the whole file (there is no `go.sum` — the engine was stdlib-only) | +| `.github/workflows/hugo.yml` | `GO_VERSION: 1.25.5` (line 23), the `Setup Go` step (lines 33-37), and the Go line in Verify installations (line 64) | +| `netlify.toml` | `GO_VERSION = "1.25.5"` (line 3) | + +Removing the pins also disposes of the latent defect the research report flagged: +`go.mod` declares `go 1.26` while both build environments pin 1.25.5, which only +works because `GOTOOLCHAIN=auto` silently downloads 1.26. Deleting Go resolves it +outright — no version alignment needed. + +## Related code files + +- Delete: `scripts/newsletter/add_url.go`, `detect_image_source.go`, + `fetch_via_defuddle.go`, `find_newsletter_number.go`, `find_substack_post.go`, + `html_text.go`, `list_existing_tags.go`, `main.go`, `post_stats.go`, + `url_utils.go` +- Delete: `go.mod` +- Modify: `.github/workflows/hugo.yml`, `netlify.toml` +- Modify: `.gitignore` — the Go block (`*.o`, `_cgo_*`, `_testmain.go`, `*.test`, + …) is dead weight afterward; remove it and keep `node_modules` +- Modify: `plans/reports/research-260918-1139-script-language-choice.md` +- Modify: `plans/260818-2056-migrate-newsletter-scripts-to-go/plan.md` — status + line only + +## Implementation steps + +1. Re-confirm phase 2 is green and phase 3's end-to-end run passed. Everything + below assumes the JS engine is the working engine. +2. Confirm Go has no other consumer: grep the repo's `*.yml`, `*.toml` and `*.md` + (excluding `node_modules/` and `plans/`) for every Go invocation and CI action + spelling, and check `config/_default/` for a `[module]` section. Both must + come back empty of anything but the sites listed above. +3. `git rm scripts/newsletter/*.go go.mod`. Commit alone. +4. Edit `.github/workflows/hugo.yml`: drop the `GO_VERSION` env entry, the whole + `Setup Go` step, and the Go line from Verify installations. Leave the Node + setup and the conditional `npm ci` — the lockfile exists, so `npm ci` runs, + which is what the engine now needs if CI ever invokes it. +5. Edit `netlify.toml`: drop `GO_VERSION`. Keep `NODE_VERSION = "24.12.0"`. +6. Remove the Go section from `.gitignore`. +7. Push and watch both builds. GitHub Actions must deploy; Netlify must build a + preview. Compare the built output against the previous deploy — the site + should be byte-identical, since Go never touched it. +8. Append a dated note to + `plans/reports/research-260918-1139-script-language-choice.md`, directly under + its recommendation #1, recording that the user overrode it on 2026-09-18, the + reason given, and a link to this plan. Do not delete or rewrite the original + recommendation — the report is a stateful record of what was concluded at the + time, and erasing it destroys the reasoning a future reader needs. +9. Update the old Go plan's status line to note it was superseded by this plan, + with the date. Leave its body untouched for the same reason. +10. Run `ak plan close` on this plan and `/ak:journal` for the migration. + +## Success criteria + +- [x] `find . -name "*.go" -not -path "./node_modules/*" -not -path "./themes/*"` → empty +- [x] `go.mod` gone; no `go.sum` was ever present +- [x] No `GO_VERSION` or Go-setup action reference remains in `.github/` or + `netlify.toml` +- [ ] GitHub Actions `hugo.yml` deploys green — not exercised: nothing pushed +- [ ] Netlify builds green — not exercised: nothing pushed +- [ ] Deployed site output unchanged from the pre-phase-4 deploy — not exercised: nothing pushed +- [x] The research report carries the override note; its original recommendation + is still readable +- [x] The 2026-08-18 Go migration plan is marked superseded +- [x] Every subcommand still works from a fresh clone + `npm ci` + +## Risk assessment + +**Something else needs Go and the grep missed it.** Hugo Modules are the obvious +candidate — a `[module]` block in Hugo config makes `hugo` itself require Go. +*Signal:* the Hugo build failing on Actions or Netlify after step 4 or 5. +*Response:* restore the Go setup step and the `GO_VERSION` pins (the engine stays +deleted — Hugo needing Go is unrelated to the engine) and align the pins to a +single version, which the research report wanted anyway. + +**Netlify caches the Go toolchain and the build passes now but fails on the next +cache eviction.** +*Mitigation:* step 7 checks a real Netlify build, and Netlify builds previews +from a clean environment per deploy. +*Response:* if a cold build fails for a Go-shaped reason, that contradicts step +2 — reopen the investigation rather than re-adding the pin blindly. + +**Irreversibility.** After this phase, the Go engine exists only in git history. +*Mitigation:* the phase ordering — parity proven (2) and skills cut over and +exercised end-to-end (3) before anything is deleted. +*Response:* recovery is `git checkout -- scripts/newsletter +go.mod`, which restores a fully working engine. Record that SHA in the journal +entry so it is findable without archaeology. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-05-unify-module-extensions.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-05-unify-module-extensions.md new file mode 100644 index 0000000..4eab166 --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/phase-05-unify-module-extensions.md @@ -0,0 +1,124 @@ +--- +phase: 5 +title: "Unify module extensions on .js" +status: done +priority: P3 +effort: "1h" +dependencies: [3] +--- + +# Phase 5: Unify module extensions on `.js` + +## Overview + +Rename `.github/scripts/*.mjs` to `.js` so the repo has one module extension. +Independent of the engine migration; sequenced after phase 3 only so the +newsletter cutover lands first and a CI break here is unambiguous. + +## Why `.js` and not `.mjs` + +Decided during validation (2026-09-18), on three checks against this repo: + +1. The root `package.json` declares `"type": "module"`, so a `.js` file here is + already ESM. `.mjs` restates what the manifest states. +2. Node's directory resolution requires `index.js` — verified empirically: with + only `eng/index.mjs` present, `node eng` fails with + `Cannot find module '…/eng'`; adding `eng/index.js` makes it resolve. This is + what keeps the engine invocation at `node scripts/newsletter ` rather + than a full file path, so `scripts/newsletter/` must use `.js` regardless. +3. `eslint.config.mjs` already globs `**/*.js` alongside `**/*.mjs`, so no lint + config change is needed. + +`.mjs` earns its place in a package without `"type": "module"`, or one shipping +dual CJS/ESM entry points. Neither applies. `eslint.config.mjs` itself keeps its +extension — ESLint's own convention, and it sits outside the `type` field's +reach in some resolution paths. + +## Requirements + +**Functional** +- No `.mjs` under `.github/scripts/`. +- `npm run projects:refresh` works locally. +- The `update-projects.yml` scheduled workflow runs green. + +**Non-functional** +- Renames use `git mv` so history follows the file. +- One commit, so a revert is clean if the workflow breaks. + +## Architecture + +Three files move; five reference sites follow. All verified by grep at plan time: + +| File | Action | +|---|---| +| `.github/scripts/update-projects-list.mjs` | → `update-projects-list.js` | +| `.github/scripts/lib/github-api.mjs` | → `lib/github-api.js` | +| `.github/scripts/lib/render-projects.mjs` | → `lib/render-projects.js` | + +| Reference site | Line(s) | What changes | +|---|---|---| +| `package.json` | 12 | `projects:refresh` script path | +| `.github/workflows/update-projects.yml` | 41 | `run: node …` path | +| `.github/scripts/update-projects-list.mjs` | 18, 25 | two `import` specifiers | +| `.github/scripts/update-projects-list.mjs` | 36, 37 | two JSDoc `import("…")` types | +| `.github/scripts/lib/render-projects.mjs` | 6, 51, 66, 80 | four JSDoc `import("…")` types | + +The JSDoc `import("./github-api.mjs").Repo` annotations are the easy miss: they +are type references inside comments, so a stale one does not throw at runtime — +it silently degrades to `any` and the repo loses the typing it adopted JSDoc for. + +## Related code files + +- Rename: the three files above +- Modify: `package.json`, `.github/workflows/update-projects.yml` +- Modify: the renamed files' own import specifiers and JSDoc type imports +- Unchanged: `eslint.config.mjs` + +## Implementation steps + +1. `git mv` the three files. +2. Update the two runtime `import` specifiers in `update-projects-list.js`. +3. Update all six JSDoc `import("…")` type references (two in + `update-projects-list.js`, four in `lib/render-projects.js`). +4. Update `package.json` line 12 and `update-projects.yml` line 41. +5. `grep -rn "\.mjs" package.json .github/` → only `eslint.config.mjs` style + matches should remain, and none under `.github/scripts/`. +6. `npm run lint` and `npm run projects:refresh` locally. The refresh needs a + GitHub token; if one is not available locally, confirm the script gets as far + as the auth check rather than failing on module resolution. +7. Push and let `update-projects.yml` run — trigger it manually rather than + waiting for the daily cron. + +## Success criteria + +- [x] `find .github/scripts -name "*.mjs"` → empty +- [x] `grep -rn "\.mjs" .github/ package.json` → empty +- [x] `npm run lint` clean +- [x] `npm run projects:refresh` resolves all modules (auth failure is acceptable + locally; a module-resolution failure is not) +- [ ] `update-projects.yml` completes green on a manual dispatch — not exercised: nothing pushed +- [ ] The projects page content it generates is unchanged — not exercised: needs a GitHub token and a workflow run + +## Risk assessment + +**A stale JSDoc `import("…")` type silently degrades to `any`.** Nothing throws, +lint stays green, and the repo quietly loses type coverage on `Repo` and +`Contribution` — the exact thing the JSDoc-not-TypeScript policy exists to +provide. +*Signal:* step 5's grep, which is why it checks JSDoc strings and not just +`import` statements. +*Response:* fix the reference. If any are found after the commit, add a lint rule +or a grep to CI rather than relying on the next person to remember. + +**`update-projects.yml` breaks and the projects page goes stale silently.** It is +a daily cron, so a failure is easy to miss. +*Mitigation:* step 7 triggers it manually instead of waiting. +*Response:* revert the single commit; the rename is cosmetic and nothing depends +on it. + +**Scope creep into an unrelated CI path.** This phase touches the projects +workflow, which has nothing to do with the newsletter engine. +*Mitigation:* it is its own phase, its own commit, and it is P3 — droppable +without affecting phases 1-4. +*Response:* if phase 4 leaves CI unstable for any reason, drop this phase rather +than stacking a second CI change on top. diff --git a/plans/260918-0647-migrate-newsletter-engine-to-javascript/plan.md b/plans/260918-0647-migrate-newsletter-engine-to-javascript/plan.md new file mode 100644 index 0000000..55660ec --- /dev/null +++ b/plans/260918-0647-migrate-newsletter-engine-to-javascript/plan.md @@ -0,0 +1,238 @@ +--- +title: "Migrate the newsletter engine from Go to JavaScript" +description: "Port the 1168-LOC stdlib-only Go newsletter engine to Node ESM with a parser dependency, prove parity with a committed golden-output suite, restructure the mt-* skills around a single command reference, and retire Go from the repo." +status: in-progress +priority: P1 +effort: "2-3d" +tags: [newsletter, scripts, skills, migration, javascript] +created: 2026-09-18 +--- + +# Migrate the newsletter engine from Go to JavaScript + +## Overview + +`scripts/newsletter/` is a 10-file, 1168-LOC `package main` in Go, stdlib-only, +invoked as `go run ./scripts/newsletter ` from ten files +across `.claude/skills/`, `.agents/skills/`, `AGENTS.md`, `README.md`, and +`docs/multi-tool-usage.md`. This plan replaces it with a Node ESM engine invoked +as `node scripts/newsletter `, takes on a small set of justified npm +dependencies, proves behavioral parity with committed golden fixtures, folds the +skill layer onto one canonical command reference, and deletes Go from the repo +and CI. + +The engine has seven subcommands: `add-url`, `find-newsletter-number`, +`list-existing-tags`, `detect-image-source`, `find-substack-post`, +`fetch-via-defuddle`, `post-stats`. + +## Context: this reverses a same-day recommendation + +`plans/reports/research-260918-1139-script-language-choice.md` (2026-09-18) +concludes *"`scripts/newsletter/` stays Go. No GitHub API, no deps, no latency +problem, three agent runtimes served identically."* `CLAUDE.md` also ranks Go +above JavaScript for new work. + +The user reviewed that and chose to migrate anyway (2026-09-18). Phase 4 records +the override in the report itself so the repo record is not self-contradictory. +The plan does not re-litigate the decision; it does carry the report's strongest +objection forward as the tracked risk (see **Risks**, R1). + +## Goals + +| # | Goal | Priority | +|---|------|----------| +| 1 | Seven subcommands running on Node with output parity against the Go engine | P1 | +| 2 | Hand-rolled HTML/XML regex extraction replaced by a real parser | P1 | +| 3 | A committed regression suite the engine did not have before | P1 | +| 4 | One canonical command reference; no enumeration of the engine's commands in more than one file | P2 | +| 5 | Go fully removed from the repo, CI, and Netlify | P2 | +| 6 | One module extension across the repo's JavaScript | P3 | + +## Non-goals + +- `.github/scripts/` keeps its current logic. Its Octokit migration is a separate + recommendation in the same research report and is not in scope. Phase 5 renames + its files' extensions only — no behavior change. +- No change to what the skills *do* — only to what they invoke and where their + shared prose lives. +- No new subcommands. The port is behavior-preserving except where this plan + names a deliberate improvement. + +## Design decisions + +**Invocation stays positional and short.** `node scripts/newsletter [args]` +from the repo root, matching the Go contract one-for-one. Node directory +resolution requires `scripts/newsletter/index.js` (not `.mjs`), so the new engine +uses `.js`; the root `package.json` already declares `"type": "module"`, so `.js` +is ESM and the existing `eslint.config.mjs` already lints `**/*.js`. +`.github/scripts/` currently uses `.mjs`; phase 5 unifies it onto `.js` for the +same reasons (see that phase for the empirical check). + +**Module layout mirrors the Go files 1:1** so the parity diff is reviewable: +`index.js` (dispatch) and one module per current `.go` file. A 1:1 map makes any +behavioral drift attributable to a single file. + +**Dependencies (3, each load-bearing).** The user authorized npm dependencies +while asking to stay minimal: + +| Package | Replaces | Why it earns its place | +|---|---|---| +| `cheerio` ^1.2.0 | ~12 hand-rolled regexes in `html_text.go` + `find_substack_post.go` | One parser covers HTML *and* RSS/sitemap XML (`xmlMode`), so no second XML dep. Eliminates `captionForUUID`'s manual `LastIndex("`. The point of that proxy is +that it fetches from a *different IP* than the blocked caller. Running defuddle +locally fetches from this box's IP instead — a genuinely different IP from the +one WebFetch uses, but not the same third IP the proxy provides. Both are kept: +local defuddle first, `defuddle.md` on failure. Dropping the proxy would silently +narrow the skill's coverage. + +**Exit codes get more precise.** `fetch-via-defuddle` defines exit 2 (bad args) +vs 1 (fetch failed), but `go run` collapses every nonzero exit to 1 — documented +in a comment in `fetch_via_defuddle.go`. Node preserves them, so the distinction +becomes real for the first time. + +## Phases + +| # | Phase | Status | Depends on | +|---|-------|--------|------------| +| 1 | [Port the engine to JavaScript](./phase-01-port-engine-to-javascript.md) | Done | — | +| 2 | [Golden-output parity suite](./phase-02-parity-test-suite.md) | Done | 1 | +| 3 | [Skill restructure and call-site cutover](./phase-03-skills-restructure-and-cutover.md) | Done | 2 | +| 4 | [Retire the Go toolchain](./phase-04-retire-go-toolchain.md) | Done | 2, 3 | +| 5 | [Unify module extensions on `.js`](./phase-05-unify-module-extensions.md) | Done | 3 | + +Go and JS coexist through phases 1-3 — parity cannot be measured against a +deleted reference. Phase 4 is the only irreversible step. Phase 5 is P3 and +independent; drop it without affecting anything else. + +## Success criteria + +- [x] All seven subcommands run under `node scripts/newsletter ` and match + the Go engine on the phase-2 case matrix (byte-identical stdout, or drift + recorded as an accepted improvement with a reason). +- [x] `npm test` passes from a clean `npm ci`. +- [x] `npm run lint` passes over `scripts/newsletter/`. +- [x] One real `mt-add-url` flow (article + YouTube + Substack image) completes + end-to-end on the JS engine and writes the same markdown the Go engine + would have. +- [x] `grep -rn "go run" .claude .agents AGENTS.md README.md docs scripts` returns + nothing. Evaluated after phase 4 — phase 3's narrower version of this check + excludes `scripts/`, because the Go sources document their own invocation + and survive until phase 4 deletes them. +- [x] The engine's seven commands are enumerated in exactly one file + (`docs/newsletter/engine-commands.md`); every other mention is a link. +- [x] No `.go` file, no `go.mod`, no Go setup step, no `GO_VERSION` anywhere in + the repo. +- [x] One module extension: no `.mjs` outside `eslint.config.mjs`. +- [x] A fresh `git clone` + `npm ci` runs every subcommand successfully. + +## Risks + +**R1 — `npm ci` becomes a prerequisite the engine never had.** This is the +research report's strongest objection and the migration's real cost. Go's +stdlib-only engine ran on a bare clone; the JS engine will not, because +`node_modules` is gitignored. Three agent runtimes (Claude Code, OpenCode, Codex) +on a headless ARM64 box all hit this. +*Mitigation:* `index.js` catches `ERR_MODULE_NOT_FOUND` on the dependency imports +and exits with `newsletter engine: dependencies missing — run 'npm ci' from the +repo root` instead of a raw stack trace. Setup docs state it once. +*Signal it broke:* any skill run failing with a module-resolution error. +*Response:* if this proves to be recurring friction rather than a one-time setup +step, vendor the three deps or fall back to the stdlib implementations in +`html_text.js` — do not re-litigate the language choice. + +**R2 — silent output drift in the port.** Seven of the Go functions encode +non-obvious decisions (query-string order preservation, `omitempty`, rune-vs-byte +length, `null` vs omitted fields). A JS-idiomatic rewrite breaks them quietly and +the damage lands in published posts. +*Mitigation:* phase 2's golden fixtures are captured from the Go engine *before* +the JS is trusted, and phase 1 lists all seven traps explicitly. +*Signal:* any fixture mismatch. +*Response:* match Go, do not "improve" — parity first, improvements as a separate +recorded decision. + +**R3 — cheerio changes extraction results.** Replacing regexes with a real parser +is the point, but a parser will legitimately extract *different* (usually better) +text on malformed markup. +*Mitigation:* treat extraction differences as expected-to-review rather than +expected-to-match; phase 2 records each one with a judgment. +*Response:* accept improvements, document them; reject anything that loses +content the regex found. + +**R4 — Codex may not follow symlinked skill files.** Phase 3's `.agents` +de-duplication assumes it does; this is untested. +*Mitigation:* verify before committing the symlinks. +*Response:* fall back to the current thin-pointer adapters, which already work. + +## Resolved during validation (2026-09-18) + +**Skill naming — uniform `mt--`.** Two of the six violate it today; +the other four already conform, so renaming them would be churn. + +| Current | New | +|---|---| +| `mt-add-url` | unchanged (dispatcher) | +| `mt-add-post` | **`mt-add-article`** — it adds an article *to* a post | +| `mt-add-video` | unchanged | +| `mt-add-image` | unchanged | +| `mt-add-tags` | unchanged | +| `mt-webfetch` | **`mt-fetch-url`** — verb-object, like its siblings | + +**Fixture redirection uses the subprocess CWD, not a config knob.** Both engines +already resolve `content/post` from the process working directory, so phase 2's +tests set `cwd` and neither engine gains a test-only environment variable. + +**R1 mitigation stays at a clear error message.** No auto-`npm ci` in the skills +and no self-installing engine; `npm ci` is a documented one-time setup step. + +**Module extension unified on `.js`** — see phase 5. + +## Validation Log + +### Session 1 — 2026-09-18 + +**Verification pass:** Standard tier (5 phases at time of writing: 4). +Claims checked 24 · Verified 19 · Failed 5. + +| # | Claim | Result | Evidence | +|---|---|---|---| +| F1 | `netlify.toml` `GO_VERSION` at line 4 | FAILED | it is line 3; `HUGO_VERSION` is line 4 — corrected in phase 4 | +| F2 | `hugo.yml` Setup Go step at lines 32-37 | FAILED | it is 33-37 — corrected in phase 4 | +| F3 | `hugo.yml` Go verify line at 61 | FAILED | it is line 64 — corrected in phase 4 | +| F4 | Phase 3 criterion `grep -rn "go run" … scripts` → empty | FAILED | the 10 Go sources carry 16 `go run` occurrences in their own doc comments, so it cannot pass before phase 4 — criterion rescoped to exclude `scripts/` | +| F5 | Phase 2 requires `NEWSLETTER_CONTENT_DIR`; phase 1 never adds it | FAILED | contract gap between phases — resolved by dropping the env var for CWD redirection | +| V1 | Node directory resolution needs `index.js`, not `index.mjs` | VERIFIED | empirical: `node eng` fails with `Cannot find module` when only `index.mjs` exists; resolves once `index.js` is added | +| V2 | Hugo has no `[module]` config, so Go has no other consumer | VERIFIED | grep across `config/` returns nothing | +| V3 | All phase-3 call-site line numbers | VERIFIED | `grep -n` across `.claude`, `.agents`, `AGENTS.md`, `README.md`, `docs/` | +| V4 | `defuddle` exports `./node`; `linkedom` is an optional dep | VERIFIED | `npm view defuddle exports optionalDependencies` | +| V5 | `opencode.json` allows `node *` and never allowed `go *` | VERIFIED | read directly | + +**Questions asked:** 5 across two rounds. All answered; no unresolved decisions. + +### Whole-Plan Consistency Sweep + +Run after propagating the decisions above to phases 1-5. + +- `NEWSLETTER_CONTENT_DIR` removed from every plan file — 0 remaining hits. +- `mt-add-post` / `mt-webfetch` appear only as *old* names in phase 3's rename + table and call-site inventory, which is correct. +- The design-decisions `.mjs` note in `plan.md` reconciled with phase 5's + existence (phase 1 carried no such note). +- Non-goals reconciled: `.github/scripts/` logic stays out of scope; its + extensions do not. +- Success criteria in `plan.md` and phases 3, 4, 5 agree on what `grep` must + return and when. +- Two contradictions found and fixed during the sweep: + 1. The dependency table listed `defuddle` as *replacing* the `defuddle.md` + proxy, contradicting the two-tier design decision that keeps both. + 2. `plan.md`'s `go run` criterion and phase 3's differed on whether `scripts/` + is included; both now state which phase they are evaluated after. +- **Unresolved contradictions: 0.** diff --git a/plans/reports/code-review-260918-newsletter-js-migration.md b/plans/reports/code-review-260918-newsletter-js-migration.md new file mode 100644 index 0000000..85e6380 --- /dev/null +++ b/plans/reports/code-review-260918-newsletter-js-migration.md @@ -0,0 +1,308 @@ +# Code Review — Newsletter engine Go → JavaScript migration (2026-09-18) + +Reviewer pass over the uncommitted change set on `main`. Read-only: no file in +the change set was modified. Verification was done by reading the JS against +`git show HEAD:scripts/newsletter/*.go`, running `npm run lint` / `npm test`, and +probing the engine directly. + +## Scope + +- `scripts/newsletter/` — 10 new `.js` modules + 2 test files + `__fixtures__/` + (~1,050 LOC), replacing 10 deleted `.go` files (1,168 LOC) and `go.mod` +- 6 `mt-*` skills restructured, `.agents/skills/` converted to symlinks +- `docs/newsletter/engine-commands.md` (new), `post-mechanics.md` (moved), + `AGENTS.md`, `README.md`, `docs/multi-tool-usage.md` +- `.github/scripts/*.mjs` → `.js`, `hugo.yml`, `update-projects.yml`, + `netlify.toml`, `.gitignore`, `package.json`, `eslint.config.mjs` + +## Overall assessment + +The port is careful and, on the evidence, faithful. All seven named traps are +genuinely handled — verified by reading, not by trusting the report. The golden +suite is real regression coverage (byte-for-byte stdout plus exit codes through +the actual CLI surface), not phantom tests. No scope drift: no invented +abstractions, no `any` widening, no catch-and-swallow beyond the deliberate +"return empty on network failure" contract the Go engine already had. + +`npm run lint` passes clean. `npm test` passes 33/33 (8 network cases skipped). + +One structural defect and three contract gaps are worth fixing before this is +committed. None is a security issue; the threat model here (a personal blog's +content CLI, run by the repo owner) does not warrant hardening beyond what is +present. + +## Critical + +None. + +## High + +### H1 — `index.js` runs the CLI as an import side effect + +`scripts/newsletter/index.js:95` calls `main()` unconditionally at module scope, +with no entry-point guard. The four command modules that emit JSON +(`add-url.js`, `detect-image-source.js`, `find-substack-post.js`, +`post-stats.js`) statically `import { printJson } from "./index.js"`. Importing +any of them therefore *runs the CLI* against the host process's `argv`. + +Reproduced: + +``` +$ node -e 'import("./scripts/newsletter/post-stats.js")' +Usage: node scripts/newsletter [args] +... +exit=1 +``` + +A one-line unit test for an exported pure function fails the same way: + +```js +import { countPostEntries } from ".../post-stats.js"; // → suite dies, exit 1 +``` + +Impact — this is the practical answer to "can you break `npm test`": yes, by +adding the most obvious next test. The port exports `detectYouTube`, +`extractInnerUrl`, `countPostEntries`, `parseLastmod` and `loadPublications` +specifically so they can be tested, and today none of them can be. The parity +suite only survives because it imports `html-text.js`, the one helper module with +no back-edge to `index.js`. Worse than the failure itself: if the host argv ever +begins with a valid command name, the import silently *executes that command* +instead of erroring. + +The circular import is known — the comment at `index.js:93` works around its +deadlock symptom ("command modules import printJson from this file, so a +top-level await here would deadlock") rather than removing the cycle. + +Fix (either one, both small): + +- move `printJson` into its own module (`output.js`) so no command module imports + the entry point; or +- guard the call: + `if (process.argv[1] && pathToFileURL(process.argv[1]).href === import.meta.url) main().catch(...)` + +## Medium + +### M1 — the local defuddle tier can consume the proxy tier's turn on junk output + +`fetch-via-defuddle.js:27,32` gates local success on exactly two conditions: +`res.ok`, and `body.trim() !== ""`. Anything else that comes back 200 with text — +a Cloudflare interstitial, a "Verifying you are human" page, a "Subscribe to keep +reading" paywall stub — extracts to a non-empty body, so `runFetchViaDefuddle` +returns at line 69 and **the `defuddle.md` proxy never runs**. + +That is the scenario the proxy was deliberately kept for. `mt-fetch-url` is +invoked only *after* WebFetch was already blocked, which strongly predicts that a +fetch from this same box is also blocked. The agent-level mitigation in +`mt-fetch-url/SKILL.md` ("If tier 1 ... returns a body with no usable content, +try a reader proxy") routes past the failure to `r.jina.ai` — skipping the +proxy stage entirely, because the engine already reported exit 0. + +Not re-litigating the two-tier decision (accepted, and the ordering is right). +The gap is the gate. A minimal quality check before returning local output — +e.g. fall through when `result.wordCount` is below a small threshold, or when the +body matches a short challenge/paywall shape — restores the intended chain. + +### M2 — the local tier's output shape breaks `mt-fetch-url`'s documented parsing contract + +The proxy returns YAML frontmatter plus body: + +``` +--- +title: "Example Domain" +site: "example.com" +source: "https://example.com/" +... +--- + +``` + +The local tier returns `String(result.content)` only — bare markdown, no +frontmatter, no title. Verified against `https://example.com/`. + +`mt-fetch-url/SKILL.md` still instructs the agent, for the tier the engine now +tries **first**: + +- step 4: "Parse the returned markdown (tier 1 has YAML frontmatter with + title/description/etc.)" +- Output handling: "The frontmatter gives you the page title for free — + preferred over parsing from HTML." +- Failure modes: "Only frontmatter with no body" — a proxy-shaped signal that + cannot occur on the local tier. + +So on the common path the agent is told to read a title that is not there. For a +skill whose output feeds an article heading in a published post, a missing title +is exactly the "wrong output lands in a post" failure mode worth catching. + +`Defuddle(...)` already returns `title`, `author`, `description`, `domain`, +`site`, `published` and `wordCount` (confirmed by inspecting the result object) — +all discarded at `fetch-via-defuddle.js:31`. Emitting an equivalent frontmatter +block from those fields makes both tiers shape-compatible and satisfies the +skill as written. Alternatively, correct the skill text. Aligning the engine is +the smaller change and keeps the skill's title guidance true. + +### M3 — output-contract changes not recorded in the parity report + +Acceptance criterion 3 asks for anything that changed without being called out. +Three items, all benign or improvements, none currently documented: + +1. **Sitemap `` entities are now decoded.** Go used + `([^<]+)` on raw text; the port uses `$(el).children("loc").text()`, + which decodes. Verified: `https://x.com/p/a?b=1&c=2` now yields + `https://x.com/p/a?b=1&c=2` where Go yielded the literal `&`. This changes + both the URL the deep crawl fetches and the `postUrl` emitted on a sitemap + hit. JS is correct per the sitemap spec; Go was fetching a broken URL. Worth + recording as an accepted improvement alongside the RSS `` note. + +2. **`itemLink` is not byte-identical to Go for numeric entities.** The report + states link values are "byte-identical to what Go produced". The cheerio + parse/serialize round trip normalizes `&` → `&`: + + ``` + https://x.com/p?a=1&b=2&c=3 + Go: …?a=1&b=2&c=3 JS: …?a=1&b=2&c=3 + ``` + + `&` and CDATA both round-trip exactly, so real Substack feeds are + unaffected. The claim just needs narrowing to "`&` and CDATA". + +3. **`cleanUrl` inherits WHATWG normalizations Go's `url.Parse` did not apply.** + Verified: + + | input | Go | JS | + |---|---|---| + | `https://x.com/a/../b?keep=1` | unchanged | `https://x.com/b?keep=1` | + | `https://x.com:443/p` | port kept | port stripped | + | `https://x.com/p?a=b c&d=1` | raw space kept | `a=b%20c` | + + The query-verbatim trap itself is correctly handled — the port rebuilds the + query by string surgery and never touches `URLSearchParams`, and + `%20`/`%7e`/valueless-key all survive (confirmed by the golden case and by + direct probe). These three are path/authority-level, not query-level, and are + implausible for real newsletter inputs. Record rather than change. + +## Low + +### L1 — `engine-commands.md` overstates the missing-dependency contract + +"Every subcommand prints `newsletter engine: dependencies missing …` and exits 1 +if that step was skipped." Verified against a copy of the engine outside +`node_modules` reach: + +- `find-substack-post` → correct message, exit 1 ✔ +- `fetch-via-defuddle` → `ERR_MODULE_NOT_FOUND` from the dynamic + `import("defuddle/node")` is caught by `runFetchViaDefuddle`'s own try block, + reported as "local defuddle failed", then the proxy runs and the command + **succeeds with exit 0** + +The runtime behaviour is better than the doc (graceful degradation). Fix the +sentence, not the code. + +### L2 — `\s` is wider in JS than in Go + +`NEWSLETTER_NUM_RE`, `TAGS_LINE_RE` and the three `post-stats` regexes use `\s`, +which in JavaScript matches NBSP and other Unicode spaces; Go's RE2 `\s` is +`[\t\n\f\r ]`. So `Newsletter #42` now parses where it previously did not. +An improvement, but it is a silent widening of a parsing contract and belongs in +the accepted-differences list. + +### L3 — one export without JSDoc + +`find-newsletter-number.js:9` — `export const NEWSLETTER_NUM_RE` is the only +exported symbol in the engine with no doc comment. Every other export +(30 of them) is annotated. Repo policy is JavaScript + JSDoc; this is the one +gap. + +### L4 — nothing in CI runs `npm test` or `npm run lint` + +`hugo.yml` runs `npm ci` (guarded, for the site build) and `update-projects.yml` +runs `npm ci` for the projects script. Neither runs the new suite, so the +regression coverage this migration just bought only exists on the developer's +machine. A job on `pull_request` / `push` running `npm ci && npm run lint && +npm test` is a few lines. Flagging as a recommendation, not a blocker — CI was +explicitly out of scope for this pass. + +## Verified correct (risk calibration) + +Checked directly rather than taken from the report, because the report is the +artifact under review: + +- **Seven traps.** Query rebuilt verbatim by string surgery, never + `URLSearchParams` (`url-utils.js:49-66`). `hasBoundaryMatch` is an index loop + with the same delimiter set and optional-trailing-slash rule as Go + (`url-utils.js:259-298`); `/p/foo` vs stored `/p/foo-bar` probed and correctly + *not* a duplicate. Optional fields are added conditionally, so `title`, + `author`, `uuid`, `innerUrl` are absent rather than `""`. `cutoff` initialises + to `null` and is always emitted. `[...text].length` counts code points. + `padStart(6)` reproduces `%6d`. `decodeURIComponent` is wrapped and falls back + to the raw substring. +- **JSON key order and types** match the Go struct field order one-for-one in all + four JSON-emitting commands, and the suite asserts stdout byte-for-byte. +- **Walk order.** `collectMarkdown` sorts each directory's entries lexically and + recurses in place, reproducing `filepath.WalkDir` ordering — which is what makes + the `list-existing-tags` tie-break deterministic. `Array.prototype.sort` is + stable (ES2019+), matching `sort.SliceStable`. +- **`captionForUuid`** is faithful despite the rewrite. Probed the divergence + candidates — uuid first appearing in a ``, in an earlier ``, or + on an ancestor of the figure — all return `""` in both engines. +- **`process.exit()` after a write** is safe here: on Linux, pipe writes from + `process.stdout`/`stderr` are synchronous, so no diagnostic is truncated. +- **Call sites.** Every command named in `.claude/skills/`, `docs/`, `AGENTS.md` + and `README.md` exists with the arguments it is given. `grep "go run"` across + the repo is empty. All six `.agents/skills/*/SKILL.md` symlinks resolve. + `mt-add-post` / `mt-webfetch` survive only inside `plans/`, which is correct. +- **Go retirement is complete.** No `.go`, no `go.mod`/`go.sum`, no `GO_VERSION`, + no `setup-go`. `eslint.config.mjs` is the only remaining `.mjs`. + `.github/scripts` internal imports were updated to `.js`. + +## Carried over from Go, not a regression + +`urlBoundaryOk` treats `_` as a delimiter, so `https://x.com/p/foo` is reported +as a duplicate of a stored `https://x.com/p/foo_bar` (probed: `true`). The `_` +is there to let `_WxH` match, and the behaviour is byte-identical to the Go +original, so it is out of scope for this migration. Recording it because the +consequence — a URL silently dropped from a newsletter as a false duplicate — is +the kind of failure that is hard to notice after the fact. + +## Recommended actions + +1. **H1** — remove the import cycle (move `printJson` out of `index.js`) or guard + `main()`. Then add unit tests for the pure exports that cannot be tested today. +2. **M2** — emit frontmatter from the local tier's `title`/`author`/`site`/ + `description`, or correct `mt-fetch-url/SKILL.md`'s output-handling section. +3. **M1** — add a minimal quality gate before the local tier returns, so a + 200-with-challenge-page does not short-circuit the proxy. +4. **M3** — add the three items to the parity report's accepted differences and + narrow the `itemLink` "byte-identical" claim. +5. **L1** — correct the missing-dependency sentence in `engine-commands.md`. +6. **L2, L3, L4** — record the `\s` widening; add JSDoc to `NEWSLETTER_NUM_RE`; + consider a CI job running lint + test. + +## Plan status + +All five phases in +`plans/260918-0647-migrate-newsletter-engine-to-javascript/plan.md` are marked +Done and every success-criteria checkbox is ticked. Spot-checked against the +repo, each ticked criterion holds. Two carry caveats worth noting before the plan +is closed: the "`npm test` passes from a clean `npm ci`" criterion is true but +fragile per H1, and "one real `mt-add-url` flow completes end-to-end" was not +re-verified in this pass. Plan file not modified — plan mutation belongs to the +lead. + +## Metrics + +- Type coverage: JSDoc on 30 of 31 exported symbols (L3); no TypeScript, per repo policy +- Test: 33 deterministic assertions pass offline in 2.3s; 8 live cases opt-in +- Lint: 0 issues (`eslint .`) +- Build: n/a (no build step); Go toolchain fully removed from CI and Netlify + +## Unresolved questions + +1. M2 — align the engine's local tier to the proxy's frontmatter shape, or + rewrite the skill's output-handling section? The engine change is smaller and + keeps the skill's title guidance true, but it is a (small) output-contract + change on a command the user has already accepted as changed. +2. M1 — is a word-count/shape heuristic wanted at all, or is the agent-level + "no usable content" judgement in `mt-fetch-url` considered sufficient? That + judgement currently routes past the proxy rather than into it. +3. L4 — is CI for lint/test in scope, or deliberately deferred with the rest of + the unexercised CI work? diff --git a/plans/reports/parity-260918-newsletter-js-migration-report.md b/plans/reports/parity-260918-newsletter-js-migration-report.md new file mode 100644 index 0000000..1fdb7c9 --- /dev/null +++ b/plans/reports/parity-260918-newsletter-js-migration-report.md @@ -0,0 +1,174 @@ +# Parity Report — Newsletter Engine Go → JavaScript (2026-09-18) + +The Go engine's stdout was captured for 26 deterministic cases before the +JavaScript port was trusted, committed under +`scripts/newsletter/__fixtures__/golden/`, and asserted byte-for-byte by +`scripts/newsletter/parity.test.js`. Live-upstream behaviour is covered by +`network.test.js`, which is opt-in. + +## Results + +`npm test` — 40 assertions, 40 pass, 0 fail, offline. +`NEWSLETTER_NET=1 npm test` — 48 assertions, 48 pass, 0 fail. + +| Subcommand | Deterministic cases | Live cases | Result | +|---|---|---|---| +| `add-url` | 15 | 4 | pass | +| `detect-image-source` | 4 | — | pass | +| `find-newsletter-number` | 3 | — | pass | +| `list-existing-tags` | 1 | — | pass | +| `post-stats` | 3 | — | pass | +| `find-substack-post` | 3 (extraction, saved markup) | 3 | pass | +| `fetch-via-defuddle` | 1 (exit 2) | 2 | pass | + +Content-scanning commands are pointed at `__fixtures__/repo` by setting the +subprocess working directory. Both engines resolve `content/post` from the +process CWD, so neither gained a test-only configuration knob. Verified by +renaming the real `content/post` away and re-running: 32/32 still pass. + +`add-url` performs a HEAD request on every URL, so its deterministic cases use +`.invalid` hosts (RFC 2606, guaranteed not to resolve). The status is a stable +`"000"` whether or not the machine is online. + +## Trap coverage + +Each of the seven traps identified in phase 1 has a case that goes red when the +naive implementation is reintroduced. Verified by mutating the engine and +re-running: + +| Trap | Discriminating case | Reintroducing the trap | +|---|---|---| +| Query kept verbatim | `add-url-query-verbatim` (`%20`, valueless key, lowercase `%7e`) | 1 failure | +| Boundary-aware dedup | `add-url-prefix-not-duplicate` | 3 failures | +| Empty optional fields omitted | `add-url-article-no-metadata`, `detect-image-non-substack` | 3 failures | +| `cutoff` is `null`, not omitted | live deep-miss case | see below | +| Code points, not UTF-16 units | the 70-code-point bullet | 5 failures | +| Six-column right-aligned counts | `list-existing-tags-main` | 3 failures | +| Malformed percent escape | `detect-image-malformed-percent` | 3 failures | + +The first discriminating case chosen for the query trap did not discriminate: +`%7E` and `+` survive a `URLSearchParams` round trip unchanged. The case was +replaced with one that does — `%20` becomes `+`, a valueless key gains `=`, and +lowercase `%7e` is upper-cased by the re-encoder. + +`cutoff` is exercised live only. The publication list is deliberately not +test-configurable (the "no test-only knob" decision), so the branch that emits +`"cutoff": null` cannot be reached offline. A live `--deep` miss was diffed +against the Go engine directly and came back byte-identical. + +## Accepted differences + +**The parser recovers a TOC bullet the regex dropped.** On the live ByteByteGo +feed, `extractCandidates` returns one more candidate than the Go regex did — the +first bullet of the "in this issue" list, +`"An LLM needs evidence to answer questions"`. The markup is +`

  • …

    • `, identical in shape to the bullets the regex did +match, so this is the regex losing an item, not the parser inventing one. Every +candidate the regex found is still present and in the same order; the suite +asserts that explicitly rather than pinning the full list. + +Accepted: more complete candidate lists, nothing lost. This is the R3 case the +plan anticipated. + +**`fetch-via-defuddle` exit 2 is now observable.** The Go source defined exit 2 +for bad arguments and exit 1 for a failed fetch, but `go run` collapsed every +nonzero exit to 1. Node preserves them, so the distinction is real for the first +time. No caller depended on the collapsed behaviour. + +**`fetch-via-defuddle` gained a local extraction tier.** Local defuddle runs +first, the `defuddle.md` proxy remains as the second tier. The proxy is the tier +that matters when this machine's IP is the one being blocked, so it was not +dropped. Verified on ARM64: `defuddle/node` + `linkedom` import and extract +without native dependencies. + +**`--uuid` no longer accepts the single-dash form.** Go's `flag` package accepted +`-uuid`; `node:util`'s `parseArgs` accepts only `--uuid`. Every documented call +site uses the double-dash form. Accepted narrowing. + +**RSS `` entity handling is unchanged, deliberately.** cheerio's `.text()` +decodes entities, which would have turned a stored `&` into `&`. The port +reads the element's raw serialized inner content instead. `&` and CDATA +round-trip exactly; a *numeric* entity is normalized by the parser's +re-serialization (`&` → `&`), which no observed Substack feed emits. + +**Sitemap `` entities are now decoded.** Go's `([^<]+)` kept +`&` literal, so a sitemap entry with an escaped ampersand produced a URL the +crawler then failed to fetch and a broken `postUrl` on a hit. `children("loc").text()` +decodes it. XML says the decoded form is the URL; accepted as a fix. + +**`cleanUrl` inherits WHATWG normalizations Go's `url.Parse` did not apply** — +dot-segment removal (`/a/../b` → `/b`), default-port stripping (`:443`), and a +raw space becoming `%20`. These are path- and authority-level; the query is still +rebuilt by string surgery and never re-encoded, which is what the stored +`clean_url` values depend on. No observed input in `content/post` is affected. + +**JavaScript `\s` matches more than RE2's `\s`.** `NEWSLETTER_NUM_RE`, +`TAGS_LINE_RE` and the `post-stats` patterns now also match a non-breaking space +where Go did not. A silent widening, and in this content the forgiving direction: +`Newsletter #132` typed with an NBSP would be found rather than missed. + +## Carried over from Go, not introduced here + +`urlBoundaryOk` treats `_` as a delimiter, so a stored +`https://x.invalid/p/foo_bar` makes `https://x.invalid/p/foo` report as a +duplicate. Byte-identical to the Go behaviour and therefore out of this +migration's scope, recorded because the consequence — a URL silently dropped as a +false duplicate — is hard to notice after the fact. + +## Found during review, fixed + +**Piping any command into a reader that closes early crashed.** +`node scripts/newsletter list-existing-tags | head -3` printed an EPIPE stack +trace; Go took SIGPIPE and died silently. `index.js` now exits quietly on EPIPE, +with a regression test. + +**Importing a command module ran the CLI.** The four JSON-emitting commands +imported `printJson` from `index.js`, which calls `main()` at module scope — so +importing any of them executed the dispatcher against the host process's argv, +and the pure functions the port exports for testing could not be imported at all. +`printJson` moved to `json-out.js`, breaking the cycle. Six unit tests now cover +`detectYouTube`, `extractInnerUrl`, `countPostEntries`, `parseLastmod`, +`loadPublications` and `findMostRecentNewsletter`, which is the standing proof +the cycle stays gone. + +**The local defuddle tier could consume the proxy tier's turn.** A bot wall +answers 200 with a real body, which extracted to non-empty and returned exit 0 — +so the `defuddle.md` proxy, the tier that exists precisely because this machine's +IP is blocked, never ran. A challenge-shape check on the extraction title and the +opening of the body now counts that as a local failure. Marker-based rather than +length-based on purpose: a legitimate short page (example.com is 17 words) must +still succeed. + +**The two tiers returned different shapes.** The proxy returns YAML frontmatter +plus body; the local tier returned a bare body, while the skill told the agent +tier 1 carries a title in frontmatter — for a value that becomes an article +heading in a published post. The local tier now emits the same frontmatter +(title, author, description, site, published, source) from defuddle's metadata. + +## Fixtures + +Reviewed by eye before commit. No fixture was found to encode a Go bug. + +- `__fixtures__/repo/` — six posts covering every shape a URL is stored in + (bare, trailing slash, markdown autolink, markdown link, image), a newsletter + in an older year holding a *higher* number than the newest year, and the three + `post-stats` shapes. +- `__fixtures__/repo-empty/`, `__fixtures__/repo-year-fallback/` — the empty tree + and the newest-year-has-none cases for `find-newsletter-number`. +- `__fixtures__/markup/bytebytego-feed.xml` — two real feed items, 64 KB. +- `__fixtures__/markup/bytebytego-sitemap.xml` — 40 real sitemap entries, 6 KB. +- `__fixtures__/markup/edge-cases.html` — synthetic: a nested `
    `, a + caption-less figure, and bullets at exactly 5, 6, 70 and 71 code points with a + non-BMP character. + +The live feed carries no `
    ` at all, which is why the engine's own +comment says ByteByteGo does not caption its images. Caption extraction is +therefore covered by the synthetic nested-figure fixture, where the innermost +enclosing figure must supply the caption and a caption-less figure must yield +nothing rather than borrowing the outer one. + +## Verdict + +Parity proven. The JavaScript engine reproduces the Go engine on every captured +case, and every difference is deliberate, recorded above, and an improvement or +an accepted narrowing. Safe to cut the call sites over. diff --git a/plans/reports/research-260918-1139-script-language-choice.md b/plans/reports/research-260918-1139-script-language-choice.md index 0ee1c7d..875728c 100644 --- a/plans/reports/research-260918-1139-script-language-choice.md +++ b/plans/reports/research-260918-1139-script-language-choice.md @@ -120,6 +120,17 @@ engine does not call the GitHub API. 1. **`scripts/newsletter/` stays Go.** No GitHub API, no deps, no latency problem, three agent runtimes served identically. Preserve the no-`go.sum` invariant. + + > **Superseded 2026-09-18.** The user reviewed this recommendation the same + > day and chose to migrate the engine to JavaScript anyway, to collapse the + > repo onto one language and replace the hand-rolled HTML/XML regexes with a + > real parser. Carried out by + > `plans/260918-0647-migrate-newsletter-engine-to-javascript/`; parity proven + > in `plans/reports/parity-260918-newsletter-js-migration-report.md`. The + > recommendation above is left as written — it records what the evidence + > supported at the time, and its strongest objection (a bare clone no longer + > runs the engine without `npm ci`) is carried forward as risk R1 in that + > plan. 2. **`.github/scripts/` stays JavaScript; migrate to `octokit`.** Add a root `package.json` (+ lockfile) with `octokit` as the single dependency, plus ESLint and JSDoc per repo policy. Expected outcome: `lib/github-api.mjs` @@ -133,6 +144,10 @@ engine does not call the GitHub API. ### Fix first (latent, found while scouting) +> **Resolved 2026-09-18 by removal.** Go was retired from the repo along with the +> engine, so there are no pins left to align. + + `go.mod` declares `go 1.26`, but `hugo.yml` pins `GO_VERSION: 1.25.5` and `netlify.toml` sets `GO_VERSION = "1.25.5"`. Running any `go` command in those environments only succeeds because `GOTOOLCHAIN=auto` silently downloads 1.26.