Week 14: The Approved Pull Request That Stopped Passing

#7746, the OIDC login
initiation, has been approved and waiting since week 10. This week I went back
to actually merge it and found its test suite failing on an error that had
nothing to do with anything I remembered writing.

A NameError in code I had already shipped

CI was red across the whole oidc_login context, ten examples, all the same
failure:

NameError:
  uninitialized constant LtiController::LTI_STATE_PURPOSE

That should have been impossible. oidc_login and lti_state_verifier both
reference LTI_STATE_PURPOSE, and the request spec references
LtiController::LTI_STATE_PURPOSE directly to verify the state it gets back.
If that constant had never existed, this whole context should have failed the
day it was written, back in week 10. It did not. Something had removed a
constant that used to be there.

Tracing it

git log -p on the controller file gave a clean timeline. The constants first
appear on 5 August, in the commit that adds oidc_login itself, with a comment
explaining why the state is signed rather than kept in the session. They survive
the review fixes from week 10. They even survive a rough merge from master on 9
August that briefly left the file unparseable, a splice that dropped the closing
end of oidc_login and spliced two of master's filter lines into the method
body. The very next commit repairs that, and its diff still shows the constants
sitting untouched at the top of the class, comment and all.

The actual disappearance happened somewhere I was not looking. On 21 August, a
different contributor merged a repository wide change to master, adding
after_action :verify_authorized safety nets across dozens of controllers. On
this controller, on master's copy of the file, that meant inserting
skip_after_action :verify_authorized as the very first line inside the class
body. Master's copy at that point had no LTI_STATE_PURPOSE constants, because
none of this branch's work had merged into master yet.

On 5 September I merged master into my branch again, to catch up before opening
the merge. Both sides had inserted a new line in exactly the same spot, right
after class LtiController < ApplicationController: master's
skip_after_action, and my branch's constants with their comment. Diffing the
file immediately before and after that merge shows exactly what happened:

 class LtiController < ApplicationController
-  # The state is signed rather than kept in the session: the launch returns as a
-  # cross-site POST that a SameSite cookie would not survive, and the signature
-  # is what proves the launch answers an initiation we made.
-  LTI_STATE_PURPOSE = "lti.launch.state"
-  LTI_STATE_TTL = 5.minutes
+  skip_after_action :verify_authorized

No conflict markers survive in the final commit, which is the part that makes
this easy to miss. Two branches touching the same handful of lines usually forces
a manual conflict resolution, and somewhere in resolving that one I kept
master's line and let mine go, without noticing the branch had put something in
that exact spot too. A merge that finishes clean does not look like something
that needs a second look, which is exactly why this one went unnoticed for two
weeks.

The fix

Both constants, reinstated:

LTI_STATE_PURPOSE = "lti_oidc_state"
LTI_STATE_TTL = 5.minutes

The purpose string itself does not need to match the original literal, nothing
outside this file ever reads it, only the two internal call sites and the spec
need to agree on the same constant, which they now do by definition. The TTL
does need to stay at five minutes, since the spec travels six minutes forward
and checks the state is gone by then. I confirmed nothing else in the codebase
referenced either constant, ran the project's RuboCop config against the change
to be sure the addition was clean, and pushed. CI went from ten failures back to
green across every job, not just the one that had been failing.

What this says about "approved and waiting"

An approved pull request sitting unmerged is not a pull request that is done.
It is a pull request whose last green check run is quietly getting further from
the truth every time master moves underneath it, and nothing about the GitHub UI
makes that visible. The checkmark from three weeks ago looks exactly like a
checkmark from this morning.

This is the same lesson as week 10's development database drift, just at a
different layer. State that nobody actively re-verifies accumulates damage that
stays invisible until something finally runs against it again. There, it was a
migration nobody had rolled back. Here, it was a merge nobody had rerun. Both
times the fix was cheap once found and the finding was the entire cost.

The practical rule I am taking from this: before merging anything that has sat
approved for more than a couple of weeks, rebase it against current master and
rerun the suite first, rather than trusting a check that ran against a branch
state that no longer exists.

Where the project stands

#7746 is rebased, fixed, and green again, waiting on nothing but the merge
button. #7764, the access token exchange, is still open from last week. The
testbench model and runner from two weeks ago are both under review. The test
case editor is still finished and waiting locally.

Next week: actually watching the login initiation merge, and picking the editor
back up now that it has an access token to eventually authenticate against.