コードレビューとPull Request、そしてその承認機能の副作用について考える

用語

レビュアー

対象となるコードをレビューする人のことを指します。

レビュイー

レビューを受ける人、つまりレビューする対象のコードを書いた人のことを指します。

tl;dr

• アプリケーション開発業務におけるコードレビューはコードの正しさや質そして一貫性を保ち、それらと同時にコードに対するチームとしての共有知を作り上げる良いプラクティスだと思います
• アプリケーション開発チーム内でのコードレビューにおいてPull Requestを使ったレビューのスタイルは一般的ですが、Pull Requestの承認は実際にはほとんど意味がないのではないでしょうか？
• ほとんど意味がないにも関わらず、承認の有無によって業務フローが左右されることでそれが権威的に扱われてしまいオーナーシップを希薄化させ、結果的にコードレビューのコストが増加したりそれを行う目的を見失ってしまっていることはないでしょうか？
• Pull Requestの承認機能はなるべく利用しないか、利用するにしても既読管理程度の意味合いにとどめたほうがよいのではないでしょうか？
  • 2021/11/08 11:39追記: ただし、これはレビューの必須化をやめるという意味ではありません。承認の必須化によって必ずこういった問題が起きるという主張でもありませんし、例に挙げたような事例はこれが原因だという主張でもありません。原則としてはレビューそのものは必ずやるフローにするべきで、承認必須化の仕組みと承認という言葉がもたらす重みを問題視しています。言い換えるならば、それを上回るメリットがそれぞれのチームにもたらされるかどうかを考えて採用するべきで、思考停止的に有効にするべき機能ではないのではないか？ということです。要はこのあたりも実はよく考えたほうがいいんじゃない？と言いたいだけです。（様々なフィードバックがいただけて大変参考になります。ありがとうございます。）

2021/11/09 00:10追記: どうやら単なる問題の可能性の提起のつもりが、そういった問題がぼくのいるチームで起きているかのような誤解を一部に与えていたようです。そのような事実はありません。(心配させてしまった方にはすみません)
「考える」というタイトルの通りこれは単なる考察です。もちろん、その元となった体験は僕の中にはいくつかありますが、そのなかでジュニアというほどではないはずなのにオーナーシップの希薄化が起きたり権威的なレビューを行う人を見かけることが稀にあり、そのような思考に至る上で何が影響しているのかを考えていて、そのうちの一つとしてこれに至ったという次第です。それがたまたまこのタイミングだっただけで、他意はありません。

前置き

背景を説明するために、まずは一般的なコードレビューのあり方について自分の理解を共有しておきます。

アプリケーション開発業務はほとんどの場合においてチームを組んで行うのが一般的だと思います。 中でも（特にWeb系の企業の）比較的モダンな開発スタイルを取るチームでは、そのコードの変更をチーム内でレビューすることを開発のフローに組み込むことも一般的だと思います。

コードレビューの目的

アプリケーション開発チームがコードレビューを行う目的は様々ありますが、多くの場合は以下のような目的を持ってそれを行うことが多いのではないでしょうか。*1

• コードの正しさを保つ
  • （たとえば）アプリケーションや関連するAPI・ライブラリの仕様を正しく理解して実装できているか
    • 誤っていそうなコードやコメントはないか
  • （たとえば）その実装方法によって想定外の仕様上の制約を産んでしまっていないか
  • （たとえば）適切なインターフェースをデザインできているか
• コードの質を保つ
  • （たとえば）パフォーマンス上の問題やセキュリティ上の問題を起こし得る実装になっていないか
    • それは様々な開発上の背景のもと、現状で許容できるレベルを超えうるか
  • （たとえば）適切なアルゴリズムやデータ構造を利用した実装になっているか
  • （たとえば）適切に言語やライブラリの機能を利用できているか
  • （たとえば）コードの追加や変更に対して適切に自動テストの実装や修正が行われているか
• コードの一貫性を保つ
  • （たとえば）抽象化のスタイルが他と一貫しているか
  • （たとえば）命名規則が適切に適用されているか
  • （たとえば）利用しているライブラリやツールが他と一貫しているか

コードレビューは上記のような目的のもと、様々な観点でコードに対して考察し、それをもとにディスカッションを行います。第三者的な視点でコードを見ることによって、ひとりでは気づきにくかったことに対する気付きを得たり、見落としていたことに気付けたりすることがあるので、その目的に対して大きな効果を上げることになります。（上級者がコードをレビューすることに価値があると考える人もいるかと思いますが、最も重要なことは第三者的な視点が入ることそのものです。）

それぞれの目的にどの程度の比重を置き、またどの程度のコストをかけてコードレビューを行うのかはチーム（あるいは、もしかするとそのメンバーそれぞれで）異なる場合もあるとは思いますがが、おそらくほとんどのチームにおいてはこういったことを目的としてコードレビューを取り入れていることが多いのではないでしょうか。

さらに、コードレビューをチームとして行う過程においてレビュアーとレビュイーがディスカッションを重ねることによって、コードや関連仕様などに対する理解がメンバーの間で深まります。それによって、結果的にチームとしての共有知を作りあげることにもつながっていることもあると思います。
個人的にもそれをいくつかのチームで経験したことがありますがが、これができているチームは高い効率で開発を進められていたように思います。 もっと言えば、個人的にはこれこそがコードレビューを行う最大の目的であるべきではないだろうかとすら思います。

他にも、うまく言語化できないメリットや書きそびれていることも様々あると思いますが、とにかくコードレビューはこういったことを実現する上で、一般的に良いプロダクト作りに有効なプラクティスとして多くのチームで取り入れられているのではないかと思います。
そして、当然ながらコードレビューにはそれにはそれなりのコストが掛かりますが、多くのチームにおいては前述の目的のもとそのコストに見合うメリットを見込んでそれを行っていることが多いでしょう。

コードレビューのスタイル

コードレビューのスタイルは様々ありますが、昨今ではGithubやBitbucket、GitlabなどのPull Request機能を用いてコードレビューが行われることが昨今では多いのではないかと思います。

このスタイルにおいては、まずレビュイーはメインストリームとなるブランチからフォークしたブランチを作成し、そこでコードの変更を行って最低1つのコミットを行い、それをリモートのリポジトリにプッシュしてPull Requestを作成します。

さらに、そのPull Requestに対して必要な説明を（主にテキストを用いて）行い、1名以上のレビュアーにレビューを依頼*2します。

レビュアーによってレビューが行われた後にその変更を承認するかどうかを通知*3し、レビュアーはその結果に応じてそのPull Requestをどうするかを決めます。

承認された場合は、権限を持つ誰かがそのPull Requestをマージします。承認されなかった場合は、レビューされた内容に応じて変更を行ったり、場合によってはPull Requestごと作り直したり、その変更自体をやめることもあります。

もしかすると、細かい部分で異なるということはあるかもしれませんが、多くの場合はPull Requestを利用した開発はこのように行われているかと思います。

前置きが長くなりましたが、今回の話ではアプリケーション開発チームがこのPull Requestを用いた開発フローを採用した上でその過程におけるコードレビューにおいて、Pull Requestとその承認機能がもたらすものに注目していきます。

Pull Requestとその承認機能

Pull Requestをもとにコードをレビューするとき、チーム内でそれを行っている場合においてその承認機能がどのようにチームに作用するのかを考えます。

まず、大前提としてチーム内でコードレビューを行う場合は、レビュアーとレビュイーはその双方がそのプロダクトのコードに対する当事者意識を持って開発しているでしょう。 そのコードがもらたす問題は自分たちの問題であり、その問題は自分たちが自律的に対処するべきであるという意識を持っています。少なくともそうであるべきだと思って開発に臨んでいると思います。

Pull Requestはレビューに限らず、コードの変更をチーム内外に知らせるために作成されます。そしてその歳には前述したような目的に沿ってレビューが行われることが期待されるでしょうし、コードの変更を行ったメンバーは多くの場合はそれを期待してPull Requestを作成しているでしょう。

Pull Requestの承認の意味

では、そのPull Requestを承認したということにはどういった意味があるのでしょうか？

文字通りに読み取ればレビュアーがその変更の取り込みを承認したということですが、実際にはレビュアーだけではなくレビュイーもその変更に限らないコードに対してオーナーシップを持っているはずです。レビュイーはそれが承認されずともその変更を取り込む権限がありますし、もしそのコードが緊急的に即座に取り込まれるべきものであれば取り込むべきです。

つまり、レビュイーは本来であればそのコードが取り込まれることに対して十分な責任とその権限を持っているはずです。 そのため、変更に対するレビューを求める意味はあったとしても、その変更の取り込みの承認を求める意味は特に無いはずなのではないでしょうか？

2021/11/08 12:30追記: コードレビューの文脈外なので触れていませんでしたが、技術監査ないしセキュリティ管理上の何らかの理由で証跡を残したいケースはあるかと思います。ただ、コードレビューの文脈においては、仮にコードを見ていなくても承認を行うことは可能（ノールックLGTMという言葉をご存知でしょうか？）であり、十分にコードがレビューされたことを保証するには不十分です。本当に必要なのは仕組みではなく必ずコードをレビューする必要性の理解とそれを実行し継続するメンタリティであるはずです。

Pull Requestの承認を必須化する副作用

コードレビューはチームとしてコードの正しさや質や一貫性を保つためのセーフティーネットのうちの一つと言っても差し支えないかと思います。 もちろん、それが必ず行われていることは、健全なコードを保つ助けになることでしょう。 そのため、Pull Requestの承認は前述のとおりあまり意味がないとしても、Pull Requestの承認をマージの必須条件にすることで、仕組み的にそれを担保しているチームは多いのではないかと思います。

しかし、そういった意図とは裏腹に、承認という言葉が持つ重みがいくつかの副作用を生んでしまうこともあるのではないでしょうか？

個人的には以下のような副作用が出ることがあるのではないかと考えます:

• オーナーシップの希薄化
• レビュアーの権威化

それぞれ順を追って説明します。

オーナーシップの希薄化

Pull Requestの承認をマージの必須条件にすることで、結果的にレビュイーはそのコードがマージできるかどうかの判断をレビュアーに委ねることになってしまいます。 これは見方によっては、レビュアーがその変更の取り込みを承認するまでPull Requestの作成者はその変更をマージできるかどうか判断できないとも捉えることができてしまいます。

そしてその結果として、コードに対するオーナーシップが希薄化してしまうことはないでしょうか？

具体的には、Pull Requestが承認されるまでマージされないことに油断して、安易に変更をPull Requestにしてしまうことはないでしょうか？ *4

レビューの承認機能があることによってそれに甘んじ、あまり推敲せずにそれをPull Requestにしてしまう。つまりコードに対するオーナーシップの意識を承認機能が希薄にさせてしまうことはないでしょうか？

前述した通り、コードレビューにはそれなりにコストが掛かります。経験した人であれば分かると思いますが、前述の目的を達成する上で十分に意味のあるレビューを行おうとすると、コードや仕様やそれらの周辺知識を理解し、それをもとに様々なことを想像して、様々な観点において問題が無いかどうかを考えなければなりません。これは非常にコストが掛かる行為です。

しかし、多くのチームではそれだけ大変なことにも関わらずコードレビューを取り入れています。コードレビューはコードを健全に保つ上で有効な手段ですが、逆に言えばコードを健全に保ち続けることはコードレビューが必要なほど困難であり、それと同時にプロダクトの価値をうまくユーザーに届ける上で重要なことなのです。

安易に変更をPull Requestにしてしまう行為はコードレビューのコストを増加させ、それと同時に健全でないコードがブランチのメインストリームに取り込まれてしまうリスクを増加させます。 前述した通り、コードレビューはコードを健全に保つためのセーフティーネットのような役割なので、そもそもなるべく問題がないコードを書くべきなのは当然のことです。

つまり、本来であればPull Requestの作成そのものにも一定の慎重さを持っているべきで、コードに対するオーナーシップを発揮して万が一レビューをすり抜けても問題がない自信を持ってPull Requestを作成するべきですが、承認という言葉がそのオーナーシップを希薄化させてしまうことで、そういった問題を引き起こす要因の一つになっているなんてことはないでしょうか？

さらに言えば、もし承認を必須化していなくとも、Pull Requestが承認されることに対する安心感によってその変更に対する自信が初めて持てるような状況があったとしたら、それは健全ではない状態でしょう。*5

レビュアーの権威化

繰り返しますが、Pull Requestの承認をマージの必須条件にすることで、結果的にレビュイーはそのコードがマージできるかどうかの判断をレビュアーに委ねることになってしまうことは前述した通りです。 では、それを委ねられたレビュアーはどうなるでしょうか？

レビュアーはある意味でその変更に対してとても強い権限を持つことになります。本来であれば、チームメンバーはそれぞれがそのコードに対して皆オーナーシップを持っているはずですが、 Pull Requestの承認が必須化されている状況下ではレビュアーとレビュイーの間ではレビュアーのほうがより強い権限を持っていることになります。

どういうことなのかイメージしづらいと思うので具体例を考えてみましょう。

たとえば、あるチームのメンバーAがPull Requestを作成し、同じチームのメンバーBにレビューを依頼したとします。 レビュアーであるメンバーBはそのPull Requestをレビューする上で、ある問題に気付きました。レビュイーであるメンバーAはそれでもその問題は些細であり元のコードのままで問題ないと考えていましたが、メンバーBとは意見が合わず、修正を求められました。

結果として、メンバーAはメンバーBのいうとおりに修正を行いました。その変更内容はメンバーBが求めた内容そのもので、メンバーBは問題ないと判断してその変更を承認し、いずれそれはマージされてリリースに至りました。 しかし、その修正箇所が原因で問題が起きてしまいました。それによってこの問題が起こり得ることには、メンバーA/Bの2人とも気付いていませんでした。そして、メンバーAが元々書いていたコードが（たまたま）実際に引き起こす問題が最小限にとどまるようでした。

さて、この場合、コードを書いたのはメンバーAですが、レビュイーであるメンバーAは承認を得るためにはレビュアーの意見に従わざるを得ない、あるいは従ったほうがスムーズに仕事が進むと判断したため、メンバーAはメンバーBの意見に従ったはずです。前述したようなオーナーシップの希薄化もそれに拍車をかけ、メンバーBの意見を鵜呑みにしてしまったのでしょう。

こういった問題は時々みかけることがあるかと思います。いずれも悪気はなく、それぞれがそれぞれの役割を果たそうとした結果として起きてしまった事故です。

しかし、これはレビュアーの権威化が起こっていた結果なのではないでしょうか？ もし承認という概念がなかったならばどうなっていたでしょうか？

承認が必須化されていなければ、そういったコメントは単に意見をおなじ開発メンバーから受け取っただけに過ぎません。 必ずしもレビュアーの意見に従う意味はありませんが、それを無視するためには自分が作成したPull Requestに対してそれ相応の自信と覚悟を持って臨む必要があるでしょう。

しかし、承認が必須化されている場合は、そういったコメントは見方によってはそれに従わなければ（あるいは納得させなければ）その変更を取り入れさせないという脅迫にも映りかねません。 レビュアーが権威化してしまい、健全なコードレビューが行われなくなります。

前述したとおり、コードレビューはコードの変更に対して第三者の視点が入ることそのものがとても重要です。その一方で、コードのオーナーシップを誰もが持って開発に臨むのが健全な開発チームでしょう。 レビュアーの権威化はその第三者の視点が強く影響しすぎてしまいます。

繰り返しますが、コードレビューは本来であればコードの変更に対して第三者の視点が入ることによって、コードを健全に保つことを助けてくれます。 コードを書いた本人が本来であればそのコードを一番理解しているはずですが、レビュアーが権威化してしまっているとそれより強い権限を持った第三者がそのコードをマージさせるかどうかに大きく影響を与えるという状況が生み出されてしまい、このアンバランスな構図はさきほどの具体例のような悲劇を招きかねません。

その場合、コードレビューの目的はどこに行ってしまっているのでしょうか？ レビュアーが権威化していることによって独りよがりなレビューがまかり通ってしまっていないでしょうか？

そしてそういった問題は、実はPull Requestの承認機能がそれを引き起こす遠因となっていないでしょうか？

どうしていったらよいのか

あくまでもこれらの副作用は単なる仮説であり、確実に因果関係があるとまで言えるようなものではありません。単にオーナーシップが希薄なメンバーや、権威的なレビュアーがいるから引き起こされる問題とも考えられるかもしれません。 しかし、Pull Requestの承認機能は前述のとおりあまりメリットがないと思われる割には、こういった副作用を生み出し得る要素を持っているように思えてなりません。つまり、卵か鶏かでいえばPull Requestの承認機能はこの問題の卵のひとつなのではないかというのが自分の意見です。

では、どうしていったらよいのでしょうか？

自分が考える理想的な状況は、メンバーの誰もが健全にオーナーシップを持って開発を行い、マージされても品質を毀損しない自信があるPull Requestだけが作成されることです。*6
そして、それに対してメンバーの誰もが平等な立場のもとでディスカッションを行い、基本的にはテックリードや他のメンバーのサポートのもとにマージの最終的な判断はPull Requestの作成者が行うべきではないかと思います。

その状況を作るために出来ることのひとつは、承認という言葉をやめることです。 既読、程度の意味合いの言葉にとどめるべきでしょう。その程度であればレビュイーは既読が付いただけで安心感を持てるはずもなく、レビュアーが権威化することもなく、同時にコードレビューがコンスタントに行われていることをある程度は保証できるでしょう。
ただし、残念ながらGithubなどでは承認という言葉を変えることはできません。現実的には、チーム内でその意味を読み替えて利用する程度が関の山で、その程度ではその意識が希薄化すればいずれ同じ状況に戻ってしまうでしょう。

もうひとつの方法は、Pull Requestの承認機能を利用しないことです。承認が無い状況ではレビュイーは十分な自信と覚悟がなければそれをマージすることができません。レビュアーが権威化することもなく、フラットにディスカッションが行えるでしょう。
ただし、残念ながらコードレビューがコンスタントに行われていることを保証することはできません。レビューが行われなくともマージはできてしまうので、オーナーシップの持ち方やスキルが未熟なメンバーがいれば結局ルールとしてレビューと一定のメンバーによる承認を必須化せざるを得ないかもしれません。

いずれの方法も銀の弾丸ではありません。しかし、オーナーシップの希薄化やレビュアーの権威化に悩まされている場合は一考の余地があるのではないかと考えています。

また、それと同時に、もし曖昧にコードレビューを行っている場合はその目的意識をはっきり持ち、同時にそれぞれのメンバーがオーナーシップを持って開発を行うことができているかを考えたほうが良いのではないかと思います。

もし、自分自身を内省してコードレビューの目的をうまく認識できていなかったり、安易にPull Requestを出してしまうことがあるとすれば、その姿勢について考え直してみることをおすすめします。

*1:残念ながら、コードレビューの目的をはっきり意識してそれを行うことができているチームは少ないのではないのかと思いますが……。

*2:依頼という行為を行わず、気が向いたメンバーなどでボランティア的にレビューを行うチームもあると思います。

*3:承認機能を利用しないケース、利用しないチームもあると思います。

*4:ただし、実装やPull Requestの作成が素早いことそのものは何の問題もなく、素晴らしいことです。

*5:ただし、これはあくまでもチーム内でそれを行う場合の話であり、チーム外からPull Requestを送って取り込んでもらう場合は例外です。

*6:単なるサンプルや提案を意図したものはこの限りでありませんが、その場合はそれはそのままマージせずに清書して新たにPull Requestを作成することが望ましいでしょう。