Improve Windows support for bin commands#76
Open
shuntagami wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughこのプルリクエストは、dotfiles リポジトリの複数の Bash スクリプトを Node.js に変換し、Windows サポートを追加します。新たに共有クリップボード抽象化モジュールを導入し、Windows 用の PowerShell ベース展開インフラストラクチャと batch シム生成を実装します。 ChangesWindows Support via Node.js and Deployment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
bin/cleanup (1)
60-67: 💤 Low valueワイルドカードパターンごとに
readdirSync(home)を繰り返し呼び出しています
.goutputstream*、.recently-used.xbel*、tmux-client-*.log、.v8flags.*の4パターンそれぞれでホームディレクトリ全体を読み直しています。一度だけ読み込んでキャッシュすることでI/Oを削減できます。♻️ リファクタリング案
+const homeEntries = fs.readdirSync(home); + function matchingTargets(home, pattern) { if (!pattern.includes('*')) return [path.join(home, pattern)]; const re = wildcardToRegExp(pattern); - return fs.readdirSync(home) + return homeEntries .filter(name => re.test(name)) .map(name => path.join(home, name)); }🤖 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 `@bin/cleanup` around lines 60 - 67, matchingTargets reads the home directory every time it's called for a pattern, causing repeated I/O; change it to read and cache the directory listing once and reuse it for subsequent calls by either (a) adding a simple per-home cache inside matchingTargets (e.g., store fs.readdirSync(home) in a module-scoped Map keyed by home) or (b) moving the single fs.readdirSync(home) call to the caller and passing the names array into matchingTargets; then use that cached names array to .filter(name => wildcardToRegExp(pattern).test(name)) and .map to full paths so each pattern no longer triggers a full readdirSync.bin/watch-downloads-copy (1)
18-18: 💤 Low valueファイルを2回読み込んでいる（バッファ → UTF-8 文字列）
isProbablyText内（Line 18）と Line 35 でそれぞれ同じファイルを読み込んでいる。ダウンロードファイルが大きい場合は不要な I/O が発生する。バッファをキャッシュして変換すれば一度の読み込みで済む。♻️ リファクタリング案
-function isProbablyText(filePath) { - const buf = fs.readFileSync(filePath); +function readIfText(filePath) { + const buf = fs.readFileSync(filePath); if (buf.includes(0)) return false; const sample = buf.subarray(0, Math.min(buf.length, 8192)); const controls = [...sample].filter(byte => byte < 32 && ![9, 10, 13].includes(byte)).length; - return sample.length === 0 || controls / sample.length < 0.02; + if (sample.length === 0 || controls / sample.length >= 0.02) return null; + return buf.toString('utf8'); } -if (isProbablyText(file)) { - const text = fs.readFileSync(file, 'utf8'); - if (copy(text)) { +const text = readIfText(file); +if (text !== null) { + if (copy(text)) { notify(`Copied: ${path.basename(file)}`); } }Also applies to: 34-38
🤖 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 `@bin/watch-downloads-copy` at line 18, The code reads the same file twice (fs.readFileSync at buf and again later) causing extra I/O; change the flow so the file is read once into the buffer and that buffer is reused: modify the isProbablyText call/signature to accept a Buffer (or pass the already-read buf) and, where you currently re-read the file around Line 35, use the cached buf and convert it to a UTF-8 string only when needed; update references to buf, filePath, and isProbablyText so text-detection and any subsequent string conversion reuse the single buffer read.bin/slack-mcp-wrapper (3)
17-23: 💤 Low value簡易クォート除去ロジックの境界ケース
現在の実装は
value.startsWith('"') && value.endsWith('"')で両端のクォートを剥がしますが、KEY="のように単一の"のみだとstartsWithとendsWithが同じ文字に対して両方真になり、slice(1, -1)で空文字列になります（実害は限定的ですが、入力に依存）。またvalue.length >= 2のチェックが無いため、KEY=""の意図と「不正な単一クォート」を区別できません。重要度は低いので任意対応です。♻️ 提案される修正
let value = match[2].trim(); if ( + value.length >= 2 && (value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'")) ) { value = value.slice(1, -1); }※ 上の例は演算子優先順位の関係で意図通りに動かないため、実際は次のように括弧でくくるのが安全です:
if ( value.length >= 2 && ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))) ) { value = value.slice(1, -1); }🤖 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 `@bin/slack-mcp-wrapper` around lines 17 - 23, The trimming logic that unquotes the parsed value uses value.startsWith/endsWith and slice(1, -1) without guarding length, which can mis-handle single-quote cases and empty quotes; update the condition around the unquote branch in the parsing routine so it first checks value.length >= 2 and only then performs the paired-quote check ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))), then call value = value.slice(1, -1) when true to safely strip matched surrounding quotes.
47-49: 💤 Low valueenv ファイルのパーミッションを確認することを推奨
SLACK_BOT_TOKENを含むslack.envを読むため、ファイルが他ユーザーから読み取り可能だとトークン漏えいに直結します。Unix 系ではモード0o600を期待しているはずなので、緩い場合に警告を出すと運用ミスを防げます。Windows では NTFS ACL が異なるためチェックは Unix 系のみが現実的です。🛡️ 提案される修正例
if (fs.existsSync(envFile)) { + if (process.platform !== 'win32') { + const mode = fs.statSync(envFile).mode & 0o777; + if (mode & 0o077) { + console.warn(`Warning: ${envFile} is readable by group/others (mode ${mode.toString(8)}). Run: chmod 600 ${envFile}`); + } + } Object.assign(process.env, parseEnv(fs.readFileSync(envFile, 'utf8'))); }🤖 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 `@bin/slack-mcp-wrapper` around lines 47 - 49, Before reading slack.env, check its Unix permission bits (use fs.statSync(envFile).mode & 0o777) only when process.platform !== 'win32' and if the mode is more permissive than 0o600 emit a visible warning (e.g., console.warn) including envFile and the current octal mode to alert operators; then proceed to read and parse the file with parseEnv and Object.assign(process.env, ...). Use the existing symbols envFile, fs.existsSync, fs.statSync, parseEnv, and process.env to locate where to insert this check so Windows (process.platform === 'win32') skips the permission test.
29-41: 💤 Low valueNode.js 18 以降の
fetchの実験的ステータスと互換性の注意このコードは
fetchをグローバルとして使用していますが、Node.js 18.0.0 から Node.js 20.x では実験的であり、Node.js 21.0.0 で安定化されました。PR ではOpenJS.NodeJS.LTS(Node 20/22) をインストールするため PR の実行環境では問題ありませんが、#!/usr/bin/env nodeのシェバンを使用しているため、ユーザーの環境で Node 18.0 未満の古いバージョンが PATH に存在する場合、ReferenceError: fetch is not definedで実行に失敗します。パッケージを CLI ツールとして配布する場合、以下のいずれかを検討してください:
package.jsonのenginesフィールドで最小バージョンを明示する ("engines": { "node": ">=18.0.0" })- 起動時に
typeof fetch === 'undefined'をチェックし、ユーザーフレンドリーなエラーメッセージを出力する🤖 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 `@bin/slack-mcp-wrapper` around lines 29 - 41, The fetchTeamId function uses the global fetch which can be missing on older Node versions; add a runtime guard before using fetch (e.g., at module top or inside fetchTeamId) that checks if typeof fetch === 'undefined' and throws or logs a clear, user-friendly error instructing the user to upgrade Node to a supported version (e.g., "Node >=18" or the project's minimum), and/or add a package.json engines field ("node": ">=18.0.0") to enforce the minimum Node version; reference fetchTeamId when adding the guard so callers get a deterministic error instead of ReferenceError.bin/merge_pdf (2)
52-58: 💤 Low value
process.exit(result.status)は finally より先に終了するNode.js では
process.exit()は同期的に終了処理を開始し、囲みのfinallyブロックは実行されますが、process.exit(null)のようにresult.statusがnull（プロセスがシグナルで終了した場合など）だとprocess.exit(null)は exit code 0 になります。シグナル終了時に成功と誤認しないよう、result.signalも考慮するか、throwしてまとめてcatch/finally経路に乗せる方が安全です。♻️ 提案される修正
- if (result.error) throw result.error; - if (result.status !== 0) process.exit(result.status); + if (result.error) throw result.error; + if (result.signal) throw new Error(`cpdf terminated by signal ${result.signal}`); + if (result.status !== 0) throw new Error(`cpdf exited with status ${result.status}`);🤖 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 `@bin/merge_pdf` around lines 52 - 58, The current spawnSync call stores its return in result and then calls process.exit(result.status), which can misinterpret null status (e.g., if result.signal is set) and bypass finally handling; instead, after the spawnSync invocation in the block using result, check result.error (keep throwing as-is), then if result.signal is set throw a new Error including result.signal and any stdout/stderr context, and if result.status !== 0 throw an Error with the exit code (do not call process.exit here); let the outer try/catch/finally handle termination so finally always runs and exit decisions (process.exit) are centralized.
38-49: ⚡ Quick win一時ファイルは
os.tmpdir()配下の一意ディレクトリに作成することを推奨現在は
process.cwd()直下に${name}_${i}.pdfを作成しているため、
- 同名ファイルが既に存在すると上書きされる（データロスのリスク）。
- 別プロセスで同じ PDF を同じ cwd で同時実行すると競合する。
cpdf実行中に Ctrl-C などで中断されると、cwd にゴミファイルが残る可能性がある。
fs.mkdtempSync(path.join(os.tmpdir(), 'merge_pdf-'))で一時ディレクトリを切り、ジョブ終了時にディレクトリごと削除すると安全です。♻️ 提案される修正の概略
-const outputDirectory = process.cwd(); const parsed = path.parse(originalPdf); -const tempFiles = []; +const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'merge_pdf-')); const inputs = [originalPdf]; try { for (let i = 2; i <= totalPages; i++) { - const newPdf = path.join(outputDirectory, `${parsed.name}_${i}.pdf`); + const newPdf = path.join(tempDir, `${parsed.name}_${i}.pdf`); fs.copyFileSync(originalPdf, newPdf); - tempFiles.push(newPdf); inputs.push(newPdf); } - const mergedOutput = path.join(outputDirectory, `${parsed.name}_${totalPages}page.pdf`); + const mergedOutput = path.join(process.cwd(), `${parsed.name}_${totalPages}page.pdf`); ... } finally { - for (const tempFile of tempFiles) { - fs.rmSync(tempFile, { force: true }); - } + fs.rmSync(tempDir, { recursive: true, force: true }); }※ 別途冒頭で
const os = require('os');の追加が必要です。🤖 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 `@bin/merge_pdf` around lines 38 - 49, The current loop writes temporary files into process.cwd() (outputDirectory) which risks collisions and leftover files; change to create a unique temp directory with os.tmpdir() via fs.mkdtempSync(path.join(os.tmpdir(), 'merge_pdf-')) (add const os = require('os')), then use that tempDir when creating newPdf (path.join(tempDir, `${parsed.name}_${i}.pdf`)), keep pushing those paths into tempFiles and inputs as before, and ensure the tempDir (or all tempFiles) are removed at job end (including on error/interrupt) to avoid leftovers and conflicts.bin/lll (2)
53-58: 💤 Low value
PATHEXTデフォルト値と空要素の扱いを確認挙動上ほぼ問題ありませんが、念のため:
PATHEXTの Windows デフォルトは典型的に.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSCを含みます。フォールバック値が短すぎるとPATHEXT未設定の異常系で.jsなどを実行可能と判定できません。ただし通常process.env.PATHEXTは設定済みなので影響は限定的です。- 末尾の
;等で空要素が出るとpath.extname()の結果（''）にマッチして全ファイルが実行可能扱いになる可能性があります。♻️ 提案される修正
function isWindowsExecutable(filePath) { - const pathExt = (process.env.PATHEXT || '.COM;.EXE;.BAT;.CMD;.PS1') + const pathExt = (process.env.PATHEXT || '.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC') .split(';') - .map(ext => ext.toLowerCase()); - return pathExt.includes(path.extname(filePath).toLowerCase()); + .map(ext => ext.trim().toLowerCase()) + .filter(Boolean); + const ext = path.extname(filePath).toLowerCase(); + return ext !== '' && pathExt.includes(ext); }🤖 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 `@bin/lll` around lines 53 - 58, The isWindowsExecutable function uses an incomplete PATHEXT fallback and doesn't guard against empty elements, which can make every file appear executable when path.extname(...) is ''. Update the PATHEXT fallback to the full typical Windows list (e.g. ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC") and when building pathExt normalize by splitting on ';', trimming and lowercasing items, and filtering out empty strings; then compare pathExt against path.extname(filePath).toLowerCase() as before so empty extensions won't match and common script extensions like .js are covered.
23-26: 💤 Low valueWindows 分岐の
uidCache/gidCacheのキャッシュ意図に注意Windows では
fs.lstatSync()のuid/gidは基本的に 0 になりますが、もし異なるuidが渡されても、キャッシュ機構は uid ごとにos.userInfo().username（= 常に現在ユーザー）を入れるため、結果として常に現在ユーザー名を返します。挙動上は問題ありませんが、Windows では呼び出し前にprocess.platformを 1 度だけチェックして単一の値を返す（キャッシュを回避する）ほうが意図が明確になります。低リワードなので任意対応です。🤖 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 `@bin/lll` around lines 23 - 26, On Windows the current code sets uidCache/gidCache per-uid with os.userInfo().username which always yields the same user; change this to explicitly short-circuit the platform check and return a single cached Windows username instead of caching by uid/gid. Locate the Windows branch (process.platform === 'win32') where uidCache.set(uid, os.userInfo().username) and gidCache are handled and replace the per-id cache logic with a single cached value (e.g., cachedWindowsUser) or a dedicated 'windows' key so the intent is clear and you avoid misleading per-uid entries.win/setup_win.ps1 (2)
3-4: 💤 Low value
winget importにも--accept-source-agreementsを付与する検討
winget installには両方のフラグが付与されていますが、winget import側には--accept-package-agreementsのみで、ソースのライセンス同意プロンプトに引っかかると非対話実行が止まる可能性があります。エラー時の挙動も含めて、両コマンドにフラグを揃えると無人セットアップで安全です。♻️ 提案される修正
-winget import --accept-package-agreements (Join-Path $ScriptDir "winget.json") +winget import --accept-package-agreements --accept-source-agreements (Join-Path $ScriptDir "winget.json")🤖 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 `@win/setup_win.ps1` around lines 3 - 4, Update the winget invocation so both commands consistently include both agreement flags: add --accept-source-agreements to the existing winget import call (which currently only has --accept-package-agreements) so it matches the winget install call; ensure the two commands (winget import and winget install --exact --id OpenJS.NodeJS.LTS) each include --accept-package-agreements and --accept-source-agreements to avoid interactive source/license prompts during unattended runs.
1-1: ⚡ Quick win
$PSScriptRootの利用を推奨スクリプトファイル内でスクリプト自身のディレクトリを取得する場合、PowerShell 3.0 以降では自動変数
$PSScriptRootが利用可能で、これが標準的・冪等なイディオムです。$MyInvocation.MyCommand.Pathは dot-source された場合や一部の実行コンテキストで$nullになることがあり、その場合Split-Pathが$nullを返してJoin-Pathがカレントディレクトリに対して解決される可能性があります。♻️ 提案される修正
-$ScriptDir = Split-Path -Parent $MyInvocation.MyCommand.Path - -winget import --accept-package-agreements (Join-Path $ScriptDir "winget.json") +winget import --accept-package-agreements (Join-Path $PSScriptRoot "winget.json")🤖 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 `@win/setup_win.ps1` at line 1, 現在の $ScriptDir = Split-Path -Parent $MyInvocation.MyCommand.Path は dot-sourcing や特殊な実行コンテキストで $MyInvocation.MyCommand.Path が $null になるため脆弱です；代わりにスクリプトのルートを表す組み込み変数 $PSScriptRoot を使うように置き換えてください（参照箇所: $ScriptDir を設定している箇所や Join-Path を使っている箇所を更新）。必要に応じて PowerShell 2.0 互換を保つならば $PSScriptRoot が未定義のときのフォールバック（例: $MyInvocation.MyCommand.Path ベースの Split-Path を条件付きで使う）を追加してください。win/scripts/install-bin-shims.ps1 (1)
13-14: ⚡ Quick winシバン検出の正規表現が広すぎる
"node"はnodemonやnodejs-legacyといった文字列にもマッチし、"bash|zsh|sh"のsh部分はfishの"sh"にも一致するため、意図しないシムが生成される可能性があります（例:#!/usr/bin/env fish→bashシムが生成される）。^#!でシバン行に限定し、\bで単語境界を付けると誤検出を防げます。🛠️ 修正案
- if ($firstLine -match "node") { return "node" } - if ($firstLine -match "bash|zsh|sh") { return "bash" } + if ($firstLine -match '^#!.*\bnode\b') { return "node" } + if ($firstLine -match '^#!.*\b(bash|zsh|sh)\b') { return "bash" }🤖 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 `@win/scripts/install-bin-shims.ps1` around lines 13 - 14, The shebang detection in install-bin-shims.ps1 is too broad: tighten the regexes used on $firstLine (the two if (...) -match checks) to only match a shebang line and whole words; e.g., match a shebang prefix like ^#! and use word boundaries (\b) around node and around shell names (sh, bash, zsh) so strings like nodemon, nodejs-legacy, or fish don't trigger incorrect shims—update the two if ($firstLine -match ...) conditions accordingly.
🤖 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 `@bin/cleanup`:
- Line 78: fs.writeFileSync currently writes to path.join(process.cwd(),
'SSLKEYLOGFILE.log'), which depends on the caller's working directory and can
create the file in unexpected locations; change the write target to the script
directory by replacing process.cwd() with __dirname (e.g., use
path.join(__dirname, 'SSLKEYLOGFILE.log')) so fs.writeFileSync writes to the
cleanup script's own directory, ensuring a stable, predictable location for
SSLKEYLOGFILE.log.
- Around line 60-76: The code currently lacks error handling: wrap the directory
read in matchingTargets (where fs.readdirSync(home) is used) in a try/catch so
permission or fs errors return an empty array (or log and continue) instead of
throwing; also wrap the rm call inside the main for-loop (where
fs.rmSync(target, { force: true, recursive: true }) is invoked) in a try/catch
that logs warnings for failures (including EPERM/EACCES) and continues to the
next target; reference matchingTargets, wildcardToRegExp, the home variable, the
targets loop, fs.readdirSync and fs.rmSync when making these changes.
In `@bin/extract-m3u8`:
- Around line 28-56: The code can leak a Chromium process if page.goto,
page.evaluate, or page.waitForTimeout throws because await browser.close() is
only called on the happy path; wrap the main logic that uses
browser/context/page (the async IIFE body that creates chromium.launch(),
browser.newContext(), context.newPage(), sets page.on(...), calls page.goto,
page.evaluate, and page.waitForTimeout) in a try/finally and ensure await
browser.close() (and optionally await context.close() if created separately) is
called in the finally block; also guard the finally with a check that browser is
defined before calling browser.close() so errors during setup still trigger
cleanup.
In `@bin/git-diffn`:
- Around line 154-161: The script currently sets awk_bin="$(command -v gawk
2>/dev/null || command -v awk 2>/dev/null)" but the embedded AWK code uses
gawk-only extensions (3-arg match), so replace the fallback logic to require
gawk explicitly: find the awk_bin assignment and change it to locate only gawk
(e.g., awk_bin="$(command -v gawk 2>/dev/null)"), update the error message to
say "gawk is required" and exit non-zero if not found, and keep the rest of the
pipeline using "$awk_bin" so the script fails fast rather than silently falling
back to an incompatible awk.
In `@bin/histuniq`:
- Around line 10-14: The write path currently uses writeFileSync and lacks error
handling, risking partial/empty .zsh_history replacement; change the write logic
that targets the same filename variable to perform an atomic write: create a
temp file in the same directory (e.g., filename + a unique suffix), write the
data to that temp file and fsync it (or open/write/fsync/close), then rename the
temp file to filename (fs.renameSync) so replace is atomic; wrap the whole
sequence in try/catch to ensure on any error you do not overwrite the original
(cleanup the temp file, log the error, and exit non‑zero) and apply the same
pattern where writeFileSync is currently called.
In `@bin/lib/clipboard.js`:
- Around line 15-16: Replace the Windows branch that calls clip.exe with a
PowerShell-based clipboard call: update the code in clipboard.js where
process.platform === 'win32' calls run('clip', [], text) to instead invoke
PowerShell's Set-Clipboard on an STA thread and pipe the UTF-8 stdin into it;
use the existing run(...) helper to execute PowerShell with the -STA flag and
pass text via stdin so non-ASCII (Japanese) characters are preserved. Ensure you
only change the Windows branch (the process.platform === 'win32' block) and keep
other platforms unchanged.
In `@bin/merge_pdf`:
- Around line 19-23: 現在のチェック treats any help/usage case the same: it calls
showHelp() then process.exit(0) even for invalid argument counts; change the
logic so that when args[0] === '-h' you call showHelp() and process.exit(0), but
when args.length !== 2 (invalid usage) call showHelp() and process.exit(1)
(non‑zero) so scripts/CI can detect failure; update the condition around args,
showHelp(), and process.exit(...) accordingly.
In `@bin/paste-img`:
- Around line 119-122: User-supplied dir is stored raw (dir = args[++i]) which
causes mismatches with path.join('docs','images') on Windows; change the
'-d'/'--dir' case to normalize the input path (e.g., dir =
path.normalize(args[++i])) so it uses the same platform-specific separators as
path.join and the later comparison dir === path.join('docs','images') will
behave correctly.
- Around line 80-98: The spawnSync call uses '-Command' which prevents binding
of positional params to the PowerShell script; update the invocation to use
'-File' (or '-CommandWithArgs' on PS7+) so filePath/width/raw are passed as real
args: change the arguments array in the spawnSync call (the one referencing
script, filePath, String(width), String(raw)) to use '-File' before script
(i.e., ['-NoProfile','-STA','-ExecutionPolicy','Bypass','-File', script,
filePath, String(width), String(raw)]), keep stdio/encoding/windowsHide as-is,
and ensure the PowerShell script's
param([string]$Path,[int]$Width,[string]$RawFlag) will receive those values;
after this the existing result.stdout.match(/RESIZED:(\d+)/) logic remains
valid.
In `@bin/pdf-compress`:
- Around line 32-39: The run() helper incorrectly calls
process.exit(result.status) and treats result.status === null (signal) as
success, which bypasses finally cleanup and can leave tmpGs/tmpPs orphaned;
change run(command, args) to not call process.exit but instead throw an Error on
any non-zero exit or signal (use result.status !== 0 || result.signal to detect
failure) so callers can catch and clean up, and/or register a process.on('exit')
handler at module init to synchronously remove tmpGs/tmpPs as a last-resort
cleanup; update call sites (e.g., where run(pdf2ps, [input, tmpPs]),
run(gs,...), run(qpdf,...)) to catch errors and perform fs.rmSync for
tmpPs/tmpGs in catch/finally rather than relying on run to exit the process.
In `@bin/watch-downloads-copy`:
- Around line 17-23: isProbablyText currently returns true for empty files
(sample.length === 0), causing empty downloads to be treated as text and
overwrite the clipboard; change isProbablyText (and/or its early checks on buf)
so that an empty file returns false (i.e., treat zero-length files as
non-text/ignore) instead of true to avoid copying empty strings to the
clipboard; update the logic in function isProbablyText to return false when
buf.length or sample.length is 0 and keep the existing binary/controls checks
intact.
---
Nitpick comments:
In `@bin/cleanup`:
- Around line 60-67: matchingTargets reads the home directory every time it's
called for a pattern, causing repeated I/O; change it to read and cache the
directory listing once and reuse it for subsequent calls by either (a) adding a
simple per-home cache inside matchingTargets (e.g., store fs.readdirSync(home)
in a module-scoped Map keyed by home) or (b) moving the single
fs.readdirSync(home) call to the caller and passing the names array into
matchingTargets; then use that cached names array to .filter(name =>
wildcardToRegExp(pattern).test(name)) and .map to full paths so each pattern no
longer triggers a full readdirSync.
In `@bin/lll`:
- Around line 53-58: The isWindowsExecutable function uses an incomplete PATHEXT
fallback and doesn't guard against empty elements, which can make every file
appear executable when path.extname(...) is ''. Update the PATHEXT fallback to
the full typical Windows list (e.g.
".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC") and when building
pathExt normalize by splitting on ';', trimming and lowercasing items, and
filtering out empty strings; then compare pathExt against
path.extname(filePath).toLowerCase() as before so empty extensions won't match
and common script extensions like .js are covered.
- Around line 23-26: On Windows the current code sets uidCache/gidCache per-uid
with os.userInfo().username which always yields the same user; change this to
explicitly short-circuit the platform check and return a single cached Windows
username instead of caching by uid/gid. Locate the Windows branch
(process.platform === 'win32') where uidCache.set(uid, os.userInfo().username)
and gidCache are handled and replace the per-id cache logic with a single cached
value (e.g., cachedWindowsUser) or a dedicated 'windows' key so the intent is
clear and you avoid misleading per-uid entries.
In `@bin/merge_pdf`:
- Around line 52-58: The current spawnSync call stores its return in result and
then calls process.exit(result.status), which can misinterpret null status
(e.g., if result.signal is set) and bypass finally handling; instead, after the
spawnSync invocation in the block using result, check result.error (keep
throwing as-is), then if result.signal is set throw a new Error including
result.signal and any stdout/stderr context, and if result.status !== 0 throw an
Error with the exit code (do not call process.exit here); let the outer
try/catch/finally handle termination so finally always runs and exit decisions
(process.exit) are centralized.
- Around line 38-49: The current loop writes temporary files into process.cwd()
(outputDirectory) which risks collisions and leftover files; change to create a
unique temp directory with os.tmpdir() via fs.mkdtempSync(path.join(os.tmpdir(),
'merge_pdf-')) (add const os = require('os')), then use that tempDir when
creating newPdf (path.join(tempDir, `${parsed.name}_${i}.pdf`)), keep pushing
those paths into tempFiles and inputs as before, and ensure the tempDir (or all
tempFiles) are removed at job end (including on error/interrupt) to avoid
leftovers and conflicts.
In `@bin/slack-mcp-wrapper`:
- Around line 17-23: The trimming logic that unquotes the parsed value uses
value.startsWith/endsWith and slice(1, -1) without guarding length, which can
mis-handle single-quote cases and empty quotes; update the condition around the
unquote branch in the parsing routine so it first checks value.length >= 2 and
only then performs the paired-quote check ((value.startsWith('"') &&
value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))), then
call value = value.slice(1, -1) when true to safely strip matched surrounding
quotes.
- Around line 47-49: Before reading slack.env, check its Unix permission bits
(use fs.statSync(envFile).mode & 0o777) only when process.platform !== 'win32'
and if the mode is more permissive than 0o600 emit a visible warning (e.g.,
console.warn) including envFile and the current octal mode to alert operators;
then proceed to read and parse the file with parseEnv and
Object.assign(process.env, ...). Use the existing symbols envFile,
fs.existsSync, fs.statSync, parseEnv, and process.env to locate where to insert
this check so Windows (process.platform === 'win32') skips the permission test.
- Around line 29-41: The fetchTeamId function uses the global fetch which can be
missing on older Node versions; add a runtime guard before using fetch (e.g., at
module top or inside fetchTeamId) that checks if typeof fetch === 'undefined'
and throws or logs a clear, user-friendly error instructing the user to upgrade
Node to a supported version (e.g., "Node >=18" or the project's minimum), and/or
add a package.json engines field ("node": ">=18.0.0") to enforce the minimum
Node version; reference fetchTeamId when adding the guard so callers get a
deterministic error instead of ReferenceError.
In `@bin/watch-downloads-copy`:
- Line 18: The code reads the same file twice (fs.readFileSync at buf and again
later) causing extra I/O; change the flow so the file is read once into the
buffer and that buffer is reused: modify the isProbablyText call/signature to
accept a Buffer (or pass the already-read buf) and, where you currently re-read
the file around Line 35, use the cached buf and convert it to a UTF-8 string
only when needed; update references to buf, filePath, and isProbablyText so
text-detection and any subsequent string conversion reuse the single buffer
read.
In `@win/scripts/install-bin-shims.ps1`:
- Around line 13-14: The shebang detection in install-bin-shims.ps1 is too
broad: tighten the regexes used on $firstLine (the two if (...) -match checks)
to only match a shebang line and whole words; e.g., match a shebang prefix like
^#! and use word boundaries (\b) around node and around shell names (sh, bash,
zsh) so strings like nodemon, nodejs-legacy, or fish don't trigger incorrect
shims—update the two if ($firstLine -match ...) conditions accordingly.
In `@win/setup_win.ps1`:
- Around line 3-4: Update the winget invocation so both commands consistently
include both agreement flags: add --accept-source-agreements to the existing
winget import call (which currently only has --accept-package-agreements) so it
matches the winget install call; ensure the two commands (winget import and
winget install --exact --id OpenJS.NodeJS.LTS) each include
--accept-package-agreements and --accept-source-agreements to avoid interactive
source/license prompts during unattended runs.
- Line 1: 現在の $ScriptDir = Split-Path -Parent $MyInvocation.MyCommand.Path は
dot-sourcing や特殊な実行コンテキストで $MyInvocation.MyCommand.Path が $null
になるため脆弱です；代わりにスクリプトのルートを表す組み込み変数 $PSScriptRoot を使うように置き換えてください（参照箇所: $ScriptDir
を設定している箇所や Join-Path を使っている箇所を更新）。必要に応じて PowerShell 2.0 互換を保つならば $PSScriptRoot
が未定義のときのフォールバック（例: $MyInvocation.MyCommand.Path ベースの Split-Path
を条件付きで使う）を追加してください。
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66143e24-35ea-4375-9aef-58d0a2578455
📒 Files selected for processing (21)
README.mdbin/cleanupbin/emojibin/extract-m3u8bin/git-diffnbin/histuniqbin/lib/clipboard.jsbin/lllbin/merge_pdfbin/osbin/paste-imgbin/pdf-compressbin/slack-mcp-wrapperbin/txt-to-markmapbin/watch-downloads-copywin/README.mdwin/bin/.gitignorewin/exports.ps1win/scripts/deploy.ps1win/scripts/install-bin-shims.ps1win/setup_win.ps1
Comment on lines
+60
to
+76
| function matchingTargets(home, pattern) { | ||
| if (!pattern.includes('*')) return [path.join(home, pattern)]; | ||
|
|
||
| const re = wildcardToRegExp(pattern); | ||
| return fs.readdirSync(home) | ||
| .filter(name => re.test(name)) | ||
| .map(name => path.join(home, name)); | ||
| } | ||
|
|
||
| const home = os.homedir(); | ||
|
|
||
| for target in $target_files | ||
| do | ||
| echo "rm -rf $HOME/$target" | ||
| rm -rf "$HOME/$target" | ||
| done | ||
| for (const pattern of targets) { | ||
| for (const target of matchingTargets(home, pattern)) { | ||
| console.log(`rm -rf ${target}`); | ||
| fs.rmSync(target, { force: true, recursive: true }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
エラーハンドリングの欠如によりスクリプトが途中でクラッシュする可能性があります
fs.readdirSync(home)（Line 64）はパーミッションエラーや予期せぬ状態で例外を投げます。また、fs.rmSync（Line 74）の force: true は ENOENT のみを抑制し、EPERM / EACCES などは抑制しません。いずれかで例外が発生すると、残りの削除対象がスキップされたまま中断します。
🛡️ 修正案：try/catch を追加してスクリプトを継続させる
for (const pattern of targets) {
- for (const target of matchingTargets(home, pattern)) {
- console.log(`rm -rf ${target}`);
- fs.rmSync(target, { force: true, recursive: true });
- }
+ let resolved;
+ try {
+ resolved = matchingTargets(home, pattern);
+ } catch (err) {
+ console.error(`Failed to resolve pattern "${pattern}": ${err.message}`);
+ continue;
+ }
+ for (const target of resolved) {
+ try {
+ console.log(`rm -rf ${target}`);
+ fs.rmSync(target, { force: true, recursive: true });
+ } catch (err) {
+ console.error(`Failed to remove ${target}: ${err.message}`);
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function matchingTargets(home, pattern) { | |
| if (!pattern.includes('*')) return [path.join(home, pattern)]; | |
| const re = wildcardToRegExp(pattern); | |
| return fs.readdirSync(home) | |
| .filter(name => re.test(name)) | |
| .map(name => path.join(home, name)); | |
| } | |
| const home = os.homedir(); | |
| for target in $target_files | |
| do | |
| echo "rm -rf $HOME/$target" | |
| rm -rf "$HOME/$target" | |
| done | |
| for (const pattern of targets) { | |
| for (const target of matchingTargets(home, pattern)) { | |
| console.log(`rm -rf ${target}`); | |
| fs.rmSync(target, { force: true, recursive: true }); | |
| } | |
| } | |
| function matchingTargets(home, pattern) { | |
| if (!pattern.includes('*')) return [path.join(home, pattern)]; | |
| const re = wildcardToRegExp(pattern); | |
| return fs.readdirSync(home) | |
| .filter(name => re.test(name)) | |
| .map(name => path.join(home, name)); | |
| } | |
| const home = os.homedir(); | |
| for (const pattern of targets) { | |
| let resolved; | |
| try { | |
| resolved = matchingTargets(home, pattern); | |
| } catch (err) { | |
| console.error(`Failed to resolve pattern "${pattern}": ${err.message}`); | |
| continue; | |
| } | |
| for (const target of resolved) { | |
| try { | |
| console.log(`rm -rf ${target}`); | |
| fs.rmSync(target, { force: true, recursive: true }); | |
| } catch (err) { | |
| console.error(`Failed to remove ${target}: ${err.message}`); | |
| } | |
| } | |
| } |
🤖 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 `@bin/cleanup` around lines 60 - 76, The code currently lacks error handling:
wrap the directory read in matchingTargets (where fs.readdirSync(home) is used)
in a try/catch so permission or fs errors return an empty array (or log and
continue) instead of throwing; also wrap the rm call inside the main for-loop
(where fs.rmSync(target, { force: true, recursive: true }) is invoked) in a
try/catch that logs warnings for failures (including EPERM/EACCES) and continues
to the next target; reference matchingTargets, wildcardToRegExp, the home
variable, the targets loop, fs.readdirSync and fs.rmSync when making these
changes.
| } | ||
|
|
||
| cp /dev/null SSLKEYLOGFILE.log | ||
| fs.writeFileSync(path.join(process.cwd(), 'SSLKEYLOGFILE.log'), ''); |
Contributor
There was a problem hiding this comment.
process.cwd() への書き込みは実行ディレクトリに依存して予期しない場所に SSLKEYLOGFILE.log を作成します
スクリプトが dotfiles リポジトリルート以外から呼び出された場合、ファイルが意図しない場所に作成されます。__dirname（スクリプト自身のディレクトリ）を使う方が安定します。
🔧 修正案
-fs.writeFileSync(path.join(process.cwd(), 'SSLKEYLOGFILE.log'), '');
+fs.writeFileSync(path.join(__dirname, 'SSLKEYLOGFILE.log'), '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fs.writeFileSync(path.join(process.cwd(), 'SSLKEYLOGFILE.log'), ''); | |
| fs.writeFileSync(path.join(__dirname, 'SSLKEYLOGFILE.log'), ''); |
🤖 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 `@bin/cleanup` at line 78, fs.writeFileSync currently writes to
path.join(process.cwd(), 'SSLKEYLOGFILE.log'), which depends on the caller's
working directory and can create the file in unexpected locations; change the
write target to the script directory by replacing process.cwd() with __dirname
(e.g., use path.join(__dirname, 'SSLKEYLOGFILE.log')) so fs.writeFileSync writes
to the cleanup script's own directory, ensuring a stable, predictable location
for SSLKEYLOGFILE.log.
Comment on lines
+28
to
56
| (async () => { | ||
| const { chromium } = loadPlaywright(); | ||
| const browser = await chromium.launch(); | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| page.on("response", (res) => { | ||
| if (res.url().includes(".m3u8")) { | ||
| m3u8Urls.add(res.url()); | ||
| } | ||
| }); | ||
|
|
||
| await page.goto(url, { waitUntil: "networkidle" }); | ||
|
|
||
| await page.evaluate(async () => { | ||
| for (let i = 0; i < document.body.scrollHeight; i += 500) { | ||
| window.scrollTo(0, i); | ||
| await new Promise((r) => setTimeout(r, 300)); | ||
| } | ||
| }); | ||
|
|
||
| await page.waitForTimeout(3000); | ||
| await browser.close(); | ||
|
|
||
| [...m3u8Urls].forEach((u) => console.log(u)); | ||
| })().catch((error) => { | ||
| console.error(error.message); | ||
| process.exit(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
例外発生時に browser がクローズされず Chromium プロセスがリークする
page.goto、page.evaluate、page.waitForTimeout のいずれかが例外を投げると、行 50 の browser.close() までたどり着かず、Chromium のプロセスがバックグラウンドに残ったままになります。try/finally でクリーンアップを保証してください。
🛡️ 提案される修正
(async () => {
const { chromium } = loadPlaywright();
const browser = await chromium.launch();
- const context = await browser.newContext();
- const page = await context.newPage();
-
- page.on("response", (res) => {
- if (res.url().includes(".m3u8")) {
- m3u8Urls.add(res.url());
- }
- });
-
- await page.goto(url, { waitUntil: "networkidle" });
-
- await page.evaluate(async () => {
- for (let i = 0; i < document.body.scrollHeight; i += 500) {
- window.scrollTo(0, i);
- await new Promise((r) => setTimeout(r, 300));
- }
- });
-
- await page.waitForTimeout(3000);
- await browser.close();
-
- [...m3u8Urls].forEach((u) => console.log(u));
+ try {
+ const context = await browser.newContext();
+ const page = await context.newPage();
+
+ page.on("response", (res) => {
+ if (res.url().includes(".m3u8")) {
+ m3u8Urls.add(res.url());
+ }
+ });
+
+ await page.goto(url, { waitUntil: "networkidle" });
+
+ await page.evaluate(async () => {
+ for (let i = 0; i < document.body.scrollHeight; i += 500) {
+ window.scrollTo(0, i);
+ await new Promise((r) => setTimeout(r, 300));
+ }
+ });
+
+ await page.waitForTimeout(3000);
+ } finally {
+ await browser.close();
+ }
+
+ [...m3u8Urls].forEach((u) => console.log(u));
})().catch((error) => {🤖 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 `@bin/extract-m3u8` around lines 28 - 56, The code can leak a Chromium process
if page.goto, page.evaluate, or page.waitForTimeout throws because await
browser.close() is only called on the happy path; wrap the main logic that uses
browser/context/page (the async IIFE body that creates chromium.launch(),
browser.newContext(), context.newPage(), sets page.on(...), calls page.goto,
page.evaluate, and page.waitForTimeout) in a try/finally and ensure await
browser.close() (and optionally await context.close() if created separately) is
called in the finally block; also guard the finally with a check that browser is
defined before calling browser.close() so errors during setup still trigger
cleanup.
Comment on lines
+154
to
+161
| awk_bin="$(command -v gawk 2>/dev/null || command -v awk 2>/dev/null)" | ||
| if [[ -z "$awk_bin" ]]; then | ||
| echo "git-diffn: awk or gawk is required." >&2 | ||
| exit 127 | ||
| fi | ||
|
|
||
| git diff --color=always "$@" | \ | ||
| gawk \ | ||
| "$awk_bin" \ |
Contributor
There was a problem hiding this comment.
awk フォールバックは gawk 拡張で実行時エラーになる可能性
このスクリプトの awk プログラムは 3 引数版 match(string, regex, array) という gawk 拡張機能を使用しています（行 198, 226, 257, 285）。gawk が見つからず標準 awk（BSD awk / mawk など）にフォールバックした場合、構文/関数エラーで実行時に失敗します。ファイル冒頭の FUTURE WORK コメントにも同様の懸念が記載されています。
「最低限 awk はあるかも」という安全網ではなく、実態としては gawk が必須なので、fallback が誤った成功印象を与えるよりは、gawk のみを要求して明確なエラーメッセージを出す方が安全です（または awk プログラムを完全に POSIX awk 互換に書き換える）。
🛡️ 提案される修正（gawk 必須化）
-awk_bin="$(command -v gawk 2>/dev/null || command -v awk 2>/dev/null)"
-if [[ -z "$awk_bin" ]]; then
- echo "git-diffn: awk or gawk is required." >&2
- exit 127
-fi
-
-git diff --color=always "$@" | \
-"$awk_bin" \
+if ! command -v gawk >/dev/null 2>&1; then
+ echo "git-diffn: gawk is required (this script uses gawk-specific match() with an array argument)." >&2
+ echo " macOS: brew install gawk" >&2
+ echo " Ubuntu: sudo apt-get install gawk" >&2
+ exit 127
+fi
+
+git diff --color=always "$@" | \
+gawk \🤖 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 `@bin/git-diffn` around lines 154 - 161, The script currently sets
awk_bin="$(command -v gawk 2>/dev/null || command -v awk 2>/dev/null)" but the
embedded AWK code uses gawk-only extensions (3-arg match), so replace the
fallback logic to require gawk explicitly: find the awk_bin assignment and
change it to locate only gawk (e.g., awk_bin="$(command -v gawk 2>/dev/null)"),
update the error message to say "gawk is required" and exit non-zero if not
found, and keep the rest of the pipeline using "$awk_bin" so the script fails
fast rather than silently falling back to an incompatible awk.
Comment on lines
+10
to
+14
| if (!fs.existsSync(filename)) { | ||
| console.log(`${filename} not found`); | ||
| process.exit(0); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
ヒストリファイルへの書き込みにエラーハンドリングがなく、データ損失のリスクがあります。
読み込み側に存在チェック（lines 10-14）が追加されたことで安全性が高まりましたが、書き込み側（lines 47-49）は依然として無防備です。writeFileSync が途中で例外を投げた場合（ディスクフル・パーミッションエラー等）、.zsh_history が空または不完全な状態で上書きされ、シェル履歴が永久に失われます。
アトミックな書き込み（一時ファイルへ書いてからリネーム）とエラーハンドリングを組み合わせることを推奨します。
🛡️ 修正案（アトミック書き込み + エラーハンドリング）
+const tmpFile = `${filename}.tmp`;
+try {
-fs.writeFileSync(filename, result);
-console.log(`${filename} saved`);
-fs.writeFileSync(`${filename}.bk`, result);
-console.log(`${filename}.bk saved`);
+ fs.writeFileSync(tmpFile, result);
+ fs.renameSync(tmpFile, filename);
+ console.log(`${filename} saved`);
+ fs.writeFileSync(`${filename}.bk`, result);
+ console.log(`${filename}.bk saved`);
+} catch (err) {
+ console.error(`Failed to write history: ${err.message}`);
+ try { fs.unlinkSync(tmpFile); } catch {}
+ process.exit(1);
+}Also applies to: 47-50
🤖 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 `@bin/histuniq` around lines 10 - 14, The write path currently uses
writeFileSync and lacks error handling, risking partial/empty .zsh_history
replacement; change the write logic that targets the same filename variable to
perform an atomic write: create a temp file in the same directory (e.g.,
filename + a unique suffix), write the data to that temp file and fsync it (or
open/write/fsync/close), then rename the temp file to filename (fs.renameSync)
so replace is atomic; wrap the whole sequence in try/catch to ensure on any
error you do not overwrite the original (cleanup the temp file, log the error,
and exit non‑zero) and apply the same pattern where writeFileSync is currently
called.
Comment on lines
+19
to
+23
| const args = process.argv.slice(2); | ||
| if (args.length !== 2 || args[0] === '-h') { | ||
| showHelp(); | ||
| process.exit(0); | ||
| } |
Contributor
There was a problem hiding this comment.
引数の使い方エラー時に exit 0 するため CI/スクリプトから検出できない
args.length !== 2 のような誤った引数（例: 1 個や 3 個以上）でも、ヘルプを表示した後 exit 0 で抜けています。これは「-h で意図的にヘルプを出した」場合と「使い方を間違えた」場合の終了コードが区別できないため、シェルや CI から失敗を検出できません。
🛠️ 提案される修正
-const args = process.argv.slice(2);
-if (args.length !== 2 || args[0] === '-h') {
- showHelp();
- process.exit(0);
-}
+const args = process.argv.slice(2);
+if (args[0] === '-h') {
+ showHelp();
+ process.exit(0);
+}
+if (args.length !== 2) {
+ showHelp();
+ process.exit(1);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const args = process.argv.slice(2); | |
| if (args.length !== 2 || args[0] === '-h') { | |
| showHelp(); | |
| process.exit(0); | |
| } | |
| const args = process.argv.slice(2); | |
| if (args[0] === '-h') { | |
| showHelp(); | |
| process.exit(0); | |
| } | |
| if (args.length !== 2) { | |
| showHelp(); | |
| process.exit(1); | |
| } |
🤖 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 `@bin/merge_pdf` around lines 19 - 23, 現在のチェック treats any help/usage case the
same: it calls showHelp() then process.exit(0) even for invalid argument counts;
change the logic so that when args[0] === '-h' you call showHelp() and
process.exit(0), but when args.length !== 2 (invalid usage) call showHelp() and
process.exit(1) (non‑zero) so scripts/CI can detect failure; update the
condition around args, showHelp(), and process.exit(...) accordingly.
Comment on lines
+80
to
+98
| const result = spawnSync('powershell.exe', [ | ||
| '-NoProfile', | ||
| '-STA', | ||
| '-ExecutionPolicy', | ||
| 'Bypass', | ||
| '-Command', | ||
| script, | ||
| filePath, | ||
| String(width), | ||
| String(raw), | ||
| ], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| windowsHide: true, | ||
| }); | ||
|
|
||
| if (result.error || result.status !== 0) return { ok: false }; | ||
| const match = result.stdout.match(/RESIZED:(\d+)/); | ||
| return { ok: true, resizedFrom: match ? Number(match[1]) : undefined }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# PowerShell が利用可能な Windows 環境での検証コマンド例
# (このスクリプト自体はサンドボックス外で実行すること)
# powershell.exe -NoProfile -Command "param([string]$P,[int]$W,[string]$R) Write-Output `"P=$P W=$W R=$R`"" "hello" "800" "false"
# 期待出力: P=hello W=800 R=false
echo "Verify on a Windows machine with PowerShell 5.1 and 7.x"Repository: shuntagami/dotfiles
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check the file size and content
wc -l bin/paste-imgRepository: shuntagami/dotfiles
Length of output: 81
🏁 Script executed:
#!/bin/bash
cat -n bin/paste-imgRepository: shuntagami/dotfiles
Length of output: 6254
🌐 Web query:
PowerShell -Command argument binding positional parameters
💡 Result:
In PowerShell, positional parameters are those declared with a non-negative Position in the parameter attribute. Their values are bound based on the order of the unnamed (non-dash-prefixed) arguments: the first unnamed argument maps to the first positional parameter, the second unnamed argument to the second positional parameter, and so on. By default, parameters are named unless Position is specified, and you can mix named and positional parameters; if you mix them, PowerShell processes named arguments first, then maps the remaining unnamed arguments to positional parameters. [1][2] For parameter binding in general, PowerShell starts by binding command-line arguments (named/positional), and only after that does it bind pipeline input to any remaining unbound parameters. [3][4] What this means specifically for “-Command” argument binding - The “-Command” usage is hosted by pwsh.exe/powershell.exe (CLI) and involves additional parsing/execution steps. When -Command is used with a string, that string is executed as PowerShell code. Any text/arguments after -Command are interpreted as part of the command to execute, and for pwsh.exe, the -Command string must be the last parameter because all following CLI tokens are treated as command arguments. [5][6] - Therefore, any “positional parameter” binding you observe in a command invoked via -Command is determined by how the executed command (the -Command string) contains unnamed arguments and which parameters in that executed command are declared with Position. (In other words: -Command doesn’t change the core rules above; it changes what gets executed.) [5][6][2] Practical guidance - Prefer named parameters (e.g., -ParamName value) when invoking functions/scripts/cmdlets to avoid ambiguity from positional ordering. [1][2] - If binding seems unexpected, tracing/visualization of parameter binding can show that binding begins with command-line parameters before pipeline input, and how positional tokens get bound. [4]
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/scripting/developer/cmdlet/types-of-cmdlet-parameters?view=powershell-7.5
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parameters?view=powershell-7.6
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parameter_binding?view=powershell-7.5
- 4: https://learn.microsoft.com/en-us/powershell/scripting/learn/deep-dives/visualize-parameter-binding?view=powershell-7.6
- 5: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_powershell_exe?view=powershell-5.1&viewFallbackFrom=powershell-Microsoft.PowerShell.Core
- 6: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pwsh?view=powershell-7.4
🌐 Web query:
node spawnSync powershell -Command argument passing positional parameters
💡 Result:
In Node.js, if you run PowerShell with -Command, you generally cannot pass true positional parameters after -Command by just appending extra args in spawn/spawnSync. Microsoft’s PowerShell CLI help explicitly says that when the value of -Command is a string, -Command must be the last parameter because “all arguments following it are interpreted as part of the command to execute” [1]. What to do instead (reliable patterns) 1) Use pwsh -CommandWithArgs / -cwa PowerShell 7+ provides -CommandWithArgs / -cwa, which “populates the $args built-in variable” for the additional arguments [1]. Then those “positional” values are accessible as $args inside your code. 2) Use -File and pass args normally (when possible) PowerShell also supports -File [args] syntax [2], which avoids the special “everything after -Command belongs to the -Command string” behavior [1]. 3) If you must use -Command, embed arguments into the command string yourself Because anything after -Command is treated as part of the code to execute [1], the practical approach is to construct a single PowerShell snippet that assigns your values to variables (with proper escaping) and then runs your logic. Related Node.js Windows gotchas - Node’s spawn/spawnSync on Windows can be sensitive to quoting/spaces when a shell is involved; there are ongoing/recorded issues where argument parsing breaks on Windows [3]. Keeping spawnSync arguments as an explicit array (and avoiding unnecessary {shell:true}) helps. Bottom line - For PowerShell: positional parameters after -Command are not preserved as separate arguments; they’re interpreted as part of the command string [1]. - Use -CommandWithArgs (pwsh) or -File to get predictable argument passing [1][2].
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pwsh?view=powershell-7.4
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_powershell_exe?view=powershell-5.1&viewFallbackFrom=powershell-Microsoft.PowerShell.Core
- 3: Arguments given to spawn/spawnSync on Windows breaks on spaces within quoted arguments nodejs/node#43275
PowerShell の -Command 文字列モードでは位置引数のバインドが機能しない — 修正が必須
PowerShell 公式ドキュメントに明記されているとおり、-Command フラグで文字列を渡した場合、"-Command の後のすべての引数はコマンド実行の一部として解釈される" ため、spawnSync で渡された filePath、String(width)、String(raw) は PowerShell スクリプトの param([string]$Path, [int]$Width, [string]$RawFlag) に位置引数としてバインドされません。
現在のコードは Windows 環境で動作しません。修正方法:
- PowerShell 7+ のみの場合:
-CommandWithArgsを使用 - 互換性重視（PowerShell 5.1 サポート）の場合:
-Fileモードに変更するか、引数を command 文字列に埋め込む - または、$args 自動変数を活用する
🤖 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 `@bin/paste-img` around lines 80 - 98, The spawnSync call uses '-Command' which
prevents binding of positional params to the PowerShell script; update the
invocation to use '-File' (or '-CommandWithArgs' on PS7+) so filePath/width/raw
are passed as real args: change the arguments array in the spawnSync call (the
one referencing script, filePath, String(width), String(raw)) to use '-File'
before script (i.e., ['-NoProfile','-STA','-ExecutionPolicy','Bypass','-File',
script, filePath, String(width), String(raw)]), keep stdio/encoding/windowsHide
as-is, and ensure the PowerShell script's
param([string]$Path,[int]$Width,[string]$RawFlag) will receive those values;
after this the existing result.stdout.match(/RESIZED:(\d+)/) logic remains
valid.
Comment on lines
+119
to
+122
| case '-d': | ||
| case '--dir': | ||
| dir = args[++i]; | ||
| break; |
Contributor
There was a problem hiding this comment.
Windows では -d docs/images（スラッシュ区切り）指定時に Markdown パスが不整合になる
Line 121 で dir = args[++i] にユーザー入力をそのまま格納している。Windows では path.join('docs', 'images') が docs\images を返すため、Line 169 の dir === path.join('docs', 'images') 比較が docs/images では一致しない。自動検出時（Line 148）は path.join で代入するので一致するが、ユーザー明示指定時は docs/images/name.png が生成される（images/name.png の代わりに）。
🐛 修正案：代入時にパスを正規化する
case '-d':
case '--dir':
- dir = args[++i];
+ dir = path.normalize(args[++i]);
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case '-d': | |
| case '--dir': | |
| dir = args[++i]; | |
| break; | |
| case '-d': | |
| case '--dir': | |
| dir = path.normalize(args[++i]); | |
| break; |
🤖 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 `@bin/paste-img` around lines 119 - 122, User-supplied dir is stored raw (dir =
args[++i]) which causes mismatches with path.join('docs','images') on Windows;
change the '-d'/'--dir' case to normalize the input path (e.g., dir =
path.normalize(args[++i])) so it uses the same platform-specific separators as
path.join and the later comparison dir === path.join('docs','images') will
behave correctly.
Comment on lines
+32
to
+39
| function run(command, args) { | ||
| const result = spawnSync(command, args, { | ||
| stdio: 'inherit', | ||
| windowsHide: true, | ||
| }); | ||
| if (result.error) throw result.error; | ||
| if (result.status !== 0) process.exit(result.status); | ||
| } |
Contributor
There was a problem hiding this comment.
process.exit() が finally ブロックのクリーンアップをバイパスする
Node.js では try {} ブロック内で process.exit() を呼び出すと finally {} ブロックが実行されないため、run() がサブプロセスの非ゼロ終了コードを受け取った際に tmpGs / tmpPs が残存します。具体的な漏洩パス：
flattenモード：run(pdf2ps, [input, tmpPs])でtmpPs作成後、run(gs, ...)が失敗 →tmpPsが残存- 通常モード：
run(gs, ...)でtmpGs作成後、run(qpdf, ...)が失敗 →tmpGsが残存
加えて、result.status はシグナルで終了した子プロセスの場合 null になるため、null !== 0 が true となり process.exit(null) （= 終了コード 0）が呼ばれ、失敗をサイレントに成功と報告してしまいます。
process.exit() が呼ばれると処理が停止し exit イベントが発火されますが、finally ブロックは実行されません。一方 process.on('exit') ハンドラは process.exit() が明示的に呼ばれたときも即座に実行されるため、fs.rmSync のような同期処理のクリーンアップを安全に登録できます。
🛠️ 修正案（Option A: process.on('exit') を使用する最小変更）
const tmpGs = `${output}.gs.pdf`;
const tmpPs = `${output}.tmp.ps`;
+process.on('exit', () => {
+ fs.rmSync(tmpGs, { force: true });
+ fs.rmSync(tmpPs, { force: true });
+});
+
if (!fs.existsSync(input)) { } finally {
- fs.rmSync(tmpGs, { force: true });
- fs.rmSync(tmpPs, { force: true });
}🛠️ 修正案（Option B: run() でスローし、外側で catch する）
function run(command, args) {
const result = spawnSync(command, args, {
stdio: 'inherit',
windowsHide: true,
});
if (result.error) throw result.error;
- if (result.status !== 0) process.exit(result.status);
+ if (result.status !== 0) {
+ const err = new Error(`${command} exited with code ${result.status}`);
+ err.exitCode = result.status ?? 1;
+ throw err;
+ }
}-try {
+try {
// ... 既存処理 ...
console.log('Done.');
+} catch (err) {
+ if (typeof err.exitCode === 'number') {
+ process.exitCode = err.exitCode;
+ } else {
+ throw err;
+ }
} finally {
fs.rmSync(tmpGs, { force: true });
fs.rmSync(tmpPs, { force: true });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function run(command, args) { | |
| const result = spawnSync(command, args, { | |
| stdio: 'inherit', | |
| windowsHide: true, | |
| }); | |
| if (result.error) throw result.error; | |
| if (result.status !== 0) process.exit(result.status); | |
| } | |
| function run(command, args) { | |
| const result = spawnSync(command, args, { | |
| stdio: 'inherit', | |
| windowsHide: true, | |
| }); | |
| if (result.error) throw result.error; | |
| if (result.status !== 0) { | |
| const err = new Error(`${command} exited with code ${result.status}`); | |
| err.exitCode = result.status ?? 1; | |
| throw err; | |
| } | |
| } |
🤖 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 `@bin/pdf-compress` around lines 32 - 39, The run() helper incorrectly calls
process.exit(result.status) and treats result.status === null (signal) as
success, which bypasses finally cleanup and can leave tmpGs/tmpPs orphaned;
change run(command, args) to not call process.exit but instead throw an Error on
any non-zero exit or signal (use result.status !== 0 || result.signal to detect
failure) so callers can catch and clean up, and/or register a process.on('exit')
handler at module init to synchronously remove tmpGs/tmpPs as a last-resort
cleanup; update call sites (e.g., where run(pdf2ps, [input, tmpPs]),
run(gs,...), run(qpdf,...)) to catch errors and perform fs.rmSync for
tmpPs/tmpGs in catch/finally rather than relying on run to exit the process.
Comment on lines
+17
to
+23
| function isProbablyText(filePath) { | ||
| const buf = fs.readFileSync(filePath); | ||
| if (buf.includes(0)) return false; | ||
| const sample = buf.subarray(0, Math.min(buf.length, 8192)); | ||
| const controls = [...sample].filter(byte => byte < 32 && ![9, 10, 13].includes(byte)).length; | ||
| return sample.length === 0 || controls / sample.length < 0.02; | ||
| } |
Contributor
There was a problem hiding this comment.
空ファイルが「テキストと判定」され、空文字列がクリップボードにコピーされる
sample.length === 0 の場合に true を返す（Line 22）ため、空のダウンロードファイルが存在するとクリップボードの内容が空文字列で上書きされてしまう。意図的な動作であれば問題ないが、空ファイルは無視する方が自然。
🐛 修正案
- return sample.length === 0 || controls / sample.length < 0.02;
+ return sample.length > 0 && controls / sample.length < 0.02;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isProbablyText(filePath) { | |
| const buf = fs.readFileSync(filePath); | |
| if (buf.includes(0)) return false; | |
| const sample = buf.subarray(0, Math.min(buf.length, 8192)); | |
| const controls = [...sample].filter(byte => byte < 32 && ![9, 10, 13].includes(byte)).length; | |
| return sample.length === 0 || controls / sample.length < 0.02; | |
| } | |
| function isProbablyText(filePath) { | |
| const buf = fs.readFileSync(filePath); | |
| if (buf.includes(0)) return false; | |
| const sample = buf.subarray(0, Math.min(buf.length, 8192)); | |
| const controls = [...sample].filter(byte => byte < 32 && ![9, 10, 13].includes(byte)).length; | |
| return sample.length > 0 && controls / sample.length < 0.02; | |
| } |
🤖 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 `@bin/watch-downloads-copy` around lines 17 - 23, isProbablyText currently
returns true for empty files (sample.length === 0), causing empty downloads to
be treated as text and overwrite the clipboard; change isProbablyText (and/or
its early checks on buf) so that an empty file returns false (i.e., treat
zero-length files as non-text/ignore) instead of true to avoid copying empty
strings to the clipboard; update the logic in function isProbablyText to return
false when buf.length or sample.length is 0 and keep the existing
binary/controls checks intact.
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.
Summary
Verification
Summary by CodeRabbit
リリースノート
新機能
改善
ドキュメント