mitm: address remaining review on #151#152
Merged
dangtony98 merged 3 commits intomainfrom May 6, 2026
Merged
Conversation
Three findings from the post-merge review pass: 1. cmd/run.go:149 hardcoded 127.0.0.1 in the "routing HTTP/HTTPS through MITM proxy" banner while augmentEnvWithMITM derives the actual host from the parsed --address. With a remote vault server the banner misled operators debugging proxy connectivity. Extracted resolveMITMHost(addr) so both the env-var path and the banner pull from the same source. 2. internal/mitm/forward_test.go:601 was named TestMITMForwardIPv6LiteralCanonicalises but the request line carries an explicit port — net.SplitHostPort succeeds in the old code, the buggy double-bracket fallback is never taken, and what the assertion actually verifies is the Host-header port-preservation fix. Renamed to TestMITMForwardIPv6PreservesHostHeader and rewrote the comment to match. End-to-end coverage of the no-port canonicalisation branch would need to bind port 80 on ::1, which is impractical for CI. 3. Only persistent_instructions_admin.txt is //go:embed-ed in handle_agents.go; both invite-redeem paths hardcode persistentInstructionsAdmin for every redeeming agent regardless of role. The other four files (persistent_instructions_member.txt, persistent_instructions_proxy.txt, persistent_agent_instructions.txt, instructions.txt) shipped on disk but never reached any agent. Doc cross-syncs kept repainting them. Deleted.
/simplify finding: the comment narrated the change ("Single source of
truth for both the env-var path and the operator-facing banner") rather
than describing the function contract. Trimmed to purpose + fallback
behaviour, dropped the call-site enumeration that would go stale on
the next caller.
|
💬 Discussion in Slack: #pr-review-agent-vault-152-mitm-address-remaining-review-on-151 Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review on #152: - BuildProxyEnv composed HTTPS_PROXY/HTTP_PROXY via fmt.Sprintf("%s:%d", host, port). For IPv6 --address (e.g. http://[::1]:14321), resolveMITMHost returns the bare "::1" (url.Hostname strips brackets), so the proxy URL came out as "https://tok:v@::1:14322" — net.SplitHostPort rejects the authority as "too many colons", failing every outbound request from the child agent. Switch to net.JoinHostPort for both env.go:49 and the cosmetic banner at run.go:149. - forward_test.go inline comment for TestMITMForwardIPv6PreservesHostHeader still claimed the URL was the no-port form; it actually carries an explicit ephemeral port. Realigned with the new outer docstring. https://claude.ai/code/session_011CrrNcAGDQvCLgvj79Cui2
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
2 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.
Summary
Three findings from the post-merge review on #151 that landed after the squash:
127.0.0.1in the "routing HTTP/HTTPS through MITM proxy" banner whileaugmentEnvWithMITMderives the host from--address. Misleading on remote vault deploys. ExtractedresolveMITMHost(addr)so banner and env-var path share one source.TestMITMForwardIPv6LiteralCanonicaliseswas named for the no-port canonicalisation branch, but the request URL carries an explicit port —net.SplitHostPortsucceeds in the old code and the buggy fallback is never taken. The assertion actually verifies the Host-header port-preservation fix. Renamed toTestMITMForwardIPv6PreservesHostHeaderand rewrote the comment to match.persistent_instructions_admin.txtis//go:embed-ed; both invite-redeem paths inhandle_agents.gohardcodepersistentInstructionsAdminregardless of role. The other four files (persistent_instructions_member.txt,persistent_instructions_proxy.txt,persistent_agent_instructions.txt,instructions.txt) shipped on disk but never reached any agent. Doc cross-syncs kept repainting them. Deleted.Plus a small
/simplifyfollow-up tightening theresolveMITMHostdoc comment.Test plan
go build ./...go test ./cmd/... ./internal/mitm/... ./internal/server/...