Skip to content

satisfy vet tooling to avoid noise and missed issues - #1345

Open
maleck13 wants to merge 1 commit into
Kuadrant:mainfrom
maleck13:fix/vet-issues
Open

satisfy vet tooling to avoid noise and missed issues #1345
maleck13 wants to merge 1 commit into
Kuadrant:mainfrom
maleck13:fix/vet-issues

Conversation

@maleck13

@maleck13 maleck13 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Quiets down the ouput from the go vet tooling so real issues are clearer to see

Summary by CodeRabbit

  • Tests

    • Improved test safety by adding explicit request contexts and preventing execution after fatal assertion failures.
    • Strengthened test handling for nil values and failed key decoding scenarios.
  • Chores

    • Added security-scanner annotations for intentionally safe test fixtures, logging, cookie handling, and configuration references.
    • No user-facing runtime behavior changed.

Signed-off-by: craig <cbrookes@redhat.com>
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: af032706-77d5-4027-afeb-b3a0954d4b7c

📥 Commits

Reviewing files that changed from the base of the PR and between ad73aed and fb6f58f.

📒 Files selected for processing (14)
  • internal/broker/elicitation_handler_test.go
  • internal/broker/oauth_protected_resource_handler_test.go
  • internal/broker/protocol_filter_test.go
  • internal/broker/tokens.go
  • internal/broker/tokens_test.go
  • internal/config/config_writer_test.go
  • internal/config/mcpservers_test.go
  • internal/controller/broker_router.go
  • internal/controller/deployment_test.go
  • internal/controller/keypair_test.go
  • internal/session/jwt_test.go
  • internal/tests/server2/server2.go
  • internal/tests/stateless-server/server.go
  • tests/e2e/suite_test.go

📝 Walkthrough

Walkthrough

The PR updates broker HTTP tests to use explicit contexts, adds early returns after fatal assertions, and adds gosec suppression annotations to existing production and test code.

Changes

Test and lint hygiene

Layer / File(s) Summary
Context-aware broker requests
internal/broker/*_test.go
Broker handler and token tests now create requests with context.Background().
Fatal assertion guards
internal/broker/protocol_filter_test.go, internal/controller/*_test.go, internal/session/jwt_test.go
Tests now return after fatal nil or decode assertions and when the protocol tool cache is nil.
gosec annotations
internal/broker/tokens.go, internal/config/*_test.go, internal/controller/broker_router.go, internal/controller/deployment_test.go, internal/tests/*, tests/e2e/suite_test.go
The code adds suppressions for accepted cookie, secret, logging, and context-cancellation findings.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: review-effort/medium, high-risk

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the changes that reduce vet tooling noise and prevent missed issues.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot added high-risk Touches concurrency, auth, sessions, CRDs, ext_proc, or routing review-effort/medium Medium review effort (3): few files, moderate logic labels Aug 6, 2026
@david-martin

Copy link
Copy Markdown
Member

CI failing on nolintlint: the //nolint:gosec directives are flagged as unused. Possibly needs a golangci-lint version bump locally to match what CI runs (v2.4.0)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

high-risk Touches concurrency, auth, sessions, CRDs, ext_proc, or routing review-effort/medium Medium review effort (3): few files, moderate logic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants