Skip to content

peer: call /peer/verify after starting sing-box; fix doubled /v1#466

Open
myleshorton wants to merge 7 commits into
fisk/peer-localbackendfrom
fisk/peer-call-verify
Open

peer: call /peer/verify after starting sing-box; fix doubled /v1#466
myleshorton wants to merge 7 commits into
fisk/peer-localbackendfrom
fisk/peer-call-verify

Conversation

@myleshorton
Copy link
Copy Markdown
Contributor

@myleshorton myleshorton commented May 6, 2026

Summary

Pairs with lantern-cloud PR getlantern/lantern-cloud#2696 (split live-verify out of /peer/register into an explicit /peer/verify endpoint). Surfaced through end-to-end testing on 2026-05-07; PR has grown to absorb several follow-up fixes that came out of that exercise.

Original three changes

  1. Drop the leading /v1 from peer endpoint paths in peer/api.go. baseURL already ends with /api/v1 (from common.GetBaseURL), so /v1/peer/register was hitting /api/v1/v1/peer/register on prod and 404'ing. Every other radiance API caller appends without /v1 (config/fetcher.go, issue/issue.go); peer/api.go was the odd one out.

  2. Add API.Verify(ctx, routeID) wrapping POST /peer/verify.

  3. Call API.Verify in peer.Client.Start after box.Start succeeds. The server's verifier dials back through the peer's external port using the just-built creds, so the inbound has to be listening before verify runs. Splitting verify out of register resolves the chicken-and-egg where register-time verify could never see a peer that didn't yet have its cert.

Follow-up fixes (added 2026-05-07 from end-to-end testing)

  1. Forward common headers (notably X-Lantern-Config-Client-IP) on every peer endpoint. peer/api.go was building requests with bare http.NewRequestWithContext and skipping the standard common.NewRequestWithHeaders set. lantern-cloud's util.ClientIPWithAddr (cmd/api/util/header.go:155-184) prefers X-Lantern-Config-Client-IP over X-Forwarded-For/RemoteAddr when resolving clientIP; without the header, the server-side verify-dial could target a different IP than radiance has detected as the user's public IP. Adds TestAPI_ForwardsCommonHeaders regression test.

  2. RADIANCE_PEER_EXTERNAL_PORT env-var manual override for routers without UPnP/NAT-PMP/PCP (notably eero). When set, substitutes a manualPortForwarder for the UPnP discovery path. Invalid values fall back to UPnP with a warning rather than silently disabling peer share. Pairs with operator-side router config.

  3. Operator-visibility logs in backend/radiance.go. applyPeerShare now logs the underlying Start error at Error level (and a paired success at Info), and the once-per-daemon "Detected public IP" goes from Debug to Info with the resolved IP added to the structured fields. Both make peer-share triage from local daemon logs much faster.

Stacked on PR #460

This PR's base is fisk/peer-localbackend (PR #460), which introduces the peer package itself. Mergeable in order: #460 → this.

The lantern-cloud counterpart (#2696) can land independently — once it deploys, registration starts returning 200 + ServerConfig without verifying. Until this PR lands and ships in lantern, peers won't trigger /peer/verify and will sit with callback_verified=NULL forever (catalog filter hides them, so no client-visible breakage).

Test plan

  • go test ./peer/ ./backend/ ./common/... — pass
  • End-to-end peer-share toggle hits /peer/register/peer/verify against prod (verified via SigNoz traces 2026-05-07)
  • CI

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 6, 2026 20:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the peer “Share My Connection” flow to align with the lantern-cloud change that splits verification into an explicit /peer/verify step, and fixes incorrect peer API endpoint path construction that could produce doubled /v1 in production.

Changes:

  • Remove the redundant /v1 prefix from peer API endpoint paths so they append correctly to common.GetBaseURL().
  • Add API.Verify(ctx, routeID) for POST /peer/verify.
  • Call API.Verify from peer.Client.Start after sing-box successfully starts listening.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
peer/peer.go Triggers server-side verification after sing-box startup to avoid the previous register-time chicken-and-egg.
peer/peer_test.go Extends the stub server with a /peer/verify endpoint and related counters/status fields.
peer/api.go Fixes endpoint paths (remove /v1 prefix) and adds an API method for /peer/verify.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread peer/api.go Outdated
Comment thread peer/peer_test.go
Comment thread peer/peer_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Comment thread peer/peer.go Outdated
Comment thread peer/api.go Outdated
Comment thread peer/peer.go
Comment thread peer/peer_test.go Outdated
Comment thread backend/radiance.go Outdated
Comment thread backend/radiance.go Outdated
@myleshorton myleshorton force-pushed the fisk/peer-localbackend branch from 3b19ec0 to 8cb3713 Compare May 28, 2026 23:57
@myleshorton myleshorton force-pushed the fisk/peer-call-verify branch from 024337d to c7b4a75 Compare May 29, 2026 01:58
@myleshorton myleshorton force-pushed the fisk/peer-localbackend branch from 8cb3713 to 5959bbe Compare May 29, 2026 19:51
@myleshorton myleshorton force-pushed the fisk/peer-call-verify branch 3 times, most recently from 5614c02 to dff7a12 Compare May 29, 2026 20:17
myleshorton added a commit that referenced this pull request May 29, 2026
Six clusters of fixes from the round-1 + round-2 Copilot passes:

1. peer/api.go: NewAPI doc now states the baseURL contract honestly —
   common.GetBaseURL returns either '.../v1' (stage) or '.../api/v1'
   (prod); the previous wording hard-coded the prod form and would
   mislead a future caller writing a test.

2. peer/peer_test.go: stub server registers under /v1/peer/* and
   newTestClient passes srv.server.URL+"/v1" as the baseURL. The bare
   URL the test had before would've masked a peer/api.go regression
   that double-prefixes the version segment.

3. peer/peer_test.go: /peer/verify handler decodes LifecycleRequest
   into srv.lastVerifyReq so tests can assert the route_id round-trips
   correctly.

4. peer/peer.go: defaultBuildBoxService no longer discards the caller's
   ctx. New boxRegistryCtx wraps the caller's ctx and falls back to
   box.BaseContext() on Value() lookups — preserves cancellation while
   keeping libbox's protocol-registry resolution working.

5. backend/radiance.go: 'Detected public IP' Info log no longer
   includes the IP itself. Lantern users in censored regions can't
   safely have their public IP in routinely-collected client logs;
   confidence + sources are enough for operator-side 'detection
   succeeded' triage and the IP is correlated server-side via traces.

6. backend/peer_share.go: slog calls use 'error' / 'start_error' /
   'rollback_error' keys to match the backend-package convention
   (backend/radiance.go uses 'error' exclusively; the 'err' keys came
   from the peer package's own convention and don't fit here).

New tests:
- TestClient_Start_HappyPath now asserts verifyCount==1 and the
  route_id round-trips through /peer/verify.
- TestClient_Start_VerifyFailureUnwinds: when verify returns 500,
  Start must unmap, close box, deregister, and return an error.

All existing tests still pass under -race.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@myleshorton myleshorton force-pushed the fisk/peer-localbackend branch from be86ee9 to 7d616d6 Compare May 29, 2026 20:43
Adam Fisk and others added 6 commits May 29, 2026 14:44
Two changes that pair with the lantern-cloud /peer/verify split:

  1. peer/api.go: drop the leading /v1 from peer endpoint paths.
     baseURL already ends with /api/v1 (from common.GetBaseURL), so
     /v1/peer/register was hitting /api/v1/v1/peer/register on prod
     and 404'ing. Every other radiance API caller appends without
     /v1 (config/fetcher.go, issue/issue.go); peer/api.go was the
     odd one out. Updated NewAPI's docstring to spell out the
     convention.

  2. peer/peer.go: after box.Start succeeds, call API.Verify(routeID).
     The server's verifier dials back through the peer's external
     port using the just-built creds, so the inbound has to be
     listening before verify runs. Splitting verify out of register
     resolves the chicken-and-egg where register-time verify could
     never see a peer that didn't yet have its cert. Verify failure
     here is fatal — the server has already deprecated the row, so
     the deferred cleanup tears the rest of the session down.

  3. peer/api.go: new API.Verify(ctx, routeID) wrapping POST
     /peer/verify.

Tests: stubServer's mux handles the new /peer/verify route plus
verifyCount / verifyDeviceID / verifyStatus knobs. Existing tests
exercise the new step transparently because they use the default
verifyStatus=200.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
defaultBuildBoxService used to call libbox.NewServiceWithContext with
the caller's bare ctx, which has no lantern-box protocol registries
plumbed in. The samizdat inbound type ServerConfig sends back from
/peer/register isn't a built-in sing-box protocol, so libbox's JSON
decoder couldn't resolve inbounds[0].type="samizdat" and returned
"missing inbound fields registry in context". The integration tests
stub BuildBoxService entirely, so this layer was never exercised in
CI — only surfaced live during the eero end-to-end test.

Two pieces:

  1. Use box.BaseContext() (from getlantern/lantern-box) when calling
     libbox.NewServiceWithContext. That ctx has the InboundOptionsRegistry
     populated with samizdat / reflex / etc. so the decode succeeds.
     Coexists with the user's VPN tunnel (vpn/tunnel.go) — libbox.Setup
     is process-global, the ctx registries are per-box.

  2. TestDefaultBuildBoxService_DecodesSamizdatInbound walks the actual
     decode path with a minimal samizdat-inbound JSON. Verified to fail
     with the exact production error message under the pre-fix code,
     pass under the fix. Cuts the diagnostic loop from a 5-minute
     rebuild+redeploy+toggle cycle to a 0.5s test failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…every peer endpoint

peer/api.go was building requests with bare http.NewRequestWithContext,
skipping the X-Lantern-Config-Client-IP / X-Lantern-User-Id / version
header set that /config-new sends via common.NewRequestWithHeaders.

That mattered for /peer/register specifically: the server's
util.ClientIPWithAddr (lantern-cloud cmd/api/util/header.go:155-184)
prefers X-Lantern-Config-Client-IP over X-Forwarded-For and RemoteAddr
when resolving clientIP. With the header missing, the server fell back
to whatever its X-Forwarded-For chain produced — potentially a
different IP than the radiance-detected publicIP, leading the verifier
to dial back to an address the peer's listener wasn't bound to.

Switching to common.NewRequestWithHeaders makes peer endpoints
consistent with /config-new's header set:

  - X-Lantern-Config-Client-IP (the key one for verify-dial targeting)
  - X-Lantern-App-Version, X-Lantern-Version, X-Lantern-Platform,
    X-Lantern-App, X-Lantern-User-Id, X-Lantern-Time-Zone, X-Lantern-Rand

DeviceIDHeader is set by NewRequestWithHeaders from settings; we
explicitly re-set it to a.deviceID afterward for parity with the
prior behavior in case the two ever diverge.

Adds TestAPI_ForwardsCommonHeaders which hits all four peer endpoints
against a stub server and asserts each carries the expected headers
(uses common.SetPublicIP / Cleanup to avoid leaking into other tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uters

UPnP / NAT-PMP / PCP discovery is silent or absent on a meaningful
chunk of consumer routers — eero in particular ignores all three.
For peers behind such routers, peer.Client.Start currently fails at
the MapPort step with portforward.ErrNoPortForwarding even though
the operator has perfectly valid manual port-forward rules in their
router admin UI.

Add an env-var escape hatch: when RADIANCE_PEER_EXTERNAL_PORT is set
to a 1..65535 value, NewClient's default forwarder substitutes a
manualPortForwarder that:

  - Returns the manual port unchanged for both internal and external
    sides of the Mapping (operator is responsible for matching the
    sing-box bind to the same port).
  - Returns "" from ExternalIP, letting peer_handler's "external_ip
    empty -> use observed" fall-through resolve the IP server-side.
  - Is a no-op for UnmapPort and StartRenewal (nothing to release;
    the manual rule is operator-managed).

Invalid values (non-numeric, <1, >65535) log a warning and fall back
to the default UPnP path so a typo doesn't silently disable peer
share entirely.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… log to Info

Two operator-visibility tweaks that helped during peer-share testing
and are worth keeping:

1. applyPeerShare(true) now logs the underlying Start error at Error
   level (and a paired success log at Info). Without this, a peer
   share toggle that fails server-side (4xx, UPnP miss, samizdat
   verify timeout) only surfaces via the IPC HTTP response — a
   layer the daemon log never sees, making post-hoc triage from
   the user's local logs much harder than necessary.

2. The "Detected public IP" log goes from Debug to Info, with the
   resolved IP added to the structured fields. publicIP is fetched
   exactly once per daemon lifetime; emitting it at Info gives
   operators a single line to compare against what lantern-cloud
   observed for that same client (visible in SigNoz traces) without
   needing to flip the global log level.

No behavior change beyond the log lines.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six clusters of fixes from the round-1 + round-2 Copilot passes:

1. peer/api.go: NewAPI doc now states the baseURL contract honestly —
   common.GetBaseURL returns either '.../v1' (stage) or '.../api/v1'
   (prod); the previous wording hard-coded the prod form and would
   mislead a future caller writing a test.

2. peer/peer_test.go: stub server registers under /v1/peer/* and
   newTestClient passes srv.server.URL+"/v1" as the baseURL. The bare
   URL the test had before would've masked a peer/api.go regression
   that double-prefixes the version segment.

3. peer/peer_test.go: /peer/verify handler decodes LifecycleRequest
   into srv.lastVerifyReq so tests can assert the route_id round-trips
   correctly.

4. peer/peer.go: defaultBuildBoxService no longer discards the caller's
   ctx. New boxRegistryCtx wraps the caller's ctx and falls back to
   box.BaseContext() on Value() lookups — preserves cancellation while
   keeping libbox's protocol-registry resolution working.

5. backend/radiance.go: 'Detected public IP' Info log no longer
   includes the IP itself. Lantern users in censored regions can't
   safely have their public IP in routinely-collected client logs;
   confidence + sources are enough for operator-side 'detection
   succeeded' triage and the IP is correlated server-side via traces.

6. backend/peer_share.go: slog calls use 'error' / 'start_error' /
   'rollback_error' keys to match the backend-package convention
   (backend/radiance.go uses 'error' exclusively; the 'err' keys came
   from the peer package's own convention and don't fit here).

New tests:
- TestClient_Start_HappyPath now asserts verifyCount==1 and the
  route_id round-trips through /peer/verify.
- TestClient_Start_VerifyFailureUnwinds: when verify returns 500,
  Start must unmap, close box, deregister, and return an error.

All existing tests still pass under -race.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (2)

peer/api.go:128

  • This comment references a code location (config/fetcher.go), which AGENTS.md:13-17 says to keep out of code comments. The rationale still works if it describes the standard header directly.
	// Forward the same feature-override header that config/fetcher.go uses
	// for /config-new requests, so QA can flip on `peer_proxy` ahead of the
	// public-flag rollout via FeatureOverridesKey (RADIANCE_FEATURE_OVERRIDES).
	// Without this the server-side gate rejects register/heartbeat/deregister
	// regardless of the local toggle.

peer/api.go:128

  • This comment references a code location (config/fetcher.go), which AGENTS.md:13-17 says to keep out of code comments. The rationale still works if it describes the standard header directly.
	// Forward the same feature-override header that config/fetcher.go uses
	// for /config-new requests, so QA can flip on `peer_proxy` ahead of the
	// public-flag rollout via FeatureOverridesKey (RADIANCE_FEATURE_OVERRIDES).
	// Without this the server-side gate rejects register/heartbeat/deregister
	// regardless of the local toggle.

Comment thread peer/peer.go Outdated
Comment thread peer/peer.go Outdated
Comment thread peer/api.go Outdated
Comment thread peer/peer_test.go Outdated
Comment thread peer/peer_test.go Outdated
Comment thread peer/peer.go
Comment thread peer/peer.go Outdated
Comment thread peer/peer.go Outdated
Comment thread peer/peer_test.go Outdated
Comment thread peer/peer_test.go Outdated
AGENTS.md:13-17 forbids code-location references in comments. Round-1
fixes had reintroduced several. Rewrote each to describe the contract
or invariant directly without naming files:

- peer/peer.go:37 (manualPortForwarder.ExternalIP) — drop reference to
  the server's peer_handler.
- peer/peer.go:152 (NewClient manual-override branch) — drop the 'see
  env.PeerExternalPort' pointer; the manualPort() call site is self-
  describing.
- peer/peer.go:518 (defaultBuildBoxService) — drop the explicit
  vpn/tunnel.go path; the 'same process as the user's main VPN tunnel'
  framing carries the invariant without naming the source file.
- peer/api.go:56 (NewAPI doc) — drop the 'mirroring config/fetcher.go,
  issue/issue.go' tail and the hard-coded host names. The old comment
  also named the wrong prod host: BaseURL is df.iantem.io/api/v1, not
  api.iantem.io/api/v1, so the inaccuracy is fixed too.
- peer/peer_test.go:175 + :232 — drop the peer/api.go references; the
  'regression in URL composition' framing is what matters.

Also added test coverage for RADIANCE_PEER_EXTERNAL_PORT:

- TestManualPort exercises parsing across unset / valid mid-range /
  valid 1 + 65535 boundaries / non-numeric / 0 / negative / above-
  uint16 / way-above-uint16. All non-positive and out-of-range values
  collapse to 0 (the 'use UPnP discovery' signal).
- TestManualPortForwarder exercises the full portForwarder contract:
  MapPort returns external==internal port + 'manual-env' method,
  UnmapPort and StartRenewal are no-ops, ExternalIP returns empty
  (server substitutes observed IP).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
myleshorton added a commit that referenced this pull request May 29, 2026
The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
myleshorton added a commit that referenced this pull request May 30, 2026
The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
myleshorton added a commit that referenced this pull request May 30, 2026
* peer: read PeerManualPortKey setting alongside RADIANCE_PEER_EXTERNAL_PORT

Adds settings.PeerManualPortKey so the user-facing Advanced UI can
persist the manual port forward without an env var. Resolution order
in peer.Client.Start's NewForwarder:

  1. settings.PeerManualPortKey (Advanced UI in lantern Flutter)
  2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / power-user)
  3. UPnP discovery (default)

The setting is wired through lantern-core's
PatchSettings(PeerShareEnabledKey...) path on a separate branch — the
new `setPeerManualPort` FFI export over there calls
PatchSettings({PeerManualPortKey: <int>}) which lands in radiance's
settings store and gets picked up on the next peer.Client.Start.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* portforward: extract manual port forwarder to its own file

The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* peer/portforward/settings: address Copilot review on #500

Four substantive findings; three additional Copilot comments review a
pre-consolidation state of portforward/manual.go that the consolidation
commit (22d1533) replaced wholesale — those are answered with the
relevant context in the thread replies.

1. peer.Client.Start now range-checks the PeerManualPortKey setting
   before casting to uint16. A raw uint16 cast silently wraps negative
   values (-5 → 65531) and values above the port space (70000 → 4464),
   which would register a port the peer doesn't listen on (or, worse,
   one it does listen on for a different service). Out-of-range values
   are now logged at Warn and fall through to env-var / UPnP as if the
   setting were unset.

2. common/settings PeerManualPortKey doc now documents the 1..65535
   valid range, behavior on out-of-range values, and the 0=unset
   contract. Dropped the peer.Client.Start / portforward.ManualForwarder
   code-location references — describes the contract generically.

3. portforward.NewManualForwarder doc tightened to state the caller-
   side validation contract (port must be 1..65535) without naming
   ParseManualPort or 'env-var path' / 'setting' as callers.

No behavior change in #2 or #3; only #1 changes runtime behavior, and
only for invalid setting values.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* peer/portforward: address Copilot review on #500 (round 2)

Two doc-lint follow-ups per AGENTS.md (no code-location refs in
comments):

1. portforward.ManualForwarder doc dropped the 'satisfies the
   portForwarder contract' phrasing (portForwarder is the peer
   package's private interface; mentioning it crosses a package
   boundary) and the 'peer.Client at it via setting or env var'
   reference. The new wording describes the type in terms of this
   package's own exported API: 'exposes the same Map/Unmap/
   StartRenewal/ExternalIP surface as Forwarder but does no UPnP
   work.'

2. peer.Client.Start's resolution-order comment now spells the
   persisted setting name in quotes ('peer_manual_port') rather than
   the Go identifier (settings.PeerManualPortKey). The persisted
   name is the stable contract — if the Go identifier ever moves or
   renames, the comment stays correct without needing to be updated.
   Same treatment for the env-var line, which already used the
   stable name string.

No behavior change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Adam Fisk <afisk@mini.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
myleshorton added a commit that referenced this pull request May 30, 2026
The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants