From b98d4b3d0bbbe86745711e5b5cbc22a6f21d94be Mon Sep 17 00:00:00 2001 From: Ivan Malison Date: Thu, 26 Mar 2026 21:29:24 -0700 Subject: [PATCH] docs(skills): add nixpkgs review guidance --- .../references/ignore-paths.md | 3 + .../agents/skills/nixpkgs-review/SKILL.md | 77 +++++++ .../skills/nixpkgs-review/agents/openai.yaml | 4 + .../references/review-patterns.md | 105 +++++++++ .../skills/nixpkgs-review/scripts/.gitignore | 2 + .../scripts/mine_pr_feedback.py | 208 ++++++++++++++++++ 6 files changed, 399 insertions(+) create mode 100644 dotfiles/agents/skills/nixpkgs-review/SKILL.md create mode 100644 dotfiles/agents/skills/nixpkgs-review/agents/openai.yaml create mode 100644 dotfiles/agents/skills/nixpkgs-review/references/review-patterns.md create mode 100644 dotfiles/agents/skills/nixpkgs-review/scripts/.gitignore create mode 100755 dotfiles/agents/skills/nixpkgs-review/scripts/mine_pr_feedback.py diff --git a/dotfiles/agents/skills/disk-space-cleanup/references/ignore-paths.md b/dotfiles/agents/skills/disk-space-cleanup/references/ignore-paths.md index 4170cbf5..e55dd730 100644 --- a/dotfiles/agents/skills/disk-space-cleanup/references/ignore-paths.md +++ b/dotfiles/agents/skills/disk-space-cleanup/references/ignore-paths.md @@ -5,6 +5,9 @@ Use this file to track mountpoints or directories that should be excluded from ` ## Known Ignores - `$HOME/keybase` +- `$HOME/.cache/keybase` +- `$HOME/.local/share/keybase` +- `$HOME/.config/keybase` - `/keybase` - `/var/lib/railbird` - `/run/user/*/doc` (FUSE portal mount; machine-specific example observed: `/run/user/1004/doc`) diff --git a/dotfiles/agents/skills/nixpkgs-review/SKILL.md b/dotfiles/agents/skills/nixpkgs-review/SKILL.md new file mode 100644 index 00000000..d2235429 --- /dev/null +++ b/dotfiles/agents/skills/nixpkgs-review/SKILL.md @@ -0,0 +1,77 @@ +--- +name: nixpkgs-review +description: Review or prepare nixpkgs package changes and PRs using a checklist distilled from review feedback on Ivan Malison's own NixOS/nixpkgs pull requests. Use when working in nixpkgs on package inits, updates, packaging fixes, or before opening or reviewing a nixpkgs PR. +--- + +# Nixpkgs Review + +Use this skill when the task is specifically about reviewing or tightening a change in `NixOS/nixpkgs`. + +The goal is not generic style review. The goal is to catch the kinds of issues that repeatedly came up in real nixpkgs feedback on Ivan's PRs: derivation structure, builder choice, metadata, PR hygiene, and JS packaging details. + +## Workflow + +1. Read the scope first. + Open the changed `package.nix` files, related metadata, and the PR title/body if there is one. + +2. Run the historical checklist below. + Bias toward concrete review findings and actionable edits, not abstract style commentary. + +3. Validate the package path. + Use the narrowest reasonable validation for the task: targeted build, package eval, or `nixpkgs-review` when appropriate. + +4. If you are writing a review: + Lead with findings ordered by severity, include file references, and tie each point to a nixpkgs expectation. + +5. If you are preparing a PR: + Fix the checklist items before opening it, then confirm title/body/commit hygiene. + +## Historical Checklist + +### Derivation structure + +- Prefer `finalAttrs` over `rec` for derivations and nested derivations when self-references matter. +- Prefer `tag = "v${...}"` over `rev` when fetching a tagged upstream release. +- Check whether `strictDeps = true;` should be enabled. +- Use the narrowest builder/stdenv that matches the package. If no compiler is needed, consider `stdenvNoCC`. +- Put source modifications in `postPatch` or another appropriate hook, not inside `buildPhase`. +- Prefer `makeBinaryWrapper` over `makeWrapper` when a compiled wrapper is sufficient. +- Keep wrappers aligned with `meta.mainProgram` so overrides remain clean. +- Avoid `with lib;` in package expressions; prefer explicit `lib.*` references. + +### Metadata and platform expectations + +- For new packages, ensure maintainers are present and include the submitter when appropriate. +- Check whether platform restrictions are justified. Do not mark packages Linux-only or broken without evidence. +- If a package is only workable through patch accumulation and has no maintainer, call that out directly. + +### JS, Bun, Electron, and wrapper-heavy packages + +- Separate runtime deps from build-only deps. Large closures attract review attention. +- Remove redundant env vars and duplicated configuration if build hooks already cover them. +- Check bundled tool/runtime version alignment, especially browser/runtime pairs. +- Install completions, desktop files, or icons when upstream clearly ships them and the package already exposes the feature. +- Be careful with wrappers that hardcode env vars users may want to override. + +### PR hygiene + +- PR title should match nixpkgs naming and the package version. +- Keep the PR template intact unless there is a strong reason not to. +- Avoid unrelated commits in the PR branch. +- Watch for duplicate or overlapping PRs before investing in deeper review. +- If asked, squash fixup history before merge. + +## Review Output + +When producing a review, prefer this shape: + +- Finding: what is wrong or risky. +- Why it matters in nixpkgs terms. +- Concrete fix, ideally with the exact attr/hook/builder to use. + +If there are no findings, say so explicitly and mention remaining validation gaps. + +## References + +- Read [references/review-patterns.md](references/review-patterns.md) for the curated list of recurring review themes and concrete PR examples. +- Run `scripts/mine_pr_feedback.py --repo NixOS/nixpkgs --author colonelpanic8 --limit 20 --format markdown` to refresh the source material from newer PRs. diff --git a/dotfiles/agents/skills/nixpkgs-review/agents/openai.yaml b/dotfiles/agents/skills/nixpkgs-review/agents/openai.yaml new file mode 100644 index 00000000..a09d3d9b --- /dev/null +++ b/dotfiles/agents/skills/nixpkgs-review/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Nixpkgs Review" + short_description: "Review nixpkgs changes with historical guidance" + default_prompt: "Use $nixpkgs-review to review this nixpkgs package change before I open the PR." diff --git a/dotfiles/agents/skills/nixpkgs-review/references/review-patterns.md b/dotfiles/agents/skills/nixpkgs-review/references/review-patterns.md new file mode 100644 index 00000000..06a0f22e --- /dev/null +++ b/dotfiles/agents/skills/nixpkgs-review/references/review-patterns.md @@ -0,0 +1,105 @@ +# Nixpkgs Review Patterns + +This reference is a curated summary of recurring feedback from Ivan Malison's `NixOS/nixpkgs` PRs. Use it to ground reviews in patterns that have already come up from nixpkgs reviewers. + +## Most Repeated Themes + +### 1. Prefer `finalAttrs` over `rec` + +This came up repeatedly on both package init and update PRs. + +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) `playwright-cli`: reviewer asked for `buildNpmPackage (finalAttrs: { ... })` instead of `rec`. +- [PR #490033](https://github.com/NixOS/nixpkgs/pull/490033) `rumno`: same feedback for `rustPlatform.buildRustPackage`. + +Practical rule: +- If the derivation self-references `version`, `src`, `pname`, `meta.mainProgram`, or nested outputs, default to `finalAttrs`. + +### 2. Prefer `tag` when upstream release is a tag + +This also repeated across multiple PRs. + +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) `playwright-cli` +- [PR #490033](https://github.com/NixOS/nixpkgs/pull/490033) `rumno` +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code` + +Practical rule: +- If upstream publishes a named release tag, prefer `tag = "v${finalAttrs.version}";` or the exact tag format instead of a raw `rev`. + +### 3. Use the right hook and builder + +Reviewers often push on hook placement and builder/stdenv choice. + +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code`: feedback to move work from `buildPhase` into `postPatch`. +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code`: feedback to consider `stdenvNoCC`. +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) `playwright-cli`: prefer `makeBinaryWrapper` for a simple wrapper. + +Practical rule: +- Check whether each mutation belongs in `postPatch`, `preConfigure`, `buildPhase`, or `installPhase`. +- Check whether the package genuinely needs a compiler toolchain. +- For simple env/arg wrappers, prefer `makeBinaryWrapper`. + +### 4. Enable `strictDeps` unless there is a reason not to + +This was called out explicitly on [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465). + +Practical rule: +- For new derivations, ask whether `strictDeps = true;` should be present. +- If not, be ready to justify why the builder or package layout makes it unnecessary. + +### 5. Keep metadata explicit and override-friendly + +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) `playwright-cli`: reviewer asked to avoid `with lib;`. +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code`: reviewer suggested deriving wrapper executable name from `finalAttrs.meta.mainProgram`. + +Practical rule: +- Prefer `lib.licenses.mit` over `with lib;`. +- Keep `meta.mainProgram` authoritative and have wrappers follow it when practical. + +### 6. Maintainers matter for new packages + +- [PR #496806](https://github.com/NixOS/nixpkgs/pull/496806) `gws`: reviewer would not merge until the submitter appeared in maintainers. + +Practical rule: +- For package inits, check maintainers early rather than waiting for review feedback. + +### 7. PR title and template hygiene are review targets + +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code`: asked to fix the PR title to match the version. +- [PR #490033](https://github.com/NixOS/nixpkgs/pull/490033) `rumno`: reviewer asked what happened to the PR template. + +Practical rule: +- Before opening or updating a PR, verify the title, template, and branch scope. + +### 8. Duplicate or overlapping PRs get noticed quickly + +- [PR #490227](https://github.com/NixOS/nixpkgs/pull/490227) was replaced by [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230). +- [PR #490053](https://github.com/NixOS/nixpkgs/pull/490053) overlapped with [PR #490033](https://github.com/NixOS/nixpkgs/pull/490033). +- [PR #488606](https://github.com/NixOS/nixpkgs/pull/488606), [PR #488602](https://github.com/NixOS/nixpkgs/pull/488602), and [PR #488603](https://github.com/NixOS/nixpkgs/pull/488603) were closed after reviewers pointed to existing work. + +Practical rule: +- Search for existing PRs on the package before spending time polishing a review. +- If a branch contains unrelated commits, fix that before asking for review. + +### 9. JS/Bun/Electron packages draw runtime-layout scrutiny + +This came up heavily on `t3code` and `playwright-cli`. + +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) `t3code`: reviewers proposed trimming the runtime closure, removing unnecessary env vars, and adding shell completions and desktop integration. +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) `playwright-cli`: reviewers called out mismatched bundled `playwright-core` and browser binaries, and wrapper behavior that prevented user overrides. + +Practical rule: +- For JS-heavy packages, inspect closure size, runtime vs build-only deps, wrapper env vars, and version alignment between bundled libraries and external binaries. + +### 10. Cross-platform evidence helps + +- [PR #490230](https://github.com/NixOS/nixpkgs/pull/490230) received an approval explicitly noting Darwin success. +- [PR #497465](https://github.com/NixOS/nixpkgs/pull/497465) got feedback questioning platform restrictions and build behavior. + +Practical rule: +- If the package plausibly supports Darwin, avoid premature Linux-only restrictions and mention what was or was not tested. + +## How To Use This Reference + +- Use these patterns as a focused checklist before submitting or reviewing nixpkgs changes. +- Do not blindly apply every point. Check whether the builder, language ecosystem, and upstream release model actually match. +- When in doubt, prefer concrete evidence from the current package diff over generic convention. diff --git a/dotfiles/agents/skills/nixpkgs-review/scripts/.gitignore b/dotfiles/agents/skills/nixpkgs-review/scripts/.gitignore new file mode 100644 index 00000000..7a60b85e --- /dev/null +++ b/dotfiles/agents/skills/nixpkgs-review/scripts/.gitignore @@ -0,0 +1,2 @@ +__pycache__/ +*.pyc diff --git a/dotfiles/agents/skills/nixpkgs-review/scripts/mine_pr_feedback.py b/dotfiles/agents/skills/nixpkgs-review/scripts/mine_pr_feedback.py new file mode 100755 index 00000000..74a7fb29 --- /dev/null +++ b/dotfiles/agents/skills/nixpkgs-review/scripts/mine_pr_feedback.py @@ -0,0 +1,208 @@ +#!/usr/bin/env python3 +""" +Mine external feedback from recent GitHub PRs. + +Examples: + python scripts/mine_pr_feedback.py --repo NixOS/nixpkgs --author colonelpanic8 + python scripts/mine_pr_feedback.py --repo NixOS/nixpkgs --author colonelpanic8 --limit 30 --format json +""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys +from collections import Counter +from concurrent.futures import ThreadPoolExecutor, as_completed + + +def run(cmd: list[str]) -> str: + proc = subprocess.run(cmd, capture_output=True, text=True) + if proc.returncode != 0: + raise RuntimeError(proc.stderr.strip() or f"command failed: {' '.join(cmd)}") + return proc.stdout + + +def gh_json(args: list[str]) -> object: + return json.loads(run(["gh", *args])) + + +def fetch_prs(repo: str, author: str, limit: int) -> list[dict]: + prs: dict[int, dict] = {} + for state in ("open", "closed"): + data = gh_json( + [ + "search", + "prs", + "--repo", + repo, + "--author", + author, + "--limit", + str(max(limit, 30)), + "--state", + state, + "--json", + "number,title,state,closedAt,updatedAt,url", + ] + ) + for pr in data: + prs[pr["number"]] = pr + return sorted( + prs.values(), + key=lambda pr: (pr["updatedAt"], pr["number"]), + reverse=True, + )[:limit] + + +def fetch_feedback(repo: str, author: str, pr: dict) -> dict: + owner, name = repo.split("/", 1) + number = pr["number"] + + def api(path: str) -> list[dict]: + return gh_json(["api", f"repos/{owner}/{name}/{path}", "--paginate"]) + + issue_comments = api(f"issues/{number}/comments") + review_comments = api(f"pulls/{number}/comments") + reviews = api(f"pulls/{number}/reviews") + + comments = [] + for comment in issue_comments: + login = comment["user"]["login"] + body = (comment.get("body") or "").strip() + if login != author and body: + comments.append({"kind": "issue", "user": login, "body": body}) + for comment in review_comments: + login = comment["user"]["login"] + body = (comment.get("body") or "").strip() + if login != author and body: + comments.append( + { + "kind": "review_comment", + "user": login, + "body": body, + "path": comment.get("path"), + "line": comment.get("line"), + } + ) + for review in reviews: + login = review["user"]["login"] + body = (review.get("body") or "").strip() + if login != author and body: + comments.append( + { + "kind": "review", + "user": login, + "body": body, + "state": review.get("state"), + } + ) + + return {**pr, "comments": comments} + + +def is_bot(login: str) -> bool: + return login.endswith("[bot]") or login in {"github-actions", "app/dependabot"} + + +def render_markdown(results: list[dict], include_bots: bool) -> str: + commenters = Counter() + kept = [] + for pr in results: + comments = [ + comment + for comment in pr["comments"] + if include_bots or not is_bot(comment["user"]) + ] + if comments: + kept.append({**pr, "comments": comments}) + commenters.update(comment["user"] for comment in comments) + + lines = [ + "# PR Feedback Summary", + "", + f"- PRs scanned: {len(results)}", + f"- PRs with external feedback: {len(kept)}", + "", + "## Top commenters", + "", + ] + for user, count in commenters.most_common(10): + lines.append(f"- `{user}`: {count}") + + for pr in kept: + lines.extend( + [ + "", + f"## PR #{pr['number']}: {pr['title']}", + "", + f"- URL: {pr['url']}", + f"- State: {pr['state']}", + "", + ] + ) + for comment in pr["comments"]: + body = comment["body"].replace("\r", " ").replace("\n", " ").strip() + snippet = body[:280] + ("..." if len(body) > 280 else "") + lines.append(f"- `{comment['user']}` `{comment['kind']}`: {snippet}") + + return "\n".join(lines) + "\n" + + +def main() -> int: + parser = argparse.ArgumentParser(description="Collect review feedback from recent GitHub PRs.") + parser.add_argument("--repo", required=True, help="GitHub repo in owner/name form") + parser.add_argument("--author", required=True, help="PR author to inspect") + parser.add_argument("--limit", type=int, default=20, help="How many recent PRs to inspect") + parser.add_argument( + "--format", + choices=("markdown", "json"), + default="markdown", + help="Output format", + ) + parser.add_argument( + "--include-bots", + action="store_true", + help="Keep bot comments in the output", + ) + parser.add_argument( + "--workers", + type=int, + default=6, + help="Maximum concurrent GitHub API workers", + ) + args = parser.parse_args() + + try: + run(["gh", "auth", "status"]) + except RuntimeError as err: + print(err, file=sys.stderr) + return 1 + + prs = fetch_prs(args.repo, args.author, args.limit) + + results = [] + with ThreadPoolExecutor(max_workers=args.workers) as pool: + futures = [pool.submit(fetch_feedback, args.repo, args.author, pr) for pr in prs] + for future in as_completed(futures): + results.append(future.result()) + + results.sort(key=lambda pr: (pr["updatedAt"], pr["number"]), reverse=True) + + if args.format == "json": + if not args.include_bots: + for pr in results: + pr["comments"] = [ + comment for comment in pr["comments"] if not is_bot(comment["user"]) + ] + json.dump(results, sys.stdout, indent=2) + sys.stdout.write("\n") + else: + sys.stdout.write(render_markdown(results, args.include_bots)) + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())