Skip to content

Fixes bug in Registries processing#97

Merged
anushkavirgaonkar merged 1 commit into
mainfrom
bugfix/registries
Jun 26, 2026
Merged

Fixes bug in Registries processing#97
anushkavirgaonkar merged 1 commit into
mainfrom
bugfix/registries

Conversation

@MarioBartolome

Copy link
Copy Markdown
Contributor
  • Fix three bugs in CIDR discovery pipeline that silently dropped RIR handles
  • ARIN's WHOIS REST API returns single JSON objects instead of arrays for 1-result responses — added custom unmarshaler to handle both
  • Edgar plugin sets registry: "unknown" but enrichWithHandles only fanned out on empty string — now handles both
  • Added LACNIC to the registry fan-out list (was missing)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review

No critical issues — LGTM pending human review.

Note: I attempted go test ./pkg/plugins/cidrs ./pkg/runner, but this read-only sandbox cannot create /home/runner/go for the module cache, so tests could not run here.


Reviewed by Codex (gpt-5.5)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Review

Critical issues

None.

Security

No security concerns flagged.

Test coverage

Tests are present and well-targeted — ArinRefs.UnmarshalJSON covers array/single-object/null cases, and enrichWithHandles covers both empty and unknown registry fan-out (now including lacnic).

Verified the three changes are internally consistent: edgar.go emits registry: "unknown" (now matched at run.go:360), lacnic.go consumes the lacnic_handles metaKey (so the new fan-out entry is live, not dead), and the array-first/object-fallback unmarshaler handles null correctly (array unmarshal succeeds into nil).

No critical issues — LGTM pending human review.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemini Review

Critical Issues

None.

Security

No security concerns flagged.

Suggestions

  • Improve UnmarshalJSON error transparency (pkg/plugins/cidrs/reverse_rir.go:341): If data is a JSON array but fails to unmarshal (e.g., due to an unexpected type in an array element), the fallback attempt will try to unmarshal that array into a single object struct. This second attempt will fail and return a confusing error (cannot unmarshal array into Go value...), masking the actual reason the array parsing failed. Consider checking the shape of the data first (e.g., bytes.TrimSpace(data)[0] == '[') to determine the correct target and apply only that unmarshaling strategy.
  • Match standard json.Unmarshal null behavior (pkg/plugins/cidrs/reverse_rir.go:341): Encountering JSON null currently sets *r = nil. While perfectly safe here since the parent structs are newly allocated, standard json.Unmarshal treats null as a no-op that preserves any existing slice elements. Adding an explicit if string(data) == "null" { return nil } at the top of the function aligns this custom unmarshaler strictly with standard library semantics and prevents subtle data-loss bugs if the struct is ever reused.

Reviewed by Gemini (gemini-3.1-pro-preview)

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR adds ArinRefs, a custom JSON unmarshal type that accepts either an array or a single ArinRef object, and updates ARIN org/customer/net/asn response structs to use it. It also expands enrichWithHandles so empty or "unknown" registry values broadcast to arin, ripe, apnic, afrinic, and lacnic, with tests covering the new JSON shapes and registry cases.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/registries

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/plugins/cidrs/reverse_rir.go (1)

336-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

UnmarshalJSON logic is sound; two small nits.

The array-first/object-fallback handles all four tested shapes (array, single object, null, empty) correctly. Two minor points:

  • The doc comment says standard unmarshal "fails silently" on object→[]T; it actually returns an error (which was presumably ignored upstream). Tighten the wording to avoid future confusion.
  • On the both-fail path you return the single-object error, discarding the array error. For a genuinely malformed array, the surfaced error will be misleading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/plugins/cidrs/reverse_rir.go` around lines 336 - 355, Update the doc
comment on ArinRefs.UnmarshalJSON to avoid saying standard json.Unmarshal “fails
silently”; describe it as returning an error when an object is unmarshaled into
[]T. Also adjust the array-first fallback in UnmarshalJSON so that when both
unmarshals fail, the returned error preserves the array-failure context instead
of always returning the single-object error, making malformed input easier to
diagnose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/plugins/cidrs/reverse_rir.go`:
- Around line 336-355: Update the doc comment on ArinRefs.UnmarshalJSON to avoid
saying standard json.Unmarshal “fails silently”; describe it as returning an
error when an object is unmarshaled into []T. Also adjust the array-first
fallback in UnmarshalJSON so that when both unmarshals fail, the returned error
preserves the array-failure context instead of always returning the
single-object error, making malformed input easier to diagnose.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 70f50466-7ac7-4e2d-91a5-77d84f158e8e

📥 Commits

Reviewing files that changed from the base of the PR and between 8c252a5 and c77eb27.

📒 Files selected for processing (4)
  • pkg/plugins/cidrs/pure_fns_test.go
  • pkg/plugins/cidrs/reverse_rir.go
  • pkg/runner/internal_test.go
  • pkg/runner/run.go

@anushkavirgaonkar anushkavirgaonkar merged commit 8698550 into main Jun 26, 2026
25 checks passed
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.

2 participants