v4.0: refactor error contract, decompose god class, prep for extensions#103
Merged
v4.0: refactor error contract, decompose god class, prep for extensions#103
Conversation
- scripts/smoke-examples.sh: boot each example on a free port, assert
/health and a known S3 key return 200, then kill. Exposed via
`npm run test:smoke`. Catches example regressions like the
Express-5 splat-syntax incompatibility fixed below.
- biome.json: fix `includes` patterns (`!**/dir/` → `!**/dir/**`) so
shared-testing/ and other excludes actually exclude their contents.
- package.json: lint/format scripts now run `biome check` with no path
arg so the config controls scope (modern Biome 2.x convention,
one source of truth).
- examples/express-basic.ts: `/*` → `/*splat` for Express-5
path-to-regexp compatibility (server crashed at route registration).
- vitest.{,integration.}config.ts, tsconfig.examples.json: biome
auto-formatting under the now-active config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the (command as any).middlewareStack mutation, three `any` casts, two `biome-ignore` directives, and the "Not sure why" comment. Status comes from the typed output's `$metadata.httpStatusCode`. Headers are reconstructed from the typed output fields via a small src→dst table. The previous middleware approach mutated each command per-request to read pre-serialized wire headers; the new approach uses public, typed SDK output with zero mutation. Internal cleanup that fell out: - private `client` getter centralizes the init-or-throw guard, so `getObject`/`headObject`/`headBucket` no longer need explicit `isInitialized()` calls + non-null assertions. - `handleNonFatal(e)` factors out the empty-stream substitution used by all three private send paths. (B1 will replace this with typed throws, but for A1 behavior is preserved.) Tests: - All 52 existing tests pass unchanged. - New test/streaming-memory.test.ts pipes a 50MB synthetic Readable through proxy.get() into a sink and asserts peak RSS delta < 80MB, proving the new path doesn't buffer. - vitest.config.ts: pool=forks + execArgv=['--expose-gc'] so the streaming test's optional global.gc() actually fires (otherwise the test could give silent false negatives). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) - tsconfig.base.json: shared compiler options. tsconfig.json (build) and tsconfig.examples.json (type-check) now extend it and only declare what differs (outDir/declaration vs noEmit/rootDir/include). - vitest.shared.ts: shared `globals/environment/timeouts` and `esbuild.target`. Both vitest configs spread it. - src/version.ts: replaces the build-time shell `echo > version.ts` hack with `createRequire(import.meta.url)('s3proxy/package.json')`. Self-reference is enabled by the existing `./package.json` exports entry, so this works in dev (vitest/tsx) and in the published package without copying package.json into dist or flattening the output structure. - package.json: `build` is now just `tsc`. `clean` no longer references the unused `version.txt` artifact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes seven root-level markdown files that had outlived their use (content remains in git history if anyone needs them): - MIGRATION_SUMMARY.md v3.0 migration notes; superseded by README - RELEASE.md manual release ritual; superseded by CI - RELEASE_INSTRUCTIONS.md ditto - RELEASE_QUICK_REFERENCE.md ditto - TODO.md stale task list - AI_ASSISTED_DEVELOPMENT_RULES.md superseded by CLAUDE.md - S3PROXY_DOCKER_ADAPTATION_GUIDE.md fork-specific guide Moves the two docs that *do* still apply: - MAINTENANCE.md → docs/maintenance.md - PERFORMANCE.md → docs/performance.md README links updated. Length (192 + 415 lines) ruled out inlining. README credentials section now points to ~/.s3proxy/credentials.json with a bind-mount, instead of dropping creds in the repo root — keeps short-lived STS tokens out of the working copy entirely. credentials.json itself remains gitignored and intentionally usable as dev-only Docker tooling (see auto-memory note). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BREAKING. v3.x silently substituted an empty 200/206 stream for any
"non-fatal" S3 error (NoSuchKey, NoSuchBucket, AccessDenied,
InvalidRange). Consumers had no way to distinguish a successful
zero-byte file from a 404 they were swallowing. v4 throws typed
errors instead; consumers decide how to render each one.
src/errors.ts (new):
- S3ProxyError (base; carries `statusCode` and `code`)
- S3NotFound — 404
- S3Forbidden — 403
- S3InvalidRange — 416
The original SDK error is attached as `cause` for diagnostics.
(InvalidRequest is reserved for B3b's parser hardening; not yet
exported until something throws it.)
src/index.ts:
- getObject/headObject/headBucket map SDK errors via mapError(),
which throws the typed error (or rethrows unknowns unchanged).
- isNonFatalError() and the empty-stream substitution path are
deleted entirely (4.0 — no deprecation shim).
Tests rewritten:
- All `(proxy as any).getObject/headObject/headBucket` patterns
converted to public proxy.get/head/healthCheckStream calls
against a fake HttpResponse.
- "graceful empty stream" assertions → "rejects with typed error".
- Behavioral coverage for S3NotFound, S3Forbidden (with cause),
and S3InvalidRange (with cause).
- test/helpers/http-mocks.ts: shared makeReq/makeRes/readAll/
catchError helpers for upcoming B-phase tests.
Smoke gate: now also asserts a known-missing key returns 404
(previously the empty-200 path could regress silently because the
gate only hit /health and /index.html).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add S3ProxyConfig.verifyOnInit (default true). When true, init() calls healthCheck() against the bucket and rejects on failure (unchanged behavior, just routed through healthCheck so the probe logic has a single source of truth). When false, init() resolves without sending a HeadBucket — for orchestrator deployments (Kubernetes, ECS) where the platform's readiness probe should determine health and a transient S3 hiccup shouldn't crashloop the pod. Tests cover all three cases: default-true rejects on unreachable bucket, false skips the probe entirely, and healthCheck() remains callable independently after init(verifyOnInit=false). README documents the option and recommends pairing verifyOnInit=false with a readiness handler that calls proxy.healthCheck(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a pure response-free API:
proxy.fetch(req): Promise<{ stream, status, headers }>
Pure means it never touches a `res`. Callers wire it up however
they want: pipe to express, return as a Web Response, persist to
disk, hash the bytes for caching, etc. This unblocks framework
adapters and extension packages.
proxy.get(req, res) and proxy.head(req, res) are now thin wrappers
over fetch() — three lines each (await fetch, writeHead, return
stream). Behavior is preserved: existing callers continue to work
without code changes. (B3c retires those wrappers entirely.)
fetch dispatches GET vs HEAD by req.method (defaults to GET).
proxy.head() force-sets method='HEAD' on a shallow copy of the
request so the framework's req.method (often a GET handler running
HEAD logic) doesn't matter.
S3FetchResponse exported from src/types.ts. Behavioral coverage:
- 200 + headers + body for an existing key
- 206 + content-range for a range request
- throws S3NotFound for a missing key
- throws S3Forbidden when AWS returns AccessDenied (with cause)
- HEAD parity: same status + headers, empty body
- throws S3NotFound for HEAD on missing key
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) S3Proxy was a 288-line god class. Split into three modules with single responsibilities: src/request-parser.ts (new, ~70 LOC) Pure functions: parseRequest, mapHeaderToParam, stripLeadingSlash. Boundary hardening: malformed percent-encoding (`/foo%ZZ`) and null-byte keys (`/foo%00bar`) now throw InvalidRequest at the parser instead of bubbling URIError up the stack or letting poisoned keys reach S3. src/s3-gateway.ts (new, ~115 LOC) Owns the S3Client lifecycle and the AWS-SDK boundary. Single `send(command, target)` accepts the closed union of supported commands and returns the public S3FetchResponse, mapping AWS errors to typed S3ProxyErrors. The `dispatch` helper is the only place we have to dance around S3Client.send's per-command overloads — callers see one clean method. src/index.ts (slimmed, ~125 LOC down from ~288) S3Proxy is now an orchestrator: parses the request, builds params, calls gateway. Public API surface (init, healthCheck, healthCheckStream, fetch, get, head, isInitialized, version) is unchanged. InvalidRequest is now exported (B1 deferred this until a thrower existed). Tests: - test/parse-request.test.ts now imports parseRequest from src/request-parser directly. Added cases for malformed percent-encoding and null-byte keys. - The S3Proxy.version stowaway test moved out of parse-request into the existing test/version.test.ts coverage. - All other tests pass unchanged — behavior is preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) BREAKING. The response-mutating wrappers are deleted. Callers use the pure proxy.fetch(req) introduced in B3a and write the response themselves: // v3 / v3.x: const stream = await proxy.get(req, res); // writes headers stream.pipe(res); // v4: const { stream, status, headers } = await proxy.fetch(req); res.writeHead(status, headers); stream.pipe(res); Same swap for HEAD (set req.method = 'HEAD'). For health endpoints, call proxy.healthCheck() — it throws on failure, so frameworks can use their normal error pipeline. Examples updated to the new pattern: examples/express-basic.ts examples/fastify-basic.ts examples/fastify-docker.ts (Docker integration test entrypoint) examples/http.ts (raw node:http) Tests: - test/s3proxy.test.ts: dropped describe('get'/'head'/ 'healthCheckStream'/'Express integration methods'); the equivalent behavioral coverage already exists in test/fetch.test.ts (now augmented with the empty-key, S3InvalidRange, network-error, and unrecognized-body cases that previously lived only in s3proxy). - test/streaming-memory.test.ts: rewritten to fetch(). - test/mock-express.test.ts and test/mock-fastify.test.ts: now verify framework-shaped request objects flow through fetch(), rather than asserting writeHead was called on a mock res. - test/helpers/http-mocks.ts: dropped the unused makeRes/FakeRes helpers. Type surface trimmed: HttpRequest no longer extends node:http IncomingMessage. It is a structural subset of what proxy.fetch() actually reads: { url, method?, headers, path?, query? }. Express Request, Fastify request.raw, and node IncomingMessage all satisfy it. HttpResponse is no longer in the public type surface — fetch() doesn't touch a response, so consumers use Node's ServerResponse type directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final test sweep — locks in the discipline established through B1-B3.
grep -rn 'as any' test/ → 0 results
grep -rn '(proxy as any)' test/ → 0 results
npx biome check → 0 warnings, 0 errors
Removed:
- test/types.test.ts: tested TypeScript interfaces (S3ProxyConfig
has a `bucket` field, etc.) rather than runtime behavior. The
type system already enforces these.
- test/integration-tests.js + test/mocha.opts: pre-vitest mocha
artifacts that hadn't run since the v3 migration; integration
coverage now lives in test/integration/validation.test.js (run
via `make test-validation-docker`).
- test/helpers/aws-mock.ts: the unused `mockS3Error` and
`getS3MockCalls` helpers — the only `as any` survivors in the
test tree.
Added:
- test/concurrent.test.ts: 10 parallel proxy.fetch() calls with
distinct mocked payloads. Asserts both that the streams are
distinct instances (no reuse / shared buffer) and that each
stream produces its own bytes (no crossed wires under
concurrency).
- test/streaming-memory.test.ts: tightened to 100MB body / peak
RSS delta < 50MB (was 50MB / 80MB) — exercises the streaming
invariant harder.
README's project-structure section updated to reflect the v4 layout
(parser + gateway split, error module, etc.).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-area before/after for the breaking changes in #95-#101: - typed errors instead of silent empty-stream substitution - proxy.fetch() in place of proxy.get/head/healthCheckStream - parseRequest etc. as free functions, not S3Proxy statics - HttpRequest as a structural type; HttpResponse no longer exported - new verifyOnInit flag for orchestrator deployments Includes a sed-able cheat sheet, the recommended Express handler, and "pin v3.x" guidance for users not ready to migrate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
README:
- v3.0.0 ESM-only callout replaced with a v4.0 breaking-changes
section pointing to MIGRATION.md.
- All Quick Start / Express / Fastify / large-file / health-check
examples rewritten from `proxy.get(req,res)` to
`proxy.fetch(req)` + manual `writeHead` + `pipe`.
- API Reference: drops `proxy.get/head/healthCheckStream` /
`S3Proxy.parseRequest`; adds `proxy.fetch`, `proxy.healthCheck`,
`proxy.isInitialized`, plus the free-function exports of
parseRequest/mapHeaderToParam/stripLeadingSlash.
- Types: `HttpResponse` removed, `HttpRequest` shown as the
structural subset (no IncomingMessage extends), `S3FetchResponse`
documented.
- New "Error Handling" section catalogues the typed error
hierarchy (S3ProxyError / S3NotFound / S3Forbidden /
S3InvalidRange / InvalidRequest) with a recommended catch
pattern.
- New "verifyOnInit for orchestrators" section.
- Docker image tag bumped from 3.0.0 → 4.0.0.
.github/workflows/nodejs.yml:
- Stale `typescript-migration` push branch dropped.
- `npm run lint` now fails CI on errors (was masked by
`|| echo "warnings are acceptable"` — v4 has zero warnings, so
silencing is no longer right).
- New `smoke-tests` job runs `npm run test:smoke` against the
s3proxy-public bucket (uses the same AWS creds the validation /
performance jobs already consume). Gated as a dependency of the
create-tag-and-draft-release job so a smoke regression blocks
release.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cumulative cross-module review surfaced six small wins:
src/index.ts
- constructor: replace `Object.fromEntries(filter)` cast with a
rest-spread destructure. Reads cleaner and drops the
`S3ProxyOptions` import (now derived inline by TypeScript).
src/s3-gateway.ts
- dispatch(): the three identical `instanceof` branches were a
type-system artifact, not behavior. Collapse to a single
monomorphic call with a one-line cast comment.
src/errors.ts
- drop the redundant `code` field. It always equaled `name`, and
`name` is the standard Error discriminator. Simplifies the API
surface (one stringly-typed field instead of two) and the
constructor signature drops one parameter. Examples already
fall through `err.code || err.name`, so no consumer regression.
src/types.ts
- delete the unexported `S3ProxyResponse` interface (the v3
internal shape with `s3stream`/`statusCode`). The public
`S3FetchResponse` (`stream`/`status`) replaces it entirely.
README.md
- explicit note on Express 5's `/*splat` syntax — exactly the
crash the smoke gate caught when porting v3 examples.
MIGRATION.md
- document the field rename (`s3stream`→`stream`,
`statusCode`→`status`) in the response shape.
- call out the *where* of error firing: classified errors now
throw from `proxy.fetch()` itself, so v3 stream-error handlers
won't see them.
Skipped (out of scope or low payoff):
- removing public `isInitialized()` — nice-to-have but not in the
v4 prompt; current behavior is preserved.
- scoping `pool: 'forks'` to streaming-memory only — vitest 3.x
workspace setup adds more config than the ~0.5s saving justifies.
- extracting shared example error helpers — copy-paste is
intentional for "drop into your app" examples.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…astify 5.8 \`npm update\` brought every dev dep to its latest semver-compatible release. Most notable: @aws-sdk/client-s3 3.832 → 3.1037 @biomejs/biome 2.0.4 → 2.4.13 fastify 5.4.0 → 5.8.5 @types/express 5.0.3 → 5.0.6 artillery 2.0.23 → 2.0.31 Major-version bumps deferred (typescript 5→6, vitest-coverage 3→4, wait-on 8→9, @types/node 24→25, npm-check-updates 18→20) — those should land in their own PR with their own validation. \`npm audit\` went from 102 → 4 vulnerabilities (the rest are transitive-via-artillery and need an artillery major bump that's a separate concern; none affect the published package, whose only runtime dep is @aws-sdk/client-s3). Lockfile shrank ~44% (12641 → 7087 lines) — newer npm flattens the tree better. Two small consequence-fixes the updates surfaced: - examples/fastify-basic.ts: fastify@5.8 typed setErrorHandler's \`error\` as \`unknown\` (was \`Error\`). Narrow with a single cast at the boundary. - biome.json: bump \`$schema\` URL to 2.4.13 so biome stops warning about a schema-version mismatch. - test/version.test.ts + test/integration/validation.test.js: biome 2.4 added \`useParseIntRadix\` (info level). Add the missing radix-10 argument; cleaner anyway. - src/index.ts: re-format from biome 2.4's stricter import-line collapsing. All gates green: 49/49 unit, 24/24 integration via Docker, smoke, lint (0/0), type-check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Security Audit job was failing PR #103 because the dev-tree has 20 vulns (11 critical) transitively through artillery — uuid, msal, falso. None of these reach consumers: the published package's \`dependencies\` is only \`@aws-sdk/client-s3\`, which has zero vulns. Add \`--omit=dev\` to \`npm audit\` so the gate reflects what users actually install. Dev-tree vulns continue to be visible (and auto-PR'd) via Dependabot — they just don't block PRs over an artillery transitive that we can't fix locally without \`npm audit fix --force\` downgrading artillery to 1.7.9. Local check: \`npm audit --audit-level critical --omit=dev\` → \`found 0 vulnerabilities\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The post-merge create-tag-and-draft-release job reads the version from package.json directly. Without this bump it would see 3.0.0, notice the v3.0.0 tag exists, and skip both the tag creation and the draft release on master — silently no-op'ing the v4 ship. Two test fixtures had hardcoded '3.0.0' assertions for the static \`S3Proxy.version()\` method; updated alongside. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Tracking epic: #102.
Closes #95
Closes #96
Closes #97
Closes #98
Closes #99
Closes #100
Closes #101
Summary
v4 fixes the v3 error contract, decomposes a 288-line god class into
parser + gateway + orchestrator, and replaces
proxy.get(req, res)/proxy.head(req, res)/proxy.healthCheckStream(res)with a singlepure entry point:
proxy.fetch(req)returning{ stream, status, headers }.See MIGRATION.md
for the full v3 → v4 cheat sheet.
Breaking changes
Missing keys, AccessDenied, InvalidRange, and NoSuchBucket now throw
typed
S3NotFound/S3Forbidden/S3InvalidRange/S3ProxyErrorwith the original SDK error attached as
cause. v3 swallowed theseas 200/empty.
proxy.get(req, res)/proxy.head(req, res)/proxy.healthCheckStream(res)are removed. Replaced by
proxy.fetch(req)which doesn't touch aresponse — callers do
res.writeHead(status, headers); stream.pipe(res).For health endpoints, call
proxy.healthCheck()(throws on failure).S3Proxy.parseRequest/S3Proxy.mapHeaderToParam/S3Proxy.stripLeadingSlashare now free functions exported from
's3proxy', not statics on the class.S3Proxy.isNonFatalErroris removed (vestigial after the throw migration).HttpRequestis now a structural type ({ url, method?, headers, path?, query? }),no longer extends
IncomingMessage.HttpResponseis no longerexported from the public surface —
proxy.fetch()doesn't take a response.InvalidRequestthrown byparseRequestfor malformedpercent-encoding (
/foo%ZZ) and null-byte keys.New
verifyOnInit?: boolean(default true) onS3ProxyConfig.Set to false in orchestrator deployments (Kubernetes, ECS) so a
transient S3 hiccup or misconfigured bucket name doesn't crashloop
the pod before logs/dashboards are wired up.
proxy.fetch(req)— pure: returns{ stream, status, headers },dispatches GET vs HEAD by
req.method. Throws typed errors onclassified failures.
scripts/smoke-examples.sh,npm run test:smoke).Boots each example on a free port, asserts /health and /index.html
return 200 and a missing key returns 404. Wired into CI.
Internals
src/request-parser.ts— pure functions:parseRequest,mapHeaderToParam,stripLeadingSlash+ URL-decoding hardening.src/s3-gateway.ts— owns theS3Clientlifecycle and AWS-SDKboundary; single
send(command, target)returnsS3FetchResponseorthrows a typed error.
src/index.ts— slimmed to ~120 LOC: orchestrator that composesparser + gateway. Drops the AWS SDK middleware-stack hack (Replace AWS SDK middleware-stack hack with public API #95) in
favor of typed
$metadata.httpStatusCode+ reconstructed headersfrom typed output fields. Zero
any, zerobiome-ignore.Repo hygiene
AI-rules, etc.) — content remains in git history.
MAINTENANCE.mdandPERFORMANCE.mdmoved todocs/.tsconfig.base.jsonandvitest.shared.tsextracted; both childconfigs extend / spread.
version.ts— usescreateRequire(import.meta.url)('s3proxy/package.json').~/.s3proxy/credentials.json.biome.json(modern Biome 2.x convention).Test plan
npm test— 49/49 unit tests pass (10 test files).npx biome check— 0 errors, 0 warnings (was 0 errors, 20 warnings on master).npx tsc --noEmit(bothtsconfig.jsonandtsconfig.examples.json) — clean.npm run test:smoke— express + fastify both green (health=200, /index.html=200, missing=404).make test-validation-docker— 24/24 integration tests pass against real S3 via Docker.make artillery-docker— 300/300 requests, 0 failures, ~990 MB streamed; p95 = 713ms confirms no streaming-throughput regression.smoke-testsjob runs on every PR, gates the release-tag job.What's not in this PR (deliberate follow-ups)
fetch()API is a perfect match, but aHono adapter exposes a small ergonomic gap (Web
HeadersvsRecord)that deserves its own focused PR.
wait-on 8→9, @types/node 24→25). Each carries breaking-change risk
worth its own validation pass; they shouldn't ride on a refactor PR.
proxy.isInitialized()from the public surface — vestigialbut works; cleanup beyond the v4 prompt.
🤖 Generated with Claude Code