test(server): promote 3 QA server-layer contract anchors - #1972
Merged
Conversation
kriszyp
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces three new integration test suites to verify built-in component backfilling on upgrades (QA-577), CLI exit codes on failure paths (QA-579), and SSE event-data payload handling and mid-stream error recovery (QA-702). The review feedback identifies several critical robustness improvements for these tests to prevent potential hangs and unhandled errors on CI. Specifically, it is recommended to wrap the wedged server test in a try-finally block to guarantee cleanup, guard the scratch directory cleanup in the teardown hook against uninitialized states, add error handlers to server listeners to prevent indefinite hangs, and introduce a timeout to the readiness polling request.
Contributor
|
Reviewed; no blockers found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
kriszyp
force-pushed
the
kris/qa-promote-server-contracts
branch
2 times, most recently
from
July 31, 2026 00:05
edd2502 to
ed29b46
Compare
Promotes gated qa-explorer probes into integrationTests/server/ as standalone (isolation-required) suites: - qa702-sse-event-data: Resource SSE contract — 8-case event-data payload matrix never crashes a worker, plus the F-133 mid-stream-throw regression leg and a liveness canary (anchors merged #1863 / F-133). - qa577-upgrade-builtins: in-place-upgrade built-in-component config backfill, idempotent across reboots, no-op on fresh installs and on OSS-core (regression anchor for merged PR #1814). - qa579-cli-exit-codes: 5-cell CLI exit-code matrix spawning the real dist/bin/harper.js, with a hard-kill safety net (regression anchor for merged PR #1801). Test-only additions; no runtime code changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t SSE uncaught-exception contract Pre-push review (codex, independent) on the previous commit found two gaps: - unitTests/resources/auditLog.test.js: the cherry-picked deflake capped the event-count poll at 200ms (20 x 10ms), which harper#1966 documents as already observed to still flake under a loaded runner. Switch to the shared waitFor() helper (used elsewhere in this same file/suite) with a 2000ms budget instead of a hand-rolled fixed-count loop. - integrationTests/server/qa702-sse-event-data.test.ts: case (b), the F-133 mid-stream-throw re-check, computed newUncaught but only logged it — the sibling case (a) a few lines above asserts the identical "no NEW uncaughtException" contract. Added the missing strictEqual so a regression here actually fails the test instead of only appearing in test output. Verified locally: 10/10 passing, newUncaught=0 in practice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…the timing
Format Check failed on the two files touched by the earlier commits (no local
pre-commit hook available in this environment to catch it) — ran `prettier --write`.
The audit-log deflake from the previous commit was still flaky on Node v22 even
at a 2000ms waitFor budget, which ruled out "just needs more time." Traced the
actual drop: Table.ts's subscription dispatcher intentionally discards an event
if the record's version has already moved past it ("out of order event, with
old update, don't send anything") so it always delivers the LATEST value per id
rather than every intermediate write. In this test, id 2 is written twice in a
row (put, then put-changed); under load the first write's notification can lose
the race and get coalesced away entirely — a real drop, not a delay, so no
amount of polling recovers it. The only invariant the dispatcher actually
guarantees is one event per distinct id touched (id 1's final state is the
delete, id 2's is the last put) — 2 events, not >2. Asserting `>= 2` instead of
`> 2` matches that real guarantee.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… count Independent review (codex) on the previous commit flagged that events.length >= 2 proves the right count but not the invariant the comment documents — it would also pass if, say, both delivered events happened to be for the same id. Assert the actual per-id contract: the last delivered event for id 1 must be its delete, and for id 2 must carry the latest put's value. Also confirmed the review's other finding (F-133 uncaught-exception assertion under the Bun CI matrix) empirically holds: the Bun shard that exercises this test already ran on the previous commit's SHA and passed (newUncaught=0), so no change needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… count Independent review (codex) on the previous commit caught a real race I introduced: waitFor(() => events.length >= 2) can resolve on two INTERMEDIATE events (e.g. id 1's put plus id 2's first put) that satisfy the count but aren't the final per-id state the assertions below check for — which would make those assertions race the delete/latest-put events that hadn't arrived yet. Wait for the actual condition (id 1's delete AND id 2's latest put both present) instead of a proxy count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…p/codex) qa579-cli-exit-codes.test.ts: - wrap Cell 3's blackhole-server test body in try/finally so an assertion failure can't leave it listening and hang the CI event loop (gemini, high) - guard the after() scratchHome rm() so a before() failure doesn't mask itself behind a TypeError from rm(undefined) (gemini + claude bot) - add an 'error' listener to probe.listen() so a bind failure rejects instead of hanging the promise (gemini + claude bot) - set USERPROFILE alongside HOME so the CLI child's credential resolution is actually isolated on Windows, where os.homedir() reads USERPROFILE (kriszyp/codex) qa702-sse-event-data.test.ts: - add AbortSignal.timeout(5000) to the readiness-poll request so a hung server can't block the before() poll loop indefinitely (gemini) - relabel the suite/header/comments: this suite cannot reach the actual #1863-guarded writeSSE() (confirmed sandboxing boundary), so it should not claim to be that fix's regression anchor. The real anchor already exists at the unit level (progressEmitter.test.js, which exercises writeSSE() directly with undefined/null). Reframed as its own contract test for the reachable sibling encoder (contentTypes.ts's text/event-stream serialize()) instead. Exercising the real Operations API progress stream, or unifying the two encoders, are both real production-code changes out of scope for a test-only PR — noted as an open question in the PR review rather than guessed at (kriszyp/codex) qa577-upgrade-builtins.test.ts: - readBootLog() now falls back to the real default hdb.log location (${dataRootDir}/log/hdb.log, per validation/configValidator.ts's DEFAULT_LOG_FOLDER) instead of startupOutput.stdout, which can never contain the activation log line since the harness always boots with --LOGGING_STDSTREAMS=false - the 2nd-boot idempotence test now diffs only the log bytes appended after restart, since that log file is append-only across the restart and the 1st boot's activation line would otherwise still be present and fail the "no activation log on 2nd boot" assertion even when the backfill correctly did not re-fire (kriszyp/codex) Build, full auditLog.test.js unit suite (22/22), and all 3 promoted integration suites (19/19) pass locally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- qa579-cli-exit-codes.test.ts: use killSignal SIGKILL on the spawnSync
safety net (SIGTERM alone isn't a hard ceiling against a hang regression
that installs/ignores a TERM handler); pin Cell 1's assertion to the
actual deterministic YAML-parse-error message instead of "any non-empty
output"
- qa702-sse-event-data.test.ts: raise case (a)'s outer timeout 20s -> 35s
(two sequential 15s consumeSse() calls plus a settle sleep exceeded the
old budget); assert the actual shipped shape of the F-133 mid-stream-
throw close (closed && !ended) instead of only logging a verdict; drop
a bogus `logging: { console: true }` config key (not a real schema key,
silently dropped by allowUnknown); reference harper#2026 in the pinned
falsy-data-omission assertion/comments so it reads as a known, tracked
defect rather than an intended contract
- qa577-upgrade-builtins.test.ts: pin HARPER_BUILTIN_COMPONENTS: '' in
both negative controls, since the harness spawns with
`{ ...process.env, ...env }` and an ambient value (e.g. this suite
running inside harper-pro's own CI) would otherwise flow through and
falsify what the control is asserting; fix stale "only backfilled key
today" comment (UPGRADE_BACKFILL_BUILTIN_KEYS is now ['secretCustody',
'waf'])
- auditLog.test.js: snapshot lastEventById() once after the wait resolves
instead of recomputing it in each assertion (TOCTOU-shaped, though not
currently triggerable)
Filed harper#2026 (SSE encoder drops legitimate falsy data / id:0 -- the
production defect the qa702 matrix pins) and harper#2027 (config parse-
errors silently discarded on non-upgrade boots -- out of scope for this
test-only PR, filed for follow-up) per the domain review's findings.
Build, unit (22/22), and integration (19/19) all still pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, cell 4)
Three items the domain review flagged as worth acting on before merge:
- qa702-sse-event-data.test.ts: the before() hook booted with
logging.level: 'error', which makes the uncaughtException-delta oracle
silently vacuous -- a healthy boot logs nothing at that level, hdb.log
may never even be created, and "file absent" is then indistinguishable
from "logPath drifted to the wrong place." Every uncaughtAfter/Before
assertion in the suite would still pass either way. Switched to 'info'
(guarantees real boot content) and added a positive-control assertion
in before() that hdb.log actually has content.
- qa702-sse-event-data.test.ts: the readiness poll's while loop had no
failure path -- it just fell out of the 30s deadline and proceeded, so
a component-load failure would surface as 9 confusing "expected a
payload event block" test failures instead of one clear "probe never
became ready." Added the same throw-on-timeout qa577 already had.
- harper#2026 ("SSE encoder drops falsy data (0/false/'') and id: 0")
only had its data-half pinned by this suite. Added IdZeroPayload
(resources.js) + a dedicated test case asserting id: 0 is also
silently omitted, so #2026's eventual fix updates both halves.
Plus the two comment-accuracy items both codex and grok independently
flagged: qa702-sse-event-data/{resources.js,schema.graphql} still called
this suite a "#1863 anchor" (the test file's own header already correctly
says otherwise) -- reworded for consistency; and the F-133 throw-path
assertion checked only a data-block count where its own comment claimed
exact contents -- now asserts the actual pre-throw values
(`{"n":0}`,`{"n":1}` as a prefix) via deepStrictEqual.
Also addressed the remaining nits: qa579 cell 4 now pins the rejected
table name and rules out an unhandled-rejection stack trace (previously
only checked the exit-code sign); qa579 cell 5's near-tautological
`.includes('data')` now matches the actual schema/table; auditLog.test.js
had a comment asserting something the code doesn't do (fixed the wording,
not the code); qa577's latent bare-packageIdentifier-crash comment named
one dereference site when there are two, and claimed "reported separately"
when nothing had actually been filed -- filed harper#2028 and corrected
the comment to name both sites and point at the actual invariant fix.
Left open (not fixed here, see PR Review section): qa577's inert
built-in stand-in doesn't prove the in-memory config-mirror activation
path was exercised on first boot -- the domain reviewer downgraded this
to minor (test-only PR, coverage gap not a production risk) and it needs
a stand-in with an observable load-time side effect, which is a fixture
redesign beyond a review-comment-sized change.
Build, unit (22/22), and integration (20/20, +1 for the new id:0 case)
all still pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/qa-promote-server-contracts
branch
from
July 31, 2026 14:16
ed29b46 to
5a207af
Compare
kriszyp
marked this pull request as ready for review
July 31, 2026 14:16
cb1kenobi
reviewed
Jul 31, 2026
Suite 2 and Suite 3 asserted hdb.log does NOT contain the activation-log snippet, but readBootLog() returns '' when hdb.log is missing, so the assertion would pass trivially if the log file was never created or the path drifted. Add a positive-control assertion (log has real 'info'-level boot content) before each negative check, mirroring the established pattern in qa702-sse-event-data.test.ts. Addresses cb1kenobi/Barber AI review feedback on PR #1972. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The integration harness already forces --LOGGING_LEVEL=debug (a superset of info), so the explicit logging.level: 'info' doesn't "enable" content that wasn't already there — it's a defensive pin against that harness default changing, not the reason the positive control passes. Caught by the independent pre-push review (round 2, delta). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… divergence)
`b: ThrowGen (throws after 2 of 5) over SSE` was the sole failure on
`Integration Tests 5/6 (uWS HTTP)`; every other shard was green. Rather
than assume the assertion over-reached, QA-886 characterized the two
servers byte-for-byte on the identical workload.
Both deliver exactly the two pre-throw events (`{"n":0}`, `{"n":1}`),
byte-identical modulo a uWS-only leading `:\n\n` header-flush comment.
They diverge only at termination:
- Node (`server/http.ts` pipeBodyToResponse, ~426-461) closes the socket
WITHOUT the terminal `0\r\n\r\n` chunk, deliberately -- its own comment
says this "correctly signals a failed/truncated transfer... instead of
implying it completed". The incomplete chunked framing is the only
signal a client gets, and it is the intended one.
- uWS (`server/serverHelpers/uwsServer.ts` streamResponse, ~340-356)
routes the source's 'error' and 'end' handlers through the SAME
`finish(true)` -> `res.end()` path, so it DOES write the terminal
chunk. The wire response becomes byte-indistinguishable from a
generator that legitimately finished: a mid-stream failure is silently
presented to the client as success.
So the spec was right and the uWS path is wrong. `HttpResponse.close()`
is available and is the correct primitive for the error branch; that is a
product fix, tracked separately as F-272 and not made here.
This change pins BOTH shapes explicitly rather than skipping under uWS,
so the divergence stays visible in the suite and cannot drift or be
"fixed" in the wrong direction unnoticed. Verified locally at
`c28e5f83f`: 11/11 green under Node and 11/11 under HARPER_UWS_HTTP=1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cb1kenobi
approved these changes
Aug 3, 2026
kriszyp
added a commit
that referenced
this pull request
Aug 4, 2026
Suite 2 and Suite 3 asserted hdb.log does NOT contain the activation-log snippet, but readBootLog() returns '' when hdb.log is missing, so the assertion would pass trivially if the log file was never created or the path drifted. Add a positive-control assertion (log has real 'info'-level boot content) before each negative check, mirroring the established pattern in qa702-sse-event-data.test.ts. Addresses cb1kenobi/Barber AI review feedback on PR #1972. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes gated qa-explorer probes into
integrationTests/server/as standalone (isolation-required) suites. Test-only additions — no runtime code changes.event.datapayload matrix (undefined/null/''/0/false/object/nested/~300KB string) never crashes a worker and the connection stays healthy, plus the F-133 mid-stream-throw regression leg (a generator that throws mid-stream delivers prior events then closes promptly, not hangs) and a liveness canary. Anchors merged fix: guard SSE writes against undefined event data (#1724) #1863 and F-133. Measured: 10/10 passing, ~6.3s.Activated built-in component(s)...log; a second boot over the same data dir is idempotent (no duplicate/re-log); a fresh install with the key already present is a no-op; OSS-core (no built-ins registered) never grows the Pro-only key. Regression anchor for merged fix(upgrade): activate new built-in components on in-place-upgraded configs #1814. Measured: 4/4 passing, ~12.3s.dist/bin/harper.js, with a hard-kill safety net so a regression-to-hang surfaces as an explicit kill, not a false exit 0. Regression anchor for merged fix: ensure CLI failure paths exit non-zero, including operation timeouts #1801. Measured: 5/5 passing, ~6.1s.Dropped from this batch: P-440 (qa665-ops-surface, gh#1893 ops-API failure-shape contract). Cold-rerun on current main showed 2 of 5 sub-tests failing: the two probes hardcode
get_deployment_payload/delete_deployment_payloadas "documented-but-absent" operations, but merged commit3d329a526("Implement get_deployment_payload and delete_deployment_payload operations", closing #1893) already registered both on main — they now return a different (still-correct) 4xx shape instead of the generic "Operation not found" the test asserts on. The test's core premise is stale rather than flaky; fixing it would mean picking new genuinely-absent operation names and rewriting those assertions, which is a content change, not a promotion-gate fix, so it was left out rather than fudged to pass.All three kept specs are
requires-isolation(SSE worker/crash assertions, an actual re-boot of the instance, and spawning the real CLI binary), so each stays a standalone file rather than being appended into an existing suite.Test plan
3e317c7f5) before promotingnpx prettier --checkandnpx oxlint --format stylish --deny-warningsclean on all touched files (one--writepass needed for a long line inqa702-sse-event-data/resources.js; re-ran the spec after to confirm behavior-neutral)integrationTests/server/location — all greenrestartHttpWorkers()against pre-installed fixtures (replaced 4 real calls in qa577 with a direct/Marker/readiness poll, matchingintegrationTests/database/eviction-secondary-index.test.ts's pattern)🤖 Promoted via qa-explorer, generated with Claude Opus 5.