docs(#233): add secure HTTP client guidance to AGENTS.md

[fullsend-ai-coder]

Add a "Secure HTTP clients" subsection under "Go code" documenting the five required security properties for any new http.Client that fetches external URLs:

1. SSRF-safe dialer with DNS pre-resolution and IP validation
   via netutil.CheckIP
2. HTTPS-only with redirect blocking or scheme validation 3. Transport.Proxy = nil to prevent proxy-based redirection 4. Explicit timeout (30s default) — never http.DefaultClient 5. io.LimitReader for response bodies (10 MB default)

References internal/fetch/fetch.go (FetchURL/FetchPolicy), internal/netutil/ip.go (CheckIP), and internal/security/ssrf.go (SSRFValidator) as the canonical implementations.

---

Closes #233

Post-script verification

• Branch is not main/master (agent/233-secure-http-client-guidance)
• Secret scan passed (gitleaks — c776c248ac9a91f10998ca7107e974a6c9d76514..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 1:05 PM UTC · Completed 1:05 PM UTC
Commit: c776c24 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:04 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance file. The change is authorized by issue [RTK+Ponytail] Add AGENTS.md guidance for secure HTTP client construction in Go #233 and the rationale is clearly explained, but human approval is always required for protected-path changes regardless of context.

Low

• [technical documentation accuracy] AGENTS.md:55 — The documentation instructs "Set Transport.Proxy = nil" and claims internal/fetch/fetch.go implements all five properties. While Go's stdlib confirms this is correct behavior ("If Proxy is nil or returns a nil *URL, no proxy is used"), fetch.go satisfies property [CNV-88172] Add test feature placeholder #3 implicitly via the zero-value of &http.Transport{} rather than an explicit Proxy: nil assignment. Adding an explicit field would make the canonical implementation self-documenting and prevent a future maintainer from accidentally switching to http.DefaultTransport (which sets Proxy: http.ProxyFromEnvironment).
  Remediation: Add Proxy: nil to the Transport literal in fetch.go, or reword the guidance to clarify that constructing a new http.Transport (rather than cloning http.DefaultTransport) inherently disables proxy usage.

• [incomplete-reference] AGENTS.md:59 — The parenthetical "(note: HTTPS enforcement is in fetch.go, not ssrf.go)" is factually correct but could be marginally clearer as "HTTPS-only enforcement" since ssrf.go does contain scheme validation logic (it permits both http and https while blocking dangerous schemes like file, ftp, gopher).
  Remediation: Optionally change "HTTPS enforcement" to "HTTPS-only enforcement."

Previous run

Review

Verdict: Approve

Well-scoped docs PR that adds actionable secure-HTTP-client guidance to AGENTS.md, directly addressing issue #233. The five documented security properties are technically accurate and consistent with the canonical implementation in internal/fetch/fetch.go. All referenced source files (fetch.go, netutil/ip.go, security/ssrf.go) and symbols (FetchURL, FetchPolicy, CheckIP) exist and match their descriptions. Placement under "Go code" at ### heading level is consistent with the existing document hierarchy.

No security concerns, no scope creep, no injection patterns.

Minor notes (non-blocking)

#	File	Observation
1	AGENTS.md	The doc says to validate IPs "with netutil.CheckIP" but fetch.go (the referenced canonical implementation) actually calls netutil.IsInternal — a thin wrapper around CheckIP. Both are correct; CheckIP is arguably better guidance for new code since it returns a reason string. A developer grepping fetch.go for CheckIP will find no hits.
2	AGENTS.md	Property 3 advises "Set Transport.Proxy = nil" explicitly. fetch.go achieves the same result via Go's zero value (a fresh &http.Transport{} has nil Proxy). The explicit-nil guidance is the more defensive practice — just noting that the reference implementation doesn't literally match the guideline text.
3	AGENTS.md	The closing paragraph references ssrf.go as the "URL-level SSRF validator" in the same section that mandates HTTPS-only. ssrf.go's ValidateURL permits http:// (it only blocks dangerous schemes like file://, gopher://). The doc correctly attributes all five properties to fetch.go, not ssrf.go, so this isn't a contradiction — but readers may benefit from a parenthetical clarifying that ssrf.go handles host/IP validation while HTTPS enforcement is in fetch.go.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• AGENTS.md

---

Labels: PR adds security-focused documentation to AGENTS.md

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:43 PM UTC · Completed 5:52 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 2 of 3 non-blocking review observations (IP validation function reference and SSRF validator role clarification). Disagreed with 1 (Transport.Proxy nil guidance is correct as-is for new code).

Fixed (2):

1. fetch.go uses netutil.IsInternal, not CheckIP (AGENTS.md): Updated property 1 to mention both netutil.IsInternal (used by the canonical fetch.go implementation) and netutil.CheckIP, so developers grepping the reference implementation find the function
2. ssrf.go permits http:// but doc implies it enforces HTTPS (AGENTS.md): Clarified closing paragraph: ssrf.go handles host/IP/scheme validation while HTTPS enforcement lives in fetch.go, preventing reader confusion about where each property is implemented

Disagreed (1):

1. Transport.Proxy = nil vs zero-value behavior: The explicit-nil guidance is the more defensive practice for new code, even though fetch.go relies on Go's zero value. Keeping the doc as-is gives better guidance for developers writing new HTTP clients.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:05 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[low] technical documentation accuracy

The documentation instructs "Set Transport.Proxy = nil" and claims internal/fetch/fetch.go implements all five properties. While the guidance is correct (Go docs: "If Proxy is nil or returns a nil *URL, no proxy is used"), fetch.go satisfies this property implicitly via zero-value rather than an explicit Proxy: nil assignment. Adding the explicit field would make the canonical implementation self-documenting.

Suggested fix: Add Proxy: nil to the Transport literal in fetch.go, or reword the guidance to clarify that constructing a new http.Transport (rather than cloning http.DefaultTransport) inherently disables proxy usage.

[fullsend-ai-review]

[low] incomplete-reference

The parenthetical "(note: HTTPS enforcement is in fetch.go, not ssrf.go)" is factually correct but could be marginally clearer as "HTTPS-only enforcement" since ssrf.go does contain scheme validation logic (permits both http and https while blocking dangerous schemes).

Suggested fix: Optionally change "HTTPS enforcement" to "HTTPS-only enforcement."