Skip to content

Commit 4a5ebf0

Browse files
committed
Fix lookup Go quality findings
1 parent 489494d commit 4a5ebf0

2 files changed

Lines changed: 57 additions & 5 deletions

File tree

‎.agents/sow/done/SOW-0014-20260603-maintainability-hotspots.md‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
Status: completed
66

7-
Sub-state: Windows Rust CI compile regression repaired and validated.
7+
Sub-state: residual Go quality findings repaired and CodeQL false-positive alert dismissed.
88

99
## Requirements
1010

@@ -1587,3 +1587,57 @@ Artifact impact:
15871587
- End-user/operator docs: no public docs change needed.
15881588
- End-user/operator skills: no exported integration guidance change needed.
15891589
- SOW lifecycle: this completed SOW was reopened from `done/` to `current/` for a true regression and will be completed again with the repair commit.
1590+
1591+
## Regression - 2026-06-05
1592+
1593+
What broke:
1594+
1595+
- GitHub AI quality findings reported two issues in `src/go/pkg/netipc/protocol/lookup_common.go` after the maintainability split:
1596+
- exported type `LookupLabelView` lacked a Go doc comment.
1597+
- `finishLookupResponse()` converted `itemCount` through `checkedInt(uint64(itemCount))` after the stronger `checkedInt(uint64(itemCount) * uint64(LookupDirEntrySize))` guard had already succeeded, making the second checked conversion unreachable.
1598+
- GitHub CodeQL reported one open `cpp/stack-address-escape` alert:
1599+
- alert `7641`
1600+
- `src/libnetdata/netipc/src/service/netipc_service_posix_server.c:273`
1601+
- `sctx->server = server`
1602+
1603+
Evidence:
1604+
1605+
- `src/go/pkg/netipc/protocol/lookup_common.go:19` exported `LookupLabelView` without a doc comment.
1606+
- `src/go/pkg/netipc/protocol/lookup_common.go:447` already validates `itemCount * LookupDirEntrySize` fits in `int`; since `LookupDirEntrySize` is `8`, a subsequent bare `itemCount` conversion cannot fail after that guard.
1607+
- GitHub code scanning API reported alert `7641` with rule `cpp/stack-address-escape`, path `src/libnetdata/netipc/src/service/netipc_service_posix_server.c`, line `273`.
1608+
1609+
Root-cause model:
1610+
1611+
- The Go findings are straightforward hygiene issues introduced or exposed by the lookup common split.
1612+
- The CodeQL finding is a lifecycle false positive: the session context stores the caller-owned managed-server pointer while the managed server owns the session list, and `nipc_server_drain()` / `nipc_server_destroy()` join all session threads before releasing the session array. CodeQL's own query help says this pattern can be safe if the stored address is never used after the function returns, but it is not generally recommended without clear lifetime control.
1613+
1614+
Repair plan:
1615+
1616+
- Add a Go doc comment for `LookupLabelView`.
1617+
- Replace the redundant checked conversion with `count := int(itemCount)` after the existing directory-size guard.
1618+
- Dismiss CodeQL alert `7641` as a false positive with the managed-server lifecycle reason; do not disable the rule.
1619+
1620+
Repair implemented:
1621+
1622+
- `LookupLabelView` now has a Go doc comment beginning with the exported identifier name.
1623+
- `finishLookupResponse()` now uses `count := int(itemCount)` after the existing `itemCount * LookupDirEntrySize` guard.
1624+
- GitHub CodeQL alert `7641` was dismissed as a false positive, with the managed-server/session lifetime reason recorded in the alert dismissal comment.
1625+
1626+
Validation:
1627+
1628+
- `gofmt` was run on `src/go/pkg/netipc/protocol/lookup_common.go`.
1629+
- `go test -C src/go ./pkg/netipc/protocol` passed.
1630+
- `go test -C src/go ./...` passed.
1631+
- `codacy-analysis analyze --files src/go/pkg/netipc/protocol/lookup_common.go --output-format json` passed with 0 issues and 0 errors.
1632+
- `git diff --check` passed.
1633+
- GitHub code scanning open-alert query returned 0 open alerts after dismissing alert `7641`.
1634+
- `bash .agents/sow/audit.sh` passed after moving this completed SOW back to `done/`, with status and directory consistent.
1635+
1636+
Artifact impact:
1637+
1638+
- `AGENTS.md`: no workflow or guardrail change.
1639+
- Runtime project skills: no reusable workflow change.
1640+
- Specs: no protocol/API behavior change; the Go doc comment describes existing type purpose.
1641+
- End-user/operator docs: no docs change needed.
1642+
- End-user/operator skills: no exported integration guidance change needed.
1643+
- SOW lifecycle: this completed SOW was reopened from `done/` to `current/` for residual findings and is completed again with the repair commit and alert disposition.

‎src/go/pkg/netipc/protocol/lookup_common.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const (
1616
LookupLabelEntrySize = 16
1717
)
1818

19+
// LookupLabelView represents a key-value label pair view in the lookup wire format.
1920
type LookupLabelView struct {
2021
Key CStringView
2122
Value CStringView
@@ -448,10 +449,7 @@ func finishLookupResponse(buf []byte, hdrSize int, itemCount uint32, dataOffset
448449
if !ok {
449450
return 0
450451
}
451-
count, ok := checkedInt(uint64(itemCount))
452-
if !ok {
453-
return 0
454-
}
452+
count := int(itemCount)
455453
finalPackedStart, ok := checkedAddInt(hdrSize, dirSize)
456454
if !ok {
457455
return 0

0 commit comments

Comments
 (0)