From a33b7bb02b64ecd582b7354818e8c61fd428c579 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:03:30 -0400 Subject: [PATCH 1/7] rest: composed flush-chain pin + delegate fake family + gzip error-path pins (#174) --- rest/cachecontrol_internal_test.go | 13 +-- rest/fakewriters_test.go | 108 ++++++++++++++++++++++ rest/flushchain_internal_test.go | 142 +++++++++++++++++++++++++++++ rest/gzip_internal_test.go | 132 +++++++++++++++++++-------- rest/sentry_test.go | 14 --- 5 files changed, 343 insertions(+), 66 deletions(-) create mode 100644 rest/fakewriters_test.go create mode 100644 rest/flushchain_internal_test.go diff --git a/rest/cachecontrol_internal_test.go b/rest/cachecontrol_internal_test.go index a9bf555..a35b4fb 100644 --- a/rest/cachecontrol_internal_test.go +++ b/rest/cachecontrol_internal_test.go @@ -131,7 +131,7 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { // happened, and both must KEEP. Broadening the discriminator to any // non-nil error would delete from the live map a stamp the wire already // carries and reopen with committed=false a decision the wire already - // took. The delegate (flushErrorWriter, sentry_test.go) fails the flush + // took. The delegate (flushErrorWriter, fakewriters_test.go) fails the flush // with the injected error, never the refusal sentinel; the trailing 404 // is the handler-reacts-to-the-failed-flush move from the rollback pin // above — here it must NOT reopen the commit (on a real server that 404 @@ -170,17 +170,6 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { } } -// noFlushWriter hides the recorder's Flusher — the shape gzipResponseWriter -// had before #167 (no FlushError, no Flusher, no Unwrap), where a delegated -// flush always fails with http.ErrNotSupported. -type noFlushWriter struct { - rr *httptest.ResponseRecorder -} - -func (w *noFlushWriter) Header() http.Header { return w.rr.Header() } -func (w *noFlushWriter) Write(b []byte) (int, error) { return w.rr.Write(b) } -func (w *noFlushWriter) WriteHeader(code int) { w.rr.WriteHeader(code) } - // cacheControl values are registration-time constants; a negative max-age is // always a programming error, so it fails at registration, not on the wire. func TestCacheControl_NegativeMaxAgePanics(t *testing.T) { diff --git a/rest/fakewriters_test.go b/rest/fakewriters_test.go new file mode 100644 index 0000000..aea9dcc --- /dev/null +++ b/rest/fakewriters_test.go @@ -0,0 +1,108 @@ +package rest + +// The delegate fake family (#174): every error-path pin on the chain's +// writer wrappers injects its downstream failure through one of these fakes, +// each modelling ONE base-writer shape the real wire can take. They live +// together so the next pin (e.g. #178's rollback witnesses) reuses a shape +// instead of minting a near-duplicate: +// +// - noFlushWriter: the controller's REFUSAL shape — no FlushError, no +// Flusher, no Unwrap, so a delegated flush returns http.ErrNotSupported +// and nothing reaches the wire (the rollback discriminator's "nothing +// sent" world, #164/#163 R1). +// - flushErrorWriter: the GENUINE-failure shape — FlushError returns the +// injected error (a conn write error's analogue), so the commit really +// happened before the error surfaced (the "keep the latch/stamp" world, +// #164/#172). +// - failingWriter: the broken-pipe shape — every body Write fails with the +// injected error, the failure that makes gzip's OWN writer (gz.Flush, +// gz.Close) fail; its FlushError succeeds and counts, so a pin can +// observe whether a wrapper's flush path stopped before delegating. +// - flushSnapshotWriter: the wire-faithful observer — records what the +// wrapper pushed down and snapshots the buffer the moment Flush arrives +// (the bytes that would be on the wire after the flush). +// - noFlushUnderlying: noFlushWriter with its own buffer instead of a +// recorder, for pins that must inspect the bytes downstream of a refusal. +// +// All package-internal: the wrappers under test are unexported. + +import ( + "bytes" + "net/http" + "net/http/httptest" +) + +// noFlushWriter hides the recorder's Flusher — the shape gzipResponseWriter +// had before #167 (no FlushError, no Flusher, no Unwrap), where a delegated +// flush always fails with http.ErrNotSupported. +type noFlushWriter struct { + rr *httptest.ResponseRecorder +} + +func (w *noFlushWriter) Header() http.Header { return w.rr.Header() } +func (w *noFlushWriter) Write(b []byte) (int, error) { return w.rr.Write(b) } +func (w *noFlushWriter) WriteHeader(code int) { w.rr.WriteHeader(code) } + +// flushErrorWriter is a base writer whose flush genuinely FAILS rather than +// being refused: FlushError returns the injected error, never +// http.ErrNotSupported. The real-server analogue is a conn write error — the +// implied 200 commits to the wire BEFORE the error returns to the handler. +type flushErrorWriter struct { + rr *httptest.ResponseRecorder + err error +} + +func (w *flushErrorWriter) Header() http.Header { return w.rr.Header() } +func (w *flushErrorWriter) Write(b []byte) (int, error) { return w.rr.Write(b) } +func (w *flushErrorWriter) WriteHeader(code int) { w.rr.WriteHeader(code) } +func (w *flushErrorWriter) FlushError() error { return w.err } + +// failingWriter rejects every body write with a fixed genuine error — the +// downstream-failure shape (connection torn down, write timeout) that makes +// the gzip layer's own writes fail: gz.Flush and gz.Close surface it, and it +// sticks in the gzip.Writer. Its FlushError SUCCEEDS and counts calls, so a +// pin can prove a wrapper's flush path returned early — when the failure +// came from the wrapper's own downstream write, a delegated flush must never +// run (flushes stays 0). +type failingWriter struct { + header http.Header + err error + flushes int // delegated flushes that reached this base +} + +func (w *failingWriter) Header() http.Header { return w.header } +func (w *failingWriter) Write([]byte) (int, error) { return 0, w.err } +func (w *failingWriter) WriteHeader(int) {} +func (w *failingWriter) FlushError() error { w.flushes++; return nil } + +// flushSnapshotWriter records what the wrapper under test pushed down to it, +// and snapshots that buffer the moment Flush is called — the bytes that would +// be on the wire after the flush. +type flushSnapshotWriter struct { + header http.Header + buf bytes.Buffer + flushed int + snapshot []byte // buf contents at the first Flush +} + +func (w *flushSnapshotWriter) Header() http.Header { return w.header } +func (w *flushSnapshotWriter) Write(b []byte) (int, error) { return w.buf.Write(b) } +func (w *flushSnapshotWriter) WriteHeader(int) {} +func (w *flushSnapshotWriter) Flush() { + if w.flushed == 0 { + w.snapshot = append([]byte(nil), w.buf.Bytes()...) + } + w.flushed++ +} + +// noFlushUnderlying is a writer the controller cannot flush — no FlushError, +// no Flusher, no Unwrap — with its own buffer, for pins that must inspect +// what landed downstream of the refusal. +type noFlushUnderlying struct { + header http.Header + buf bytes.Buffer +} + +func (w *noFlushUnderlying) Header() http.Header { return w.header } +func (w *noFlushUnderlying) Write(b []byte) (int, error) { return w.buf.Write(b) } +func (w *noFlushUnderlying) WriteHeader(int) {} diff --git a/rest/flushchain_internal_test.go b/rest/flushchain_internal_test.go new file mode 100644 index 0000000..a885057 --- /dev/null +++ b/rest/flushchain_internal_test.go @@ -0,0 +1,142 @@ +package rest + +// Composed flush-chain integrity (#174): #167's "flush succeeds from a +// handler" held only by composition of per-layer pins (sentry, metrics, +// cachecontrol, gzip each pinned individually) — nothing pinned the COMPOSED +// property, so a wrapper inserted into New tomorrow without +// FlushError/Flusher/Unwrap would break handler flushes in production with +// every existing test green. This test assembles the New-shaped stack — +// sentry → metrics → auth (→ sentryLabel) → gzip → clean-path 307 → mux, +// with the probe registered through handle() exactly like a production route +// (routeLabel + requireScope + cacheControl) — over a real server and pins +// the composed behavior: the mid-body flush succeeds, and the flushed prefix +// is decodable on the wire while the handler still holds the tail. Internal +// (package rest) because the chain pieces New composes are unexported. + +import ( + "compress/gzip" + "context" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFlushChain_NewShapedStackStreamsFlushedPrefix(t *testing.T) { + // Sentry ENABLED: without a client sentryMiddleware never mounts + // commitWriter, and the composed property would silently exclude the + // outermost wrapper. + enableSentry(t) + + const part1 = "first chunk, flushed mid-stream" + const part2 = "; tail held by the handler until the prefix decoded" + + flushErr := make(chan error, 1) // handler runs on the server goroutine + release := make(chan struct{}) // client → handler: prefix decoded, finish + releaseOnce := sync.OnceFunc(func() { close(release) }) + writeErr := make(chan error, 2) + probe := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, part1) + writeErr <- err + flushErr <- http.NewResponseController(w).Flush() + <-release + _, err = io.WriteString(w, part2) + writeErr <- err + }) + + // The New-shaped assembly, middleware nesting mirrored from New verbatim + // with routes() swapped for the probe mux. handle() gives the probe the + // full per-route wrap set (routeLabel — the metrics sweep's never-routed + // contract holds for this traffic too — plus requireScope and + // cacheControl), so the flush traverses every writer wrapper a production + // route's would: commitWriter → statusWriter → gzipResponseWriter → + // cacheControlWriter. + mux := http.NewServeMux() + handle(mux, "GET /", "read", maxAgeRosterFamily, probe) + h := sentryMiddleware( + metricsMiddleware( + AuthMiddleware(&sentryFakeDatastore{}, + sentryLabel( + GzipMiddleware( + cleanPathRedirect(mux)))))) + + srv := httptest.NewServer(h) + defer srv.Close() + // LIFO: unblock the handler before srv.Close waits on it, so a failure + // before the deliberate release fails the test instead of hanging it. + defer releaseOnce() + + // Watchdog: a chain that buffers the flush instead of streaming it would + // leave the client blocked forever; the deadline turns that into a crisp + // failure (same pattern as the per-layer streaming pin in gzip_test.go). + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer cav7_sentry_read") + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, "gzip", res.Header.Get("Content-Encoding"), + "the request negotiated gzip — the full wrapper chain must be in play") + assert.Equal(t, "max-age=600", res.Header.Get("Cache-Control"), + "the first write committed the implied 200 — cacheControlWriter stamped it, proof the innermost wrapper was in the chain") + + require.NoError(t, <-writeErr, "pre-flush write must succeed") + require.NoError(t, <-flushErr, + "ResponseController.Flush must succeed through the composed New-shaped chain") + + // Decode the flushed prefix while the handler still blocks on release — + // possible only if every layer pushed its part of the flush to the wire. + zr, prefix := decodeStackPrefixWithin(t, res.Body, len(part1), 5*time.Second) + assert.Equal(t, part1, prefix, + "flushed prefix must decompress to exactly the pre-flush writes") + + releaseOnce() + tail, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer + require.NoError(t, err, "tail must decode through an intact trailer") + require.NoError(t, <-writeErr, "post-flush write must succeed") + assert.Equal(t, part1+part2, prefix+string(tail), + "full body must decompress byte-identically to the handler output") +} + +// decodeStackPrefixWithin opens a gzip reader over body and decodes exactly n +// bytes, failing the test if that takes longer than timeout — the deadlock +// shape a buffered (non-streamed) flush produces. Returns the open reader so +// the caller can decode the rest of the stream. (The package-external twin +// lives in gzip_test.go; this internal test cannot reach it.) +func decodeStackPrefixWithin(t *testing.T, body io.Reader, n int, timeout time.Duration) (*gzip.Reader, string) { + t.Helper() + type result struct { + zr *gzip.Reader + buf []byte + err error + } + done := make(chan result, 1) + go func() { + zr, err := gzip.NewReader(body) + if err != nil { + done <- result{err: err} + return + } + buf := make([]byte, n) + _, err = io.ReadFull(zr, buf) + done <- result{zr: zr, buf: buf, err: err} + }() + select { + case res := <-done: + require.NoError(t, res.err, "flushed prefix must be decodable as a gzip stream") + return res.zr, string(res.buf) + case <-time.After(timeout): + t.Fatal("flushed prefix never became decodable — some layer buffered the flush instead of streaming it to the wire") + return nil, "" + } +} diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index 448f1dc..1a45d2d 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -11,6 +11,7 @@ import ( "bytes" "compress/gzip" "errors" + "fmt" "io" "log" "net/http" @@ -22,26 +23,6 @@ import ( "github.com/stretchr/testify/require" ) -// flushSnapshotWriter records what the gzip layer pushed down to it, and -// snapshots that buffer the moment Flush is called — the bytes that would be -// on the wire after the flush. -type flushSnapshotWriter struct { - header http.Header - buf bytes.Buffer - flushed int - snapshot []byte // buf contents at the first Flush -} - -func (w *flushSnapshotWriter) Header() http.Header { return w.header } -func (w *flushSnapshotWriter) Write(b []byte) (int, error) { return w.buf.Write(b) } -func (w *flushSnapshotWriter) WriteHeader(int) {} -func (w *flushSnapshotWriter) Flush() { - if w.flushed == 0 { - w.snapshot = append([]byte(nil), w.buf.Bytes()...) - } - w.flushed++ -} - // FlushError must flush the gzip.Writer's buffered output into the underlying // writer BEFORE flushing the underlying chain — in that order the bytes on // the wire at flush time are a valid gzip stream prefix decoding to exactly @@ -366,18 +347,6 @@ func TestGzipMiddleware_HeadBodylessMakesNoLengthClaim(t *testing.T) { "nothing went through the gzip layer — there is nothing to truncate") } -// failingWriter rejects every body write with a fixed genuine error — the -// downstream-failure shape (connection torn down, write timeout) that makes -// the deferred gz.Close fail for real. -type failingWriter struct { - header http.Header - err error -} - -func (w *failingWriter) Header() http.Header { return w.header } -func (w *failingWriter) Write([]byte) (int, error) { return 0, w.err } -func (w *failingWriter) WriteHeader(int) {} - // The loud side of the carve-out (#175): a GENUINE close failure — anything // but the hijack/bodyless shapes — must still log "gzip close failed". // Nothing else pins this: a carve-out widened to swallow every close error @@ -409,16 +378,99 @@ func TestGzipMiddleware_GenuineCloseFailureLogs(t *testing.T) { "the log must carry the underlying error for diagnosis") } -// noFlushUnderlying is a writer the controller cannot flush — no FlushError, -// no Flusher, no Unwrap. -type noFlushUnderlying struct { - header http.Header - buf bytes.Buffer +// A delegate whose flush genuinely FAILS (the conn-write-error shape, not +// the ErrNotSupported refusal below) must surface through gzip's FlushError +// VERBATIM (#174): the handler's errors.Is discriminators — and the rollback +// discriminators in the wrappers above gzip (commitWriter, cacheControlWriter) +// — match on the delegate's error chain, so a FlushError that wrapped, +// replaced, or swallowed it would break every one of them silently. By the +// time the delegated flush fails, gz.Flush already pushed the gzip header and +// sync block into the (working) delegate — the commit really happened, which +// is exactly why the genuine-error world must stay distinguishable from the +// refusal world. +func TestGzipResponseWriter_FlushPropagatesDelegateErrorVerbatim(t *testing.T) { + errConnReset := errors.New("conn reset") + for _, tc := range []struct { + name string + flushErr error // what the delegate's FlushError returns + want error // the sentinel that must surface through errors.Is + }{ + {name: "plain error", flushErr: errConnReset, want: errConnReset}, + {name: "wrapped sentinel", flushErr: fmt.Errorf("flush tcp conn: %w", io.ErrClosedPipe), want: io.ErrClosedPipe}, + } { + t.Run(tc.name, func(t *testing.T) { + out := &flushErrorWriter{rr: httptest.NewRecorder(), err: tc.flushErr} + flushErr := make(chan error, 1) + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, "written before the failing flush") + require.NoError(t, err) + flushErr <- http.NewResponseController(w).Flush() + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(out, req) + + err := <-flushErr + assert.Equal(t, tc.flushErr, err, + "the delegate's flush error must propagate verbatim — wrapping or replacing it breaks the chain's errors.Is discriminators") + require.ErrorIs(t, err, tc.want) + require.NotErrorIs(t, err, http.ErrNotSupported, + "a genuine failure must never read as the refusal sentinel — the rollback discriminators key on exactly that distinction") + }) + } } -func (w *noFlushUnderlying) Header() http.Header { return w.header } -func (w *noFlushUnderlying) Write(b []byte) (int, error) { return w.buf.Write(b) } -func (w *noFlushUnderlying) WriteHeader(int) {} +// The OTHER failure point in gzip's FlushError (#174): gz.Flush() itself +// fails because its downstream write — the gzip header + sync block landing +// on the base writer — fails. Three duties, all pinned here against the +// write-failing base fake: +// +// - early return: the delegated chain flush must NOT run (failingWriter's +// flushes counter stays 0) — there is nothing coherent to flush, and a +// mutant that delegates anyway would report the chain flush's nil and +// swallow the real failure; +// - sticky gzip.Writer error: every subsequent flush and write surfaces the +// same downstream error — the stream is dead, not retryable; +// - Close-path log backstop: the deferred gz.Close fails with the sticky +// error and the middleware logs "gzip close failed" — for the many +// handlers that ignore write/flush errors, that line is the ONLY +// server-side trace of the truncation. +func TestGzipResponseWriter_FlushWriteFailureReturnsEarlyAndSticks(t *testing.T) { + logged := captureErrorLog(t) + + errDownstream := errors.New("downstream write torn away") + out := &failingWriter{header: make(http.Header), err: errDownstream} + var firstFlush, secondFlush, writeErr error + var flushesAtReturn int + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rc := http.NewResponseController(w) + firstFlush = rc.Flush() // gz.Flush's header+sync write fails on the base + flushesAtReturn = out.flushes + secondFlush = rc.Flush() // the gzip.Writer error is sticky + _, writeErr = io.WriteString(w, "never leaves the dead stream") + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(out, req) + + require.ErrorIs(t, firstFlush, errDownstream, + "the base write failure under gz.Flush must surface to the flushing handler") + assert.Equal(t, 0, flushesAtReturn, + "a failed gz.Flush must return early — the delegated chain flush must never run") + require.ErrorIs(t, secondFlush, errDownstream, + "the gzip.Writer error is sticky — a retry flush surfaces the same failure") + require.ErrorIs(t, writeErr, errDownstream, + "the gzip.Writer error is sticky — a later write surfaces the same failure") + assert.Equal(t, 0, out.flushes, + "no delegated flush may reach the base at any point — the stream died at the first failure") + + assert.Contains(t, logged.String(), "gzip close failed", + "the deferred close is the log backstop — handlers that ignore flush errors leave it as the only server-side trace") + assert.Contains(t, logged.String(), errDownstream.Error(), + "the log must carry the underlying error for diagnosis") +} // When nothing below the gzip layer can flush, the handler must still hear // about it loudly: FlushError reports the chain's http.ErrNotSupported diff --git a/rest/sentry_test.go b/rest/sentry_test.go index 71441a7..8474141 100644 --- a/rest/sentry_test.go +++ b/rest/sentry_test.go @@ -741,20 +741,6 @@ func TestSentry_FailedFlushAfterCommitKeepsRepanicSemantics(t *testing.T) { } } -// flushErrorWriter is a base writer whose flush genuinely FAILS rather than -// being refused: FlushError returns the injected error, never -// http.ErrNotSupported. The real-server analogue is a conn write error — the -// implied 200 commits to the wire BEFORE the error returns to the handler. -type flushErrorWriter struct { - rr *httptest.ResponseRecorder - err error -} - -func (w *flushErrorWriter) Header() http.Header { return w.rr.Header() } -func (w *flushErrorWriter) Write(b []byte) (int, error) { return w.rr.Write(b) } -func (w *flushErrorWriter) WriteHeader(code int) { w.rr.WriteHeader(code) } -func (w *flushErrorWriter) FlushError() error { return w.err } - // The errors.Is discriminator on the #164 rollback: ONLY the delegate's // refusal (http.ErrNotSupported — nothing sent) may clear the latch. A first // flush failing with a genuine I/O error is the opposite world: the delegate From aeb14491ec538e59a55b1abb14063636d8525377 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:07:50 -0400 Subject: [PATCH 2/7] =?UTF-8?q?rest:=20mechanical=20wrapper-convention=20g?= =?UTF-8?q?uards=20=E2=80=94=20FlushError=20convention,=20informational-pr?= =?UTF-8?q?edicate=20lint,=20hijack=20tripwire=20(#174)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- rest/wrapperconventions_test.go | 425 ++++++++++++++++++++++++++++++++ 1 file changed, 425 insertions(+) create mode 100644 rest/wrapperconventions_test.go diff --git a/rest/wrapperconventions_test.go b/rest/wrapperconventions_test.go new file mode 100644 index 0000000..ab04bbd --- /dev/null +++ b/rest/wrapperconventions_test.go @@ -0,0 +1,425 @@ +package rest_test + +// Mechanical convention guards over the rest package's source (#174) — the +// same declaration-time-enforcement family as the types tag-lint (#148/#162) +// and the mux.Handle source scan (#173). The flush-chain composition test +// (flushchain_internal_test.go) proves today's chain flushes; these guards +// make the conventions that keep it flushing self-announcing for the NEXT +// wrapper, before it has any tests at all: +// +// - FlushError convention: every ResponseWriter wrapper implements +// `FlushError() error` or doesn't intercept flush at all — never bare +// `Flush()`, whose errors http.ResponseController's Flusher branch +// silently swallows (the wrapper reports success on a flush that died). +// - Informational predicate (#165 review): every WriteHeader method on a +// wrapper must consult informational() — a WriteHeader-stateful wrapper +// added without the non-latching-1xx guard (and without volunteering +// into the shared table in informational_internal_test.go) reintroduces +// the exact #165 bug class invisibly. +// - Hijack tripwire (triage ruling 2026-06-07, from #181's close-out): no +// production code in this package hijacks. The gzip close-log's coarse +// ErrHijacked carve-out (#175) is only safe while no handler hijacks +// through the chain — this makes that precondition self-announcing +// instead of remembered. + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hijackRemedy is the ruled failure message for the hijack tripwire: the two +// sanctioned ways forward when a handler legitimately needs the connection. +const hijackRemedy = "no production code in package rest may hijack — the gzip close-log's coarse ErrHijacked carve-out (#175) is only safe while nothing hijacks through the chain. Mount upgrade endpoints OUTSIDE GzipMiddleware, or implement the faithfulness spec preserved in .out-of-scope/gzip-hijack-faithfulness.md (PR #185, from #181)" + +// wrapperTypes returns the names of every struct type in files that embeds +// http.ResponseWriter — the package's writer-wrapper convention (all four +// production wrappers embed it; a wrapper holding the delegate in a named +// field would not satisfy http.ResponseWriter and could not enter the chain). +func wrapperTypes(files []*ast.File) map[string]bool { + wrappers := map[string]bool{} + for _, f := range files { + ast.Inspect(f, func(n ast.Node) bool { + ts, ok := n.(*ast.TypeSpec) + if !ok { + return true + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + return true + } + for _, fld := range st.Fields.List { + if len(fld.Names) != 0 { + continue // named field, not an embed + } + if sel, ok := fld.Type.(*ast.SelectorExpr); ok { + if x, ok := sel.X.(*ast.Ident); ok && x.Name == "http" && sel.Sel.Name == "ResponseWriter" { + wrappers[ts.Name.Name] = true + } + } + } + return true + }) + } + return wrappers +} + +// receiverTypeName resolves a method's receiver to its bare type name +// ("" when the declaration has no receiver or an unsupported shape). +func receiverTypeName(fd *ast.FuncDecl) string { + if fd.Recv == nil || len(fd.Recv.List) != 1 { + return "" + } + expr := fd.Recv.List[0].Type + if star, ok := expr.(*ast.StarExpr); ok { + expr = star.X + } + if id, ok := expr.(*ast.Ident); ok { + return id.Name + } + return "" +} + +// isFlushErrorSignature reports whether the method is exactly +// `FlushError() error` — no parameters, one unnamed error result. +func isFlushErrorSignature(fd *ast.FuncDecl) bool { + if fd.Type.Params != nil && len(fd.Type.Params.List) != 0 { + return false + } + if fd.Type.Results == nil || len(fd.Type.Results.List) != 1 { + return false + } + res := fd.Type.Results.List[0] + if len(res.Names) != 0 { + return false + } + id, ok := res.Type.(*ast.Ident) + return ok && id.Name == "error" +} + +// flushConventionViolations applies the FlushError convention to every +// ResponseWriter wrapper in files: never bare Flush() (the controller's +// Flusher branch swallows its errors), and a FlushError must carry the exact +// `FlushError() error` shape the controller's method search matches — any +// other signature is dead code the controller skips, falling through to the +// swallow-or-tunnel paths the author thought they had replaced. Returns the +// wrapper and FlushError-method counts for the caller's vacuous-pass floors. +func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violations []string, wrappers, flushErrors int) { + wrapperSet := wrapperTypes(files) + wrappers = len(wrapperSet) + for _, f := range files { + for _, decl := range f.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || !wrapperSet[receiverTypeName(fd)] { + continue + } + switch fd.Name.Name { + case "Flush": + violations = append(violations, fmt.Sprintf( + "%s: %s.Flush: bare Flush() on a ResponseWriter wrapper — http.ResponseController's Flusher branch silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error instead (see gzipResponseWriter/commitWriter/cacheControlWriter), or drop flush interception entirely and let Unwrap tunnel it", + fset.Position(fd.Pos()), receiverTypeName(fd))) + case "FlushError": + flushErrors++ + if !isFlushErrorSignature(fd) { + violations = append(violations, fmt.Sprintf( + "%s: %s.FlushError: signature must be exactly `FlushError() error` — anything else is invisible to http.ResponseController's method search and never runs", + fset.Position(fd.Pos()), receiverTypeName(fd))) + } + } + } + } + return violations, wrappers, flushErrors +} + +// informationalPredicateViolations enforces the #165 guard mechanically: +// every WriteHeader method on a ResponseWriter wrapper must reference the +// informational() predicate — the shared definition of "forwards without +// committing" (1xx minus 101). A WriteHeader-stateful wrapper that skips it +// latches on a forwarded 103 and diverges from the wire exactly as #165's +// wrappers did. Returns the WriteHeader-method count for the caller's +// vacuous-pass floor. +func informationalPredicateViolations(fset *token.FileSet, files []*ast.File) (violations []string, writeHeaders int) { + wrapperSet := wrapperTypes(files) + for _, f := range files { + for _, decl := range f.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Name.Name != "WriteHeader" || !wrapperSet[receiverTypeName(fd)] { + continue + } + writeHeaders++ + found := false + ast.Inspect(fd.Body, func(n ast.Node) bool { + if id, ok := n.(*ast.Ident); ok && id.Name == "informational" { + found = true + } + return !found + }) + if !found { + violations = append(violations, fmt.Sprintf( + "%s: %s.WriteHeader: must consult the informational() predicate (#165) — a wrapper that latches state on a forwarded non-latching 1xx diverges from net/http's commit semantics. Guard the latch with informational(code) and join the shared table in informational_internal_test.go", + fset.Position(fd.Pos()), receiverTypeName(fd))) + } + } + } + return violations, writeHeaders +} + +// hijackViolations is the tripwire: any identifier named Hijack or Hijacker +// anywhere in production source — a http.Hijacker assertion, a +// ResponseController.Hijack call, a local helper named after either — +// violates. Identifier-level deliberately: the precondition is "nothing +// hijacks", not "nothing hijacks in the shapes we thought of". Comments +// never trip it (they are not identifiers); http.ErrHijacked — the carve-out +// gzip.go legitimately matches on — is a different identifier and stays +// clean. +func hijackViolations(fset *token.FileSet, files []*ast.File) []string { + var violations []string + for _, f := range files { + ast.Inspect(f, func(n ast.Node) bool { + if id, ok := n.(*ast.Ident); ok && (id.Name == "Hijack" || id.Name == "Hijacker") { + violations = append(violations, fmt.Sprintf("%s: identifier %q: %s", + fset.Position(id.Pos()), id.Name, hijackRemedy)) + } + return true + }) + } + return violations +} + +// parseSyntheticRest parses synthetic package-rest declarations for the +// meta-tests — the harness proving each checker can actually see the break +// it exists for (same discipline as the types tag-lint's violationsOfSrc). +func parseSyntheticRest(t *testing.T, decls string) (*token.FileSet, []*ast.File) { + t.Helper() + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", + "package rest\n\nimport \"net/http\"\n\nvar _ = http.StatusOK\n\n"+decls, + parser.SkipObjectResolution) + require.NoError(t, err) + return fset, []*ast.File{f} +} + +// syntheticWrapper is a minimal ResponseWriter wrapper the meta-cases extend. +const syntheticWrapper = "type fakeWrap struct {\n\thttp.ResponseWriter\n}\n\n" + +// The flush-convention checker must see each break — and stay silent on the +// two conforming shapes (FlushError() error, or no flush interception). +func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { + cases := []struct { + name string + decls string + want string // substring of the single expected violation; "" = clean + }{ + { + name: "conforming FlushError", + decls: syntheticWrapper + "func (w *fakeWrap) FlushError() error { return nil }\n", + }, + { + name: "no flush interception at all", + decls: syntheticWrapper + "func (w *fakeWrap) Unwrap() http.ResponseWriter { return w.ResponseWriter }\n", + }, + { + name: "bare Flush", + decls: syntheticWrapper + "func (w *fakeWrap) Flush() {}\n", + want: "bare Flush()", + }, + { + name: "bare Flush on a value receiver", + decls: syntheticWrapper + "func (w fakeWrap) Flush() {}\n", + want: "bare Flush()", + }, + { + name: "FlushError without the error result", + decls: syntheticWrapper + "func (w *fakeWrap) FlushError() {}\n", + want: "exactly `FlushError() error`", + }, + { + name: "FlushError with a parameter", + decls: syntheticWrapper + "func (w *fakeWrap) FlushError(force bool) error { return nil }\n", + want: "exactly `FlushError() error`", + }, + { + name: "Flush on a non-wrapper stays out of scope", + decls: "type plainBuffer struct{ n int }\n\nfunc (b *plainBuffer) Flush() {}\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset, files := parseSyntheticRest(t, tc.decls) + got, _, _ := flushConventionViolations(fset, files) + if tc.want == "" { + assert.Empty(t, got) + return + } + require.Len(t, got, 1) + assert.Contains(t, got[0], tc.want) + assert.Contains(t, got[0], "fakeWrap", "violation must name the wrapper type") + }) + } +} + +// The informational-predicate checker must flag a WriteHeader-stateful +// wrapper that skips the predicate — and only on wrapper types. +func TestInformationalPredicateViolations_DetectsTheSkip(t *testing.T) { + cases := []struct { + name string + decls string + want string // substring of the single expected violation; "" = clean + }{ + { + name: "WriteHeader consulting the predicate", + decls: syntheticWrapper + "func (w *fakeWrap) WriteHeader(code int) {\n\tif informational(code) {\n\t\tw.ResponseWriter.WriteHeader(code)\n\t\treturn\n\t}\n\tw.ResponseWriter.WriteHeader(code)\n}\n", + }, + { + name: "WriteHeader without the predicate", + decls: syntheticWrapper + "func (w *fakeWrap) WriteHeader(code int) {\n\tw.ResponseWriter.WriteHeader(code)\n}\n", + want: "must consult the informational() predicate", + }, + { + name: "WriteHeader on a non-wrapper stays out of scope", + decls: "type plainSink struct{ n int }\n\nfunc (s *plainSink) WriteHeader(code int) { s.n = code }\n", + }, + { + name: "wrapper without WriteHeader is clean", + decls: syntheticWrapper, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset, files := parseSyntheticRest(t, tc.decls) + got, _ := informationalPredicateViolations(fset, files) + if tc.want == "" { + assert.Empty(t, got) + return + } + require.Len(t, got, 1) + assert.Contains(t, got[0], tc.want) + assert.Contains(t, got[0], "fakeWrap", "violation must name the wrapper type") + }) + } +} + +// The hijack tripwire must fire on both hijack shapes, carry the ruled +// remedy, and stay silent on the legitimate neighbours (comments, the +// ErrHijacked carve-out gzip.go matches on). +func TestHijackViolations_TripsOnEachShapeOnly(t *testing.T) { + cases := []struct { + name string + decls string + trips bool + }{ + { + name: "Hijacker assertion", + decls: "func grab(w http.ResponseWriter) {\n\t_, _ = w.(http.Hijacker)\n}\n", + trips: true, + }, + { + name: "ResponseController Hijack call", + decls: "func grab(w http.ResponseWriter) {\n\t_, _, _ = http.NewResponseController(w).Hijack()\n}\n", + trips: true, + }, + { + name: "comment-only mention stays clean", + decls: "// Hijack and Hijacker discussed here at length — comments are not code.\nfunc clean() {}\n", + trips: false, + }, + { + name: "ErrHijacked carve-out stays clean", + decls: "func carve(err error) bool {\n\treturn err == http.ErrHijacked\n}\n", + trips: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset, files := parseSyntheticRest(t, tc.decls) + got := hijackViolations(fset, files) + if !tc.trips { + assert.Empty(t, got) + return + } + require.NotEmpty(t, got) + assert.Contains(t, got[0], "OUTSIDE GzipMiddleware", + "the tripwire must name the sanctioned mounting remedy") + assert.Contains(t, got[0], ".out-of-scope/gzip-hijack-faithfulness.md", + "the tripwire must point at the preserved faithfulness spec") + }) + } +} + +// parseRestPackage parses every non-test source file of the rest package, +// anchored on this file's own location so the enforcement tests work +// regardless of how they are invoked (same anchor as the types tag-lint). +func parseRestPackage(t *testing.T) (*token.FileSet, []*ast.File) { + t.Helper() + _, thisFile, _, ok := runtime.Caller(0) + require.True(t, ok, "runtime.Caller failed — cannot locate the package directory") + dir := filepath.Dir(thisFile) + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + fset := token.NewFileSet() + var files []*ast.File + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.SkipObjectResolution) + require.NoError(t, err) + files = append(files, f) + } + require.GreaterOrEqual(t, len(files), 10, + "file filter regressed — the rest package has many more production sources than this") + return fset, files +} + +// The FlushError convention, enforced over the package: zero violations, and +// the floors prove the walker still sees the real wrapper population (four +// wrappers; gzipResponseWriter, commitWriter, and cacheControlWriter carry +// FlushError today — bump the floors when wrappers come or go). +func TestRestWrappers_FlushErrorConvention(t *testing.T) { + fset, files := parseRestPackage(t) + violations, wrappers, flushErrors := flushConventionViolations(fset, files) + + require.GreaterOrEqual(t, wrappers, 4, + "wrapper count regressed — the embed detection or file filter broke (the chain mounts four writer wrappers)") + require.GreaterOrEqual(t, flushErrors, 3, + "FlushError count regressed — the method scan broke") + for _, v := range violations { + t.Error(v) + } +} + +// The informational predicate, enforced over the package: every wrapper +// WriteHeader consults informational() (#165). The floor pins the four +// production WriteHeader methods the scan must see. +func TestRestWrappers_WriteHeaderConsultsInformationalPredicate(t *testing.T) { + fset, files := parseRestPackage(t) + violations, writeHeaders := informationalPredicateViolations(fset, files) + + require.GreaterOrEqual(t, writeHeaders, 4, + "WriteHeader count regressed — the method scan or file filter broke (all four wrappers intercept WriteHeader)") + for _, v := range violations { + t.Error(v) + } +} + +// The hijack tripwire, enforced over the package: today trivially true (only +// comments mention hijacking), and that is the point — the moment production +// code asserts Hijacker or calls Hijack, this fails with the ruled remedy +// instead of letting the #175 carve-out's precondition rot silently. +func TestRest_NoProductionCodeHijacks(t *testing.T) { + fset, files := parseRestPackage(t) + for _, v := range hijackViolations(fset, files) { + t.Error(v) + } +} From 1c8c5c132390125b80f2346da6aeedca7a547f26 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:11:17 -0400 Subject: [PATCH 3/7] rest: statusWriter FlushError closes the flush blind spot; metrics tunnel-test comment fix (#174) --- rest/gzip.go | 5 +- rest/metrics.go | 49 +++++++++-- rest/metrics_internal_test.go | 141 ++++++++++++++++++++++++++++++-- rest/wrapperconventions_test.go | 6 +- 4 files changed, 184 insertions(+), 17 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index f3533a3..c318de1 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -77,7 +77,10 @@ func GzipMiddleware(next http.Handler) http.Handler { // stream). The precise per-shape faithfulness — commit-then- // hijack smuggling, truncated-after-output, accurate // logging — is TRACKED IN #181 and is latent until a - // hijacking handler exists. No handler hijacks today. + // hijacking handler exists. No handler hijacks today — + // self-announcing, not remembered: the hijack tripwire + // (wrapperconventions_test.go, #174) fails the suite the + // moment production code in this package hijacks. if errors.Is(err, http.ErrHijacked) { return } diff --git a/rest/metrics.go b/rest/metrics.go index a42d99c..fff1963 100644 --- a/rest/metrics.go +++ b/rest/metrics.go @@ -2,6 +2,7 @@ package rest import ( "context" + "errors" "net/http" "strconv" "time" @@ -160,10 +161,10 @@ func metricsMiddleware(next http.Handler) http.Handler { // the stdlib committed on it, so that status, not the relabel, is the // honest one. A // handler that already committed a status before panicking keeps that - // status — it is on the wire. One gap: statusWriter has no FlushError, - // so a flush-committed implied 200 is invisible to this capture — a - // 103-then-flush-then-panic meters 500 against a 200 already committed - // on the wire. Pre-existing blind spot, tracked in #174. + // status — it is on the wire; a flush-committed implied 200 counts + // (statusWriter.FlushError captures it, #174 — before that it + // tunneled past via Unwrap and a flush-then-panic metered 500 against + // a wire-committed 200). // The panic is re-raised AFTER recording // (the inner defer fires as this deferred func returns) so the sentry // recovery layer outside this one (#132) — and net/http when sentry @@ -207,7 +208,9 @@ func methodLabel(m string) string { // statusWriter captures the response status for the counter's status label. // A handler that writes a body without an explicit WriteHeader gets the -// net/http implied 200. Non-latching informational WriteHeaders (1xx minus +// net/http implied 200 — committed by its first Write or by a +// ResponseController flush (FlushError below; #174 closed that blind spot). +// Non-latching informational WriteHeaders (1xx minus // 101 — rationale on the informational predicate) capture nothing (#165): // those are never the final status, so the label belongs to whatever final // write follows. A 101 IS captured, like a final status — the stdlib commits @@ -239,9 +242,41 @@ func (w *statusWriter) Write(b []byte) (int, error) { return w.ResponseWriter.Write(b) } +// FlushError captures the implied 200 a first flush commits before +// delegating it — the same treatment #164/#167 gave commitWriter and +// cacheControlWriter (#174 closed this last blind spot): without it the +// flush tunnels past via Unwrap, the capture never latches, and a +// flush-then-panic meters 500 against a 200 already committed on the wire. +// Delegating through a fresh ResponseController keeps the downstream search +// semantics identical. +// +// If the delegated flush reports http.ErrNotSupported, the capture this call +// made is rolled back (#164's discriminator, mirror of commitWriter and +// cacheControlWriter): nothing reached the wire, so the status label still +// belongs to whatever final write follows — left latched, a later explicit +// error status could not capture and the meter would claim a 200 the wire +// never carried. The rollback only fires when this call was the first to +// capture; after a prior Write or latching WriteHeader the commit already +// happened and the capture keeps. A genuine I/O error also keeps it: by then +// the delegate really flushed, so the implied 200 is on the wire. +func (w *statusWriter) FlushError() error { + captured := w.code == 0 + if captured { + w.code = http.StatusOK + } + err := http.NewResponseController(w.ResponseWriter).Flush() + if captured && errors.Is(err, http.ErrNotSupported) { + w.code = 0 + } + return err +} + // Unwrap exposes the underlying writer to http.ResponseController so inner -// layers keep Flusher/Hijacker/deadline access through this wrapper — without -// it those optional interfaces silently vanish for everything inside metrics. +// layers keep Hijacker/deadline access through this wrapper — without it +// those optional interfaces silently vanish for everything inside metrics. +// Flush never takes this route: the controller's method search prefers the +// explicit FlushError above, which is what keeps a flush-committed implied +// 200 visible to the status capture. func (w *statusWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/rest/metrics_internal_test.go b/rest/metrics_internal_test.go index d43011a..849e2d8 100644 --- a/rest/metrics_internal_test.go +++ b/rest/metrics_internal_test.go @@ -1,20 +1,28 @@ package rest import ( + "errors" "net/http" "net/http/httptest" "testing" + "time" "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// statusWriter wraps every routed ResponseWriter, so it must expose Unwrap -// for http.ResponseController to tunnel through it — otherwise Flusher, -// Hijacker, and deadline control silently vanish for every inner layer. -// Observed behaviorally: a handler flushing via ResponseController against a -// real server connection (the recorder implements Flusher directly and would -// mask the gap). +// ResponseController.Flush must keep working from inside the metrics layer. +// Since #174 statusWriter answers it directly (FlushError captures the +// flush-committed implied 200, then delegates); before that it tunneled +// through Unwrap. Observed against a real server connection for +// wire-faithfulness — NOT because a recorder would mask anything: the +// earlier rationale here ("the recorder implements Flusher directly and +// would mask the gap") was false. With the wrapper missing FlushError and +// Unwrap alike, the controller's method walk dead-ends AT statusWriter and +// fails loudly with ErrNotSupported — the recorder's Flusher below is +// unreachable either way, so it can mask nothing (#174 comment fix; same +// false-rationale family corrected in gzip_test.go on #167's branch). func TestStatusWriter_ResponseControllerTunnelsThroughMetrics(t *testing.T) { flushErr := make(chan error, 1) // handler runs on the server goroutine // The probe mounts route-labeled, like every registration in the @@ -37,6 +45,127 @@ func TestStatusWriter_ResponseControllerTunnelsThroughMetrics(t *testing.T) { "ResponseController.Flush must reach the underlying writer via statusWriter.Unwrap") } +// The verbs WITHOUT an explicit statusWriter method (deadline control here, +// as the witness; Hijacker rides the same route) reach the connection only +// through Unwrap — since FlushError landed (#174), the flush pin above no +// longer exercises Unwrap, so this is the test that keeps it from silently +// vanishing for every layer inside metrics. A real server is load-bearing +// here: a recorder has no deadlines to set (same pattern as the gzip-layer +// deadline pin in gzip_test.go). +func TestStatusWriter_ResponseControllerDeadlinesTunnelThroughMetrics(t *testing.T) { + deadlineErr := make(chan error, 1) // handler runs on the server goroutine + mux := http.NewServeMux() + mux.Handle("GET /", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + deadlineErr <- http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute)) + }))) + h := metricsMiddleware(mux) + + srv := httptest.NewServer(h) + defer srv.Close() + + res, err := srv.Client().Get(srv.URL + "/") + require.NoError(t, err) + res.Body.Close() + require.NoError(t, <-deadlineErr, + "deadline control must reach the underlying connection via statusWriter.Unwrap") +} + +// The flush blind spot, closed (#174, from the #165 review): a +// ResponseController flush before any write commits the implied 200 on the +// wire, so the status capture must see it — without statusWriter.FlushError +// the flush tunnels past via Unwrap and a flush-then-panic meters 500 +// against a wire-committed 200 (the panic relabel is only for the +// never-written case). +func TestStatusWriter_FlushThenPanicMetersCommitted200(t *testing.T) { + flushErr := make(chan error, 1) // buffered: read after the panic recovery + mux := http.NewServeMux() + mux.Handle("GET /flush-boom", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + flushErr <- http.NewResponseController(w).Flush() + panic("handler exploded after the flush") + }))) + h := metricsMiddleware(mux) + + counter200 := requestsTotal.WithLabelValues("GET /flush-boom", http.MethodGet, "200", "") + counter500 := requestsTotal.WithLabelValues("GET /flush-boom", http.MethodGet, "500", "") + before200 := testutil.ToFloat64(counter200) + before500 := testutil.ToFloat64(counter500) + + req := httptest.NewRequest(http.MethodGet, "/flush-boom", nil) + panicked := func() (p any) { + defer func() { p = recover() }() + h.ServeHTTP(httptest.NewRecorder(), req) + return nil + }() + require.Equal(t, "handler exploded after the flush", panicked, + "the panic must propagate past the metrics layer") + require.NoError(t, <-flushErr, "premise: the flush succeeded, so the implied 200 committed") + require.Equal(t, before200+1, testutil.ToFloat64(counter200), + `the flush committed the implied 200 on the wire — the capture must meter it as status="200"`) + require.Equal(t, before500, testutil.ToFloat64(counter500), + "the 500 relabel is only for panics with NOTHING committed — the flush committed") +} + +// The genuine-failure side of the capture's errors.Is discriminator (#174; +// mirror of the commitWriter #164 and cacheControlWriter #172 pins): a first +// flush failing with a real I/O error still COMMITTED — on a real server +// net/http snapshots the headers onto the wire before the conn-write error +// returns — so the captured implied 200 must keep. A handler reacting to the +// failed flush with an error status changes nothing the wire can honor; the +// meter must report the 200 that went out, not the 503 that never did. +func TestStatusWriter_GenuineFlushErrorKeepsCommitted200(t *testing.T) { + errConnReset := errors.New("conn reset") + mux := http.NewServeMux() + mux.Handle("GET /flush-genuine-fail", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + err := http.NewResponseController(w).Flush() + assert.ErrorIs(t, err, errConnReset, "the base writer's genuine flush error must surface") + assert.NotErrorIs(t, err, http.ErrNotSupported, "premise: a real failure, not the refusal") + w.WriteHeader(http.StatusServiceUnavailable) // handler reacts — superfluous on a real wire + }))) + h := metricsMiddleware(mux) + + counter200 := requestsTotal.WithLabelValues("GET /flush-genuine-fail", http.MethodGet, "200", "") + counter503 := requestsTotal.WithLabelValues("GET /flush-genuine-fail", http.MethodGet, "503", "") + before200 := testutil.ToFloat64(counter200) + before503 := testutil.ToFloat64(counter503) + + req := httptest.NewRequest(http.MethodGet, "/flush-genuine-fail", nil) + h.ServeHTTP(&flushErrorWriter{rr: httptest.NewRecorder(), err: errConnReset}, req) + + require.Equal(t, before200+1, testutil.ToFloat64(counter200), + "a genuinely failed flush really committed — the captured 200 must keep") + require.Equal(t, before503, testutil.ToFloat64(counter503), + "the later error status never reached the wire — it must not meter") +} + +// The refusal side of the discriminator (#174; the #164 rollback shape): on +// an unflushable chain the delegated flush returns http.ErrNotSupported — +// nothing reached the wire — so the capture this flush made must roll back, +// leaving the status label to the final write that CAN still happen. A +// capture left latched would meter 200 against a wire that went on to carry +// the handler's 503. +func TestStatusWriter_RefusedFlushRollsBackCaptureForFinalStatus(t *testing.T) { + mux := http.NewServeMux() + mux.Handle("GET /flush-refused", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + err := http.NewResponseController(w).Flush() + assert.ErrorIs(t, err, http.ErrNotSupported, "premise: an unflushable base refuses the flush") + w.WriteHeader(http.StatusServiceUnavailable) + }))) + h := metricsMiddleware(mux) + + counter200 := requestsTotal.WithLabelValues("GET /flush-refused", http.MethodGet, "200", "") + counter503 := requestsTotal.WithLabelValues("GET /flush-refused", http.MethodGet, "503", "") + before200 := testutil.ToFloat64(counter200) + before503 := testutil.ToFloat64(counter503) + + req := httptest.NewRequest(http.MethodGet, "/flush-refused", nil) + h.ServeHTTP(&noFlushWriter{rr: httptest.NewRecorder()}, req) + + require.Equal(t, before503+1, testutil.ToFloat64(counter503), + "the refused flush sent nothing — the final 503 is what the wire carries and what must meter") + require.Equal(t, before200, testutil.ToFloat64(counter200), + "a rolled-back capture must not leave a phantom 200 child") +} + // A handler that commits a 200 (WriteHeader+Write) and THEN panics meters as // status="200" — the status is already on the wire, so relabeling it 500 // would claim a response the client never received. The 500 relabel applies diff --git a/rest/wrapperconventions_test.go b/rest/wrapperconventions_test.go index ab04bbd..1f75ddd 100644 --- a/rest/wrapperconventions_test.go +++ b/rest/wrapperconventions_test.go @@ -384,15 +384,15 @@ func parseRestPackage(t *testing.T) (*token.FileSet, []*ast.File) { // The FlushError convention, enforced over the package: zero violations, and // the floors prove the walker still sees the real wrapper population (four -// wrappers; gzipResponseWriter, commitWriter, and cacheControlWriter carry -// FlushError today — bump the floors when wrappers come or go). +// wrappers, all four carrying FlushError since #174 gave statusWriter the +// #164/#167 treatment — bump the floors when wrappers come or go). func TestRestWrappers_FlushErrorConvention(t *testing.T) { fset, files := parseRestPackage(t) violations, wrappers, flushErrors := flushConventionViolations(fset, files) require.GreaterOrEqual(t, wrappers, 4, "wrapper count regressed — the embed detection or file filter broke (the chain mounts four writer wrappers)") - require.GreaterOrEqual(t, flushErrors, 3, + require.GreaterOrEqual(t, flushErrors, 4, "FlushError count regressed — the method scan broke") for _, v := range violations { t.Error(v) From f30d4960ca7ca021fca774c2bb54138d536ff30d Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:38:17 -0400 Subject: [PATCH 4/7] rest: extract chain() shared by New + composition test; dead-end flush lint (#174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The composition test now consumes chain() — the same function New composes its middleware with — so mirror-drift is impossible by construction. The flush convention closes its Unwrap hole: every ResponseWriter wrapper must declare FlushError() error or Unwrap() http.ResponseWriter; a dead-end wrapper (neither, no Flush) now violates, with a meta-case pinning the detection. --- rest/flushchain_internal_test.go | 37 +++++------ rest/rest.go | 18 +++-- rest/wrapperconventions_test.go | 111 +++++++++++++++++++++++++------ 3 files changed, 122 insertions(+), 44 deletions(-) diff --git a/rest/flushchain_internal_test.go b/rest/flushchain_internal_test.go index a885057..f890986 100644 --- a/rest/flushchain_internal_test.go +++ b/rest/flushchain_internal_test.go @@ -5,13 +5,15 @@ package rest // cachecontrol, gzip each pinned individually) — nothing pinned the COMPOSED // property, so a wrapper inserted into New tomorrow without // FlushError/Flusher/Unwrap would break handler flushes in production with -// every existing test green. This test assembles the New-shaped stack — -// sentry → metrics → auth (→ sentryLabel) → gzip → clean-path 307 → mux, -// with the probe registered through handle() exactly like a production route -// (routeLabel + requireScope + cacheControl) — over a real server and pins -// the composed behavior: the mid-body flush succeeds, and the flushed prefix -// is decodable on the wire while the handler still holds the tail. Internal -// (package rest) because the chain pieces New composes are unexported. +// every existing test green. This test assembles the stack through chain() +// (rest.go) — the SAME function New composes its middleware with, so the +// assembly under test is production's by construction, not a hand-kept +// mirror that could drift (#174 review) — with routes() swapped for a probe +// mux whose route registers through handle() exactly like a production route +// (routeLabel + requireScope + cacheControl). Over a real server it pins the +// composed behavior: the mid-body flush succeeds, and the flushed prefix is +// decodable on the wire while the handler still holds the tail. Internal +// (package rest) because chain and the wrappers it mounts are unexported. import ( "compress/gzip" @@ -49,21 +51,16 @@ func TestFlushChain_NewShapedStackStreamsFlushedPrefix(t *testing.T) { writeErr <- err }) - // The New-shaped assembly, middleware nesting mirrored from New verbatim - // with routes() swapped for the probe mux. handle() gives the probe the - // full per-route wrap set (routeLabel — the metrics sweep's never-routed - // contract holds for this traffic too — plus requireScope and - // cacheControl), so the flush traverses every writer wrapper a production - // route's would: commitWriter → statusWriter → gzipResponseWriter → - // cacheControlWriter. + // The production assembly itself: chain() is what New composes the + // middleware with (rest.go), here with routes() swapped for the probe + // mux. handle() gives the probe the full per-route wrap set (routeLabel — + // the metrics sweep's never-routed contract holds for this traffic too — + // plus requireScope and cacheControl), so the flush traverses every + // writer wrapper a production route's would: commitWriter → statusWriter + // → gzipResponseWriter → cacheControlWriter. mux := http.NewServeMux() handle(mux, "GET /", "read", maxAgeRosterFamily, probe) - h := sentryMiddleware( - metricsMiddleware( - AuthMiddleware(&sentryFakeDatastore{}, - sentryLabel( - GzipMiddleware( - cleanPathRedirect(mux)))))) + h := chain(&sentryFakeDatastore{}, mux) srv := httptest.NewServer(h) defer srv.Close() diff --git a/rest/rest.go b/rest/rest.go index 1ba66e0..1f3760b 100644 --- a/rest/rest.go +++ b/rest/rest.go @@ -7,7 +7,7 @@ // package is the permanent home — at cutover the single public listener // serves every route through it, and Phase 4 deletes the old stacks. // -// # Middleware chain (PRD order — assembled in New) +// # Middleware chain (PRD order — assembled in chain, New's composition) // // sentry → metrics → auth (→ sentryLabel) → gzip → clean-path 307 → mux // @@ -90,7 +90,7 @@ var ( // New assembles the new stack: the route mux wrapped in the PRD middleware // chain (sentry → metrics → auth (→ sentryLabel) → gzip → clean-path 307 → -// mux). The returned handler serves +// mux; the one definition lives on chain below). The returned handler serves // the /api surface; non-API paths (the docs UI) are the cutover slice's // concern (#134) and 404 here until then. // @@ -108,13 +108,23 @@ func New(ds datastores.Datastore, rc datastores.TicketReferenceCache) http.Handl if rc == nil { panic("rest.New: nil TicketReferenceCache — pass the refreshed referencecache.Cache (see #134)") } + return chain(ds, routes(ds, rc)) +} + +// chain wraps inner — the route mux, in production — in the PRD middleware +// order: sentry → metrics → auth (→ sentryLabel) → gzip → clean-path 307 → +// inner. ONE definition, shared by New and the composed flush-chain pin +// (flushchain_internal_test.go), so the assembly that test exercises IS the +// assembly production serves: a wrapper inserted here is exercised by the +// composition test by construction, and the test cannot rot into a +// hand-maintained mirror of an assembly that moved on (#174 review). +func chain(ds datastores.Datastore, inner http.Handler) http.Handler { return sentryMiddleware( metricsMiddleware( AuthMiddleware(ds, sentryLabel( GzipMiddleware( - cleanPathRedirect( - routes(ds, rc))))))) + cleanPathRedirect(inner)))))) } // routes builds the pattern-routing mux. Every route registers through diff --git a/rest/wrapperconventions_test.go b/rest/wrapperconventions_test.go index 1f75ddd..987cef2 100644 --- a/rest/wrapperconventions_test.go +++ b/rest/wrapperconventions_test.go @@ -8,9 +8,14 @@ package rest_test // wrapper, before it has any tests at all: // // - FlushError convention: every ResponseWriter wrapper implements -// `FlushError() error` or doesn't intercept flush at all — never bare +// `FlushError() error` or skips flush interception and declares +// `Unwrap() http.ResponseWriter` so the controller tunnels — never bare // `Flush()`, whose errors http.ResponseController's Flusher branch -// silently swallows (the wrapper reports success on a flush that died). +// silently swallows (the wrapper reports success on a flush that died), +// and never NEITHER: a wrapper with no FlushError, no Flush and no +// Unwrap dead-ends the controller's method walk, turning every handler +// flush through it into http.ErrNotSupported in production (#174 +// review — the opaque-wrapper mutant survived the whole suite). // - Informational predicate (#165 review): every WriteHeader method on a // wrapper must consult informational() — a WriteHeader-stateful wrapper // added without the non-latching-1xx guard (and without volunteering @@ -41,12 +46,12 @@ import ( // sanctioned ways forward when a handler legitimately needs the connection. const hijackRemedy = "no production code in package rest may hijack — the gzip close-log's coarse ErrHijacked carve-out (#175) is only safe while nothing hijacks through the chain. Mount upgrade endpoints OUTSIDE GzipMiddleware, or implement the faithfulness spec preserved in .out-of-scope/gzip-hijack-faithfulness.md (PR #185, from #181)" -// wrapperTypes returns the names of every struct type in files that embeds +// wrapperTypes returns every struct type in files that embeds // http.ResponseWriter — the package's writer-wrapper convention (all four -// production wrappers embed it; a wrapper holding the delegate in a named -// field would not satisfy http.ResponseWriter and could not enter the chain). -func wrapperTypes(files []*ast.File) map[string]bool { - wrappers := map[string]bool{} +// production wrappers embed it) — keyed by type name, with the TypeSpec +// position for violation messages. +func wrapperTypes(files []*ast.File) map[string]token.Pos { + wrappers := map[string]token.Pos{} for _, f := range files { ast.Inspect(f, func(n ast.Node) bool { ts, ok := n.(*ast.TypeSpec) @@ -63,7 +68,7 @@ func wrapperTypes(files []*ast.File) map[string]bool { } if sel, ok := fld.Type.(*ast.SelectorExpr); ok { if x, ok := sel.X.(*ast.Ident); ok && x.Name == "http" && sel.Sel.Name == "ResponseWriter" { - wrappers[ts.Name.Name] = true + wrappers[ts.Name.Name] = ts.Pos() } } } @@ -92,51 +97,106 @@ func receiverTypeName(fd *ast.FuncDecl) string { // isFlushErrorSignature reports whether the method is exactly // `FlushError() error` — no parameters, one unnamed error result. func isFlushErrorSignature(fd *ast.FuncDecl) bool { - if fd.Type.Params != nil && len(fd.Type.Params.List) != 0 { + res, ok := soleResult(fd) + if !ok { return false } - if fd.Type.Results == nil || len(fd.Type.Results.List) != 1 { + id, ok := res.(*ast.Ident) + return ok && id.Name == "error" +} + +// isUnwrapSignature reports whether the method is exactly +// `Unwrap() http.ResponseWriter` — the only shape +// http.ResponseController's method walk descends through. Anything else +// (parameters, a different result type) is invisible to the controller. +func isUnwrapSignature(fd *ast.FuncDecl) bool { + res, ok := soleResult(fd) + if !ok { + return false + } + sel, ok := res.(*ast.SelectorExpr) + if !ok { return false } + x, ok := sel.X.(*ast.Ident) + return ok && x.Name == "http" && sel.Sel.Name == "ResponseWriter" +} + +// soleResult returns the method's single unnamed result type, reporting +// false for any other parameter/result arity. +func soleResult(fd *ast.FuncDecl) (ast.Expr, bool) { + if fd.Type.Params != nil && len(fd.Type.Params.List) != 0 { + return nil, false + } + if fd.Type.Results == nil || len(fd.Type.Results.List) != 1 { + return nil, false + } res := fd.Type.Results.List[0] if len(res.Names) != 0 { - return false + return nil, false } - id, ok := res.Type.(*ast.Ident) - return ok && id.Name == "error" + return res.Type, true } // flushConventionViolations applies the FlushError convention to every // ResponseWriter wrapper in files: never bare Flush() (the controller's -// Flusher branch swallows its errors), and a FlushError must carry the exact +// Flusher branch swallows its errors); a FlushError must carry the exact // `FlushError() error` shape the controller's method search matches — any // other signature is dead code the controller skips, falling through to the -// swallow-or-tunnel paths the author thought they had replaced. Returns the -// wrapper and FlushError-method counts for the caller's vacuous-pass floors. +// swallow-or-tunnel paths the author thought they had replaced; and every +// wrapper must declare `FlushError() error` OR `Unwrap() http.ResponseWriter` +// — a wrapper with neither (and no Flush) dead-ends the controller's method +// walk, so every handler flush through it returns http.ErrNotSupported in +// production (#174 review: that opaque-wrapper mutant survived the entire +// suite before this rule). Returns the wrapper and FlushError-method counts +// for the caller's vacuous-pass floors. func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violations []string, wrappers, flushErrors int) { wrapperSet := wrapperTypes(files) wrappers = len(wrapperSet) + // intercepts: declares Flush or FlushError under ANY signature — shape + // problems are flagged individually above, so the dead-end rule below + // fires only when the controller finds nothing at all to walk. + intercepts := map[string]bool{} + unwraps := map[string]bool{} // declares the exact `Unwrap() http.ResponseWriter` for _, f := range files { for _, decl := range f.Decls { fd, ok := decl.(*ast.FuncDecl) - if !ok || !wrapperSet[receiverTypeName(fd)] { + if !ok { + continue + } + recv := receiverTypeName(fd) + if _, isWrapper := wrapperSet[recv]; !isWrapper { continue } switch fd.Name.Name { case "Flush": + intercepts[recv] = true violations = append(violations, fmt.Sprintf( "%s: %s.Flush: bare Flush() on a ResponseWriter wrapper — http.ResponseController's Flusher branch silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error instead (see gzipResponseWriter/commitWriter/cacheControlWriter), or drop flush interception entirely and let Unwrap tunnel it", - fset.Position(fd.Pos()), receiverTypeName(fd))) + fset.Position(fd.Pos()), recv)) case "FlushError": + intercepts[recv] = true flushErrors++ if !isFlushErrorSignature(fd) { violations = append(violations, fmt.Sprintf( "%s: %s.FlushError: signature must be exactly `FlushError() error` — anything else is invisible to http.ResponseController's method search and never runs", - fset.Position(fd.Pos()), receiverTypeName(fd))) + fset.Position(fd.Pos()), recv)) + } + case "Unwrap": + if isUnwrapSignature(fd) { + unwraps[recv] = true } } } } + for name, pos := range wrapperSet { + if intercepts[name] || unwraps[name] { + continue + } + violations = append(violations, fmt.Sprintf( + "%s: %s: ResponseWriter wrapper with neither `FlushError() error` nor `Unwrap() http.ResponseWriter` — http.ResponseController's method walk dead-ends here, so every handler flush (and every other controller verb) through this layer returns http.ErrNotSupported in production. Implement FlushError() error to intercept flush (see gzipResponseWriter/commitWriter/cacheControlWriter), or declare Unwrap and let the controller tunnel", + fset.Position(pos), name)) + } return violations, wrappers, flushErrors } @@ -152,7 +212,10 @@ func informationalPredicateViolations(fset *token.FileSet, files []*ast.File) (v for _, f := range files { for _, decl := range f.Decls { fd, ok := decl.(*ast.FuncDecl) - if !ok || fd.Name.Name != "WriteHeader" || !wrapperSet[receiverTypeName(fd)] { + if !ok || fd.Name.Name != "WriteHeader" { + continue + } + if _, isWrapper := wrapperSet[receiverTypeName(fd)]; !isWrapper { continue } writeHeaders++ @@ -227,6 +290,14 @@ func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { name: "no flush interception at all", decls: syntheticWrapper + "func (w *fakeWrap) Unwrap() http.ResponseWriter { return w.ResponseWriter }\n", }, + { + // The opaque-wrapper mutant from the #174 review: no FlushError, + // no Flush, no Unwrap — the controller's method walk dead-ends + // and every handler flush returns http.ErrNotSupported. + name: "dead end: neither FlushError nor Unwrap", + decls: syntheticWrapper, + want: "neither `FlushError() error` nor `Unwrap() http.ResponseWriter`", + }, { name: "bare Flush", decls: syntheticWrapper + "func (w *fakeWrap) Flush() {}\n", From a5c0893e70ee349b0ee1eda5d6d1001654255380 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:41:11 -0400 Subject: [PATCH 5/7] rest: statusWriter keep-side rollback pin; commitWriter witness; 103-flush-panic pin (#174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three pins from the #174 reviews: the rollback's captured&& guard (refused flush after WriteHeader(503) keeps the capture — symmetric with cachecontrol's keep-side pin), the commitWriter-presence witness the composition test's premise stands on, and the #165 x #174 composition (103 non-latching, flush captures implied 200, panic meters 200). --- rest/flushchain_internal_test.go | 4 +- rest/metrics_internal_test.go | 69 ++++++++++++++++++++++++++++++++ rest/sentry_test.go | 16 ++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/rest/flushchain_internal_test.go b/rest/flushchain_internal_test.go index f890986..e630600 100644 --- a/rest/flushchain_internal_test.go +++ b/rest/flushchain_internal_test.go @@ -32,7 +32,9 @@ import ( func TestFlushChain_NewShapedStackStreamsFlushedPrefix(t *testing.T) { // Sentry ENABLED: without a client sentryMiddleware never mounts // commitWriter, and the composed property would silently exclude the - // outermost wrapper. + // outermost wrapper. The premise that an enabled client mounts the + // wrapper UNCONDITIONALLY is itself pinned — + // TestSentryMiddleware_MountsCommitWriterWhenEnabled (sentry_test.go). enableSentry(t) const part1 = "first chunk, flushed mid-stream" diff --git a/rest/metrics_internal_test.go b/rest/metrics_internal_test.go index 849e2d8..c47e74c 100644 --- a/rest/metrics_internal_test.go +++ b/rest/metrics_internal_test.go @@ -166,6 +166,75 @@ func TestStatusWriter_RefusedFlushRollsBackCaptureForFinalStatus(t *testing.T) { "a rolled-back capture must not leave a phantom 200 child") } +// The KEEP side of the rollback's first-to-capture guard (#174 review; the +// symmetric pin to cachecontrol_internal_test.go's "FlushError after +// WriteHeader 500 does not stamp"): the rollback may only undo what THIS +// flush latched. A refused flush AFTER an explicit status must leave the +// capture alone — without the `captured &&` guard it wipes it, and a +// WriteHeader(503) → refused flush meters 200 against a wire-committed 5xx. +func TestStatusWriter_RefusedFlushAfterExplicitStatusKeepsCapture(t *testing.T) { + mux := http.NewServeMux() + mux.Handle("GET /flush-refused-after-status", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + err := http.NewResponseController(w).Flush() + assert.ErrorIs(t, err, http.ErrNotSupported, "premise: an unflushable base refuses the flush") + }))) + h := metricsMiddleware(mux) + + counter200 := requestsTotal.WithLabelValues("GET /flush-refused-after-status", http.MethodGet, "200", "") + counter503 := requestsTotal.WithLabelValues("GET /flush-refused-after-status", http.MethodGet, "503", "") + before200 := testutil.ToFloat64(counter200) + before503 := testutil.ToFloat64(counter503) + + req := httptest.NewRequest(http.MethodGet, "/flush-refused-after-status", nil) + h.ServeHTTP(&noFlushWriter{rr: httptest.NewRecorder()}, req) + + require.Equal(t, before503+1, testutil.ToFloat64(counter503), + "the 503 was captured before the refused flush — the rollback must not touch a capture it did not make") + require.Equal(t, before200, testutil.ToFloat64(counter200), + "a refused flush after an explicit status must not relabel the request 200") +} + +// The #165 × #174 composition the issue comment named, pinned nowhere else: +// a forwarded non-latching 103 first (captures nothing, #165), then a flush +// commits the implied 200 (FlushError captures it, #174), then a panic — the +// meter must report the 200 the wire carries: not the never-written relabel +// 500, and never the 103 (a non-latching informational is never the final +// status label). +func TestStatusWriter_InformationalThenFlushThenPanicMetersCommitted200(t *testing.T) { + flushErr := make(chan error, 1) // buffered: read after the panic recovery + mux := http.NewServeMux() + mux.Handle("GET /hints-flush-boom", routeLabel(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusEarlyHints) // forwarded, non-latching (#165) + flushErr <- http.NewResponseController(w).Flush() + panic("handler exploded after the 103 and the flush") + }))) + h := metricsMiddleware(mux) + + counter200 := requestsTotal.WithLabelValues("GET /hints-flush-boom", http.MethodGet, "200", "") + counter103 := requestsTotal.WithLabelValues("GET /hints-flush-boom", http.MethodGet, "103", "") + counter500 := requestsTotal.WithLabelValues("GET /hints-flush-boom", http.MethodGet, "500", "") + before200 := testutil.ToFloat64(counter200) + before103 := testutil.ToFloat64(counter103) + before500 := testutil.ToFloat64(counter500) + + req := httptest.NewRequest(http.MethodGet, "/hints-flush-boom", nil) + panicked := func() (p any) { + defer func() { p = recover() }() + h.ServeHTTP(httptest.NewRecorder(), req) + return nil + }() + require.Equal(t, "handler exploded after the 103 and the flush", panicked, + "the panic must propagate past the metrics layer") + require.NoError(t, <-flushErr, "premise: the flush succeeded, so the implied 200 committed") + require.Equal(t, before200+1, testutil.ToFloat64(counter200), + `the flush after the 103 committed the implied 200 — it must meter as status="200"`) + require.Equal(t, before103, testutil.ToFloat64(counter103), + "a forwarded non-latching 103 must never become the final status label") + require.Equal(t, before500, testutil.ToFloat64(counter500), + "the 500 relabel is only for panics with NOTHING committed — the flush committed") +} + // A handler that commits a 200 (WriteHeader+Write) and THEN panics meters as // status="200" — the status is already on the wire, so relabeling it 500 // would claim a response the client never received. The 500 relabel applies diff --git a/rest/sentry_test.go b/rest/sentry_test.go index 8474141..7a37341 100644 --- a/rest/sentry_test.go +++ b/rest/sentry_test.go @@ -162,6 +162,22 @@ func TestSetupSentry_DSNEnablesClientWithRelease(t *testing.T) { assert.Equal(t, testRelease, events[0].Release, "events must carry the build-time release") } +// commitWriter mounts UNCONDITIONALLY whenever a client is bound — the +// composed flush-chain pin (flushchain_internal_test.go) enables sentry on +// exactly this premise, so a refactor that mounts the wrapper conditionally +// (per-route, per-method, …) fails here by name instead of silently +// shrinking that pin's coverage to a commitWriter-less chain (#174 review). +func TestSentryMiddleware_MountsCommitWriterWhenEnabled(t *testing.T) { + enableSentry(t) + var sawCommitWriter bool + h := sentryMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, sawCommitWriter = w.(*commitWriter) + })) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + require.True(t, sawCommitWriter, + "with a client bound, sentryMiddleware must hand every handler a *commitWriter — the recovery's committed signal and the composed flush-chain pin both stand on it") +} + // The headline acceptance (#132): a panicking handler produces ONE event — // tagged with the release, the validated key id, and the matched route // pattern — and the request still completes as a 500 in the contract error From f63a9b4f0a9349ae208cd8017798e458ca914733 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:46:22 -0400 Subject: [PATCH 6/7] rest: wrapper scan goes transitive, field-shape-blind, alias-proof (#174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three confirmed detection evasions closed: a wrapper embedding another wrapper (fixpoint over embeds, feeding the flush AND informational guards, with promotion-aware dead-end escapes), a named-field http.ResponseWriter delegate (the embed-only comment's rationale was false — corrected), and an aliased net/http import (local name resolved per file). Meta-cases pin each; floors keep, with failure messages that name the bump-the-floor path and violations printing before floor aborts. --- rest/wrapperconventions_test.go | 263 ++++++++++++++++++++++++++------ 1 file changed, 215 insertions(+), 48 deletions(-) diff --git a/rest/wrapperconventions_test.go b/rest/wrapperconventions_test.go index 987cef2..9e85c17 100644 --- a/rest/wrapperconventions_test.go +++ b/rest/wrapperconventions_test.go @@ -46,13 +46,51 @@ import ( // sanctioned ways forward when a handler legitimately needs the connection. const hijackRemedy = "no production code in package rest may hijack — the gzip close-log's coarse ErrHijacked carve-out (#175) is only safe while nothing hijacks through the chain. Mount upgrade endpoints OUTSIDE GzipMiddleware, or implement the faithfulness spec preserved in .out-of-scope/gzip-hijack-faithfulness.md (PR #185, from #181)" -// wrapperTypes returns every struct type in files that embeds -// http.ResponseWriter — the package's writer-wrapper convention (all four -// production wrappers embed it) — keyed by type name, with the TypeSpec -// position for violation messages. -func wrapperTypes(files []*ast.File) map[string]token.Pos { - wrappers := map[string]token.Pos{} +// httpImportName resolves the file-local name bound to the "net/http" import +// — "http" by default, the alias when one is declared, "" when the file does +// not import net/http at all (then no selector can reference it). Dot- and +// blank-imports also yield "" — neither produces the selector shape the +// scans match, and neither appears in this package. +func httpImportName(f *ast.File) string { + for _, imp := range f.Imports { + if imp.Path.Value != `"net/http"` { + continue + } + if imp.Name == nil { + return "http" + } + if name := imp.Name.Name; name != "." && name != "_" { + return name + } + } + return "" +} + +// wrapperTypes returns every struct type in files that carries a +// http.ResponseWriter delegate — keyed by type name, with the TypeSpec +// position for violation messages — plus the embedded-type edges of EVERY +// struct, for the caller's method-promotion reasoning. Wrapper evidence is +// transitive and field-shape-blind (#174 review closed three detection +// holes): +// +// - a field of type http.ResponseWriter counts whether EMBEDDED or NAMED: +// a named delegate plus hand-written Header/Write/WriteHeader methods +// satisfies the interface exactly as well, so the field shape is no +// boundary (the four production wrappers all embed; the _test.go +// delegate fakes are the named shape, and the production scan skips +// test files); +// - embedding another wrapper, by value or pointer, makes a wrapper — +// computed to fixpoint, so a wrapper-of-a-wrapper inherits every guard +// (its OWN method set is what http.ResponseController's walk sees); +// - the net/http import's LOCAL name is resolved per file +// (httpImportName), so an aliased import cannot smuggle a wrapper past +// the scan. +func wrapperTypes(files []*ast.File) (wrappers map[string]token.Pos, embeds map[string][]string) { + wrappers = map[string]token.Pos{} + embeds = map[string][]string{} + positions := map[string]token.Pos{} for _, f := range files { + httpName := httpImportName(f) ast.Inspect(f, func(n ast.Node) bool { ts, ok := n.(*ast.TypeSpec) if !ok { @@ -62,20 +100,47 @@ func wrapperTypes(files []*ast.File) map[string]token.Pos { if !ok { return true } + positions[ts.Name.Name] = ts.Pos() for _, fld := range st.Fields.List { - if len(fld.Names) != 0 { - continue // named field, not an embed - } if sel, ok := fld.Type.(*ast.SelectorExpr); ok { - if x, ok := sel.X.(*ast.Ident); ok && x.Name == "http" && sel.Sel.Name == "ResponseWriter" { + if x, ok := sel.X.(*ast.Ident); ok && httpName != "" && x.Name == httpName && sel.Sel.Name == "ResponseWriter" { wrappers[ts.Name.Name] = ts.Pos() } + continue + } + if len(fld.Names) != 0 { + continue // named fields of package-local types carry no promotion + } + switch ft := fld.Type.(type) { + case *ast.Ident: + embeds[ts.Name.Name] = append(embeds[ts.Name.Name], ft.Name) + case *ast.StarExpr: + if id, ok := ft.X.(*ast.Ident); ok { + embeds[ts.Name.Name] = append(embeds[ts.Name.Name], id.Name) + } } } return true }) } - return wrappers + // Fixpoint: embedding a wrapper promotes its delegate, so the embedder + // is a wrapper too and must satisfy every guard itself. + for changed := true; changed; { + changed = false + for name, embedded := range embeds { + if _, done := wrappers[name]; done { + continue + } + for _, e := range embedded { + if _, isWrapper := wrappers[e]; isWrapper { + wrappers[name] = positions[name] + changed = true + break + } + } + } + } + return wrappers, embeds } // receiverTypeName resolves a method's receiver to its bare type name @@ -109,7 +174,8 @@ func isFlushErrorSignature(fd *ast.FuncDecl) bool { // `Unwrap() http.ResponseWriter` — the only shape // http.ResponseController's method walk descends through. Anything else // (parameters, a different result type) is invisible to the controller. -func isUnwrapSignature(fd *ast.FuncDecl) bool { +// httpName is the declaring file's local name for the net/http import. +func isUnwrapSignature(fd *ast.FuncDecl, httpName string) bool { res, ok := soleResult(fd) if !ok { return false @@ -119,7 +185,7 @@ func isUnwrapSignature(fd *ast.FuncDecl) bool { return false } x, ok := sel.X.(*ast.Ident) - return ok && x.Name == "http" && sel.Sel.Name == "ResponseWriter" + return ok && httpName != "" && x.Name == httpName && sel.Sel.Name == "ResponseWriter" } // soleResult returns the method's single unnamed result type, reporting @@ -151,46 +217,72 @@ func soleResult(fd *ast.FuncDecl) (ast.Expr, bool) { // suite before this rule). Returns the wrapper and FlushError-method counts // for the caller's vacuous-pass floors. func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violations []string, wrappers, flushErrors int) { - wrapperSet := wrapperTypes(files) + wrapperSet, embeds := wrapperTypes(files) wrappers = len(wrapperSet) - // intercepts: declares Flush or FlushError under ANY signature — shape - // problems are flagged individually above, so the dead-end rule below - // fires only when the controller finds nothing at all to walk. - intercepts := map[string]bool{} - unwraps := map[string]bool{} // declares the exact `Unwrap() http.ResponseWriter` + // escapes: the type declares Flush or FlushError under ANY signature + // (shape problems are flagged individually, so the dead-end rule fires + // only when the controller finds nothing at all to walk) or the exact + // `Unwrap() http.ResponseWriter`. Recorded for EVERY receiver type, not + // just wrappers: an embedder inherits the promoted methods below. + escapes := map[string]bool{} for _, f := range files { + httpName := httpImportName(f) for _, decl := range f.Decls { fd, ok := decl.(*ast.FuncDecl) if !ok { continue } recv := receiverTypeName(fd) - if _, isWrapper := wrapperSet[recv]; !isWrapper { + if recv == "" { continue } + _, isWrapper := wrapperSet[recv] switch fd.Name.Name { case "Flush": - intercepts[recv] = true - violations = append(violations, fmt.Sprintf( - "%s: %s.Flush: bare Flush() on a ResponseWriter wrapper — http.ResponseController's Flusher branch silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error instead (see gzipResponseWriter/commitWriter/cacheControlWriter), or drop flush interception entirely and let Unwrap tunnel it", - fset.Position(fd.Pos()), recv)) - case "FlushError": - intercepts[recv] = true - flushErrors++ - if !isFlushErrorSignature(fd) { + escapes[recv] = true + if isWrapper { violations = append(violations, fmt.Sprintf( - "%s: %s.FlushError: signature must be exactly `FlushError() error` — anything else is invisible to http.ResponseController's method search and never runs", + "%s: %s.Flush: bare Flush() on a ResponseWriter wrapper — http.ResponseController's Flusher branch silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error instead (see gzipResponseWriter/commitWriter/cacheControlWriter), or drop flush interception entirely and let Unwrap tunnel it", fset.Position(fd.Pos()), recv)) } + case "FlushError": + escapes[recv] = true + if isWrapper { + flushErrors++ + if !isFlushErrorSignature(fd) { + violations = append(violations, fmt.Sprintf( + "%s: %s.FlushError: signature must be exactly `FlushError() error` — anything else is invisible to http.ResponseController's method search and never runs", + fset.Position(fd.Pos()), recv)) + } + } case "Unwrap": - if isUnwrapSignature(fd) { - unwraps[recv] = true + if isUnwrapSignature(fd, httpName) { + escapes[recv] = true + } + } + } + } + // Promotion fixpoint: an embedder with no flush machinery of its own + // still escapes the dead end when an embedded type provides it — Go + // promotes the embedded FlushError/Unwrap into the embedder's method + // set, where the controller's walk finds them. + for changed := true; changed; { + changed = false + for name, embedded := range embeds { + if escapes[name] { + continue + } + for _, e := range embedded { + if escapes[e] { + escapes[name] = true + changed = true + break } } } } for name, pos := range wrapperSet { - if intercepts[name] || unwraps[name] { + if escapes[name] { continue } violations = append(violations, fmt.Sprintf( @@ -208,7 +300,7 @@ func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violatio // wrappers did. Returns the WriteHeader-method count for the caller's // vacuous-pass floor. func informationalPredicateViolations(fset *token.FileSet, files []*ast.File) (violations []string, writeHeaders int) { - wrapperSet := wrapperTypes(files) + wrapperSet, _ := wrapperTypes(files) // the transitive set feeds this guard too (#174 review) for _, f := range files { for _, decl := range f.Decls { fd, ok := decl.(*ast.FuncDecl) @@ -275,12 +367,14 @@ func parseSyntheticRest(t *testing.T, decls string) (*token.FileSet, []*ast.File const syntheticWrapper = "type fakeWrap struct {\n\thttp.ResponseWriter\n}\n\n" // The flush-convention checker must see each break — and stay silent on the -// two conforming shapes (FlushError() error, or no flush interception). +// two conforming shapes (FlushError() error, or Unwrap tunnelling with no +// flush interception). func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { cases := []struct { - name string - decls string - want string // substring of the single expected violation; "" = clean + name string + decls string + want string // substring of the single expected violation; "" = clean + wantType string // wrapper the violation must name; "" = fakeWrap }{ { name: "conforming FlushError", @@ -322,6 +416,40 @@ func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { name: "Flush on a non-wrapper stays out of scope", decls: "type plainBuffer struct{ n int }\n\nfunc (b *plainBuffer) Flush() {}\n", }, + { + // Detection hole 1 (#174 review): a wrapper embedding another + // wrapper is itself a wrapper — its OWN method set is what the + // controller's walk sees, so it carries the conventions itself. + name: "wrapper embedding another wrapper inherits the conventions", + decls: syntheticWrapper + + "func (w *fakeWrap) FlushError() error { return nil }\n\n" + + "type outerWrap struct{ *fakeWrap }\n\nfunc (w *outerWrap) Flush() {}\n", + want: "bare Flush()", + wantType: "outerWrap", + }, + { + // The promotion escape: a method-less embedder inherits the + // embedded wrapper's FlushError into its method set, where the + // controller's walk finds it — no dead end, no violation. + name: "method-less wrapper embedding a conforming wrapper is clean", + decls: syntheticWrapper + + "func (w *fakeWrap) FlushError() error { return nil }\n\n" + + "type outerWrap struct{ *fakeWrap }\n", + }, + { + // Detection hole 2 (#174 review): a NAMED http.ResponseWriter + // delegate plus hand-written interface methods satisfies + // http.ResponseWriter exactly as well as an embed — the old + // embed-only match was blind to this shape. + name: "named-field delegate is a wrapper", + decls: "type namedWrap struct {\n\trw http.ResponseWriter\n}\n\n" + + "func (w *namedWrap) Header() http.Header { return w.rw.Header() }\n\n" + + "func (w *namedWrap) Write(b []byte) (int, error) { return w.rw.Write(b) }\n\n" + + "func (w *namedWrap) WriteHeader(code int) { w.rw.WriteHeader(code) }\n\n" + + "func (w *namedWrap) Flush() {}\n", + want: "bare Flush()", + wantType: "namedWrap", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -331,20 +459,43 @@ func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { assert.Empty(t, got) return } + wantType := tc.wantType + if wantType == "" { + wantType = "fakeWrap" + } require.Len(t, got, 1) assert.Contains(t, got[0], tc.want) - assert.Contains(t, got[0], "fakeWrap", "violation must name the wrapper type") + assert.Contains(t, got[0], wantType, "violation must name the wrapper type") }) } } +// Detection hole 3 (#174 review): an aliased net/http import must not slip +// the wrapper scan — the local import name is resolved per file +// (httpImportName), never assumed to be "http". Its own test because the +// shared synthetic harness pins the standard import clause. +func TestFlushConventionViolations_SeesAliasedNetHTTPImport(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic_alias.go", + "package rest\n\nimport nethttp \"net/http\"\n\ntype aliasWrap struct {\n\tnethttp.ResponseWriter\n}\n\nfunc (w *aliasWrap) Flush() {}\n", + parser.SkipObjectResolution) + require.NoError(t, err) + + got, wrappers, _ := flushConventionViolations(fset, []*ast.File{f}) + assert.Equal(t, 1, wrappers, "the aliased embed must count as a wrapper") + require.Len(t, got, 1) + assert.Contains(t, got[0], "bare Flush()") + assert.Contains(t, got[0], "aliasWrap", "violation must name the wrapper type") +} + // The informational-predicate checker must flag a WriteHeader-stateful // wrapper that skips the predicate — and only on wrapper types. func TestInformationalPredicateViolations_DetectsTheSkip(t *testing.T) { cases := []struct { - name string - decls string - want string // substring of the single expected violation; "" = clean + name string + decls string + want string // substring of the single expected violation; "" = clean + wantType string // wrapper the violation must name; "" = fakeWrap }{ { name: "WriteHeader consulting the predicate", @@ -363,6 +514,16 @@ func TestInformationalPredicateViolations_DetectsTheSkip(t *testing.T) { name: "wrapper without WriteHeader is clean", decls: syntheticWrapper, }, + { + // The transitive wrapper set feeds this guard too (#174 review): + // a wrapper embedding another wrapper carries the #165 + // obligation itself — its own WriteHeader is what runs. + name: "WriteHeader on a wrapper-embedding wrapper without the predicate", + decls: syntheticWrapper + + "type outerWrap struct{ *fakeWrap }\n\nfunc (w *outerWrap) WriteHeader(code int) {\n\tw.fakeWrap.WriteHeader(code)\n}\n", + want: "must consult the informational() predicate", + wantType: "outerWrap", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -372,9 +533,13 @@ func TestInformationalPredicateViolations_DetectsTheSkip(t *testing.T) { assert.Empty(t, got) return } + wantType := tc.wantType + if wantType == "" { + wantType = "fakeWrap" + } require.Len(t, got, 1) assert.Contains(t, got[0], tc.want) - assert.Contains(t, got[0], "fakeWrap", "violation must name the wrapper type") + assert.Contains(t, got[0], wantType, "violation must name the wrapper type") }) } } @@ -461,13 +626,14 @@ func TestRestWrappers_FlushErrorConvention(t *testing.T) { fset, files := parseRestPackage(t) violations, wrappers, flushErrors := flushConventionViolations(fset, files) - require.GreaterOrEqual(t, wrappers, 4, - "wrapper count regressed — the embed detection or file filter broke (the chain mounts four writer wrappers)") - require.GreaterOrEqual(t, flushErrors, 4, - "FlushError count regressed — the method scan broke") + // Violations first: a floor abort below must never hide the findings. for _, v := range violations { t.Error(v) } + require.GreaterOrEqual(t, wrappers, 4, + "wrapper count fell below the floor — either the scan rotted (field detection, file filter, net/http import resolution) or a writer wrapper was deliberately removed from the chain; if the removal is intentional, bump this floor (and the FlushError floor below) alongside it") + require.GreaterOrEqual(t, flushErrors, 4, + "FlushError count fell below the floor — either the method scan rotted or a wrapper deliberately swapped flush interception for Unwrap tunnelling; if intentional, bump this floor alongside the change") } // The informational predicate, enforced over the package: every wrapper @@ -477,11 +643,12 @@ func TestRestWrappers_WriteHeaderConsultsInformationalPredicate(t *testing.T) { fset, files := parseRestPackage(t) violations, writeHeaders := informationalPredicateViolations(fset, files) - require.GreaterOrEqual(t, writeHeaders, 4, - "WriteHeader count regressed — the method scan or file filter broke (all four wrappers intercept WriteHeader)") + // Violations first: a floor abort below must never hide the findings. for _, v := range violations { t.Error(v) } + require.GreaterOrEqual(t, writeHeaders, 4, + "WriteHeader count fell below the floor — either the method scan / file filter rotted or a wrapper deliberately stopped intercepting WriteHeader; if intentional, bump this floor alongside it (all four production wrappers intercept it today)") } // The hijack tripwire, enforced over the package: today trivially true (only From b55dc7c12bc74d625deb47edc5090efed31349ca Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 21:06:39 -0400 Subject: [PATCH 7/7] rest: lint credits flush escapes only on exact signatures (#174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Flush/FlushError switch cases credited an escape for EVERY receiver type under ANY signature, while violations fire only on wrappers — the promotion fixpoint then laundered a non-wrapper helper's escape into a clean bill for a wrapper embedding it. Two demonstrated evasions, both previously zero-violation: - bare Flush() on a plain helper, embedded: the promoted method IS matched by http.ResponseController's Flusher branch and silently swallows errors — now flagged on the embedding wrapper, naming the helper the method promoted from (provenance via the embed graph); - malformed FlushError() (int, error) on a plain helper, embedded: invisible to the controller's method search, so the wrapper is a runtime dead end — now caught by the dead-end rule. Non-wrapper receivers now earn escape credit only on the exact shapes the controller matches (FlushError() error — the same discipline Unwrap always had); wrappers keep any-signature credit because their shape problems are flagged individually. Meta-cases added for both evasion shapes plus the legitimate-promotion counterpart (exact FlushError on a helper stays clean); all prior meta-cases unchanged and green. --- rest/wrapperconventions_test.go | 108 +++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 24 deletions(-) diff --git a/rest/wrapperconventions_test.go b/rest/wrapperconventions_test.go index 9e85c17..38fc3d7 100644 --- a/rest/wrapperconventions_test.go +++ b/rest/wrapperconventions_test.go @@ -205,26 +205,41 @@ func soleResult(fd *ast.FuncDecl) (ast.Expr, bool) { } // flushConventionViolations applies the FlushError convention to every -// ResponseWriter wrapper in files: never bare Flush() (the controller's -// Flusher branch swallows its errors); a FlushError must carry the exact -// `FlushError() error` shape the controller's method search matches — any -// other signature is dead code the controller skips, falling through to the -// swallow-or-tunnel paths the author thought they had replaced; and every -// wrapper must declare `FlushError() error` OR `Unwrap() http.ResponseWriter` -// — a wrapper with neither (and no Flush) dead-ends the controller's method -// walk, so every handler flush through it returns http.ErrNotSupported in -// production (#174 review: that opaque-wrapper mutant survived the entire -// suite before this rule). Returns the wrapper and FlushError-method counts -// for the caller's vacuous-pass floors. +// ResponseWriter wrapper in files: never bare Flush() — hand-written or +// promoted from an embedded non-wrapper helper, the controller's Flusher +// branch swallows its errors either way (a promoted one is flagged on the +// embedding wrapper, naming its origin — #174 delta review); a FlushError +// must carry the exact `FlushError() error` shape the controller's method +// search matches — any other signature is dead code the controller skips, +// falling through to the swallow-or-tunnel paths the author thought they had +// replaced; and every wrapper must declare `FlushError() error` OR +// `Unwrap() http.ResponseWriter` — a wrapper with neither (and no Flush) +// dead-ends the controller's method walk, so every handler flush through it +// returns http.ErrNotSupported in production (#174 review: that +// opaque-wrapper mutant survived the entire suite before this rule). Returns +// the wrapper and FlushError-method counts for the caller's vacuous-pass +// floors. func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violations []string, wrappers, flushErrors int) { wrapperSet, embeds := wrapperTypes(files) wrappers = len(wrapperSet) - // escapes: the type declares Flush or FlushError under ANY signature - // (shape problems are flagged individually, so the dead-end rule fires - // only when the controller finds nothing at all to walk) or the exact - // `Unwrap() http.ResponseWriter`. Recorded for EVERY receiver type, not - // just wrappers: an embedder inherits the promoted methods below. + // escapes: the type declares flush machinery the controller's method + // walk actually finds. Recorded for EVERY receiver type, not just + // wrappers — an embedder inherits the promoted methods below — but the + // crediting discipline differs (#174 delta review): wrappers earn credit + // for Flush/FlushError under ANY signature, because their shape problems + // are flagged individually and the dead-end rule should fire only when + // the controller finds nothing at all to walk; non-wrappers earn credit + // only on the EXACT shapes the controller matches (`FlushError() error`, + // `Unwrap() http.ResponseWriter`) — their shape problems are flagged + // nowhere, so any-signature credit here would launder through the + // promotion fixpoint into a clean bill for a wrapper embedding them. escapes := map[string]bool{} + // flushOrigins: bare Flush() on a non-wrapper receiver, keyed by type. + // Deliberately NOT an escape — promoted into a wrapper it IS matched, by + // the controller's Flusher branch, which swallows its errors exactly + // like a hand-written bare Flush; the embedding wrapper gets the + // bare-Flush violation below, naming the origin via the embed graph. + flushOrigins := map[string]string{} for _, f := range files { httpName := httpImportName(f) for _, decl := range f.Decls { @@ -239,21 +254,25 @@ func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violatio _, isWrapper := wrapperSet[recv] switch fd.Name.Name { case "Flush": - escapes[recv] = true if isWrapper { + escapes[recv] = true violations = append(violations, fmt.Sprintf( "%s: %s.Flush: bare Flush() on a ResponseWriter wrapper — http.ResponseController's Flusher branch silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error instead (see gzipResponseWriter/commitWriter/cacheControlWriter), or drop flush interception entirely and let Unwrap tunnel it", fset.Position(fd.Pos()), recv)) + } else { + flushOrigins[recv] = recv } case "FlushError": - escapes[recv] = true if isWrapper { + escapes[recv] = true flushErrors++ if !isFlushErrorSignature(fd) { violations = append(violations, fmt.Sprintf( "%s: %s.FlushError: signature must be exactly `FlushError() error` — anything else is invisible to http.ResponseController's method search and never runs", fset.Position(fd.Pos()), recv)) } + } else if isFlushErrorSignature(fd) { + escapes[recv] = true } case "Unwrap": if isUnwrapSignature(fd, httpName) { @@ -265,18 +284,20 @@ func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violatio // Promotion fixpoint: an embedder with no flush machinery of its own // still escapes the dead end when an embedded type provides it — Go // promotes the embedded FlushError/Unwrap into the embedder's method - // set, where the controller's walk finds them. + // set, where the controller's walk finds them. Bare-Flush provenance + // rides the same edges: promotion puts the helper's Flush into the + // embedder's method set too, where the Flusher branch matches it. for changed := true; changed; { changed = false for name, embedded := range embeds { - if escapes[name] { - continue - } for _, e := range embedded { - if escapes[e] { + if escapes[e] && !escapes[name] { escapes[name] = true changed = true - break + } + if flushOrigins[e] != "" && flushOrigins[name] == "" { + flushOrigins[name] = flushOrigins[e] + changed = true } } } @@ -285,6 +306,12 @@ func flushConventionViolations(fset *token.FileSet, files []*ast.File) (violatio if escapes[name] { continue } + if origin := flushOrigins[name]; origin != "" { + violations = append(violations, fmt.Sprintf( + "%s: %s: bare Flush() promoted from embedded non-wrapper %s — http.ResponseController's Flusher branch matches the promoted method and silently swallows its errors, so the handler hears success on a flush that died. Implement FlushError() error on the wrapper (see gzipResponseWriter/commitWriter/cacheControlWriter), or stop embedding the flushing helper and let Unwrap tunnel", + fset.Position(pos), name, origin)) + continue + } violations = append(violations, fmt.Sprintf( "%s: %s: ResponseWriter wrapper with neither `FlushError() error` nor `Unwrap() http.ResponseWriter` — http.ResponseController's method walk dead-ends here, so every handler flush (and every other controller verb) through this layer returns http.ErrNotSupported in production. Implement FlushError() error to intercept flush (see gzipResponseWriter/commitWriter/cacheControlWriter), or declare Unwrap and let the controller tunnel", fset.Position(pos), name)) @@ -436,6 +463,39 @@ func TestFlushConventionViolations_DetectsEachBreak(t *testing.T) { "func (w *fakeWrap) FlushError() error { return nil }\n\n" + "type outerWrap struct{ *fakeWrap }\n", }, + { + // Laundered escape (#174 delta review): a non-wrapper helper's + // bare Flush, promoted into an embedding wrapper, is matched by + // the controller's Flusher branch and swallows errors exactly + // like a hand-written bare Flush — the violation lands on the + // wrapper and names the helper the method promoted from. + name: "promoted bare Flush from an embedded non-wrapper helper", + decls: "type plainBuffer struct{ n int }\n\nfunc (b *plainBuffer) Flush() {}\n\n" + + "type sneakyWrap struct {\n\thttp.ResponseWriter\n\t*plainBuffer\n}\n", + want: "bare Flush() promoted from embedded non-wrapper plainBuffer", + wantType: "sneakyWrap", + }, + { + // Laundered escape, malformed flavour (#174 delta review): a + // non-wrapper helper's FlushError under the wrong signature + // promotes a method the controller's search never matches — the + // embedding wrapper is a runtime dead end and must be flagged + // as one, not credited for the helper's dead code. + name: "promoted malformed FlushError leaves the embedder a dead end", + decls: "type leakyBuffer struct{ n int }\n\nfunc (b *leakyBuffer) FlushError() (int, error) { return 0, nil }\n\n" + + "type sneakyWrap struct {\n\thttp.ResponseWriter\n\t*leakyBuffer\n}\n", + want: "neither `FlushError() error` nor `Unwrap() http.ResponseWriter`", + wantType: "sneakyWrap", + }, + { + // The legitimate promotion: a non-wrapper helper whose + // FlushError carries the exact shape IS matched by the + // controller once promoted — exact-signature credit (the same + // discipline Unwrap always had) is what keeps this clean. + name: "promoted exact FlushError from a non-wrapper helper is clean", + decls: "type flushHelper struct{ n int }\n\nfunc (h *flushHelper) FlushError() error { return nil }\n\n" + + "type helperWrap struct {\n\thttp.ResponseWriter\n\t*flushHelper\n}\n", + }, { // Detection hole 2 (#174 review): a NAMED http.ResponseWriter // delegate plus hand-written interface methods satisfies