From 8bc00d1e901af72c6b009c27b910d475b847cb71 Mon Sep 17 00:00:00 2001 From: Sanaei Date: Mon, 27 Jul 2026 14:37:57 +0200 Subject: [PATCH] style: drop the line comments added with the triage fixes CLAUDE.md rules out // line comments in committed Go. The rationale they carried is in the commit messages for each fix; doc comments that already existed are kept, updated where the code they describe changed. Also replaces reflect.Ptr with reflect.Pointer and rewrites the YAML keyword alternation as a lookup table, both flagged by golangci-lint. --- internal/sub/clash_yaml.go | 45 ++++++------------- internal/sub/clash_yaml_test.go | 12 ----- internal/sub/remark_vars_test.go | 3 -- internal/web/job/xray_traffic_inform_test.go | 6 --- internal/web/job/xray_traffic_job.go | 11 ----- internal/web/service/global_traffic_test.go | 7 --- internal/web/service/inbound_disable.go | 9 ---- .../web/service/xray_config_clients_test.go | 8 ---- 8 files changed, 14 insertions(+), 87 deletions(-) diff --git a/internal/sub/clash_yaml.go b/internal/sub/clash_yaml.go index 4c41db04c..99b067d9b 100644 --- a/internal/sub/clash_yaml.go +++ b/internal/sub/clash_yaml.go @@ -8,23 +8,24 @@ import ( "github.com/goccy/go-yaml" ) -// yamlQuotedString is a string that must reach a YAML parser as a string. -// Single-quoted style is used because it carries the value literally — no -// escape processing — which suits hex ids, passwords and pre-shared keys. type yamlQuotedString string func (s yamlQuotedString) MarshalYAML() ([]byte, error) { return []byte("'" + strings.ReplaceAll(string(s), "'", "''") + "'"), nil } -// yamlPlainScalarNotString matches the plain scalars a YAML parser resolves to -// something other than a string. It covers the YAML 1.2 core schema plus the -// 1.1 forms go-yaml v3 — the library the Clash cores use — still resolves: -// null, booleans (including y/yes/on and friends), integers in every base, -// sexagesimals, floats, and dates. -var yamlPlainScalarNotString = regexp.MustCompile(`^(?:` + - `~|[Nn]ull|NULL|` + - `[Tt]rue|TRUE|[Ff]alse|FALSE|[Yy]es|YES|[Nn]o|NO|[Oo]n|ON|[Oo]ff|OFF|[YyNn]|` + +var yamlNonStringWords = map[string]bool{ + "~": true, "null": true, "Null": true, "NULL": true, + "true": true, "True": true, "TRUE": true, + "false": true, "False": true, "FALSE": true, + "yes": true, "Yes": true, "YES": true, + "no": true, "No": true, "NO": true, + "on": true, "On": true, "ON": true, + "off": true, "Off": true, "OFF": true, + "y": true, "Y": true, "n": true, "N": true, +} + +var yamlNonStringNumber = regexp.MustCompile(`^(?:` + `[-+]?[0-9]+|` + `0[oO]?[0-7]+|0[xX][0-9a-fA-F]+|` + `[-+]?[0-9][0-9_]*(?::[0-5]?[0-9])+|` + @@ -33,29 +34,13 @@ var yamlPlainScalarNotString = regexp.MustCompile(`^(?:` + `[0-9]{4}-[0-9]{1,2}-[0-9]{1,2}(?:[Tt ].*)?` + `)$`) -// yamlScalarIsAmbiguous reports whether s must be quoted to survive as a string. -// -// The encoder quotes most of these itself, but not the exponent-float form: a -// REALITY short-id such as "2351e1" is valid hex and, emitted bare, is read as -// 23510. A Clash core then hex-decodes a five-digit number, rejects the proxy -// and drops the whole provider to zero nodes (#6104). goccy's own parser reads -// that token back as a string, so the encoder never sees a problem and a -// round-trip check through it cannot find one either — the resolution rules, -// not the encoder, are the thing to test against. The same shape reaches -// passwords, obfs-passwords and pre-shared keys, so every string in the -// document is checked rather than an enumerated list of fields. func yamlScalarIsAmbiguous(s string) bool { if s == "" { return false } - return yamlPlainScalarNotString.MatchString(s) + return yamlNonStringWords[s] || yamlNonStringNumber.MatchString(s) } -// quoteAmbiguousYAMLScalars rebuilds v with every ambiguous string wrapped so -// the encoder is forced to quote it. Containers are rebuilt as map[string]any / -// []any because a typed container cannot hold the wrapper; unambiguous values -// are carried through untouched, so the emitted document is unchanged apart -// from the quotes that were missing. func quoteAmbiguousYAMLScalars(v any) any { if v == nil { return nil @@ -89,7 +74,7 @@ func quoteAmbiguousYAMLScalars(v any) any { out[i] = quoteAmbiguousYAMLScalars(rv.Index(i).Interface()) } return out - case reflect.Ptr, reflect.Interface: + case reflect.Pointer, reflect.Interface: if rv.IsNil() { return v } @@ -99,8 +84,6 @@ func quoteAmbiguousYAMLScalars(v any) any { } } -// marshalClashYAML serializes a Clash config, keeping string values typed as -// strings in the output. func marshalClashYAML(config any) ([]byte, error) { return yaml.Marshal(quoteAmbiguousYAMLScalars(config)) } diff --git a/internal/sub/clash_yaml_test.go b/internal/sub/clash_yaml_test.go index df26784a4..710fc01f9 100644 --- a/internal/sub/clash_yaml_test.go +++ b/internal/sub/clash_yaml_test.go @@ -7,10 +7,6 @@ import ( "github.com/goccy/go-yaml" ) -// The encoder's own parser reads every one of these back as a string, so a -// round-trip through it proves nothing. What matters is the emitted text: a -// plain scalar that the YAML resolution rules turn into a number, bool, null -// or date is what breaks a Clash core, so those must carry quotes. func TestAmbiguousScalarsAreQuoted(t *testing.T) { tests := []struct { name string @@ -54,7 +50,6 @@ func TestAmbiguousScalarsAreQuoted(t *testing.T) { if !tt.mustQuote && quoted { t.Errorf("%q needs no quoting, got %q", tt.value, got) } - // Whatever the quoting decision, the document must still parse. var decoded map[string]any if err := yaml.Unmarshal(out, &decoded); err != nil { t.Fatalf("output must stay parseable: %v\n%s", err, out) @@ -63,8 +58,6 @@ func TestAmbiguousScalarsAreQuoted(t *testing.T) { } } -// The value in the report: unquoted, YAML reads 2351e1 as 23510, mihomo hex- -// decodes an odd-length "23510" and the provider drops to zero nodes (#6104). func TestClashShortIDIsQuotedInOutput(t *testing.T) { out, err := marshalClashYAML(map[string]any{ "proxies": []any{map[string]any{ @@ -80,8 +73,6 @@ func TestClashShortIDIsQuotedInOutput(t *testing.T) { } } -// A password or pre-shared key of the same shape reaches the output the same -// way, so the guard is not specific to short-id. func TestAmbiguousPasswordIsQuoted(t *testing.T) { out, err := marshalClashYAML(map[string]any{ "proxies": []any{map[string]any{ @@ -101,8 +92,6 @@ func TestAmbiguousPasswordIsQuoted(t *testing.T) { } } -// Values that are unambiguous must not pick up noise quoting, so the emitted -// document stays byte-identical to what the encoder produced before. func TestUnambiguousScalarsAreUnchanged(t *testing.T) { config := map[string]any{ "port": 7890, @@ -125,7 +114,6 @@ func TestUnambiguousScalarsAreUnchanged(t *testing.T) { } } -// A quote inside the value must not terminate the scalar. func TestQuotedScalarEscapesQuotes(t *testing.T) { out, err := marshalClashYAML(map[string]any{"password": "1e2'3"}) if err != nil { diff --git a/internal/sub/remark_vars_test.go b/internal/sub/remark_vars_test.go index 7d66eb333..21235980f 100644 --- a/internal/sub/remark_vars_test.go +++ b/internal/sub/remark_vars_test.go @@ -624,9 +624,6 @@ func TestUsageOnFirstLinkOnly_SingleBracket(t *testing.T) { } } -// Every link of a subscription carries the client's identity, because that is -// what tells one imported profile from another in the client app. Only the -// usage block, which is identical on all of them, is first-link-only (#6098). func TestEmailOnEveryLink(t *testing.T) { s := &SubService{ remarkTemplate: "{{INBOUND}} {{EMAIL}}|📊{{TRAFFIC_LEFT}}", diff --git a/internal/web/job/xray_traffic_inform_test.go b/internal/web/job/xray_traffic_inform_test.go index 9941d3d9d..b94b8977b 100644 --- a/internal/web/job/xray_traffic_inform_test.go +++ b/internal/web/job/xray_traffic_inform_test.go @@ -10,9 +10,6 @@ import ( "github.com/valyala/fasthttp" ) -// stallingListener accepts connections, reads whatever is sent and then never -// answers and never closes — the receiver shape that used to hold the traffic -// job open indefinitely (#6115). func stallingListener(t *testing.T) (addr string, release func()) { t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") @@ -65,7 +62,6 @@ func TestExternalInformClient_StalledReceiverTimesOut(t *testing.T) { if !errors.Is(err, fasthttp.ErrTimeout) { t.Errorf("want a timeout error, got %v", err) } - // The whole point is that the call cannot outlast the 5s poll cadence. if elapsed > externalInformTimeout+2*time.Second { t.Errorf("call took %v, must be bounded by %v", elapsed, externalInformTimeout) } @@ -80,8 +76,6 @@ func TestExternalInformClient_HasDeadlines(t *testing.T) { } } -// An idle panel posts nothing, which keeps a misbehaving receiver out of the -// job's path entirely for most ticks. func TestInformSkippedWhenNothingToReport(t *testing.T) { j := &XrayTrafficJob{} done := make(chan struct{}) diff --git a/internal/web/job/xray_traffic_job.go b/internal/web/job/xray_traffic_job.go index b071d541d..6e6baaa89 100644 --- a/internal/web/job/xray_traffic_job.go +++ b/internal/web/job/xray_traffic_job.go @@ -29,19 +29,8 @@ type XrayTrafficJob struct { // refetch for the rest. const clientStatsSnapshotMaxClients = 5000 -// externalInformTimeout bounds the traffic-notify POST. Run() is scheduled -// every 5s under cron.SkipIfStillRunning, so an unbounded call to a receiver -// that accepts the connection and then stalls does not merely delay one -// notification: it holds the job, and every following tick is skipped for as -// long as the stall lasts. AddTraffic — quota enforcement, auto-renew — and -// the online/websocket work all sit in that same tick (#6115). const externalInformTimeout = 3 * time.Second -// externalInformClient is kept separate from fasthttp's shared default client -// so this endpoint's timeouts and connection handling cannot be influenced by, -// or influence, any other caller. Idempotent-call retries stay off on purpose: -// the payload carries per-tick deltas, so an attempt that reached the receiver -// but failed on the response leg would be counted twice if it were resent. var externalInformClient = &fasthttp.Client{ ReadTimeout: externalInformTimeout, WriteTimeout: externalInformTimeout, diff --git a/internal/web/service/global_traffic_test.go b/internal/web/service/global_traffic_test.go index 007c6714b..34fb1b3e4 100644 --- a/internal/web/service/global_traffic_test.go +++ b/internal/web/service/global_traffic_test.go @@ -91,9 +91,6 @@ func TestDepletedCond_ProbeGuard(t *testing.T) { } } -// A master that stopped pushing leaves its last snapshot behind forever. Those -// frozen counters must not keep enforcing quota, or a client well inside its -// limit is disabled on every traffic poll with no way back (#6113). func TestStaleGlobalTraffic_Ignored(t *testing.T) { db := initTrafficTestDB(t) svc := &InboundService{} @@ -112,7 +109,6 @@ func TestStaleGlobalTraffic_Ignored(t *testing.T) { } t.Run("stale row alone neither enforces nor selects the cross-panel predicate", func(t *testing.T) { - // 200 of 1000 used locally, 1900 reported by a master gone for a day. seedClientRow(t, "cap", 1, 100, 100, 1000) seedStaleGlobal(t, "dead-master", "cap", 1000, 900) @@ -138,8 +134,6 @@ func TestStaleGlobalTraffic_Ignored(t *testing.T) { }) t.Run("a live master still enforces alongside the stale row", func(t *testing.T) { - // This is the #6113 shape: one dead master frozen over quota, one live - // master well under it. Only the live one may decide. if err := svc.AcceptGlobalTraffic("live-master", []*xray.ClientTraffic{{Email: "cap", Up: 1, Down: 1}}); err != nil { t.Fatalf("AcceptGlobalTraffic: %v", err) } @@ -152,7 +146,6 @@ func TestStaleGlobalTraffic_Ignored(t *testing.T) { t.Fatalf("the live master reports usage well under quota, disabled %d", count) } - // And once the live master reports real depletion, it takes effect. if err := svc.AcceptGlobalTraffic("live-master", []*xray.ClientTraffic{{Email: "cap", Up: 600, Down: 500}}); err != nil { t.Fatalf("AcceptGlobalTraffic: %v", err) } diff --git a/internal/web/service/inbound_disable.go b/internal/web/service/inbound_disable.go index c18cdd4b1..1976350d9 100644 --- a/internal/web/service/inbound_disable.go +++ b/internal/web/service/inbound_disable.go @@ -49,15 +49,6 @@ func (s *InboundService) disableInvalidInbounds(tx *gorm.DB) (bool, int64, error return needRestart, count, err } -// globalTrafficFreshWindow bounds how long a pushed client_global_traffics row -// stays authoritative. Masters refresh their rows every nodeGlobalPushInterval -// (30s), so a row older than this belongs to a master that stopped pushing — -// decommissioned, reinstalled under a new GUID, or detached from this node. -// Such a row keeps its last-seen counters forever, and without this bound a -// long-dead master's numbers permanently trip the cross-panel quota check and -// disable clients that are nowhere near their limit (#6113). The window is far -// wider than any real push gap, so a master that is merely unreachable for a -// while keeps enforcing. const globalTrafficFreshWindow = 24 * time.Hour func globalTrafficFreshSince() int64 { diff --git a/internal/web/service/xray_config_clients_test.go b/internal/web/service/xray_config_clients_test.go index 1e59f9de7..9ad61a3f1 100644 --- a/internal/web/service/xray_config_clients_test.go +++ b/internal/web/service/xray_config_clients_test.go @@ -28,9 +28,6 @@ func seedVlessInbound(t *testing.T, tag string, port int, clients []model.Client } } -// ClientRecord.Enable carries gorm:"default:true", so a false on insert is -// replaced by the default. Production disables through an UPDATE -// (disableInvalidClients); do the same here. func disableClients(t *testing.T, emails ...string) { t.Helper() if err := database.GetDB().Model(&model.ClientRecord{}). @@ -62,9 +59,6 @@ func emittedClients(t *testing.T, tag string) (any, bool) { return nil, false } -// An inbound whose clients are all filtered out must hand xray-core an empty -// array. It used to emit "clients": null, which the panel treats as invalid -// data elsewhere and coerces to [] at startup (#6117). func TestGetXrayConfig_EmptyClientListIsArrayNotNull(t *testing.T) { seedVlessInbound(t, "vless-empty", 43101, []model.Client{ {Email: "gone@x", ID: "11111111-1111-1111-1111-111111111111", Enable: true}, @@ -87,8 +81,6 @@ func TestGetXrayConfig_EmptyClientListIsArrayNotNull(t *testing.T) { } } -// The disabled/depleted filter must keep working — an empty array is only -// correct because those clients are genuinely excluded. func TestGetXrayConfig_EnabledClientsStillEmitted(t *testing.T) { seedVlessInbound(t, "vless-mixed", 43102, []model.Client{ {Email: "live@x", ID: "22222222-2222-2222-2222-222222222222", Enable: true},