docs(service-cluster-redis): record the measured ioredis 5-vs-6 gap in the contract test header, and drop a live-Redis path that never existed - #15985
Conversation
…n the contract test header, and drop a promised live-Redis path that never existed
The contract test runs `ioredis-mock@8.13.1` (peer `ioredis: ^5`) against a
package depending on `ioredis@^6`, with the untyped import suppressed by
`@ts-expect-error`. This commit changes no code and no dependency: it writes
down the measurement that was missing, so the next reader starts where this
one finished.
Measured against ioredis 5.11.1 (newest release satisfying the mock's `^5`
peer) and 6.0.0 (the version resolved in this workspace):
- all 14 Redis commands this package issues carry every v5 overload verbatim
into v6's RedisCommander.d.ts; `set` is a strict superset there (v6 adds
IFEQ/IFNE/IFDEQ/IFDNE), and nothing used here was removed or re-shaped
- v6's RESP3 reply mapping is opt-in: the class defaults its ReplyMapping
parameter to "legacy", ChainableCommander defaults to "resp2", and
duplicate() with no override inherits the caller's mapping. This package
never passes replyMapping, so every reply shape it sees is the v5 one
- the three RedisOptions keys client.ts sets (lazyConnect,
maxRetriesPerRequest, enableAutoPipelining) are declared identically
The peer mismatch is therefore real but inert on the surface these suites
drive. The header now says so, names the version pair the claim rests on, and
states that the claim expires if either range in package.json moves.
Also recorded: these suites reach the mock only through the injected client
and the pub/sub duplicate, never through createRedisClient(), so
`new Redis(url, options)` — this package's only contact with ioredis's
constructor and connection surface, and the area v6 changed most — is
exercised by nothing in this file. That is why the gap is inert here, and it
is not a reason to trust the double.
The header additionally promised `RUN_REAL_REDIS=1` + `REDIS_URL` and
"conditional describe.skipIf blocks at the bottom". No such blocks exist, and
both names occurred nowhere in the package outside that one sentence. The
false promise is removed rather than left standing as an escape hatch nobody
can take.
The `@ts-expect-error` stays. Removing it fails typecheck with TS7016 —
ioredis-mock ships no `types` field and no .d.ts of its own — so the
suppression has not rotted. Its comment now also records what it costs: with
the module untyped the client is `any` and satisfies ioredis's Redis type
without being checked against it, which is the second reason a v5-vs-v6
divergence could not surface here. `@types/ioredis-mock` would type the seam
but only asserts `new(): ioredis.Redis` rather than describing the mock, so
adopting it would trade an honest `any` for an unearned certainty; that trade
is left to a maintainer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
Clause-② contract review — PR #15985 (card #15467)Tier. Verdict: NOT PASS as written. Every measurement the PR rests on re-derives and holds; three sentences in the shipped header state a wider population than was measured (§7). With those three line edits this is a PASS.1. Intersection empty on the suite-driven surface — re-derived, holdsPopulation: ioredis 5.11.1 (
2. Census population — is this file the only mock driver? Yes
Census re-taken with my own recording Proxy (mutation B): injected client → 3.
|
…redis-mock-major-seam
…ces to the populations actually measured
Review found three sentences in the header whose stated population was wider
than the measurement behind them. All three are corrected here; the diff stays
comments-only (proved: every added and removed line is a comment line, with a
control showing the filter admits code).
1. The "no live-Redis path" parenthetical claimed both names appeared nowhere
in the package. Re-scanned the WHOLE package rather than `src/`:
`RUN_REAL_REDIS` and `skipIf` do occur nowhere outside that sentence, so the
escape hatch genuinely never existed — but `REDIS_URL` occurs at
`README.md:42`, as the env var a caller feeds to `createRedisClient()`. The
sentence now says exactly that instead of over-reaching.
2. `multi` was listed among the commands carrying their v5 overloads verbatim
into `RedisCommander.d.ts`. That is false for `multi` specifically: it is
declared there in NEITHER version (measured 0 and 0, against a control
member `exec` reading 1 and 1); it lives on `Transaction`. Measured on its
real home, it DID change — all four overloads went from returning
`ChainableCommander` to returning it parameterised by a reply mapping. It is
removed from the verbatim list and given its own bullet stating why the
change is inert here: the parameter defaults to "resp2", the class defaults
to "legacy" and extends `Transaction` at "resp2", so a client built without
`replyMapping` resolves `multi()` to the non-RESP3 instantiation.
3. "v6's one substantive change reachable from this package" was wrong in the
other direction: v6 also changes connection defaults that are NOT opt-in —
`protocol: 3` (no such option in v5) and `keepAlive` 0 to 30000. The
sentence is scoped to "the surface these suites drive", which is what was
measured, and the connection-default changes are named as
changed-but-unexercised, since no suite here calls `createRedisClient()`.
Two further wording fixes, each closing a way the note could read as true when
it is not:
- the expiry clause now also fires on a RESOLVED-version move under an
unchanged caret range, which a lockfile bump alone will do; as written it
expired only when a range moved
- the unmet-peer warning is scoped to a resolving install, since a frozen
re-link prints nothing
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
PM verification — undrafted and armedVerified at head
The three fixes were re-measured, not reworded⭐ (a) went further than I asked. I said "either scope the sentence to ⭐ (b) is the one I flagged as not-a-word-move, and it was treated that way. Measured on ⭐ And it declined to overstate where the conclusion holds: v6's context gains a (c) confirmed before narrowing: v6's Both advisories taken: the expiry clause now also fires on a resolved-version move under an unchanged caret range, and the unmet-peer warning is scoped to a resolving install. ⭐ A self-report worth more than the fix it concerns
That is the eighth instance this session of a claim outrunning its evidence, and the first caught by its own author before it reached a reviewer. ⇒ The rule it names — measure first, write after — is the one that would have prevented most of the other seven, and it costs nothing. Gate family re-derived at this head: 45 families, no STALE TREE, list identical to round 1; 43 green directly, with ⛔ #15986 (the pin that would defend this declaration mechanically) is deliberately not folded in, and the lockfile advisory has been recorded there as widening what such a pin must cover. Undrafted and auto-merge armed. Generated by Claude Code |
CI red — measured NOT this PR's
The failing step, named: Merge-driver wiring gate → The control that settles ownership. The identical step fails on Where it is owned. #15992 (p0, Measured here. Why no fix is ported into this PR. Why no re-run. An unpinned launcher resolving to a version that rejects a flag is deterministic, not a flake. The one re-run this PR is entitled to is left unspent. State of this PR: the review's three population narrowings are in ( Generated by Claude Code |
Fixes #15467
Comment-only change to one test file. No code, no dependency, no behaviour touched. Head
a2bbec441.⭐ Round 2 — the clause-② review returned NOT PASS on three header sentences whose stated population was wider than what was measured. All three are corrected, plus the two advisories. The three corrections and their new measurements are in the last section; the body below is corrected in place so nothing here still states the over-broad version.
What was measured, before anything was proposed
The card asked whether the contract test still tests the shipped behaviour. On the surface these suites drive, the intersection with "surface where ioredis 5 and 6 differ" is empty — that is the finding, and the scoping words are load-bearing.
Population 1 — what this package issues that lives in
RedisCommander.d.ts. 14 commands: get, set, del, incr, incrby, pttl, watch, unwatch, exec, publish, subscribe, unsubscribe, quit, eval.multi()andduplicate()are not in this population — they are declared elsewhere and are measured separately below.Population 2 — the version pair. ioredis 5.11.1 (newest release satisfying the mock's
^5peer, fetched vianpm pack) against ioredis 6.0.0 (the version resolved in this workspace). Compared declaration-by-declaration, multi-line aware, normalising only quote style,(T)[]versusT[], and whitespace.RedisCommandercommandssetPipeline.d.tsmulti()Transaction, inRedisCommander.d.tsin neither version (measured 0 and 0; control memberexecreads 1 and 1). All four overloads did change return type, toChainableCommanderparameterised by a reply mappingReplyMappingparameter defaulting tolegacy, and extendsTransactionat resp2 when it is legacy;duplicate()with no override inherits the caller's mappingRedisOptionskeysclient.tssets⇒
multi()'s change is inert here: the mapping parameter defaults to resp2, the class defaults to legacy and extendsTransactionat resp2, so a client built withoutreplyMappingresolvesmulti()to the non-RESP3 instantiation — andexec()'s own declaration is byte-identical across the pair.Two controls on the normaliser, both fired: a genuinely different pair (NX versus XX) stayed different after normalisation; a purely cosmetic pair collapsed. An earlier run reported
evalas changed — bracket-adjacent whitespace from my own multi-line join; the normaliser was fixed rather than the result eyeballed.Separately, the command-metadata tables (
@ioredis/commands1.11.0 for the mock, 2.0.0 for ioredis 6) differ on 3 of the 15 used entries —del,subscribe,eval— entirely in server-side and cluster-routing fields. None changes a single-node client's call or reply shape. Control: 12 commands exist only in 2.0.0, 0 only in 1.11.0, an absent name reads undefined in both.⇒ On the surface these suites drive, v6's one substantive change is RESP3 reply mapping, and it is opt-in. This package never passes⚠️ Scoped deliberately: v6 also changes connection defaults that are not opt-in —
replyMapping.protocol: 3(no such option in v5) andkeepAlive0 to 30000. Those live on the connection surface, reached in production viacreateRedisClient()and by no suite here, so they are changed-but-unexercised. The card's promotion trigger to p2 asks about a surface this package uses in these tests, and it does not fire.Where the suite stops standing in for shipped behaviour
Measured, not assumed: the shipped test's
makeClientwas temporarily wrapped in a recording Proxy and the suite run unmodified. All 28 tests passed under instrumentation; the file was then restored to a state proved byte-identical to HEAD.The suite reaches the mock only through the injected client and the pub/sub
duplicate(). It never callscreateRedisClient()— sonew Redis(url, options), this package's only contact with ioredis's constructor and connection surface, is exercised by nothing in this file. That is why the version gap is inert here, and it is not a reason to trust the double. It is also what makes the scoping above honest rather than convenient: the non-opt-in connection changes are real, and unexercised.The
@ts-expect-error: proven still needed, not assumedVacuity first: this package's
tsconfigusesinclude: ["src"]and the test lives insrc/. Proved withtsc --listFiles— the contract test is in the file set (present-control 1, absent-control 0).Then the removal experiment, under a trap with the anchor asserted unique, the mutation proved on disk by a blob-hash delta plus a marker count going 1 to 0, and the restore proved byte-identical:
⇒ The suppression has not rotted.
ioredis-mock@8.13.1ships notypesfield and no.d.ts. Its comment now also records the cost: with the module untyped the client isanyand satisfies ioredis'sRedistype without being checked against it.Routes, including the ones not taken
@types/ioredis-mock— not taken. It declaresnew(): ioredis.Redis: it asserts the mock is a real client rather than describing it, trading an honestanyfor an unearned certainty. ⭐ The review added a fact that strengthens this: the package is in the store but on no@typesdirectorytscwalks from here, so it is not the near-free change it looks like.ioredisback to^5/ real Redis in CI — both above the manual floor per the card's triage, and neither warranted by a measurement that found the gap inert.Bounded in-place fix
The header promised
RUN_REAL_REDIS=1plusREDIS_URLand "conditionaldescribe.skipIfblocks at the bottom". Scanning the whole package (not justsrc/):RUN_REAL_REDISandskipIfoccur nowhere outside that sentence, so the escape hatch never existed.REDIS_URLdoes occur, atREADME.md:42, as the env var a caller feeds tocreateRedisClient()— unrelated to any test path. The header now says exactly that. Same defect class as the card (the header misdescribing its own instrument), same file, same gate family.Verification at
a2bbec441Gate family derived with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackfrom the actual changed file, never hand-built: 45 families, no STALE TREE warning, list identical to round 1.origin/mainmerged (nomerge=os-regenpath in the merge).node scripts/check-adr-0087-registration.mjs --self-test→ exit 0 (325 assertions) — control.node scripts/check-adr-0087-registration.mjs --base origin/main --head a2bbec441→ exit 0, no declared-breaking changeset.check:dual-build-cjs-loadsandcheck:type-check-debt; their prerequisite was built and both re-run green. Neither 3 is reported as a pass.pnpm --filter @objectstack/service-cluster-redis test→ 28 passed, exit 0.typecheck→ exit 0 (which re-proves the directive is still "used").check:nul-bytesgreen plus a control-byte scan of the edited file, with a planted-byte control that fired.Round-2 corrections
src/, andREDIS_URLoccurs atREADME.md:42. Re-scanned package-wide and the sentence now names each of the three strings by what it actually found.multiwas listed among the verbatimRedisCommandercommands. It is declared there in neither version (0 and 0, controlexec1 and 1) and its return type did change. Removed from the list and given its own measured bullet, including why it is inert here.Out of scope, filed separately
#15983 —
multi().delis never called, soRedisKV.delete's versioned branch is unexercised, and nothing produces theexec() === nullWATCH-abort retry. #15986 tracks the pin that would defend this declaration mechanically.🤖 Generated with Claude Code
https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y