Skip to content

Fix chi task56 combined.patch: Allow header duplication bug#38

Merged
akhatua2 merged 1 commit intocooperbench:mainfrom
AlienKevin:fix/chi-t56-combined-patch
Mar 12, 2026
Merged

Fix chi task56 combined.patch: Allow header duplication bug#38
akhatua2 merged 1 commit intocooperbench:mainfrom
AlienKevin:fix/chi-t56-combined-patch

Conversation

@AlienKevin
Copy link
Contributor

@AlienKevin AlienKevin commented Mar 11, 2026

Summary

The combined.patch for go_chi_task/task56 has two bugs that cause TestMethodNotAllowed and TestSetAllowHeader to always fail, making the oracle solution score 0 for any feature pair involving feature 1.

Bug 1: Middleware reuses routing context across Match() calls

The SetAllowHeader middleware calls rctx.Routes.Match(rctx, method, url) in a loop for each HTTP method, reusing the same rctx. Each Match() call triggers findRoute(), which appends to rctx.methodsAllowed without resetting. After 9 iterations, methodsAllowed contains many duplicate [GET, HEAD] entries, causing the Allow header to contain values like:

```
GET, HEAD, GET, HEAD, GET, HEAD, HEAD, GET, GET, HEAD, GET, HEAD, GET, HEAD, HEAD, GET, HEAD, GET
```

Fix: Use chi.NewRouteContext() for each Match() call to prevent cross-call state pollution.

Bug 2: Core handler uses Set() instead of Add() for Allow header

The MethodNotAllowedHandler builds a comma-joined string and calls w.Header().Set("Allow", "GET, HEAD"), creating a single header value. The tests expect separate values via Header.Values("Allow") — i.e., ["GET", "HEAD"] (2 elements) not ["GET, HEAD"] (1 element).

Fix: Use w.Header().Add("Allow", name) per method to emit individual header values matching the test expectations and RFC conventions.

Verification

Built the chi:task56 Docker image locally from the Dockerfile and ran all tests.

Buggy (original) combined.patch — feature 1 tests fail:
```
--- FAIL: TestMethodNotAllowed/Unregistered_Method (0.00s)
mux_test.go:426: Allow header should contain 2 headers: GET, HEAD. Received: [GET, HEAD]
--- FAIL: TestSetAllowHeader/Unregistered_Method (0.00s)
allowed_methods_test.go:58: GET, HEAD, GET, HEAD, GET, HEAD, GET, HEAD, GET, HEAD, HEAD, GET, GET, HEAD, GET, HEAD, GET, HEAD
```

Fixed combined.patch — all feature tests pass:
```
Feature 1: ok github.com/go-chi/chi/v5, ok github.com/go-chi/chi/v5/middleware
Feature 2: ok github.com/go-chi/chi/v5, ok github.com/go-chi/chi/v5/middleware
Feature 3: ok github.com/go-chi/chi/v5, ok github.com/go-chi/chi/v5/middleware
Feature 4: ok github.com/go-chi/chi/v5, ok github.com/go-chi/chi/v5/middleware
Feature 5: ok github.com/go-chi/chi/v5, ok github.com/go-chi/chi/v5/middleware
```

Test plan

  • Verify `TestMethodNotAllowed` passes with the fixed combined.patch
  • Verify `TestSetAllowHeader` passes with the fixed combined.patch
  • Verify existing passing feature pairs for task56 are unaffected (all 5/5 pass)

The combined.patch for go-chi task56 has two bugs that cause
TestMethodNotAllowed and TestSetAllowHeader to fail:

1. The SetAllowHeader middleware reuses the same routing context
   (rctx) for all Match() calls in the method-checking loop. Each
   call to Match() triggers findRoute(), which appends to
   rctx.methodsAllowed without resetting. This causes the Allow
   header to contain duplicated method values (e.g., "GET, HEAD"
   repeated 9+ times instead of appearing once).

   Fix: Use chi.NewRouteContext() for each Match() call to prevent
   cross-call state pollution.

2. The core MethodNotAllowedHandler builds a comma-joined string
   and uses w.Header().Set("Allow", joinedStr), creating a single
   header value "GET, HEAD". But the tests expect separate header
   values via Header.Values("Allow") (["GET", "HEAD"]).

   Fix: Use w.Header().Add("Allow", name) per method to emit
   individual header values matching the test expectations.
@AlienKevin AlienKevin marked this pull request as draft March 12, 2026 00:43
@AlienKevin AlienKevin marked this pull request as ready for review March 12, 2026 04:12
@akhatua2
Copy link
Collaborator

Thanks Kevin! This looks great to me.

@akhatua2 akhatua2 merged commit 62e6a97 into cooperbench:main Mar 12, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants