Conversation
## Summary - Upgrades `go` directive to `1.25.0` and `toolchain` to `go1.25.7` in all workspace modules and `go.work` - Addresses GO-2026-4337 (`crypto/tls` vulnerability, fixed in go1.25.7) - Updates `golangci-lint` from `v2.1` to `v2.8.0` in CI workflow and Makefile (required for Go 1.25 compatibility) - Updates `govulncheck` `go-version-input` to `1.25.7` in `checks.yaml` and `vulnerability-check.yaml` - Updates `sonarcloud.yml` Go version from `1.23` to `1.25.7` ## Test plan - [x] `go mod tidy` succeeds in all modules - [x] `go build ./...` succeeds in all modules - [ ] CI checks pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Paul Flynn <pflynn@virtru.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @c-r33d, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural improvement by abstracting the Key Access Server (KAS) URI resolution mechanism. It replaces direct string-based KAS URI handling with a flexible interface and an injectable resolver. This change allows for more dynamic and configurable KAS URI management, while ensuring that existing setups continue to function seamlessly through a default static resolver. The primary impact is increased extensibility and maintainability for how KAS endpoints are discovered and utilized within the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. A URI, once fixed, Now resolves with new intent, Flexibility. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a KAS URI resolver, which is a great step towards more dynamic service configuration. The changes are well-structured, introducing a RegisteredKasURIResolver interface and a default StaticRegisteredKasURIResolver implementation for backward compatibility. The necessary plumbing to inject the resolver from server start options down to the KAS service is correctly implemented. The KeyIndexer is refactored to use this resolver, and the tests are updated accordingly.
I have a few minor suggestions to improve code consistency and reduce duplication in tests, but overall, this is a solid contribution.
| mockClient := new(MockKeyAccessServerRegistryClient) | ||
| kasURI := testKasURI | ||
| resolver, err := NewStaticRegisteredKasURIResolver(kasURI) | ||
| s.Require().NoError(err) | ||
| keyIndexer := &KeyIndexer{ | ||
| sdk: &sdk.SDK{ | ||
| KeyAccessServerRegistry: mockClient, | ||
| }, | ||
| resolver: resolver, | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, consider refactoring this common test setup logic into a helper method on the KeyIndexTestSuite. This block of code is repeated in TestListKeys, TestFindKeyByAlgorithm, and TestFindKeyByID.
A helper function could look like this:
func (s *KeyIndexTestSuite) newTestKeyIndexer(kasURI string) (*KeyIndexer, *MockKeyAccessServerRegistryClient) {
mockClient := new(MockKeyAccessServerRegistryClient)
resolver, err := NewStaticRegisteredKasURIResolver(kasURI)
s.Require().NoError(err)
keyIndexer := &KeyIndexer{
sdk: &sdk.SDK{
KeyAccessServerRegistry: mockClient,
},
resolver: resolver,
}
return keyIndexer, mockClient
}Then, each test could simply call keyIndexer, mockClient := s.newTestKeyIndexer(testKasURI).
| if r.kasURI == "" { | ||
| return "", fmt.Errorf("registered KAS URI is empty") | ||
| } |
There was a problem hiding this comment.
For consistency, it would be better to return the package-level errKasURIEmpty here, which is also used in the NewStaticRegisteredKasURIResolver constructor when the URI is empty. This makes the error handling more uniform.
| if r.kasURI == "" { | |
| return "", fmt.Errorf("registered KAS URI is empty") | |
| } | |
| if r.kasURI == "" { | |
| return "", errKasURIEmpty | |
| } |
|
|
||
| resolver = &StaticRegisteredKasURIResolver{} | ||
| uri, err = resolver.ResolveURI() | ||
| require.Error(t, err) |
There was a problem hiding this comment.
To make this test more specific and robust, consider using require.ErrorIs to assert that the expected error errKasURIEmpty is returned. This is especially relevant given the suggestion to make the error returned from ResolveURI consistent.
| require.Error(t, err) | |
| require.ErrorIs(t, err, errKasURIEmpty) |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
2 participants
Proposed Changes
1.) Add a kas uri resolver that can be injected into the server start options
2.) Default is to use the static resolver that reads the
registered_kas_urifrom the kas configuration for previous backwards compatibility.Checklist
Testing Instructions