Phase 2: secure OIDC auth and PostgreSQL task persistence - #2
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Reviewer's GuideAdds strict OIDC-based bearer authentication around the A2A HTTP transport, wires a tenant/subject-scoped PostgreSQL task store with migrations and pool management into the server, and updates configuration, tests, and documentation to support secure, durable storage in non-development environments. Sequence diagram for authenticated A2A task handling with PostgreSQL taskstoresequenceDiagram
actor Client
participant Middleware as Authenticator.Middleware
participant Verifier as OIDCVerifier.Verify
participant REST as a2asrv.NewRESTHandler
participant Interceptor as identityInterceptor.Before
participant Store as Store.Create
Client->>Middleware: HTTP request with Authorization
Middleware->>Verifier: Verify(ctx, rawToken)
Verifier-->>Middleware: Identity{Issuer,Subject,Tenant,Scopes}
Middleware->>Middleware: WithIdentity(ctx, identity)
Middleware->>Middleware: Header.Del(Authorization, Proxy-Authorization, Cookie)
Middleware->>REST: ServeHTTP(response, request)
REST->>Interceptor: Before(ctx, callCtx, request)
Interceptor->>Interceptor: requestTenant(payload)
Interceptor->>Interceptor: setRequestTenant(payload, identity.Tenant)
Interceptor->>REST: NewAuthenticatedUser(ownerKey(identity), attributes)
REST->>Store: Create(ctx, task)
Store->>Store: verifiedIdentity(ctx)
Store->>Store: INSERT INTO a2a_tasks(... tenant_id, owner_subject ...)
Store-->>REST: TaskVersion
REST-->>Client: HTTP response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds OIDC authentication, tenant-aware request handling, PostgreSQL task persistence, migration management, configuration loading, server startup wiring, and integration tests. ChangesAuthenticated PostgreSQL runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Authenticator
participant OIDCVerifier
participant Server
participant PostgreSQL
Client->>Authenticator: Send bearer token request
Authenticator->>OIDCVerifier: Verify token
OIDCVerifier-->>Authenticator: Return identity
Authenticator->>Server: Forward request with identity context
Server->>PostgreSQL: Read or write tenant-scoped task
PostgreSQL-->>Server: Return task result
Server-->>Client: Return authenticated response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
codeant-ai
Bot
added
the
size:XXL
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- OIDC configuration is validated both in
internal/config/loadOIDCConfigandinternal/auth/validateOIDCConfig; consider consolidating this logic to a single source of truth to avoid subtle drift between the two validators. - The auth middleware currently strips
Authorization,Proxy-Authorization, andCookieonly after successful verification; if a verifier implementation does logging or tracing on the raw token, consider moving the header scrubbing earlier or enforcing a consistent approach to avoid accidental credential exposure in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- OIDC configuration is validated both in `internal/config/loadOIDCConfig` and `internal/auth/validateOIDCConfig`; consider consolidating this logic to a single source of truth to avoid subtle drift between the two validators.
- The auth middleware currently strips `Authorization`, `Proxy-Authorization`, and `Cookie` only after successful verification; if a verifier implementation does logging or tracing on the raw token, consider moving the header scrubbing earlier or enforcing a consistent approach to avoid accidental credential exposure in future changes.
## Individual Comments
### Comment 1
<location path="internal/config/config.go" line_range="247-249" />
<code_context>
+ }, nil
+}
+
+func splitCSV(value string) []string {
+ var result []string
+ for item := range strings.SplitSeq(value, ",") {
+ if item = strings.TrimSpace(item); item != "" {
+ result = append(result, item)
</code_context>
<issue_to_address>
**issue (bug_risk):** splitCSV iterates over indices rather than values, and strings.SplitSeq is nonstandard, which will not compile as written.
In this loop, `item` is the index, not the slice element, so it will be of type `int`, which breaks the trimming and append logic. In addition, `strings.SplitSeq` is not a standard library function and will not compile unless you’ve defined it yourself. To use the standard library, iterate as `for _, item := range strings.Split(value, ",") { ... }`, then trim and filter non-empty items before appending to `result`.
</issue_to_address>
### Comment 2
<location path="internal/storage/postgres/store_integration_test.go" line_range="158" />
<code_context>
+ var wg sync.WaitGroup
+ errorsOut := make(chan error, contenders)
+ versions := make(chan taskstore.TaskVersion, contenders)
+ for i := range contenders {
+ wg.Add(1)
+ go func() {
</code_context>
<issue_to_address>
**issue (testing):** The CAS concurrency test loop does not iterate, so the concurrent update behavior is never exercised
In `TestPostgresStoreConcurrentVersions`, `contenders` is an `int`, so `for i := range contenders` won’t compile or run any goroutines. Update this to a standard numeric loop, e.g. `for i := 0; i < contenders; i++ {`, so the test actually exercises concurrent CAS behavior.
</issue_to_address>
### Comment 3
<location path="internal/auth/oidc.go" line_range="26" />
<code_context>
+ now func() time.Time
+}
+
+func NewOIDCVerifier(ctx context.Context, cfg config.OIDCConfig) (*OIDCVerifier, error) {
+ issuerURL, err := validateOIDCConfig(cfg)
+ if err != nil {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the OIDC verifier construction by validating configuration only once and using the concrete `*oidc.IDTokenVerifier` instead of an extra claims-verifier abstraction.
You’re re-validating and re-wrapping quite a bit, which adds indirection without much gain. Two focused changes can reduce complexity while keeping behavior intact:
### 1. Avoid double `validateOIDCConfig` calls
`validateOIDCConfig` is called in both `NewOIDCVerifier` and `newOIDCVerifier`, causing redundant work and error paths. You can validate once and pass the `issuerURL` down:
```go
func NewOIDCVerifier(ctx context.Context, cfg config.OIDCConfig) (*OIDCVerifier, error) {
issuerURL, err := validateOIDCConfig(cfg)
if err != nil {
return nil, err
}
httpClient := &http.Client{
Timeout: cfg.HTTPTimeout,
CheckRedirect: oidcRedirectPolicy(issuerURL.Scheme == "http"),
}
return newOIDCVerifier(ctx, cfg, issuerURL, httpClient)
}
func newOIDCVerifier(
ctx context.Context,
cfg config.OIDCConfig,
issuerURL *url.URL,
sourceClient *http.Client,
) (*OIDCVerifier, error) {
if sourceClient == nil {
return nil, fmt.Errorf("OIDC HTTP client is required")
}
httpClient := *sourceClient
httpClient.Timeout = cfg.HTTPTimeout
httpClient.CheckRedirect = oidcRedirectPolicy(issuerURL.Scheme == "http")
providerContext := oidc.ClientContext(ctx, &httpClient)
provider, err := oidc.NewProvider(providerContext, cfg.Issuer)
if err != nil {
return nil, fmt.Errorf("discover OIDC provider: %w", err)
}
// ... rest unchanged ...
}
```
If you need `newOIDCVerifier` to be usable without `NewOIDCVerifier`, you can add a small wrapper that validates when `issuerURL` is `nil`, but avoid calling `validateOIDCConfig` twice in the common path.
### 2. Inline `claimsVerifier` / `oidcClaimsVerifier`
The `claimsVerifier` interface only wraps `oidc.IDTokenVerifier.Verify` and `Claims`, and you still parse claims manually. Dropping the interface and using `*oidc.IDTokenVerifier` directly simplifies the flow:
```go
type OIDCVerifier struct {
verifier *oidc.IDTokenVerifier
issuer string
tenantClaim string
clockSkew time.Duration
now func() time.Time
}
func newOIDCVerifier(... ) (*OIDCVerifier, error) {
// ...
idTokenVerifier := provider.VerifierContext(providerContext, &oidc.Config{
ClientID: cfg.Audience,
SupportedSigningAlgs: append([]string(nil), cfg.AllowedAlgorithms...),
SkipExpiryCheck: true,
})
return &OIDCVerifier{
verifier: idTokenVerifier,
issuer: cfg.Issuer,
tenantClaim: cfg.TenantClaim,
clockSkew: cfg.ClockSkew,
now: time.Now,
}, nil
}
func (v *OIDCVerifier) Verify(ctx context.Context, rawToken string) (Identity, error) {
claims := make(map[string]json.RawMessage)
token, err := v.verifier.Verify(ctx, rawToken)
if err != nil {
return Identity{}, ErrInvalidToken
}
if err := token.Claims(&claims); err != nil {
return Identity{}, ErrInvalidToken
}
// rest of Verify unchanged
}
```
This keeps all functionality (including custom claim parsing and skew checks) but removes an abstraction layer that doesn’t currently buy you polymorphism.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add A2A-RedPandaServer-Container to your team's repositories. |
31 new issues
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+60
to
+62
| allowLoopbackHTTP := issuerURL.Scheme == "http" | ||
| if _, err := validateRemoteURL(metadata.JWKSURL, allowLoopbackHTTP, true); err != nil { | ||
| return nil, fmt.Errorf("unsafe OIDC jwks_uri: %w", err) |
There was a problem hiding this comment.
Suggestion: The discovered jwks_uri and all redirect destinations are accepted when they are any HTTPS URL, including private or internal hosts unrelated to the configured issuer. A malicious or compromised discovery response can therefore make the server fetch JWKS data from an arbitrary internal HTTPS endpoint. Restrict JWKS retrieval and redirects with a private-network-aware dial policy and an explicit trusted-host policy appropriate for the deployment. [ssrf]
Severity Level: Critical 🚨
- ❌ OIDC startup can probe private HTTPS services.
- ⚠️ JWKS discovery is not restricted to the issuer trust boundary.
- ⚠️ Internal service responses may be exposed through observable authentication behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/auth/oidc.go
**Line:** 60:62
**Comment:**
*Ssrf: The discovered `jwks_uri` and all redirect destinations are accepted when they are any HTTPS URL, including private or internal hosts unrelated to the configured issuer. A malicious or compromised discovery response can therefore make the server fetch JWKS data from an arbitrary internal HTTPS endpoint. Restrict JWKS retrieval and redirects with a private-network-aware dial policy and an explicit trusted-host policy appropriate for the deployment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Collaborator
Author
There was a problem hiding this comment.
Resolved in 401027f: discovery redirects and jwks_uri are restricted to the exact issuer origin, and the production dialer resolves and connects through a private, loopback, link-local, and special-use IP deny policy. Development and test retain explicit private-loopback support. Cross-origin and private-target tests were added.
Comment on lines
+133
to
+137
| func loadOIDCConfig() (OIDCConfig, error) { | ||
| issuer := strings.TrimSpace(os.Getenv("OIDC_ISSUER")) | ||
| audience := strings.TrimSpace(os.Getenv("OIDC_AUDIENCE")) | ||
| if issuer == "" && audience == "" { | ||
| return OIDCConfig{}, nil |
There was a problem hiding this comment.
Suggestion: When both OIDC_ISSUER and OIDC_AUDIENCE are empty, every other OIDC setting is silently ignored and authentication is disabled. Supplying OIDC_REQUIRED_SCOPES, OIDC_ALLOWED_ALGORITHMS, or OIDC_TENANT_CLAIM without the two required connection settings should instead return a configuration error; otherwise a partially configured deployment can run unauthenticated while appearing to have OIDC policy configured. [security]
Severity Level: Critical 🚨
- ❌ Incomplete OIDC deployments can run without authentication.
- ⚠️ Configured scope policy is silently ignored.
- ⚠️ Startup does not reveal the authentication misconfiguration.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/config/config.go
**Line:** 133:137
**Comment:**
*Security: When both `OIDC_ISSUER` and `OIDC_AUDIENCE` are empty, every other OIDC setting is silently ignored and authentication is disabled. Supplying `OIDC_REQUIRED_SCOPES`, `OIDC_ALLOWED_ALGORITHMS`, or `OIDC_TENANT_CLAIM` without the two required connection settings should instead return a configuration error; otherwise a partially configured deployment can run unauthenticated while appearing to have OIDC policy configured.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Collaborator
Author
There was a problem hiding this comment.
Resolved in 401027f: any non-empty OIDC policy variable without the issuer and audience pair now fails startup instead of silently disabling authentication.
Comment on lines
+119
to
+123
| applied, err := loadAppliedMigrations(ctx, pool) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return verifyMigrationSet(migrations, applied, false) |
There was a problem hiding this comment.
Suggestion: VerifySchema reads the migration ledger without acquiring the migration advisory lock. A migrator can therefore apply a migration immediately after verification and while the server is starting, allowing the process to run against a schema that was not the schema it verified. Acquire the same session advisory lock for the complete verification operation, or otherwise coordinate verification and migration. [race condition]
Severity Level: Major ⚠️
- ⚠️ Server startup can race concurrent schema changes.
- ❌ An incompatible migration can break durable task operations.
- ⚠️ Verification does not coordinate with the migration command.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/storage/postgres/migrations.go
**Line:** 119:123
**Comment:**
*Race Condition: `VerifySchema` reads the migration ledger without acquiring the migration advisory lock. A migrator can therefore apply a migration immediately after verification and while the server is starting, allowing the process to run against a schema that was not the schema it verified. Acquire the same session advisory lock for the complete verification operation, or otherwise coordinate verification and migration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Collaborator
Author
There was a problem hiding this comment.
Hardened in 401027f: VerifySchema now acquires the same session advisory lock and performs all ledger reads on that connection. Lock acquisition or unlock ambiguity destroys the pooled connection, and pg_advisory_unlock must return true. Deployment docs still prohibit concurrent migrations because a short startup lock cannot guard the full process lifetime.
Comment on lines
+162
to
+166
| return &taskstore.StoredTask{ | ||
| Task: task, | ||
| Version: taskstore.TaskVersion(version), | ||
| User: owner, | ||
| }, nil |
There was a problem hiding this comment.
Suggestion: The store persists only the raw subject as StoredTask.User, but the authentication interceptor identifies the caller with the length-prefixed ownerKey containing issuer, tenant, and subject. The A2A task-store authorization path compares the stored user with the authenticated user name, so tasks loaded from PostgreSQL can fail ownership checks for subsequent operations. Persist and return the same canonical owner value used by the interceptor, or change both implementations to use a shared identity contract. [api mismatch]
Severity Level: Major ⚠️
- ❌ Persisted task retrieval can fail ownership checks.
- ❌ Follow-up A2A task operations can be rejected.
- ⚠️ Durable tasks behave differently from in-memory task handling.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/storage/postgres/store.go
**Line:** 162:166
**Comment:**
*Api Mismatch: The store persists only the raw subject as `StoredTask.User`, but the authentication interceptor identifies the caller with the length-prefixed `ownerKey` containing issuer, tenant, and subject. The A2A task-store authorization path compares the stored user with the authenticated user name, so tasks loaded from PostgreSQL can fail ownership checks for subsequent operations. Persist and return the same canonical owner value used by the interceptor, or change both implementations to use a shared identity contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Collaborator
Author
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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.
Inline comments:
In `@cmd/server/main.go`:
- Line 35: Update the error returned in the server configuration validation to
start with a lowercase character, changing the message in the relevant error
construction while preserving the environment value and remaining wording.
In `@go.mod`:
- Around line 12-19: Update the golang.org/x/text indirect dependency in go.mod
from v0.37.0 to v0.39.0 or later, preserving the existing dependency declaration
and ensuring the resolved module version reflects the upgrade.
In `@internal/auth/auth_test.go`:
- Line 54: Replace every httptest.NewRequest call in the auth tests with
httptest.NewRequestWithContext, passing t.Context() so requests are bound to the
test lifetime. Apply this consistently to the request setups in the affected
tests, including the calls near lines 54, 90, 113, and 133.
In `@internal/auth/auth.go`:
- Around line 240-254: Update writeAuthError to encode a typed response struct
instead of nested map[string]any, satisfying errchkjson while preserving the
existing JSON shape and checked-error handling. Set WWW-Authenticate for both
unauthorized and forbidden statuses, using invalid_token for 401 and
insufficient_scope for 403. Update the related assertion in auth_test.go to
expect the 403 challenge.
In `@internal/auth/oidc_test.go`:
- Line 167: Fix the four golangci-lint findings in the tests: update the
ErrInvalidToken assertions at the branches around lines 167 and 219 to use
errors.Is, matching the existing pattern near line 110, and update the Encode
calls around lines 338 and 365 to encode a typed value or capture and check the
returned error. Preserve the current test behavior while ensuring all errors are
handled.
- Around line 230-246: Extend
TestNewOIDCVerifierRejectsIssuerMismatchAndUnsafeJWKS with a redirect-policy
case that makes the discovery endpoint return a 302 to a cleartext non-loopback
URL, then call newOIDCVerifier and assert discovery fails. Exercise
oidcRedirectPolicy through the discovery request rather than the JWKS override,
and verify the returned error indicates provider discovery failure.
- Around line 115-122: Remove the custom stringsContain helper and import the
standard-library strings package. Update all three call sites in the affected
test code to use strings.Contains with the existing value and fragment
arguments.
In `@internal/auth/oidc.go`:
- Around line 49-70: Update NewOIDCVerifier’s oidc.VerifierContext call to use a
long-lived context independent of the startup context’s cancellation or timeout,
such as context.WithoutCancel(providerContext). Preserve the existing bounded
HTTP client and all verifier configuration, including SupportedSigningAlgs and
SkipExpiryCheck.
In `@internal/config/config_test.go`:
- Around line 146-165: Add table-driven coverage for both remaining validation
branches in loadDatabaseConfig: DATABASE_PASSWORD_FILE set without DATABASE_URL,
and a relative password-file path with a valid database URL. Reuse
setValidEnvironment, set the relevant environment variables per subtest, and
assert Load returns an error for each case.
In `@internal/config/config.go`:
- Around line 157-167: Consolidate the duplicated OIDC security decisions by
exporting one algorithm allow-list and the loopback-host helper from the config
package, then update loadOIDCConfig and validateOIDCConfig in auth to reuse
those shared symbols. Remove the local algorithm map and isLoopbackHost
implementations while preserving their current validation and loopback behavior.
- Around line 146-148: Pass the resolved environment from Load into
loadOIDCConfig, then allow the HTTP loopback exception only outside staging and
production. Keep HTTPS required for all non-loopback issuers and reject
cleartext loopback OIDC_ISSUER values when the environment is staging or
production.
In `@internal/server/server_test.go`:
- Line 237: Update the HTTP request in the agent-card test around cardResponse
to use http.NewRequestWithContext with t.Context(), then execute it via
testServer.Client().Do instead of Client().Get; preserve the existing URL and
response handling.
In `@internal/storage/postgres/migrations_test.go`:
- Around line 16-32: Add a focused test for the migration-gap branch in
verifyMigrationSet, using a synthetic two-migration slice where only the
higher-version migration is applied and migration mode is enabled; assert that
verification returns an error for the missing lower-version migration.
In `@internal/storage/postgres/migrations/0001_create_a2a_tasks.sql`:
- Around line 1-12: Make task identity tenant-scoped: in
internal/storage/postgres/migrations/0001_create_a2a_tasks.sql lines 1-12,
define task_id as NOT NULL and use a composite primary key of (tenant_id,
task_id); in internal/storage/postgres/store.go lines 55-68, update the conflict
target to (tenant_id, task_id); in
internal/storage/postgres/store_integration_test.go lines 46-48, update the test
task ID to a UUID to cover cross-tenant collisions.
In `@internal/storage/postgres/pool.go`:
- Around line 94-111: Update readPasswordFile to open the password file once,
obtain metadata from that file handle, and perform the regular-file, permission,
and size checks against it before reading. Replace the path-based os.ReadFile
call with a handle-based read (using io as needed), preserve the existing error
messages and limits, and ensure the handle is closed on every path.
In `@internal/storage/postgres/store_integration_test.go`:
- Around line 319-337: Resolve all reported golangci-lint findings in this test
file: update httpTestVerifier.Verify to avoid initializing tenant to an unused
empty string before the switch, while preserving its token-to-tenant mapping and
invalid-token return; explicitly handle the Body.Close return values at the two
reported call sites, using deferred closure cleanup for sendResponse.Body and
ignored-error assignment for response.Body.
In `@internal/storage/postgres/store.go`:
- Around line 309-312: Update the StatusTimestampAfter predicate in listFilter
to require a non-null status_timestamp strictly greater than the supplied
timestamp, replacing the current IS NULL OR >= condition with a > comparison
while preserving parameter numbering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8d7f9951-1c28-44b1-ad68-60599015f6be
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.env.exampleREADME.mdcmd/migrate/main.gocmd/server/main.godocs/architecture.mdgo.modinternal/auth/auth.gointernal/auth/auth_test.gointernal/auth/oidc.gointernal/auth/oidc_test.gointernal/config/config.gointernal/config/config_test.gointernal/orchestrator/executor.gointernal/server/server.gointernal/server/server_test.gointernal/storage/postgres/cursor.gointernal/storage/postgres/cursor_test.gointernal/storage/postgres/migrations.gointernal/storage/postgres/migrations/0001_create_a2a_tasks.sqlinternal/storage/postgres/migrations_test.gointernal/storage/postgres/pool.gointernal/storage/postgres/pool_test.gointernal/storage/postgres/store.gointernal/storage/postgres/store_integration_test.go
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
EntelligenceAI PR SummaryThis PR introduces strict OIDC bearer-token authentication with private IP blocking and a PostgreSQL-backed task store with tenant/owner isolation, optimistic concurrency, and keyset pagination. It adds config validation for both auth and database secret handling, wires authentication and database initialization into the server startup, and sanitizes internal errors before they reach clients. The PR also includes migrations, tests, and documentation updates. Review Scorecard
Review recommended before merging. 8 unresolved comment(s) from previous reviews remain open. Key Findings:
Evaluated against
|
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| if err != nil { | ||
| return DatabaseConfig{}, err | ||
| } | ||
| minConnections, err := parseInt32("DATABASE_MIN_CONNECTIONS", 2, 0, maxConnections) |
There was a problem hiding this comment.
When an operator sets DATABASE_MAX_CONNECTIONS=1 (valid: range is 1–500) but leaves DATABASE_MIN_CONNECTIONS unset, parseInt32 falls back to the default "2" and then rejects it because 2 > maxConnections(1). The server fails to start with "DATABASE_MIN_CONNECTIONS must be between 0 and 1" — blaming a variable the operator never set. The validation is safe (no wrong runtime result), but the default min (2) is inconsistent with the allowed max floor (1).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
a2asrvtaskstore.Storewith globally unique task IDs, atomic versions, CAS, keyset pagination, filtering, and A2A-compatible list shapingSDK decision
The server continues to use
github.com/a2aproject/a2a-go/v2/a2asrvv2.4.0 for A2A v1.0 routing, SSE, task lifecycle, and protocol behavior. Project code owns the outer auth boundary and PostgreSQL adapter because the SDK copies request headers before interceptors, leaves tenant handling incomplete for unprefixed REST routes, and ships no PostgreSQL task-store implementation.Security invariants
Authorization,Proxy-Authorization, andCookieare removed before SDK transport metadata is createdcmd/migrateapplies checksummed forward-only migrationsVerification
go test -count=1 ./...— 122 passed without the optional database DSNTEST_DATABASE_URL=postgresql://postgres@127.0.0.1:55432/bridge_a2a?sslmode=disable go test -count=1 ./internal/storage/postgres— 11 passed against PostgreSQL 17go vet ./...go build ./cmd/server ./cmd/migrategit diff --checkThe real-database suite covers migrations, persistence, global ID collisions, tenant/owner isolation, optimistic and unconditional concurrent updates, list filters/cursors/shaping, and HTTP-to-OIDC-to-PostgreSQL identity propagation. The OIDC suite uses a real TLS discovery/JWKS test server with signed RS256 tokens, rotation, cache/outage, issuer/audience/signature/algorithm failures, and unsafe URL rejection.
Windows race detection remains unavailable because this host has no GCC/CGO toolchain; Linux CI is the authoritative race gate.
Summary by Sourcery
Introduce secure OIDC-based authentication and tenant-scoped PostgreSQL task persistence, and wire them into the server and migration workflow while tightening configuration validation and tests.
New Features:
Enhancements:
Tests:
CodeAnt-AI Description
Secure A2A requests with OIDC and add tenant-isolated PostgreSQL task persistence
What Changed
Impact
✅ Fewer unauthorized A2A requests✅ Isolated tasks across tenants and users✅ Durable task history across server restarts✅ Safer database schema rollouts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.