feat/project bootstrap skill#23
Open
strategicthings wants to merge 10 commits into
Open
Conversation
strategicthings
commented
Feb 20, 2026
strategicthings
commented
- feat: add project-bootstrap skill for automatic cross-platform scaffolding
- feat: add automatic version checking on session start
- fix: address review feedback — hardening and refactoring
- fix: harden project-bootstrap hook + codex skill sync
- fix: make global hooks opt-in and reduce update-check noise
- feat: prompt separately for update checks during global setup
- fix: remove version-check hook when user opts out
- fix: use portable UTC timestamp in bootstrap error log
…lding Adds a new `project-bootstrap` skill that auto-scaffolds metaswarm into any project on first session. Works across both Claude Code and Codex: - **Claude Code**: Fully automatic via SessionStart hook. The bootstrap script runs on every session start, checks for the metaswarm marker file, and runs `npx metaswarm install` if missing. Zero-latency no-op for already-bootstrapped projects. - **Codex**: Global skill discovery via ~/.agents/skills/ symlink. The agent invokes the project-bootstrap skill at session start to check and scaffold if needed. The install command now also: - Copies the bootstrap script to ~/.local/bin/ - Adds the SessionStart hook to ~/.claude/settings.json - Installs global Codex skills and symlink (if codex is present) New files: - skills/project-bootstrap/SKILL.md - skills/project-bootstrap/scripts/metaswarm-bootstrap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a metaswarm-version-check script that runs as a SessionStart hook alongside the bootstrap script. Compares the installed metaswarm version against npm latest, with a 24-hour cache to avoid redundant network calls. Notifies the agent when an update is available. The install command now wires both hooks (bootstrap + version-check) into ~/.claude/settings.json and always overwrites the scripts to keep them current across upgrades. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract CLI logic into focused functions: installGlobalScripts(), updateClaudeSettings(), installCodexSkills() - Create ~/.claude/settings.json if it doesn't exist instead of silently skipping hook installation - Guard against empty/malformed SessionStart array structure - Use exact path matching for duplicate hook detection instead of fragile .includes() - Handle broken symlinks via lstat + proper error handling - Warn if ~/.local/bin is not in PATH - Fail explicitly on corrupted settings.json with actionable message - Replace python3 dependency with node (already required by metaswarm) for JSON parsing in version-check script - Replace macOS-incompatible `timeout` command with node child_process timeout - Validate cache timestamp with grep -E before arithmetic comparison - Use atomic write (mktemp + mv) for cache file to prevent corruption - Log bootstrap errors to ~/.metaswarm/bootstrap.log instead of swallowing with || true - Move cache file to ~/.metaswarm/version-cache for cleaner home dir Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR introduces global hook and skill installation capabilities to the metaswarm CLI. It adds five new helper functions for installing bootstrap scripts to ~/.local/bin, updating Claude settings with SessionStart hooks, managing Codex skills via symlinks, and includes documentation and bash scripts to support project bootstrapping and version checking workflows. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as metaswarm CLI
participant FileSystem as FileSystem
participant ClaudeSettings as ~/.claude/settings.json
participant CodexSkills as ~/.agents/skills/metaswarm
User->>CLI: Run with --install-global-hooks
CLI->>CLI: Prompt user (if TTY)
CLI->>FileSystem: installGlobalScripts()
FileSystem->>FileSystem: Copy bootstrap scripts to ~/.local/bin
FileSystem-->>CLI: Scripts installed
CLI->>ClaudeSettings: updateClaudeSettings()
ClaudeSettings->>ClaudeSettings: Read & modify SessionStart hooks
ClaudeSettings-->>CLI: Settings updated
CLI->>CodexSkills: installCodexSkills()
CodexSkills->>CodexSkills: Create symlink to skills directory
CodexSkills-->>CLI: Skills installed
CLI-->>User: Global hooks setup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryAdds project-bootstrap skill with SessionStart hooks for auto-scaffolding and version checking. The PR implements cross-platform automatic setup but has critical error handling gaps and a variable shadowing bug. Key Changes:
Critical Issues:
Code Quality Issues:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| cli/metaswarm.js | Adds global hook installation with filesystem operations lacking error handling and variable shadowing at line 590 |
| skills/project-bootstrap/scripts/metaswarm-bootstrap | Bootstrap script with proper guards and error logging, no critical issues found |
| skills/project-bootstrap/scripts/metaswarm-version-check | Version check script with timestamp validation issue and mktemp path problem at line 114 |
| skills/project-bootstrap/SKILL.md | Documentation file with clear instructions and anti-patterns guide, no issues |
Last reviewed commit: 4190e41
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+232
to
+251
| function installGlobalScripts(scriptsDir, localBinDir, hookScripts) { | ||
| mkdirp(localBinDir); | ||
|
|
||
| // Warn if ~/.local/bin is not in PATH | ||
| const pathDirs = (process.env.PATH || '').split(path.delimiter); | ||
| if (!pathDirs.some(d => d === localBinDir || d === path.join(os.homedir(), '.local', 'bin'))) { | ||
| warn('~/.local/bin is not in your PATH — hooks will use absolute paths'); | ||
| } | ||
|
|
||
| for (const { name, desc } of hookScripts) { | ||
| const src = path.join(scriptsDir, name); | ||
| const dest = path.join(localBinDir, name); | ||
| if (fs.existsSync(src)) { | ||
| // Always overwrite to keep scripts current | ||
| fs.copyFileSync(src, dest); | ||
| fs.chmodSync(dest, 0o755); | ||
| info(`~/.local/bin/${name} (${desc} installed)`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Extracting these four helper functions (installGlobalScripts, updateClaudeSettings, removeClaudeSessionStartHook, installCodexSkills) violates Single Responsibility. The install function now has multiple reasons to change (local project install, global hooks, Claude settings, Codex skills). Each should be its own module or at minimum a separate file.
Also, these functions directly access the filesystem with no error handling strategy beyond inline warns - errors swallowed silently will make debugging difficult when hooks fail to install.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 232-251
Comment:
Extracting these four helper functions (`installGlobalScripts`, `updateClaudeSettings`, `removeClaudeSessionStartHook`, `installCodexSkills`) violates Single Responsibility. The `install` function now has multiple reasons to change (local project install, global hooks, Claude settings, Codex skills). Each should be its own module or at minimum a separate file.
Also, these functions directly access the filesystem with no error handling strategy beyond inline warns - errors swallowed silently will make debugging difficult when hooks fail to install.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+254
to
+310
| const claudeSettingsPath = path.join(os.homedir(), '.claude', 'settings.json'); | ||
|
|
||
| // Create ~/.claude/ and settings.json if they don't exist | ||
| if (!fs.existsSync(path.dirname(claudeSettingsPath))) { | ||
| mkdirp(path.dirname(claudeSettingsPath)); | ||
| } | ||
| if (!fs.existsSync(claudeSettingsPath)) { | ||
| fs.writeFileSync(claudeSettingsPath, JSON.stringify({ hooks: {} }, null, 2) + '\n'); | ||
| info('~/.claude/settings.json (created)'); | ||
| } | ||
|
|
||
| let settings; | ||
| try { | ||
| settings = JSON.parse(fs.readFileSync(claudeSettingsPath, 'utf-8')); | ||
| } catch (err) { | ||
| warn(`~/.claude/settings.json contains invalid JSON: ${err.message}`); | ||
| warn('Skipping hook installation — please fix settings.json manually'); | ||
| return; | ||
| } | ||
|
|
||
| if (!settings.hooks) settings.hooks = {}; | ||
| if (!Array.isArray(settings.hooks.SessionStart) || settings.hooks.SessionStart.length === 0) { | ||
| settings.hooks.SessionStart = [{ matcher: '', hooks: [] }]; | ||
| } | ||
| let sessionStart = settings.hooks.SessionStart.find(entry => | ||
| entry && typeof entry === 'object' && entry.matcher === '' | ||
| ); | ||
| if (!sessionStart) { | ||
| sessionStart = { matcher: '', hooks: [] }; | ||
| settings.hooks.SessionStart.push(sessionStart); | ||
| } | ||
| if (!sessionStart.hooks || !Array.isArray(sessionStart.hooks)) { | ||
| sessionStart.hooks = []; | ||
| } | ||
|
|
||
| let changed = false; | ||
| for (const { name } of hookScripts) { | ||
| const dest = path.join(localBinDir, name); | ||
| const alreadyInstalled = sessionStart.hooks.some(h => | ||
| h.command === dest || (h.command && h.command.endsWith(`/${name}`)) | ||
| ); | ||
| if (!alreadyInstalled) { | ||
| sessionStart.hooks.push({ | ||
| type: 'command', | ||
| command: dest, | ||
| }); | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| if (changed) { | ||
| fs.writeFileSync(claudeSettingsPath, JSON.stringify(settings, null, 2) + '\n'); | ||
| info('~/.claude/settings.json (SessionStart hooks added)'); | ||
| } else { | ||
| skip('~/.claude/settings.json (hooks already present)'); | ||
| } | ||
| } |
There was a problem hiding this comment.
updateClaudeSettings is 57 lines doing multiple things: creating files, parsing JSON, validating structure, mutating nested objects, and writing back. Break this into smaller functions: ensureSettingsFile(), parseSettings(), addSessionStartHook(), writeSettings(). Each should be testable in isolation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 254-310
Comment:
`updateClaudeSettings` is 57 lines doing multiple things: creating files, parsing JSON, validating structure, mutating nested objects, and writing back. Break this into smaller functions: `ensureSettingsFile()`, `parseSettings()`, `addSessionStartHook()`, `writeSettings()`. Each should be testable in isolation.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+265
to
+272
| let settings; | ||
| try { | ||
| settings = JSON.parse(fs.readFileSync(claudeSettingsPath, 'utf-8')); | ||
| } catch (err) { | ||
| warn(`~/.claude/settings.json contains invalid JSON: ${err.message}`); | ||
| warn('Skipping hook installation — please fix settings.json manually'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
JSON parsing can throw for multiple reasons (malformed JSON, filesystem errors, etc.) but catching with generic err loses type information. The error message only shows err.message but doesn't include enough context to fix the issue (like which hook tried to install, or what the invalid JSON looks like).
Use typed error classes or result types:
try {
settings = JSON.parse(fs.readFileSync(claudeSettingsPath, 'utf-8'));
} catch (err) {
warn(`Failed to parse ~/.claude/settings.json: ${err.message}`);
warn(`File contents (first 200 chars): ${fs.readFileSync(claudeSettingsPath, 'utf-8').slice(0, 200)}`);
return;
}Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 265-272
Comment:
JSON parsing can throw for multiple reasons (malformed JSON, filesystem errors, etc.) but catching with generic `err` loses type information. The error message only shows `err.message` but doesn't include enough context to fix the issue (like which hook tried to install, or what the invalid JSON looks like).
Use typed error classes or result types:
```javascript
try {
settings = JSON.parse(fs.readFileSync(claudeSettingsPath, 'utf-8'));
} catch (err) {
warn(`Failed to parse ~/.claude/settings.json: ${err.message}`);
warn(`File contents (first 200 chars): ${fs.readFileSync(claudeSettingsPath, 'utf-8').slice(0, 200)}`);
return;
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+345
to
+397
| function installCodexSkills() { | ||
| if (!which('codex')) return; | ||
|
|
||
| const codexSkillsDir = path.join(os.homedir(), '.codex', 'metaswarm'); | ||
| const codexGlobalSkillsDir = path.join(codexSkillsDir, 'skills'); | ||
| const agentsSymlink = path.join(os.homedir(), '.agents', 'skills', 'metaswarm'); | ||
| const desiredSymlinkTarget = codexGlobalSkillsDir; | ||
|
|
||
| mkdirp(codexSkillsDir); | ||
| if (fs.existsSync(codexGlobalSkillsDir)) { | ||
| fs.rmSync(codexGlobalSkillsDir, { recursive: true, force: true }); | ||
| } | ||
| copyDir(path.join(PKG_ROOT, 'skills'), codexGlobalSkillsDir); | ||
| info('~/.codex/metaswarm/skills (global Codex skills installed/updated)'); | ||
|
|
||
| let createSymlink = false; | ||
| try { | ||
| const stats = fs.lstatSync(agentsSymlink); | ||
| if (!stats.isSymbolicLink()) { | ||
| warn('~/.agents/skills/metaswarm exists and is not a symlink — leaving as-is'); | ||
| return; | ||
| } | ||
|
|
||
| const currentTarget = fs.readlinkSync(agentsSymlink); | ||
| const resolvedCurrent = path.resolve(path.dirname(agentsSymlink), currentTarget); | ||
| const resolvedDesired = path.resolve(desiredSymlinkTarget); | ||
|
|
||
| if (resolvedCurrent === resolvedDesired) { | ||
| skip('~/.agents/skills/metaswarm'); | ||
| return; | ||
| } | ||
|
|
||
| fs.unlinkSync(agentsSymlink); | ||
| createSymlink = true; | ||
| } catch (err) { | ||
| if (err.code === 'ENOENT') { | ||
| createSymlink = true; | ||
| } else { | ||
| warn(`Could not check ~/.agents/skills/metaswarm: ${err.message}`); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (createSymlink) { | ||
| mkdirp(path.dirname(agentsSymlink)); | ||
| try { | ||
| fs.symlinkSync(desiredSymlinkTarget, agentsSymlink); | ||
| info('~/.agents/skills/metaswarm (symlinked for global discovery)'); | ||
| } catch (symlinkErr) { | ||
| warn(`Could not create symlink ~/.agents/skills/metaswarm: ${symlinkErr.message}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
installCodexSkills has deep nesting (7 levels at the symlink check) and multiple exit points. Extract symlink validation into its own function: validateSymlink(path, expectedTarget) returning { valid: boolean, reason?: string }. This flattens the nesting and makes the logic testable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 345-397
Comment:
`installCodexSkills` has deep nesting (7 levels at the symlink check) and multiple exit points. Extract symlink validation into its own function: `validateSymlink(path, expectedTarget)` returning `{ valid: boolean, reason?: string }`. This flattens the nesting and makes the logic testable.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+354
to
+356
| if (fs.existsSync(codexGlobalSkillsDir)) { | ||
| fs.rmSync(codexGlobalSkillsDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
Silently deleting ~/.codex/metaswarm/skills without checking if it's a symlink or has user modifications could destroy data. Check if it's a symlink first, or if it contains files not in the metaswarm package, and warn before deletion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 354-356
Comment:
Silently deleting `~/.codex/metaswarm/skills` without checking if it's a symlink or has user modifications could destroy data. Check if it's a symlink first, or if it contains files not in the metaswarm package, and warn before deletion.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+587
to
+598
| if (process.stdin.isTTY && process.stdout.isTTY) { | ||
| const answer = await askUser( | ||
| ' Install global hooks/skills (~/.claude/settings.json, ~/.local/bin, ~/.codex, ~/.agents)? [y/N] ' | ||
| ); | ||
| installGlobalHooks = answer === 'y' || answer === 'yes'; | ||
| if (!installGlobalHooks) { | ||
| note('global hooks/skills not installed'); | ||
| } | ||
| } else { | ||
| note('global hooks/skills not installed (--install-global-hooks to enable)'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using process.stdin.isTTY to decide whether to prompt is good, but when not TTY, the code assumes the user wants to skip (installGlobalHooks = false). This makes CI/automated installs diverge from interactive behavior. Better: provide a clear default (like --no-global-hooks in non-TTY mode) and document it in the help text.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 587-598
Comment:
Using `process.stdin.isTTY` to decide whether to prompt is good, but when not TTY, the code assumes the user wants to skip (`installGlobalHooks = false`). This makes CI/automated installs diverge from interactive behavior. Better: provide a clear default (like `--no-global-hooks` in non-TTY mode) and document it in the help text.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+41
to
+43
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | ||
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | ||
| fi |
There was a problem hiding this comment.
The script pipes install output to tee -a which appends to the log, but if install succeeds, there's no log entry. Only failures are logged. This makes it impossible to distinguish between "never ran" and "ran successfully". Log both success and failure:
if echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE"
else
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE"
fiPrompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-bootstrap
Line: 41-43
Comment:
The script pipes install output to `tee -a` which appends to the log, but if install succeeds, there's no log entry. Only failures are logged. This makes it impossible to distinguish between "never ran" and "ran successfully". Log both success and failure:
```bash
if echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE"
else
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE"
fi
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| # --- Fast path: skip if checked recently --- | ||
| if [ -f "$CACHE_FILE" ]; then | ||
| last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0") |
There was a problem hiding this comment.
Parsing the timestamp with grep -E '^[0-9]+$' only validates format but doesn't verify it's a reasonable timestamp. If the cache file is corrupted with a value like 999999999999999, the script will trust it. Add bounds checking:
last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0")
now=$(date +%s)
# Reject timestamps from the future or before 2020
if [ "$last_check" -gt "$now" ] || [ "$last_check" -lt 1577836800 ]; then
last_check=0
fiPrompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-version-check
Line: 24
Comment:
Parsing the timestamp with `grep -E '^[0-9]+$'` only validates format but doesn't verify it's a reasonable timestamp. If the cache file is corrupted with a value like `999999999999999`, the script will trust it. Add bounds checking:
```bash
last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0")
now=$(date +%s)
# Reject timestamps from the future or before 2020
if [ "$last_check" -gt "$now" ] || [ "$last_check" -lt 1577836800 ]; then
last_check=0
fi
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+91
to
+98
| tmp_cache=$(mktemp "${CACHE_FILE}.XXXXXX") | ||
| { | ||
| date +%s | ||
| if [ -n "$msg" ]; then | ||
| echo "$msg" | ||
| fi | ||
| } > "$tmp_cache" | ||
| mv "$tmp_cache" "$CACHE_FILE" |
There was a problem hiding this comment.
mktemp "${CACHE_FILE}.XXXXXX" creates the temp file in the current directory, not in $CACHE_DIR. If the script runs from a read-only directory, this will fail silently. Use:
tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX")Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-version-check
Line: 91-98
Comment:
`mktemp "${CACHE_FILE}.XXXXXX"` creates the temp file in the current directory, not in `$CACHE_DIR`. If the script runs from a read-only directory, this will fail silently. Use:
```bash
tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX")
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/metaswarm.js (1)
686-689:⚠️ Potential issue | 🟠 MajorAsync entry points lack
.catch()— unhandled rejections will crash with an unhelpful stack trace.Both
init()andinstall()areasyncand called without error handling. If anyawait(e.g.,askUseron a broken pipe, or a filesystem error in the new global hooks code) rejects, Node emits anUnhandledPromiseRejectionWarning(or crashes in newer Node versions with--unhandled-rejections=throw).Proposed fix
if (cmd === 'init') { - init(args.slice(1)); + init(args.slice(1)).catch(err => { console.error(err.message); process.exit(1); }); } else if (cmd === 'install') { - install(args.slice(1)); + install(args.slice(1)).catch(err => { console.error(err.message); process.exit(1); }); } else if (cmd === '--version' || cmd === '-v') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/metaswarm.js` around lines 686 - 689, The async entry calls to init(...) and install(...) are invoked without error handling; wrap each call so any rejection is caught and handled rather than letting Node emit an unhandled rejection. Modify the branch that calls init(args.slice(1)) and install(args.slice(1)) to call them as promises and attach a .catch handler (or use Promise.resolve(...).catch) that logs the error (including err.message/stack) and exits with a non‑zero code; reference the init and install call sites to locate and update the two call expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/metaswarm.js`:
- Around line 241-250: The loop over hookScripts silently skips missing source
files; update the block that iterates hookScripts (using variables hookScripts,
scriptsDir, localBinDir) to log a warning when fs.existsSync(src) is false
instead of doing nothing—call warn(...) with the src and script name (and
optional desc) so missing packaged scripts are visible, while keeping the
existing copy/permission/info logic for present files.
- Around line 600-625: When the user opts out of update checks the code filters
metaswarm-version-check out of selectedHookScripts but doesn't remove any
existing script file from localBinDir; after calling
removeClaudeSessionStartHook(localBinDir, 'metaswarm-version-check') add logic
(e.g., in the same block where installGlobalScripts/updateClaudeSettings are
called) to check for and delete the metaswarm-version-check executable in
localBinDir (using scriptsDir/localBinDir path resolution already used by
installGlobalScripts) so an earlier install doesn't leave a stale file;
reference the existing symbols selectedHookScripts, installGlobalScripts,
updateClaudeSettings, removeClaudeSessionStartHook, scriptsDir and localBinDir
to implement the explicit file removal.
In `@skills/project-bootstrap/scripts/metaswarm-bootstrap`:
- Line 17: The script enables strict errexit with "set -euo pipefail" which can
cause the script to exit non-zero if the log append (echo >> "$LOG_FILE") fails
and thus break the documented "always exit 0" contract; wrap the log-write so
failures are ignored (either temporarily disable errexit around that block with
"set +e" before the write and "set -e" after, or append "|| true" to the echo >>
"$LOG_FILE" command) and apply the same guard to the adjacent lines that write
to the log (the echo >> "$LOG_FILE" block referenced in the diff and lines
41-43).
- Around line 38-43: The current pipeline "echo 'Y' | npx metaswarm install 2>&1
| tee -a \"$LOG_FILE\"" writes both stdout and stderr to LOG_FILE; change it so
only stderr is appended to LOG_FILE while stdout remains on the console (so the
log only contains errors). Concretely, update the command around npx metaswarm
install to redirect stderr into tee -a \"$LOG_FILE\" (e.g., via a stderr-only
redirection/process substitution) and leave stdout untouched; alternatively,
implement truncation/rotation for \"$LOG_FILE\" before appending if you want to
keep full output but limit growth. Ensure references to LOG_DIR and LOG_FILE
remain and that the failure branch that writes the timestamped bootstrap failed
line still appends to \"$LOG_FILE\".
In `@skills/project-bootstrap/scripts/metaswarm-version-check`:
- Around line 89-98: The atomic cache-write sequence (mkdir -p "$CACHE_DIR";
tmp_cache=$(mktemp ...); { ... } > "$tmp_cache"; mv "$tmp_cache" "$CACHE_FILE")
can fail (mktemp/mv) and will cause the script to exit under set -euo pipefail;
wrap that whole sequence in a subshell and ensure its failure is ignored (e.g.,
run the subshell and append || true) so that errors creating the temp file or
moving it do not abort the script; reference the CACHE_DIR/CACHE_FILE variables,
the mktemp invocation assigned to tmp_cache, and the mv "$tmp_cache"
"$CACHE_FILE" move when applying the change.
- Around line 83-87: The current string equality check between installed_version
and latest_version can misclassify versions; replace the if condition that uses
[ "$installed_version" != "$latest_version" ] with a semver-aware comparison
invoked via node -e: call Node with both variables passed in and implement a
proper semver comparator (parse major/minor/patch and prerelease rules) that
returns true only when latest_version is greater than installed_version, and
only then set msg="metaswarm update available: ${installed_version} →
${latest_version}. Run: npx metaswarm@${latest_version} install"; update the
conditional around variables installed_version and latest_version in the script
to use that node-based comparison.
In `@skills/project-bootstrap/SKILL.md`:
- Line 90: Add explicit language specifiers to the two fenced code blocks
described as the "flow diagram" and the "update notice" in SKILL.md by changing
their opening fences to include text (i.e., use ```text) so the MD040 linter
passes; leave the content unchanged. You can ignore the MD031 warnings for the
numbered-list-nested code blocks (lines around the nested examples) if the
rendered output is correct, but ensure the two identified standalone fences
(flow diagram and update notice) are updated to use the text specifier.
- Around line 130-135: The docs mention a non-existent --force flag for the
installer; update the step that references `npx metaswarm@<latest> install
--force` to remove that flag and instead instruct users to manually remove old
files before running `npx metaswarm@<latest> install`, or list the actual
supported flags (`--with-husky`, `--install-global-hooks`, `--no-global-hooks`)
so users know the correct options; edit the SKILL.md step text (the installer /
install command and step 3) to remove the --force reference and clarify the
correct workflow for force-updating files.
---
Outside diff comments:
In `@cli/metaswarm.js`:
- Around line 686-689: The async entry calls to init(...) and install(...) are
invoked without error handling; wrap each call so any rejection is caught and
handled rather than letting Node emit an unhandled rejection. Modify the branch
that calls init(args.slice(1)) and install(args.slice(1)) to call them as
promises and attach a .catch handler (or use Promise.resolve(...).catch) that
logs the error (including err.message/stack) and exits with a non‑zero code;
reference the init and install call sites to locate and update the two call
expressions.
Comment on lines
+600
to
+625
| if (installGlobalHooks) { | ||
| let selectedHookScripts = hookScripts; | ||
| if (process.stdin.isTTY && process.stdout.isTTY) { | ||
| const updateCheckAnswer = await askUser( | ||
| ' Enable session-start update checks? [y/N] ' | ||
| ); | ||
| installVersionCheck = updateCheckAnswer === 'y' || updateCheckAnswer === 'yes'; | ||
| if (!installVersionCheck) { | ||
| selectedHookScripts = hookScripts.filter(s => s.name !== 'metaswarm-version-check'); | ||
| info('session-start update checks disabled'); | ||
| } | ||
| } | ||
|
|
||
| if (fs.existsSync(scriptsDir)) { | ||
| installGlobalScripts(scriptsDir, localBinDir, selectedHookScripts); | ||
| updateClaudeSettings(localBinDir, selectedHookScripts); | ||
| if (!installVersionCheck) { | ||
| removeClaudeSessionStartHook(localBinDir, 'metaswarm-version-check'); | ||
| } | ||
| } | ||
|
|
||
| // Install global skills for Codex | ||
| installCodexSkills(); | ||
| } else if (noGlobalHooksFlag) { | ||
| note('global hooks/skills not installed'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stale metaswarm-version-check file left in ~/.local/bin when user opts out of update checks.
When the user declines version checks (line 606-608), selectedHookScripts excludes metaswarm-version-check so it's not installed. removeClaudeSessionStartHook (line 617) cleans the hook from settings.json. But if a previous install had already copied the script to ~/.local/bin, that file is never deleted. Consider removing the file explicitly:
Proposed fix
if (!installVersionCheck) {
selectedHookScripts = hookScripts.filter(s => s.name !== 'metaswarm-version-check');
info('session-start update checks disabled');
}
}
if (fs.existsSync(scriptsDir)) {
installGlobalScripts(scriptsDir, localBinDir, selectedHookScripts);
updateClaudeSettings(localBinDir, selectedHookScripts);
if (!installVersionCheck) {
removeClaudeSessionStartHook(localBinDir, 'metaswarm-version-check');
+ const staleScript = path.join(localBinDir, 'metaswarm-version-check');
+ if (fs.existsSync(staleScript)) {
+ fs.unlinkSync(staleScript);
+ info('~/.local/bin/metaswarm-version-check (removed)');
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/metaswarm.js` around lines 600 - 625, When the user opts out of update
checks the code filters metaswarm-version-check out of selectedHookScripts but
doesn't remove any existing script file from localBinDir; after calling
removeClaudeSessionStartHook(localBinDir, 'metaswarm-version-check') add logic
(e.g., in the same block where installGlobalScripts/updateClaudeSettings are
called) to check for and delete the metaswarm-version-check executable in
localBinDir (using scriptsDir/localBinDir path resolution already used by
installGlobalScripts) so an earlier install doesn't leave a stale file;
reference the existing symbols selectedHookScripts, installGlobalScripts,
updateClaudeSettings, removeClaudeSessionStartHook, scriptsDir and localBinDir
to implement the explicit file removal.
| # 0 — Not a git repo (silently skipped) | ||
| # 0 — Install failed (logged to ~/.metaswarm/bootstrap.log) | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
set -e can break the "always exit 0" contract if the error log write itself fails.
The script documents exit 0 in all paths, but set -e applies inside the then body. If the echo >> "$LOG_FILE" on line 42 fails (e.g., permission denied after a partial mkdir -p), the script exits non-zero, which could surface as a hook error to the user on session start.
A simple guard:
Proposed fix
if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then
- echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE"
+ echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" 2>/dev/null || true
fiAlso applies to: 41-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/project-bootstrap/scripts/metaswarm-bootstrap` at line 17, The script
enables strict errexit with "set -euo pipefail" which can cause the script to
exit non-zero if the log append (echo >> "$LOG_FILE") fails and thus break the
documented "always exit 0" contract; wrap the log-write so failures are ignored
(either temporarily disable errexit around that block with "set +e" before the
write and "set -e" after, or append "|| true" to the echo >> "$LOG_FILE"
command) and apply the same guard to the adjacent lines that write to the log
(the echo >> "$LOG_FILE" block referenced in the diff and lines 41-43).
Comment on lines
+38
to
+43
| # Run non-interactive install (pipe Y to accept CLAUDE.md prompt) | ||
| # Log errors for debugging instead of swallowing them | ||
| mkdir -p "$LOG_DIR" | ||
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | ||
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
tee -a logs all npx output (including successful runs) to the bootstrap log.
The comment on line 39 says "Log errors for debugging instead of swallowing them", but 2>&1 | tee -a "$LOG_FILE" appends all output (stdout + stderr from every bootstrap) to the log. Over many projects and sessions, this file grows without bound and contains mostly success noise.
If the intent is error-only logging, consider redirecting only stderr to the log, or truncating/rotating the log. If logging everything is intentional, a brief comment clarifying that would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/project-bootstrap/scripts/metaswarm-bootstrap` around lines 38 - 43,
The current pipeline "echo 'Y' | npx metaswarm install 2>&1 | tee -a
\"$LOG_FILE\"" writes both stdout and stderr to LOG_FILE; change it so only
stderr is appended to LOG_FILE while stdout remains on the console (so the log
only contains errors). Concretely, update the command around npx metaswarm
install to redirect stderr into tee -a \"$LOG_FILE\" (e.g., via a stderr-only
redirection/process substitution) and leave stdout untouched; alternatively,
implement truncation/rotation for \"$LOG_FILE\" before appending if you want to
keep full output but limit growth. Ensure references to LOG_DIR and LOG_FILE
remain and that the failure branch that writes the timestamped bootstrap failed
line still appends to \"$LOG_FILE\".
|
|
||
| **Flow:** | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks (MD040).
The flow diagram on line 90 and the update notice format on line 116 lack language specifiers. Use ```text for both to satisfy the linter without implying they're executable code.
The MD031 warnings on lines 130/132 are likely false positives due to code blocks nested inside numbered list items. Based on learnings, these can be safely ignored when the rendered output is correct.
Also applies to: 116-118
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/project-bootstrap/SKILL.md` at line 90, Add explicit language
specifiers to the two fenced code blocks described as the "flow diagram" and the
"update notice" in SKILL.md by changing their opening fences to include text
(i.e., use ```text) so the MD040 linter passes; leave the content unchanged. You
can ignore the MD031 warnings for the numbered-list-nested code blocks (lines
around the nested examples) if the rendered output is correct, but ensure the
two identified standalone fences (flow diagram and update notice) are updated to
use the text specifier.
|
|
||
| # --- Write cache atomically (temp file + rename) --- | ||
| mkdir -p "$CACHE_DIR" | ||
| tmp_cache=$(mktemp "${CACHE_FILE}.XXXXXX") |
There was a problem hiding this comment.
mktemp creates temp file in current directory, not $CACHE_DIR. Fails in read-only directories.
Suggested change
| tmp_cache=$(mktemp "${CACHE_FILE}.XXXXXX") | |
| tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-version-check
Line: 91
Comment:
`mktemp` creates temp file in current directory, not `$CACHE_DIR`. Fails in read-only directories.
```suggestion
tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX")
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+582
to
+583
| let installGlobalHooks = installGlobalHooksFlag; | ||
| let installVersionCheck = installGlobalHooksFlag; |
There was a problem hiding this comment.
Shadowing outer variable installGlobalHooks with installVersionCheck. Use distinct name like shouldInstallVersionCheck.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 582-583
Comment:
Shadowing outer variable `installGlobalHooks` with `installVersionCheck`. Use distinct name like `shouldInstallVersionCheck`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+601
to
+611
| let selectedHookScripts = hookScripts; | ||
| if (process.stdin.isTTY && process.stdout.isTTY) { | ||
| const updateCheckAnswer = await askUser( | ||
| ' Enable session-start update checks? [y/N] ' | ||
| ); | ||
| installVersionCheck = updateCheckAnswer === 'y' || updateCheckAnswer === 'yes'; | ||
| if (!installVersionCheck) { | ||
| selectedHookScripts = hookScripts.filter(s => s.name !== 'metaswarm-version-check'); | ||
| info('session-start update checks disabled'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The installVersionCheck variable is read before being set by the conditional block. If installGlobalHooksFlag is true and !process.stdin.isTTY, line 616 will use the uninitialized value from line 583 (which shadows line 582). Move installVersionCheck = installGlobalHooksFlag; before line 601 or use a different variable name.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 601-611
Comment:
The `installVersionCheck` variable is read before being set by the conditional block. If `installGlobalHooksFlag` is true and `!process.stdin.isTTY`, line 616 will use the uninitialized value from line 583 (which shadows line 582). Move `installVersionCheck = installGlobalHooksFlag;` before line 601 or use a different variable name.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+232
to
+251
| function installGlobalScripts(scriptsDir, localBinDir, hookScripts) { | ||
| mkdirp(localBinDir); | ||
|
|
||
| // Warn if ~/.local/bin is not in PATH | ||
| const pathDirs = (process.env.PATH || '').split(path.delimiter); | ||
| if (!pathDirs.some(d => d === localBinDir || d === path.join(os.homedir(), '.local', 'bin'))) { | ||
| warn('~/.local/bin is not in your PATH — hooks will use absolute paths'); | ||
| } | ||
|
|
||
| for (const { name, desc } of hookScripts) { | ||
| const src = path.join(scriptsDir, name); | ||
| const dest = path.join(localBinDir, name); | ||
| if (fs.existsSync(src)) { | ||
| // Always overwrite to keep scripts current | ||
| fs.copyFileSync(src, dest); | ||
| fs.chmodSync(dest, 0o755); | ||
| info(`~/.local/bin/${name} (${desc} installed)`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
No try-catch around filesystem operations. If fs.copyFileSync or fs.chmodSync throws (permissions, disk full, etc.), the error bubbles up unhandled, potentially crashing the install process. At minimum wrap in try-catch and warn on failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 232-251
Comment:
No try-catch around filesystem operations. If `fs.copyFileSync` or `fs.chmodSync` throws (permissions, disk full, etc.), the error bubbles up unhandled, potentially crashing the install process. At minimum wrap in try-catch and warn on failure.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+354
to
+356
| if (fs.existsSync(codexGlobalSkillsDir)) { | ||
| fs.rmSync(codexGlobalSkillsDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
Unconditionally deleting ~/.codex/metaswarm/skills without checking contents. If user has modifications or other files, they're lost. Check if directory contains user modifications before deleting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 354-356
Comment:
Unconditionally deleting `~/.codex/metaswarm/skills` without checking contents. If user has modifications or other files, they're lost. Check if directory contains user modifications before deleting.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+687
to
+695
| init(args.slice(1)).catch(err => { | ||
| console.error(err && err.stack ? err.stack : String(err)); | ||
| process.exit(1); | ||
| }); | ||
| } else if (cmd === 'install') { | ||
| install(args.slice(1)); | ||
| install(args.slice(1)).catch(err => { | ||
| console.error(err && err.stack ? err.stack : String(err)); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
Added async error handlers to catch and display CLI errors with stack traces
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 687-695
Comment:
Added async error handlers to catch and display CLI errors with stack traces
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+41
to
+42
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | ||
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" |
There was a problem hiding this comment.
Only failures are logged. Success leaves no trace, making it impossible to distinguish "never ran" from "succeeded". Add success logging:
Suggested change
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | |
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | |
| else | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE" | |
| fi |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-bootstrap
Line: 41-42
Comment:
Only failures are logged. Success leaves no trace, making it impossible to distinguish "never ran" from "succeeded". Add success logging:
```suggestion
if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE"
else
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE"
fi
```
How can I resolve this? If you propose a fix, please make it concise.
| ]; | ||
|
|
||
| let installGlobalHooks = installGlobalHooksFlag; | ||
| let installVersionCheck = installGlobalHooksFlag; |
There was a problem hiding this comment.
Variable installVersionCheck shadows outer scope declaration. If second prompt is skipped, line 613 reads wrong variable.
Suggested change
| let installVersionCheck = installGlobalHooksFlag; | |
| let shouldInstallVersionCheck = installGlobalHooksFlag; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 590
Comment:
Variable `installVersionCheck` shadows outer scope declaration. If second prompt is skipped, line 613 reads wrong variable.
```suggestion
let shouldInstallVersionCheck = installGlobalHooksFlag;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+246
to
+247
| fs.copyFileSync(src, dest); | ||
| fs.chmodSync(dest, 0o755); |
There was a problem hiding this comment.
fs.copyFileSync and fs.chmodSync have no error handling. Permissions errors, disk full, etc. will crash install instead of warning.
Wrap in try-catch and warn on failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 246-247
Comment:
`fs.copyFileSync` and `fs.chmodSync` have no error handling. Permissions errors, disk full, etc. will crash install instead of warning.
Wrap in try-catch and warn on failure.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+307
to
+308
| fs.writeFileSync(claudeSettingsPath, JSON.stringify(settings, null, 2) + '\n'); | ||
| info('~/.claude/settings.json (SessionStart hooks added)'); |
There was a problem hiding this comment.
fs.writeFileSync has no error handling. Permissions/disk errors crash install instead of warning.
Wrap in try-catch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 307-308
Comment:
`fs.writeFileSync` has no error handling. Permissions/disk errors crash install instead of warning.
Wrap in try-catch.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+342
to
+343
| fs.writeFileSync(claudeSettingsPath, JSON.stringify(settings, null, 2) + '\n'); | ||
| info(`~/.claude/settings.json (${scriptName} hook removed)`); |
There was a problem hiding this comment.
fs.writeFileSync has no error handling. Permissions/disk errors crash process.
Wrap in try-catch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/metaswarm.js
Line: 342-343
Comment:
`fs.writeFileSync` has no error handling. Permissions/disk errors crash process.
Wrap in try-catch.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+41
to
+43
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | ||
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | ||
| fi |
There was a problem hiding this comment.
Only failure is logged. Success leaves no trace, making "never ran" indistinguishable from "succeeded". Add success logging:
Suggested change
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | |
| fi | |
| if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE" | |
| else | |
| echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE" | |
| fi |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-bootstrap
Line: 41-43
Comment:
Only failure is logged. Success leaves no trace, making "never ran" indistinguishable from "succeeded". Add success logging:
```suggestion
if ! echo "Y" | npx metaswarm install 2>&1 | tee -a "$LOG_FILE"; then
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap failed in $(pwd)" >> "$LOG_FILE"
else
echo "[$(date -u '+%Y-%m-%dT%H:%M:%SZ')] bootstrap succeeded in $(pwd)" >> "$LOG_FILE"
fi
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+24
to
+26
| last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0") | ||
| now=$(date +%s) | ||
| age=$(( now - last_check )) |
There was a problem hiding this comment.
Timestamp validation only checks format, not reasonableness. Corrupted cache with 999999999999999 or future timestamp will be trusted. Add bounds checking:
Suggested change
| last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0") | |
| now=$(date +%s) | |
| age=$(( now - last_check )) | |
| last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0") | |
| now=$(date +%s) | |
| # Reject timestamps from future or before 2020 | |
| if [ "$last_check" -gt "$now" ] || [ "$last_check" -lt 1577836800 ]; then | |
| last_check=0 | |
| fi | |
| age=$(( now - last_check )) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-version-check
Line: 24-26
Comment:
Timestamp validation only checks format, not reasonableness. Corrupted cache with `999999999999999` or future timestamp will be trusted. Add bounds checking:
```suggestion
last_check=$(head -1 "$CACHE_FILE" 2>/dev/null | grep -E '^[0-9]+$' || echo "0")
now=$(date +%s)
# Reject timestamps from future or before 2020
if [ "$last_check" -gt "$now" ] || [ "$last_check" -lt 1577836800 ]; then
last_check=0
fi
age=$(( now - last_check ))
```
How can I resolve this? If you propose a fix, please make it concise.| # --- Write cache atomically (temp file + rename) --- | ||
| ( | ||
| mkdir -p "$CACHE_DIR" | ||
| tmp_cache=$(mktemp "${CACHE_FILE}.XXXXXX") |
There was a problem hiding this comment.
mktemp "${CACHE_FILE}.XXXXXX" creates temp file in current directory, not $CACHE_DIR. Fails in read-only directories.
Suggested change
| tmp_cache=$(mktemp "${CACHE_FILE}.XXXXXX") | |
| tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/project-bootstrap/scripts/metaswarm-version-check
Line: 114
Comment:
`mktemp "${CACHE_FILE}.XXXXXX"` creates temp file in current directory, not `$CACHE_DIR`. Fails in read-only directories.
```suggestion
tmp_cache=$(mktemp "${CACHE_DIR}/version-cache.XXXXXX")
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.