fix: decode Job.Signal as a signal name, not an int - #354
Conversation
The REST API returns the symbolic signal name ("SIGKILL", "SIGTERM") —
the agent sets it via process.SignalString — but go-buildkite v5.7.0
typed Job.Signal as *int. Any job killed by a signal therefore failed to
decode:
json: cannot unmarshal string into Go struct field Job.items.signal of type int
Because the SDK discards the whole response on a decode error, a single
signal-killed job took out every other job in the build. Timed-out jobs
get SIGKILL'd, so any build with a mass timeout hit this.
go-buildkite fixed the field to string in v5.8.0 (75e7abd). Bump to
v5.10.0 and follow the type through JobDetail, which mirrored the same
wrong *int.
Note JobDetail.signal changes from number to string in tool output.
The existing tests build buildkite.Job values directly through the mock,
so they never exercised JSON decoding and could not have caught this.
The added test starts from wire bytes instead, covering a signal name,
an explicit null, and an absent field.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes job listing/fetching failures when Buildkite returns a symbolic signal name (e.g., "SIGKILL") by aligning this repo and the pinned go-buildkite SDK with the API’s real wire type for signal.
Changes:
- Bump
github.com/buildkite/go-buildkite/v5fromv5.7.0tov5.10.0(includes upstream fix forJob.Signaldecoding). - Update this repo’s
JobDetail.Signaltostringand keep it as a passthrough from the SDK job model. - Add a wire-level regression test (
TestJobSignalDecodesFromWire) to ensure string/null/absent signal values decode and propagate throughlist_jobsdetailed output.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/buildkite/jobs.go | Updates JobDetail.Signal to string and passes through the SDK’s decoded signal name. |
| pkg/buildkite/jobs_test.go | Updates existing expectations and adds a wire-level JSON decoding regression test for job signals. |
| go.mod | Bumps go-buildkite/v5 dependency to v5.10.0. |
| go.sum | Updates module sums for the new go-buildkite/v5 version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
signal changes type in the public list_jobs / get_job output contract, so this needs human review; I didn't find any implementation issues to call out.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 10687, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
Job listing and job fetching fail outright for any job killed by a signal. The REST API returns the symbolic signal name — the agent sets it via
process.SignalString— butgo-buildkitev5.7.0, which we pin, typedJob.Signalas*int:The SDK discards the entire response on a decode error, so one signal-killed job takes out every other job in the build. Timed-out jobs get SIGKILL'd, which means any build with a mass timeout — a 90-shard suite hitting its limit, say — returns nothing at all rather than 89 usable jobs.
go-buildkite/v5v5.7.0 → v5.10.0; the field was corrected tostringupstream in v5.8.0 (buildkite/go-buildkite#350)JobDetail.Signal, which mirrored the same wrong*intand passed it straight through fromdetailJobTestJobSignalDecodesFromWire, covering a signal name, an explicitnull, and an absent fieldBehaviour change
JobDetail.signalinlist_jobs/get_joboutput changes from a number to a string —9becomes"SIGKILL". This is the field's real wire type; it has never successfully returned a number, since any response that would have populated it failed to decode first. Nothing downstream can be relying on the old shape.Why our tests missed this
Every existing job test constructs
buildkite.Jobvalues directly throughMockJobsClient, so none of them exercise JSON decoding — the mock hands back an already-built struct and the wire format is never parsed. A*intfield looks fine under that setup right up until it meets a real API response.The added test starts from wire bytes instead. Worth noting it only closes the hole for this one field: the same blind spot covers the whole mocked tool surface, so wire-type drift in builds, artifacts, or annotations would land exactly the same way. Broader wire-level fixtures are worth doing separately.
Verification
Signalone; thebackoff→rokodependency swap didn't movego.sumgo build ./...clean,golangci-lint run ./...0 issues, full suite greenDisclosures/Credits