feat(rules): add code comment placement rule - #190
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Skills dependency now uses version ChangesSkills CLI workflow
Comment guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 118: Quote the SKILLS_CLI expansion at every command site in the
Makefile, including the add command and the commands around lines 141 and 161,
so paths containing spaces execute correctly and printed commands remain
copy-safe.
- Around line 92-95: Remove the --minimum-release-age 0 override from both bun
install --frozen-lockfile commands in the Makefile, including the commands near
the skills SDK installation and the other installation flow, so Bun’s configured
minimumReleaseAge protection remains active.
In `@package.json`:
- Line 20: Declare the required Node.js runtime for the skills dependency by
adding a repository runtime marker such as .node-version set to 22.20.0, or
documenting Node.js 22.20.0+ in the README. Ensure the contract clearly applies
to supported checkouts using skills@1.5.22.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f13810d-d209-40da-bd7c-fb8a1a468eb9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
MakefileUPGRADE_LOG.mdpackage.jsonrules/code-comments.md
| if ! (cd "$(SKILLS_PROJECT_DIR)" && bun install --frozen-lockfile --minimum-release-age 0 --no-progress >/dev/null); then \ | ||
| echo "Error: failed to install the skills SDK from bun.lock"; \ | ||
| exit 1; \ | ||
| fi; \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
command -v bun
bun --version
bun install --help | rg -n -- '--minimum-release-age|--frozen-lockfile'
rg -n --hidden --glob 'Makefile' --glob 'bunfig.toml' \
'minimumReleaseAge|--minimum-release-age|--frozen-lockfile' . || trueRepository: shunkakinoki/dotagents
Length of output: 160
🌐 Web query:
Bun publish minimumReleaseAge lockfile minimum-release-age bunfig.toml
💡 Result:
The minimumReleaseAge setting is a feature for bun install designed to prevent the installation of recently published packages (to protect against supply chain attacks) [1][2]. It is not a feature related to bun publish [3][4]. For bun install, minimumReleaseAge can be configured in your bunfig.toml file under the [install] section [1][2]. Configuration in bunfig.toml: [install] # Accepts a number of seconds (e.g., 259200 for 3 days) or an ms-style duration string (e.g., "3d", "1 week") minimumReleaseAge = "3d" # Optional: List packages to bypass this check minimumReleaseAgeExcludes = ["@types/bun", "typescript"] Key details regarding minimumReleaseAge: - Scope: This setting affects dependency resolution during bun install and bun add, not the publishing process [1][5]. - Input format: It supports both a raw number of seconds or a duration string (e.g., "3d", "48h", "1 week") [6]. - Exclusion: You can use minimumReleaseAgeExcludes to exempt specific packages from this age gate [2]. - bun publish: This command does not use minimumReleaseAge [3][4]. When running bun publish, you can control lockfile behavior using flags like --no-lockfile (to not update the lockfile) or --disallow-lockfile-changes [3][4]. If you are seeing issues with this setting, note that it should be defined in a project-local bunfig.toml [7]. There have been reports of global $HOME/.bunfig.toml configurations being ignored in certain environments [7].
Citations:
- 1: https://bun.com/docs/pm/cli/install
- 2: https://bun.com/docs/runtime/bunfig
- 3: https://bun.com/docs/pm/cli/publish
- 4: https://bun.sh/docs/pm/cli/publish
- 5: Add support to minimumReleaseAge and minimumReleaseAgeExclude for
bun installoven-sh/bun#22679 - 6: install: accept ms-style duration strings for minimumReleaseAge oven-sh/bun#30529
- 7:
[install] minimumReleaseAgein$HOME/.bunfig.tomlsilently ignored (project-local works) oven-sh/bun#30750
🌐 Web query:
Bun install minimumReleaseAge frozen lockfile does not fail
💡 Result:
In earlier versions of Bun, the minimumReleaseAge configuration was only enforced during dependency resolution (e.g., when adding new packages or updating dependencies) [1]. Historically, this meant that if a package version was already pinned in the bun.lock file, bun install and bun install --frozen-lockfile would silently install that version, even if it violated the minimumReleaseAge cooldown, essentially bypassing the protection [1]. However, this behavior has been addressed to ensure better security [1]. A fix was implemented (referenced in Bun issue #30525) where bun install --frozen-lockfile now performs a pre-install gate check [1]. This check walks the bun.lock file, verifies the publication timestamps of the pinned versions against the minimumReleaseAge threshold, and will fail (exit with an error) if any pinned version violates the cooldown [1]. If you find that your minimumReleaseAge is being ignored, ensure you are running a version of Bun that includes this fix [1]. Additionally, note that minimumReleaseAge is intended to gate resolution; if you are encountering issues where it appears ignored, check that you are not accidentally bypassing it via minimumReleaseAgeExcludes or by setting the age to 0 [1][2][3]. If a lockfile was generated before a strict minimumReleaseAge was enforced, bun install --frozen-lockfile should now correctly identify and error on those previously-allowed, now-blocked versions [1].
Citations:
- 1: minimumReleaseAge is silently bypassed for versions already in bun.lock oven-sh/bun#30525
- 2: feat: set default minimumReleaseAge to 3 days (259200s) oven-sh/bun#28729
- 3: install: accept ms-style duration strings for minimumReleaseAge oven-sh/bun#30529
Remove the release-age override if supply-chain protection is desired.
Lines 92 and 140 pass --minimum-release-age 0, which disables Bun’s minimumReleaseAge pre-install gate. If this is not intentional, remove the flag from both bun install --frozen-lockfile commands so pinned lockfile entries are still age-checked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 92 - 95, Remove the --minimum-release-age 0 override
from both bun install --frozen-lockfile commands in the Makefile, including the
commands near the skills SDK installation and the other installation flow, so
Bun’s configured minimumReleaseAge protection remains active.
Source: MCP tools
| count=$$(printf '%s\n' "$$names" | wc -l | tr -d ' '); \ | ||
| echo "Installing $$count skill(s) from $$source..."; \ | ||
| bun x skills add "$$source" --global --yes $$skill_args </dev/null; \ | ||
| $(SKILLS_CLI) add "$$source" --global --yes $$skill_args </dev/null; \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote SKILLS_CLI at every command site.
SKILLS_CLI includes SKILLS_PROJECT_DIR, but Lines 118 and 141 expand it without quotes. A checkout path containing spaces will fail before the CLI starts. The command printed at Line 161 is also not copy-safe.
Proposed fix
- $(SKILLS_CLI) add "$$source" --global --yes $$skill_args </dev/null; \
+ "$(SKILLS_CLI)" add "$$source" --global --yes $$skill_args </dev/null; \
- @$(SKILLS_CLI) update --global --yes </dev/null
+ @"$(SKILLS_CLI)" update --global --yes </dev/null
- echo "warn: no installed skills for install-all repo $$repo; run: bun install --frozen-lockfile --minimum-release-age 0 && $(SKILLS_CLI) add $$repo --global --yes --skill '*'"; \
+ echo "warn: no installed skills for install-all repo $$repo; run: bun install --frozen-lockfile --minimum-release-age 0 && \"$(SKILLS_CLI)\" add $$repo --global --yes --skill '*'"; \Also applies to: 141-141, 161-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 118, Quote the SKILLS_CLI expansion at every command site
in the Makefile, including the add command and the commands around lines 141 and
161, so paths containing spaces execute correctly and printed commands remain
copy-safe.
| "@intellectronica/ruler": "^0.3.23", | ||
| "lefthook": "^2.0.15", | ||
| "skills": "^1.5.20" | ||
| "skills": "^1.5.22" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -e node_modules/.bin/skills ]; then
readlink node_modules/.bin/skills || true
head -n 1 node_modules/.bin/skills
fi
command -v node >/dev/null && node --version || echo "node is not on PATH"
command -v bun >/dev/null && bun --version || echo "bun is not on PATH"
fd -H -t f '^(package\.json|\.nvmrc|\.node-version|\.tool-versions|Dockerfile.*)$' . \
-x sh -c 'printf "\n--- %s ---\n" "$1"; rg -n "engines|node|bun|setup-node|setup-bun" "$1" || true' sh {}Repository: shunkakinoki/dotagents
Length of output: 523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- package.json ---\n'
cat -n package.json
printf '\n--- lockfile skills package entry ---\n'
if [ -f bun.lock ]; then
python3 - <<'PY'
from pathlib import Path
p = Path('bun.lock')
text = p.read_text()
for i,line in enumerate(text.splitlines(), 1):
if '"skills' in line or 'skills@' in line or 'name": "skills"' in line:
start=max(1,i-8); end=min(len(text.splitlines()), i+15)
for n in range(start,end+1):
print(f"{n:4d}: {text.splitlines()[n-1]}")
print("...")
PY
fi
printf '\n--- files mentioning runtime/tooling ---\n'
fd -H -t f '^(package\.json|bun\.lock$|\.nvmrc|\.node-version|\.tool-versions|Dockerfile.*|Makefile|\.github/workflows/.*)$' . \
-x sh -c 'printf "\n--- %s ---\n" "$1"; rg -n "engines|node|bun|setup-node|setup-bun|node_modules/.bin/skills|skills:" "$1" || true' sh {}Repository: shunkakinoki/dotagents
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package.json ---'
cat -n package.json
printf '%s\n' ''
printf '%s\n' '--- lockfile skills package entry ---'
if [ -f bun.lock ]; then
python3 - <<'PY'
from pathlib import Path
p = Path('bun.lock')
text = p.read_text()
for i,line in enumerate(text.splitlines(), 1):
if '"skills' in line or 'skills@' in line or 'name": "skills"' in line:
start=max(1,i-8); end=min(len(text.splitlines()), i+15)
for n in range(start,end+1):
print(f"{n:4d}: {text.splitlines()[n-1]}")
print("...")
PY
fi
printf '%s\n' ''
printf '%s\n' '--- files mentioning runtime/tooling ---'
find . \( -type f \( -name package.json -o -name bun.lock -o -name .nvmrc -o -name .node-version -o -name .tool-versions -o 'iname Dockerfile*' \) -o -path '.github/workflows/*' \) -print0 2>/dev/null | sort -z | xargs -0 -r sh -c 'printf "%s\n" "--- $1 ---"; rg -n "engines|node|bun|setup-node|setup-bun|node_modules/.bin/skills|skills:" "$1" || true' sh {}Repository: shunkakinoki/dotagents
Length of output: 5786
🌐 Web query:
vercel-labs skills package.json engines node bin cli.mjs skills@1.5.22
💡 Result:
The vercel-labs/skills package (npm: skills), specifically version 1.5.22, is the CLI tool for the open agent skills ecosystem [1][2][3]. Key details from its package.json and configuration: Node.js Engine Requirements: The package.json for version 1.5.22 declares the following engine requirement [1]: "engines": { "node": ">=22.20.0" } Note that while older documentation and previous versions of the package historically claimed support for Node.js 18+ [4][5][6], the repository has updated its requirements to align with its build and runtime behavior, which relies on features available in newer Node.js versions (specifically those supporting native TypeScript execution or newer ESM exports) [4][5]. Binaries (CLI): The package defines its executable entry points in the bin field of package.json [1]: "bin": { "skills": "./bin/cli.mjs", "add-skill": "./bin/cli.mjs" } Both the primary command, skills, and its alias, add-skill, map to the same file, bin/cli.mjs [1][7][8]. This file acts as a wrapper that manages the execution flow for the CLI [7][8]. General Information: - Package Manager: The project uses pnpm, as specified in package.json by "packageManager": "pnpm@10.17.1" [1][6]. - Module System: The package is configured as an ESM-only package ("type": "module") [1]. - Functionality: The CLI is used to install, manage, and list skills for various AI agents (e.g., Claude Code, OpenCode, Cursor) [2][8][3]. It can be invoked directly via npx skills without local installation [6][7][8].
Citations:
- 1: https://github.com/vercel-labs/skills/blob/main/package.json
- 2: https://registry.npmjs.org/skills
- 3: https://www.npmjs.com/package/skills
- 4: Build/test runtime requirement is higher than package.json engines.node claims vercel-labs/skills#603
- 5: [Bug]: skills@1.5.16 crashes on supported Node versions despite engines.node >=18 vercel-labs/skills#1672
- 6: https://vercel-labs-skills.mintlify.app/installation
- 7: https://deepwiki.com/vercel-labs/skills/2.1-installation
- 8: https://deepwiki.com/vercel-labs/skills/2-getting-started
Document or enforce Node 22.20.0+ for skills@1.5.22.
skills@1.5.22 declares node >=22.20.0, and its CLI entry is bin/cli.mjs. This repository does not declare a Node runtime contract, so supported checkouts can still fail if they run an older Node. Add a runtime marker such as .node-version, or update the README with this Node requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 20, Declare the required Node.js runtime for the skills
dependency by adding a repository runtime marker such as .node-version set to
22.20.0, or documenting Node.js 22.20.0+ in the README. Ensure the contract
clearly applies to supported checkouts using skills@1.5.22.
Source: MCP tools
1 participant
Adds a compact code-comments rule to
rules/, propagated to all agent instruction files via ruler.Based on the placement test: not "is this information useful?" but "is the code the right place for it?"
🤖 Generated with Claude Code
Summary by cubic
Adds a clear code comment rule: write only non‑obvious WHY; keep WHAT, history, and task IDs out of source. Also switches skills tasks to the project‑local CLI and bumps
skillsto^1.5.22for consistent installs.New Features
rules/code-comments.mdwith concise guidance; propagated via@intellectronica/ruler.Dependencies
skillsto^1.5.22; updatedbun.lock(includestarand related deps).$(SKILLS_CLI)instead ofbun x; runbun install --frozen-lockfile --minimum-release-age 0before skills tasks.UPGRADE_LOG.mdwith verification steps.Written for commit f587962. Summary will update on new commits.