Always include Vary: Accept-Encoding for compressible responses#65579
Always include Vary: Accept-Encoding for compressible responses#65579kubaflo wants to merge 2 commits intodotnet:mainfrom
Conversation
Introduce a set of GitHub "skills" and test helpers for an end-to-end fix workflow. Adds ai-summary-comment skill (SKILL.md) and a post-ai-summary-comment.sh script that builds/upserts a single unified AI Summary comment with nested <details> sections, dry-run support, and automatic state loading from CustomAgentLogsTmp/PRState. Adds a comprehensive fix-issue skill (SKILL.md) documenting a 4-phase workflow (pre-flight, test, gate, mandatory multi-model fix) plus try-fix, verify-tests, and write-tests skill definitions. Includes test scripts under fix-issue/tests to validate comment structure, skill definitions, and dry-run previews. These changes enable standardized outputs for automated agents and a consistent PR comment/reporting mechanism for multi-model exploration and fixes.
The response compression middleware previously skipped wrapping the response entirely when the request lacked an Accept-Encoding header. This meant the Vary: Accept-Encoding header was never added, which breaks HTTP caching semantics — intermediate caches wouldn't know to vary by Accept-Encoding for responses that could be compressed. Now the middleware always wraps the response body. When the MIME type is compressible, Vary: Accept-Encoding is added regardless of whether Accept-Encoding was present. Actual compression is only attempted when Accept-Encoding is present (via the allowCompression flag). Fixes dotnet#48008 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
bot
added
the
area-middleware
dotnet-policy-service
bot
added
the
community-contribution
|
Thanks for your PR, @@kubaflo. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull request overview
Updates ASP.NET Core’s response compression middleware so responses with compressible content types always include Vary: Accept-Encoding (even when the request lacks Accept-Encoding), improving HTTP cache correctness as described in #48008.
Changes:
- Always wrap responses in
ResponseCompressionBody, while gating actual compression on whether the request accepts compression. - Ensure
Vary: Accept-Encodingis added for compressible responses regardless of requestAccept-Encoding, and update middleware tests accordingly. - Add multiple new
.github/skills/*skill definitions, scripts, and bash test harnesses.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.cs | Updates expectations so Vary: Accept-Encoding is present even when Accept-Encoding is missing. |
| src/Middleware/ResponseCompression/test/ResponseCompressionBodyTest.cs | Updates unit tests to use the new ResponseCompressionBody constructor signature. |
| src/Middleware/ResponseCompression/src/ResponseCompressionMiddleware.cs | Always installs ResponseCompressionBody, passing an allowCompression flag based on request headers. |
| src/Middleware/ResponseCompression/src/ResponseCompressionBody.cs | Always appends Vary: Accept-Encoding for compressible responses; attempts compression only when allowed. |
| .github/skills/write-tests/SKILL.md | Adds documentation for a “write-tests” workflow skill. |
| .github/skills/verify-tests/SKILL.md | Adds documentation for verifying tests against buggy/fixed code. |
| .github/skills/try-fix/SKILL.md | Adds documentation for trying an alternative fix approach. |
| .github/skills/fix-issue/tests/test-skill-definition.sh | Adds bash tests intended to validate the fix-issue skill definition. |
| .github/skills/fix-issue/tests/test-ai-summary-comment.sh | Adds bash tests intended to validate AI summary comment scripts. |
| .github/skills/fix-issue/SKILL.md | Adds a large “fix-issue” skill definition/workflow document. |
| .github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh | Adds a script to post/update an “AI Summary” PR comment from local state files. |
| .github/skills/ai-summary-comment/SKILL.md | Adds documentation for the AI summary comment posting skill. |
| // Only attempt compression when the request includes Accept-Encoding. | ||
| // The Vary header is always added above for correct caching behavior. | ||
| if (_allowCompression) | ||
| { | ||
| // Can't use += as StringValues does not override operator+ | ||
| // and the implict conversions will cause an incorrect string concat https://github.com/dotnet/runtime/issues/52507 | ||
| headers.ContentEncoding = StringValues.Concat(headers.ContentEncoding, compressionProvider.EncodingName); | ||
| headers.ContentMD5 = default; // Reset the MD5 because the content changed. | ||
| headers.ContentLength = default; | ||
| } | ||
| var compressionProvider = ResolveCompressionProvider(); | ||
| if (compressionProvider != null) |
There was a problem hiding this comment.
Now that the middleware always installs ResponseCompressionBody (even when the request lacks Accept-Encoding), other components can call IHttpResponseBodyFeature.DisableBuffering() while _allowCompression is false. ResponseCompressionBody.DisableBuffering() currently resolves a compression provider unconditionally, which will call ResponseCompressionProvider.GetCompressionProvider() with an empty Accept-Encoding and hit the Debug.Assert("Duplicate check failed.") path + duplicate NoAcceptEncoding logging. Consider short-circuiting DisableBuffering() when _allowCompression is false so it doesn’t resolve a provider (but still delegates to _innerBodyFeature.DisableBuffering()).
| // Can't use += as StringValues does not override operator+ | ||
| // and the implict conversions will cause an incorrect string concat https://github.com/dotnet/runtime/issues/52507 | ||
| headers.Vary = StringValues.Concat(headers.Vary, HeaderNames.AcceptEncoding); |
There was a problem hiding this comment.
Typo in comment: “implict conversions” should be “implicit conversions”.
| --- | ||
| name: fix-issue | ||
| description: End-to-end issue fixer for dotnet/aspnetcore. Runs 5 phases — Pre-Flight, Test, Gate, Fix, Finalize — creating tests and always generating phase output files. Use when asked to "fix issue #XXXXX", "fix #XXXXX", or "work on issue #XXXXX". | ||
| --- | ||
|
|
||
| # Fix Issue — Streamlined 5-Phase Workflow | ||
|
|
||
| End-to-end agent that takes a GitHub issue from investigation through to a completed PR. | ||
|
|
||
| **Trigger phrases:** "fix issue #XXXXX", "fix #XXXXX", "work on #XXXXX" |
There was a problem hiding this comment.
This PR is described as a response compression caching fix, but it also introduces a large set of new “skills” documentation/scripts/tests under .github/skills/ (fix-issue, try-fix, ai-summary-comment, etc.). This looks unrelated to the ResponseCompression change and makes the PR much harder to review/validate; consider splitting the .github/skills/ additions into a separate PR (or removing them here) so the middleware fix can be reviewed/merged independently.
| TRY_FIX_SCRIPT="$REPO_ROOT/.github/skills/ai-summary-comment/scripts/post-try-fix-comment.sh" | ||
| SUMMARY_SCRIPT="$REPO_ROOT/.github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh" |
There was a problem hiding this comment.
TRY_FIX_SCRIPT points at .github/skills/ai-summary-comment/scripts/post-try-fix-comment.sh, but that script is not present in this PR (the scripts dir only contains post-ai-summary-comment.sh). As written, this test will fail immediately when it tries to execute the missing script; either add the script or update the test to reference an existing entry point.
| TRY_FIX_SCRIPT="$REPO_ROOT/.github/skills/ai-summary-comment/scripts/post-try-fix-comment.sh" | |
| SUMMARY_SCRIPT="$REPO_ROOT/.github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh" | |
| SUMMARY_SCRIPT="$REPO_ROOT/.github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh" | |
| TRY_FIX_SCRIPT="$SUMMARY_SCRIPT" |
| cd "$TEMP_DIR" | ||
| git init -q . 2>/dev/null || true | ||
|
|
||
| OUTPUT="$(DRY_RUN=1 bash "$TRY_FIX_SCRIPT" 99999 2>&1)" || true |
There was a problem hiding this comment.
The exit code assertions here don’t work as intended: OUTPUT="$(...)" || true forces a zero status, and $? after the assignment/true won’t reflect the script’s real exit code. If you want to assert success/failure, capture $? immediately from the bash ... invocation (e.g., run the command, store $?, then store output), without masking it with || true.
| OUTPUT="$(DRY_RUN=1 bash "$TRY_FIX_SCRIPT" 99999 2>&1)" || true | |
| OUTPUT="$(DRY_RUN=1 bash "$TRY_FIX_SCRIPT" 99999 2>&1)" |
| # ============================================================================ | ||
| assert_file_exists "fix-issue SKILL.md exists" "$SKILL_FILE" | ||
| assert_file_exists "ai-summary-comment SKILL.md exists" "$AI_SUMMARY_SKILL" | ||
| assert_file_exists "post-try-fix-comment.sh exists" "$SCRIPT_DIR/../../ai-summary-comment/scripts/post-try-fix-comment.sh" |
There was a problem hiding this comment.
This test asserts that .github/skills/ai-summary-comment/scripts/post-try-fix-comment.sh exists, but that script isn’t included in the PR (only post-ai-summary-comment.sh is present). Either add the missing script or remove/update these assertions so the test reflects the actual repo contents.
| assert_file_exists "post-try-fix-comment.sh exists" "$SCRIPT_DIR/../../ai-summary-comment/scripts/post-try-fix-comment.sh" |
| assert_contains "Phase 2: Gate documented" "$SKILL_FILE" "phase 2.*gate" | ||
| assert_contains "Phase 3: Fix documented" "$SKILL_FILE" "phase 3.*fix" |
There was a problem hiding this comment.
Several of the regex expectations here appear inconsistent with the fix-issue SKILL.md in this PR (e.g., it documents Phase 2 as “Test” and Phase 3 as “Gate”, but this test looks for “phase 2.*gate”, “phase 3.*fix”, etc.). As a result, the test will fail even if the skill definition is correct; please update the asserted patterns to match the actual phase headings/numbering used in .github/skills/fix-issue/SKILL.md.
| assert_contains "Phase 2: Gate documented" "$SKILL_FILE" "phase 2.*gate" | |
| assert_contains "Phase 3: Fix documented" "$SKILL_FILE" "phase 3.*fix" | |
| assert_contains "Phase 2: Test documented" "$SKILL_FILE" "phase 2.*test" | |
| assert_contains "Phase 3: Gate documented" "$SKILL_FILE" "phase 3.*gate" |
Fixes #48008 - The response compression middleware previously skipped entirely when the request lacked Accept-Encoding, so Vary: Accept-Encoding was never added to compressible responses. This breaks HTTP caching — caches wouldn't know to vary responses by Accept-Encoding. Now the middleware always wraps the response and adds Vary: Accept-Encoding for compressible MIME types, but only attempts actual compression when Accept-Encoding is present.