Add tests for agentic handlers: query, batch, discover#810
Conversation
… handlers Adds three new test files covering edge cases and missing code paths: - query_handler_test.go: default limit clamping, invalid RFC3339 dates, offset out-of-bounds, missing resource field, response structure - batch_test.go: invalid JSON, single/max operations, POST bodies, sub-request errors, auth header propagation, concurrent integrity - discover_test.go: method filter, limit clamp >100, combined filters, see_also references, Smart404 suggestions with auth filtering Coverage: 91.5% -> 92.6% (BatchHandler: 79.4% -> 94.1%, DiscoverHandler: 92.3% -> 100.0%)
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
🔴 PR-AF Review — Merge Blocked — Must-Fix Found
🚫 Merge blocked. 1 must-fix issue found by automated review.
Automated multi-agent code review · PR-AF built with AgentField
5 findings · 🚫 1 blocking · 💬 4 advisory · 🔴 0 critical · 🟠 1 important · 🔵 4 suggestions · ⚪ 0 nitpicks
PR Overview
Summary
- Adds comprehensive behavior-driven table tests for the three agentic HTTP handlers: query, batch, and discover
- 755 lines of new test code across three test files with no production code changes
- Covers error envelopes, dispatch, per-item batch status, auth-header propagation, pagination, formats, and concurrency integrity
- Package coverage improves from 91.5% to 92.6%
Changes
control-plane/internal/handlers/agentic/query_handler_test.go— 293 lines covering query handler dispatch, auth propagation, error envelopescontrol-plane/internal/handlers/agentic/batch_test.go— 249 lines covering per-item status, batch dispatch, concurrency integritycontrol-plane/internal/handlers/agentic/discover_test.go— 213 lines covering pagination, output formats, error handling
Test plan
go test -cover ./internal/handlers/agentic/passes with 92.6% coverage- All tests use table-driven patterns with subtests for clear failure messages
- Verified:
go test -count=1 ./internal/handlers/agentic/— all 755 lines of new tests pass
Closes #409
🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField
Key Findings
1 issue(s) should be addressed before merge:
- 🟠 Batch tests miss the caller-identity authorization divergence (
control-plane/internal/handlers/agentic/batch_test.go:137) —TestBatchHandler_AuthHeaderPropagationgives a passing result without exercising the production identity contract. Gate: Authorization bypass vulnerability: batch handler fails to propagate caller identity headers to nested requests, allowing circumvention of scope ownership checks on real production endpoints.
4 advisory finding(s) surfaced as non-blocking:
- 🔵 Out-of-range offset test does not assert the returned page (
control-plane/internal/handlers/agentic/query_handler_test.go:170) - 🔵 Add valid RFC3339 filter-forwarding assertions (
control-plane/internal/handlers/agentic/query_handler_test.go:149) - 🔵 Discovery and Smart404 tests bypass production authentication and never assert authorized catalog membership (
control-plane/internal/handlers/agentic/discover_test.go:18) - 🔵 Add a server-wiring test for the claimed authenticated agentic surface (
control-plane/internal/handlers/agentic/batch_test.go:33)
Files with findings: control-plane/internal/handlers/agentic/batch_test.go, control-plane/internal/handlers/agentic/discover_test.go, control-plane/internal/handlers/agentic/query_handler_test.go
All Findings by Severity
🟠 Important (1)
- Batch tests miss the caller-identity authorization divergence
control-plane/internal/handlers/agentic/batch_test.go:137
🔵 Suggestion (4)
- Out-of-range offset test does not assert the returned page
control-plane/internal/handlers/agentic/query_handler_test.go:170 - Add valid RFC3339 filter-forwarding assertions
control-plane/internal/handlers/agentic/query_handler_test.go:149 - Discovery and Smart404 tests bypass production authentication and never assert authorized catalog membership
control-plane/internal/handlers/agentic/discover_test.go:18 - Add a server-wiring test for the claimed authenticated agentic surface
control-plane/internal/handlers/agentic/batch_test.go:33
Review Process Details
Dimensions Analyzed (4):
- Batch subrequests preserve protected-route authorization semantics — 1 file(s)
- Query tests verify filter, page, total, and failure semantics for every resource branch — 1 file(s)
- Discover and Smart404 tests prove authorization-filtered catalog visibility — 1 file(s)
- Production route-stack coverage for agentic handler tests — 3 file(s)
Meta-Dimension Lenses (3):
- Semantic — 3 dimension(s), 90% coverage confidence
- Mechanical — 2 dimension(s), 88% coverage confidence
- Systemic — 2 dimension(s), 88% coverage confidence
Cross-Reference & Adversary Analysis:
- 4 finding(s) adversarially tested: 4 confirmed, 0 challenged
Pipeline Stats
| Metric | Value |
|---|---|
| Duration | 1159.6s |
| Agent invocations | 28 |
| Coverage iterations | 1 |
| Estimated cost | N/A (provider does not report cost) |
| Budget exhausted | No |
| PR type | test |
| Complexity | medium |
Review ID: rev_d6624ce16c65
| assert.Equal(t, float64(404), bad["status"]) | ||
| } | ||
|
|
||
| func TestBatchHandler_AuthHeaderPropagation(t *testing.T) { |
There was a problem hiding this comment.
Caution
Must-fix before merge. Authorization bypass vulnerability: batch handler fails to propagate caller identity headers to nested requests, allowing circumvention of scope ownership checks on real production endpoints.
Batch tests miss the caller-identity authorization divergence
🟠 IMPORTANT · confidence 99%
Add a production-stack test that asserts identical status, caller identity, and inner error body for direct and batched requests carrying X-Caller-Agent-ID and X-Agent-Node-ID.
TestBatchHandler_AuthHeaderPropagation mounts no auth or permission middleware and only asserts the two forwarded credential headers, so it cannot expose the divergence. Because the nested request is a fresh http.Request that does not propagate X-Caller-Agent-ID or X-Agent-Node-ID, APIKeyAuth sets no caller_agent_id and MemoryPermissionMiddleware denies a direct actor-scope request as scope_ownership_denied while allowing the batched subrequest through the global/anonymous path.
Evidence
Step 1: Production registers
POST /api/v1/agentic/batchwithagentic.BatchHandler(s.Router)on the authenticatedagentAPIgroup (routes_agentic.go), so a batch entersAPIKeyAuthonce and each operation re-enters the complete router.
Step 2:APIKeyAuthsetscaller_agent_idonly by readingX-Caller-Agent-ID, thenX-Agent-Node-ID, from the current request (middleware/auth.go:132-138).
Step 3:BatchHandlerbuilds a new request from onlyc.Request.Context()and copiesContent-Type,X-API-Key, andAuthorization; it does not copy either caller-identity header (batch.go:64-87). A new Gin request context is created whenrouter.ServeHTTPhandles that request, so the outer Gin value is not inherited.
Step 4:MemoryPermissionMiddlewareresolves identity from those request headers (memory_permission.go:144-152) and rejects a mismatched actor scope only when a caller ID is present (memory_permission.go:75-86,220-227). Thus a direct authenticatedagent-brequest withX-Actor-ID: agent-ais denied (also pinned byTestMemoryPermission_ScopeOwnership_ActorScope_DifferentAgent), while its batched subrequest has no caller/scope headers and takes the allowed global/anonymous path.
Step 5: The added test at lines 137-177 mounts no auth or permission middleware and asserts only the two forwarded credential headers, so it cannot fail for this production behavior change.
💡 Suggested Fix
Mount the batch endpoint beside a protected route under the real auth/permission middleware and compare direct versus batched requests with X-Caller-Agent-ID and X-Agent-Node-ID. The implementation needs an explicit policy for propagating the identity/scope headers (or an internal identity mechanism) before equivalence can pass; include unauthorized, 5xx-envelope, and canceled/deadline subcases in the regression test.
Batch authenticated nested request coverage · confidence 99%
🤖 Reviewed by AgentField PR-AF
| store.AssertExpectations(t) | ||
| } | ||
|
|
||
| func TestQueryHandler_AgentOffsetOutOfBounds(t *testing.T) { |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Missing test assertions for edge cases is a test coverage issue, not a production defect.
Out-of-range offset test does not assert the returned page
🔵 SUGGESTION · confidence 99%
Add distinguishable-item table cases that assert results, total, and the offset behavior for negative, in-range, and past-end offsets for each non-events branch (including the runs branch's distinct pagination behavior).
TestQueryHandler_AgentOffsetOutOfBounds calls an offset past the returned list but asserts only total, so the test passes whether the caller receives the original page (the current non-events behavior), an empty page (the events behavior), or an error. There are likewise no negative or past-end page assertions for executions, workflows, or sessions, and the response-structure tests use empty results.
Evidence
Step 1: a caller posts the request at line 180 with
resource=agents, two returned agents, andoffset=10. Step 2:queryAgentscomputestotal := len(agents)and only slices whenreq.Offset > 0 && req.Offset < len(agents); with 10 and 2, it leaves the returned items unchanged. Step 3: the test at line 190 asserts onlytotal == 2and never readsdata["results"]. Step 4: both the current first-page result and an implementation returning an empty page or an erroneous page would satisfy this assertion, so the externally visible past-end-offset contract is not locked down.
💡 Suggested Fix
Make this a table-driven pagination contract with identifiable returned items, and assert the exact page and pre-pagination total for negative, in-range, and past-end offsets across all five branches.
Query caller-visible semantics · confidence 99%
🤖 Reviewed by AgentField PR-AF
| require.True(t, resp.OK) | ||
| } | ||
|
|
||
| func TestQueryHandler_ExecutionsInvalidSinceUntil(t *testing.T) { |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Missing test coverage for valid input paths; does not affect production correctness.
Add valid RFC3339 filter-forwarding assertions
🔵 SUGGESTION · confidence 98%
Add table-driven tests that supply valid RFC3339 since/until values and assert the exact StartTime/EndTime forwarded to storage for runs, executions, workflows, and sessions.
The current tests only verify that invalid timestamps become nil, while the storage filter is never captured to inspect valid bounds. A regression that swaps bounds, drops one bound, or replaces the parsed value with time.Now() would still return 200 and pass all existing assertions.
Evidence
Step 1: a caller can POST
{"resource":"executions","filters":{"since":"...","until":"..."}}toQueryHandler. Step 2:queryExecutionsparses those fields and passes anExecutionFiltertostore.QueryExecutionRecords. Step 3: the added execution test at line 151 configuresQueryExecutionRecordswithmock.Anything, so it does not inspect either parsed bound; the other added timestamp tests at lines 52-147 assert only that invalid bounds become nil. Step 4: therefore an incorrect non-nil valid bound reaches storage while every added timestamp assertion still succeeds, leaving the caller's time-window query semantics unprotected.
💡 Suggested Fix
Use fixed timestamps and capturing fakes/matched filters to assert the exact StartTime and EndTime sent to storage for every branch that supports them.
Query caller-visible semantics · confidence 98%---
🤖 Reviewed by AgentField PR-AF
| func TestDiscoverHandler_WithMethodFilter(t *testing.T) { | ||
| catalog := setupCatalog() | ||
| router := gin.New() | ||
| router.Use(func(c *gin.Context) { c.Set("auth_level", "api_key"); c.Next() }) |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Missing test coverage for authentication filtering is a test quality issue, not a production vulnerability or regression.
Discovery and Smart404 tests bypass production authentication and never assert authorized catalog membership
🔵 SUGGESTION · confidence 98%
Replace the middleware-bypassing unit tests with router-level integration tests that mount APIKeyAuth and assert authorized catalog membership.
TestDiscoverHandler_WithMethodFilter and TestSmart404Handler_SuggestionsAreAuthFiltered inject auth_level: "api_key" directly at discover_test.go:18 and discover_test.go:109 instead of deriving it through middleware.APIKeyAuth (routes_middleware.go:79-84, middleware/auth.go:101-144), so a 401 abort path or auth-level handoff regression would not be caught. The discovery assertions never verify that total matches the post-filter, post-limit endpoint count (discover.go:22-32), and the Smart404 assertion at discover_test.go:121-129 only checks field presence without verifying that protected paths are absent (discover.go:64-86). A broken FilterByAuth, incorrect total calculation, or missing middleware abort would all leave this suite green.
Evidence
Step 1:
TestDiscoverHandler_WithMethodFilterinstalls middleware that unconditionally executesc.Set("auth_level", "api_key")atdiscover_test.go:18, then invokesDiscoverHandlerdirectly.TestSmart404Handler_SuggestionsAreAuthFiltereddoes the same at line 109.
Step 2: The production route is behind the globalmiddleware.APIKeyAuthstack (routes_middleware.go:79-84). With a configured key,APIKeyAuthvalidatesX-API-Key/Bearer credentials and, on failure, callsAbortWithStatusJSON(401, ...)and returns (middleware/auth.go:101-134); only successful requests receiveauth_level = "api_key"(auth.go:137-144). The test routers never run this path.
Step 3:DiscoverHandlerfilters catalog entries using that context value before truncating and returnstotal: len(results)(discover.go:22-32), but none of the new discovery assertions checks entry paths/auth levels ortotal.
Step 4:Smart404Handlerobtains five candidates then manually filters them (discover.go:64-86), but the test atdiscover_test.go:121-129checks only that every returned object hasmethod,path,summary, andscore; it never checks that protected paths are absent or permitted paths are present.
Step 5: Consequently, changing/removingFilterByAuth, changing the total calculation, or breaking the middleware abort/auth-level handoff can expose incorrect discovery behavior without failing these newly added tests.
💡 Suggested Fix
Add table-driven integration-style tests in this file using a small catalog with public, api_key, and admin/privileged entries. Mount middleware.APIKeyAuth(AuthConfig{APIKey: ...}) on the Gin router before DiscoverHandler and Smart404Handler; assert missing/wrong credentials return 401 before either handler, valid credentials expose public plus api_key but not privileged paths, and every returned total equals the number of authorized endpoints after the requested limit. Make Smart404's candidate ranking place protected entries among the top five, then assert they are absent and an allowed candidate remains. Keep the existing /ui/ delegation test unchanged.
Semantic security contract · confidence 98%
🤖 Reviewed by AgentField PR-AF
| } | ||
|
|
||
| func TestBatchHandler_SingleOperation(t *testing.T) { | ||
| router := gin.New() |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: This is a test coverage gap for middleware composition, not a demonstrated production defect or reachable vulnerability.
Add a server-wiring test for the claimed authenticated agentic surface
🔵 SUGGESTION · confidence 94%
Add a server-package test that builds the production route stack to verify the authenticated /api/v1/agentic surface. The current tests replace the server router with gin.New() and register handlers directly at /query or omit middleware, bypassing setupRoutes where APIKeyAuth is installed before registerAgenticRoutes. This misses the production composition boundary where batch subrequests re-enter router.ServeHTTP and must re-establish authenticated context through the middleware stack.
Evidence
Step 1:
AgentFieldServer.setupRoutescallsapplyGlobalMiddleware()before creating/api/v1and callingregisterAgenticRoutes(agentAPI);applyGlobalMiddlewareinstallsmiddleware.APIKeyAuthons.Router.
Step 2:APIKeyAuthrejects a request with a missing/invalid key before handlers run, and on success setsauth_level(and optional caller-agent context) before callingNext.
Step 3: In the added tests,TestBatchHandler_SingleOperationcreatesgin.New()and manually installsBatchHandler(router)(batch_test.go:33-37);TestQueryHandler_DefaultLimitinstallsQueryHandlerat/query(query_handler_test.go:34-35); discover tests either directly setauth_levelor omit middleware (discover_test.go:18-19,86-89). None executes the server's router orAPIKeyAuth.
Step 4: ProductionBatchHandlercallsrouter.ServeHTTPfor each nested operation (batch.go:91), whererouteriss.RouterfromregisterAgenticRoutes; thus a real batch subrequest runs the global middleware again, while the synthetic tests only run unprotected synthetic endpoints. A regression in route/middleware composition or in the authenticated nested-request path would therefore pass all added tests while changing real/api/v1/agentic/*behavior.
💡 Suggested Fix
Add a server-level test around setupRoutes() using a non-empty configured API key. Assert 401 without the key and successful authenticated requests at /api/v1/agentic/discover, /api/v1/agentic/query, and /api/v1/agentic/batch; make the batch operation invoke an authenticated registered route to exercise the nested middleware pass.
Public API server-composition test coverage · confidence 94%
🤖 Reviewed by AgentField PR-AF
|
|
Realigns embedded mirrors with skills/ sources inherited from the main merge so skillkit drift tests pass branch-locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b75f01d to
a10ae27
Compare
…rding Closes the patch-coverage gap on the caller-identity fix: the empty-page branches for executions/workflows/sessions (only agents was exercised) and the X-Agent-Node-ID forwarding branch in batch sub-requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Changes
control-plane/internal/handlers/agentic/query_handler_test.go— 293 lines covering query handler dispatch, auth propagation, error envelopescontrol-plane/internal/handlers/agentic/batch_test.go— 249 lines covering per-item status, batch dispatch, concurrency integritycontrol-plane/internal/handlers/agentic/discover_test.go— 213 lines covering pagination, output formats, error handlingTest plan
go test -cover ./internal/handlers/agentic/passes with 92.6% coveragego test -count=1 ./internal/handlers/agentic/— all 755 lines of new tests passCloses #409
🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField