Skip to content

WI-004: #17 Missing response helpers: Accepted, NoContent - #22

Merged
rluders merged 3 commits into
mainfrom
weave/wi-004-17-missing-response-helpers-accepted-noc
Oct 23, 2026
Merged

rluders merged 3 commits into
mainfrom
weave/wi-004-17-missing-response-helpers-accepted-noc

Conversation

@rluders

@rluders rluders commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Work item WI-004 of work package WP-001 (GitHub issues).

`response_helpers.go` has `OK`, `OKWithMeta`, and `Created`, but no `Accepted` (202) or `NoContent` (204) — both common for async-job-style and delete/update-with-empty-body endpoints. `ReplyBuilder` (the fluent path) has the same gap: `OK`/`Created` methods exist, no `Accepted`/`NoContent`.

Current workaround is the generic `Respond(data).Status(http.StatusAccepted).Write(w)` / `Respondany.Status(http.StatusNoContent).Write(w)`, which works but is exactly the "low-level escape hatch" the direct helpers exist to avoid.

Requested:
```go
func Accepted[T any](w http.ResponseWriter, data T)
func NoContent(w http.ResponseWriter)
```
plus `ReplyBuilder.Accepted`/`ReplyBuilder.NoContent` for the header/meta-configured path.

Implemented by the Weave Autoloop, attempt 1, execution exe_b61112dcc128.

Changed files:

  • response_builder.go
  • response_helpers.go
  • response_helpers_test.go

Implemented by the Weave Autoloop, attempt 1 (execution exe_b61112dcc128).
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ 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: 380a1c52-8b8e-4021-8cf3-cd073ab5d870
📥 Commits

Reviewing files that changed from the base of the PR and between 36e6b59 and 7afcc31.

📒 Files selected for processing: 4
  • README.md
  • response_builder.go
  • response_helpers.go
  • response_helpers_test.go
  • 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

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

@codecov-commenter

codecov-commenter commented Oct 12, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.71%. Comparing base (36e6b59) to head (7afcc31).
⚠️ Report is 2 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   85.48%   85.71%   +0.22%     
==========================================
  Files          14       14              
  Lines         565      574       +9     
==========================================
+ Hits          483      492       +9     
  Misses         63       63              
  Partials       19       19              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rluders

rluders commented Oct 12, 2026

Copy link
Copy Markdown
Owner Author

Weave review: approved (execution exe_a9d7dbfdcaaf)

  • Reviewer by agent go-reviewer: approved — The change meets all four acceptance criteria and each one has a test. go vet ./... and go test -count=1 ./... both exited 0 (report.json:4-12). I didn't run them myself, so that rests on the report.
Criterion Implementation Test
Accepted[T any](w, data) helper (202) response_helpers.go:20-23, delegating to Reply().Accepted response_helpers_test.go:39-48 checks the 202 status and a non-empty body
NoContent(w) helper (204) response_helpers.go:25-28 response_helpers_test.go:50-59 checks the 204 status and an empty body
ReplyBuilder.Accepted with header and meta response_builder.go:82-85 goes through Respond(...).Status(202).Meta().Headers().Write, so it matches how Created is built response_helpers_test.go:63-66 sets a header and meta and checks the 202 status and the header
ReplyBuilder.NoContent with headers response_builder.go:89-92 applies headers with the existing applyHeaders (response_write.go:91), then calls WriteHeader(204) response_helpers_test.go:69-78 checks the 204 status, the header, an empty body and no Content-Type
  • Minimal: the change adds only the four functions and their tests.
  • Idiomatic: the new code follows the style of the existing helpers, and every public function has a doc comment.
  • 204 handling: NoContent skips the JSON writer, so there's no body or Content-Type. Meta is ignored, and the doc comment says so.

Two small gaps, neither of which blocks approval:

  • The 202 tests don't assert the Content-Type or the meta in the body.
  • NoContent on the direct helper doesn't check for a stray Content-Type, though the builder test does.
  • Person: approved — Operator review (Orkestra dogfood operator; NOT a GitHub review): diff read. Accepted/NoContent helpers and ReplyBuilder methods follow the existing Created pattern; NoContent writes headers and 204 with no body or Content-Type. Tests cover all four. CI observed by Orkestra (ci_passed at e5087ef). Ready for a human merge decision.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FHBdej3KvFACHzafbEhw7C
@rluders
rluders marked this pull request as ready for review October 23, 2026 22:13
@rluders
rluders merged commit bc30568 into main Oct 23, 2026
8 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.

3 participants