Skip to content

feat(skills): add pocketjs-review-pr community review skill - #194

Merged
doodlewind merged 1 commit into
mainfrom
feat/review-pr-skill
Jul 27, 2026
Merged

feat(skills): add pocketjs-review-pr community review skill#194
doodlewind merged 1 commit into
mainfrom
feat/review-pr-skill

Conversation

@doodlewind

Copy link
Copy Markdown
Collaborator

Why

Reviewing #189 produced a method worth keeping: the PR's own description was a hypothesis, the real findings came from reproducing the bug on main across a wider input matrix than the PR tested, checking its load-bearing claims against node_modules/ source, and asking a fixed set of drift/rot questions. Two of those questions found real issues (a hand-maintained cache-key list that reintroduced the very rot it patched, and an anchored regex that only handled the single-comment shape), which then shipped as a commit on the contributor's branch rather than a round-trip.

What

skills/pocketjs-review-pr/SKILL.md — a ten-step workflow plus gotchas, carrying the repo-specific facts that make the difference between a review and a rubber stamp:

  • There is no test CI here (.github/workflows/ is deploy + esp32p4 + release), so gh pr checks reporting "no checks reported" is normal and the local run is the gate. Bare bun test is not that gate — it pulls in emulator/hardware harnesses; bun run test is.
  • Getting a fork's branch without git checkout main (held by ~/code/pocketjs), and bun tools/build.ts hero first so tests can resolve framework/src/styles.generated.ts.
  • The base-vs-branch probe pattern (git show main:<file> > <file>.base.ts, absolute imports from the scratchpad, one line per case).
  • The drift/rot checklist: derived sets over hand-kept lists (the chore(npm): pin the framework files map to its governed surface #174 governed-surface idiom), cache keys that omit implementation inputs, comments that claim more generality than the surrounding code delivers, and modules that no longer have an importer while docs still name them.
  • Proving a cache-key change empirically instead of asserting it, and pushing to maintainerCanModify forks.

Validation

Markdown only — no code paths touched. Frontmatter matches the four existing skills, length (159 lines) sits inside their range, and bun test tests/npm-package.test.ts tests/release-check.test.ts passes (7/7): skills/ is not part of the npm files surface, so the packaging governance is unaffected.

🤖 Generated with Claude Code

Distills the #189 review into a repeatable method: reproduce the bug on base
over a widened input matrix, check the PR's load-bearing claims against the
dependency source in node_modules, sweep for drift and rot (hand-maintained
lists, incomplete cache keys, comments that overclaim, unreachable modules),
run `bun run test` + tsc as the gate this repo has no CI for, push the findings
onto the contributor's branch, and squash-merge behind a review comment that
records the evidence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doodlewind
doodlewind marked this pull request as ready for review July 27, 2026 18:15
@doodlewind
doodlewind merged commit f7d116d into main Jul 27, 2026
@doodlewind
doodlewind deleted the feat/review-pr-skill branch July 27, 2026 18:15
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