← Insights

The Finding Count Fell and the Severity Did Not

Five review passes over one backend in seven days. The finding count fell every time, the top severity band did not move until the last pass, and two of those top findings were defects the pass before had introduced.

Five security review passes over one serverless backend in seven days returned twenty-eight findings, then sixteen, then twelve, then eleven, then six. Every one of those numbers comes from the commit that closed the pass it belongs to. It is the shape a converging review program is supposed to have, and I was reading it as evidence that the system was getting safer.

The severity says something else. Three of the five passes filed criticals. The fourth filed six findings under a single heading reading Critical/High: and never separated them. Only the fifth came back with nothing worse than a medium. Two of those top-severity findings were defects that the pass immediately before had introduced.

Each pass read a different codebase

Here is every pass, with the finding count from its own commit message and the most severe thing in it.

Pass Findings Top band The worst thing it found
T+0 28 Critical Lambda has no route out and cannot reach Cognito to validate a token
T+37m 16 Critical The firewall the previous pass added is attached to nothing
T+3d18h 12 Critical A failed identity delete leaves an account that can sign back in
T+3d18h37m 11 Critical/High The sign-in guard the previous pass wrote runs after the write it prevents
T+6d21h30m 6 Medium The only two write endpoints in the API with no rate limit

Row two is the re-audit of row one, thirty-seven minutes behind it, and that pair is a story of its own that I wrote up last week. What matters here is that it belongs in the sequence at all. It read a diff rather than a repository, exactly as row four read row three, and it filed a critical.

I expected the first pass to be the real review and the rest to be cleanup. That is how review normally gets budgeted: one deep look, a punch list, then the punch list gets worked. What the table says instead is that these were five reviews of five different codebases, and each one could only have found what it found because the one before it had landed.

Two of the first pass's three criticals were absences rather than mistakes. The Lambda functions sat in private subnets with no gateway out and no route table associations. In the review document's words: "At runtime, Lambda cannot reach Cognito (AdminDeleteUser, JWKS for JWT validation), SSM Parameter Store, or Apple Sign-In token endpoints. This is a functional blocker — the API will not work at all." Express 4 does not catch rejected promises from async handlers, so a database error in any of twenty-one routes would have taken the process down.

Nothing written was wrong; something outside what was written was missing. That is what a reviewer with no priors finds, because every part of a system it has never seen is equally a candidate and the quickest way through is to check what is not there.

The count measures the reviewer's input, not the system

The first pass had a whole repository in front of it, and every structural decision in that repository was unexamined. Every pass after it read a diff. That is why the first number is twenty-eight and every number after it is smaller, and it is a fact about what went into the review rather than about what came out of it. The same descending series is produced by a system getting safer and by reviewers running out of things they have not already looked at.

The counts of what each pass changed do not even fall: thirty-three files, then thirteen, sixteen, fourteen, seven. The third pass's sixteen include a 2,736-line certificate bundle for the database connection that nobody reviewed and nobody should have. A number that wobbles is at least honest about wobbling. The finding count fell smoothly enough to look like a measurement.

Two passes filed a top-band finding the previous pass created

The second pass filed REGRESSION-1: the web application firewall that the first pass had added to satisfy a finding was declared in the template and bound to no resource, so it inspected nothing. A finding closed by a commit, reopened by the next reader of that commit.

The fourth pass filed the same shape against the third. Fixing a deleted account that could sign back in, the third pass had put a check after the write it was meant to guard:

const result = await db.insert(users)
  .values({ ... })
  .onConflictDoUpdate({ ... })
  .returning();

if (result[0].deletedAt) {
  throw new DeletedUserError();
}

The fourth pass moved it onto a read that happens before any write, filed under Prevent GDPR PII re-write: check deletedAt before upsert in findOrCreateUser, not after:

const existing = await db
  .select({ id: users.id, deletedAt: users.deletedAt })
  .from(users)
  .where(eq(users.cognitoSub, cognitoSub))
  .limit(1);

if (existing.length > 0 && existing[0].deletedAt) {
  throw new DeletedUserError();
}

No amount of re-reading the first pass's document would have surfaced either of these, because neither defect existed when the document was written.

Neither version of that guard can fire

Both versions find the deleted row through the same column. The third pass's upsert conflicts on users.cognito_sub, which carries the table's only unique index; the fourth pass's select filters on it. Deletion does not leave a row with that value in it.

The first pass had already closed a finding of its own. H6 made account deletion null the email, display name and avatar and overwrite cognito_sub with the string deleted: followed by the row's own identifier, in a single statement that is also the only place in the codebase that ever sets deleted_at to anything. So the upsert finds no conflict and inserts a fresh row whose deleted_at is null, and the fourth pass's select matches nothing at all. Three passes wrote, moved and approved a guard that the first pass had made unreachable on day one.

The user is not refused. They get a clean new account, which is closer to the intent of a deletion request than the resurrection the third pass was guarding against, and nobody chose it. Nothing about the original finding changed. What changed is that I now know none of the reviewers checked whether the row they were guarding could still be found.

The fifth pass found one wrong word

Both security findings in the last pass were the same finding: PUT /users/me and PUT /users/me/settings were the only write endpoints in the API without a rate limit, and they now carry fifty and one hundred requests per day. Everything else on its list was code quality, and one of those was a single wrong word in a comment. This line sat in src/utils/cognito.ts:

// [L3] Hash cognito_sub in logs to avoid PII exposure

The code below it truncates the identifier. The original finding's remediation line reads "Hash or truncate in logs," so truncation was one of the two remedies it accepted: the control was correct, and the sentence describing it had been wrong since the first pass wrote both of them into the same commit. The third pass edited the same catch block, nine lines below the comment. The fourth pass edited the same function, eighteen lines above it. The fifth changed Hash to Truncate and nothing else.

That pass also labeled its two rate-limit findings M2 and M3. In the first pass's document, M2 and M3 are an unvalidated score payload and a race in game submission, both closed on day one. The identifiers were reused because nothing was reading the earlier document, which is the same reason the comment survived three passes: each reviewer read the code in front of it, and personally identifiable information in a log line is not something the code will tell you about.

The strongest objection

The strongest objection is that the flat severity band is the review program working, not failing. Remediation is new code; new code carries defects; a loop that catches a critical regression thirty-seven minutes after introducing it is doing exactly what it exists to do. On that reading, four passes in a row topping out at critical or high is a healthy signal, and expecting a decline was my error rather than the data's.

That is right, and it is the reason I did not stop running passes. Where it fails is the use I was going to put the curve to. I wanted the shape to tell me when to stop, and the shape cannot tell me that. The finding I most needed was invisible to five consecutive passes precisely because it is not a defect in any one file. It is two functions disagreeing about the same column, and nothing that reads a diff will see it.

What I ask instead of reading the trend

Two questions, both asked of the most recent pass alone rather than of the series. Would the worst thing it found have woken someone up? If yes, run another pass, because the distance between that finding and the last one says the system is still moving. And: does anything in this pass show a control being exercised rather than read? If every finding is a fact about source text, the passes have converged on each other and not on the running system.

The boundary is cost. Five passes in seven days over one repository was possible because each was a language model reading the whole codebase in one sitting, in the three roles the first pass's document names: a security auditor, a cloud architect, and a code quality reviewer. All five fix commits carry the same co-author trailer, which is how I can say the arrangement did not change between them. The cadence went into a release-readiness checklist the following day, 152 lines covering weekly maintenance, post-feature checks and pre-release validation, because once a pass is that cheap the expensive part is remembering to run it.

If a review pass costs a week of a senior engineer's time, you get one pass, and one pass tells you nothing about the shape of anything. It tells you what a stranger noticed on a Monday.

The comment is fixed now, and the rate limits on those two endpoints are real and they work. There is also a test called returns 401 for deleted users, and it passes. It mocks findOrCreateUser to reject with a DeletedUserError, so what it establishes is that the middleware turns that error into a 401; it never calls the function that would have to raise it. The workflow test that deletes an account stops at confirming the row is marked deleted, and never tries to sign back in. Five reviews read all of that, and the number falling from twenty-eight to six is why none of them looked twice.