Skip to content

refactor: implement architecture review action plan - #251

Merged
tjjh89017 merged 3 commits into
mainfrom
refactor/arch-review-plan
Jul 29, 2026
Merged

refactor: implement architecture review action plan#251
tjjh89017 merged 3 commits into
mainfrom
refactor/arch-review-plan

Conversation

@tjjh89017

@tjjh89017 tjjh89017 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Implements the prioritized refactor plan produced by an earlier architecture-review pass (tmp/arch-review/refactor-plan.md), which independently reviewed all 12 subsystems of this codebase and concluded the top-level shape is sound — this PR is a targeted repair series, not a re-architecture. All 35 items (34 planned + 1 discovered mid-loop) landed, were independently code-reviewed, and were spot-checked a second time via random sampling; every item passed both gates.

P0 — Correctness fixes (crash/interop risk)

  • Guard crypto.Endpoint.Decrypt against short/malformed ciphertext (was a process-crashing panic on attacker/operator-controlled input)
  • Join daemon worker goroutines before shutdown proceeds, so main's cleanup can no longer race a controller still using the WireGuard client (both the Run and RunOneshot code paths)
  • Warn instead of silently dropping invalid/malformed peer public keys during config loading
  • Fix the contrib Cloudflare exec plugin's double-hash bug (interop-breaking, see behavior change note below)

P1 — Structural/DI repairs

  • Extract narrow, consumer-owned interfaces for PingMonitorController, plugin.Manager, and Daemon's four controller fields — closes the recurring pattern of concrete-type coupling that was blocking test coverage in several places
  • Add a test seam to internal/wg/client_ctrl.go (the default WireGuard backend)
  • Collapse byte-for-byte-duplicated Linux/Darwin-BSD STUN client code into a shared file
  • Resolve the proxy.enabled per-interface vs. global-OR mismatch (chosen: option (a), per-device — see behavior notes)
  • Fix mobile's plugin-traffic full-tunnel escape gap: corrected a false comment claiming the traffic was already protected, then implemented a real protected http.Transport for mobile's plugin manager

P2 — Test coverage

  • Ping-monitor retry/backoff state machine, daemon signal/ticker/shutdown behavior, mobile package (previously zero coverage), BPF filter/payload-offset logic, STUN Read's error paths, wgproxy's Darwin/Windows escape watchers, repo.Devices/repo.Peers concurrency (-race), and script-level smoke tests for all 4 contrib plugins
  • Extracted the shared endpoint-selection and STUN-discovery logic desktop and mobile had independently duplicated (and had already drifted) into one implementation both platforms now use — see behavior note below
  • Added protocol whitelist validation to mobile's config parser, matching desktop's existing behavior

P3 — Hygiene / doc drift

  • One cleanup pass per subsystem: ctrl, entity/repo, config, plugin, stun, wgproxy/routeprobe, composition root, crypto/logger, contrib, and CI/e2e config
  • Added docs/architecture.md recording the recorded-but-undocumented patterns this review surfaced (narrow-interface style, why proxy wiring lives in main, the plugin registry's init() self-registration, etc.)

Discovered mid-loop: wire.go was missing several wire.Bind directives from earlier interface-extraction work, so go generate wire.go/the wire CLI's actual codegen path (go generate .) had been silently broken for a while — the already-committed wire_gen.go still worked via Go's structural typing, but regeneration would fail. Fixed; go generate . now succeeds cleanly with a no-op diff.

Externally-visible behavior changes (all deliberate, all called out in the plan)

  • Invalid peer public keys: previously silently dropped, now logged as a warning (config loading still succeeds)
  • Cloudflare exec plugin DNS record naming: fixed a double-hash bug that made this plugin incompatible with the builtin Cloudflare plugin and its own shell-script sibling. Existing deployments self-heal on the next publish cycle; see contrib/cloudflare/README.md's new compatibility note for the one-time re-publish/pin-old-binary options.
  • Mobile config protocol validation: an unrecognized protocol string in mobile config now errors at parse time instead of being silently accepted (matches desktop's existing, documented behavior)
  • Mobile dualstack discovery policy: mobile now uses desktop's "soft" warn-and-continue-on-partial-failure policy instead of its previous silent-both-empty check
  • proxy.enabled: resolved as truly per-interface (option (a)) — a mixed config (one interface enabled, one not) now behaves as documented instead of collapsing to a global OR

No config-file-format changes, no plugin wire-protocol changes (exec JSON shape, shell variable protocol, pluginapi.Store all unchanged), no CLI flag changes.

Test plan

  • Every item independently code-reviewed via a blind two-stage process (broad scan + deep verify), several with fixes applied and re-verified
  • Every item spot-checked a second time via random sampling (5 consecutive clean batches)
  • go build/go vet clean across linux/darwin/freebsd/windows throughout
  • go test ./... passes (one pre-existing, unrelated sandbox network-permission failure in internal/plugin/dialer, confirmed present before this branch too)
  • go generate . (Wire codegen) succeeds cleanly
  • Full build-tag combination matrix verified for the plugin package ("", builtin_all, builtin_cloudflare, builtin_opendht, both together)
  • Mobile package built/tested with -tags mobile

Additional minor behavior deltas surfaced by post-hoc review (all benign)

  • Wrong-length-but-valid-base64 peer public keys: previously accepted with zero-padding/truncation (a broken identity), now dropped with a warning — stricter than the "warn instead of silent drop" item above states
  • Ping monitor's startup delay is now cancellable by context, so daemon shutdown within the first ~10s no longer blocks on it (required by the new goroutine join)
  • Logger falls back to info (was zerolog's trace default) on an unparseable level — unreachable via normal config loading, which rejects unknown levels first
  • Two message-only string changes: proxy listen-port error now says "between 0 and 65535" (matching the actual check), and a STUN short-packet trace log was reworded

Targeted repair series from a full architecture review of all 12
subsystems; the top-level shape was judged sound, so this is repairs,
not re-architecture.

Correctness fixes:
- Guard crypto.Endpoint.Decrypt against short/malformed ciphertext
  (was a process-crashing panic on stored-data input)
- Join daemon worker goroutines before shutdown in Run and RunOneshot,
  so cleanup can no longer race a controller using the WireGuard client
- Warn instead of silently dropping invalid peer public keys in config
- Fix the contrib Cloudflare exec plugin's double-hash record naming,
  making it interoperable with the builtin plugin and shell sibling

Structural/DI repairs:
- Narrow consumer-owned interfaces for PingMonitorController,
  plugin.Manager, and Daemon's controller fields; test seam for
  internal/wg/client_ctrl.go
- Collapse duplicated Linux/Darwin-BSD STUN client code into a shared
  file; extract shared endpoint-selection and STUN-discovery logic
  (internal/ctrl/discover.go, endpoint_select.go) used by both desktop
  and mobile, removing drifted duplicates
- Resolve proxy.enabled as truly per-interface instead of a global OR
- Implement a real protected http.Transport for mobile plugin traffic
  (previous comment claiming protection was false)
- Restore missing wire.Bind directives so `go generate wire.go` works

Test coverage:
- Ping retry/backoff state machine, daemon signal/ticker/shutdown,
  mobile package, BPF filter/payload offsets, STUN Read error paths,
  wgproxy escape watchers, repo concurrency (-race), contrib plugin
  smoke tests, mobile config protocol validation

Hygiene:
- One cleanup pass per subsystem; docs/architecture.md records the
  previously undocumented patterns

Deliberate behavior changes: invalid peer keys now warn; cloudflare
exec plugin DNS names change (self-heals on next publish, see its
README); mobile config rejects unknown protocol strings; mobile
dualstack uses desktop's warn-and-continue policy; mixed proxy.enabled
configs behave per-interface. No config-format, plugin wire-protocol,
or CLI changes.

Signed-off-by: Date Huang <tjjh89017@hotmail.com>
… tags

Signed-off-by: Date Huang <tjjh89017@hotmail.com>
Corrects the golangci build-tags list, wire regeneration command (the
directive lives in wire_gen.go, so 'go generate wire.go' was a no-op),
plugin manager path, entity constructor signatures (firewallMark,
DeviceId/PeerPublicKey types), mock-generation scope, protocol
default-vs-validation attribution, STUN method signatures and the
Windows stub, plugin-type extension steps (no PluginType constant),
CI gate names, and release archive naming.

Signed-off-by: Date Huang <tjjh89017@hotmail.com>
@tjjh89017
tjjh89017 merged commit eca0b57 into main Jul 29, 2026
59 checks passed
@tjjh89017
tjjh89017 deleted the refactor/arch-review-plan branch July 29, 2026 06:51
tjjh89017 added a commit that referenced this pull request Jul 29, 2026
#251 added script-level smoke tests for the contrib plugins but they
were never invoked by any Makefile target or GitHub workflow, only
guarding regressions when run manually.

Add plugin-test/contrib-test targets (root Makefile -> contrib/Makefile
-> per-plugin Makefiles) that run each plugin's smoke_test.sh or go
test, wire a new contrib-test CI job into main.yml, and document the
smoke tests in contrib/README.md and CLAUDE.md.

Signed-off-by: Date Huang <tjjh89017@hotmail.com>
tjjh89017 added a commit that referenced this pull request Jul 29, 2026
Batches six minor findings flagged during #251's review:

- internal/config/config.go: use %d directly instead of %s +
  strconv.Itoa for the proxy listen/fib validation error messages
- contrib/cloudflare-shell/smoke_test.sh: fix a garbled header comment
- internal/ctrl/publish.go: remove a dead trailing continue (already
  the last statement in its loop body)
- internal/wg: add a test (client_ctrl_windows_test.go, windows-only
  since the mapping itself is Windows-only) for the access-denied ->
  ErrElevationRequired path, previously untested
- internal/ctrl/discover_test.go: replace the "must not be called"
  sentinel error, which DiscoverEndpoints' single-family error
  handling silently swallowed, with a resolver stub that fails the
  test immediately if invoked
- internal/daemon: replace daemon_signal_test.go's flaky fixed 50ms
  sleep with a signalReady readiness channel closed by Run right
  after signal.Notify registers

Signed-off-by: Date Huang <tjjh89017@hotmail.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.

1 participant