Chrome拡張クラウド会計連携 - コードレビューとモジュール分割

3つのレビューエージェントを並列に走らせて会計ソフトA連携Chrome拡張をレビューし、Critical 4件・High 6件・Medium 6件の指摘が返ってきた。修正を片付けた後、4400行のcontent.jsを5ファイルに分割した。分割作業中にXSS修正で埋め込んだバグが1つ見つかり、再発防止のテストまで入れて一日が終わった。

3エージェント並列コードレビュー

content.js（4440行）、popup.js、background.jsの3ファイルを、それぞれ別のレビューエージェントに投げた。プロンプトには「セキュリティ」「エラーハンドリング」「保守性」の観点を指定。3つが同時に走って、数分で結果が揃った。

Codexにもレビューを投げたが、サンドボックスポリシーに引っかかってファイルを読めなかった。ファイルパスを渡しても「アクセスできません」が返ってくるだけで、レビュー以前の問題だった。Claude Codeのサブエージェントが主力になった。

Critical 4件 - 即座に修正

1. 二重実行防止: `responded = true` の早期セット

メッセージハンドラで非同期処理の途中に responded = true をセットしていた。処理が完了する前に次のメッセージが来ると、同じ処理が二重に走る。responded = true をハンドラの先頭に移動した。

2. `switchYearByCti` の `res.ok` チェック漏れ

年度切り替えのfetchで、レスポンスのステータスを確認せずにそのまま処理を続けていた。404が返ってきても正常系として流れる状態だった。if (!res.ok) のガードを追加。

3. innerHTML経由のXSS対策

ユーザー入力を含む文字列がinnerHTMLにそのまま流れ込んでいた。事業者名に<script>を仕込めば動く状態。escapeHtml関数を作り、テンプレートリテラルに埋め込む変数を全てエスケープした。

const escapeHtml = (str) =>
  String(str)
    .replace(/&/g, '&amp;')
    .replace(/</g, '&lt;')
    .replace(/>/g, '&gt;')
    .replace(/"/g, '&quot;')
    .replace(/'/g, '&#039;');

4. message handlerのsender未検証

chrome.runtime.onMessageのリスナーで、senderの検証をしていなかった。任意の拡張機能やページからメッセージを送り込める状態だった。sender.id === chrome.runtime.id のチェックを追加。

High 6件

fetchCsvのタイムアウト未設定: AbortControllerで30秒タイムアウトを追加
拡張URLのハードコード: chrome.runtime.getURL() に置き換え
parseImportFormResponseのエラー検知が広すぎる: エラー判定条件を具体化し、正常なレスポンスを誤検知しないように修正
バリデーション関数の接続: レビュー指摘で接続したが、後に運用でノイズが多すぎると判断して削除（後述）
スクレイピングロジックの二重実装: content.jsとlib.jsに同じパース処理が存在していた。lib.js側に統一
saveEntitySettingのread-modify-writeレース: chrome.storage.localの読み→加工→書きが非同期で、同時に呼ばれると後勝ちで先の変更が消える。ロック機構を追加

Medium 6件

CTI復帰追跡の状態管理、loadAllクリック上限の設定、resolveSheetNamesのサフィックス衝突回避、fetchCsvの不要ダウンロード防止、UNFILTERED配列の重複排除、buildMultiEntityConfirmMessageの可読性改善。いずれも局所的な修正で対応。

4400行content.jsを5ファイルに分割

レビュー指摘の修正が一通り終わった後、content.jsの分割に着手した。

分割構成

ファイル	責務	行数目安
`storage.js`	chrome.storage.localの読み書き、設定管理	~300
`bridge.js`	popup/backgroundとのメッセージ通信	~400
`export.js`	スプレッドシートへのエクスポート処理	~800
`import.js`	CSVインポート、フォーム操作	~600
`content.js`	DOM操作、スクレイピング、メインロジック	~2300

manifest.jsonのcontent_scriptsにファイルを追加し、読み込み順序を storage → bridge → export → import → content に設定。グローバル変数の依存関係を整理して、前段のファイルで定義した変数を後段が参照する形にした。

escapedNameバグの発見と修正

XSS対策でescapeHtml関数を追加した際、元々あったescapedName変数を削除していた。しかし、テンプレートリテラル内で${escapedName}を参照している箇所が残っていた。JavaScriptはテンプレートリテラル内の未定義変数をエラーにせず、undefinedという文字列にする。画面に「undefined」が表示されて初めて気づいた。

再発防止: テンプレートリテラル内の未定義変数を静的検知するテスト

テンプレートリテラルから${...}を抽出し、スコープ内で定義されていない変数名がないかチェックするテストを追加した。正規表現でソースコードを走査し、未定義の変数参照を検出する。

// テンプレートリテラル内の変数参照を抽出して検証
const templateVarPattern = /\$\{([^}]+)\}/g;
// 各ファイルのトップレベル変数・関数引数と照合

このテストがあれば、変数を削除・リネームした際に参照が残っていればCIで検出できる。

レビューでHigh指摘だった「バリデーション関数が定義されているが呼ばれていない」を受けて、エクスポート前にバリデーションを実行するように接続した。しかし実際に動かすと、会計ソフトAのデータは例外パターンだらけで、正常なデータでも警告が出続けた。毎回「無視」を押す作業が増えただけだったので、バリデーション実行部分を削除した。関数定義自体はlib.jsに残してあるので、将来必要になれば再接続できる。

振り返り

3エージェント並列レビューは、1ファイルずつ順番にレビューするより待ち時間が短く、観点の偏りも減った。ただし3つの結果をマージする作業は手動で、重複指摘の整理に時間がかかった。

escapedNameバグは、XSSを潰す修正が別のバグを埋め込んだ形だった。テンプレートリテラルが未定義変数を黙ってundefinedにするJavaScriptの挙動に足を掬われた。静的検知テストを入れたので、同じパターンは今後CIが拾ってくれる。

content.jsの分割は、レビュー→修正→分割の順番が正解だった。先に分割してからレビュー修正を入れると、修正箇所がどのファイルに移動したか追跡するコストが増える。