🐛 fix(edge): post-review cleanup — slog test race, doc truth-ups, pollInterval tolerance#50
Merged
Merged
Conversation
TestConnectHelloRejectedByController called slog.SetDefault() while marked t.Parallel(), racing the process-global logger against every other parallel test in the package (reproduced live: DATA RACE on the shared bytes.Buffer under `go test -race`, intermittently failing CI). Drop t.Parallel() for this one test instead of widening a sleep: Go only runs parallel-marked tests concurrently with each other, so a sequential test never overlaps with another test's execution and can safely swap the global logger for the duration of its run.
…unparseable If the controller's TypeError envelope failed to json.Unmarshal into ErrorMessage, the code silently skipped slog.Warn and returned "controller rejected hello: ()" with zero diagnostic information. Now it logs unconditionally, including the raw env.Data, on unmarshal failure. Retry/backoff semantics are unchanged.
Drydock's Edge Mode welcome frame sends pollInterval as a JSON number (confirmed against app/api/portwing-ws.ts), and WelcomeMessage.PollInterval correctly decodes that. But the ecosystem documents a numeric-string shape too (Drydock's REST AgentInfo surface), and today a string value would fail the whole WelcomeMessage unmarshal — silently dropping the serverCompatLevel mismatch warning along with the poll interval. Add a custom UnmarshalJSON that accepts either a JSON number or a numeric string for pollInterval, keeping the real numeric frame working exactly as before (this is NOT `,string`, which would break the numeric case). - 🧪 test(protocol): regression tests for numeric, numeric-string, missing, and invalid pollInterval shapes, plus a round-trip test
…e shape
- 🐛 fix: SPEC.md §3.3 was missing the dd:container_delete_request /
dd:container_delete_response row, present in code/CHANGELOG/the .mdx
mirror but not the canonical spec table. Add it matching the
DDContainerDeleteRequestMessage/DDContainerDeleteResponseMessage structs
in internal/protocol/messages.go.
- 🐛 fix: "a 404 returns and the agent loops reconnect failures
indefinitely" was wrong — a 404 on the WebSocket upgrade is wrapped in
errFatal (internal/edge/client.go) and Run() returns immediately, so
the agent process exits with code 1 on the first attempt
(TestConnectFatal404). Corrected in drydock-integration.mdx,
drydock-integration.md, and authentication.mdx.
- 🐛 fix: the welcome frame's pollInterval was documented as a bare
integer string ("300"). Verified against drydock's
app/api/portwing-ws.ts:679-683 — the welcome frame actually sends a
JSON number. The string shape belongs to Drydock's REST AgentInfo
surface, not the welcome frame. Corrected in both docs.
… env exposure - 🐛 fix: README marketed inspect_container's "values never exposed" as if it covered the whole product; it only applies to that MCP tool. Reword to scope the claim correctly and note that GET /api/containers (and the edge WebSocket container sync) return full env var values by design — redaction is out of scope for Portwing, it's the downstream consumer's (Drydock's) job. - 📝 add matching notes to docs/security-model.md and docs/content/docs/security-model.mdx (kept in lockstep per policy), flagging this as most relevant to standalone / no-Sockguard deployments where the caller/consumer trust boundary is wider. No code behavior changes — this is documentation accuracy only.
The portwing-with-compose.yaml row omitted POST /networks/*/connect, the rule the preset's own header flags as genuinely new vs portwing.yaml. Add it to that row's added-rules list.
biggest-littlest
approved these changes
Jul 4, 2026
biggest-littlest
left a comment
biggest-littlest
left a comment
Member
There was a problem hiding this comment.
Post-review cleanup: slog test-race fix verified under -race -count=20 -shuffle, plus doc truth-ups and the pollInterval tolerant decoder. LGTM.
ALARGECOMPANY
approved these changes
Jul 4, 2026
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
Member
There was a problem hiding this comment.
Confirmed: the race fix and the doc corrections (404 behavior, pollInterval wire shape, Env exposure scoping, COMPATIBILITY connect rule) all check out. Approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-review cleanup from the tri-repo correctness/completeness sweep. One real test-race fix, one small robustness fix, one protocol-tolerance addition, and a batch of doc truth-ups where the docs had drifted from the code. No behavior change to the edge wire beyond tolerating one extra input shape.
Fix: data race in a parallel test
internal/edge/wire_contract_test.go'sTestConnectHelloRejectedByControllercalledslog.SetDefault()while markedt.Parallel()— a real race on the global logger that intermittently fails CIgo test -race. Reworked it to not mutate global slog under parallelism. Verified with-race -count=20 -shuffle=ononinternal/edge+internal/protocol(clean).Fix: silent hello-rejection when the error payload is unparseable
internal/edge/client.goproducedcontroller rejected hello: ()with no log when the controller's error envelope failed to unmarshal. Now logs unconditionally with the raw payload.Add: tolerate a numeric-string pollInterval in the welcome frame
The welcome frame carries a numeric
pollInterval(drydock sends300), and that stays the canonical shape. Added a tolerant decoder that also accepts a numeric string, since the integration docs describe three different pollInterval shapes across the REST + WS surfaces. Numeric decoding is unchanged — no,stringtag (that would break the real numeric frame). Covered byinternal/protocol/welcome_test.go.Docs truth-ups (code was right, docs were stale)
drydock-integration.mdx,drydock-integration.md,authentication.mdxclaimed a 404 from/api/portwing/wsmakes the agent "loop reconnect failures indefinitely." The code treats 404 as fatal → immediate exit (TestConnectFatal404). Corrected all three."300"; it's numeric300(the string shape is drydock's RESTAgentInfoonly). Corrected.dd:container_delete_request/responserow to the §3.3 message table (present in code/CHANGELOG/mdx, absent from SPEC).RuntimeDetails.Envreturns plaintext container env overGET /api/containers+ edge sync, which contradicted README's "values never exposed." Behavior is by design (redaction is the consumer's job) so this is docs-only: scoped the README claim to the MCPinspect_containertool (which really does return onlyenvCount) and added a security-model note flagging the surface for standalone/no-sockguard deploys. Bothsecurity-model.mdandsecurity-model.mdxupdated in lockstep.authentication.mdxvssecurity-model.mdxon whether Ed25519 is "the recommended" auth.portwing-with-composerow omittedPOST /networks/*/connect(the rule its own header flags as new). Added it.Build +
-raceon the affected packages green.