follow-up: review-squad LOW/MED follow-ups (F4, F8, F11, F14)#401
Conversation
|
📄 lib/utils/lz4.ts
▎ Module-global lz4 memoization leaks into the first lz4.test.ts test, silently defeating its injected loader. resolvedModule (lz4.ts:44) is memoized on the first getResolvedModule() call and only recomputed when it is undefined (lz4.ts:46-48). Sibling suites ArrowResultHandler.test.ts and CloudFetchResultHandler.test.ts run before lz4.test.ts (mocha loads paths alphabetically: result/ < utils/) and construct handlers with lz4Compressed: true, which call LZ4() and memoize resolvedModule to the real module. Because lz4.test.ts calls resetLz4CacheForTest() in afterEach (test:12-16) rather than beforeEach, the FIRST test ('should successfully load and use lz4 module when available') runs against that stale memoized module: the setLz4LoaderForTest(fakeLz4) it installs is never consulted. The assertions still pass (the real module round-trips), so the test silently stops exercising the injected-loader seam it was written to cover. |
8d81b92 to
b2aa909
Compare
Rebuilt cleanly on top of merged main (post-#390). The original #401 branch had diverged badly (branched off 1.15.0, carried a stale duplicate of the entire #390 security-bump, missing ~20k lines of since-merged kernel/telemetry work). This is only the genuinely-new follow-up content. F2 (lz4 test ESM-safety) is intentionally DROPPED: #390 already made the lz4 tests ESM-safe a different way (golden-frame real-codec suite + an availability-gated before()), so #401's setLz4LoaderForTest seam would revert merged work. - F4: SECURITY-OVERRIDES.md documents each of the 7 package.json `overrides` entries (class, dependent chain, CVE ids, exit condition), matching the actual merged override set (not the stale 18-entry set the original branch documented). Referenced from CONTRIBUTING.md. - F8: FederationProvider gains a setFederationFetchForTest() seam so tests can stub node-fetch without patching the import system; adds 3 unit tests exercising the previously-uncovered HTTP exchange branch (exchange success + endpoint/method/AbortSignal assertions, signal contract, non-retryable-failure fallback to the original token). - F11: tests/e2e/README.md documents the parallelism contract (per-job E2E_TABLE_SUFFIX, uuid.v4() staging file names) and warehouse-capacity considerations for the Node 20/22/24/26 matrix. Drive-by: fix the lint script glob (`tests/e2e/**` -> `tests/e2e/**/*.{js,ts}`) so eslint no longer tries to parse the new README.md. - F14: OAuthCallbackServerStub drops the pile of no-op http.Server shim methods (setTimeout/closeAllConnections/ref/unref/Symbol.asyncDispose/ the maxHeadersCount... property pile). Production OAuth code only calls listen/close/address; the AuthorizationCode.test.ts call site now carries the structural-type assertion via an `as unknown as ...` cast, so @types/node widening no longer forces stub churn. Verified on Node 20: tsc --noEmit clean, eslint clean, prettier clean, 1253 unit tests passing. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
7bc63c9 to
d112b5e
Compare
gopalldb
left a comment
There was a problem hiding this comment.
Approved via Isaac Review: no blocking comments (0 critical / 0 major / 0 minor). All 5 raised comments were Info-level, and 3 were rejected as invalid during validation.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
f6d4e60 to
3293568
Compare
Summary
The LOW/MED review-squad follow-ups from the #390 self-review that were deferred to keep that PR tight. Rebuilt cleanly on top of merged
main(post-#390) as a single commit — the earlier version of this branch had diverged badly (branched off 1.15.0, carried a stale duplicate of the entire #390 security-bump, and was missing ~20k lines of since-merged kernel/telemetry work).F4 —
SECURITY-OVERRIDES.mdDocuments each of the 7
package.jsonoverridesentries (basic-ftp,@75lb/deep-merge,ws,ip-address,form-data,uuid,serialize-javascript) — class (runtime vs dev), dependent chain, CVE id(s) cleared, and exit condition. Matches the actual merged override set (the earlier draft documented a stale 18-entry set). Referenced fromCONTRIBUTING.md.F8 — FederationProvider HTTP exchange tests
Adds a
setFederationFetchForTest()seam so tests can stubnode-fetchwithout patching the import system, then 3 unit tests covering the previously-uncovered token-exchange branch:/oidc/v1/tokenon the Databricks host and thatinit.signalreaches fetch as anAbortSignal(the cast site TS 5 strictness caught in 2.0.0: breaking security bump — Node >=20, thrift 0.23, uuid 11, lz4→lz4-napi (0 OSV findings) #390).aborted: boolean+addEventListener.returnOriginalTokenOnFailure: true.F11 — e2e parallel-safety audit
Adds
tests/e2e/README.mddocumenting the parallelism contract (per-jobE2E_TABLE_SUFFIX,uuid.v4()staging file names) and warehouse-capacity considerations for the Node 20/22/24/26 matrix. Drive-by: the lint globtests/e2e/**expanded to include the new.md; changed totests/e2e/**/*.{js,ts}(same scope, no false positives).F14 —
OAuthCallbackServerStubinterface-drift refactorDrops the pile of no-op
http.Servershim methods (setTimeout,closeAllConnections,ref/unref,Symbol.asyncDispose, themaxHeadersCount/timeout/… property pile) that existed only to satisfy the structural type and grew with every@types/nodewidening. Production OAuth code calls onlylisten/close/address; theAuthorizationCode.test.tscall site now carries the assertion via anas unknown as ...cast.Test plan
tsc --noEmitclean (Node 20)eslintclean (glob fix verified — no README parse attempt)prettier --checkcleanThis pull request was AI-assisted by Isaac.