4
2

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

引き継いだコードの「この案件だけの作法」を、git logから掘り起こしてレビュー観点にした話

4
Posted at

はじめに

GMOコネクトの永田です。

5年以上動いている業務システムのAPIに、新機能を足すことになりました。コミットは1,000件を超えていて、書いた人は10人近く。自分の寄与は1割もありません。

汎用のチェックリストを渡してAIエージェントにレビューさせています。入力値の検証、境界値、エラー処理。これはこれで機能していて、不満はありません。

困ったのは別のことでした。このコードには、クエリの組み立て方や認可の掛け方に独特の作法があります。同じ場所が繰り返し壊れてきたのも、たいていそこです。その作法を踏み外した、という指摘は返ってきません。 知見が誰の頭にも、どのドキュメントにも残っていないからです。引き継いだコードなので当然ではあります😇

残っていたのはgit logだけでした。この記事は、そこからレビュー観点を作らせて、実際に指摘が出るまでの記録です。

先にまとめ

  • git履歴を素材に、このコードベース固有のレビュー観点を作らせました。依頼は平文2つで、手順は何も指定していません
  • 出来たチェックリストを当てたところ、汎用チェックリストのレビューと単体試験300件超を通過した差分から、要対処7件が出ました
  • 再現できるように、チェックリストの構成を7つのポイントに分けて書き出しました。そのまま使える依頼文も置いています
  • 汎用と固有はどちらも相手が拾わなかった指摘を出しました。上位互換ではなく、別の層を見ています

1. 既存の方法が使えなかった

「過去の指摘からルールを作る」というやり方は、既に良い記事がいくつもあります。

いずれも素材はレビュー指摘・PRコメント・会話ログです。指摘の蓄積があることが前提になっています。

引き継いだコードには、それがありません。

実測
稼働期間 5年以上
コミット 1,000件超
著者 10人近く
自分の寄与 1割未満

残っているのはコミットの差分だけです。そこで、差分そのものを素材にすることにしました。

2. 依頼したこと

投げた文をそのまま出します。凝ったところはありません。

過去のgit logのdiffを分析し、バグの傾向を確認できないか?
先ほどのチェックリストの観点として、本件で頻発する設計・実装バグのパターンや、クエリ言語独特の考慮事項があるのでは?と思っている。

数日後、実装が一段落したところでもう1つ。

固有のチェックリストも作成して、レビューして

これで64観点のチェックリストが出来て、再レビューまで終わりました。1つ目の依頼で作った分析結果があるとはいえ、チェックリスト作成から再レビュー完了までの所要は6分55秒です。

ただし、まったくの手ぶらだったわけではありません。別の案件で作ったチェックリストを併せて渡しています。(先ほどのチェックリストの観点 のこと)
そちらは課題管理システムとGitHub Issueに蓄積があり、そこから作ったものでした。様式はそこから持ち込まれています。

初回の人にはその1本がありません。なので、何が書いてあれば同じものが作れるかを、次の章で項目にします。

3. チェックリストの構成 — 再現するための7点

素材の作り方

ポイント1 全差分を読ませず、コミットメッセージで一次絞り込みをさせる

5年分の差分を全部読ませたら終わりません。粗く絞ってから読む、という段構えを先に指定します。

段階 件数
対象ディレクトリを触るコミット 約580
メッセージに「修正・不具合・バグ・障害・是正・fix」を含むもの 約220
差分を精読 約80

限界も一緒に書かせます。 実際に出てきた成果物には「コミットメッセージ由来の抽出のため、機能追加コミットに紛れた修正は数に入っていない。件数は傾向の目安として扱う」と書いてありました。これが無いと、後で件数を根拠に使ってしまいます。

ポイント2 修正が集中しているファイルの一覧を出させる

修正系コミットの多い順にファイルを並べさせます。今回は多いもので50件超、5番目でも20件台でした。

これが章立てと優先度の根拠になります。観点を「一般的に危ないところ」ではなく、このコードで実際に壊れてきたところへ寄せられます。上位に出たファイルに対応する章が薄ければ、まだ掘り足りないという合図にもなります。

一覧そのものも残しておくと、次にこのコードを触る人への引き継ぎになります。

チェックリストの形

ポイント3 優先度を、過去の発生頻度で3段階にする

「実績があり、影響が全ルートに及ぶ」「実績があり、影響は限定的」「実績なし・予防的」の3段階です。

実際に出来たものを数えたところ、実績の出典が付いている割合が優先度と相関していました。

優先度 実績の出典つき
最優先 52%
28%
予防的 15%

閾値の言葉は案件に合わせて変わってよいと思います。今回は最優先の定義に「または実機でしか検出できない」が入りました。外部サービス連携が絡むと、手元の試験では原理的に出ない層があるからです。

ポイント4 観点ごとに、いつ何が起きたかを書かせる

「クエリ組み立ての既定がAND連結ではなく、条件が意図せずOR連結になる仕様。3年前に実害」のように、実績を紐づけさせます。

後から検算できますし、書けない観点は予防的として下げられます。 優先度を人の感覚で付けると、だいたい全部が高くなります。

ポイント5 汎用と固有の2本に分け、基準を「次の案件に持ち越せるか」にする

固有のほうに入力検証・ループ・境界値・命名を入れません。ただし理由は「汎用の観点が要らない」ではありません。汎用は案件をまたいで使い回すから別ファイルなのであって、両方要ります。

中身 寿命 作り方
汎用 入力検証・境界値・エラー処理・命名など 案件をまたいで貯まる 一度作って持ち歩く。手持ちが無ければ、同じ分析の結果を2つに割る
固有 その案件で繰り返し壊れている場所 この案件限り git logから作る(この記事の話)

分ける基準は「同じ技術スタックの別案件でも当てはまるか」です。当てはまるなら汎用側へ回します。

手持ちが0でも困りません。同じ分析から両方作れます。 実際、出てきた分析結果には「汎用の頻発パターン」という章があり、コピペ由来の直し忘れ、応答オブジェクトの取り違え、ライブラリ更新で例外型が変わる、といった別案件でも当てはまるものが入っていました。上の基準で2つのファイルに割れば、汎用側だけが次の案件へ残ります。

ただしこの作り方だと、汎用側にはその案件がたまたま踏んだものしか入りません。公開されているチェックリストと突き合わせて、抜けている章を足すのが早いです。

両方要ることは実測でも出ました。同じ差分に対して、汎用だけのレビューと、汎用に固有を足したレビューを比べています。

要対処 うち相手が拾わなかったもの
汎用のみ 6件 3件
汎用+固有 7件 2件

片方がもう片方を含みません。固有チェックリストは汎用の上位互換ではなく、別の層を見ています。混ぜて1本にすると、量が増えて読まれないうえ、次の案件へ持ち越せる部分が固有の記述に埋もれます。

ポイント6 章を「機能」ではなく「壊れ方」で切らせる

これはやってみるまで意識していませんでした。

チェックリスト 章の切り方
汎用(持ち込んだもの) 入力/出力/ページネーション/ループ/DB/状態管理/エラー処理/表示/命名/テスト
固有(今回作られたもの) 認可/クエリの組み立て/応答の受け取り/件数上限/更新の原子性/項目名/日時/ログ

固有のほうは、この案件で壊れてきた場所で切られています。機能で切ると「登録機能」「一覧機能」になって、章の中身が汎用チェックリストと同じ話に戻ります。

これは偶然ではなさそうでした。あとから文脈を持たない別のセッションでもう一度作らせたところ、互いを見ていないのに章立てが7〜8個も対応しました。

ポイント7 1行を「✓ / 優先度 / 観点 / 詳細」の4列にする

観点は疑問文にします。レビュー時にそのまま埋められる形にしておくためです。

| ✓ | 優先度 | 観点 | 詳細 |
|---|---|------|------|
| ☐ | ★★★ | **送信する項目名を実機の定義と突き合わせたか** | 項目名の誤りはコンパイルも単体試験も素通りする(スタブに同じ誤りを書くため)。実機でしか検出できない。実績: その機能の全操作が500になった障害 |
| ☐ | ★★ | **可変長のリストから IN 句を組むとき、件数の上限を考えたか** | 一覧の上限をそのまま並べるとクエリのURIが長すぎて失敗する。失敗しても付帯情報だけが欠落して一覧は200で返るため気づきにくい |

冒頭には、根拠・適用対象・使い方・汎用チェックリストとの関係を書かせます。引き継ぎのためです。

依頼文

7点をそのまま落とすと、こうなります。

このリポジトリ固有のコードレビュー用チェックリストを作ってください。

# 素材
- 対象ディレクトリを触るコミットを git log で列挙する
- コミットメッセージに「修正・不具合・バグ・障害・是正・fix」を含むものへ絞り込む
- そのうち差分の性質が読み取れるものを精読する。現行ソースも直接確認する
- 各段階の件数と、この抽出方法の限界を冒頭に明記する
- 修正系コミットが多いファイルを上位から一覧にする

# 形式
- 1行を「✓ | 優先度 | 観点 | 詳細」の4列にする。観点は「〜していないか」の疑問文
- 優先度は3段階。★★★=実績があり影響が広い / ★★=実績があり影響は限定的 / ★=実績なし・予防的
- 観点ごとに、いつ何が起きたか(コミットや障害)を「実績:」として書く。書けないものは★に下げる
- 章は機能名ではなく「壊れ方」で切る(例: 認可 / クエリ組み立て / 応答の受け取り / 件数上限)

# 入れないもの
- 入力検証・ループ・境界値・命名など、同じ技術スタックの別案件でも当てはまる観点
  (それらは汎用チェックリストの持ち物とし、本ファイルからは参照だけする)

# 冒頭に書くこと
- 目的 / 適用対象 / 使い方 / 根拠(分析したコミット数)/ 汎用チェックリストとの関係

4. 当てたら何が出たか

対象は40ファイル弱、約6,000行の新規実装です。この時点で、汎用チェックリストでのレビュー、単体試験300件超、E2E60件超を、すべて通過していました。

そこから要対処7件です。中身は案件が特定されない形に置き換えますが、性質はそのまま書きます。

  • 同じキーが、読み側と書き側で意味が違う。 詳細取得は「名前」を返し、更新は同じキーを「ID」として受け取る。画面が取得した内容をそのまま送り返すと壊れる
  • 可変長のリストからクエリの条件を組んでいて、件数の上限が無い。 上限に近づくとクエリが失敗するが、失敗しても付帯情報だけが欠けて一覧は正常に返る
  • 既存APIの取得項目を増やしたため、連携先の準備が先に要る。 その順序がリリース手順書に書かれていない。準備前に出すと、新機能ではなく既存機能が止まる
  • 監査ログに渡す識別子が、その経路では常に空になる
  • リクエスト全体を平文でログに出している。既存のハンドラも同じ作法だが、新規でもう1本増やした

1つ目は、単体試験が書き込み側しか検証しておらず、取得した内容を送り返す経路を通っていないため素通りしていました。2つ目は、失敗しても正常に見えるので、試験が通ることと動いていることが一致しません。

いずれも汎用の観点からは出ませんでした。「過去に同種の修正を繰り返していた場所」として観点になっていたから引っかかっています。

5. 補足 レビューは1回では終わりません

もう1つ、試して分かったことがあります。

チェックリストを変えず、同じ差分をもう一度レビューさせただけで、追加の指摘が出ました。 汎用チェックリストだけを当てた条件で、2周させたときの結果です。

要対処
1周目 6件
2周目(追加分のみ) 2件

観点は1つも足していません。しかも2周目に出た2件のほうが重く、どちらも認可に関するものでした。権限の判定に使う値と、実際に処理する値が結び付いていない、という型です。

チェックリストの整備に時間をかけるより先に、同じチェックリストで2周するほうが費用対効果は良さそうです。日程を引くときは「レビュー1回」ではなく、往復の回数で見積もるのが安全でした。

まとめ

  • 引き継いだコードにはレビュー指摘の蓄積がありません。代わりにコミットの差分が素材になります
  • 依頼は平文で足ります。ただしチェックリストの構成は指定したほうが安定します。7点にまとめました
    • 素材: コミットメッセージで一次絞り込み/修正が集中するファイルの一覧
    • 形式: 優先度は過去の発生頻度で3段階/観点に実績を紐づける/4列の疑問文
    • 分け方: 汎用と固有の2本。基準は「次の案件に持ち越せるか」
    • 章立て: 機能ではなく「壊れ方」で切る
  • 汎用と固有はどちらも相手が拾わなかった指摘を出しました(6件中3件、7件中2件)。上位互換ではありません
  • 単体試験300件超とE2E60件超を通過した差分から、要対処7件が出ました
  • 同じチェックリストで2周すると、まだ出ます(6件 → 追加2件)

測れたのは「過去に繰り返し壊れた場所を、もう一度見に行く」ところまでです。新しく作り込んだ場所の妥当性は、これでは分かりません。それでも、誰の頭にもドキュメントにも残っていなかった作法が、レビューの観点として戻ってきました。


最後に、GMOコネクトではサービス開発支援や技術支援をはじめ、幅広い支援を行っておりますので、何かありましたらお気軽にお問合せください。

お問合せ:https://gmo-connect.jp/contactus/

4
2
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
4
2

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?