diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 95bf373b1..74087e691 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -57,13 +57,16 @@ to recover a lost creation response. Session metadata updates require a supplied metadata field, with null/empty clearing it. Validate an empty update before any resource lookup, after authentication. -List order parsing distinguishes omission from an explicit empty value. Reuse the -shared parser and error serializer, preserving the observed Beta, Files and Skills -error fields rather than applying one error code to every resource. Qualification -of one query error does not authorize changing page bounds, cursor ownership or -parent lookup order. Record uncertain range/lookup behavior separately; do not -reproduce observed upstream server failures as compatibility behavior. See -`contracts/agents-api/list-query-semantics.md` for the bounded evidence. +List order parsing distinguishes omission from an explicit empty value. Lists read by +the shared list parser and single-resource routes ignore unknown query keys; a +repeated supported list key still rejects. The Environment Files list keeps its own +strict key parser and still rejects unknown keys; that difference is deferred. Reuse the shared parser and error serializer, preserving the observed +Beta, Files and Skills error fields and per-family limit bounds rather than applying +one policy to every resource. Change page bounds, cursor ownership or parent lookup +order only with owned evidence for that family. Record uncertain range/lookup +behavior separately; do not reproduce observed upstream server failures as +compatibility behavior. See `contracts/agents-api/list-query-semantics.md` for the +bounded evidence. Keep runtime state, test artifacts and build output under `~/.parsar/`. Require absolute user-supplied working directories. Keep credentials out of source and @@ -1359,8 +1362,9 @@ replaced; do not carry obsolete compatibility code forward to satisfy this secti Status accepts `active`/`archived` as a scalar or SDK `status[]` array, with both included by default. Private stored classification defaults existing/new rows to active; it is never exposed in the Vault response. Listing reads no Credentials - and needs no encryption key or execution service connection. Mixed status encodings - and repeated scalar parameters are rejected locally. Exact hosted errors, equal-time + and needs no encryption key or execution service connection. A scalar status + combined with `status[]` filters by their union; a repeated scalar parameter is + rejected. Exact hosted errors, equal-time ordering and changes between pages remain unverified. Private archived fixtures prove filtering only: there is no public archive writer, archive timestamp or inferred delete-to-archive behavior. Retrieval, Session binding and dispatch retain @@ -1559,13 +1563,17 @@ replaced; do not carry obsolete compatibility code forward to satisfy this secti accepted Session; new references cannot resolve an absent source. Historical identities retain their documented limitation. Exact hosted errors and ordering of overlapping source creation/deletion remain unverified; no tombstone or - successful result is fabricated for an absent resource. Reject query/body data. + successful result is fabricated for an absent resource. Reject body data; unknown + query keys are ignored. - Reusable Agent listing uses the same tenant/Beta-header and response mapping as create/retrieve. Page by `(created_at, id)` with a same-tenant saved-Agent cursor; listing never resolves Sessions, product objects or execution capabilities. - Reuse shared list-query parsing. Agent requests accept positive int64 limits and - return at most 100 records per page with accurate continuation; other resources - retain their current 1..100 request rule. The local default is 20. Return the + Reuse shared list-query parsing and its per-family limit policy. Agent, Session, + Item and Template lists treat limit 0 as 1 and larger limits as 100; Vault and + Credential lists also clamp negative limits; Turn, Subagent and Artifact lists + reject limits outside 1–100; Skill lists accept 0–100, where 0 returns an empty + page; Files accept 1–10000. Pages hold at most 100 records (Files 10000) with + accurate continuation. The local default is 20 (Files 10000). Return the list envelope with data/has_more and first/last IDs (null for empty pages). Exact pinned upstream default/cap, empty-envelope and error semantics remain unverified; do not present local limits or generic SDK parsing as full conformance. diff --git a/contracts/agents-api/README.md b/contracts/agents-api/README.md index 52983e3ca..30b046669 100644 --- a/contracts/agents-api/README.md +++ b/contracts/agents-api/README.md @@ -251,8 +251,9 @@ upgrade the protocol. classification is stored, never returned; existing/new Vaults default active. Synthetic archived fixtures prove read/filter behavior only. No public archive writer or delete-to-archive mapping is implemented. Equal creation times use ID - order locally; repeated scalars and mixed status encodings are rejected. Exact - hosted query errors and pagination over changing data remain unverified. + order locally. A repeated scalar status is rejected; a scalar combined with + `status[]` filters by their union. Other hosted query errors and pagination over + changing data remain unverified. - `DELETE /vaults/{vault_id}` returns `id`, `deleted: true` and `object: vault.deleted` after project-scoped parent removal and atomic cascade of all stored Credentials. It needs no encryption key or execution connection. Local parent/child reads, @@ -308,8 +309,9 @@ upgrade the protocol. 404. Exact hosted errors and overlapping creation/deletion ordering are unverified. - `GET /agents` lists tenant-owned reusable resources with `after`, `limit` and `order` (default `desc`). It uses creation-time/ID keysets and the same resource - mapping as retrieval. Positive int64 limits are accepted; pages contain up to - 100 resources, with `has_more` and the final resource ID guiding continuation. + mapping as retrieval. Limit 0 is treated as 1 and larger limits as 100; negative + and non-integer limits reject. Pages contain up to 100 resources, with `has_more` + and the final resource ID guiding continuation. The local default is 20. The list envelope includes `object`, `data`, `has_more`, `first_id` and `last_id`; empty pages use null IDs. The pinned SDK omits null limits and empty cursors. Exact upstream default/cap, empty-envelope nullability @@ -370,7 +372,8 @@ upgrade the protocol. and character limits as creation. Preserve execution state, effective configuration and the original creation retry identity. Fixed SDK/raw HTTP checks cover these distinctions, tenant isolation, active Session reads and restart persistence. -- `GET /agents/sessions` accepts `after`, `limit` (1..100, default 20), `order` +- `GET /agents/sessions` accepts `after`, `limit` (default 20; 0 is treated as 1 and + values above 100 as 100), `order` (default `desc`) and optional `agent_id`. The filter matches the immutable root Agent ID, including inline IDs and Sessions whose saved source was changed or deleted. Filter before pagination within the authenticated tenant; no source diff --git a/contracts/agents-api/environment-files.md b/contracts/agents-api/environment-files.md index 26881ebf0..8f256ba5b 100644 --- a/contracts/agents-api/environment-files.md +++ b/contracts/agents-api/environment-files.md @@ -35,7 +35,9 @@ returns `data` and `next` (null on the final page), with no additional page fiel Omitted path selects the workspace root. Do not recurse or follow symlinks; directory, symlink and other non-regular entries are omitted. - Omitted limit uses 20. Query keys may occur once; empty values, unknown keys and - malformed query encoding are rejected. The pinned SDK's + malformed query encoding are rejected. This list keeps its own key parser; unlike + the shared list parser it still rejects unknown keys, a deferred difference. Its + limit range errors use the Beta `invalid_request_error` code. The pinned SDK's [query serializer](https://github.com/openai/openai-python/blob/d7c41efee1b0802b79f3f88a678ef2052b06e9ce/src/openai/_qs.py) omits scalar `None` values, so nullable limit/path follow omission behavior. Literal `null` and empty scalar query values are not accepted. @@ -73,8 +75,8 @@ The pinned create union requires `type: inline`, standard Base64 `data` and an absolute destination `path` under `/workspace`, or `type: file_id`, `file_id` and that path. Source IDs resolve only within the authenticated execution project; filenames, URLs and filesystem paths cannot substitute for an ID. Both members -use the same destination writer. Required null/omitted fields, extra fields, -query parameters and invalid Base64 are rejected. Empty bytes are valid. Inline +use the same destination writer. Required null/omitted fields, extra fields and +invalid Base64 are rejected; unknown query keys are ignored. Empty bytes are valid. Inline paths must be canonical and cannot name the workspace root; the parent must exist. The current destination limit is 50 MiB for either source, with bounded JSON and 64 KiB daemon frames. These are local limits and policies, not verified upstream restrictions. diff --git a/contracts/agents-api/environment-templates.md b/contracts/agents-api/environment-templates.md index 5dc2f20f6..fc72d7c2b 100644 --- a/contracts/agents-api/environment-templates.md +++ b/contracts/agents-api/environment-templates.md @@ -19,7 +19,8 @@ and five-operation SandboxProvider path as inline configuration. - Empty/null installation fields retain empty defaults. Responses contain safe metadata and never `env`, `setup_commands` or inline file data. Initial files are supported as described below, together with inline/referenced Skills, env, ordered setup and system/npm/Python packages; remaining populated installations reject explicitly. -- Listing uses `after`, `limit` (1–100, default 20), and `order` (default `desc`). +- Listing uses `after`, `limit` (default 20; 0 is treated as 1 and larger values as + 100), and `order` (default `desc`). Creation timestamp plus ID supplies stable local ordering. Missing/foreign IDs and cursors return the same not-found result. No compute is allocated by CRUD. - Session `environment_template_id` resolves under the caller's tenant. Omitted diff --git a/contracts/agents-api/environments.md b/contracts/agents-api/environments.md index 0dab75d58..f515682ff 100644 --- a/contracts/agents-api/environments.md +++ b/contracts/agents-api/environments.md @@ -139,7 +139,7 @@ is an inventory, not a replacement schema. | [Files](https://github.com/openai/openai-python/blob/d7c41efee1b0802b79f3f88a678ef2052b06e9ce/src/openai/resources/beta/agents/environments/files.py) | `POST`, `GET /agents/environments/{id}/files` | Create accepts `file_id` or inline base64 data with an absolute path inside `/workspace`. List uses opaque `page`, not `after`, with stable path/order/limit across pages. | These are eight operations, separate from Session creation and live events. -Templates use an `after` cursor and limit 1–100, default 20; file listing has nullable +Templates use an `after` cursor and limit default 20, clamped to 1–100; file listing has nullable limit/path, non-null order/page when supplied, and case-sensitive path-component ordering. Both default to descending order. Do not reuse cursor decoding merely because both endpoints paginate. diff --git a/contracts/agents-api/file-resource-semantics.md b/contracts/agents-api/file-resource-semantics.md index 31621ecb9..9683d9175 100644 --- a/contracts/agents-api/file-resource-semantics.md +++ b/contracts/agents-api/file-resource-semantics.md @@ -28,8 +28,8 @@ The qualified list values are `user_data`, `assistants`, `batch`, `fine-tune`, `vision`, `evals`, `assistants_output`, `batch_output` and `fine-tune-results`. These observations establish validation before a missing cursor, not successful filtering for every purpose. Omitted and explicitly empty purpose also reached -cursor lookup. Core retains its existing exact empty filter; successful upstream -empty-filter semantics remain unverified. Accepting a list filter does not enable +cursor lookup. Core then retained its exact empty filter; the later [list query tolerance](list-query-semantics.md#list-query-tolerance--september-23-2026) +batch treats an explicit empty purpose as omitted, as a successful official page showed. Accepting a list filter does not enable uploads, processing or jobs for that purpose. Core still uploads user_data only. The fixed request/response `evals` union discrepancy is unchanged. diff --git a/contracts/agents-api/list-query-semantics.md b/contracts/agents-api/list-query-semantics.md index 95d5d3622..1554fb972 100644 --- a/contracts/agents-api/list-query-semantics.md +++ b/contracts/agents-api/list-query-semantics.md @@ -99,3 +99,89 @@ after inspecting the full diff and evidence; API tests and `git diff --check` passed independently. Server logs are retained under `~/.parsar/remediation/20260923/list-query-alignment/`. The remaining limits, cursor, lookup and Files verbose error differences above are not declared compatible. + +## List query tolerance — September 23, 2026 + +The pin is unchanged: SDK 3.13.0, commit `d7c41ef`, `agents=v1`. This batch starts +from main `284cbcf` and aligns query-string handling with owned official observations +from campaign scan 1. Findings with request IDs are retained in +`~/.parsar/remediation/20260923/campaign-scan-1/{vaults-agents,sessions,skills-files-templates}/findings.json`; +the batch plan is `~/.parsar/remediation/20260923/list-query-tolerance/PLAN.md`. +Parts of the deferred list above are now qualified; the remaining parts stay +deferred below. + +| Row | Case and families | Core behavior | Evidence (finding: request ID) | +| --- | --- | --- | --- | +| A1 | Unknown list key: Agents, Sessions, Turns, Items, Templates, Vaults, Credentials, Skills, Skill versions, Files, Subagent and Artifact lists | Ignored; the page equals the request without the key | VA-02: `req_03f2f965efd54bfab3d22d079121e604`, `req_00357eb8b919413483dba9b59fb18174`; SES-17: `req_8186446b7f68412b8e80e6ec03f8d6d0`; SFT-11: `req_89660f3432624f57afe2c103356fec45` | +| A2 | Unknown key on single-resource GET/POST/DELETE routes | Ignored, including `tenant_id` and `include`; missing and foreign resources still return the same 404 | VA-02: `req_29b59b9bf58a482ca00222b86b542c8a` (deleted Vault read), `req_1aa8acb5602f4a09b7043885d42fca68` (deleted Agent delete) | +| B1 | Repeated supported key on Beta lists, including scalar `status` | 400, code `invalid_request_error`, param null, ``Failed to deserialize query string: duplicate field `` `` | VA-03: `req_83ee26a0b9ba4e84af8996a9346f261b`, `req_de6a02d9a9c547e490e6e53aeb45544d`, `req_92b924b186cf48488cf625638130bb16`; SES-16: `req_86ab2f32451a426399e76b21debeadba`; SFT-24: `req_794a86f4e72946dfb688a2e6f23fff32` | +| B2 | Repeated supported key on Skills and Skill versions | 400, code `duplicate_parameter`, param ``, with the observed message | SFT-10: `req_36e64628c2a44b1198d00949c4ed6cc8` | +| B3 | Repeated key on Files | Unchanged local `unsupported_parameter` rejection | SFT-18: `req_9a38e0628b4e40bd8e7202ac0880209d` accepted one identical repeated `purpose`; a single sample does not define which differing value wins | +| C1, C2 | `limit=0` and `limit>100` on Agents, Sessions, Items, Templates | Clamped to 1 and 100 | VA-01: `req_4954e90768b54c588167333e83f0a958`; VA-18: `req_d43d918872cb40b5b6e545582ea94205`, `req_2efee54ffd6f41179e294870b0e62e02`; SES-10: `req_4f1fb6cb479a4d1cb64486c72d250e9f`; SES-11: `req_5af4e60768b14c9eab284bce8638348a`; SES-12: `req_2f7cab87400348408ab8bb6e68789dc9`; SES-13: `req_b5ebab5691314bfba6ad59c84c42d04e`; SFT-23: `req_f63fef0e08c1462c8158c0ee8523a88d` | +| C3 | `limit` 0 or above 100 on Turns | 400, code `invalid_request_error`, param null, `limit must be between 1 and 100` | SES-14: `req_2793f57b6a454c399a031277b6a02e45` | +| C4 | Negative or non-integer `limit` on Beta lists other than Vaults and Credentials | 400, code `invalid_request_error`, param null, `Failed to deserialize query string: limit: invalid digit found in string` | VA-04: `req_1362046e9d69497da9c23ca69517a026`, `req_c384455192dc4a99a032299a91416a74`; SES-15: `req_918128738f3a47b69203ca091f9cb9ca` | +| C5 | Vault and Credential `limit` | 0, negative and above-100 values keep the pinned clamp; a non-integer uses the C4 error | VA-04: `req_f08cda4e1b9048808be9465b9a56c4a4`, `req_e53033a2e01e4b87aa0bb8d32c932a44`; VA-06 (pin conflict): `req_2ca22663c9414afa920b5509d3574812`, `req_1d088639a3614f6ba545cd36497ff35c`; VA-18: `req_bc47269acf8143f7869908567272e433`, `req_62eaf8fd15e8483a98a9a09ebc4d5e51` | +| C6 | Skills and Skill versions `limit` | `0` returns 200 with empty `data`, null first/last IDs and `has_more` true only if a resource follows the cursor; above 100 is `integer_above_max_value`, below 0 is `integer_below_min_value`, both with param `limit` | SFT-08: `req_b71c126adaf3437d9b01aee3ec913863`, `req_218a5d425b7047a190e2c5dc2e047e81`; SFT-09: `req_0ed2668ecab54253ab40d2655954de2c`, `req_4dd2e453717f4f82b5a7921a34d68b0f` | +| C7 | Files `limit` 0 or 10001 | 400, code null, param null; the local message stays | SFT-17: `req_05ec0b8f86b445b2beb2fdfc595d0879` | +| D1 | Scalar `status` plus `status[]` on Vaults and Credentials | 200, filtering by the union; invalid values still reject | VA-05: `req_1e89a6740ecd49b5b3b42f514a144e71`, `req_6223c9b384a246df848cfb94ffe2144d` | +| D2 | Explicit empty `purpose=` on Files | Same as omission, no filter | SFT-14: `req_af6df1ba19fd428fba6b1a66745ce37f` | + +### Decisions + +The pinned Turn, Item and Template docstrings say "between 1 and 100". A server +that clamps out-of-range values keeps each effective page inside that documented +range, so clamping Items and Templates to match the live service does not contradict +the pin. Turns keep rejecting because the live service rejects. Vault and Credential +negative limits keep clamping because their pinned description says values are +"clamped between 1 and 100"; the official 400 for `-1` (VA-06) remains a recorded +pin conflict. + +No pinned single-resource operation sends a typed query parameter, including +Environment retrieval, so no single-resource route keeps a query rejection. + +Unchanged: every default (20, Files 10000), the maximum page capacity (100, Files +10000), `order` handling, cursor lookup order, `after` trimming, authentication, +tenant scoping and missing/foreign masking. A `tenant_id` query key is an ignored +unknown key; only authentication selects the tenant. Shared-parser list rejections +happen before any resource lookup, so tenant B receives the same error. The +Environment Files list validates its query only after the Environment lookup, so a +foreign or missing Environment returns 404 first. No rejected request writes. + +Families are still selected by path, as for order errors. Local choices for +unsampled inputs: + +- Subagent and Artifact lists keep rejecting 0 and values above 100, now with the + Turn error fields. +- Beta limits above the signed 64-bit range still reject, with code + `invalid_request_error` and `...limit: number too large to fit in target type`. + A leading sign or any other non-digit input, including an empty value, uses the + C4 message. +- A non-integer Skills limit keeps the local `invalid_request` code; its message now + names the 0–100 range. A non-integer Files limit keeps `invalid_request`. The + hosted Files schema message and `detail` member are not copied. +- Duplicate keys are reported before value errors, and limit errors before order + errors. Vault status validation still precedes limit and order. Precedence + between simultaneous errors was not sampled. + +### Deferred + +These remain registered differences and are not changed here: repeated Files +`purpose` values (SFT-18); unsampled overflowing limits; unknown and repeated keys +on the Environment Files list, which keeps its own strict parser (its limit range +errors now use the Beta code through the shared limit reader); malformed non-UUID +path IDs (SES-28); metadata and name error envelopes (VA-07/08/09) and U+0000 +(VA-10); Skill sole-version deletion and number reuse (SFT-01/02); Session deletion +lifecycle (SES-29/30); whitespace input (SES-01..04); Template network forms and +codes (SFT-20/21/22); and response defaults (VA-11, SES-23/25). + +### Acceptance boundary + +Go handler and parser tests cover every row, including tenant isolation, and the +Skills store test covers the zero page against PostgreSQL. `official_list_query.py` +replays rows A1–D2 through raw HTTP and the pinned SDK against Core and a dedicated +PostgreSQL database as tenant A and tenant B, and confirms that rejected requests +change no resource. It also checks that single-resource reads, event admission, +streams, updates and deletions ignore unknown keys. This is resource and query +acceptance with no model execution: query parsing does not affect execution, so +live model acceptance does not apply. The independent batch acceptance, server gate +and review are recorded separately when complete. diff --git a/contracts/agents-api/openapi.yaml b/contracts/agents-api/openapi.yaml index 7e7b6e46f..611c1b11b 100644 --- a/contracts/agents-api/openapi.yaml +++ b/contracts/agents-api/openapi.yaml @@ -2031,9 +2031,9 @@ paths: /agents: get: description: Lists only the authenticated tenant's saved Agents, independently - of Sessions. Positive int64 limits are accepted; each page returns at most - 100 resources with continuation. The local default is 20; exact upstream default/cap, - empty cursor fields and error conformance remain unverified. + of Sessions. Limit 0 is treated as 1 and larger limits as 100, as observed + on the hosted service. The local default is 20; exact upstream default/cap + and empty cursor fields remain unverified. parameters: - description: agents=v1 in: header @@ -2044,9 +2044,10 @@ paths: in: query name: after type: string - - description: Maximum requested resources; pages contain at most 100 + - default: 20 + description: Page size; 0 is treated as 1 and values above 100 as 100 in: query - minimum: 1 + minimum: 0 name: limit type: integer - default: desc @@ -2490,9 +2491,9 @@ paths: /agents/environments/templates: get: description: Lists tenant-owned safe template metadata in creation order with - ID tie-breaking. Defaults to limit 20 and descending order; limit must be - 1–100. Foreign and missing cursors reject identically. Concurrent-page and - exact hosted error behavior remain unverified. + ID tie-breaking. Defaults to limit 20 and descending order; limit 0 is treated + as 1 and larger limits as 100. Foreign and missing cursors reject identically. + Concurrent-page and exact hosted error behavior remain unverified. parameters: - description: agents=v1 in: header @@ -2504,10 +2505,9 @@ paths: name: after type: string - default: 20 - description: Page size + description: Page size; 0 is treated as 1 and values above 100 as 100 in: query - maximum: 100 - minimum: 1 + minimum: 0 name: limit type: integer - default: desc @@ -2768,10 +2768,9 @@ paths: name: after type: string - default: 20 - description: Page size + description: Page size; 0 is treated as 1 and values above 100 as 100 in: query - maximum: 100 - minimum: 1 + minimum: 0 name: limit type: integer - default: desc @@ -3499,10 +3498,9 @@ paths: name: after type: string - default: 20 - description: Page size + description: Page size; 0 is treated as 1 and values above 100 as 100 in: query - maximum: 100 - minimum: 1 + minimum: 0 name: limit type: integer - default: desc @@ -4056,9 +4054,9 @@ paths: description: Lists project-owned Files without reading their bodies. The limit defaults to 10000 and must be 1–10000. Equal creation times use ID ordering. Purpose validation precedes cursor lookup; current storage contains only user_data. - An explicit empty purpose retains the local exact-filter behavior. Hosted - positive filtering, default order and concurrent-page behavior remain unverified. - No Beta header is required. + An explicit empty purpose is treated as omitted. Repeated purpose values remain + rejected. Hosted positive filtering, default order and concurrent-page behavior + remain unverified. No Beta header is required. parameters: - description: Last File ID from the previous page in: query @@ -4299,14 +4297,18 @@ paths: /skills: get: description: Lists tenant-owned metadata in timestamp order. Default page size - 20, maximum 100; exact hosted defaults and limit-zero semantics remain unverified. + 20, maximum 100. Limit 0 returns an empty page whose has_more reports whether + any Skill follows the cursor; exact hosted defaults remain unverified. parameters: - description: Skill resource cursor in: query name: after type: string - - description: Page size + - default: 20 + description: Page size; 0 returns an empty page in: query + maximum: 100 + minimum: 0 name: limit type: integer - description: Creation order; omit for descending, explicit empty values are @@ -4451,7 +4453,8 @@ paths: /skills/{skill_id}/versions: get: description: Orders by version number; after identifies a version resource, - not a version number. No contents are decrypted. + not a version number. No contents are decrypted. Limit 0 returns an empty + page whose has_more reports whether any version follows the cursor. parameters: - description: Skill ID in: path @@ -4462,8 +4465,11 @@ paths: in: query name: after type: string - - description: Page size + - default: 20 + description: Page size; 0 returns an empty page in: query + maximum: 100 + minimum: 0 name: limit type: integer - description: Version order; omit for descending, explicit empty values are @@ -4595,8 +4601,8 @@ paths: /vaults: get: description: Lists project-owned Vaults independently of execution. Includes - active and archived records by default. Status accepts a scalar or the SDK's - status[] array; mixed encodings and repeated scalars are rejected locally. + active and archived records by default. Status accepts a scalar, the SDK's + status[] array or both, filtering by their union; a repeated scalar is rejected. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering; exact hosted errors and concurrent-page behavior remain unverified. Archive/delete lifecycle is not implemented. @@ -4632,7 +4638,7 @@ paths: name: status type: string - collectionFormat: multi - description: Array status filter; cannot be combined with status + description: Array status filter; combined with status as a union in: query items: enum: @@ -4817,8 +4823,8 @@ paths: get: description: Lists only metadata from the authenticated project's requested Vault, without decryption or execution. Includes active and archived Credentials - by default, independently of Vault status. Status accepts a scalar or SDK - status[] array; mixed encodings and repeated scalars are rejected locally. + by default, independently of Vault status. Status accepts a scalar, the SDK + status[] array or both, filtering by their union; a repeated scalar is rejected. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering. Hosted errors, concurrent-page behavior and archive/delete lifecycle remain unverified or unimplemented. @@ -4859,7 +4865,7 @@ paths: name: status type: string - collectionFormat: multi - description: Array status filter; cannot be combined with status + description: Array status filter; combined with status as a union in: query items: enum: diff --git a/contracts/agents-api/operation-evidence.md b/contracts/agents-api/operation-evidence.md index e5fd55e25..8ac76c37c 100644 --- a/contracts/agents-api/operation-evidence.md +++ b/contracts/agents-api/operation-evidence.md @@ -1,6 +1,6 @@ # Pinned operation evidence inventory — 2026-09-23 -Baseline inventory of main `b5715912f09333e2b4449ec6f0eecaabce44c9b7`. The Session admission batch below updates creation and metadata validation; historical evidence retains its original revision and scope. This inventory guides repeated qualification and does not assert complete compatibility. +Baseline inventory of main `b5715912f09333e2b4449ec6f0eecaabce44c9b7`. The Session admission batch below updates creation and metadata validation, and the list query tolerance batch (L) updates list and resource query handling; historical evidence retains its original revision and scope. This inventory guides repeated qualification and does not assert complete compatibility. Baseline: `contracts/agents-api/upstream.json`, SDK **3.13.0**, upstream commit **d7c41efee1b0802b79f3f88a678ef2052b06e9ce**, `OpenAI-Beta: agents=v1`. AGENTS.md and relevant CONTRIBUTING.md compatibility, ownership and evidence rules govern this inventory. @@ -36,6 +36,7 @@ Repository paths below are relative to the inspected worktree; private evidence | O | `services/agents-api/oauth-credentials.md:144`: recorded genuine Keycloak 26.7.4 PKCE grants, real TLS MCP and Kimi execution. Codex Basic client-auth initial/refresh/restart/replacement/revocation/delete; Claude POST initial/refresh. Trusted `none` profiles only; not MiniMax, hosted, arbitrary-provider or complete error equivalence. | | V | [Resource selector and error qualification](resource-selector-semantics.md); private September23 `skill-version-alignment/official/` and `files-error-alignment/official-probe.json`: owned Skill/Template/Session selector observations and seven missing File/cursor requests. Preserve each probe phase and distinguish actual hosted execution from metadata-only reads. | | W | `~/.parsar/remediation/20260923/file-resource-semantics/official-files/` and `official-skills/`: 56 owned resource requests, no Sessions/models; [qualified observations and limitations](file-resource-semantics.md). | +| L | [List query tolerance](list-query-semantics.md#list-query-tolerance--september-23-2026); private `~/.parsar/remediation/20260923/campaign-scan-1/{vaults-agents,sessions,skills-files-templates}/findings.json`: owned-collection unknown/repeated keys, limit bounds, Vault status union and Files empty purpose, plus unknown keys on a deleted Vault read and Agent delete. Rows A1–D2 of that section; no model execution. | ## Per-operation evidence matrix @@ -46,18 +47,18 @@ Paths in the appendix include `/v1`. SDK names here omit `client.`. `P` means pa | 1 | beta.agents.create | P: saved configuration, 201 | R `agent-create-supported`; initial unsupported model case 400 | C DB create; T saved-Agent execution references | Model-derived reasoning defaults and unsupported configurations; complete default/null/errors unknown | | 2 | beta.agents.retrieve | P: tenant-owned saved read | R `agent-read`, `agent-read-deleted` | C DB own/foreign/deleted read | Full field defaults and inline-vs-saved lifetime | | 3 | beta.agents.update | P: atomic replacements; empty body touches timestamp | R `agent-patch-metadata`, `agent-null-fields`, `agent-noop`, `agent-nested-reasoning`, rejection labels | C DB no-op/unchanged snapshot; resource implementation-validation.md actual PostgreSQL SDK update tests | Model-dependent default recomputation; uncommon nested/null/error variants | -| 4 | beta.agents.list | P: scoped cursor list | R `agent-list-empty-scoped`, `agent-list-limit101` | Recorded controlled SDK/DB coverage; no positive list in C resource replay | Official 101 accepted in sampled empty page; Core generic max 100. No inferred official cap | +| 4 | beta.agents.list | P: scoped cursor list; unknown keys ignored, limit 0/above 100 clamp | R `agent-list-empty-scoped`, `agent-list-limit101`; L VA-01/02/03/04/18 | L DB `official_list_query.py` tenant A/B | Core page capacity 100; no inferred official cap. Overflowing limits unsampled | | 5 | beta.agents.delete | P: resource deletion | R `cleanup-agent`, subsequent 404 | C DB delete/post-delete | Referenced/in-flight/repeated-delete exact parity | | 6 | beta.agents.sessions.create | P: JSON/live SSE 201, saved/inline frozen config, initial messages, native profiles | S `create-1/2.json`, `omitted-input.json`, `null-input.json`, `empty-array-input.json`, `retry-status-original/repeat.json`; H stream | N Live none admission and retry; C Live three hosted profiles; D/T/K/I recorded additional workflows | Session admission batch removes idle `none` creation; local idempotent create still differs from two official IDs. Many input/tool/environment combinations restricted | | 7 | beta.agents.sessions.retrieve | P: persisted state, required actions, usage | S `retrieve-1.json`, `session-after-1.json`; H recovered state | C Live history; T pending actions; H Core acceptance recorded | Complete statuses/actions/lifecycle timing; Claude/MiniMax public usage remains null | | 8 | beta.agents.sessions.update | P: metadata-only replacement/clear | S `metadata-replace/null/empty/omit/invalid-value.json`; `update-agent.json` uses newer unpinned field | N Live completed Session metadata rejection/clear/isolation; recorded active controlled metadata coverage | Session admission batch changes empty update to observed official 400; Session agent update belongs to baseline upgrade, not fixed-pin operation gap | -| 9 | beta.agents.sessions.list | P: Agent filter, full envelope, cursor paging | S `list-filter.json`, `list-empty-after.json`, `list-owned-cross-filter-cursor.json`, limit/order/unknown-query samples | C Live order/cursors/empty/tenant checks | Official >100 sample accepted; Core max 100. Empty order/unknown query differences; eventual visibility sample is not a required delay | +| 9 | beta.agents.sessions.list | P: Agent filter, full envelope, cursor paging; unknown keys ignored, limit 0/above 100 clamp | S `list-filter.json`, `list-empty-after.json`, `list-owned-cross-filter-cursor.json`, limit/order/unknown-query samples; L SES-10/11/15/16/17 | C Live order/cursors/empty/tenant checks; L DB tenant A/B | Core page capacity 100; official cap unknown. Eventual visibility sample is not a required delay | | 10 | beta.agents.sessions.delete | P: public deletion, owned managed cleanup, user compute retained | S cleanup files 200/deleted; retry-session active cleanup initially 409 | D recorded real cleanup; C cleanup separately recorded | Physical purge/retention and all active/unknown-effect races; caller compute ownership preserved | | 11 | beta.agents.sessions.events.create | P: 202/empty body, empty-array authenticated no-op, text/cancel/function admission | S `second-turn-create.json`, `events-empty/null.json`; H second-input | C Live real continuation/no-op; T qualified message/result/cancel workflows | Mixed prepared-environment batches, native receipt vs durable acceptance, cancel-before-result-publication timing; unqualified content/tools | | 12 | beta.agents.sessions.events.stream | P: live-only SSE, typed persisted projections | H create/reconnect frames; no historical frames in sampled idle interval | C Live; H recorded three-harness disconnect/recovery; T pending actions | Full SSE/Item variants/order; child deltas settle late; no replay guarantee or observer-disconnect proof for every state | | 13 | beta.agents.sessions.turns.retrieve | P: persisted root/child Turn identity | H/S contain Turn list payloads; no isolated positive retrieve raw request identified in this set | Recorded H/B scoped Turn recovery; C history uses list | Distinguish list-shape evidence from retrieve wire qualification; full lifecycle/usage | -| 14 | beta.agents.sessions.turns.list | P: full envelope, ordered root+child history | S `turns-final/empty-page/limit-high/order-empty.json`; H both directions | C Live paging; H/B real child/root identity recorded | All interleavings, same-timestamp paging, interim/failed usage | -| 15 | beta.agents.sessions.items.list | P: scoped root Items, full envelope | S `items-final/empty-page/limit-high.json`; H both directions | C Live; H/T recorded content/coordination/result variants | Full Item union; official 101 accepted despite pinned 1–100 doc. Newer turn_id filter excluded from pin | +| 14 | beta.agents.sessions.turns.list | P: full envelope, ordered root+child history; limit outside 1–100 rejects with the Beta code | S `turns-final/empty-page/limit-high/order-empty.json`; H both directions; L SES-14/15/16/17 | C Live paging; H/B real child/root identity recorded; L DB tenant A/B | All interleavings, same-timestamp paging, interim/failed usage | +| 15 | beta.agents.sessions.items.list | P: scoped root Items, full envelope; limit 0/above 100 clamp within the pinned 1–100 page | S `items-final/empty-page/limit-high.json`; H both directions; L SES-12/13/15/16/17 | C Live; H/T recorded content/coordination/result variants; L DB tenant A/B | Full Item union. Newer turn_id filter excluded from pin | | 16 | beta.agents.sessions.artifacts.retrieve | P: immutable captured metadata | None located | D recorded live Docker/user-managed workspace output | Exact hosted metadata/default/error and capture-edge parity | | 17 | beta.agents.sessions.artifacts.list | P: scoped stored list | None located | D recorded live output enumeration | Paging during capture/delete; unchanged-file republishing | | 18 | beta.agents.sessions.artifacts.delete | P: stored deletion | None located | D recorded workflow summary; per-case remote evidence not re-read | Repeated/in-flight deletion and physical retention parity | @@ -74,37 +75,37 @@ Paths in the appendix include `/v1`. SDK names here omit `client.`. `P` means pa | 29 | beta.agents.environments.templates.create | P: reusable network/files/env/setup/packages/Skills/Plugins/capability config, 201 | R `template-create`; V nullable Skill selector projection | C DB; I/K recorded real frozen-reference initialization | Restricted forms and unqualified combinations; complete hosted initialization semantics | | 30 | beta.agents.environments.templates.retrieve | P: safe resource read | R `template-read`, deleted owned read; V nullable Skill selector projection | C DB; I/K recorded reference workflow | Full field/default/redaction parity; no live-secret projection inference | | 31 | beta.agents.environments.templates.update | P: field replacement/null clearing, empty timestamp touch | R `template-patch`, `template-null`, `template-noop`; V nullable Skill selector projection | C DB no-op; I/K recorded frozen Session behavior | Template update and referencing Session selection are distinct; composition and null inheritance are qualified separately in environment-templates.md/template-null-selection.md; uncommon fields/errors remain unverified | -| 32 | beta.agents.environments.templates.list | P: scoped cursor list | R `template-list-empty-scoped` | Recorded resource DB checks; no positive C list replay | Full nonempty/multipage/mutation/default/error parity | +| 32 | beta.agents.environments.templates.list | P: scoped cursor list; unknown keys ignored, limit 0/above 100 clamp | R `template-list-empty-scoped`; L SFT-11/23/24 | Recorded resource DB checks; L DB tenant A/B | Multipage mutation and default parity | | 33 | beta.agents.environments.templates.delete | P: delete resource, preserve committed Session snapshot | R `cleanup` at Template path, post-delete read | C DB; K recorded live deletion then continuation | Concurrent references/delete and exact errors | | 34 | beta.agents.vaults.create | P: tenant resource, 201 | R `vault-create`, `vault-empty-token-fixture` | C DB; O recorded MCP attachment workflow | Archive lifecycle, full defaults and selection parity | | 35 | beta.agents.vaults.retrieve | P: safe metadata/status | R `vault-read`, post-cleanup read | C DB deleted/error checks; O recorded lifecycle | Full archived-state and visibility semantics | -| 36 | beta.agents.vaults.list | P: stored status filter, separate clamping pagination | R `vault-list-empty-scoped` | Recorded resource DB coverage; no positive C list replay | Real archive transitions, nonempty pages/filter/limits/error parity | +| 36 | beta.agents.vaults.list | P: stored status filter (scalar and `status[]` union), separate clamping pagination | R `vault-list-empty-scoped`; L VA-02/03/04/05/06/18 | Recorded resource DB coverage; L DB tenant A/B | Real archive transitions; official negative-limit 400 is a recorded pin conflict | | 37 | beta.agents.vaults.delete | P: atomic credential cascade, frozen attachment boundaries | R `cleanup` at Vault path | C DB deletion; O recorded grant lifecycle | Archive vs delete, already-delivered tokens and active effects | | 38 | beta.agents.vaults.credentials.create | P: encrypted static/OAuth, nonempty tokens, 201 | R static/OAuth create + empty token/access-token rejection labels | C DB exact-row preservation/restart; O recorded genuine grants/real MCP | Full provider grants/default/error/selection behavior; qualified none profiles only | | 39 | beta.agents.vaults.credentials.retrieve | P: safe metadata, secret-free read | R `credential-read`, post-delete read | C DB own/foreign/restart; O recorded lifecycle | Full response metadata and archived-state parity | | 40 | beta.agents.vaults.credentials.update | P: static replacement/OAuth grant patch, reject empty effective update | R `credential-rotate`, `credential-empty-token`, `credential-oauth-empty-patch` | C DB rejected-write preservation; O recorded real Codex replacement/refresh | Official empty-access-token update not separately probed; concurrent refresh/replacement/provider errors and in-flight withdrawal | -| 41 | beta.agents.vaults.credentials.list | P: safe scoped metadata/status list | R `credential-list` | C DB mixed list/rejected-write/restart; O recorded resource checks | Complete status-filter/page limits/archive behavior | +| 41 | beta.agents.vaults.credentials.list | P: safe scoped metadata/status list (scalar and `status[]` union) | R `credential-list`; L VA-03/04/05/06/18 | C DB mixed list/rejected-write/restart; O recorded resource checks; L DB tenant A/B | Archive behavior; official negative-limit 400 is a recorded pin conflict | | 42 | beta.agents.vaults.credentials.delete | P: encrypted resource deletion and dispatch denial | R `cleanup` at Credential paths | C DB; O recorded live Codex refusal after deletion | Cannot recall already-delivered token; exact hosted withdrawal/error timing | | 43 | files.create | P: immutable multipart purpose=user_data | W three owned200 uploads (two user_data, assistants control) | F recorded DB upload; D/K recorded native source consumption | Other Core upload purposes/expires_after unsupported; purpose acceptance does not imply platform processing | | 44 | files.retrieve | P: project-owned metadata | E/V missing-ID404 param=id; W positive metadata/status/nulls | F recorded DB workflow; V actual HTTP/PG foreign==missing | Other purposes/errors/defaults beyond the sampled profile | -| 45 | files.list | P: validated purpose filter, default/max10000, cursor list | Q order error; V missing-after404; W nine purpose values reach cursor lookup, unknown/case variant400purpose | F recorded actual PostgreSQL SDK/raw pages/restart/isolation | Successful official filtering, empty-purpose meaning, ordering/concurrent mutation remain unqualified | +| 45 | files.list | P: validated purpose filter, default/max10000, cursor list; unknown keys and empty purpose ignored, range errors null code | Q order error; V missing-after404; W nine purpose values reach cursor lookup, unknown/case variant400purpose; L SFT-11/14/17/18 | F recorded actual PostgreSQL SDK/raw pages/restart/isolation; L DB tenant A/B | Successful official filtering, repeated purpose, ordering/concurrent mutation remain unqualified | | 46 | files.delete | P: transactional metadata/body unlink | V random nonexistent ID404 param=id | F recorded DB deletion; V actualHTTP/PG foreign delete denial preserves bytes; D/K retained workspace copies | Admitted immutable read may complete after deletion; hosted concurrency/retention unknown | | 47 | files.content | P: tenant lookup then user_data400 denial; internal source consumption retained | V missing-ID404param=id; W user_data/assistants400 nullcode/nullparam | Source store/copy regressions; joint acceptance in file-resource-semantics.md | Other-purpose content policy and full error details remain unqualified | | 48 | skills.create | P: encrypted bounded directory/ZIP bundle | V successful owned directory uploads | K recorded DB resources and Live three-harness upload/reference | Fixed SDK single-tuple multipart issue; local upload limits remain partial | | 49 | skills.retrieve | P: safe metadata follows default version | E missing-ID404; W distinct names/descriptions follow default1→2→1 | Skill store and joint resource acceptance | Other error/visibility behavior remains unqualified | | 50 | skills.update | P: atomic default pointer and descriptive metadata update | V default mutation/frozen Sessions; W default1→2→1 name/description changes | Skill store and joint resource acceptance; retained frozen Session regression | Complete errors and concurrent official behavior | -| 51 | skills.list | P: scoped resource list | None located | K recorded DB resources; no model needed | Current documentation minimum zero is only a lead; fixed/common 1–100 implementation and exact official behavior require evidence | +| 51 | skills.list | P: scoped resource list; limit 0 empty page with has_more, 0–100 with observed range and duplicate codes | L SFT-08/09/10/11 | K recorded DB resources; L DB tenant A/B | Non-integer and overflowing limits unsampled | | 52 | skills.delete | P: remove owned source, retain committed Session content | None located | K recorded DB and Live source deletion/continuation | Exact hosted deletion/idempotence/default/latest semantics | | 53 | skills.content.retrieve | P: unversioned content selects default | W distinct default1/latest2 bytes and default2 transition | K recorded DB plus joint resource acceptance | Headers/errors and source deletion/read races | | 54 | skills.versions.create | P: immutable increasing version; optional default change | V second version default=false preserves default1/latest2 | K recorded DB resources | Broader numbering/default/top-level metadata/error/null semantics; upload limits | | 55 | skills.versions.retrieve | P: owned immutable version metadata | V immediate version1 read404, bounded delayed read200 | K recorded DB resources and Live concrete Session freeze | Visibility timing is observational; full selector/metadata/error parity unqualified | -| 56 | skills.versions.list | P: scoped version cursor list | V delayed owned list contains created versions | K recorded DB resource checks | Exact ordering/cursors/zero/default/max limits and concurrent version mutation | +| 56 | skills.versions.list | P: scoped version cursor list; limit 0 empty page with has_more | V delayed owned list contains created versions; L SFT-08/09 | K recorded DB resource checks; L DB zero page | Exact ordering/cursors/default and concurrent version mutation | | 57 | skills.versions.delete | P: nondefault deletion; default rejects invalid_value/version | W two-version default400; latest200 with parent pointer fallback | Skill store and joint resource acceptance | Sole deletion/number reuse unverified; observed stale official version reads are not emulated | | 58 | skills.versions.content.retrieve | P: decrypt/read immutable concrete bundle | W v1/v2 ZIP members and markers | K recorded DB/live consumption; joint resource acceptance | Content headers/errors and source deletion/read races | ## Remaining gaps without task ordering -1. **Public generic semantics:** sampled create/event/envelope/error/no-op corrections are merged. Resource-by-resource omissions/null/default/error params, malformed queries, list caps, unknown/empty query handling, concurrent mutation and deletion require separate evidence. Metadata U+0000 remains an implementation-validation question from the prior audit, not a newly reproduced result here. +1. **Public generic semantics:** sampled create/event/envelope/error/no-op corrections are merged. L aligns unknown/repeated list keys, sampled limit bounds and single-resource unknown keys. Resource-by-resource omissions/null/default/error params, malformed path IDs, overflowing limits, Environment Files list query tolerance, list caps, concurrent mutation and deletion require separate evidence. Metadata U+0000 remains an implementation-validation question from the prior audit, not a newly reproduced result here. 2. **Session differences:** The Session admission batch removes idle `none` creation and empty metadata update. Local durable creation idempotency remains an explicit difference. Whitespace-only input succeeds officially but is rejected by the existing Core message validator; this newly observed difference is queued separately. Session agent updates, newer Environment shapes and root Item turn_id are baseline-upgrade questions. 3. **Template/Skill composition:** shared env/files/setup/packages selection is covered by the composition batch; template-reference null network/capability lists are covered by the null-selection batch. Official derived capability-directory projection remains different. Skill content/default metadata are covered by file-resource-semantics.md; sole-version deletion, visibility and broader numbering/error behavior remain unverified. 4. **Execution coverage:** use T's qualified matrix, not a blanket missing-image/structured-output claim. MiniMax functions/service MCP, optional tool combinations, unsupported images/placements and broader native lifecycle are explicit restrictions. PTC omission retains approved native behavior; Claude/MiniMax public Usage remains null; child settlement cadence/native close limits remain visible. No second executor/model loop or guessed counters are justified. diff --git a/contracts/agents-api/source-files.md b/contracts/agents-api/source-files.md index d0e4b6946..e9cfe3214 100644 --- a/contracts/agents-api/source-files.md +++ b/contracts/agents-api/source-files.md @@ -30,9 +30,9 @@ The cursor must name a currently visible File in the same project. The optional purpose filter accepts the nine values qualified by validation probes (including `evals` and output-purpose names); an unknown or case-variant value returns 400 with `param: purpose` before cursor resolution. Valid other-purpose filters return -an empty page because storage currently accepts only `user_data`. Explicit empty -purpose remains an exact empty filter locally; its upstream successful-page meaning -is unverified. Successful official filtering/order, deleted-cursor behavior and +an empty page because storage currently accepts only `user_data`. An explicit empty +purpose is treated as omitted and unknown query keys are ignored, as observed in the +[list query tolerance](list-query-semantics.md#list-query-tolerance--september-23-2026) batch. Successful official filtering/order, deleted-cursor behavior and pagination during concurrent mutation remain unqualified. Metadata includes `id`, `object: file`, `bytes`, Unix-second `created_at`, diff --git a/services/agents-api/README.md b/services/agents-api/README.md index c72dd6ee6..59fed1505 100644 --- a/services/agents-api/README.md +++ b/services/agents-api/README.md @@ -224,7 +224,8 @@ Execution uses the selected [engine profile](../../contracts/agents-api/README.md#public-engine-profiles), including `none` and the colocated self-hosted profile described below. Ordinary JSON requests have a 1 MiB body limit; file transfers use the separate -bounds in the Files contracts. Session lists support `after`, `limit` (1..100), +bounds in the Files contracts. Session lists support `after`, `limit` (0 is treated +as 1 and values above 100 as 100), `order` (`asc`/`desc`) and optional immutable root `agent_id`. The local defaults are 20 and descending order; exact hosted limits/error semantics remain unverified. Session updates require the metadata field; null/empty clears it and an object diff --git a/services/agents-api/internal/api/agents.go b/services/agents-api/internal/api/agents.go index ae118b149..ba7af6ed5 100644 --- a/services/agents-api/internal/api/agents.go +++ b/services/agents-api/internal/api/agents.go @@ -31,10 +31,6 @@ type AgentStore interface { // @Failure 400,401,413,500 {object} v1.ErrorResponse // @Router /agents [post] func (h *Handler) createAgent(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Agent creation does not accept query parameters.") - return - } raw, ok := readJSONBody(w, r) if !ok { return @@ -68,10 +64,6 @@ func (h *Handler) createAgent(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/{agent_id} [get] func (h *Handler) getAgent(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Agent retrieval does not accept query parameters.") - return - } agent, err := h.lookupAgent(r.Context(), tenantID(r), chi.URLParam(r, "agent_id")) if err != nil { writeStoreError(w, r, err) diff --git a/services/agents-api/internal/api/agents_delete.go b/services/agents-api/internal/api/agents_delete.go index 449d8af64..85b4d876b 100644 --- a/services/agents-api/internal/api/agents_delete.go +++ b/services/agents-api/internal/api/agents_delete.go @@ -20,10 +20,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /agents/{agent_id} [delete] func (h *Handler) deleteAgent(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Agent deletion does not accept query parameters.") - return - } body, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/agents_list.go b/services/agents-api/internal/api/agents_list.go index 505211fcd..5af979863 100644 --- a/services/agents-api/internal/api/agents_list.go +++ b/services/agents-api/internal/api/agents_list.go @@ -7,19 +7,19 @@ import ( ) // @Summary List reusable Agents -// @Description Lists only the authenticated tenant's saved Agents, independently of Sessions. Positive int64 limits are accepted; each page returns at most 100 resources with continuation. The local default is 20; exact upstream default/cap, empty cursor fields and error conformance remain unverified. +// @Description Lists only the authenticated tenant's saved Agents, independently of Sessions. Limit 0 is treated as 1 and larger limits as 100, as observed on the hosted service. The local default is 20; exact upstream default/cap and empty cursor fields remain unverified. // @Tags Agents // @Produce json // @Security BearerAuth // @Param OpenAI-Beta header string true "agents=v1" // @Param after query string false "Last Agent ID from the previous page" -// @Param limit query int64 false "Maximum requested resources; pages contain at most 100" minimum(1) +// @Param limit query int64 false "Page size; 0 is treated as 1 and values above 100 as 100" minimum(0) default(20) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Success 200 {object} v1.SavedAgentList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents [get] func (h *Handler) listAgents(w http.ResponseWriter, r *http.Request) { - options, ok := readPageSize(w, r, false) + options, ok := readClampedPage(w, r) if !ok { return } diff --git a/services/agents-api/internal/api/agents_update.go b/services/agents-api/internal/api/agents_update.go index 5041fdfd1..f71a361ef 100644 --- a/services/agents-api/internal/api/agents_update.go +++ b/services/agents-api/internal/api/agents_update.go @@ -23,10 +23,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /agents/{agent_id} [post] func (h *Handler) updateAgent(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Agent updates do not accept query parameters.") - return - } raw, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/credentials.go b/services/agents-api/internal/api/credentials.go index c1cf7a765..5cb61813e 100644 --- a/services/agents-api/internal/api/credentials.go +++ b/services/agents-api/internal/api/credentials.go @@ -34,10 +34,6 @@ type CredentialStore interface { // @Failure 400,401,404,413,500,503 {object} v1.ErrorResponse // @Router /vaults/{vault_id}/credentials [post] func (h *Handler) createCredential(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Credential creation does not accept query parameters.") - return - } vaultID, ok := credentialResourceID(w, r, "vault_id") if !ok { return @@ -98,10 +94,6 @@ func (h *Handler) createCredential(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /vaults/{vault_id}/credentials/{credential_id} [get] func (h *Handler) getCredential(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Credential retrieval does not accept query parameters.") - return - } vaultID, ok := credentialResourceID(w, r, "vault_id") if !ok { return diff --git a/services/agents-api/internal/api/credentials_delete.go b/services/agents-api/internal/api/credentials_delete.go index b1dd8bafe..1981a78bc 100644 --- a/services/agents-api/internal/api/credentials_delete.go +++ b/services/agents-api/internal/api/credentials_delete.go @@ -19,10 +19,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /vaults/{vault_id}/credentials/{credential_id} [delete] func (h *Handler) deleteCredential(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Credential deletion does not accept query parameters.") - return - } body, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/credentials_delete_test.go b/services/agents-api/internal/api/credentials_delete_test.go index 041787261..b7472df0f 100644 --- a/services/agents-api/internal/api/credentials_delete_test.go +++ b/services/agents-api/internal/api/credentials_delete_test.go @@ -20,20 +20,23 @@ func (f *credentialFixture) DeleteCredential(_ context.Context, tenant, vault, i } func TestCredentialDeletionConfirmationAndScope(t *testing.T) { - h, f, tenant := credentialHandler(t) - w := credentialRequest(h, "DELETE", "/v1/vaults/"+f.credential.VaultID+"/credentials/"+f.credential.ID, "") - var got map[string]any - if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &got) != nil { - t.Fatal("deletion failed", w.Code) - } - want := map[string]any{"id": f.credential.ID, "deleted": true, "object": "vault.credential.deleted"} - if !reflect.DeepEqual(got, want) || f.tenant != tenant || f.vault != f.credential.VaultID || f.id != f.credential.ID || f.calls != 1 { - t.Fatal("deletion changed authenticated scope or confirmation") + // Unknown query keys, including a tenant hint, are ignored. + for _, query := range []string{"", "?tenant_id=untrusted&unknown=1"} { + h, f, tenant := credentialHandler(t) + w := credentialRequest(h, "DELETE", "/v1/vaults/"+f.credential.VaultID+"/credentials/"+f.credential.ID+query, "") + var got map[string]any + if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &got) != nil { + t.Fatal("deletion failed", w.Code) + } + want := map[string]any{"id": f.credential.ID, "deleted": true, "object": "vault.credential.deleted"} + if !reflect.DeepEqual(got, want) || f.tenant != tenant || f.vault != f.credential.VaultID || f.id != f.credential.ID || f.calls != 1 { + t.Fatal("deletion changed authenticated scope or confirmation") + } } } func TestCredentialDeletionRejectsBeforeMutation(t *testing.T) { - for _, mode := range []string{"auth", "beta", "query", "body", "null", "vault", "credential", "zero"} { + for _, mode := range []string{"auth", "beta", "body", "null", "vault", "credential", "zero", "zero-query"} { t.Run(mode, func(t *testing.T) { h, f, _ := credentialHandler(t) path := "/v1/vaults/" + f.credential.VaultID + "/credentials/" + f.credential.ID @@ -41,8 +44,6 @@ func TestCredentialDeletionRejectsBeforeMutation(t *testing.T) { switch mode { case "auth": status = http.StatusUnauthorized - case "query": - path += "?tenant_id=untrusted" case "body": body = `{"token":"credential-canary"}` case "null": @@ -53,6 +54,8 @@ func TestCredentialDeletionRejectsBeforeMutation(t *testing.T) { path, status = strings.Replace(path, f.credential.ID, "invalid", 1), http.StatusNotFound case "zero": path, status = strings.Replace(path, f.credential.ID, uuid.Nil.String(), 1), http.StatusNotFound + case "zero-query": + path, status = strings.Replace(path, f.credential.ID, uuid.Nil.String(), 1)+"?tenant_id=untrusted", http.StatusNotFound } r := httptest.NewRequest("DELETE", path, strings.NewReader(body)) if mode != "auth" { diff --git a/services/agents-api/internal/api/credentials_list.go b/services/agents-api/internal/api/credentials_list.go index a324d5be9..6183e9f8f 100644 --- a/services/agents-api/internal/api/credentials_list.go +++ b/services/agents-api/internal/api/credentials_list.go @@ -7,7 +7,7 @@ import ( ) // @Summary List safe Vault Credential metadata -// @Description Lists only metadata from the authenticated project's requested Vault, without decryption or execution. Includes active and archived Credentials by default, independently of Vault status. Status accepts a scalar or SDK status[] array; mixed encodings and repeated scalars are rejected locally. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering. Hosted errors, concurrent-page behavior and archive/delete lifecycle remain unverified or unimplemented. +// @Description Lists only metadata from the authenticated project's requested Vault, without decryption or execution. Includes active and archived Credentials by default, independently of Vault status. Status accepts a scalar, the SDK status[] array or both, filtering by their union; a repeated scalar is rejected. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering. Hosted errors, concurrent-page behavior and archive/delete lifecycle remain unverified or unimplemented. // @Tags Credentials // @Produce json // @Security BearerAuth @@ -17,7 +17,7 @@ import ( // @Param limit query integer false "Requested page size, clamped to 1–100" default(20) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Param status query string false "Scalar status filter" Enums(active,archived) -// @Param status[] query []string false "Array status filter; cannot be combined with status" collectionFormat(multi) Enums(active,archived) +// @Param status[] query []string false "Array status filter; combined with status as a union" collectionFormat(multi) Enums(active,archived) // @Success 200 {object} v1.CredentialList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /vaults/{vault_id}/credentials [get] diff --git a/services/agents-api/internal/api/credentials_list_test.go b/services/agents-api/internal/api/credentials_list_test.go index 4a8ecb42c..e86201e85 100644 --- a/services/agents-api/internal/api/credentials_list_test.go +++ b/services/agents-api/internal/api/credentials_list_test.go @@ -25,6 +25,8 @@ func TestCredentialListScopeProjectionAndParameters(t *testing.T) { {"", 20, nil}, {"?limit=0", 1, nil}, {"?limit=101", 100, nil}, {"?status=archived", 20, []string{"archived"}}, {"?status[]=active&status[]=archived", 20, []string{"active", "archived"}}, + {"?status=active&status[]=archived", 20, []string{"active", "archived"}}, + {"?limit=-3&tenant_id=foreign&unknown=1", 1, nil}, } { h, f, tenant := credentialHandler(t) f.page = store.CredentialPage{Credentials: []store.Credential{f.credential}, NextCursor: f.credential.ID} @@ -54,7 +56,7 @@ func TestCredentialListScopeProjectionAndParameters(t *testing.T) { } func TestCredentialListRejectsInvalidInputBeforeStorage(t *testing.T) { - for _, suffix := range []string{"?status=deleted", "?status=active&status[]=archived", "?limit=1.5", "?limit=1&limit=2", "?tenant_id=foreign"} { + for _, suffix := range []string{"?status=deleted", "?status=active&status=archived", "?status=active&status[]=deleted", "?limit=1.5", "?limit=1&limit=2"} { h, f, _ := credentialHandler(t) w := credentialRequest(h, "GET", "/v1/vaults/"+f.credential.VaultID+"/credentials"+suffix, "") if w.Code != 400 || f.calls != 0 { diff --git a/services/agents-api/internal/api/credentials_update.go b/services/agents-api/internal/api/credentials_update.go index dd20aa2c5..6ae3fee19 100644 --- a/services/agents-api/internal/api/credentials_update.go +++ b/services/agents-api/internal/api/credentials_update.go @@ -22,10 +22,6 @@ import ( // @Failure 400,401,404,413,500,503 {object} v1.ErrorResponse // @Router /vaults/{vault_id}/credentials/{credential_id} [post] func (h *Handler) updateCredential(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Credential update does not accept query parameters.") - return - } vaultID, ok := credentialResourceID(w, r, "vault_id") if !ok { return diff --git a/services/agents-api/internal/api/credentials_update_test.go b/services/agents-api/internal/api/credentials_update_test.go index cf537d3c3..1d670d9f3 100644 --- a/services/agents-api/internal/api/credentials_update_test.go +++ b/services/agents-api/internal/api/credentials_update_test.go @@ -24,7 +24,12 @@ func TestCredentialUpdatePreservesOpaqueInputAndSafeProjection(t *testing.T) { h, f, tenant := credentialHandler(t) f.credential.Name, f.credential.MCPServerURL = "Retained name", "https://example.invalid/mcp?q=x" body, _ := json.Marshal(map[string]any{"auth": map[string]string{"type": "static_bearer", "token": token}}) - w := credentialRequest(h, "POST", "/v1/vaults/"+f.credential.VaultID+"/credentials/"+f.credential.ID, string(body)) + query := "" + if token == "credential-canary" { + // Unknown query keys are ignored and never select another tenant. + query = "?tenant_id=untrusted&unknown=1" + } + w := credentialRequest(h, "POST", "/v1/vaults/"+f.credential.VaultID+"/credentials/"+f.credential.ID+query, string(body)) var got map[string]any if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &got) != nil { t.Fatal("update failed", w.Code) @@ -60,15 +65,15 @@ func TestCredentialUpdateRejectsInvalidBodiesBeforeStorage(t *testing.T) { func TestCredentialUpdateUsesExistingBoundariesAndSafeErrors(t *testing.T) { const body = `{"auth":{"type":"static_bearer","token":"credential-canary"}}` - for _, mode := range []string{"missing auth", "missing beta", "method", "query", "invalid Vault", "invalid Credential", "zero ID"} { + for _, mode := range []string{"missing auth", "missing beta", "method", "invalid Vault", "invalid Credential", "zero ID", "zero ID with query"} { h, f, _ := credentialHandler(t) path := "/v1/vaults/" + f.credential.VaultID + "/credentials/" + f.credential.ID method, status := "POST", http.StatusBadRequest switch mode { case "method": method, status = "PATCH", http.StatusMethodNotAllowed - case "query": - path += "?unknown=1" + case "zero ID with query": + path, status = strings.Replace(path, f.credential.ID, uuid.Nil.String(), 1)+"?unknown=1", http.StatusNotFound case "invalid Vault": path, status = strings.Replace(path, f.credential.VaultID, "invalid", 1), http.StatusNotFound case "invalid Credential": diff --git a/services/agents-api/internal/api/environment_files_create.go b/services/agents-api/internal/api/environment_files_create.go index 2dd7999f2..ca00e6743 100644 --- a/services/agents-api/internal/api/environment_files_create.go +++ b/services/agents-api/internal/api/environment_files_create.go @@ -42,10 +42,6 @@ func (h *Handler) createEnvironmentFile(w http.ResponseWriter, r *http.Request) writeStoreError(w, r, err) return } - if r.URL.RawQuery != "" { - writeStoreError(w, r, store.ErrInvalidInput) - return - } const maxJSON = int64(((proto.WorkspaceWriteMaxBytes+2)/3)*4 + (16 << 10)) raw, ok := readJSONBodyLimit(w, r, maxJSON, "Inline upload exceeds this service's bounded file limit.") if !ok { @@ -94,7 +90,7 @@ func (h *Handler) createEnvironmentFile(w http.ResponseWriter, r *http.Request) return } } else { - if !h.sourceFilesReady(w, r) { + if !h.sourceFilesAvailable(w) { return } ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second) diff --git a/services/agents-api/internal/api/environment_files_query.go b/services/agents-api/internal/api/environment_files_query.go index d7050b4d9..3bf7e51d1 100644 --- a/services/agents-api/internal/api/environment_files_query.go +++ b/services/agents-api/internal/api/environment_files_query.go @@ -40,7 +40,7 @@ func readEnvironmentFileQuery(w http.ResponseWriter, r *http.Request, environmen pageQuery[key] = values } } - page, ok := readPageQuery(w, r, pageQuery, true) + page, ok := readPageQuery(w, r, pageQuery, false) if !ok { return options, false } diff --git a/services/agents-api/internal/api/environment_templates.go b/services/agents-api/internal/api/environment_templates.go index 832bd49be..7ad261b3a 100644 --- a/services/agents-api/internal/api/environment_templates.go +++ b/services/agents-api/internal/api/environment_templates.go @@ -69,18 +69,7 @@ func templateResponse(t store.EnvironmentTemplate) v1.EnvironmentTemplate { return v1.EnvironmentTemplate{ID: t.ID, Object: "agent.environment.template", Name: t.Name, CreatedAt: t.CreatedAt.Unix(), UpdatedAt: t.UpdatedAt.Unix(), CapabilityDirectories: append([]string{}, t.CapabilityDirectories...), Network: v1.EnvironmentNetwork{Access: t.NetworkAccess, AllowedDomains: append([]string{}, t.AllowedDomains...)}, Packages: packageMetadata(&t.Packages), Files: templateFileResponse(t.Files), Plugins: pluginResponse(t.Plugins), Skills: skillResponse(t.Skills)} } -func templateNoQuery(w http.ResponseWriter, r *http.Request) bool { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "This template operation does not accept query parameters.") - return false - } - return true -} - func readTemplateInput(w http.ResponseWriter, r *http.Request) (store.EnvironmentTemplateInput, bool) { - if !templateNoQuery(w, r) { - return store.EnvironmentTemplateInput{}, false - } raw, ok := readJSONBodyLimit(w, r, 16*1024*1024, "Request exceeds 16 MiB.") if !ok { return store.EnvironmentTemplateInput{}, false @@ -128,9 +117,6 @@ func (h *Handler) createEnvironmentTemplate(w http.ResponseWriter, r *http.Reque // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/environments/templates/{environment_template_id} [get] func (h *Handler) getEnvironmentTemplate(w http.ResponseWriter, r *http.Request) { - if !templateNoQuery(w, r) { - return - } value, err := h.store.GetEnvironmentTemplate(r.Context(), tenantID(r), chi.URLParam(r, "environment_template_id")) if err != nil { writeStoreError(w, r, err) @@ -175,9 +161,6 @@ func (h *Handler) updateEnvironmentTemplate(w http.ResponseWriter, r *http.Reque // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/environments/templates/{environment_template_id} [delete] func (h *Handler) deleteEnvironmentTemplate(w http.ResponseWriter, r *http.Request) { - if !templateNoQuery(w, r) { - return - } id, err := h.store.DeleteEnvironmentTemplate(r.Context(), tenantID(r), chi.URLParam(r, "environment_template_id")) if err != nil { writeStoreError(w, r, err) @@ -187,19 +170,19 @@ func (h *Handler) deleteEnvironmentTemplate(w http.ResponseWriter, r *http.Reque } // @Summary List Environment Templates -// @Description Lists tenant-owned safe template metadata in creation order with ID tie-breaking. Defaults to limit 20 and descending order; limit must be 1–100. Foreign and missing cursors reject identically. Concurrent-page and exact hosted error behavior remain unverified. +// @Description Lists tenant-owned safe template metadata in creation order with ID tie-breaking. Defaults to limit 20 and descending order; limit 0 is treated as 1 and larger limits as 100. Foreign and missing cursors reject identically. Concurrent-page and exact hosted error behavior remain unverified. // @Tags Environment Templates // @Produce json // @Security BearerAuth // @Param OpenAI-Beta header string true "agents=v1" // @Param after query string false "Previous Template ID" -// @Param limit query integer false "Page size" default(20) minimum(1) maximum(100) +// @Param limit query integer false "Page size; 0 is treated as 1 and values above 100 as 100" default(20) minimum(0) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Success 200 {object} v1.EnvironmentTemplateList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/environments/templates [get] func (h *Handler) listEnvironmentTemplates(w http.ResponseWriter, r *http.Request) { - options, ok := readPage(w, r) + options, ok := readClampedPage(w, r) if !ok { return } diff --git a/services/agents-api/internal/api/environments.go b/services/agents-api/internal/api/environments.go index 86f386f1f..475043b83 100644 --- a/services/agents-api/internal/api/environments.go +++ b/services/agents-api/internal/api/environments.go @@ -21,10 +21,6 @@ import ( // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/environments/{environment_id} [get] func (h *Handler) getEnvironment(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Environment retrieval does not accept query parameters.") - return - } environment, err := h.store.GetEnvironment(r.Context(), tenantID(r), chi.URLParam(r, "environment_id")) if err != nil { writeStoreError(w, r, err) diff --git a/services/agents-api/internal/api/environments_test.go b/services/agents-api/internal/api/environments_test.go index 9ee62301f..94df3e386 100644 --- a/services/agents-api/internal/api/environments_test.go +++ b/services/agents-api/internal/api/environments_test.go @@ -129,7 +129,9 @@ func TestEnvironmentResourceRequestAndStoreErrors(t *testing.T) { {"wrong auth", "GET", "", "Bearer unknown", "agents=v1", nil, 401, 0}, {"no beta", "GET", "", "Bearer resource-key", "", nil, 400, 0}, {"wrong beta", "GET", "", "Bearer resource-key", "agents=v2", nil, 400, 0}, - {"query", "GET", "?tenant_id=foreign", "Bearer resource-key", "agents=v1", nil, 400, 0}, + // Unknown keys, including include, are not pinned retrieval parameters. + {"ignored query", "GET", "?tenant_id=foreign&include=files", "Bearer resource-key", "agents=v1", nil, 200, 1}, + {"not found with query", "GET", "?include=files", "Bearer resource-key", "agents=v1", store.ErrNotFound, 404, 1}, {"method", "POST", "", "Bearer resource-key", "agents=v1", nil, 405, 0}, {"not found", "GET", "", "Bearer resource-key", "agents=v1", store.ErrNotFound, 404, 1}, {"invalid id", "GET", "", "Bearer resource-key", "agents=v1", store.ErrInvalidInput, 400, 1}, @@ -146,6 +148,9 @@ func TestEnvironmentResourceRequestAndStoreErrors(t *testing.T) { if w.Code != test.status || f.calls != test.calls || strings.Contains(w.Body.String(), "private-backend-canary") { t.Fatal("request or shared error handling changed", w.Code, w.Body, f.calls) } + if test.calls > 0 && f.tenant != f.environment.TenantID { + t.Fatal("query changed the authenticated tenant", f.tenant) + } }) } } diff --git a/services/agents-api/internal/api/handler.go b/services/agents-api/internal/api/handler.go index 6952bd632..b0e2302e1 100644 --- a/services/agents-api/internal/api/handler.go +++ b/services/agents-api/internal/api/handler.go @@ -136,10 +136,6 @@ func NewHandler(s ResourceStore, auth *Authenticator, engine string, options ... // @Failure 400,401,404,409,413,500,503 {object} v1.ErrorResponse // @Router /agents/sessions [post] func (h *Handler) createSession(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Session creation does not accept query parameters.") - return - } var request decodedSessionRequest decoder := json.NewDecoder(http.MaxBytesReader(w, r.Body, 16*1024*1024)) decoder.DisallowUnknownFields() @@ -288,10 +284,6 @@ func (h *Handler) createSession(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id} [get] func (h *Handler) getSession(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Session retrieval does not accept query parameters.") - return - } session, err := h.store.GetSession(r.Context(), tenantID(r), chi.URLParam(r, "session_id")) if err != nil { writeStoreError(w, r, err) @@ -321,13 +313,13 @@ func (h *Handler) respondSessionStatus(w http.ResponseWriter, r *http.Request, s // @Param OpenAI-Beta header string true "agents=v1" // @Param agent_id query string false "Root Agent ID whose Sessions to return" // @Param after query string false "Last Session ID from the previous page" -// @Param limit query int false "Page size" minimum(1) maximum(100) default(20) +// @Param limit query int false "Page size; 0 is treated as 1 and values above 100 as 100" minimum(0) default(20) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Success 200 {object} v1.SessionList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/sessions [get] func (h *Handler) listSessions(w http.ResponseWriter, r *http.Request) { - options, ok := readPage(w, r, "agent_id") + options, ok := readClampedPage(w, r, "agent_id") if !ok { return } diff --git a/services/agents-api/internal/api/handler_test.go b/services/agents-api/internal/api/handler_test.go index 0792b1512..e796ae05e 100644 --- a/services/agents-api/internal/api/handler_test.go +++ b/services/agents-api/internal/api/handler_test.go @@ -59,7 +59,8 @@ func TestHTTPConfigurationAndTenantIdentity(t *testing.T) { s := &recordingStore{} h, _, tenant := testHandler(t, WithExecution(&inputRecorder{ResourceStore: s})) body := `{"agent":{"model":"requested-model","instructions":"Keep this."},"environment":{"type":"none"},"metadata":{"tenant_id":"untrusted-tenant"},"input":"Follow the configured instructions."}` - request := httptest.NewRequest(http.MethodPost, "/v1/agents/sessions", strings.NewReader(body)) + // Unknown query keys are ignored and never select the tenant. + request := httptest.NewRequest(http.MethodPost, "/v1/agents/sessions?tenant_id=untrusted-tenant", strings.NewReader(body)) request.Header.Set("Authorization", "Bearer test-api-key") request.Header.Set("OpenAI-Beta", "agents=v1") request.Header.Set("Idempotency-Key", "retry-key") @@ -87,7 +88,6 @@ func TestHTTPRejectsUntrustedOrUnsupportedRequests(t *testing.T) { {"missing auth", "", "agents=v1", "/v1/agents/sessions", valid, 401}, {"invalid auth", "Bearer wrong", "agents=v1", "/v1/agents/sessions", valid, 401}, {"missing beta", "Bearer test-api-key", "", "/v1/agents/sessions", valid, 400}, - {"tenant query", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions?tenant_id=other", valid, 400}, {"tenant body", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"agent":`, `"tenant_id":"other","agent":`, 1), 400}, {"hosted environment without managed deployment", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"none"`, `"openai_hosted"`, 1), 503}, {"self-hosted environment", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"none"`, `"self_hosted"`, 1), 400}, diff --git a/services/agents-api/internal/api/inputs.go b/services/agents-api/internal/api/inputs.go index f54c776ab..8cd708fb8 100644 --- a/services/agents-api/internal/api/inputs.go +++ b/services/agents-api/internal/api/inputs.go @@ -36,10 +36,6 @@ func WithExecution(s InputSubmitter) Option { return func(h *Handler) { h.inputs // @Failure 400,401,404,409,413,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/events [post] func (h *Handler) createEvents(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) != 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Event submission does not accept query parameters.") - return - } var request struct { Events []json.RawMessage `json:"events"` } diff --git a/services/agents-api/internal/api/items.go b/services/agents-api/internal/api/items.go index 6b0910e9b..4c599ebd2 100644 --- a/services/agents-api/internal/api/items.go +++ b/services/agents-api/internal/api/items.go @@ -13,13 +13,13 @@ import ( // @Param OpenAI-Beta header string true "agents=v1" // @Param session_id path string true "Session ID" // @Param after query string false "Last Item ID from the previous page" -// @Param limit query int false "Page size" minimum(1) maximum(100) default(20) +// @Param limit query int false "Page size; 0 is treated as 1 and values above 100 as 100" minimum(0) default(20) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Success 200 {object} v1.ItemList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/items [get] func (h *Handler) listItems(w http.ResponseWriter, r *http.Request) { - options, ok := readPage(w, r) + options, ok := readClampedPage(w, r) if !ok { return } diff --git a/services/agents-api/internal/api/items_test.go b/services/agents-api/internal/api/items_test.go index f77a650ab..99caf60cf 100644 --- a/services/agents-api/internal/api/items_test.go +++ b/services/agents-api/internal/api/items_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "testing" v1 "github.com/MiniMax-AI-Dev/parsar/contracts/agents-api/v1" @@ -40,9 +41,14 @@ func TestItemRouteUsesAuthenticationAndSharedPagination(t *testing.T) { if w := request("", "test-api-key"); w.Code != 200 || s.limit != 20 || s.ascending || w.Body.String() != "{\"object\":\"list\",\"first_id\":null,\"last_id\":null,\"data\":[],\"has_more\":false}\n" { t.Fatal(w.Code, w.Body, s) } - for _, q := range []string{"?limit=0", "?limit=101", "?order=bad", "?limit=2&limit=3", "?tenant_id=other"} { - if w := request(q, "test-api-key"); w.Code != 400 { - t.Fatal(q, w.Code) + for q, limit := range map[string]int{"?limit=0": 1, "?limit=101": 100, "?limit=1000&tenant_id=other&unknown=1": 100} { + if w := request(q, "test-api-key"); w.Code != 200 || s.limit != limit || s.tenant != tenant { + t.Fatal(q, w.Code, s.limit, s.tenant) + } + } + for _, q := range []string{"?limit=-1", "?limit=abc", "?order=bad", "?limit=2&limit=3"} { + if w := request(q, "test-api-key"); w.Code != 400 || !strings.Contains(w.Body.String(), `"code":"invalid_request_error"`) { + t.Fatal(q, w.Code, w.Body) } } if w := request("", "invalid"); w.Code != 401 { diff --git a/services/agents-api/internal/api/list_query_errors.go b/services/agents-api/internal/api/list_query_errors.go new file mode 100644 index 000000000..84067567b --- /dev/null +++ b/services/agents-api/internal/api/list_query_errors.go @@ -0,0 +1,99 @@ +package api + +import ( + "errors" + "fmt" + "net/http" + "strconv" + "strings" +) + +// listFamily selects the list-query bounds and error fields observed for a +// group of official collections. Agents API Beta lists are the default. +type listFamily int + +const ( + betaList listFamily = iota + filesList + skillsList +) + +func listFamilyOf(r *http.Request) listFamily { + switch { + case r.URL.Path == "/v1/files" || strings.HasPrefix(r.URL.Path, "/v1/files/"): + return filesList + case r.URL.Path == "/v1/skills" || strings.HasPrefix(r.URL.Path, "/v1/skills/"): + return skillsList + default: + return betaList + } +} + +func writeListOrderError(w http.ResponseWriter, r *http.Request, order string) { + switch listFamilyOf(r) { + case filesList: + writeError(w, http.StatusBadRequest, "", "order must be asc or desc.") + case skillsList: + writeError(w, http.StatusBadRequest, "invalid_value", fmt.Sprintf("Invalid value: '%s'. Supported values are: 'asc' and 'desc'.", order), "order") + default: + writeError(w, http.StatusBadRequest, "invalid_request_error", fmt.Sprintf("Failed to deserialize query string: order: unknown variant `%s`, expected `asc` or `desc`", order)) + } +} + +func writeListDuplicateError(w http.ResponseWriter, r *http.Request, key string, keys []string) { + switch listFamilyOf(r) { + case filesList: + // Only an identical repeated purpose was sampled; Files keep the local rejection. + writeError(w, http.StatusBadRequest, "unsupported_parameter", "Supported list parameters are "+strings.Join(keys[:len(keys)-1], ", ")+" and "+keys[len(keys)-1]+", each supplied once.") + case skillsList: + writeError(w, http.StatusBadRequest, "duplicate_parameter", fmt.Sprintf("Duplicate parameter: '%[1]s'. You provided multiple values for this parameter, whereas only one is allowed. If you are trying to provide a list of values, use the array syntax instead e.g. '%[1]s[]='.", key), key) + default: + writeError(w, http.StatusBadRequest, "invalid_request_error", fmt.Sprintf("Failed to deserialize query string: duplicate field `%s`", key)) + } +} + +// readListLimit applies a family's limit bounds. For Beta lists, clamp maps 0 +// and values above maxLimit into range instead of rejecting them. +func readListLimit(w http.ResponseWriter, r *http.Request, raw string, maxLimit int, clamp bool) (int, bool) { + requested, err := strconv.ParseInt(raw, 10, 64) + switch listFamilyOf(r) { + case filesList: + message := fmt.Sprintf("limit must be between 1 and %d.", maxLimit) + switch { + case err != nil: + writeError(w, http.StatusBadRequest, "invalid_request", message) + case requested < 1 || requested > int64(maxLimit): + // Hosted range errors have a null code; their schema text and detail member are not copied. + writeError(w, http.StatusBadRequest, "", message) + default: + return int(requested), true + } + case skillsList: + switch { + case err != nil: + writeError(w, http.StatusBadRequest, "invalid_request", fmt.Sprintf("limit must be an integer between 0 and %d.", maxLimit)) + case requested < 0: + writeError(w, http.StatusBadRequest, "integer_below_min_value", fmt.Sprintf("Invalid 'limit': integer below minimum value. Expected a value >= 0, but got %d instead.", requested), "limit") + case requested > int64(maxLimit): + writeError(w, http.StatusBadRequest, "integer_above_max_value", fmt.Sprintf("Invalid 'limit': integer above maximum value. Expected a value <= %d, but got %d instead.", maxLimit, requested), "limit") + default: + // Zero returns an empty page whose has_more reports remaining resources. + return int(requested), true + } + default: + // Beta lists deserialize an unsigned integer, so a sign is an invalid digit. + switch { + case strings.HasPrefix(raw, "-") || (err != nil && !errors.Is(err, strconv.ErrRange)): + writeError(w, http.StatusBadRequest, "invalid_request_error", "Failed to deserialize query string: limit: invalid digit found in string") + case err != nil: + writeError(w, http.StatusBadRequest, "invalid_request_error", "Failed to deserialize query string: limit: number too large to fit in target type") + case clamp: + return int(max(1, min(requested, int64(maxLimit)))), true + case requested < 1 || requested > int64(maxLimit): + writeError(w, http.StatusBadRequest, "invalid_request_error", fmt.Sprintf("limit must be between 1 and %d", maxLimit)) + default: + return int(requested), true + } + } + return 0, false +} diff --git a/services/agents-api/internal/api/pagination.go b/services/agents-api/internal/api/pagination.go index a4df0317c..112f737bc 100644 --- a/services/agents-api/internal/api/pagination.go +++ b/services/agents-api/internal/api/pagination.go @@ -1,11 +1,8 @@ package api import ( - "fmt" "net/http" "net/url" - "slices" - "strconv" "strings" ) @@ -15,40 +12,36 @@ type pageOptions struct { ascending bool } +// readPage rejects limits outside 1–100. func readPage(w http.ResponseWriter, r *http.Request, extraKeys ...string) (pageOptions, bool) { - return readPageSize(w, r, true, extraKeys...) + return readPageQuery(w, r, r.URL.Query(), false, extraKeys...) } -func readPageSize(w http.ResponseWriter, r *http.Request, rejectLarger bool, extraKeys ...string) (pageOptions, bool) { - return readPageQuery(w, r, r.URL.Query(), rejectLarger, extraKeys...) +// readClampedPage clamps limit 0 to 1 and larger limits to 100. +func readClampedPage(w http.ResponseWriter, r *http.Request, extraKeys ...string) (pageOptions, bool) { + return readPageQuery(w, r, r.URL.Query(), true, extraKeys...) } -func readPageQuery(w http.ResponseWriter, r *http.Request, q url.Values, rejectLarger bool, extraKeys ...string) (pageOptions, bool) { - return readPageQueryLimits(w, r, q, 20, 100, rejectLarger, extraKeys...) +func readPageQuery(w http.ResponseWriter, r *http.Request, q url.Values, clamp bool, extraKeys ...string) (pageOptions, bool) { + return readPageQueryLimits(w, r, q, 20, 100, clamp, extraKeys...) } -func readPageQueryLimits(w http.ResponseWriter, r *http.Request, q url.Values, defaultLimit, maxLimit int, rejectLarger bool, extraKeys ...string) (pageOptions, bool) { +// readPageQueryLimits ignores unknown keys, as the official lists do, but each +// supported key may appear only once. +func readPageQueryLimits(w http.ResponseWriter, r *http.Request, q url.Values, defaultLimit, maxLimit int, clamp bool, extraKeys ...string) (pageOptions, bool) { keys := append([]string{"after", "limit", "order"}, extraKeys...) - for key, values := range q { - if !slices.Contains(keys, key) || len(values) != 1 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Supported list parameters are "+strings.Join(keys[:len(keys)-1], ", ")+" and "+keys[len(keys)-1]+", each supplied once.") + for _, key := range keys { + if len(q[key]) > 1 { + writeListDuplicateError(w, r, key, keys) return pageOptions{}, false } } limit, order := defaultLimit, q.Get("order") - if raw, ok := q["limit"]; ok { - var err error - var requested int64 - requested, err = strconv.ParseInt(raw[0], 10, 64) - if err != nil || requested < 1 || (rejectLarger && requested > int64(maxLimit)) { - message := "limit must be a positive 64-bit integer." - if rejectLarger { - message = fmt.Sprintf("limit must be between 1 and %d.", maxLimit) - } - writeError(w, http.StatusBadRequest, "invalid_request", message) + if raw, supplied := q["limit"]; supplied { + var valid bool + if limit, valid = readListLimit(w, r, raw[0], maxLimit, clamp); !valid { return pageOptions{}, false } - limit = int(min(requested, int64(maxLimit))) } if _, supplied := q["order"]; supplied && order != "asc" && order != "desc" { writeListOrderError(w, r, order) @@ -56,14 +49,3 @@ func readPageQueryLimits(w http.ResponseWriter, r *http.Request, q url.Values, d } return pageOptions{after: strings.TrimSpace(q.Get("after")), limit: limit, ascending: order == "asc"}, true } - -func writeListOrderError(w http.ResponseWriter, r *http.Request, order string) { - switch { - case r.URL.Path == "/v1/files" || strings.HasPrefix(r.URL.Path, "/v1/files/"): - writeError(w, http.StatusBadRequest, "", "order must be asc or desc.") - case r.URL.Path == "/v1/skills" || strings.HasPrefix(r.URL.Path, "/v1/skills/"): - writeError(w, http.StatusBadRequest, "invalid_value", fmt.Sprintf("Invalid value: '%s'. Supported values are: 'asc' and 'desc'.", order), "order") - default: - writeError(w, http.StatusBadRequest, "invalid_request_error", fmt.Sprintf("Failed to deserialize query string: order: unknown variant `%s`, expected `asc` or `desc`", order)) - } -} diff --git a/services/agents-api/internal/api/pagination_test.go b/services/agents-api/internal/api/pagination_test.go index e3e135ae0..5e51b0c7b 100644 --- a/services/agents-api/internal/api/pagination_test.go +++ b/services/agents-api/internal/api/pagination_test.go @@ -10,33 +10,138 @@ import ( "testing" ) -func TestPageSizePolicy(t *testing.T) { +const ( + invalidDigit = "Failed to deserialize query string: limit: invalid digit found in string" + tooLarge = "Failed to deserialize query string: limit: number too large to fit in target type" + betaRange = "limit must be between 1 and 100" +) + +func TestPageLimitPolicies(t *testing.T) { + type accepted struct { + query string + limit int + } + type rejected struct { + query, message string + code, param any + } for _, test := range []struct { - query string - cappedOK, strictOK bool - limit int + name string + path string + read func(http.ResponseWriter, *http.Request, ...string) (pageOptions, bool) + accepted []accepted + rejected []rejected }{ - {"", true, true, 20}, {"limit=1", true, true, 1}, {"limit=100", true, true, 100}, - {"limit=101", true, false, 100}, {"limit=9223372036854775807", true, false, 100}, - {"limit=9223372036854775808", false, false, 0}, {"limit=0", false, false, 0}, - {"limit=-1", false, false, 0}, {"limit=1.5", false, false, 0}, {"limit=", false, false, 0}, - {"limit=null", false, false, 0}, {"limit=2&limit=3", false, false, 0}, {"order=invalid", false, false, 0}, - {"order=", false, false, 0}, {"tenant_id=other", false, false, 0}, + { + // Agents, Sessions, Items and Templates clamp 0 and values above 100. + "beta clamped", "/v1/agents", readClampedPage, + []accepted{{"", 20}, {"limit=0", 1}, {"limit=1", 1}, {"limit=%2B5", 5}, {"limit=100", 100}, {"limit=101", 100}, {"limit=1000", 100}, {"limit=9223372036854775807", 100}}, + []rejected{ + {"limit=-1", invalidDigit, "invalid_request_error", nil}, {"limit=-0", invalidDigit, "invalid_request_error", nil}, + {"limit=abc", invalidDigit, "invalid_request_error", nil}, {"limit=1.5", invalidDigit, "invalid_request_error", nil}, + {"limit=", invalidDigit, "invalid_request_error", nil}, {"limit=null", invalidDigit, "invalid_request_error", nil}, + {"limit=-99999999999999999999", invalidDigit, "invalid_request_error", nil}, + {"limit=9223372036854775808", tooLarge, "invalid_request_error", nil}, + }, + }, + { + // Turns, Subagent and Artifact lists keep rejecting values outside 1–100. + "beta strict", "/v1/agents/sessions/session/turns", readPage, + []accepted{{"", 20}, {"limit=1", 1}, {"limit=100", 100}}, + []rejected{ + {"limit=0", betaRange, "invalid_request_error", nil}, {"limit=101", betaRange, "invalid_request_error", nil}, + {"limit=1000", betaRange, "invalid_request_error", nil}, {"limit=-1", invalidDigit, "invalid_request_error", nil}, + {"limit=abc", invalidDigit, "invalid_request_error", nil}, {"limit=99999999999999999999", tooLarge, "invalid_request_error", nil}, + }, + }, + { + "skills", "/v1/skills", readPage, + []accepted{{"", 20}, {"limit=0", 0}, {"limit=1", 1}, {"limit=100", 100}}, + []rejected{ + {"limit=101", "Invalid 'limit': integer above maximum value. Expected a value <= 100, but got 101 instead.", "integer_above_max_value", "limit"}, + {"limit=-1", "Invalid 'limit': integer below minimum value. Expected a value >= 0, but got -1 instead.", "integer_below_min_value", "limit"}, + {"limit=abc", "limit must be an integer between 0 and 100.", "invalid_request", nil}, + {"limit=", "limit must be an integer between 0 and 100.", "invalid_request", nil}, + }, + }, + { + "skill versions", "/v1/skills/skill-example/versions", readPage, + []accepted{{"limit=0", 0}, {"limit=100", 100}}, + []rejected{ + {"limit=1000", "Invalid 'limit': integer above maximum value. Expected a value <= 100, but got 1000 instead.", "integer_above_max_value", "limit"}, + {"limit=-7", "Invalid 'limit': integer below minimum value. Expected a value >= 0, but got -7 instead.", "integer_below_min_value", "limit"}, + }, + }, } { - t.Run(test.query, func(t *testing.T) { - for _, strict := range []bool{true, false} { + for _, want := range test.accepted { + t.Run(test.name+"/"+want.query, func(t *testing.T) { w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/v1/agents?"+test.query, nil) - page, ok := readPageSize(w, r, strict) - want := test.cappedOK - if strict { - want = test.strictOK + page, ok := test.read(w, httptest.NewRequest(http.MethodGet, test.path+"?"+want.query, nil)) + if !ok || page.limit != want.limit || w.Body.Len() != 0 { + t.Fatalf("page=%+v ok=%t response=%s", page, ok, w.Body.String()) } - if ok != want || (ok && page.limit != test.limit) || (!ok && w.Code != 400) { - t.Fatalf("strict=%t page=%+v ok=%t status=%d", strict, page, ok, w.Code) + }) + } + for _, want := range test.rejected { + t.Run(test.name+"/"+want.query, func(t *testing.T) { + w := httptest.NewRecorder() + if _, ok := test.read(w, httptest.NewRequest(http.MethodGet, test.path+"?"+want.query, nil)); ok { + t.Fatal("invalid limit accepted") } - } - }) + assertListQueryError(t, w, want.code, want.param, want.message) + }) + } + } +} + +func TestListQueryIgnoresUnknownKeys(t *testing.T) { + for _, path := range []string{"/v1/agents", "/v1/agents/sessions/session/turns", "/v1/files", "/v1/skills", "/v1/skills/skill-example/versions"} { + for _, unknown := range []string{"unknown=1", "tenant_id=other", "limit[]=500", "Limit=0", "order[]=sideways", "after[]=x&after[]=y", "unknown=1&unknown=2"} { + t.Run(path+"?"+unknown, func(t *testing.T) { + read := func(query string) (pageOptions, bool, string) { + w := httptest.NewRecorder() + page, ok := readPage(w, httptest.NewRequest(http.MethodGet, path+"?"+query, nil)) + return page, ok, w.Body.String() + } + want, wantOK, _ := read("after=cursor&limit=2&order=asc") + got, ok, body := read("after=cursor&limit=2&order=asc&" + unknown) + if !wantOK || !ok || got != want || body != "" { + t.Fatalf("unknown key changed the page: %+v %t %s", got, ok, body) + } + }) + } + } +} + +func TestListDuplicateKeyErrors(t *testing.T) { + skills := "Duplicate parameter: '%[1]s'. You provided multiple values for this parameter, whereas only one is allowed. If you are trying to provide a list of values, use the array syntax instead e.g. '%[1]s[]='." + for _, test := range []struct { + path, key string + extra []string + code, param any + message string + }{ + {"/v1/agents", "limit", nil, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `%s`"}, + {"/v1/agents", "order", nil, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `%s`"}, + {"/v1/agents/sessions", "after", []string{"agent_id"}, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `%s`"}, + {"/v1/agents/sessions", "agent_id", []string{"agent_id"}, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `%s`"}, + {"/v1/agents/sessions/session/artifacts", "environment_id", []string{"environment_id"}, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `%s`"}, + {"/v1/skills", "limit", nil, "duplicate_parameter", "limit", skills}, + {"/v1/skills/skill-example/versions", "order", nil, "duplicate_parameter", "order", skills}, + {"/v1/skills", "after", nil, "duplicate_parameter", "after", skills}, + } { + // Identical and different repeated values are both duplicates. + for _, values := range [][2]string{{"1", "1"}, {"1", "2"}} { + t.Run(test.path+"/"+test.key+"="+values[1], func(t *testing.T) { + w := httptest.NewRecorder() + query := url.Values{test.key: values[:], "unknown": {"ignored"}} + r := httptest.NewRequest(http.MethodGet, test.path+"?"+query.Encode(), nil) + if _, ok := readPage(w, r, test.extra...); ok { + t.Fatal("repeated key accepted") + } + assertListQueryError(t, w, test.code, test.param, fmt.Sprintf(test.message, test.key)) + }) + } } } @@ -87,30 +192,27 @@ func TestListOrderErrorEnvelopes(t *testing.T) { } } -func TestListOrderValidationPreservesOtherQueryErrors(t *testing.T) { - for _, test := range []struct{ query, code string }{ - {"order=&limit=0", "invalid_request"}, - {"order=asc&order=desc", "unsupported_parameter"}, - {"order=&tenant_id=other", "unsupported_parameter"}, +func TestListQueryErrorPrecedence(t *testing.T) { + skillsOrder := "Invalid value: ''. Supported values are: 'asc' and 'desc'." + for _, test := range []struct { + path, query string + code, param any + message string + }{ + // Duplicates precede value errors, and limit errors precede order errors. + {"/v1/skills", "order=&limit=101", "integer_above_max_value", "limit", "Invalid 'limit': integer above maximum value. Expected a value <= 100, but got 101 instead."}, + {"/v1/skills", "order=&limit=0", "invalid_value", "order", skillsOrder}, + {"/v1/skills", "order=&tenant_id=other", "invalid_value", "order", skillsOrder}, + {"/v1/skills", "limit=-1&order=asc&order=desc", "duplicate_parameter", "order", "Duplicate parameter: 'order'. You provided multiple values for this parameter, whereas only one is allowed. If you are trying to provide a list of values, use the array syntax instead e.g. 'order[]='."}, + {"/v1/agents", "order=&limit=-1", "invalid_request_error", nil, invalidDigit}, + {"/v1/agents", "limit=abc&order=&order=", "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `order`"}, } { - t.Run(test.query, func(t *testing.T) { + t.Run(test.path+"?"+test.query, func(t *testing.T) { w := httptest.NewRecorder() - r := httptest.NewRequest(http.MethodGet, "/v1/skills?"+test.query, nil) - if _, ok := readPage(w, r); ok { + if _, ok := readPage(w, httptest.NewRequest(http.MethodGet, test.path+"?"+test.query, nil)); ok { t.Fatal("invalid query accepted") } - var response struct { - Error struct { - Code string - Param *string - } - } - if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { - t.Fatal(err) - } - if w.Code != http.StatusBadRequest || response.Error.Code != test.code || response.Error.Param != nil { - t.Fatalf("other query error changed: %d %s", w.Code, w.Body.String()) - } + assertListQueryError(t, w, test.code, test.param, test.message) }) } } diff --git a/services/agents-api/internal/api/resource_query_test.go b/services/agents-api/internal/api/resource_query_test.go new file mode 100644 index 000000000..ba3274734 --- /dev/null +++ b/services/agents-api/internal/api/resource_query_test.go @@ -0,0 +1,367 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "mime/multipart" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + v1 "github.com/MiniMax-AI-Dev/parsar/contracts/agents-api/v1" + "github.com/MiniMax-AI-Dev/parsar/internal/agentdaemon/device" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/store" + "github.com/google/uuid" +) + +// missingResourceStore reports every resource as missing, recording the tenant +// each lookup used. +type missingResourceStore struct { + ResourceStore + tenants []string +} + +func (s *missingResourceStore) missing(tenant string) error { + s.tenants = append(s.tenants, tenant) + return store.ErrNotFound +} + +func (s *missingResourceStore) GetAgent(_ context.Context, tenant, _ string) (store.SavedAgent, error) { + return store.SavedAgent{}, s.missing(tenant) +} + +func (s *missingResourceStore) DeleteAgent(_ context.Context, tenant, _ string) (string, error) { + return "", s.missing(tenant) +} + +func (s *missingResourceStore) UpdateAgent(_ context.Context, tenant, _ string, _ store.UpdateAgentInput) (store.SavedAgent, error) { + return store.SavedAgent{}, s.missing(tenant) +} + +func (s *missingResourceStore) GetSession(_ context.Context, tenant, _ string) (store.Session, error) { + return store.Session{}, s.missing(tenant) +} + +func (s *missingResourceStore) DeleteSession(_ context.Context, tenant, _ string) error { + return s.missing(tenant) +} + +func (s *missingResourceStore) UpdateSessionMetadata(_ context.Context, tenant, _ string, _ map[string]string) (store.Session, error) { + return store.Session{}, s.missing(tenant) +} + +func (s *missingResourceStore) GetEnvironmentTemplate(_ context.Context, tenant, _ string) (store.EnvironmentTemplate, error) { + return store.EnvironmentTemplate{}, s.missing(tenant) +} + +func (s *missingResourceStore) UpdateEnvironmentTemplate(_ context.Context, tenant, _ string, _ store.EnvironmentTemplateInput) (store.EnvironmentTemplate, error) { + return store.EnvironmentTemplate{}, s.missing(tenant) +} + +func (s *missingResourceStore) DeleteEnvironmentTemplate(_ context.Context, tenant, _ string) (string, error) { + return "", s.missing(tenant) +} + +// twoTenantHandler authenticates "test-api-key" as the owner and "foreign-key" as +// another project. +func twoTenantHandler(t *testing.T, s ResourceStore, options ...Option) (http.Handler, string, string) { + t.Helper() + owner, foreign := uuid.NewString(), uuid.NewString() + auth, err := NewAuthenticator([]APIKey{ + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "owner", TokenSHA256: device.HashCredential("test-api-key"), TenantID: owner}, + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "foreign", TokenSHA256: device.HashCredential("foreign-key"), TenantID: foreign}, + }) + if err != nil { + t.Fatal(err) + } + h, err := NewHandler(s, auth, "codex", options...) + if err != nil { + t.Fatal(err) + } + return h, owner, foreign +} + +// Unknown query keys on single-resource routes are ignored: a missing or foreign +// resource keeps the same not-found response and authenticated tenant. +func TestSingleResourceRoutesIgnoreUnknownQueryKeys(t *testing.T) { + id := uuid.NewString() + for _, route := range []struct{ method, path, body string }{ + {http.MethodGet, "/v1/agents/" + id, ""}, + {http.MethodPost, "/v1/agents/" + id, `{"name":"Renamed"}`}, + {http.MethodDelete, "/v1/agents/" + id, ""}, + {http.MethodGet, "/v1/agents/sessions/" + id, ""}, + {http.MethodPost, "/v1/agents/sessions/" + id, `{"metadata":{"key":"value"}}`}, + {http.MethodDelete, "/v1/agents/sessions/" + id, ""}, + {http.MethodGet, "/v1/agents/environments/templates/" + id, ""}, + {http.MethodPost, "/v1/agents/environments/templates/" + id, `{"name":"Renamed"}`}, + {http.MethodDelete, "/v1/agents/environments/templates/" + id, ""}, + } { + t.Run(route.method+" "+route.path, func(t *testing.T) { + s := &missingResourceStore{} + h, tenant, _ := twoTenantHandler(t, s) + var bodies []string + for _, query := range []string{"", "?tenant_id=foreign&include=files&unknown=1&unknown=2"} { + r := httptest.NewRequest(route.method, route.path+query, strings.NewReader(route.body)) + r.Header.Set("Authorization", "Bearer test-api-key") + r.Header.Set("OpenAI-Beta", "agents=v1") + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + if w.Code != http.StatusNotFound { + t.Fatalf("%s: %d %s", query, w.Code, w.Body) + } + bodies = append(bodies, w.Body.String()) + } + if bodies[0] != bodies[1] || len(s.tenants) != 2 || s.tenants[0] != tenant || s.tenants[1] != tenant { + t.Fatalf("query changed the response or tenant: %v %v", bodies, s.tenants) + } + }) + } +} + +// missingSkillStore reports every Skill as missing and records list parameters. +type missingSkillStore struct { + SkillStore + tenants []string + limit int + hasMore bool +} + +func (s *missingSkillStore) GetSkill(_ context.Context, tenant, _ string) (store.Skill, error) { + s.tenants = append(s.tenants, tenant) + return store.Skill{}, store.ErrNotFound +} + +func (s *missingSkillStore) DeleteSkill(_ context.Context, tenant, _ string) error { + s.tenants = append(s.tenants, tenant) + return store.ErrNotFound +} + +func (s *missingSkillStore) GetSkillVersion(_ context.Context, tenant, _, _ string) (store.SkillVersion, error) { + s.tenants = append(s.tenants, tenant) + return store.SkillVersion{}, store.ErrNotFound +} + +func (s *missingSkillStore) ListSkills(_ context.Context, tenant, _ string, limit int, _ bool) (store.SkillPage, error) { + s.tenants, s.limit = append(s.tenants, tenant), limit + return store.SkillPage{HasMore: s.hasMore}, nil +} + +func (s *missingSkillStore) ListSkillVersions(_ context.Context, tenant, _, _ string, limit int, _ bool) (store.SkillVersionPage, error) { + s.tenants, s.limit = append(s.tenants, tenant), limit + return store.SkillVersionPage{HasMore: s.hasMore}, nil +} + +func skillQueryHandler(t *testing.T, s SkillStore) (http.Handler, string) { + t.Helper() + h, tenant, _ := twoTenantHandler(t, &missingResourceStore{}, WithSkills(s)) + return h, tenant +} + +func skillQueryRequest(h http.Handler, method, path string) *httptest.ResponseRecorder { + r := httptest.NewRequest(method, path, nil) + r.Header.Set("Authorization", "Bearer test-api-key") + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w +} + +func TestSkillResourceRoutesIgnoreUnknownQueryKeys(t *testing.T) { + for _, route := range []struct{ method, path string }{ + {http.MethodGet, "/v1/skills/skill_missing"}, + {http.MethodDelete, "/v1/skills/skill_missing"}, + {http.MethodGet, "/v1/skills/skill_missing/versions/1"}, + } { + t.Run(route.method+" "+route.path, func(t *testing.T) { + s := &missingSkillStore{} + h, tenant := skillQueryHandler(t, s) + plain := skillQueryRequest(h, route.method, route.path) + query := skillQueryRequest(h, route.method, route.path+"?tenant_id=foreign&limit=5&unknown=1") + if plain.Code != http.StatusNotFound || query.Code != plain.Code || query.Body.String() != plain.Body.String() || len(s.tenants) != 2 || s.tenants[1] != tenant { + t.Fatalf("query changed the response: %d %s / %d %s %v", plain.Code, plain.Body, query.Code, query.Body, s.tenants) + } + }) + } +} + +func TestSkillListLimitZeroReturnsEmptyPage(t *testing.T) { + for _, path := range []string{"/v1/skills", "/v1/skills/skill_example/versions"} { + for _, hasMore := range []bool{true, false} { + s := &missingSkillStore{hasMore: hasMore} + h, tenant := skillQueryHandler(t, s) + w := skillQueryRequest(h, http.MethodGet, path+"?limit=0&unknown=1") + want := fmt.Sprintf(`{"object":"list","data":[],"first_id":null,"last_id":null,"has_more":%t}`, hasMore) + var got, expected any + if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &got) != nil || json.Unmarshal([]byte(want), &expected) != nil || !reflect.DeepEqual(got, expected) { + t.Fatalf("%s: %d %s", path, w.Code, w.Body) + } + if s.limit != 0 || len(s.tenants) != 1 || s.tenants[0] != tenant { + t.Fatalf("zero page parameters: limit=%d tenants=%v", s.limit, s.tenants) + } + } + } +} + +// Write routes ignore unknown query keys too: the body is still validated, query +// keys never become form fields, and a foreign resource stays equal to a missing one. + +func TestEnvironmentFileCreateIgnoresUnknownQueryKeys(t *testing.T) { + h, f := environmentFileCreateHandler(t) + request := func(id, key, body string) *httptest.ResponseRecorder { + query := "?tenant_id=" + f.environment.TenantID + "&path=/workspace/other&unknown=1" + r := httptest.NewRequest(http.MethodPost, "/v1/agents/environments/"+id+"/files"+query, strings.NewReader(body)) + r.Header.Set("Authorization", "Bearer "+key) + r.Header.Set("OpenAI-Beta", "agents=v1") + w := httptest.NewRecorder() + h.ServeHTTP(environmentFilesRecorder{w}, r) + return w + } + body := `{"type":"inline","data":"YWJj","path":"/workspace/a"}` + foreign, missing := request(f.environment.ID, "other-key", body), request(uuid.NewString(), "files-key", body) + if foreign.Code != http.StatusNotFound || foreign.Body.String() != missing.Body.String() || f.writes != 0 { + t.Fatalf("foreign create: %d %s / %s writes=%d", foreign.Code, foreign.Body, missing.Body, f.writes) + } + if w := request(f.environment.ID, "files-key", `{"type":"inline","path":"/workspace/a"}`); w.Code != http.StatusBadRequest || f.writes != 0 { + t.Fatalf("invalid body admitted: %d", w.Code) + } + if w := request(f.environment.ID, "files-key", body); w.Code != http.StatusOK || f.writes != 1 || f.path != "a" || string(f.data) != "abc" { + t.Fatalf("create: %d %s path=%q", w.Code, w.Body, f.path) + } +} + +type ownedArtifactStore struct { + SessionArtifactStore + owner string + tenants []string + deleted int +} + +func (s *ownedArtifactStore) DeleteSessionArtifact(_ context.Context, tenant, session, id string) error { + s.tenants = append(s.tenants, tenant) + if tenant != s.owner || session != "session" || id != "artifact" { + return store.ErrNotFound + } + s.deleted++ + return nil +} + +func TestArtifactDeletionIgnoresUnknownQueryKeys(t *testing.T) { + s := &ownedArtifactStore{} + h, owner, foreign := twoTenantHandler(t, &missingResourceStore{}, WithSessionArtifacts(s)) + s.owner = owner + request := func(id, key string) *httptest.ResponseRecorder { + r := httptest.NewRequest(http.MethodDelete, "/v1/agents/sessions/session/artifacts/"+id+"?tenant_id="+owner+"&unknown=1", nil) + r.Header.Set("Authorization", "Bearer "+key) + r.Header.Set("OpenAI-Beta", "agents=v1") + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w + } + denied, missing := request("artifact", "foreign-key"), request("missing", "test-api-key") + if denied.Code != http.StatusNotFound || denied.Body.String() != missing.Body.String() || s.deleted != 0 || s.tenants[0] != foreign { + t.Fatalf("foreign deletion: %d %s / %s tenants=%v", denied.Code, denied.Body, missing.Body, s.tenants) + } + w := request("artifact", "test-api-key") + var body map[string]any + if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &body) != nil || body["id"] != "artifact" || body["deleted"] != true || s.deleted != 1 { + t.Fatalf("deletion: %d %s", w.Code, w.Body) + } +} + +func TestSourceFileUploadIgnoresUnknownQueryKeys(t *testing.T) { + f := &sourceFilesFixture{} + h, env := environmentFileCreateHandler(t, WithSourceFiles(f)) + server := newSourceFileServer(t, h) + query := "?purpose=assistants&tenant_id=" + env.environment.TenantID + "&unknown=1" + // A query purpose is not a form field: the body purpose is still validated. + invalid, invalidType := sourceMultipart(t, []string{"file", "purpose=assistants"}, []byte("abc")) + if status, _ := sourceRequest(t, server, http.MethodPost, "/v1/files?purpose=user_data", "files-key", invalidType, invalid); status != http.StatusBadRequest || f.file.ID != "" { + t.Fatalf("query purpose admitted an upload: %d", status) + } + body, contentType := sourceMultipart(t, []string{"file", "purpose=user_data"}, []byte("abc")) + status, raw := sourceRequest(t, server, http.MethodPost, "/v1/files"+query, "files-key", contentType, body) + var file v1.SourceFile + if status != http.StatusOK || json.Unmarshal(raw, &file) != nil || file.Purpose != "user_data" || f.tenant != env.environment.TenantID || string(f.data) != "abc" { + t.Fatalf("upload: %d %s tenant=%s", status, raw, f.tenant) + } + // Another project's upload stays in its own tenant despite the tenant_id key. + if status, _ := sourceRequest(t, server, http.MethodPost, "/v1/files"+query, "other-key", contentType, body); status != http.StatusOK || f.tenant == env.environment.TenantID { + t.Fatalf("foreign upload changed tenant: %d %s", status, f.tenant) + } + if status, _ := sourceRequest(t, server, http.MethodGet, "/v1/files/"+file.ID+query, "other-key", "", nil); status != http.StatusNotFound { + t.Fatalf("foreign read: %d", status) + } +} + +// ownedSkillStore accepts uploads and knows one owned Skill. +type ownedSkillStore struct { + SkillStore + owner string + tenants []string + defaults []bool + created int +} + +func (s *ownedSkillStore) CreateSkill(_ context.Context, tenant string, _ []byte) (store.Skill, error) { + s.tenants, s.created = append(s.tenants, tenant), s.created+1 + return store.Skill{ID: "skill_created", Name: "proof", DefaultVersion: 1, LatestVersion: 1}, nil +} + +func (s *ownedSkillStore) CreateSkillVersion(_ context.Context, tenant, skill string, _ []byte, makeDefault bool) (store.SkillVersion, error) { + s.tenants, s.defaults = append(s.tenants, tenant), append(s.defaults, makeDefault) + if tenant != s.owner || skill != "skill_owned" { + return store.SkillVersion{}, store.ErrNotFound + } + s.created++ + return store.SkillVersion{ID: "skillver_created", SkillID: skill, Name: "proof", Version: 2}, nil +} + +func skillUpload(t *testing.T, include bool) ([]byte, string) { + t.Helper() + var buffer bytes.Buffer + w := multipart.NewWriter(&buffer) + if include { + part, err := w.CreateFormFile("files[]", "proof/SKILL.md") + if err != nil { + t.Fatal(err) + } + if _, err = part.Write([]byte("---\nname: proof\ndescription: Verify query tolerance.\n---\nProof.")); err != nil { + t.Fatal(err) + } + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + return buffer.Bytes(), w.FormDataContentType() +} + +func TestSkillUploadsIgnoreUnknownQueryKeys(t *testing.T) { + s := &ownedSkillStore{} + h, owner, foreign := twoTenantHandler(t, &missingResourceStore{}, WithSkills(s)) + s.owner = owner + server := newSourceFileServer(t, h) + query := "?tenant_id=" + owner + "&default=true&unknown=1" + body, contentType := skillUpload(t, true) + empty, emptyType := skillUpload(t, false) + if status, _ := sourceRequest(t, server, http.MethodPost, "/v1/skills"+query, "test-api-key", emptyType, empty); status != http.StatusBadRequest || s.created != 0 { + t.Fatalf("empty upload admitted: %d", status) + } + status, raw := sourceRequest(t, server, http.MethodPost, "/v1/skills"+query, "test-api-key", contentType, body) + if status != http.StatusOK || !strings.Contains(string(raw), `"id":"skill_created"`) || s.created != 1 || s.tenants[0] != owner { + t.Fatalf("create: %d %s %v", status, raw, s.tenants) + } + // Version creation: a foreign Skill equals a missing one and writes nothing. + _, denied := sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_owned/versions"+query, "foreign-key", contentType, body) + _, missing := sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_missing/versions"+query, "test-api-key", contentType, body) + if string(denied) != string(missing) || !strings.Contains(string(denied), "Resource not found.") || s.created != 1 || s.tenants[1] != foreign { + t.Fatalf("foreign version: %s / %s tenants=%v", denied, missing, s.tenants) + } + status, raw = sourceRequest(t, server, http.MethodPost, "/v1/skills/skill_owned/versions"+query, "test-api-key", contentType, body) + // The default query key is not the multipart default field. + if status != http.StatusOK || !strings.Contains(string(raw), `"version":"2"`) || s.created != 2 || s.defaults[len(s.defaults)-1] { + t.Fatalf("version: %d %s defaults=%v", status, raw, s.defaults) + } +} diff --git a/services/agents-api/internal/api/session_artifacts.go b/services/agents-api/internal/api/session_artifacts.go index ecadde853..5b049909b 100644 --- a/services/agents-api/internal/api/session_artifacts.go +++ b/services/agents-api/internal/api/session_artifacts.go @@ -22,15 +22,11 @@ func WithSessionArtifacts(s SessionArtifactStore) Option { return func(h *Handler) { h.artifacts = s } } -func (h *Handler) artifactsReady(w http.ResponseWriter, r *http.Request, list bool) bool { +func (h *Handler) artifactsReady(w http.ResponseWriter) bool { if h.artifacts == nil { writeError(w, http.StatusServiceUnavailable, "artifact_storage_unavailable", "Artifact storage is unavailable.") return false } - if !list && r.URL.RawQuery != "" { - writeStoreError(w, r, store.ErrInvalidInput) - return false - } return true } @@ -49,7 +45,7 @@ func (h *Handler) artifactsReady(w http.ResponseWriter, r *http.Request, list bo // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/artifacts [get] func (h *Handler) listSessionArtifacts(w http.ResponseWriter, r *http.Request) { - if !h.artifactsReady(w, r, true) { + if !h.artifactsReady(w) { return } options, ok := readPage(w, r, "environment_id") @@ -79,7 +75,7 @@ func (h *Handler) listSessionArtifacts(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/artifacts/{artifact_id} [get] func (h *Handler) getSessionArtifact(w http.ResponseWriter, r *http.Request) { - if !h.artifactsReady(w, r, false) { + if !h.artifactsReady(w) { return } artifact, err := h.artifacts.GetSessionArtifact(r.Context(), tenantID(r), chi.URLParam(r, "session_id"), chi.URLParam(r, "artifact_id")) @@ -102,7 +98,7 @@ func (h *Handler) getSessionArtifact(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/artifacts/{artifact_id} [delete] func (h *Handler) deleteSessionArtifact(w http.ResponseWriter, r *http.Request) { - if !h.artifactsReady(w, r, false) { + if !h.artifactsReady(w) { return } id := chi.URLParam(r, "artifact_id") @@ -125,7 +121,7 @@ func (h *Handler) deleteSessionArtifact(w http.ResponseWriter, r *http.Request) // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/artifacts/{artifact_id}/content [get] func (h *Handler) sessionArtifactContent(w http.ResponseWriter, r *http.Request) { - if !h.artifactsReady(w, r, false) { + if !h.artifactsReady(w) { return } serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { diff --git a/services/agents-api/internal/api/session_artifacts_test.go b/services/agents-api/internal/api/session_artifacts_test.go index 4a57706e9..37b22cadb 100644 --- a/services/agents-api/internal/api/session_artifacts_test.go +++ b/services/agents-api/internal/api/session_artifacts_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -86,12 +87,18 @@ func TestSessionArtifactRoutesAndPublicProjection(t *testing.T) { if w.Code != 200 || json.Unmarshal(w.Body.Bytes(), &deleted) != nil || !deleted.Deleted || deleted.ID != "artifact" || deleted.Object != "agent.session.artifact.deleted" { t.Fatalf("delete: %d %s", w.Code, w.Body) } - for _, suffix := range []string{"?limit=0", "?limit=101", "?order=random", "?environment_id=a&environment_id=b", "?limit=1&limit=2", "?unknown=x", "/artifact?unknown=x", "/artifact/content?unknown=x"} { + for _, suffix := range []string{"?limit=0", "?limit=101", "?limit=-1", "?order=random", "?environment_id=a&environment_id=b", "?limit=1&limit=2"} { before := f.calls - if w := request("GET", suffix, "agents=v1"); w.Code != 400 || f.calls != before { + if w := request("GET", suffix, "agents=v1"); w.Code != 400 || f.calls != before || !strings.Contains(w.Body.String(), `"code":"invalid_request_error"`) { t.Fatalf("invalid query reached storage: %s %d", suffix, w.Code) } } + for _, suffix := range []string{"?unknown=x&tenant_id=other", "/artifact?unknown=x", "/artifact/content?unknown=x"} { + before := f.calls + if w := request("GET", suffix, "agents=v1"); w.Code != 200 || f.calls != before+1 || f.tenant != tenant { + t.Fatalf("unknown query was not ignored: %s %d", suffix, w.Code) + } + } for _, route := range []struct{ method, suffix string }{{"GET", ""}, {"GET", "/artifact"}, {"GET", "/artifact/content"}, {"DELETE", "/artifact"}} { before := f.calls if w := request(route.method, route.suffix, ""); w.Code != 400 || f.calls != before { diff --git a/services/agents-api/internal/api/session_deletion.go b/services/agents-api/internal/api/session_deletion.go index 4f3e76fda..1ef76af01 100644 --- a/services/agents-api/internal/api/session_deletion.go +++ b/services/agents-api/internal/api/session_deletion.go @@ -21,10 +21,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id} [delete] func (h *Handler) deleteSession(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Session deletion does not accept query parameters.") - return - } body, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/session_metadata.go b/services/agents-api/internal/api/session_metadata.go index b871bc166..75b35a468 100644 --- a/services/agents-api/internal/api/session_metadata.go +++ b/services/agents-api/internal/api/session_metadata.go @@ -23,10 +23,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id} [post] func (h *Handler) updateSession(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Session updates do not accept query parameters.") - return - } raw, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/skills.go b/services/agents-api/internal/api/skills.go index b98a5673d..a613f7029 100644 --- a/services/agents-api/internal/api/skills.go +++ b/services/agents-api/internal/api/skills.go @@ -40,15 +40,11 @@ func (h *Handler) registerSkillRoutes(r chi.Router) { r.Get("/v1/skills/{skill_id}/versions/{version}/content", h.skillVersionContent) } -func (h *Handler) skillsReady(w http.ResponseWriter, r *http.Request, list bool) bool { +func (h *Handler) skillsReady(w http.ResponseWriter) bool { if h.skills == nil { writeError(w, http.StatusServiceUnavailable, "skill_storage_unavailable", "Skill storage is unavailable.") return false } - if !list && r.URL.RawQuery != "" { - writeStoreError(w, r, store.ErrInvalidInput) - return false - } return true } @@ -61,7 +57,7 @@ func (h *Handler) skillsReady(w http.ResponseWriter, r *http.Request, list bool) // @Success 200 {object} v1.Skill // @Router /skills/{skill_id} [get] func (h *Handler) getSkill(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } value, err := h.skills.GetSkill(r.Context(), tenantID(r), chi.URLParam(r, "skill_id")) @@ -83,7 +79,7 @@ func (h *Handler) getSkill(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.Skill // @Router /skills/{skill_id} [post] func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } body, ok := readJSONBodyLimit(w, r, 64<<10, "Request exceeds 64 KiB.") @@ -112,7 +108,7 @@ func (h *Handler) updateSkill(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.SkillDeleted // @Router /skills/{skill_id} [delete] func (h *Handler) deleteSkill(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } id := chi.URLParam(r, "skill_id") @@ -132,7 +128,7 @@ func (h *Handler) deleteSkill(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.SkillVersion // @Router /skills/{skill_id}/versions/{version} [get] func (h *Handler) getSkillVersion(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } value, err := h.skills.GetSkillVersion(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), chi.URLParam(r, "version")) @@ -153,7 +149,7 @@ func (h *Handler) getSkillVersion(w http.ResponseWriter, r *http.Request) { // @Success 200 {object} v1.SkillVersionDeleted // @Router /skills/{skill_id}/versions/{version} [delete] func (h *Handler) deleteSkillVersion(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } value, err := h.skills.DeleteSkillVersion(r.Context(), tenantID(r), chi.URLParam(r, "skill_id"), chi.URLParam(r, "version")) diff --git a/services/agents-api/internal/api/skills_list.go b/services/agents-api/internal/api/skills_list.go index 6692df0c4..6fd18dde4 100644 --- a/services/agents-api/internal/api/skills_list.go +++ b/services/agents-api/internal/api/skills_list.go @@ -8,17 +8,17 @@ import ( ) // @Summary List Skills -// @Description Lists tenant-owned metadata in timestamp order. Default page size 20, maximum 100; exact hosted defaults and limit-zero semantics remain unverified. +// @Description Lists tenant-owned metadata in timestamp order. Default page size 20, maximum 100. Limit 0 returns an empty page whose has_more reports whether any Skill follows the cursor; exact hosted defaults remain unverified. // @Tags Skills // @Produce json // @Security BearerAuth // @Param after query string false "Skill resource cursor" -// @Param limit query integer false "Page size" +// @Param limit query integer false "Page size; 0 returns an empty page" default(20) minimum(0) maximum(100) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) // @Success 200 {object} v1.SkillList // @Router /skills [get] func (h *Handler) listSkills(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, true) { + if !h.skillsReady(w) { return } options, ok := readPage(w, r) @@ -42,18 +42,18 @@ func (h *Handler) listSkills(w http.ResponseWriter, r *http.Request) { } // @Summary List Skill versions -// @Description Orders by version number; after identifies a version resource, not a version number. No contents are decrypted. +// @Description Orders by version number; after identifies a version resource, not a version number. No contents are decrypted. Limit 0 returns an empty page whose has_more reports whether any version follows the cursor. // @Tags Skills // @Produce json // @Security BearerAuth // @Param skill_id path string true "Skill ID" // @Param after query string false "Version resource cursor" -// @Param limit query integer false "Page size" +// @Param limit query integer false "Page size; 0 returns an empty page" default(20) minimum(0) maximum(100) // @Param order query string false "Version order; omit for descending, explicit empty values are invalid" Enums(asc,desc) // @Success 200 {object} v1.SkillVersionList // @Router /skills/{skill_id}/versions [get] func (h *Handler) listSkillVersions(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, true) { + if !h.skillsReady(w) { return } options, ok := readPage(w, r) diff --git a/services/agents-api/internal/api/skills_transfer.go b/services/agents-api/internal/api/skills_transfer.go index 0000cf040..70b748897 100644 --- a/services/agents-api/internal/api/skills_transfer.go +++ b/services/agents-api/internal/api/skills_transfer.go @@ -39,7 +39,7 @@ func (h *Handler) createSkillVersion(w http.ResponseWriter, r *http.Request) { } func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bool) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } deadline := time.Now().Add(sourceTransferTimeout) @@ -87,7 +87,7 @@ func (h *Handler) uploadSkill(w http.ResponseWriter, r *http.Request, version bo // @Success 200 {file} binary // @Router /skills/{skill_id}/content [get] func (h *Handler) skillContent(w http.ResponseWriter, r *http.Request) { - if !h.skillsReady(w, r, false) { + if !h.skillsReady(w) { return } serveStoredContent(w, r, func(ctx context.Context, consume func(string, int64, io.Reader) error) error { diff --git a/services/agents-api/internal/api/source_files.go b/services/agents-api/internal/api/source_files.go index 5ee7f04bc..940f93632 100644 --- a/services/agents-api/internal/api/source_files.go +++ b/services/agents-api/internal/api/source_files.go @@ -31,17 +31,6 @@ func (h *Handler) sourceFilesAvailable(w http.ResponseWriter) bool { return true } -func (h *Handler) sourceFilesReady(w http.ResponseWriter, r *http.Request) bool { - if !h.sourceFilesAvailable(w) { - return false - } - if r.URL.RawQuery != "" { - writeStoreError(w, r, store.ErrInvalidInput) - return false - } - return true -} - // @Summary Retrieve source file metadata // @Description Returns immutable project-owned user_data file metadata. No Beta header is required. Other purposes, expiration and full hosted status/error semantics remain unimplemented or unverified. // @Tags Files @@ -52,7 +41,7 @@ func (h *Handler) sourceFilesReady(w http.ResponseWriter, r *http.Request) bool // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /files/{file_id} [get] func (h *Handler) getSourceFile(w http.ResponseWriter, r *http.Request) { - if !h.sourceFilesReady(w, r) { + if !h.sourceFilesAvailable(w) { return } ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second) @@ -75,7 +64,7 @@ func (h *Handler) getSourceFile(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /files/{file_id} [delete] func (h *Handler) deleteSourceFile(w http.ResponseWriter, r *http.Request) { - if !h.sourceFilesReady(w, r) { + if !h.sourceFilesAvailable(w) { return } ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second) diff --git a/services/agents-api/internal/api/source_files_content.go b/services/agents-api/internal/api/source_files_content.go index d65237fd0..9abf81c81 100644 --- a/services/agents-api/internal/api/source_files_content.go +++ b/services/agents-api/internal/api/source_files_content.go @@ -20,7 +20,7 @@ import ( // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /files/{file_id}/content [get] func (h *Handler) sourceFileContent(w http.ResponseWriter, r *http.Request) { - if !h.sourceFilesReady(w, r) { + if !h.sourceFilesAvailable(w) { return } ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second) diff --git a/services/agents-api/internal/api/source_files_errors_test.go b/services/agents-api/internal/api/source_files_errors_test.go index afdb23cfd..e7e7b75c9 100644 --- a/services/agents-api/internal/api/source_files_errors_test.go +++ b/services/agents-api/internal/api/source_files_errors_test.go @@ -16,6 +16,11 @@ func TestSourceFileMissingErrorParameters(t *testing.T) { {http.MethodDelete, "/v1/files/file-missing", "id"}, {http.MethodGet, "/v1/files/file-missing/content", "id"}, {http.MethodGet, "/v1/files?after=file-missing", "after"}, + // Unknown query keys do not change single-resource or list lookups. + {http.MethodGet, "/v1/files/file-missing?unknown=1&tenant_id=foreign", "id"}, + {http.MethodDelete, "/v1/files/file-missing?unknown=1", "id"}, + {http.MethodGet, "/v1/files/file-missing/content?unknown=1", "id"}, + {http.MethodGet, "/v1/files?after=file-missing&unknown=1", "after"}, } { t.Run(tc.method+tc.path, func(t *testing.T) { f := &sourceFilesFixture{listErr: fmt.Errorf("wrapped: %w", store.ErrNotFound)} diff --git a/services/agents-api/internal/api/source_files_list.go b/services/agents-api/internal/api/source_files_list.go index 68c8fddb9..e7dbaf46e 100644 --- a/services/agents-api/internal/api/source_files_list.go +++ b/services/agents-api/internal/api/source_files_list.go @@ -7,7 +7,7 @@ import ( ) // @Summary List source files -// @Description Lists project-owned Files without reading their bodies. The limit defaults to 10000 and must be 1–10000. Equal creation times use ID ordering. Purpose validation precedes cursor lookup; current storage contains only user_data. An explicit empty purpose retains the local exact-filter behavior. Hosted positive filtering, default order and concurrent-page behavior remain unverified. No Beta header is required. +// @Description Lists project-owned Files without reading their bodies. The limit defaults to 10000 and must be 1–10000. Equal creation times use ID ordering. Purpose validation precedes cursor lookup; current storage contains only user_data. An explicit empty purpose is treated as omitted. Repeated purpose values remain rejected. Hosted positive filtering, default order and concurrent-page behavior remain unverified. No Beta header is required. // @Tags Files // @Produce json // @Security BearerAuth @@ -44,16 +44,17 @@ func (h *Handler) listSourceFiles(w http.ResponseWriter, r *http.Request) { func readSourceFilePage(w http.ResponseWriter, r *http.Request) (pageOptions, *string, bool) { q := r.URL.Query() - options, ok := readPageQueryLimits(w, r, q, 10000, 10000, true, "purpose") + options, ok := readPageQueryLimits(w, r, q, 10000, 10000, false, "purpose") if !ok { return pageOptions{}, nil, false } - values, present := q["purpose"] - if !present { + // An explicit empty purpose applies no filter, as observed on the hosted service. + values := q["purpose"] + if len(values) == 0 || values[0] == "" { return options, nil, true } switch values[0] { - case "", "user_data", "assistants", "batch", "fine-tune", "vision", "evals", "assistants_output", "batch_output", "fine-tune-results": + case "user_data", "assistants", "batch", "fine-tune", "vision", "evals", "assistants_output", "batch_output", "fine-tune-results": default: writeError(w, http.StatusBadRequest, "", "Invalid purpose.", "purpose") return pageOptions{}, nil, false diff --git a/services/agents-api/internal/api/source_files_list_test.go b/services/agents-api/internal/api/source_files_list_test.go index 61e65ac25..2e82cdcef 100644 --- a/services/agents-api/internal/api/source_files_list_test.go +++ b/services/agents-api/internal/api/source_files_list_test.go @@ -43,16 +43,48 @@ func TestSourceFileListParametersAndEnvelope(t *testing.T) { } func TestSourceFileListRejectsInvalidQueriesBeforeStorage(t *testing.T) { - for _, query := range []string{ - "limit=", "limit=0", "limit=10001", "limit=1.5", "limit=1&limit=2", - "order=", "order=invalid", "after=a&after=b", "purpose=a&purpose=b", "tenant_id=foreign", + duplicate := "Supported list parameters are after, limit, order and purpose, each supplied once." + for _, test := range []struct { + query, message string + code any + }{ + // Hosted range errors have a null code; other local rejections are unchanged. + {"limit=0", "limit must be between 1 and 10000.", nil}, + {"limit=10001", "limit must be between 1 and 10000.", nil}, + {"limit=-1", "limit must be between 1 and 10000.", nil}, + {"limit=", "limit must be between 1 and 10000.", "invalid_request"}, + {"limit=1.5", "limit must be between 1 and 10000.", "invalid_request"}, + {"limit=1&limit=2", duplicate, "unsupported_parameter"}, + {"after=a&after=b", duplicate, "unsupported_parameter"}, + {"purpose=a&purpose=b", duplicate, "unsupported_parameter"}, + {"purpose=user_data&purpose=user_data", duplicate, "unsupported_parameter"}, + {"order=", "order must be asc or desc.", nil}, + {"order=invalid", "order must be asc or desc.", nil}, } { - f := &sourceFilesFixture{} - h, _ := environmentFileCreateHandler(t, WithSourceFiles(f)) - server := newSourceFileServer(t, h) - status, _ := sourceRequest(t, server, http.MethodGet, "/v1/files?"+query, "files-key", "", nil) - if status != http.StatusBadRequest || f.listCalls != 0 { - t.Fatalf("invalid %q: status=%d calls=%d", query, status, f.listCalls) + t.Run(test.query, func(t *testing.T) { + f := &sourceFilesFixture{} + h, _ := environmentFileCreateHandler(t, WithSourceFiles(f)) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/v1/files?"+test.query, nil) + r.Header.Set("Authorization", "Bearer files-key") + h.ServeHTTP(w, r) + assertListQueryError(t, w, test.code, nil, test.message) + if f.listCalls != 0 { + t.Fatalf("invalid %q reached storage", test.query) + } + }) + } +} + +func TestSourceFileListIgnoresUnknownKeysAndEmptyPurpose(t *testing.T) { + f := &sourceFilesFixture{} + h, env := environmentFileCreateHandler(t, WithSourceFiles(f)) + server := newSourceFileServer(t, h) + want, wantBody := sourceRequest(t, server, http.MethodGet, "/v1/files?after=file-a&limit=2&order=asc", "files-key", "", nil) + for _, query := range []string{"purpose=", "unknown=1", "tenant_id=foreign", "purpose[]=batch", "unknown=1&unknown=2&purpose="} { + status, body := sourceRequest(t, server, http.MethodGet, "/v1/files?after=file-a&limit=2&order=asc&"+query, "files-key", "", nil) + if status != want || string(body) != string(wantBody) || f.tenant != env.environment.TenantID || f.listPurpose != nil || f.listAfter != "file-a" || f.listLimit != 2 || !f.listAsc { + t.Fatalf("%s changed the listing: %d %s tenant=%s purpose=%v", query, status, body, f.tenant, f.listPurpose) } } } @@ -85,7 +117,10 @@ func TestSourceFileListPurposeValidationBeforeCursorLookup(t *testing.T) { if e := body["error"]; e["type"] != "invalid_request_error" || e["code"] != nil || e["param"] != wantParam { t.Fatalf("purpose error projection: %s", raw) } - if wantCalls == 1 && (f.listPurpose == nil || *f.listPurpose != purpose) { + if wantCalls == 1 && purpose == "" && f.listPurpose != nil { + t.Fatalf("empty purpose filtered: %v", *f.listPurpose) + } + if wantCalls == 1 && purpose != "" && (f.listPurpose == nil || *f.listPurpose != purpose) { t.Fatalf("purpose filter changed: %v", f.listPurpose) } }) diff --git a/services/agents-api/internal/api/source_files_upload.go b/services/agents-api/internal/api/source_files_upload.go index 153336ab1..23475e492 100644 --- a/services/agents-api/internal/api/source_files_upload.go +++ b/services/agents-api/internal/api/source_files_upload.go @@ -25,7 +25,7 @@ const sourceTransferTimeout = 5 * time.Minute // @Failure 400,401,413,500,503 {object} v1.ErrorResponse // @Router /files [post] func (h *Handler) createSourceFile(w http.ResponseWriter, r *http.Request) { - if !h.sourceFilesReady(w, r) { + if !h.sourceFilesAvailable(w) { return } deadline := time.Now().Add(sourceTransferTimeout) diff --git a/services/agents-api/internal/api/stream.go b/services/agents-api/internal/api/stream.go index 0bb337039..175f75930 100644 --- a/services/agents-api/internal/api/stream.go +++ b/services/agents-api/internal/api/stream.go @@ -31,10 +31,6 @@ type eventStore interface { // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/events [get] func (h *Handler) streamEvents(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Live streams do not accept query parameters.") - return - } events, ok := h.store.(eventStore) if !ok { writeError(w, http.StatusServiceUnavailable, "stream_unavailable", "Live events are unavailable.") diff --git a/services/agents-api/internal/api/subagent_turns.go b/services/agents-api/internal/api/subagent_turns.go index d090f1f3e..2ec1d7cab 100644 --- a/services/agents-api/internal/api/subagent_turns.go +++ b/services/agents-api/internal/api/subagent_turns.go @@ -19,10 +19,6 @@ import ( // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/subagents/{subagent_id}/turns/{turn_id} [get] func (h *Handler) getSubagentTurn(w http.ResponseWriter, r *http.Request) { - if r.URL.RawQuery != "" { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Subagent Turn retrieval does not accept query parameters.") - return - } if !h.subagentsReady(w) { return } diff --git a/services/agents-api/internal/api/subagents.go b/services/agents-api/internal/api/subagents.go index a7a54306b..8d960fc5c 100644 --- a/services/agents-api/internal/api/subagents.go +++ b/services/agents-api/internal/api/subagents.go @@ -51,10 +51,6 @@ func (h *Handler) subagentsReady(w http.ResponseWriter) bool { // @Failure 400,401,404,500,503 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/subagents/{subagent_id} [get] func (h *Handler) getSubagent(w http.ResponseWriter, r *http.Request) { - if r.URL.RawQuery != "" { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Subagent retrieval does not accept query parameters.") - return - } if !h.subagentsReady(w) { return } diff --git a/services/agents-api/internal/api/subagents_test.go b/services/agents-api/internal/api/subagents_test.go index 08eb22312..0a5f8cf9d 100644 --- a/services/agents-api/internal/api/subagents_test.go +++ b/services/agents-api/internal/api/subagents_test.go @@ -141,22 +141,40 @@ func TestSubagentRoutesRejectInvalidQueriesBeforeStore(t *testing.T) { s := &subagentReadStore{} h, _, _ := testHandler(t, WithSubagents(s)) for _, route := range subagentRoutes { - queries := []string{"unknown=1", "tenant_id=foreign"} - if route.list { - queries = append(queries, "limit=0", "limit=101", "limit=null", "limit=2&limit=3", "order=random", "order=asc&order=desc", "after=a&after=b") - } else { - queries = append(queries, "limit=1", "after=a", "order=asc") + if !route.list { + continue } - for _, query := range queries { + for _, query := range []string{"limit=0", "limit=101", "limit=null", "limit=-1", "limit=2&limit=3", "order=random", "order=asc&order=desc", "after=a&after=b"} { calls := s.calls w := requestSubagents(h, route.path+"?"+query, "Bearer test-api-key", "agents=v1") - if w.Code != http.StatusBadRequest || s.calls != calls { + if w.Code != http.StatusBadRequest || s.calls != calls || !strings.Contains(w.Body.String(), `"code":"invalid_request_error"`) { t.Fatalf("accepted %s?%s: %d %s", route.path, query, w.Code, w.Body) } } } } +func TestSubagentRoutesIgnoreUnknownQueryKeys(t *testing.T) { + s := &subagentReadStore{} + h, _, tenant := testHandler(t, WithSubagents(s)) + for _, route := range subagentRoutes { + // Retrieval routes also ignore list keys, which carry no semantics there. + for _, query := range []string{"unknown=1", "tenant_id=foreign&unknown=1&unknown=2", "limit=1&after=a&order=asc"} { + if route.list && strings.Contains(query, "limit") { + continue + } + calls := s.calls + w := requestSubagents(h, route.path+"?"+query, "Bearer test-api-key", "agents=v1") + if w.Code != http.StatusOK || s.calls != calls+1 || s.method != route.method || s.tenant != tenant || s.subagent != route.subagent || s.turn != route.turn { + t.Fatalf("query changed %s?%s: %d %s %+v", route.path, query, w.Code, w.Body, s) + } + if route.list && (s.limit != 20 || s.after != "" || s.ascending) { + t.Fatalf("unknown keys changed pagination: %+v", s) + } + } + } +} + func TestSubagentRoutesUseExistingAuthenticationAndErrors(t *testing.T) { s := &subagentReadStore{} h, _, _ := testHandler(t, WithSubagents(s)) diff --git a/services/agents-api/internal/api/turns.go b/services/agents-api/internal/api/turns.go index ded7ebb91..d8259da90 100644 --- a/services/agents-api/internal/api/turns.go +++ b/services/agents-api/internal/api/turns.go @@ -22,10 +22,6 @@ import ( // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /agents/sessions/{session_id}/turns/{turn_id} [get] func (h *Handler) getTurn(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Turn retrieval does not accept query parameters.") - return - } sessionID := chi.URLParam(r, "session_id") turn, err := h.store.GetTurn(r.Context(), tenantID(r), sessionID, chi.URLParam(r, "turn_id")) if err != nil { diff --git a/services/agents-api/internal/api/turns_test.go b/services/agents-api/internal/api/turns_test.go index de4d6f5fc..10e85d0a9 100644 --- a/services/agents-api/internal/api/turns_test.go +++ b/services/agents-api/internal/api/turns_test.go @@ -65,13 +65,16 @@ func TestTurnRoutesUseAuthenticatedScopeAndSafeProjection(t *testing.T) { if w := request("/v1/agents/sessions/session/turns"); w.Code != 200 || s.limit != 20 || s.ascending { t.Fatalf("bad defaults: %d", w.Code) } - for _, query := range []string{"limit=0", "limit=101", "limit=2&limit=3", "order=random", "agent_id=other"} { - if w := request("/v1/agents/sessions/session/turns?" + query); w.Code != 400 { - t.Fatalf("accepted %s: %d", query, w.Code) + for _, query := range []string{"limit=0", "limit=101", "limit=-1", "limit=2&limit=3", "order=random"} { + if w := request("/v1/agents/sessions/session/turns?" + query); w.Code != 400 || !strings.Contains(w.Body.String(), `"code":"invalid_request_error"`) { + t.Fatalf("accepted %s: %d %s", query, w.Code, w.Body) } } - if w := request("/v1/agents/sessions/session/turns/turn?unknown=1"); w.Code != 400 { - t.Fatalf("accepted retrieve query: %d", w.Code) + if w := request("/v1/agents/sessions/session/turns?agent_id=other&tenant_id=other&limit=3"); w.Code != 200 || s.tenant != tenant || s.limit != 3 { + t.Fatalf("unknown list keys changed the page: %d %s", w.Code, s.tenant) + } + if w := request("/v1/agents/sessions/session/turns/turn?unknown=1&tenant_id=other"); w.Code != 200 || s.tenant != tenant || s.turnID != "turn" { + t.Fatalf("retrieve query was not ignored: %d", w.Code) } s.session.Configuration = json.RawMessage(`{}`) if w := request("/v1/agents/sessions/session/turns/turn"); w.Code != 500 || strings.Contains(w.Body.String(), "snapshot") { diff --git a/services/agents-api/internal/api/vault_pagination.go b/services/agents-api/internal/api/vault_pagination.go index 5f9b614ee..08fbede36 100644 --- a/services/agents-api/internal/api/vault_pagination.go +++ b/services/agents-api/internal/api/vault_pagination.go @@ -3,37 +3,31 @@ package api import ( "errors" "net/http" + "slices" "strconv" ) func readVaultPage(w http.ResponseWriter, r *http.Request) (pageOptions, []string, bool) { q := r.URL.Query() - statuses, scalar := q["status"] - array, bracketed := q["status[]"] - if (scalar && bracketed) || (scalar && len(statuses) != 1) { - writeError(w, http.StatusBadRequest, "invalid_request", "Supply status once or use status[] for an array.") + if len(q["status"]) > 1 { + writeListDuplicateError(w, r, "status", nil) return pageOptions{}, nil, false } - if bracketed { - statuses = array - } + // A scalar status and status[] entries filter by their union. + statuses := slices.Concat(q["status"], q["status[]"]) for _, status := range statuses { if status != "active" && status != "archived" { writeError(w, http.StatusBadRequest, "invalid_request_error", "Failed to deserialize query string: status: data did not match any variant of untagged enum VaultStatusFilterParam") return pageOptions{}, nil, false } } - q.Del("status") - q.Del("status[]") - // Vault and Credential limits clamp at both ends; other lists retain their policy. + // Vault and Credential limits also clamp negative and overflowing integers; + // other values reach the shared Beta parser. if raw := q["limit"]; len(raw) == 1 { - requested, err := strconv.ParseInt(raw[0], 10, 64) - if err != nil && !errors.Is(err, strconv.ErrRange) { - writeError(w, http.StatusBadRequest, "invalid_request", "limit must be an integer.") - return pageOptions{}, nil, false + if requested, err := strconv.ParseInt(raw[0], 10, 64); err == nil || errors.Is(err, strconv.ErrRange) { + q.Set("limit", strconv.FormatInt(max(1, min(requested, 100)), 10)) } - q.Set("limit", strconv.FormatInt(max(1, min(requested, 100)), 10)) } - options, ok := readPageQuery(w, r, q, false) + options, ok := readPageQuery(w, r, q, true) return options, statuses, ok } diff --git a/services/agents-api/internal/api/vault_pagination_test.go b/services/agents-api/internal/api/vault_pagination_test.go index 1fe129480..d743af7ad 100644 --- a/services/agents-api/internal/api/vault_pagination_test.go +++ b/services/agents-api/internal/api/vault_pagination_test.go @@ -3,12 +3,13 @@ package api import ( "net/http" "net/http/httptest" + "reflect" "testing" ) func TestVaultStatusErrorEnvelopes(t *testing.T) { for _, path := range []string{"/v1/vaults", "/v1/vaults/vault-example/credentials"} { - for _, query := range []string{"status=invalid", "status=", "status[]=active&status[]=invalid", "status[]="} { + for _, query := range []string{"status=invalid", "status=", "status[]=active&status[]=invalid", "status[]=", "status=active&status[]=invalid", "status=invalid&status[]=active"} { t.Run(path+"?"+query, func(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, path+"?"+query, nil) @@ -21,15 +22,61 @@ func TestVaultStatusErrorEnvelopes(t *testing.T) { } } -func TestVaultStatusGrammarUnchanged(t *testing.T) { - for _, query := range []string{"status=active&status=archived", "status=active&status[]=archived"} { - t.Run(query, func(t *testing.T) { - w := httptest.NewRecorder() - r := httptest.NewRequest(http.MethodGet, "/v1/vaults?"+query, nil) - if _, _, ok := readVaultPage(w, r); ok { - t.Fatal("invalid status grammar accepted") - } - assertListQueryError(t, w, "invalid_request", nil, "Supply status once or use status[] for an array.") - }) +func TestVaultStatusUnionAndDuplicates(t *testing.T) { + for _, path := range []string{"/v1/vaults", "/v1/vaults/vault-example/credentials"} { + for _, test := range []struct { + query string + statuses []string + }{ + {"status=active&status[]=archived", []string{"active", "archived"}}, + {"status[]=archived&status=active", []string{"active", "archived"}}, + {"status=active&status[]=active&status[]=archived", []string{"active", "active", "archived"}}, + {"status=archived&unknown=1", []string{"archived"}}, + } { + t.Run(path+"?"+test.query, func(t *testing.T) { + w := httptest.NewRecorder() + _, statuses, ok := readVaultPage(w, httptest.NewRequest(http.MethodGet, path+"?"+test.query, nil)) + if !ok || !reflect.DeepEqual(statuses, test.statuses) || w.Body.Len() != 0 { + t.Fatalf("statuses=%v ok=%t response=%s", statuses, ok, w.Body.String()) + } + }) + } + for _, key := range []string{"status", "limit", "after", "order"} { + t.Run(path+"/duplicate-"+key, func(t *testing.T) { + w := httptest.NewRecorder() + values := map[string]string{"status": "active", "limit": "5", "after": "x", "order": "asc"} + query := key + "=" + values[key] + "&" + key + "=" + values[key] + if _, _, ok := readVaultPage(w, httptest.NewRequest(http.MethodGet, path+"?"+query, nil)); ok { + t.Fatal("repeated key accepted") + } + assertListQueryError(t, w, "invalid_request_error", nil, "Failed to deserialize query string: duplicate field `"+key+"`") + }) + } + } +} + +func TestVaultLimitClamp(t *testing.T) { + for _, path := range []string{"/v1/vaults", "/v1/vaults/vault-example/credentials"} { + for query, limit := range map[string]int{ + "": 20, "limit=0": 1, "limit=-1": 1, "limit=-8": 1, "limit=3": 3, "limit=100": 100, "limit=101": 100, + "limit=9999999999999999999999999": 100, "limit=-9999999999999999999999999": 1, + } { + t.Run(path+"?"+query, func(t *testing.T) { + w := httptest.NewRecorder() + page, _, ok := readVaultPage(w, httptest.NewRequest(http.MethodGet, path+"?"+query, nil)) + if !ok || page.limit != limit || w.Body.Len() != 0 { + t.Fatalf("page=%+v ok=%t response=%s", page, ok, w.Body.String()) + } + }) + } + for _, query := range []string{"limit=abc", "limit=1.5", "limit=", "limit=null"} { + t.Run(path+"?"+query, func(t *testing.T) { + w := httptest.NewRecorder() + if _, _, ok := readVaultPage(w, httptest.NewRequest(http.MethodGet, path+"?"+query, nil)); ok { + t.Fatal("non-integer limit accepted") + } + assertListQueryError(t, w, "invalid_request_error", nil, invalidDigit) + }) + } } } diff --git a/services/agents-api/internal/api/vaults.go b/services/agents-api/internal/api/vaults.go index c96464724..27b129686 100644 --- a/services/agents-api/internal/api/vaults.go +++ b/services/agents-api/internal/api/vaults.go @@ -32,10 +32,6 @@ type VaultStore interface { // @Failure 400,401,413,500 {object} v1.ErrorResponse // @Router /vaults [post] func (h *Handler) createVault(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Vault creation does not accept query parameters.") - return - } raw, ok := readJSONBody(w, r) if !ok { return @@ -87,10 +83,6 @@ func (h *Handler) createVault(w http.ResponseWriter, r *http.Request) { // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /vaults/{vault_id} [get] func (h *Handler) getVault(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Vault retrieval does not accept query parameters.") - return - } id := chi.URLParam(r, "vault_id") if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil { writeStoreError(w, r, store.ErrNotFound) diff --git a/services/agents-api/internal/api/vaults_delete.go b/services/agents-api/internal/api/vaults_delete.go index 21ee01d05..c67367713 100644 --- a/services/agents-api/internal/api/vaults_delete.go +++ b/services/agents-api/internal/api/vaults_delete.go @@ -18,10 +18,6 @@ import ( // @Failure 400,401,404,413,500 {object} v1.ErrorResponse // @Router /vaults/{vault_id} [delete] func (h *Handler) deleteVault(w http.ResponseWriter, r *http.Request) { - if len(r.URL.Query()) > 0 { - writeError(w, http.StatusBadRequest, "unsupported_parameter", "Vault deletion does not accept query parameters.") - return - } body, ok := readJSONBody(w, r) if !ok { return diff --git a/services/agents-api/internal/api/vaults_delete_test.go b/services/agents-api/internal/api/vaults_delete_test.go index 9604e3ed5..eabae4dc8 100644 --- a/services/agents-api/internal/api/vaults_delete_test.go +++ b/services/agents-api/internal/api/vaults_delete_test.go @@ -19,28 +19,29 @@ func (f *vaultResourceFixture) DeleteVault(_ context.Context, tenant, id string) } func TestVaultDeletionConfirmationAndScope(t *testing.T) { - h, f := vaultResourceHandler(t) - w := vaultRequest(h, "DELETE", "/v1/vaults/"+f.vault.ID, "") - var got map[string]any - if w.Code != 200 || json.Unmarshal(w.Body.Bytes(), &got) != nil { - t.Fatal("deletion failed", w.Code) - } - want := map[string]any{"id": f.vault.ID, "deleted": true, "object": "vault.deleted"} - if !reflect.DeepEqual(got, want) || f.tenant != f.vault.TenantID || f.id != f.vault.ID || f.calls != 1 { - t.Fatal("deletion changed authenticated scope or confirmation") + // Unknown query keys, including a tenant hint, are ignored. + for _, query := range []string{"", "?tenant_id=untrusted&unknown=1"} { + h, f := vaultResourceHandler(t) + w := vaultRequest(h, "DELETE", "/v1/vaults/"+f.vault.ID+query, "") + var got map[string]any + if w.Code != 200 || json.Unmarshal(w.Body.Bytes(), &got) != nil { + t.Fatal("deletion failed", w.Code) + } + want := map[string]any{"id": f.vault.ID, "deleted": true, "object": "vault.deleted"} + if !reflect.DeepEqual(got, want) || f.tenant != f.vault.TenantID || f.id != f.vault.ID || f.calls != 1 { + t.Fatal("deletion changed authenticated scope or confirmation") + } } } func TestVaultDeletionRejectsBeforeMutation(t *testing.T) { - for _, mode := range []string{"auth", "beta", "query", "body", "null", "invalid", "zero"} { + for _, mode := range []string{"auth", "beta", "body", "null", "invalid", "zero", "zero-query"} { t.Run(mode, func(t *testing.T) { h, f := vaultResourceHandler(t) path, body, status := "/v1/vaults/"+f.vault.ID, "", 400 switch mode { case "auth": status = 401 - case "query": - path += "?tenant_id=untrusted" case "body": body = `{"token":"vault-delete-canary"}` case "null": @@ -49,6 +50,8 @@ func TestVaultDeletionRejectsBeforeMutation(t *testing.T) { path, status = "/v1/vaults/invalid", 404 case "zero": path, status = "/v1/vaults/"+uuid.Nil.String(), 404 + case "zero-query": + path, status = "/v1/vaults/"+uuid.Nil.String()+"?tenant_id=untrusted", 404 } r := httptest.NewRequest("DELETE", path, strings.NewReader(body)) if mode != "auth" { diff --git a/services/agents-api/internal/api/vaults_list.go b/services/agents-api/internal/api/vaults_list.go index 862915863..7714836c1 100644 --- a/services/agents-api/internal/api/vaults_list.go +++ b/services/agents-api/internal/api/vaults_list.go @@ -7,7 +7,7 @@ import ( ) // @Summary List Vaults -// @Description Lists project-owned Vaults independently of execution. Includes active and archived records by default. Status accepts a scalar or the SDK's status[] array; mixed encodings and repeated scalars are rejected locally. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering; exact hosted errors and concurrent-page behavior remain unverified. Archive/delete lifecycle is not implemented. +// @Description Lists project-owned Vaults independently of execution. Includes active and archived records by default. Status accepts a scalar, the SDK's status[] array or both, filtering by their union; a repeated scalar is rejected. Limits default to 20 and clamp to 1–100. Equal creation times use ID ordering; exact hosted errors and concurrent-page behavior remain unverified. Archive/delete lifecycle is not implemented. // @Tags Vaults // @Produce json // @Security BearerAuth @@ -16,7 +16,7 @@ import ( // @Param limit query integer false "Requested page size, clamped to 1–100" default(20) // @Param order query string false "Creation order; omit for descending, explicit empty values are invalid" Enums(asc,desc) default(desc) // @Param status query string false "Scalar status filter" Enums(active,archived) -// @Param status[] query []string false "Array status filter; cannot be combined with status" collectionFormat(multi) Enums(active,archived) +// @Param status[] query []string false "Array status filter; combined with status as a union" collectionFormat(multi) Enums(active,archived) // @Success 200 {object} v1.VaultList // @Failure 400,401,404,500 {object} v1.ErrorResponse // @Router /vaults [get] diff --git a/services/agents-api/internal/api/vaults_list_test.go b/services/agents-api/internal/api/vaults_list_test.go index 67e60fdc3..186c2623a 100644 --- a/services/agents-api/internal/api/vaults_list_test.go +++ b/services/agents-api/internal/api/vaults_list_test.go @@ -28,6 +28,8 @@ func TestVaultListParameters(t *testing.T) { {"?limit=-9999999999999999999999999", 1, nil}, {"?status=active", 20, []string{"active"}}, {"?status=archived", 20, []string{"archived"}}, {"?status[]=archived&status[]=active", 20, []string{"archived", "active"}}, + {"?status=archived&status[]=active", 20, []string{"archived", "active"}}, + {"?tenant_id=foreign&unknown=1&limit=2", 2, nil}, } { t.Run(tc.query, func(t *testing.T) { h, f := vaultResourceHandler(t) @@ -45,7 +47,7 @@ func TestVaultListParameters(t *testing.T) { } }) } - for _, query := range []string{"limit=", "limit=null", "limit=1.5", "limit=1&limit=2", "order=invalid", "status=", "status=deleted", "status[]=active&status[]=invalid", "status=active&status=archived", "status=active&status[]=archived", "tenant_id=foreign", "after=a&after=b"} { + for _, query := range []string{"limit=", "limit=null", "limit=1.5", "limit=1&limit=2", "order=invalid", "status=", "status=deleted", "status[]=active&status[]=invalid", "status=active&status=archived", "status=active&status[]=deleted", "after=a&after=b"} { h, f := vaultResourceHandler(t) w := vaultRequest(h, http.MethodGet, "/v1/vaults?"+query, "") if w.Code != 400 || f.calls != 0 { diff --git a/services/agents-api/internal/api/vaults_test.go b/services/agents-api/internal/api/vaults_test.go index 9b8d2e644..2bfa12193 100644 --- a/services/agents-api/internal/api/vaults_test.go +++ b/services/agents-api/internal/api/vaults_test.go @@ -106,10 +106,9 @@ func TestVaultResourceInvalidRequestsDoNotReachStore(t *testing.T) { method, path string status int }{ - {"POST", "/v1/vaults?tenant_id=foreign", 400}, {"GET", "/v1/vaults/not-a-vault", 404}, {"GET", "/v1/vaults/" + uuid.Nil.String(), 404}, - {"GET", "/v1/vaults/" + uuid.NewString() + "?tenant_id=foreign", 400}, + {"GET", "/v1/vaults/" + uuid.Nil.String() + "?tenant_id=foreign", 404}, {"POST", "/v1/agents/vaults", 404}, } { h, f := vaultResourceHandler(t) @@ -120,6 +119,30 @@ func TestVaultResourceInvalidRequestsDoNotReachStore(t *testing.T) { } } +func TestVaultResourceIgnoresUnknownQueryKeys(t *testing.T) { + for _, test := range []struct { + method string + status int + }{{"POST", 201}, {"GET", 200}} { + h, f := vaultResourceHandler(t) + path := "/v1/vaults" + if test.method == "GET" { + path += "/" + f.vault.ID + } + w := vaultRequest(h, test.method, path+"?tenant_id=foreign&unknown=1", `{}`) + if w.Code != test.status || f.calls != 1 || f.tenant != f.vault.TenantID { + t.Fatal(test.method, w.Code, f.calls, f.tenant) + } + // A missing or foreign Vault stays indistinguishable with the same query. + f.err = store.ErrNotFound + missing := vaultRequest(h, "GET", "/v1/vaults/"+uuid.NewString()+"?tenant_id=foreign", "") + plain := vaultRequest(h, "GET", "/v1/vaults/"+uuid.NewString(), "") + if missing.Code != 404 || missing.Body.String() != plain.Body.String() || f.tenant != f.vault.TenantID { + t.Fatal("query changed not-found masking", missing.Code, missing.Body.String()) + } + } +} + func TestVaultResourceUsesSharedAuthenticationAndErrors(t *testing.T) { for _, method := range []string{"POST", "GET"} { for _, test := range []struct { diff --git a/services/agents-api/internal/store/skills_list.go b/services/agents-api/internal/store/skills_list.go index 5363ad0ad..16e9c6cd6 100644 --- a/services/agents-api/internal/store/skills_list.go +++ b/services/agents-api/internal/store/skills_list.go @@ -19,12 +19,14 @@ type SkillVersionPage struct { HasMore bool } +// ListSkills and ListSkillVersions accept limit 0: the page is empty and HasMore +// reports whether any resource follows the cursor. func (s *Store) ListSkills(ctx context.Context, tenantID, after string, limit int, ascending bool) (SkillPage, error) { tenant, err := parseID(tenantID) if err != nil { return SkillPage{}, err } - if limit < 1 || limit > 100 { + if limit < 0 || limit > 100 { return SkillPage{}, ErrInvalidInput } params := sqlc.ListSkillsParams{TenantID: tenant, PageLimit: int32(limit + 1), Ascending: ascending, AfterID: pgtype.UUID{Valid: true}} @@ -55,7 +57,7 @@ func (s *Store) ListSkillVersions(ctx context.Context, tenantID, skillID, after if err != nil { return SkillVersionPage{}, err } - if limit < 1 || limit > 100 { + if limit < 0 || limit > 100 { return SkillVersionPage{}, ErrInvalidInput } if _, err := s.GetSkill(ctx, tenantID, skillID); err != nil { diff --git a/services/agents-api/internal/store/skills_list_test.go b/services/agents-api/internal/store/skills_list_test.go new file mode 100644 index 000000000..af3c39017 --- /dev/null +++ b/services/agents-api/internal/store/skills_list_test.go @@ -0,0 +1,66 @@ +package store + +import ( + "bytes" + "errors" + "testing" + + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/credentialcrypto" + "github.com/google/uuid" +) + +func TestSkillListsAcceptLimitZero(t *testing.T) { + _, pool := testStore(t) + cipher, err := credentialcrypto.New(bytes.Repeat([]byte{43}, 32)) + if err != nil { + t.Fatal(err) + } + s := NewWithCredentialCipher(pool, cipher) + tenant, foreign := uuid.NewString(), uuid.NewString() + archive := skillArchive(t, "limit-zero") + created, err := s.CreateSkill(t.Context(), tenant, archive) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = s.DeleteSkill(t.Context(), tenant, created.ID) }) + if _, err = s.CreateSkillVersion(t.Context(), tenant, created.ID, archive, false); err != nil { + t.Fatal(err) + } + + // A zero page is empty; HasMore reports whether a resource follows the cursor. + for _, test := range []struct { + tenant, after string + hasMore bool + }{{tenant, "", true}, {tenant, created.ID, false}, {foreign, "", false}} { + page, err := s.ListSkills(t.Context(), test.tenant, test.after, 0, true) + if err != nil || len(page.Skills) != 0 || page.HasMore != test.hasMore { + t.Fatalf("zero Skill page after %q: %+v %v", test.after, page, err) + } + } + versions, err := s.ListSkillVersions(t.Context(), tenant, created.ID, "", 100, true) + if err != nil || len(versions.Versions) != 2 { + t.Fatal("versions", versions, err) + } + for after, hasMore := range map[string]bool{"": true, versions.Versions[0].ID: true, versions.Versions[1].ID: false} { + page, err := s.ListSkillVersions(t.Context(), tenant, created.ID, after, 0, true) + if err != nil || len(page.Versions) != 0 || page.HasMore != hasMore { + t.Fatalf("zero version page after %q: %+v %v", after, page, err) + } + } + + // Foreign cursors and parents stay indistinguishable from missing ones. + if _, err = s.ListSkills(t.Context(), foreign, created.ID, 0, true); !errors.Is(err, ErrNotFound) { + t.Fatal("foreign cursor", err) + } + if _, err = s.ListSkillVersions(t.Context(), foreign, created.ID, "", 0, true); !errors.Is(err, ErrNotFound) { + t.Fatal("foreign versions", err) + } + for _, limit := range []int{-1, 101} { + if _, err = s.ListSkills(t.Context(), tenant, "", limit, true); !errors.Is(err, ErrInvalidInput) { + t.Fatal("Skill limit", limit, err) + } + if _, err = s.ListSkillVersions(t.Context(), tenant, created.ID, "", limit, true); !errors.Is(err, ErrInvalidInput) { + t.Fatal("version limit", limit, err) + } + } +} diff --git a/services/agents-api/tests/official_agent_delete.py b/services/agents-api/tests/official_agent_delete.py index 4a566acc9..47b379e3b 100644 --- a/services/agents-api/tests/official_agent_delete.py +++ b/services/agents-api/tests/official_agent_delete.py @@ -37,12 +37,13 @@ def main(): assert http.delete(base + "/v1/agents/" + foreign_agent.id, headers=headers).status_code == 404 assert http.delete(endpoint, headers={"Authorization": "Bearer " + token}).status_code == 400 assert http.delete(endpoint, headers={"OpenAI-Beta": "agents=v1"}).status_code == 401 - assert http.delete(endpoint + "?cascade=true", headers=headers).status_code == 400 + assert http.delete(endpoint + "?cascade=true", headers=headers | {"Authorization": "Bearer " + foreign}).status_code == 404 assert http.request("DELETE", endpoint, headers=headers, json={"cascade": True}).status_code == 400 assert agents.retrieve(original.id) == original assert sessions.retrieve(current.id) == current # The result carries the stored canonical ID, independently of path spelling. - raw = agents.with_raw_response.delete(original.id.upper()) + # cascade is not a pinned parameter: it is ignored and Sessions remain. + raw = agents.with_raw_response.delete(original.id.upper(), extra_query={"cascade": "true"}) expected = {"id": original.id, "object": "agent.deleted", "deleted": True} assert raw.status_code == 200 and raw.http_response.json() == expected assert raw.parse().to_dict() == expected diff --git a/services/agents-api/tests/official_agent_list.py b/services/agents-api/tests/official_agent_list.py index 032f11100..dcda38b9d 100644 --- a/services/agents-api/tests/official_agent_list.py +++ b/services/agents-api/tests/official_agent_list.py @@ -41,12 +41,14 @@ def verify_agent_list(client, other, invalid, saved, expect_error): default = raw.get(url, headers=headers).json() assert len(default["data"]) == min(20, len(saved)) and default["has_more"] == (len(saved) > 20) assert [agent["id"] for agent in default["data"]] == [agent.id for agent in desc[:20]] - for params in ({"limit": "0"}, {"limit": "-1"}, {"limit": "1.5"}, {"limit": "null"}, + for params in ({"limit": "-1"}, {"limit": "1.5"}, {"limit": "null"}, {"limit": ""}, {"limit": str(2**63)}, {"order": "newest"}, {"after": "invalid-id"}, - {"tenant_id": "other"}, [("limit", "1"), ("limit", "2")]): + [("limit", "1"), ("limit", "2")]): response = raw.get(url, headers=headers, params=params) assert response.status_code == 400, (params, response.status_code) assert response.json()["error"]["type"] == "invalid_request_error" + zero = raw.get(url, headers=headers, params={"limit": "0", "tenant_id": "other"}).json() + assert [agent["id"] for agent in zero["data"]] == [desc[0].id] and zero["has_more"] is True assert raw.get(url).status_code == 401 print("Agent list: fixed SDK auto-pagination/raw HTTP, order/cursors, snapshots, isolation and local limit/envelope behavior passed; exact upstream defaults/caps/errors remain unverified.") return [agent.id for agent in asc] diff --git a/services/agents-api/tests/official_agent_update.py b/services/agents-api/tests/official_agent_update.py index fa4b109b0..e1958b5df 100644 --- a/services/agents-api/tests/official_agent_update.py +++ b/services/agents-api/tests/official_agent_update.py @@ -58,7 +58,10 @@ def main(): response = http.post(base + "/v1/agents/" + target, headers=headers | {"Authorization": "Bearer " + foreign}, json={"name": "foreign"}) assert response.status_code == 404 - assert http.post(endpoint + "?unknown=1", headers=headers, json={}).status_code == 400 + # Unknown query keys are ignored; the body and resource scope still decide. + assert http.post(endpoint + "?unknown=1", headers=headers, json={"updated_at": 1}).status_code == 400 + assert http.post(endpoint + "?unknown=1", headers=headers | {"Authorization": "Bearer " + foreign}, + json={"name": "foreign"}).status_code == 404 assert http.post(endpoint, headers={"Authorization": "Bearer " + token}, json={}).status_code == 400 assert http.post(endpoint, headers={"OpenAI-Beta": "agents=v1"}, json={}).status_code == 401 assert agents.retrieve(original.id) == updated diff --git a/services/agents-api/tests/official_agents.py b/services/agents-api/tests/official_agents.py index cce0320ec..f442074cd 100644 --- a/services/agents-api/tests/official_agents.py +++ b/services/agents-api/tests/official_agents.py @@ -103,8 +103,11 @@ def verify_agents(client, other, invalid, expect_error): for content in ("{}", "null", "[]", '{"model":"x"} {}'): assert raw.post(base, headers=headers, content=content).status_code == 400 assert raw.post(base, headers=headers, content='{"model":"' + "x" * (1024 * 1024) + '"}').status_code == 413 - assert raw.post(base, headers=headers, params={"tenant_id": "other"}, json={"model": "x"}).status_code == 400 - assert raw.get(base + "/" + saved[0].id, headers=headers, params={"tenant_id": "other"}).status_code == 400 + # tenant_id is an ignored query key; it never selects another tenant. + assert raw.post(base, headers=headers, params={"tenant_id": "other"}, content="{}").status_code == 400 + plain = raw.get(base + "/" + saved[0].id, headers=headers) + scoped = raw.get(base + "/" + saved[0].id, headers=headers, params={"tenant_id": "other"}) + assert plain.status_code == scoped.status_code == 200 and scoped.json() == plain.json() for resource_id in (str(uuid.uuid4()), "unrecognized-agent", str(uuid.UUID(int=0))): expect_error(NotFoundError, lambda: agents.retrieve(resource_id)) expect_error(NotFoundError, lambda: other.beta.agents.retrieve(saved[0].id)) diff --git a/services/agents-api/tests/official_client.py b/services/agents-api/tests/official_client.py index e566602a2..a8dddbf16 100644 --- a/services/agents-api/tests/official_client.py +++ b/services/agents-api/tests/official_client.py @@ -204,7 +204,7 @@ def expect_error(error, operation): assert list(sessions.list(agent_id="unknown-agent")) == [] assert list(sessions.list(agent_id=first.agent.id)) == [first] assert list(b.beta.agents.sessions.list(agent_id=first.agent.id)) == [] - expect_error(BadRequestError, lambda: sessions.list(limit=0)) + assert sessions.list(limit=0).data == sessions.list(limit=1).data assert {item.id for item in sessions.list()} == expected metadata = {str(i): "🧪" * 512 for i in range(16)} large = sessions.create(**spec, metadata=metadata) diff --git a/services/agents-api/tests/official_credential_delete.py b/services/agents-api/tests/official_credential_delete.py index c80fdcc88..89edd07d0 100644 --- a/services/agents-api/tests/official_credential_delete.py +++ b/services/agents-api/tests/official_credential_delete.py @@ -26,15 +26,17 @@ def verify_credential_deletion(client, other, invalid, peer, canary, expect_erro assert raw.delete(url).status_code == 401 response = raw.delete(url, headers={"Authorization": headers["Authorization"]}) assert response.status_code == 400 and response.json()["error"]["code"] == "invalid_beta" - for kwargs in [{"params": {"include": "token"}}, {"content": b"{}"}]: - assert raw.request("DELETE", url, headers=headers, **kwargs).status_code == 400 + # The body rejects; the unknown include key is ignored and never exposes the token. + for kwargs in [{"content": b"{}"}, {"params": {"include": "token"}, "content": b"{}"}]: + response = raw.request("DELETE", url, headers=headers, **kwargs) + assert response.status_code == 400 and canary not in response.text assert credentials.retrieve(target.id, vault_id=vault.id) == target response = peer.beta.agents.vaults.credentials.with_raw_response.delete(target.id, vault_id=vault.id) expected = {"id": target.id, "deleted": True, "object": "vault.credential.deleted"} assert response.status_code == 200 and response.parse().to_dict() == expected assert response.http_response.json() == expected and canary not in response.http_response.text assert response.headers["cache-control"] == "no-store" - response = raw.delete(endpoint + "/" + values[1].id, headers=headers) + response = raw.delete(endpoint + "/" + values[1].id, headers=headers, params={"include": "token"}) assert response.status_code == 200 and response.json() == {**expected, "id": values[1].id} assert canary not in response.text for value in values[:2]: diff --git a/services/agents-api/tests/official_credential_list.py b/services/agents-api/tests/official_credential_list.py index a1485c260..5dd6f2d4d 100644 --- a/services/agents-api/tests/official_credential_list.py +++ b/services/agents-api/tests/official_credential_list.py @@ -113,7 +113,9 @@ def safe_error(response, status): vault.id, descending[:size], True, canary) for params, values in (({"status": "active"}, list(reversed(active))), ([("status[]", "archived")], list(reversed(archived))), - ([("status[]", "active"), ("status[]", "archived")], descending)): + ([("status[]", "active"), ("status[]", "archived")], descending), + ([("status", "archived"), ("status[]", "active")], descending), + ({"include": "token", "tenant_id": "foreign"}, descending)): verify_page(raw.get(endpoint, headers=headers, params=params), vault.id, values[:20], True, canary) for order, values in (("asc", expected), ("desc", descending)): params = {"order": order, "limit": "100"} @@ -138,9 +140,8 @@ def safe_error(response, status): expect_error(NotFoundError, lambda: credentials.list(owner, after=cursor)) invalid_queries = [ {"after": "invalid-credential"}, {"status": "unknown"}, {"limit": "null"}, - {"order": "invalid"}, {"include": "token"}, {"tenant_id": "foreign"}, - [("status", "active"), ("status", "archived")], - [("status", "active"), ("status[]", "archived")], + {"order": "invalid"}, [("status", "active"), ("status", "archived")], + [("status", "active"), ("status[]", "unknown")], ] for params in invalid_queries: safe_error(raw.get(endpoint, headers=headers, params=params), 400) diff --git a/services/agents-api/tests/official_credential_rotation.py b/services/agents-api/tests/official_credential_rotation.py index b44705ae4..84e4eebd0 100644 --- a/services/agents-api/tests/official_credential_rotation.py +++ b/services/agents-api/tests/official_credential_rotation.py @@ -100,7 +100,9 @@ def metadata(response, previous): for scope in ({"OpenAI-Project": "other-project"}, {"OpenAI-Organization": "other-organization"}): expect_error(AuthenticationError, lambda: credentials.update( original.id, vault_id=vault.id, **replacement, extra_headers=scope)) - safe(raw.post(endpoint, headers=headers, params={"include": "token"}, json=replacement), 400) + safe(raw.post(endpoint, headers=headers, params={"include": "token"}, + json={"auth": {"type": "static_bearer", "token": ""}}), 400) + assert safe(raw.get(endpoint, headers=headers, params={"include": "token"}), 200) == current.to_dict() assert safe(raw.get(endpoint, headers=headers), 200) == current.to_dict() assert credentials.retrieve(original.id, vault_id=vault.id) == current diff --git a/services/agents-api/tests/official_credentials.py b/services/agents-api/tests/official_credentials.py index 96e70fda6..2ccc6196c 100644 --- a/services/agents-api/tests/official_credentials.py +++ b/services/agents-api/tests/official_credentials.py @@ -112,8 +112,10 @@ def safe_body(response, status): safe_body(raw.request(method, url, **body), 401) response = raw.request(method, url, headers={"Authorization": headers["Authorization"]}, **body) assert safe_body(response, 400)["error"]["code"] == "invalid_beta" - safe_body(raw.post(endpoint, headers=headers, params={"tenant_id": "other"}, json=request), 400) - safe_body(raw.get(endpoint + "/" + saved[0].id, headers=headers, params={"include": "token"}), 400) + # Unknown query keys are ignored: they neither select a tenant nor expose tokens. + safe_body(raw.post(endpoint, headers=headers, params={"tenant_id": "other"}, json={"name": "x"}), 400) + plain = safe_body(raw.get(endpoint + "/" + saved[0].id, headers=headers), 200) + assert safe_body(raw.get(endpoint + "/" + saved[0].id, headers=headers, params={"include": "token"}), 200) == plain assert [list(api.beta.agents.sessions.list()) for api in (client, other)] == sessions_before assert [client.beta.agents.vaults.retrieve(value.id) for value in vaults] == vaults diff --git a/services/agents-api/tests/official_environment_retrieve.py b/services/agents-api/tests/official_environment_retrieve.py index 3d38286a7..198581db5 100644 --- a/services/agents-api/tests/official_environment_retrieve.py +++ b/services/agents-api/tests/official_environment_retrieve.py @@ -95,7 +95,12 @@ def rejected(url, status, code, request_headers=headers, method="GET"): if beta is not None: request_headers["OpenAI-Beta"] = beta rejected(endpoint, 400, "invalid_beta", request_headers) - rejected(endpoint + "?include=files", 400, "unsupported_parameter") + # include is not a pinned retrieval parameter; unknown keys are ignored. + for query in ("?include=files", "?tenant_id=" + result["foreign_environment_id"]): + response = http.get(endpoint + query, headers=headers) + assert response.status_code == 200 and response.json() == expected + rejected(base + "/v1/agents/environments/" + result["foreign_environment_id"] + query, 404, "not_found_error") + assert api.beta.agents.environments.retrieve(environment_id, extra_query={"include": "files"}).to_dict() == expected for method in ("POST", "PATCH", "DELETE"): rejected(endpoint, 405, "unsupported_operation", method=method) assert api.beta.agents.environments.retrieve(environment_id).to_dict() == expected diff --git a/services/agents-api/tests/official_environment_templates.py b/services/agents-api/tests/official_environment_templates.py index 069b38ce5..16d2552b1 100644 --- a/services/agents-api/tests/official_environment_templates.py +++ b/services/agents-api/tests/official_environment_templates.py @@ -63,8 +63,13 @@ def verify_environment_templates(client, foreign, http): assert [v.id for v in other.list()] == [foreign_template.id] finally: other.delete(foreign_template.id) - for query in ['limit=0', 'limit=101', 'limit=bad', 'order=wrong', 'limit=1&limit=2', 'unknown=1']: - assert http.get(base + '?' + query, headers=headers).status_code == 400 + for query in ['limit=bad', 'limit=-1', 'order=wrong', 'limit=1&limit=2']: + response = http.get(base + '?' + query, headers=headers) + assert response.status_code == 400 and response.json()['error']['code'] == 'invalid_request_error' + everything = http.get(base + '?limit=100', headers=headers).json() + assert http.get(base + '?limit=101&unknown=1', headers=headers).json() == everything + first = http.get(base + '?limit=0', headers=headers).json() + assert [value['id'] for value in first['data']] == [everything['data'][0]['id']] and first['has_more'] == (len(everything['data']) > 1) canary = 'template-private-' + uuid.uuid4().hex for body in [{'env': {'PATH': canary}}, {'setup_commands': [{'command': canary, 'cwd': 'relative'}]}, {'files': [{'type': 'inline', 'path': '/workspace/a', 'data': canary}]}, diff --git a/services/agents-api/tests/official_items.py b/services/agents-api/tests/official_items.py index f8117c36f..8d6963f7b 100644 --- a/services/agents-api/tests/official_items.py +++ b/services/agents-api/tests/official_items.py @@ -15,7 +15,8 @@ def verify_items(a, b, invalid, session, peer_session, turns, expect_error): expect_error(NotFoundError, lambda: b.beta.agents.sessions.items.list(session)) expect_error(NotFoundError, lambda: items.list(peer_session, after=recovered[0].id)) expect_error(AuthenticationError, lambda: invalid.beta.agents.sessions.items.list(session)) - expect_error(BadRequestError, lambda: items.list(session, limit=101)) + assert items.list(session, limit=101).data == items.list(session, limit=100).data + assert items.list(session, limit=0, order="asc").data == [recovered[0]] assert "PRIVATE" not in repr(recovered) and "SECRET" not in repr(recovered) for i, turn in enumerate(turns): group = [item for item in recovered if item.turn_id == turn] diff --git a/services/agents-api/tests/official_list_query.py b/services/agents-api/tests/official_list_query.py index b862cba1f..291be4d49 100644 --- a/services/agents-api/tests/official_list_query.py +++ b/services/agents-api/tests/official_list_query.py @@ -2,6 +2,7 @@ Owned fixtures use real Worker admission with dispatch paused. No native executor or model runs, and initial Turn/Item history remains visible throughout the test. +The tolerance checks follow contracts/agents-api/list-query-semantics.md. """ import importlib.metadata @@ -15,6 +16,167 @@ from openai import APIStatusError, DefaultHttpxClient, OpenAI +BETA_DUPLICATE = "Failed to deserialize query string: duplicate field `{}`" +BETA_DIGIT = "Failed to deserialize query string: limit: invalid digit found in string" +SKILLS_DUPLICATE = ("Duplicate parameter: '{0}'. You provided multiple values for this parameter, whereas only one is " + "allowed. If you are trying to provide a list of values, use the array syntax instead e.g. '{0}[]='.") +# Limit policy per family: accepted limit -> page size, or the rejected error fields. +LIMITS = { + "clamp": {"0": 1, "101": None, "-1": ("invalid_request_error", None, BETA_DIGIT), "abc": ("invalid_request_error", None, BETA_DIGIT)}, + "strict": {"0": ("invalid_request_error", None, "limit must be between 1 and 100"), "101": ("invalid_request_error", None, "limit must be between 1 and 100"), + "-1": ("invalid_request_error", None, BETA_DIGIT), "abc": ("invalid_request_error", None, BETA_DIGIT)}, + "vault": {"0": 1, "-1": 1, "101": None, "abc": ("invalid_request_error", None, BETA_DIGIT)}, + "skills": {"0": 0, "101": ("integer_above_max_value", "limit", "Invalid 'limit': integer above maximum value. Expected a value <= 100, but got 101 instead."), + "-1": ("integer_below_min_value", "limit", "Invalid 'limit': integer below minimum value. Expected a value >= 0, but got -1 instead.")}, + "files": {"0": (None, None, "limit must be between 1 and 10000."), "10001": (None, None, "limit must be between 1 and 10000.")}, +} + + +def verify_tolerance(raw, url, headers, foreign_headers, name, listing, owned, policy, nested, private): + """Checks unknown/repeated keys and limit policy for one family as tenant A and B.""" + checks = 0 + + def get(params, owner=True): + response = raw.get(url, headers=headers if owner else foreign_headers, params=params) + # Errors and tenant B responses never disclose owned resources or secrets. + if not owner or response.status_code != 200: + assert all(value not in response.text for value in private), (name, params) + return response + + def foreign_matches(params, plain): + # Tenant B sees its own view: an empty top-level page or the same parent 404. + response = get(params, owner=False) + expected = get(plain, owner=False) + assert response.status_code == expected.status_code == (404 if nested else 200), (name, params) + assert response.json() == expected.json(), (name, params) + if not nested: + assert response.json()["data"] == [] and response.json()["has_more"] is False, (name, params) + + def rejected(params, code, param, message): + nonlocal checks + response = get(params) + assert response.status_code == 400, (name, params, response.status_code) + error = response.json()["error"] + assert (error["type"], error["code"], error["param"], error["message"]) == ("invalid_request_error", code, param, message), (name, params, error) + # Query parsing precedes resource lookup, so tenant B receives the same error. + foreign = get(params, owner=False) + assert foreign.status_code == 400 and foreign.json() == response.json(), (name, params) + checks += 1 + + # A1: unknown keys are ignored for both tenants and through the SDK. + plain = {"order": "asc", "limit": 1} + baseline = get(plain).json() + for extra in ({"query_probe": "1"}, {"tenant_id": "foreign", "limit[]": "5"}, [("query_probe", "1"), ("query_probe", "2")]): + params = list(plain.items()) + (list(extra.items()) if isinstance(extra, dict) else extra) + assert get(params).json() == baseline, (name, extra) + foreign_matches(params, plain) + checks += 1 + assert [value.id for value in listing(order="asc", limit=1, extra_query={"query_probe": "1"}).data] == owned[:1], name + + # B1-B3: a repeated supported key rejects with the family's observed fields. + repeated = [("limit", "1"), ("limit", "2")] + if policy == "skills": + rejected(repeated, "duplicate_parameter", "limit", SKILLS_DUPLICATE.format("limit")) + elif policy == "files": + rejected(repeated, "unsupported_parameter", None, "Supported list parameters are after, limit, order and purpose, each supplied once.") + else: + rejected(repeated, "invalid_request_error", None, BETA_DUPLICATE.format("limit")) + rejected([("order", "asc"), ("order", "asc")], "invalid_request_error", None, BETA_DUPLICATE.format("order")) + + # C1-C7: limit bounds. + for limit, outcome in LIMITS[policy].items(): + params = {"order": "asc", "limit": limit} + if isinstance(outcome, tuple): + rejected(params, *outcome) + continue + size = len(owned) if outcome is None else outcome + body = get(params).json() + assert [value["id"] for value in body["data"]] == owned[:size], (name, limit) + assert body["has_more"] is (size < len(owned)), (name, limit) + assert (body["first_id"], body["last_id"]) == ((owned[0], owned[size - 1]) if size else (None, None)), (name, limit) + foreign_matches(params, {"order": "asc"}) + checks += 1 + if policy == "skills": + # A zero page reports only whether a resource follows the cursor. + tail = get({"order": "asc", "limit": 0, "after": owned[-1]}).json() + assert tail == {"object": "list", "data": [], "has_more": False, "first_id": None, "last_id": None}, name + assert listing(limit=0).data == [] and listing(limit=0).has_more is True, name + checks += 1 + return checks + +def verify_resource_queries(raw, base, token, foreign, client, owned, session_id, vault_id): + """A2: single-resource routes ignore unknown query keys for both tenants.""" + probe = "?tenant_id=foreign&include=files&cascade=true&query_probe=1&query_probe=2" + private = [value for values in owned.values() for value in values] + beta = {"OpenAI-Beta": "agents=v1"} + checks = 0 + + def call(method, path, key, query="", headers=beta, **body): + return raw.request(method, base + "/v1" + path + query, headers={**headers, "Authorization": "Bearer " + key}, **body) + + def same(method, path, status, target, headers=beta, **body): + nonlocal checks + for key, expected in ((token, status), (foreign, 404)): + plain, probed = call(method, path, key, "", headers, **body), call(method, path, key, probe, headers, **body) + assert plain.status_code == probed.status_code == expected, (method, path, plain.status_code, probed.status_code) + assert plain.content == probed.content, (method, path) + # A foreign resource stays indistinguishable from a missing one of the same shape. + assert all(value not in probed.text for value in private), path + absent = target[:-1] + ("1" if target.endswith("0") else "0") + missing = call(method, path.replace(target, absent), token, probe, headers, **body) + assert missing.status_code == 404 and missing.json() == probed.json(), (path, missing.text, probed.text) + checks += 1 + + for prefix, target in (("/agents/", owned["agents"][0]), ("/agents/environments/templates/", owned["templates"][0]), + ("/agents/sessions/", session_id), (f"/agents/sessions/{session_id}/turns/", owned["turns"][0]), + ("/vaults/", vault_id), (f"/vaults/{vault_id}/credentials/", owned["credentials"][0])): + same("GET", prefix + target, 200, target) + for prefix, target in (("/files/", owned["files"][0]), ("/skills/", owned["skills"][0])): + same("GET", prefix + target, 200, target, headers={}) + # An empty event batch is an authorized no-op; the unknown key changes nothing. + same("POST", f"/agents/sessions/{session_id}/events", 202, session_id, json={"events": []}) + with raw.stream("GET", base + f"/v1/agents/sessions/{session_id}/events" + probe, + headers={**beta, "Authorization": "Bearer " + token}) as stream: + assert stream.status_code == 200 and stream.headers["content-type"].startswith("text/event-stream") + checks += 1 + + # Writes with unknown keys apply normally for the owner and stay 404 for tenant B. + templates, vaults = client.beta.agents.environments.templates, client.beta.agents.vaults + template, vault = templates.create(name="Query Probe Template"), vaults.create(name="Query Probe Vault") + for method, path, body in (("POST", "/agents/environments/templates/" + template.id, {"json": {"name": "Foreign"}}), + ("DELETE", "/agents/environments/templates/" + template.id, {}), + ("DELETE", "/vaults/" + vault.id, {})): + assert call(method, path, foreign, probe, **body).status_code == 404, (method, path) + assert templates.retrieve(template.id) == template and vaults.retrieve(vault.id) == vault + renamed = call("POST", "/agents/environments/templates/" + template.id, token, probe, json={"name": "Renamed Probe"}) + assert renamed.status_code == 200 and renamed.json()["name"] == "Renamed Probe" + for path, kind in (("/agents/environments/templates/" + template.id, "agent.environment.template.deleted"), ("/vaults/" + vault.id, "vault.deleted")): + deleted = call("DELETE", path, token, probe) + assert deleted.status_code == 200 and deleted.json() == {"id": path.rsplit("/", 1)[1], "object": kind, "deleted": True}, path + assert call("GET", path, token).status_code == 404, path + + # Uploads: query keys are not form fields and never select a tenant. + upload = client.files.create(file=("query-probe.txt", b"probe"), purpose="user_data", + extra_query={"purpose": "assistants", "tenant_id": "foreign"}) + assert upload.purpose == "user_data" and call("GET", "/files/" + upload.id, foreign, probe, headers={}).status_code == 404 + rejected = raw.post(base + "/v1/files?purpose=user_data", headers={"Authorization": "Bearer " + token}, + files={"file": ("query-probe.txt", b"probe")}, data={"purpose": "assistants"}) + assert rejected.status_code == 400 + client.files.delete(upload.id) + manifest = b"---\nname: query-probe\ndescription: Query probe.\n---\nProbe.\n" + bundle = [("query-probe/SKILL.md", manifest, "text/markdown")] + skill = client.skills.create(files=bundle, extra_query={"tenant_id": "foreign", "default": "true"}) + target = "/skills/" + skill.id + "/versions" + multipart = {"files": [("files[]", ("query-probe/SKILL.md", manifest, "text/markdown"))]} + denied = call("POST", target, foreign, probe, headers={}, **multipart) + absent = skill.id[:-1] + ("1" if skill.id.endswith("0") else "0") + missing = call("POST", "/skills/" + absent + "/versions", token, probe, headers={}, **multipart) + assert denied.status_code == missing.status_code == 404 and denied.json() == missing.json() + version = client.skills.versions.create(skill.id, files=bundle, extra_query={"default": "true", "tenant_id": "foreign"}) + assert version.version == "2" and client.skills.retrieve(skill.id).default_version == "1" + client.skills.delete(skill.id) + return checks + 5 + def main(): base, token, foreign = sys.argv[1:] pin = json.loads((Path(__file__).resolve().parents[3] / "contracts/agents-api/upstream.json").read_text()) @@ -89,6 +251,9 @@ def main(): ("files", "/files", client.files.list, False, False), ("skills", "/skills", client.skills.list, False, False), ] + policies = {"agents": "clamp", "templates": "clamp", "sessions": "clamp", "items": "clamp", "turns": "strict", + "vaults": "vault", "credentials": "vault", "files": "files", "skills": "skills"} + tolerated = 0 state_before = {name: [value.to_dict() for value in listing(order="asc", limit=1)] for name, _, listing, _, _ in families} rejected = 0 for name, path, listing, beta, nested in families: @@ -138,6 +303,26 @@ def main(): else: assert foreign_page.status_code == 200 and foreign_page.json()["data"] == [], name assert secret not in foreign_page.text and all(value not in foreign_page.text for value in owned[name]), name + private = [secret] + [value for values in owned.values() for value in values] + foreign_headers = {**headers, "Authorization": "Bearer " + foreign} + tolerated += verify_tolerance(raw, url, headers, foreign_headers, name, listing, owned[name], policies[name], nested, private) + if name in {"vaults", "credentials"}: + # D1: a scalar status and status[] filter by their union. + union = raw.get(url, headers=headers, params=[("order", "asc"), ("status", "active"), ("status[]", "archived")]) + assert union.status_code == 200 and union.json() == raw.get(url, headers=headers, params={"order": "asc"}).json(), name + assert [value.id for value in listing(order="asc", status="active", extra_query={"status[]": "archived"})] == owned[name], name + duplicate = raw.get(url, headers=headers, params=[("status", "active"), ("status", "active")]) + assert duplicate.status_code == 400 and duplicate.json()["error"]["message"] == BETA_DUPLICATE.format("status"), name + assert raw.get(url, headers=foreign_headers, params=[("status", "active"), ("status", "active")]).json() == duplicate.json(), name + tolerated += 2 + if name == "files": + # D2: an explicit empty purpose is the same as omission. + for query in ({"purpose": ""}, {"purpose": "", "query_probe": "1"}): + response = raw.get(url, headers=headers, params={"order": "asc", **query}) + assert response.status_code == 200 and [value["id"] for value in response.json()["data"]] == owned[name], name + foreign_page = raw.get(url, headers=foreign_headers, params={"order": "asc", **query}) + assert foreign_page.status_code == 200 and foreign_page.json()["data"] == [], name + tolerated += 1 if name in {"vaults", "credentials"}: for query in ({"status": "query-invalid"}, {"status[]": "query-invalid"}): response = raw.get(url, headers=headers, params=query) @@ -155,7 +340,8 @@ def main(): assert {name: [value.to_dict() for value in listing(order="asc", limit=1)] for name, _, listing, _, _ in families} == state_before assert list(sessions.turns.list(session_id, order="asc")) == turns assert list(sessions.items.list(session_id, order="asc")) == items - print(json.dumps({"result": "passed", "families": len(families), "sdk_and_raw_rejections": rejected, + resources = verify_resource_queries(raw, base, token, foreign, client, owned, session_id, vault_id) + print(json.dumps({"single_resource_checks": resources, "result": "passed", "families": len(families), "sdk_and_raw_rejections": rejected, "tolerance_checks": tolerated, "sdk_empty_order_omitted": len(families), "retained_turns": len(turns), "retained_items": len(items), "postgres": True, "worker_admission": True, "native_model_execution": False})) diff --git a/services/agents-api/tests/official_session_agent_filter.py b/services/agents-api/tests/official_session_agent_filter.py index 44db9eea4..8afb3a8ef 100644 --- a/services/agents-api/tests/official_session_agent_filter.py +++ b/services/agents-api/tests/official_session_agent_filter.py @@ -62,13 +62,18 @@ def main(): for value in ("", "unknown", root.id + " ", "' OR true --"): reply = http.get(endpoint, headers=headers, params={"agent_id": value}) assert reply.status_code == 200 and reply.json() == {"object": "list", "data": [], "has_more": False, "first_id": None, "last_id": None} - for query in ([("agent_id", root.id), ("agent_id", peer.id)], {"agent_id": root.id, "tenant_id": "other"}): - assert http.get(endpoint, headers=headers, params=query).status_code == 400 + repeated = http.get(endpoint, headers=headers, params=[("agent_id", root.id), ("agent_id", peer.id)]) + assert repeated.status_code == 400 and repeated.json()["error"]["message"] == "Failed to deserialize query string: duplicate field `agent_id`" + filtered = http.get(endpoint, headers=headers, params={"agent_id": root.id}) + assert http.get(endpoint, headers=headers, params={"agent_id": root.id, "tenant_id": "other"}).json() == filtered.json() assert http.get(endpoint, headers=headers, params={"agent_id": root.id, "after": foreign_session.id}).status_code == 404 assert http.get(endpoint, headers={"Authorization": "Bearer " + token}, params={"agent_id": root.id}).status_code == 400 assert http.get(endpoint, headers={"OpenAI-Beta": "agents=v1"}, params={"agent_id": root.id}).status_code == 401 + # agent_id is unknown to the other lists and ignored there. for path in ("/v1/agents", "/v1/agents/sessions/" + inline.id + "/items", "/v1/agents/sessions/" + inline.id + "/turns"): - assert http.get(base + path, headers=headers, params={"agent_id": root.id}).status_code == 400 + unfiltered = http.get(base + path, headers=headers) + response = http.get(base + path, headers=headers, params={"agent_id": root.id}) + assert unfiltered.status_code == response.status_code == 200 and response.json() == unfiltered.json() print("Session Agent filter: SDK/raw HTTP, saved/inline IDs, both pagination orders, tenant isolation, source update/deletion, reconnect and unfiltered behavior passed.") diff --git a/services/agents-api/tests/official_session_delete.py b/services/agents-api/tests/official_session_delete.py index 1f008f869..de3d9e945 100644 --- a/services/agents-api/tests/official_session_delete.py +++ b/services/agents-api/tests/official_session_delete.py @@ -49,14 +49,14 @@ def client(url, key): rejected(404, lambda: other.delete(session.id)) assert http.delete(endpoint, headers={"Authorization": "Bearer " + token}).status_code == 400 assert http.delete(endpoint, headers={"OpenAI-Beta": "agents=v1"}).status_code == 401 - assert http.delete(endpoint + "?cascade=true", headers=headers).status_code == 400 + assert http.delete(endpoint + "?cascade=true", headers=headers | {"Authorization": "Bearer " + foreign}).status_code == 404 for body in ("null", "{}"): assert http.request("DELETE", endpoint, headers=headers, content=body).status_code == 400 assert sessions.retrieve(session.id).id == session.id # Existing live streams close on public removal without a fabricated event. with http.stream("GET", endpoint + "/events", headers=headers) as stream: assert stream.status_code == 200 - raw = sessions.with_raw_response.delete(session.id.upper()) + raw = sessions.with_raw_response.delete(session.id.upper(), extra_query={"cascade": "true"}) expected = {"id": session.id, "object": "agent.session.deleted", "deleted": True} assert raw.status_code == 200 and raw.http_response.json() == expected assert raw.parse().to_dict() == expected diff --git a/services/agents-api/tests/official_session_metadata.py b/services/agents-api/tests/official_session_metadata.py index d1133901e..5ebd66c3d 100644 --- a/services/agents-api/tests/official_session_metadata.py +++ b/services/agents-api/tests/official_session_metadata.py @@ -69,8 +69,11 @@ def assert_metadata(session, expected): response = raw.post(url, headers=headers, content=oversized) assert response.status_code == 413 and response.json()["error"]["code"] == "request_too_large" assert raw.patch(url, headers=headers, json={"metadata": {}}).status_code == 405 + empty = raw.post(url, headers=headers, json={}) + assert empty.status_code == 400 for path in [url + "?tenant_id=other", url + "?unsupported=1"]: - assert raw.post(path, headers=headers, json={}).status_code == 400 + response = raw.post(path, headers=headers, json={}) + assert response.status_code == 400 and response.json() == empty.json() for target in [other, invalid]: expected_error = AuthenticationError if target is invalid else NotFoundError for fields in [{"metadata": None}, {"metadata": {"tenant_id": "untrusted"}}]: diff --git a/services/agents-api/tests/official_source_file_list.py b/services/agents-api/tests/official_source_file_list.py index 240a175e9..3a5a9a998 100644 --- a/services/agents-api/tests/official_source_file_list.py +++ b/services/agents-api/tests/official_source_file_list.py @@ -58,10 +58,14 @@ def sdk_page(values, has_more, query, **request): response = raw.get(endpoint, headers=headers, params={"after": foreign.id}) assert response.status_code == 404 and response.json()["error"]["code"] is None assert foreign.id not in response.text - for query in ("limit=0", "limit=10001", "limit=null", "order=invalid", "after=a&after=b", "purpose=a&purpose=b", "unknown=x"): + for query, code in (("limit=0", None), ("limit=10001", None), ("limit=null", "invalid_request"), ("order=invalid", None), + ("after=a&after=b", "unsupported_parameter"), ("purpose=a&purpose=b", "unsupported_parameter")): response = raw.get(endpoint + "?" + query, headers=headers) assert response.status_code == 400 - assert response.json()["error"]["type"] == "invalid_request_error" + error = response.json()["error"] + assert (error["type"], error["code"], error["param"]) == ("invalid_request_error", code, None), (query, error) + for query in ("unknown=x", "purpose=", "tenant_id=other&purpose="): + verify_page(raw.get(endpoint + "?" + query, headers=headers), descending, False) for purpose in ("unknown", "USER_DATA"): response = raw.get(endpoint, headers=headers, params={"purpose": purpose, "after": "file-missing"}) assert response.status_code == 400 diff --git a/services/agents-api/tests/official_vault_delete.py b/services/agents-api/tests/official_vault_delete.py index b5846fe15..8bede45b6 100644 --- a/services/agents-api/tests/official_vault_delete.py +++ b/services/agents-api/tests/official_vault_delete.py @@ -29,7 +29,8 @@ def verify_vault_deletion(client, other, invalid, peer, canary, expect_error): assert raw.delete(endpoint + target.id).status_code == 401 response = raw.delete(endpoint + target.id, headers={"Authorization": auth_headers["Authorization"]}) assert response.status_code == 400 and response.json()["error"]["code"] == "invalid_beta" - for kwargs in [{"params": {"include": "credentials"}}, {"content": b"{}"}]: + # The body rejects; the unknown include key is ignored. + for kwargs in [{"content": b"{}"}, {"params": {"include": "credentials"}, "content": b"{}"}]: assert raw.request("DELETE", endpoint + target.id, headers=auth_headers, **kwargs).status_code == 400 assert vaults.retrieve(target.id) == target assert list(vaults.credentials.list(target.id, order="asc")) == children @@ -38,7 +39,7 @@ def verify_vault_deletion(client, other, invalid, peer, canary, expect_error): assert response.status_code == 200 and response.parse().to_dict() == expected assert response.http_response.json() == expected and canary not in response.http_response.text assert response.headers["cache-control"] == "no-store" - response = raw.delete(endpoint + values[1].id, headers=auth_headers) + response = raw.delete(endpoint + values[1].id, headers=auth_headers, params={"include": "credentials"}) assert response.status_code == 200 and response.json() == {**expected, "id": values[1].id} for value in values[:2]: for method in ["GET", "DELETE"]: diff --git a/services/agents-api/tests/official_vault_list.py b/services/agents-api/tests/official_vault_list.py index 9929077d2..2fc38d7a2 100644 --- a/services/agents-api/tests/official_vault_list.py +++ b/services/agents-api/tests/official_vault_list.py @@ -86,7 +86,9 @@ def sdk_page(values, has_more, query, **request): descending[:size], True) for params, values in (({"status": "active"}, list(reversed(active))), ([("status[]", "archived")], list(reversed(archived))), - ([("status[]", "active"), ("status[]", "archived")], descending)): + ([("status[]", "active"), ("status[]", "archived")], descending), + ([("status", "archived"), ("status[]", "active")], descending), + ({"tenant_id": "foreign", "unknown": "1"}, descending)): verify_page(raw.get(endpoint, headers=headers, params=params), values[:20], True) for order, last in (("asc", expected[-1]), ("desc", expected[0])): params = {"after": last.id, "order": order} diff --git a/services/agents-api/tests/official_vaults.py b/services/agents-api/tests/official_vaults.py index d3b289d6d..0ecc6cdf4 100644 --- a/services/agents-api/tests/official_vaults.py +++ b/services/agents-api/tests/official_vaults.py @@ -96,8 +96,11 @@ def verify_vaults(client, other, invalid, peer, binding, expect_error): response = raw.request(method, base + suffix, headers=request_headers, json={} if method == "POST" else None) assert response.status_code == 400 and response.json()["error"]["code"] == "invalid_beta" - assert raw.post(base, headers=headers, params={"tenant_id": "other"}, json={}).status_code == 400 - assert raw.get(base + "/" + saved[0].id, headers=headers, params={"include": "credentials"}).status_code == 400 + # Unknown query keys are ignored; they never select a tenant or expand a Vault. + assert raw.post(base, headers=headers, params={"tenant_id": "other"}, json={"name": 1}).status_code == 400 + plain = raw.get(base + "/" + saved[0].id, headers=headers) + expanded = raw.get(base + "/" + saved[0].id, headers=headers, params={"include": "credentials"}) + assert plain.status_code == expanded.status_code == 200 and expanded.json() == plain.json() alias = str(client.base_url).rstrip("/") + "/agents/vaults/" + saved[0].id assert raw.get(alias, headers=headers).status_code == 404