fix(config): add integer conversion safety checks

[lml2468]

Summary

Fix CodeQL-reported integer conversion issues in config/ package (GO-S1040).

Changes

1. GetEnvInt — config.go:978

• Changed strconv.ParseInt(v, 10, 64) → strconv.ParseInt(v, 10, 0)
• bitSize=0 validates against platform int width, making the subsequent int(i) always safe
• On 64-bit: accepts full int64 range. On 32-bit: rejects values > math.MaxInt32

2. SendMessageWithResult — msg.go:160

• Added bounds check before uint32(messageSeq) cast
• Returns explicit error if WuKongIM returns message_seq outside [0, math.MaxUint32]
• Prevents silent truncation that could cause message ordering/dedup issues

3. Avatar path functions — config.go:881/887/893

• GetGroupAvatarFilePath, GetCommunityAvatarFilePath, GetCommunityCoverFilePath
• Guard Avatar.Partition against non-positive values (fallback to 1)
• Prevents uint32 wrap on negative config values and division-by-zero on zero

Verification

• go build ./... ✅
• go vet ./... ✅
• go test ./config/... ✅

Not changed (false positives)

• zapcore.Level → int (int8 underlying, always safe)
• seqStep int64 → int (hardcoded constant 1000, always safe)
• GetContentType int64 → int (enum values, always within range)

[lml2468]

CI is red, so I stopped the review before code analysis.

Blocking items

1. CI checks: check-sprint / check-sprint and welcome / welcome are failing. Please fix or explain these failures before code review proceeds.

Non-blocking notes

• Test, Vet, and analyze / CodeQL (go) were still pending at gate-check time.
• I did not inspect the diff because the requested gate-check rule says to stop on red CI.

Highlights

• Build and Detect changed paths are passing.

[lml2468]

[COMMENT]

Note: Cannot APPROVE — PR author and reviewer share the same GitHub account (lml2468). Requesting a second maintainer to approve.

Clean, well-scoped fix for CodeQL GO-S1040 integer conversion warnings. All three change areas are correct:

1. Avatar partition guard (config.go:881/887/893) ✅
partition <= 0 fallback to 1 prevents both division-by-zero (partition=0) and uint32 wrapping (negative values). Sound defensive fix.

2. GetEnvInt bitSize change (config.go:978) ✅
strconv.ParseInt(v, 10, 0) validates against platform int width at parse time, making the subsequent int(i) cast provably safe. Cleaner than the previous bitSize=64 approach.

3. SendMessageWithResult bounds check (msg.go:160) ✅
Explicit range validation before uint32(messageSeq) cast prevents silent truncation. Error message is clear and actionable.

The false-positive analysis in the PR description is also well-reasoned — zapcore.Level, seqStep, and GetContentType are indeed safe conversions.

Nit (non-blocking): The three avatar functions share identical partition-guard logic — could be extracted into a small helper like func safePartition(p int) uint32, but not worth blocking for.

[yujiawei]

Code Review — PR #44 (octo-lib)

Summary

This PR addresses three CodeQL GO-S1040 integer-conversion findings in the config/ package:

1. GetEnvInt — switch strconv.ParseInt bitSize from 64 to 0 so the value is validated against the platform int width before int(i) is taken.
2. SendMessageWithResult — explicit range check on message_seq before casting int64 → uint32.
3. Three *AvatarFilePath helpers — clamp Avatar.Partition to 1 if it is non-positive, preventing both division-by-zero and uint32 wrap on negative values.

Changes are narrowly scoped, the description correctly explains both what changed and which CodeQL hits were deliberately left as false positives. Build, vet, and go test ./config/... pass locally on the PR head (799ad5d).

Verification

Check	Result	Evidence
GetEnvInt truncation fixed	✅	config/config.go:990 — ParseInt(v, 10, 0) rejects values outside the platform int range before int(i) at L995. On 64-bit hosts (typical), behavior is identical to before; on 32-bit hosts, oversized inputs are now rejected explicitly instead of silently truncating to a wrapped value.
message_seq cast guarded	✅	config/msg.go:159-161 — explicit `< 0
Avatar partition guards	✅	config/config.go:881-885, 891-895, 901-905 — partition := c.Avatar.Partition; if partition <= 0 { partition = 1 } covers both the panic case (%0) and the wrap case (uint32(-1)). Avatar.Partition is declared int at config/config.go:163, default 100 at config/config.go:404, so the fallback only matters under explicit misconfiguration.
Build / vet / tests	✅	go build ./..., go vet ./..., and go test ./config/... all pass on 799ad5d.
Scope discipline (false-positive justifications)	✅	The PR description correctly identifies zapcore.Level → int (int8 underlying), seqStep (compile-time constant), and GetContentType (bounded enum) as safe, and leaves them untouched.

Findings

P0 / P1 — Blockers

None.

P2 / Nits / Suggestions (non-blocking)

1. Silent fallback on misconfigured Avatar.Partition — config/config.go:881-885 (and the two siblings). When Partition <= 0, the code transparently falls back to 1. This is the right runtime behavior, but operators get no signal that their config is broken; every avatar path will route to bucket 0. Consider one of:

   • a one-shot log.Warn at config-load time (e.g. in the section that calls c.getInt("avatar.partition", ...) at config/config.go:703) when the resulting value is <= 0; or
   • clamping once at load time rather than per-call, so the three helpers stay simple and the warning lives in one place.
     This also avoids the (tiny) per-call branch on a hot-ish path.

2. No regression tests added. The repo already has config/msg_test.go, config/config_s3_test.go, etc., so the harness is in place. Worth adding:

   • A GetEnvInt case that sets an env var to a value > math.MaxInt32 (only meaningful as a behavior assertion; can be 64-bit-only via build tag or just assert the warn-and-default path with a clearly out-of-range value).
   • A table-driven test for GetGroupAvatarFilePath / GetCommunityAvatarFilePath / GetCommunityCoverFilePath covering Partition = 0, Partition = -1, and Partition = 100, asserting no panic and that the 0/-1 cases produce the bucket-0 path.
   • Optional: a SendMessageWithResult test using httptest returning message_seq = 5_000_000_000 to exercise the new error branch.

3. SendMessageWithResult error language consistency. The new error message at config/msg.go:160 is in Chinese, matching surrounding messages in this file — fine and intentional. Flagging only because the rest of the changed surface (GetEnvInt warning) uses English; not a request to change anything, just an observation.

4. GetEnvInt64 is unchanged. config/config.go:976 still uses bitSize=64, which is correct since the return type is int64. No action — noting it because a future reviewer might wonder why the two functions diverge after this PR.

Additional observations (out of scope for this PR)

• Several other uint32(intExpr) and similar conversions exist elsewhere in config/msg.go (e.g. the MessageSeq uint32 struct fields populated from JSON via gjson .Int() in adjacent flows). The PR description rightly limits scope to the CodeQL-flagged sites; a follow-up sweep could audit any remaining .Int() → uint32 paths the linter does not currently flag, but that belongs in a separate change.
• The partition <= 0 guard pattern is duplicated three times. Extracting a tiny helper (c.avatarPartition() returning a sanitized uint32) would be a clean follow-up — but it is not worth blocking this PR over, especially given suggestion (1) above which would move the clamp to config-load time and remove the duplication entirely.

Verdict

APPROVED. The three fixes are correct, narrowly scoped, and address the CodeQL findings without altering observable behavior on 64-bit hosts under normal configuration. The suggestions above are quality-of-life follow-ups, not merge blockers.

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/octo-lib and correctly addresses integer conversion safety in the config package.

💬 Non-blocking

🟡 Warning: The new boundary behavior is not covered by targeted tests. Consider adding tests for GetEnvInt overflow fallback, non-positive Avatar.Partition, and SendMessageWithResult rejecting message_seq < 0 or > math.MaxUint32. Relevant areas: config/config.go:880, config/config.go:890, config/config.go:900, config/config.go:985, config/msg.go:156.

✅ Highlights

🔵 Suggestion: The avatar partition fallback is duplicated three times in config/config.go:881, config/config.go:891, and config/config.go:901. A small helper could reduce repetition, but the current code is clear and acceptable.

The message_seq range check in config/msg.go:159 prevents silent truncation before conversion to uint32.

The strconv.ParseInt(v, 10, 0) change in config/config.go:990 is appropriate for validating values against the platform int width before casting.

Verification performed: go test ./config/..., go test ./..., go vet ./..., and git diff --check main...HEAD all passed.

[lml2468]

LGTM — focused, correct fix for CodeQL GO-S1040 findings.

Reviewed changes:

1. GetEnvInt (config.go:987) — bitSize=64 → bitSize=0: correct. bitSize=0 validates against platform int width, so the subsequent int(i) cast is guaranteed safe. Previously on 32-bit, values > MaxInt32 would silently truncate.

2. SendMessageWithResult (msg.go:159-161) — bounds check before uint32(messageSeq): correct. gjson.Int() returns int64; without the guard, negative or >MaxUint32 values would silently wrap. Error message includes the offending value — good for debugging.

3. Avatar partition functions (config.go:881-905) — guard Partition <= 0 with fallback to 1: correct. Prevents both division-by-zero panic (Partition==0) and uint32 wrap on negative values.

Verification:

• No remaining unsafe uint32(c.Avatar.Partition) patterns in codebase
• MsgSendResp.MessageSeq is uint32 — bounds check matches target type
• Build ✅, Test ✅, Vet ✅

Non-blocking:

• The three avatar functions share identical partition-guard logic. A small helper like func (c *Config) safePartition() uint32 could DRY this up in a follow-up — not blocking for this PR.

[github-actions]

This pull request has been automatically marked as stale due to inactivity. Please add an update or it will be closed.