fix(proxy): default a nil HTTPClient instead of dereferencing it

coilyco-ops commented

2026-08-22 22:39:35 +00:00

Member

Milestone 17 phase 5.

ProxyClient is exported with exported fields, so a caller can build one directly. Every other field treats zero as take the packaged default, and HTTPClient treated it as a nil dereference on the first turn. httpClient() is the same shape as the existing now(), which is what the issue asked for.

The runtime never reaches this, NewAgent always supplying an instrumented client, so it is only reachable by a second caller. That is how it was found, wiring cmd/sirens-echo-bridge.

TestASuppliedHTTPClientIsStillUsed holds the other half, so the default cannot quietly displace the otelhttp transport NewAgent depends on.

With the accessor removed the test reproduces the reported panic exactly:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0]

closes #959

Milestone 17 phase 5. `ProxyClient` is exported with exported fields, so a caller can build one directly. **Every other field treats zero as take the packaged default**, and `HTTPClient` treated it as a nil dereference on the first turn. `httpClient()` is the same shape as the existing `now()`, which is what the issue asked for. The runtime never reaches this, `NewAgent` always supplying an instrumented client, so it is only reachable by a second caller. That is how it was found, wiring `cmd/sirens-echo-bridge`. `TestASuppliedHTTPClientIsStillUsed` holds the other half, so the default cannot quietly displace the otelhttp transport `NewAgent` depends on. With the accessor removed the test reproduces the reported panic exactly: ``` panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0] ``` closes #959

coilyco-ops added this to the Headless engineer lane: the queue wait first milestone

2026-08-22 22:39:35 +00:00

coilyco-ops added 1 commit

2026-08-22 22:39:35 +00:00

fix(proxy): default a nil HTTPClient instead of dereferencing it

ci / test (pull_request) Successful in 58s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 35s

Details

75ab01d246

ProxyClient is exported with exported fields, so a caller can build one
directly. Every other field treats zero as take the packaged default, and
this one treated it as a nil dereference on the first turn.

The runtime never reaches it, because NewAgent always supplies an
instrumented client, so it is only reachable by a second caller. That is
what found it: wiring cmd/sirens-echo-bridge against the proxy.

httpClient() is the same shape as the existing now(), which is what the
issue asked for. A supplied client is still used, so nothing quietly
drops the otelhttp transport NewAgent depends on.

The test reproduces the reported panic exactly when the accessor is
removed, rather than only asserting the fixed behaviour.

closes #959

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: engineer

coilyco-ops referenced this pull request

2026-08-22 22:42:52 +00:00

fix(scratch): bound a read at the file, not after it has crossed #1114

coilyco-ops referenced this pull request

2026-08-22 22:45:36 +00:00

fix(scratch): bound a read at the file, not after it has crossed #1114

coilyco-ops referenced this pull request

2026-08-22 22:47:12 +00:00

main is red on publish-echo-image, and the change that landed with it is proven not to be the cause #1117

coilyco-ops referenced this pull request

2026-08-22 22:48:12 +00:00

feat(image): a check that reads a definition against the tree it ships with #1116

coilyco-ops commented

2026-08-22 22:49:18 +00:00

Author

Member

Reviewed, correct, and the zero-value reasoning in the comment is the right justification: every other field on this struct means the packaged default when unset, so a nil client meaning a panic was the odd one out.

One thing I checked rather than assumed, because it would have made this worse than the panic. http.DefaultClient has no timeout, so defaulting to it on a path that calls a model could have turned a loud crash into a silent hang, which is the trade nobody wants. It does not, because proxy.go:1120 builds the request with http.NewRequestWithContext(modelCtx, ...) and modelCtx carries the turn budget. The deadline lives on the request rather than the client, so the default is bounded by the same budget an explicit client would be.

Worth a half-sentence in the httpClient comment saying the deadline comes from the request context, since the next reader's first instinct will be the same as mine was.

No file overlap with #1114 or #1116, and this one touches nothing in the image, so it is unaffected by #1117.

Reviewed, correct, and the zero-value reasoning in the comment is the right justification: every other field on this struct means the packaged default when unset, so a nil client meaning a panic was the odd one out. One thing I checked rather than assumed, because it would have made this worse than the panic. **`http.DefaultClient` has no timeout**, so defaulting to it on a path that calls a model could have turned a loud crash into a silent hang, which is the trade nobody wants. It does not, because `proxy.go:1120` builds the request with `http.NewRequestWithContext(modelCtx, ...)` and `modelCtx` carries the turn budget. **The deadline lives on the request rather than the client**, so the default is bounded by the same budget an explicit client would be. Worth a half-sentence in the `httpClient` comment saying the deadline comes from the request context, since the next reader's first instinct will be the same as mine was. No file overlap with #1114 or #1116, and this one touches nothing in the image, so it is unaffected by #1117.

coilyco-ops referenced this pull request

2026-08-22 22:51:38 +00:00

main is red on publish-echo-image, and the change that landed with it is proven not to be the cause #1117

coilyco-ops added 1 commit

2026-08-22 22:51:51 +00:00

Merge branch 'main' into aos/claude/proxy-nil-client-959

ci / test (pull_request) Successful in 1m36s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details