Skip to content

test: promote five regression anchors, each measured green on main and red without its fix - #2094

Merged
kriszyp merged 10 commits into
mainfrom
kris/qa-promote-measured-anchors
Aug 29, 2026
Merged

test: promote five regression anchors, each measured green on main and red without its fix#2094
kriszyp merged 10 commits into
mainfrom
kris/qa-promote-measured-anchors

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 5, 2026

Copy link
Copy Markdown
Member

Five exploratory-QA specs promoted to permanent regression anchors. Test-only, no product code.

Every one was measured twice — green on current main, and red with the commit that fixed the issue it anchors rewound. Only the delta is evidence, so both numbers are stated:

spec anchors fixed by green on main failures with the fix rewound
security/mcp-record-scoped-rbac #1487 #1522 (2e3620c6e) 11/11 5 of 11 ¹
server/sse-throw-midstream #1763 #1789 (8930b1ef2) 4/4 2 of 4
database/condition-mutation-integrity #1572 #1911 (b8c843a24) 17/17 12 of 17
database/eviction-phantom-null #1894 #1896 (21c521000) 4/4 3 of 4
database/longtxn-index-orphan #1407 #1411 (3249fb58e) 3/3 2 of 3 ¹

39/39 running together, on RocksDB, against main at 11a1c4891.

¹ These two rewound figures are from the original QA measurement against the head each spec was written on. Both fixes have since been built on — #1522's liveResource indirection, and #1411's abort/poison path (chainStillActive, the canonical-source exclusions from harper-pro#348) — so neither reverts cleanly onto current main, and a surgical partial rewind measures the rewind rather than the anchor. The other three rows were re-measured today by rewinding the fix on this head. Two of them moved: eviction-phantom-null is a 4-test suite (not 5), and sse-throw-midstream now fails 2 of 4 rather than 3.

What changed since this PR was opened

Why five and not thirteen

These came from 13 candidates that a citation-based gate classified as ratified invariants — each cites a closed issue, so each looked like a regression anchor. Measuring them changed the answer:

  • 7 are red on main anyway. Identical fail counts with and without the fix, so rewinding it changed nothing and the red proved nothing about detection. They fail criterion 1 (green on current main) and are not promotable in either direction.
  • 1 could not execute on main at all.

Those eight need triage — stale spec versus a genuine regression in main — and that is worth doing separately, because if any is the latter it is a defect report rather than a test problem. Listed in the branch's commit message.

This is now the third time in this batch's history that measurement contradicted a plausible classification, counting QA-686. Worth stating plainly: a citation-gated auto-merge lane would have merged all 13, including 7 tests that cannot fail, 1 that does not run, and 1 attributed to the wrong issue.

For the human reviewer

  • The base has drifted again, and CI is red on one Windows shard because of it. This branch is now 104 commits behind origin/main (df5355aaa), so the main figure above is measured against 11a1c4891, not against today's main — the same freshness objection that held this PR in draft. Integration Tests 2/6 (Windows, Node.js v24) fails twice deterministically (re-run to confirm it is not a flake) in integrationTests/apiTests/describe-metadata-upgrade.test.ts, a file this PR does not touch: HTTP workers never come back after restart_service (Probe /SeoPageCache/ did not become ready within 120000ms, ECONNREFUSED). That path has since been worked on upstream — #2316 (worker respawns blocking restarts), #2112 (swallowed uWS startup-listen failures), #2227 (Windows deploy deflake) — none of which are in this base. A base refresh, followed by re-measuring the green on main column, is the next step; it needs a force-push, so it is not something this pass could do.
  • The two runs per spec are the claim; the two ¹ rows are the weakest part of it. Their rewound figures are older than the rest of the table, for the reason given in the footnote. If you want a durable answer there rather than a footnote, the honest options are to re-derive each fix's behaviour against current main in a follow-up, or to drop the rewound column for those two rows and let the green run stand alone.
  • Declined from the pre-push review, twice each: the reviewer wants the provenance narration removed from the spec docstrings (issue numbers, the code reading, the disproven hypothesis in QA-601) as PR-ephemeral clutter, and it objects to the ordered suites sharing state across tests (pristineLive set in Q0a and read in Q2+, so a single test run in isolation fails). I left both. For these specs the narration is the artifact — the value of a promoted QA spec is that the next reader can tell what it measured and why the oracle is not vacuous — and the ordering is what makes the non-vacuity controls (Q0c proves the oracle can detect a real mutation before anything trusts its silence) meaningful. Both are worth a maintainer's opinion rather than mine.
  • countOverTimeOccurrences() counts each monitor firing twice, once from hdb.log and once from the captured process output. It is only ever compared as > baseline, and the baseline is measured the same way, so nothing is wrong today — but it is a trap for anyone who later asserts an exact count. Pre-existing; not touched here.
  • One reviewer nit left unfixed on purpose: consumeSse's error/close paths finish without calling decoder.end(), so an abruptly-closed stream that ends mid-multi-byte-sequence drops those trailing bytes rather than recording them as replacement characters. Every payload these cases send is ASCII, so it changes nothing today; it would matter only if the fixtures later yield non-ASCII at the crash boundary. integrationTests/server/sse-throw-midstream.test.ts:175.
  • Two fixture resources.js files needed unused-parameter fixes. They had never been linted, because the promotion gate globbed *.test.* only and never the fixtures sitting beside them — a gap in the gate, now known.

Verification

  • npm run build, then all five specs together: 39/39 pass, 0 fail, 0 skipped on RocksDB.
  • Re-measured with the fix rewound, on this head, rebuilding each time: condition-mutation-integrity 12 of 17 fail (removing the cloneConditions call), eviction-phantom-null 3 of 4 fail (restoring record && … in updateIndices), sse-throw-midstream 2 of 4 fail (restoring the bare body.pipe(nodeResponse) in pipeBodyToResponse). Working tree restored and rebuilt after each.
  • oxlint --deny-warnings and prettier --check clean on all five specs.
  • QA-601 still reports overTimeFired=true on both trials after the log-scanning rewrite, so the incremental counter still detects a fresh monitor firing per trial.

What these do and do not prove

They prove each spec detects the specific defect it anchors, on the engine and surface it exercises. They do not prove anything about the areas the dropped eight covered — allowRead enforcement across delivery surfaces, MCP tool authz, deployment-payload boundaries, env/config boot, redeploy staleness, astral-plane index bounds — that coverage is still missing and now has a measured reason for being missing.

Refs #1487, #1763, #1572, #1894, #1407

Generated by Claude Opus 5.

Review-Coverage: authored=claude; ran=gemini; declined=codex,cursor-grok,cursor-composer,domain; rounds=6 @ ec36e86

Human-Review-Need: 3 @ ec36e86

Comment thread integrationTests/server/log-rotation-fd-reuse.test.ts Outdated
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive suite of integration tests covering critical database and server behaviors, including query condition mutation integrity, TTL eviction index cleanup, multi-store transaction timeouts, MCP row-level RBAC enforcement, log rotation unit parsing, and mid-stream SSE generator errors. The review feedback provides highly actionable improvements to ensure these tests are robust and reliable. Key recommendations include replacing fixed sleeps with bounded polling loops to prevent test flakiness, wrapping test bodies and teardown hooks in try/finally blocks to guarantee process cleanup, and using unique temporary filenames incorporating the process ID to avoid conflicts on shared CI environments.

Comment thread integrationTests/security/mcp-record-scoped-rbac.test.ts
Comment thread integrationTests/security/mcp-record-scoped-rbac.test.ts Outdated
Comment thread integrationTests/server/log-rotation-fd-reuse.test.ts Outdated
Comment thread integrationTests/database/longtxn-index-orphan.test.ts Outdated
Comment thread integrationTests/server/sse-throw-midstream.test.ts Outdated
Comment thread integrationTests/server/sse-throw-midstream.test.ts Outdated
Comment thread integrationTests/server/sse-throw-midstream.test.ts
Each of these was verified twice: GREEN on current main, and RED with the commit
that fixed the issue it anchors rewound. Only the delta is evidence, so both runs
are stated per spec.

  security/mcp-record-scoped-rbac         #1422   11/11 green   5/11 red rewound
  server/sse-throw-midstream              #1628    4/4  green   3/4  red rewound
  server/log-rotation-fd-reuse            #683     3/3  green   2/3  red rewound
  database/condition-mutation-integrity   #1572   17/17 green  12/17 red rewound
  database/eviction-phantom-null          #1894    5/5  green   3/5  red rewound
  database/longtxn-index-orphan           #1407    3/3  green   2/3  red rewound

43/43 together, ~consistent with each spec's solo run.

Selected from 13 candidates that a citation-based gate called ratified invariants.
Seven were dropped because they are red on main ANYWAY — with identical fail counts
with and without the fix, so rewinding changed nothing and the red proved nothing.
One could not execute on main at all. Those eight need triage (stale spec vs a real
regression in main) and are not promotable either way: criterion 1 is green-on-main
and they fail it.

Also fixed here: two fixture resources.js files carried unused-parameter lint
errors. They were never caught because the promotion gate only ever linted
`*.test.*` and never the fixtures beside them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp
kriszyp force-pushed the kris/qa-promote-measured-anchors branch from a172a96 to 63c28e4 Compare August 17, 2026 11:35
@kriszyp

kriszyp commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Refreshed before flipping this out of draft, because the reason it was still open is the same thing it argues about.

It was 123 commits behind main. The green CI on it was green against that stale base — which is exactly the failure F-287 describes and the freshness assertion now added to the QA skill exists to prevent. A green on a stale main does not satisfy criterion 1, so flipping it on that evidence would have been the wrong shape of confidence for a PR whose whole claim is "measured".

Rebased onto current main (11a1c4891) and re-ran all six specs: 43/43 pass, 0 fail, 0 behind. The red-without-the-fix half is unchanged — that is a property of each spec against its own anchored fix, not of the base.

Also removed a commit that did not belong here. fix(config): extend Windows atomic-write retry budget (config/configUtils.ts) had been pushed onto this branch and existed nowhere else — no PR of its own. This PR says "test-only, no product code changes", which was no longer true. It has been relocated verbatim onto current main as #2191 so it is not lost, and dropped from here. This PR is test-only again.

Claude Opus 5

@kriszyp
kriszyp marked this pull request as ready for review August 17, 2026 11:35
kriszyp and others added 9 commits August 17, 2026 08:03
Replace fixed settling sleeps with bounded state polling, isolate the MCP diagnostic output, and guarantee fixture teardown across partial failures.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
QA-686 was promoted as `server/log-rotation-fd-reuse` and attributed to #683,
but the spec never inspects file descriptors or the fd-reopen-after-rotate path.
Its own docstring, fixture, and schema all describe maxSize unit-parsing against
#1877, which is still OPEN. Its red result with #683's fix rewound is incidental,
not a regression contract for #683, so it fails the promotion criterion that a
spec must detect the defect it anchors.

Dropping it also reconciles the batch arithmetic: the citation gate produced 13
candidates -> 5 promoted, 7 red on main anyway, 1 that could not execute.
QA-686 was never one of the 13; it targets an open issue and was swept in.

Leaves five anchors, 39/39 green on this head.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`mcp-record-scoped-rbac` already cancelled its unused response bodies; the other
four promoted specs did not. The readiness polls are the ones that matter: they
issue up to 480 requests over 120s and leaked every body, which under undici's
keep-alive pool is a wedge waiting for a slow boot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pre-push review (Gemini) on the response-body-consumption commit:

- `readDoc()` in the MCP RBAC anchor returned `null` when the ops-API read-back
  was unparsable. `null` is that oracle's "row is absent" answer, so a 500 on the
  read-back was indistinguishable from a successful denial and every
  `strictEqual(after, null)` below it would pass vacuously. Throw instead, on a
  non-OK status, on non-JSON, and on a non-array payload.
- The three ops-API seed calls cancelled the body before asserting 200, so a
  failed `add_role`/`add_user`/`insert` reported a bare status with the reason
  discarded. Read the body and put it in the assertion message.
- `getJSON`/`postJSON` in the conditions anchor had the same bare-status problem
  and additionally left the failing body unconsumed; route both through an
  `assertOK` helper.

28/28 green on the two touched specs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 2 of the pre-push review (Gemini):

- `sse-throw-midstream` accumulated the SSE body with a per-chunk
  `d.toString('utf8')`, which turns any multi-byte character straddling a TCP
  chunk boundary into U+FFFD. Decode through a `StringDecoder` instead.
- Both `uncaughtException` sweeps in that spec read hdb.log the instant the
  response closed. They assert a NON-event, so outrunning the worker's log flush
  passes vacuously and hides the exact crash they anchor; settle first.
- `mcp-record-scoped-rbac` computed `bypassed = !result.isError && persisted`,
  so a handler that writes the row and then fails while formatting its reply
  would print `ENFORCED` one line before the assertion caught it. Unauthorized
  persistence is the bypass, whatever the tool reported.
- `longtxn-index-orphan` re-read all three log files from byte 0 on every
  iteration of a 100ms/5s poll loop, rescanning a log Harper is actively
  writing. Read only appended bytes, with a per-file carry so a match is counted
  exactly once and an un-terminated trailing line is still seen.

39/39 green on the five anchors; both QA-601 trials still report
overTimeFired=true, so the incremental counter still detects a fresh firing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 3 of the pre-push review (Gemini):

- Node's `fetch` has no default timeout and a `before()` hook has no runner
  timeout, so a server that accepts the socket and never answers would hang the
  whole suite rather than fail the poll. Bound the two readiness polls, the three
  ops-API seed calls, `readDoc()`, and the conditions anchor's GET/POST helpers.
- `search_Doc` accepts either "filtered the denied row" or "rejected the call",
  which meant a schema or parameter-validation failure would satisfy `!leaked`
  without the search ever running. A rejection must now look like an
  authorization rejection; a success must return the lowuser-owned row, proving
  the filter was actually exercised. Measured: the tool returns
  `Unauthorized access to resource`, so the guard passes on its intended branch.
- Size the incremental log read off a 1 MiB cap rather than the log file, so a
  suddenly-verbose log cannot force one huge synchronous allocation.

39/39 green on the five anchors.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 4 of the pre-push review (Gemini), both minor:

- `longtxn-index-orphan`'s `dumpA`/`dumpB` and the CrossBaseline control asserted
  a 200 and then walked away from the body. An unread body holds its socket in
  undici's keep-alive pool, and this suite goes on to wait on the over-time
  monitor — so a leak here surfaces as a teardown hang rather than as the
  assertion that caused it. Route them through an `assertOK` helper and bound
  their fetches like the rest.
- The conditions anchor's non-vacuity controls matched Node's
  `deepStrictEqual` failure text (`/Expected values to be strictly deep-equal/`),
  which is not API and moves between Node majors. Match `AssertionError`.

39/39 green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp kriszyp changed the title test: promote six regression anchors, each measured green on main and red without its fix test: promote five regression anchors, each measured green on main and red without its fix Aug 25, 2026
@kriszyp
kriszyp merged commit 9104b6b into main Aug 29, 2026
104 of 106 checks passed
@kriszyp
kriszyp deleted the kris/qa-promote-measured-anchors branch August 29, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant