There’s a mailbox I worry about more than the others. If you find a hole in something I’ve published there’s an address to send it to, and what turns up there is encrypted, because a vulnerability report is about the only sort of message where being read in transit is the whole disaster. The code that opens those is mine (a small tool called sigillum and two encryption modules underneath it), so when I set a heavyweight review running over the lot of it late one night I was braced to care quite a lot about what came back.

I did care, as it turned out… just not about the findings.

Forty-two notes saying “it’s fine”

The findings themselves were fine, in the sense that they were ordinary and fixable and what you run a review to get. What stopped me was sitting underneath them: forty-two comments scattered across the three modules, each one telling a checker to leave a particular line alone.

//nolint:gosec // bounded by the caller.

If you’ve not met one before, //nolint is the Go convention for switching a check off on a single line, and gosec is the security checker. So what that comment actually says is yes, this looks dangerous, and I’m telling you it isn’t, which makes it a strange sort of comment. What it’s really doing is vouching for the code.

Whoever wrote it looked at that line, decided the checker had it wrong, left a note saying so, and then left the building. Whoever reads it next takes their word for it, and why wouldn’t you? Someone clearly thought about it, they’ve written it down, and the alternative is going and working out the bounds of a bit-shift yourself at the end of a long day. So you nod and move on, and that’s how the trick works: forty-two character references, none of which I could have told you the contents of.

So I didn’t want a count. I wanted the list, one line at a time, so I could see what each was covering for.

Some of them showed their working

Line them up next to each other and they stop looking interchangeable.

// bounded above.
// bounded by the input length.
// seconds since epoch, well inside uint32 until 2106.

That third one is doing proper work. It names the quantity, it states the bound, and it tells you the year the claim stops being true, which means you can check it in a few seconds and satisfy yourself that it holds. It’s a reason, and it behaves like one. The first two are vibes.

“Bounded above” by what, exactly? And “bounded by the caller” is the worst of the lot, because it’s a promise about every call site that exists today plus every one that gets added next year, made in a file where you can’t see a single one of them. It’s about as reliable as a bloke in the pub telling you his mate will definitely pay you back.

They arrived in days

This wasn’t years of accumulated debt. Those forty-two arrived in days. Most of that code was written by agents working through review findings at speed, and the awkward part is that they were doing what I’d asked them to.

That’s the problem, really. Ask for a clean lint run and there are two ways to get one: fix the code, or tell the checker to shut up. Only one of those requires you to understand the finding, and the other is available pretty much every time, on any line, for free.

I’ve written before about an agent that has to make the build pass and about the gap between a claim of success and a green pipeline, and this is that pressure one floor further down. There the agent said it had finished; here it manufactured the finish and left a polite note explaining why that was fine.

When I write one of these at the end of a long day I at least feel a bit grubby about it, and I suspect a loop doesn’t feel anything of the sort (nor does it get bored of taking the cheap option).

So I went through them

Simple ones first, then whatever was left standing. The true ones mostly stopped needing a comment at all, which I hadn’t expected: a counter that had been excused on the grounds its value was bounded now gets checked, and a bare 3 that got waved through as obvious now multiplies two named quantities so the number explains itself. That’s the pattern, and I was slower to spot it than I’d care to admit, because most of these were the linter being right and me being in a hurry.

The false ones were more interesting. Several of those narrowing conversions really were unchecked. “Bounded by the caller” was load-bearing and unproven, so there were real defects behind those notes, with a reason draped over them like a dust sheet.

And then there was this one

Mid clean-up, a new directive turned up.

//nolint:errcheck // the read below is what matters

It was on a Close(), and it’s backwards.

If you’ve not worked with OpenPGP before, this catches people out. An encrypted message carries a modification detection code, the MDC, which is how you know nobody meddled with the ciphertext on its way to you, and it isn’t checked as you read the message. It’s checked when you close the reader, once every last byte has gone past. So the error being thrown away there was the integrity check… all of it!

A message that decrypts is not the same as a message that arrived intact, and that cheerful little note had the two the wrong way round with enough confidence to stop anyone looking. Ship it and sigillum decrypt writes attacker-modified vulnerability reports into my security rota and exits zero, which is about the worst thing that tool could do.

It never landed. It got caught in review on the very pass that was busy clearing out its forty-two cousins, which is a neater demonstration than anything I could have argued for. A test covers it now, and I’d rather have the test than the argument.

The good ones still need somewhere to live

Some exceptions are real, though, and the sanctioned move is to lift them off the line and into .golangci.yaml, scoped to the one file entitled to it, with the reasoning written beside it. There’s one case in this code that deserves exactly that.

OpenPGP identifies a key by its fingerprint, and the spec defines that fingerprint as a SHA-1 hash. SHA-1 is broken, which is why the security checker objects to seeing it, but it isn’t being used as a security guarantee here, only as a name, and a name only works if everyone computes the same one. Reach for something stronger and you’ve produced a key that no other OpenPGP implementation on the planet can match against.

So you use SHA-1, and you write down why, and that’s a paragraph of argument instead of six words at the end of an import, somewhere a reviewer will actually meet it.

Then the fix was worse

So that’s what I did, and it looked lovely for about six commits, right up until I noticed that three of those exclusions named linters that weren’t in the enable list.

They suppressed nothing! Those linters hadn’t been failing, they were never going to fail, and the careful reasoning I’d written to govern the whole module wasn’t governing anything. There was even a comment in there claiming that the same construct introduced anywhere else would still get caught, which it wouldn’t, with no linter there to watch.

That’s worse than what it replaced, and I took a minute to admit it. An inline directive at least sits next to the line it excuses, where a reader trips over it and has a fighting chance of arguing back. A dead exclusion is a paragraph of confident prose vouching for a rule that isn’t running, filed somewhere a reader rarely goes. And you won’t hear about it, either: golangci-lint will not mention that you’ve excluded a linter it had no intention of running. Why would it.

What couldn’t be fixed got a number

Not all of it could be cleared where it stood. go/signing had its own crop and untangling those wasn’t part of that day’s work, so they became an issue instead… chore: remove the 16 inline nolint directives.

That’s most of the argument in one filing decision, really. The exception still exists, it’s still unresolved, nothing has been fixed by writing it down. It’s just in a tracker with a number on it now, instead of whispering from the end of a line where I’d probably not have gone looking.

It’s a cousin of waivers with an expiry date, where the argument was that a dependency waiver needs a date on it so it can’t outlive its reason. Same instinct, different beast: that one is about a waiver going stale, this one is about a note that never said anything to begin with.

Who’s actually listening

There’s one inline directive left across the three modules now, in a sigillum test, and it carries its reason with it (“identity is the point”). You can go and check that rather than take it from me. What I’d keep out of all this is a test. It reads .golangci.yaml, pulls out every linter named by a file-scoped exclusion, and fails if that linter isn’t switched on:

%q is excluded for a path but is not enabled, so the exclusion suppresses nothing and its written reasoning governs nothing.

Forty lines and no dependencies, and it exists because the fix I wrote had failed just like the notes it was replacing… a character reference, written in my own hand, for someone who’d never turned up.