From 175d21d11c8f2d7d93dddefd58880c5005a56981 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Wed, 2 Sep 2026 11:13:02 -0500 Subject: [PATCH 1/2] fix(scripts): refuse to reuse a backend that is serving the previous database (#257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit start-local.ps1 said "something is already listening on 3000; leaving it alone" and carried on. That is safe only while the database has not changed underneath that process. When it has, the old backend is serving a database it no longer owns and its in-memory state describes rows that no longer exist. The rate limiter is the sharpest example. It keys on customer id and keeps buckets in memory for an hour, so a recreated database restarting ids at 1 hands a brand-new customer a previous run's spent allowance. That is #257: the resend-verification allowance test failing roughly one full run in three, never in isolation, with three refusal toasts where one was expected. Reproduced deterministically rather than reasoned about. Fresh database and fresh backend: three of three pass. Recreate the database only, leaving the same backend running: the same test fails with exactly the reported "resolved to 3 elements". Control — recreate the ids again but restart the backend as well: passes. So the variable is the process outliving the database, not the id restart on its own. That also explains why #257 could not find the mechanism. It had ruled out contention, a mis-keyed limiter, a shared fixture and identity reuse in the test helpers, all correctly. The recycling happens outside the suite entirely, in a process the suite never sees. Now it refuses, names the reason, and says to run -Stop. A run that stops loudly is recoverable; one that quietly tests the wrong thing is not — and a stale listener on 3000 has already produced two wrong measurements in this project, a rate-limiter reading and an e2e run reported as 23 passed when the backend was talking to a deleted database. DatabaseIsNew is set when -Fresh removes the container, when the container is created, and always under -E2eDb, whose tmpfs storage means it comes up empty whether created or restarted. Verified by AST-parsing the script and confirming every reference to the flag is script-scoped — a function-local read would see $null and the guard would never fire. The script is deliberately never executed from an agent shell. Co-Authored-By: Claude Opus 5 --- scripts/start-local.ps1 | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/scripts/start-local.ps1 b/scripts/start-local.ps1 index d3b7348..28af2cc 100644 --- a/scripts/start-local.ps1 +++ b/scripts/start-local.ps1 @@ -65,6 +65,11 @@ $ErrorActionPreference = 'Stop' $RepoRoot = Split-Path -Parent $PSScriptRoot $StateDir = Join-Path $RepoRoot '.local' $PidFile = Join-Path $StateDir 'pids.json' + +# Whether this run is starting from an empty database. Set below, and read by +# Start-Backend, which must refuse to reuse a backend that is still serving the +# previous one. See #257. +$script:DatabaseIsNew = $false # Which database this run uses. Everything downstream reads these four, so the # choice is made once here rather than branched at each use. if ($E2eDb) { @@ -184,14 +189,22 @@ function Start-Database { Write-Step 'Removing the existing database (-Fresh)' docker rm -f $Container *>$null Write-Good 'removed' + $script:DatabaseIsNew = $true } + # The e2e container stores its data on tmpfs, so it comes up empty whether it + # is created or merely restarted. There is no case where it carries anything + # over, which is the point of it (#186) — and it means a backend from a + # previous run is always stale against it. + if ($E2eDb) { $script:DatabaseIsNew = $true } + $existing = (docker ps -a --filter "name=^/$Container$" --format '{{.Names}}') if ($existing -eq $Container) { Write-Step "Reusing the database container" docker start $Container *>$null } else { + $script:DatabaseIsNew = $true Write-Step "Creating the database container on port $DbPort" # tmpfs for the e2e database only: its contents are worthless the # moment the run ends, and storing nothing is what makes it start empty @@ -269,6 +282,41 @@ function Start-Backend { finally { Pop-Location } if (Test-Listening -Port $ApiPort) { + # Leaving a stranger's backend alone is only safe when the database has + # not just changed underneath it. When it has, that process is serving a + # database it no longer owns, and its in-memory state describes rows that + # no longer exist. + # + # That is not theoretical. The rate limiter keys on customer id and keeps + # its buckets in memory for an hour; a recreated database restarts ids at + # 1, so a new customer inherits a previous run's spent allowance. It + # reproduced exactly the intermittent resend-verification failure in #257 + # — three refusal toasts where one was expected — and the control (same + # id restart, restarted backend) passed. Whatever else is stale, a + # process serving the wrong database gives wrong answers confidently, + # which is the worst kind. + # + # So this refuses rather than continuing, and says what to do. A run that + # stops loudly is recoverable; one that quietly tests the wrong thing is + # what cost two wrong measurements in this project already. + $tracked = Get-TrackedProcesses + $ours = $tracked.ContainsKey('backend') -and + (Get-Process -Id $tracked['backend'] -ErrorAction SilentlyContinue) + + if ($script:DatabaseIsNew -and -not $ours) { + throw @" +Something is already listening on $ApiPort, and this run just created a new database. + +That process is serving the previous database. Its in-memory state — rate-limit +buckets keyed on customer id, most obviously — describes rows that no longer +exist, and recycled ids will inherit them (#257). + +Stop it and run this again: + + .\scripts\start-local.ps1 -Stop +"@ + } + Write-Note "something is already listening on $ApiPort; leaving it alone" return } From d75c45cf914e3cd9f1c07998f0bcea686fac4dec Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Wed, 2 Sep 2026 11:36:42 -0500 Subject: [PATCH 2/2] chore(sonar): name the findings instead of counting them (#181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The measures step reported "Quality gate ERROR" and "Code smells 5", which is enough to notice debt and useless for clearing it. It now also prints the failing gate conditions, the open issues with their rule, file, line and effort, and the security hotspots awaiting review. #181 is why this matters. It assumed the five smells were the five eslint-plugin-sonarjs warnings, on the strength of the counts matching, and hedged that the server's rule set is not the plugin's. The hedge was right: those five warnings were fixed under #261, both workspaces lint at zero, and the analysis of the #261 merge still reported five smells and 24 minutes. They are a different five, and nothing short of the list settles which. The gate condition list matters for the same reason. "ERROR" sends a reader to a dashboard, which is the thing this script exists to avoid needing. Issues are capped at 25 rather than paged: past a couple of dozen the answer is not "read the list", and an unbounded fetch on every CI run is a cost with no reader. Verified against a stub SonarQube serving canned responses, so the parsing and formatting are exercised rather than reasoned about — failing conditions filtered from passing and NO_VALUE ones, components stripped of their project-key prefix, absent metrics rendered as a dash, and every path still exiting 0. The no-server and unreachable-server paths were exercised too; the step still cannot fail the job. Co-Authored-By: Claude Opus 5 --- scripts/summarize-sonar.js | 91 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/scripts/summarize-sonar.js b/scripts/summarize-sonar.js index ac3faae..e7bc99f 100644 --- a/scripts/summarize-sonar.js +++ b/scripts/summarize-sonar.js @@ -1,5 +1,6 @@ #!/usr/bin/env node -// Prints the project's SonarQube measures into the job log. +// Prints the project's SonarQube measures, its failing quality gate conditions, +// its open issues and its unreviewed security hotspots into the job log. // // This exists because the numbers are otherwise unreachable. The server is // SonarQube 9.9 Community: it has no Bearer auth, so the official MCP cannot @@ -183,12 +184,96 @@ async function main() { const gate = await get( `${base}/api/qualitygates/project_status?projectKey=${encodeURIComponent(PROJECT)}` ); - const gateStatus = parse(gate.body)?.projectStatus?.status; - lines.push('', `Quality gate ${gateStatus ?? 'unknown'}`, ''); + const projectStatus = parse(gate.body)?.projectStatus; + lines.push('', `Quality gate ${projectStatus?.status ?? 'unknown'}`); + + // Which conditions failed, not just that the gate did. "ERROR" on its own + // sends a reader to the dashboard, which is the thing this script exists to + // avoid needing. + const failed = (projectStatus?.conditions ?? []).filter( + (c) => c.status !== 'OK' && c.status !== 'NO_VALUE' + ); + if (failed.length > 0) { + lines.push('', 'Failing gate conditions:'); + for (const c of failed) { + lines.push( + ` ${c.metricKey}: ${c.actualValue} (${c.comparator} threshold ${c.errorThreshold})` + ); + } + } + + lines.push(...(await describeIssues(base))); + lines.push(...(await describeHotspots(base))); + lines.push(''); console.log(lines.join('\n')); } +/** + * The open maintainability issues, named. + * + * A count is enough to notice debt and useless for clearing it. #181 assumed + * the five smells were the five eslint-plugin-sonarjs warnings, because the + * counts matched — they were fixed, and the count stayed five, so they were a + * different five. Nothing short of the list settles that. + * + * Capped rather than paged: past a couple of dozen the answer is not "read the + * list" anyway, and an unbounded fetch on every CI run is a cost with no reader. + */ +async function describeIssues(base) { + const res = await get( + `${base}/api/issues/search?componentKeys=${encodeURIComponent(PROJECT)}` + + `&resolved=false&types=CODE_SMELL,BUG,VULNERABILITY&ps=25&s=SEVERITY&asc=false` + ); + if (res.status !== 200) return ['', `Open issues: could not fetch (status ${res.status}).`]; + + const json = parse(res.body); + const issues = json?.issues; + if (!Array.isArray(issues)) return ['', 'Open issues: unexpected response shape.']; + if (issues.length === 0) return ['', 'Open issues: none.']; + + const total = json.total ?? issues.length; + const out = ['', `Open issues (${issues.length} of ${total}):`]; + for (const issue of issues) { + // The component is "projectKey:path"; the key is noise in a log line. + const where = String(issue.component ?? '').split(':').slice(1).join(':') || issue.component; + const at = issue.line ? `:${issue.line}` : ''; + out.push(` [${issue.severity}] ${where}${at}`); + out.push(` ${issue.rule} — ${issue.message}`); + out.push(` effort ${issue.effort ?? issue.debt ?? 'n/a'}`); + } + return out; +} + +/** + * Security hotspots awaiting review. + * + * A hotspot is not a defect — it is a place the scanner wants a human to say + * whether the surrounding code is safe. That review happens in the dashboard + * and cannot be done from here, so the point of listing them is to say what is + * waiting and where, rather than to fix anything. + */ +async function describeHotspots(base) { + const res = await get( + `${base}/api/hotspots/search?projectKey=${encodeURIComponent(PROJECT)}` + + `&status=TO_REVIEW&ps=25` + ); + if (res.status !== 200) return ['', `Security hotspots: could not fetch (status ${res.status}).`]; + + const hotspots = parse(res.body)?.hotspots; + if (!Array.isArray(hotspots)) return ['', 'Security hotspots: unexpected response shape.']; + if (hotspots.length === 0) return ['', 'Security hotspots awaiting review: none.']; + + const out = ['', `Security hotspots awaiting review (${hotspots.length}):`]; + for (const h of hotspots) { + const where = String(h.component ?? '').split(':').slice(1).join(':') || h.component; + const at = h.line ? `:${h.line}` : ''; + out.push(` ${where}${at}`); + out.push(` ${h.ruleKey ?? h.securityCategory} — ${h.message}`); + } + return out; +} + main().catch((err) => { // Reported rather than thrown, for the reason in the header: this step must // not be able to fail the job.