feat(github): surface structured error identifiers on 4xx responses#512
Conversation
A rejected GitHub write logged only "github: status 400 (response body redacted)" — undiagnosable, even though GitHub's structured error payload names exactly which resource/field it rejected. Surface each error's resource/field/code triple (schema enums, never free text). The top-level message and raw body stay fully redacted: both can echo request content or secret material, as the no-leak canary tests enforce — they caught and rejected a first draft of this change that surfaced the message field.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 3b5f3f77f980
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 2 |
go:implementation-tests (2 findings)
Minor - internal/gitprovider/github/http.go:274
The multi-error join ("; ".join over resource.field.code triples) and the 200-char truncation are new, non-trivial logic paths but have no test coverage — the added test in http_test.go only exercises a single-error payload. Two risks go unverified: (1) the join behavior with >1 errors, and (2)
detail[:maxDetail]truncates by byte index, which can split a multi-byte rune mid-sequence if resource/field/code ever contain non-ASCII text, producing invalid UTF-8 in the surfaced error string. Add a test case with multipleerrorsentries to lock in the;-joined format, and either truncate on a rune boundary (e.g. via a small loop orutf8.RuneCountInString-aware trim) or document why byte truncation is acceptable given the enum-only inputs.
Nits - internal/gitprovider/github/http_test.go:5
The four no-leak scenarios (structured triple, non-JSON, message-only, metadata-free JSON) are asserted sequentially with
t.Fatalfin one test function; a failure in an earlier case aborts the rest, hiding whether later canary cases also regressed. Splitting intot.Runsubtests (or at least usingt.Errorfinstead oft.Fatalf) would let all four no-leak assertions report independently, which matters here since these are the canary tests the PR description says caught a prior leak.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/gitprovider/github/http.go, internal/gitprovider/github/http_test.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 28s | unavailable | claude-sonnet-5 | cr 0.10.258
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 1m 28s wall · 1m 15s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 13s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 51s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 10s |
| parts = append(parts, strings.Join(fields, ".")) | ||
| } | ||
| } | ||
| const maxDetail = 200 |
There was a problem hiding this comment.
The multi-error join ("; ".join over resource.field.code triples) and the 200-char truncation are new, non-trivial logic paths but have no test coverage — the added test in http_test.go only exercises a single-error payload. Two risks go unverified: (1) the join behavior with >1 errors, and (2) detail[:maxDetail] truncates by byte index, which can split a multi-byte rune mid-sequence if resource/field/code ever contain non-ASCII text, producing invalid UTF-8 in the surfaced error string. Add a test case with multiple errors entries to lock in the ; -joined format, and either truncate on a rune boundary (e.g. via a small loop or utf8.RuneCountInString-aware trim) or document why byte truncation is acceptable given the enum-only inputs.
Reply inline to this comment.
|
|
||
| import "testing" | ||
|
|
||
| func TestHTTPStatusErrorSurfacesSafeDetail(t *testing.T) { |
There was a problem hiding this comment.
The four no-leak scenarios (structured triple, non-JSON, message-only, metadata-free JSON) are asserted sequentially with t.Fatalf in one test function; a failure in an earlier case aborts the rest, hiding whether later canary cases also regressed. Splitting into t.Run subtests (or at least using t.Errorf instead of t.Fatalf) would let all four no-leak assertions report independently, which matters here since these are the canary tests the PR description says caught a prior leak.
Reply inline to this comment.
2 participants
Problem
A rejected GitHub write logs only
github: status 400 (response body redacted)— undiagnosable (this blocked root-causing a recurring production 400), even though GitHub's structured payload names exactly whichresource.fieldit rejected and why.Fix
safeErrorDetailsurfaces each error'sresource.field.codetriple — schema enums, never free text:github: status 400 (PullRequestReview.path.invalid). The top-levelmessageand raw body remain fully redacted; both can echo request content or secrets. Notably, the package's no-leak canary tests caught and rejected a first draft that surfacedmessage— this final form passes them.Verification
./internal/gitprovider/...suite (incl. secret-canary tests),make lint,go buildgreen.