On 26 July, #7647 merged. It
was the first piece of my project to land in CircuitVerse's master branch, fourteen
days after I opened it and about seven weeks into the project.
It is a small change. It makes sure an LTI 1.1 grade is written to CircuitVerse's
database before it is pushed to the LMS gradebook, so the two cannot silently disagree
when a local save fails. Plus the fix from last week's review — clearing stale grading
context only after a launch has been verified, so a forged request cannot wipe a
teacher's session on its way to being rejected.
Most of this week was spent not writing code, which is a thing I want to write about
honestly rather than skip over.
What actually shipped
The merged change has two halves, and the second only exists because of review.
The first is ordering: persist the grade, then push it to the LMS. Previously the
passback ran first, so a local save that failed validation left the gradebook holding a
score CircuitVerse had no record of. The LMS is the more visible system, which makes
this the worse direction to be wrong in — a student and an instructor both see a grade
that does not exist anywhere in our database.
The second is the fix from anxkhn's review: stale grading context is now cleared only
after a launch has been verified, not before. An unauthenticated request that gets
rejected can no longer wipe a teacher's in-progress grading session on its way out.
Both are small. Neither is glamorous. Together they mean the gradebook and CircuitVerse
cannot silently disagree, in an integration that schools are using today, which is worth
more than another thousand lines of the 1.3 work sitting unmerged on a branch.
The shape of the week
My commit graph for 16–25 July is empty. Two pull requests were open and waiting
(#7654 and
#7659), one was awaiting the
final look before merge, and the next thing in the plan — the JWT validator — depended
on decisions inside them.
Early in the project, a week like this would have gone one of two ways. Either I would
have sat refreshing the PR page, or I would have started building three PRs ahead and
piled up work that might need reworking depending on how the review went.
I had done the second thing already, in weeks 2 and 3, and it is how I ended up with a
2,000 line branch nobody could review.
What review latency actually is
The reframe that helped: maintainer response time is not an obstacle in the process. It
is the process.
CircuitVerse's maintainers review contributions alongside their own jobs. A day or two
of latency on a pull request is not neglect, it is the normal operating speed of a
volunteer-maintained project. Every plan I make has to be built for that speed rather
than in spite of it.
Concretely, that changes what "being blocked" means. If PR N is in review and PR N+1
depends on it, I am not blocked from working — I am blocked from committing to a
particular implementation of N+1. Those are different, and the gap between them is
where the useful work of a waiting week lives.
What I did with it
Read the review comments properly. Not just applying them, but working out the
general rule behind each. anxkhn's note about clearing grading context before
verification was specifically about one method, but the rule it implies — nothing that
mutates state may happen before the security check, including deletes — is one I could
then apply to code they had not looked at. A review comment is a sample from a
distribution of mistakes I am prone to. Fixing the sample is the minimum; finding the
distribution is the value.
Wrote the next PR's tests before its code. The JWT validator was next, and while I
did not want to commit to its implementation before the deployment model merged, the
attacks it must survive did not depend on any of that. A token signed with the wrong
key. A token with alg: none. An HS256 token where the validator expects RS256 —
algorithm confusion, where an attacker signs with the public key as an HMAC secret and a
naive validator accepts it. Expired tokens, wrong issuer, wrong audience, missing
subject, replayed nonce.
Writing those first meant that when the validator got written, its shape was determined
by what it had to refuse rather than by what I found convenient to implement.
Read other people's LTI implementations. Specifically for how they handle key
rotation — a platform can publish a new signing key at any time, and a tool that caches
the key set has to notice. The pattern I settled on: cache the key set, but if a token
arrives with a kid that is not in the cache, refetch immediately rather than waiting
for the cache to expire. Rotation is exactly the case where the cache is stale, so
treating a cache miss as a rotation signal is the right default.
Kept the queue moving. The point of the twenty-four PR plan is a steady stream of
small reviewable things. That only works if the queue is never empty from my side, and
never so full that it becomes a burden. Two open at a time, both small, both green,
turned out to be about right.
On the size of the win
Fourteen days from open to merge, for a change of about 150 lines that fixes a real bug
in a feature real users have today.
Measured in lines of code per day it is not much. But the previous seven weeks produced
zero merged lines, and the thing that changed was not that I started working harder — it
was that I started sending work in a form somebody could say yes to.
That is the actual lesson of the first half of this project, and I could not have
learned it by reading about it. I had to spend three weeks building something good that
could not merge.
There is a version of this project where I never write the POC, go straight to small
PRs, and have six things merged by now. I do not think I would have understood why
they had to be small. The POC being closed is what made the constraint make sense rather
than feel like bureaucracy.
Where things stand
- Merged: the 1.1 grade passback fix.
- Approved and waiting: the deployment model, and the JWKS and tool configuration endpoints.
- Next: the JWT validator — the security-critical brick, reviewed on its own.
Next week: the token validator, and finding out my key-resolution code had two failure
modes I had not handled.
Top comments (0)