[hnakamur]

what

• Always update Content-Length when the test has already Content-Length.
• Add Content-Length only when the length is not zero if the test does not have Content-Length.

why

• Previously Content-Length: 0 are added to requests whose method is GET or HEAD, but they should not be added.

references

• Previous modification: 37e1f133d

Summary by CodeRabbit

• Bug Fixes
  • Empty responses no longer receive an automatically added Content-Length header when one was not already provided. Existing length headers continue to be updated, and chunked responses remain unchanged. This keeps headers consistent with empty response bodies while preserving existing behavior for responses with a specified length or chunked transfer.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4d1ca503-242c-4038-ab61-79dcdf36c253

Commits

Reviewing files that changed from the base of the PR and between a44c779 and 00738f5.

Files selected for processing: 1

• test/regression/regression_test.cc

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

Changes

Content-Length Handling

Layer / File(s)	Summary
Zero-length header check test/regression/regression_test.cc	The test adds a missing Content-Length header only when the body length is nonzero. Existing header updates and chunked-transfer behavior remain unchanged.

Priority: Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other

Suggested reviewers: airween

Merge Risk: Minimal · up to 00738

No actionable merge-blocking risk remains in this test-only change.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main behavior change: Content-Length is added only when the length is nonzero. The raw summary confirms that update_content_length no longer adds a Content-Length header for a zero…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main behavior change: Content-Length is added only when the length is nonzero. The raw summary confirms that update_content_length no longer adds a Content-Length header for a zero-length body when none exists, and the objectives say this stops Content-Length: 0 on GET and HEAD requests. The title is specific and matches the change, though it does not mention that existing headers are still updated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[airween]

Thanks @hnakamur, this looks good to me.

Not adding Content-Length: 0 to requests and responses that have no body and no Content-Length header (GET, HEAD, empty responses) is the right call. Existing Content-Length headers are still rewritten as before, chunked messages are still left alone, and since this only affects the opt-in UPDATE_CONTENT_LENGTH=1 ... format path, the normal test runs are unaffected. Approving.

An idea for a follow-up, if you're interested:

The format + UPDATE_CONTENT_LENGTH feature (from 37e1f13) rewrites every Content-Length to the real body length. That is very convenient in general, but some test cases may mismatch on purpose, e.g. to check how the engine behaves when the Content-Length header and the actual body length (REQUEST_BODY) differ. After a bulk reformat these mismatches are silently "fixed", the tests lose their meaning, and the change is easy to miss in a large diff.

It would be nice to mark these cases explicitly in the test JSON, for example with a per-test field like:

"content_length_mismatch": true

and use it in two places:

1. format with UPDATE_CONTENT_LENGTH: skip the tests that carry the marker. For unmarked tests with a mismatch, print a warning listing the file and test number (instead of fixing them silently, or in addition to fixing them), so the author can decide whether the mismatch is intentional.
2. Normal regression_tests runs: print a warning (not a failure) when the Content-Length header doesn't match the body length and the test isn't marked. This way accidental mismatches surface over time, intentional ones stay quiet, and the existing suite doesn't turn red all at once.

The name of the field is of course open for discussion.

For reference, the v2 regression test runner (tests/run-regression-tests.pl.in on v2/master) has a similar distinction, though an implicit one: a request given as an HTTP::Request goes through LWP, which notices a Content-Length that doesn't match the body. A request given as a raw string is written to the socket as-is, without any checks. So in v2 "deliberately malformed" is expressed by the request format rather than by an explicit flag. An explicit marker would be clearer, and it would be nice to have the same convention on both sides eventually. On the v3 side it would be a small, self-contained change.

Would you be interested in taking this on in a separate PR?

Thank you again!

[hnakamur]

@airween Thank you for your comment and proposal for adding content_length_mismatch.
I will make a seperate PR for it.