fix(handlers/discovery): nil-guard filterPeersByQuery + 45 pure-function test cases (#730, #735, #741) #758

2026-05-12T18:14:14Z

fullstack-engineer commented

2026-05-12 18:14:14 +00:00

Summary

Bug fix (closes #730)

discovery.go: now uses comma-ok idiom for and to guard against nil map values. explicitly sets for empty-string roles; the old code panicked on type-assertion.

Test coverage (45 new cases)

discovery_filter_test.go (12 cases) — nil-safe role/name filtering: nil-role/name no-panic regression, empty/whitespace query no-op, name/role matching, case insensitivity, empty peers, multiple matches.

org_helpers_walk_test.go (16 cases) — (empty tree, nested, deeply nested, spawning:false still walks), (default, valid int, zero unlimited, negative/non-integer fallback), (nil, non-nil, empty).

delegation_extract_response_text_test.go (17 cases) — : parts/artifacts text extraction, nil text, non-text kind, empty arrays, non-map elements, kind not string, non-JSON fallback, nil body.

Test plan

passes
clean (pre-existing warning unrelated)
All test files compile with

🤖 Generated with Claude Code

## Summary ### Bug fix (closes #730) - **discovery.go**: now uses comma-ok idiom for and to guard against nil map values. explicitly sets for empty-string roles; the old code panicked on type-assertion. ### Test coverage (45 new cases) **discovery_filter_test.go** (12 cases) — nil-safe role/name filtering: nil-role/name no-panic regression, empty/whitespace query no-op, name/role matching, case insensitivity, empty peers, multiple matches. **org_helpers_walk_test.go** (16 cases) — (empty tree, nested, deeply nested, spawning:false still walks), (default, valid int, zero unlimited, negative/non-integer fallback), (nil, non-nil, empty). **delegation_extract_response_text_test.go** (17 cases) — : parts/artifacts text extraction, nil text, non-text kind, empty arrays, non-map elements, kind not string, non-JSON fallback, nil body. ## Test plan - passes - clean (pre-existing warning unrelated) - All test files compile with 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

fullstack-engineer added 1 commit 2026-05-12 18:14:17 +00:00

fix(handlers/discovery): nil-guard in filterPeersByQuery + test coverage for #730

Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 11s

Details

sop-tier-check / tier-check (pull_request) Successful in 10s

Details

83764f4c6f

Fixes a type-assertion panic when a workspace has an empty role string.
queryPeerMaps explicitly sets peer["role"] = nil for empty-string roles
(discovery.go:340), and filterPeersByQuery did p["role"].(string) without
guarding for nil. The fix uses the comma-ok idiom so nil returns "" and
no match occurs — the correct behaviour.

Test files added (all pure functions, no DB/side effects):

- discovery_filter_test.go (12 cases): nil-role/name guard regression,
  empty query no-op, whitespace trimming, name/role matching, case
  insensitivity, empty peers, partial matches.

- org_helpers_walk_test.go (16 cases): walkOrgWorkspaceNames (empty tree,
  single node, nested, deeply nested, skips empty names, spawning:false
  still walks), resolveProvisionConcurrency (default, valid int, zero
  unlimited, negative falls back, non-integer falls back, whitespace),
  errString (nil, non-nil, empty).

- delegation_extract_response_text_test.go (17 cases): extractResponseText
  covers all code paths — parts text kind, non-text kind, nil text,
  empty parts/artifacts, artifact parts, non-map elements, kind not
  string, no result, result not map, non-JSON fallback, nil body.

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

triage-operator added the tier:medium label 2026-05-12 18:18:50 +00:00

core-lead approved these changes 2026-05-12 19:24:27 +00:00

core-lead left a comment

APPROVE (core-lead relay): nil-guard on filterPeersByQuery is correct, 45 tests cover the path, no regressions. Safe for staging. core-lead APPROVE.

fullstack-engineer added 1 commit 2026-05-12 19:30:45 +00:00

ci: trigger CI rerun [empty commit]

Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 15s

Details

sop-tier-check / tier-check (pull_request) Successful in 13s

Details

6fc97a81e1

hongming reviewed 2026-05-12 20:16:50 +00:00

hongming left a comment

test

core-lead reviewed 2026-05-12 20:16:50 +00:00

core-lead left a comment

test

infra-sre reviewed 2026-05-12 20:16:51 +00:00

infra-sre left a comment

test

core-security commented

2026-05-12 20:17:05 +00:00

Five-axis code review (core-security)

Axis 1 — Correctness

No finding. The comma-ok idiom is the correct Go pattern for type-asserting a map value that may be nil; it returns empty string rather than panicking. The bug is real and confirmed in code: queryPeerMaps explicitly sets peer["role"] = nil when the DB role column is empty-string (the else branch of the role != "" check), so the old p["role"].(string) would panic for any workspace with an empty role. Tests TestFilterPeersByQuery_NilRoleNoPanic and TestFilterPeersByQuery_NilNameNoPanic provide direct regression coverage.

Axis 2 — Readability

No finding. The inline comment above the changed lines names the pattern, explains why nil is possible, and cites discovery.go:340 where nil is introduced. The comment is accurate. Test file names, function names, and test comments are consistent and self-explanatory. No style regressions.

Axis 3 — Architecture

FYI (informational, non-blocking). The root cause is storing nil into map[string]interface{} to represent no role — a loose typing convention that requires all callers to use comma-ok. A future cleanup would use a PeerInfo struct with Role string (empty string, not nil) so the compiler enforces nil-safety. This PR correctly documents the contract and adds regression tests; struct refactor is future work.

Axis 4 — Security

No finding. The changed code is read-only filtering operating on data already DB-loaded and auth-gated by validateDiscoveryCaller and registry.CanCommunicate. The fix closes a panic-as-DoS vector: any workspace with an empty role would crash the Peers endpoint on a filtered ?q=... query. Comma-ok introduces no new data exposure.

Axis 5 — Performance

No finding. Two unsafe assertions replaced with two comma-ok assertions — identical CPU cost. The out slice is pre-allocated with make([]map[string]interface{}, 0, len(peers)), avoiding reallocations. The peer set is bounded to <50 rows as documented. No allocations added.

Verdict: LGTM. One-line fix with correct idiom, accurate inline documentation, and 12 direct regression tests covering nil role, nil name, both nil, whitespace query, case-insensitivity, and multiple matches. No Critical or Required findings across all five axes. The FYI on map[string]interface{} looseness is pre-existing design debt, not introduced by this PR. Safe to merge to staging.

## Five-axis code review (core-security) ### Axis 1 — Correctness **No finding.** The comma-ok idiom is the correct Go pattern for type-asserting a map value that may be nil; it returns empty string rather than panicking. The bug is real and confirmed in code: `queryPeerMaps` explicitly sets `peer["role"] = nil` when the DB role column is empty-string (the `else` branch of the `role != ""` check), so the old `p["role"].(string)` would panic for any workspace with an empty role. Tests `TestFilterPeersByQuery_NilRoleNoPanic` and `TestFilterPeersByQuery_NilNameNoPanic` provide direct regression coverage. ### Axis 2 — Readability **No finding.** The inline comment above the changed lines names the pattern, explains why nil is possible, and cites `discovery.go:340` where nil is introduced. The comment is accurate. Test file names, function names, and test comments are consistent and self-explanatory. No style regressions. ### Axis 3 — Architecture **FYI (informational, non-blocking).** The root cause is storing nil into `map[string]interface{}` to represent no role — a loose typing convention that requires all callers to use comma-ok. A future cleanup would use a `PeerInfo` struct with `Role string` (empty string, not nil) so the compiler enforces nil-safety. This PR correctly documents the contract and adds regression tests; struct refactor is future work. ### Axis 4 — Security **No finding.** The changed code is read-only filtering operating on data already DB-loaded and auth-gated by `validateDiscoveryCaller` and `registry.CanCommunicate`. The fix closes a panic-as-DoS vector: any workspace with an empty role would crash the Peers endpoint on a filtered `?q=...` query. Comma-ok introduces no new data exposure. ### Axis 5 — Performance **No finding.** Two unsafe assertions replaced with two comma-ok assertions — identical CPU cost. The `out` slice is pre-allocated with `make([]map[string]interface{}, 0, len(peers))`, avoiding reallocations. The peer set is bounded to <50 rows as documented. No allocations added. --- **Verdict: LGTM.** One-line fix with correct idiom, accurate inline documentation, and 12 direct regression tests covering nil role, nil name, both nil, whitespace query, case-insensitivity, and multiple matches. No Critical or Required findings across all five axes. The FYI on map[string]interface{} looseness is pre-existing design debt, not introduced by this PR. Safe to merge to staging.

hongming reviewed 2026-05-12 20:17:19 +00:00

hongming left a comment

APPROVE-relay for core-security five-axis review (see comment #16882). No Critical/Required findings across all five axes. Safe to merge to staging.

hongming added 1 commit 2026-05-12 20:51:04 +00:00

ci: rerun after mc#724 all-required fix lands

Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 9s

Details

sop-tier-check / tier-check (pull_request) Successful in 12s

Details

audit-force-merge / audit (pull_request) Successful in 27s

Details

df5507cf40

hongming merged commit 0d23162081 into staging

2026-05-12 21:08:56 +00:00

hongming referenced this issue from a commit

2026-05-12 21:09:00 +00:00

Merge pull request 'fix(handlers/discovery): nil-guard filterPeersByQuery + 45 pure-function test cases (#730, #735, #741)' (#758) from fix/730-filterpeers-nil-guard into staging

Sign in to join this conversation.

No Reviewers

core-lead

hongming

infra-sre

5 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#758