Repository navigation
Conversation
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two critical and two moderate findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Establishes typed reasoning, provenance, guarded provider-profile replay, and lifecycle foundations while keeping opaque replay disabled.
Changes:
- Adds typed reasoning, assistant phases, provenance persistence, and fail-closed storage.
- Adds request validation, isolated transport, model evidence, and offline replay coverage.
- Refactors stream ownership, canonical tool history, recorder fixtures, and migrations.
Unresolved findings cover opaque error redaction, retained-size accounting, and duplicate WebSocket lane handling.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
|
@Zheng-Lu I'll convert the PR to draft. please breakdown the PR into smaller pieces with a more reflective PR titles, the description references so many issues as well. |
Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Renumber the reasoning provenance migration to 0007 after main's 0006_item_references. Stored rows now keep public item IDs, so tests that persist the same recorded items twice use distinct IDs. Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Proposed split: Each PR would reference only #335; PR 2 references #244 instead. PR 1 to 3 have no dependencies on each other, so I'll open those first. Should 7 wait as a draft until the profile can actually be enabled? |
…384) Part of #335 (split from #350) ## Summary - Replace the untyped JSON in `ReasoningOutput` with typed reasoning content, summaries and status, plus a bounded `OpaqueReasoning` string for `encrypted_content`: redacted `Debug`, 16 MiB ceiling. Public JSON field names are unchanged. The shared retained-response budget now charges reasoning parts and opaque bytes. - Keep the existing vLLM replay policy: plaintext is joined, summaries are left out of the upstream copy, and opaque-only continuation is rejected before inference. - Stop silently dropping stored context. Undecodable history rows, missing referenced rows, and malformed history references or response metadata now return typed storage errors. The errors never include stored values. SQL NULL keeps its legacy meaning. - Keep reasoning stored by earlier releases readable and continuable. One compatibility decoder mirrors the schema earlier releases accepted. It keeps plaintext (as `reasoning_text`) and every summary part, string opaque state, status and agent attribution that satisfies the typed schema. It covers history rows, stored response snapshots and multi-agent tree histories, on both `previous_response_id` and `conversation` continuation. Anything earlier releases could not have stored still fails closed, and so does an item whose only replay state the decoder would drop: opaque-only continuation is still rejected before it reaches the model. Dropped opaque state is logged with its byte count, never its content. Compatibility: request input and strict upstream ingestion now reject reasoning items that don't match the typed schema, for example a non-string `encrypted_content` or an unknown `status`; earlier releases accepted them. The default (lenient) upstream path keeps such an item, using the same decoder, instead of relaying the invalid values: the item keeps only the fields the typed schema accepts. No migration is needed, and existing rows are not rewritten. ## Test Plan - `cargo test -p agentic-server-core -p agentic-server`: 1,979 passed, 0 failed, 18 ignored. `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --check` and pre-commit (including Rust file sizes) pass. - `legacy_reasoning_test.rs` rewrites stored rows and stored response snapshots to shapes earlier releases stored: - Rows decode with their valid fields kept. A `previous_response_id` continuation completes and replays the plaintext to vLLM as `reasoning_text`. - A rewritten snapshot is retrievable through `ResponseHandler::retrieve` (which serves `GET /v1/responses/{id}`). It is continuable by both `previous_response_id` and `conversation`. - Shapes earlier releases could not have stored fail closed: rows with `InvalidHistoryItem`, snapshots with `InvalidResponseMetadata`. The shapes are a content part without a string `type`, non-array content, a non-string `status` and a malformed `agent`. - Opaque-only reasoning never reaches the model, whether it is continued by `previous_response_id` or by `conversation`. String state is rejected with the existing 400. Object state, which earlier releases accepted, fails closed with `InvalidHistoryItem`. In both cases only the stored turn reaches the mock upstream, and the object case fails without the fix. - Negative control: before the shared decoder, the snapshot test fails because retrieval errors, and the row test fails because the rows are accepted. Before `ConversationStore` used it, only the `conversation` continuation fails. - A `storage/types/response.rs` unit test does the same for multi-agent tree histories. It also fails before the shared decoder. - A `types/io/reasoning/legacy.rs` unit test checks dropped opaque state. Non-string state and state over the ceiling are logged with their byte count and reason only. Kept or absent state logs nothing. The test fails with the warning removed. - `accumulator/tests.rs`: lenient JSON and SSE ingestion keep a reasoning item with the invalid fields dropped, and strict ingestion rejects the same item on both paths. Shapes earlier releases rejected are dropped from JSON, and in SSE the completion is ignored and the opened item is kept, byte-for-byte as on `main` (checked against `main`). - `reasoning_types_test.rs` covers schema rejection, nullability, exact opaque round trips, redaction and the size ceiling. `storage_response_integrity_test.rs` and the storage suites cover invalid rows, missing rows, bad references and bad metadata, all with redacted errors. - The existing recorder-generated OpenAI and gateway reasoning cassettes replay unchanged. No cassette was added or edited. --------- Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
## summary fixes #385. stored response history used each item's public id as the `items.id` primary key. a client that sent a previous response's `output` back as `input` with `store: true` (the default) hit `UNIQUE constraint failed: items.id` and a 500, even though the request is valid: Persistence(Storage(Database(Database(SqliteError { code: 1555, message: "UNIQUE constraint failed: items.id" })))) `item::create_in_tx` now gives each response-history occurrence a fresh `item_…` row id and keeps the public id in a new nullable `items.public_id` column. `Item::public_id()` still returns the public id, so clients see stable ids, and repeated occurrences keep every position. conversation inserts keep their membership checks, sequence allocation, and reference resolution in the same function. conversation inserts, cursors, retrieval and detach resolve an item by public identity (`COALESCE(public_id, reference_id, id)`). the lookup is indexed on that expression with the tenant, because a plain `public_id` index is not usable by the resolved predicate. the join resolves one source row per incoming item first (a `source_rows` CTE): several rows can share a public identity, and joining on the identity alone fans out into duplicate row ids, which then fails on `items.id`. this supersedes #388, which used the same mechanism but no longer compiles on main and left the conversation resolution untouched. thanks to @RainieLLM for the original approach. ## test plan - `resubmitted_item_history_test` replays the recorded two-turn `OpenAI` responses (`text_only/responses/`, streaming and non-streaming). it asserts the upstream request the second turn sends, then the rehydrated order, roles, content, public ids, and distinct row ids. the resubmission case is constructed from the same recording and keeps every submitted occurrence. - `stateful_conversation_integration` asserts the same for the recorded two-turn conversation path (`text_only/conversation/`, streaming and non-streaming): rehydrated history, public ids, row keys, and the upstream history. - `conversation_storage_regressions` covers repeated occurrences of one public id, cross-tenant non-reuse, and batch rollback. - `conversations_api_cassette_test` covers the conversation path, including `edge_replay::gateway_matches_all_openai_edge_case_status_codes_and_bodies`, which posts one stored public id twice and caught the fan-out. - reverting the resolution predicate reproduces `UNIQUE constraint failed: items.id`, and removing the single-source reduction reproduces `UNIQUE constraint failed: items.conversation_id, items.seq`. - index check on sqlite with 100k existing items in one tenant: inserting 100 items takes 1.0 ms with the expression index and 640 ms without it. the plan changes from `SEARCH source USING INDEX idx_items_tenant_id` to `SEARCH source USING INDEX idx_items_public_identity (tenant_id=? AND <expr>=?)`. - also ran `cargo fmt -- --check`, `cargo clippy -p agentic-server-core --all-targets -- -D warnings`, `cargo test --workspace`, and the production file-size check. ## notes - the migration is `0007_response_item_public_ids.sql`. open #350 also proposes a `0007`; whichever merges second renumbers to `0008`. - consolidating the insert into `item.rs` crosses the 500-line production cap, so `.rust-file-sizes.json` records a reviewed exception (510) with the reason. - existing rows keep the public id as their row id and a null `public_id`; `Item::public_id()` falls back to `reference_id` and then `id`, so no backfill is needed. - postgres was not exercised locally (no server available). the expression index uses standard `CREATE INDEX` syntax, and the ci `postgres` job applies the migration and runs the storage tests. --------- Signed-off-by: Abhinandan <abhinandanverma551@gmail.com> Signed-off-by: maral <maralbahari.98@gmail.com> Co-authored-by: maral <maralbahari.98@gmail.com>


Summary
Implement the typed reasoning and provenance foundation for #335. Current progress: typed reasoning, fail-closed stored history, server-owned policy/provenance, upstream-reported model evidence, pinned candidate-profile compatibility preflight, the stateless opaque request/isolated transport adapter, stream-owned producer cancellation, pinned provider-reference replay coverage, assistant phase preservation, offline full-executor acceptance, canonical durable tool-round ordering, modeled request-surface preflight, and closed nested HTTP/WebSocket wire guards and matching core preflight are implemented. Opaque replay remains disabled; this draft does not close #335.
Review follow-up (2026-09-24): default plaintext provenance no longer fingerprints credentials; bounded legacy reasoning rows remain continuable without gaining opaque trust; lenient JSON/SSE treat malformed upstream model metadata as unknown evidence; the closed opaque profile rejects
prompt_cache_key. The request guard shares the typed field list with core preflight. Full workspace tests, Clippy with-D warnings, formatting, and staged pre-commit hooks pass. Opaque qualification remains gated.responses.reasoning_replay_policy, defaulting tovllm_plaintext. Reservedopaque_responsesfails before storage initialization, rehydration, tool discovery, inference, compaction, or external commit.ResponsePayload.model. Only explicit, consistent terminal metadata supplies evidence. Missing/null metadata, nonterminal JSON, lenient EOF completion, and ambiguous lenient streams remain unknown. Malformed model metadata becomes unknown evidence in lenient ingestion and a redacted upstream error in strict ingestion; strict ingestion also rejects changing model names. Model retention is charged once per round.gpt-5.6-solearly and echoes requestedgpt-5.6at completion. Lenient execution succeeds with unknown model evidence; strict decoding rejects the conflict. No alias or model-family equivalence is inferred.openai_gpt_5_4_2026_03_05_v1, pinning the exact OpenAI Responses endpoint andgpt-5.4-2026-03-05requested/reported snapshot. Validate policy/profile consistency, exact routing, canonical provenance, credential compatibility, nonempty opaque state, and unsupported compaction before inference. Profile fingerprints bind a separate contract domain and cannot match legacy observational identities. Post-ingestion profile provenance requires exact terminal model evidence. Errors use static redacted messages and typed HTTP/SSE mappings. Even a compatible candidate remains unavailable: configuration and request execution still fail the qualification gate.store: false, preserving exact opaque state, summaries, IDs, optional status, and ordering while omitting plaintext content and internal provenance. Canonical item types and gateway storage policy stay unchanged; the initial plaintext sanitizer remains vLLM-only.gpt-5.4-2026-03-05,store: false, complete item replay, low reasoning effort and concise summaries. The provider accepts continuations using the exactoutput_item.doneopaque bytes retained by gateway ingestion, which differ from the terminal envelope bytes. WebSocket references use a fresh connection per request, not gateway connection-local continuation.MessagePhase::{Commentary, FinalAnswer}through completion, output-to-input conversion, storage and rehydration. Live captures exposed that this field was previously dropped. Reject phase on non-assistant input; absent/null legacy phase remains absent. OpenAI phase guidance informs the contract without introducing a second production ingestion path.store: false.ExecuteRequestexecution, durable storage, transient forks/promotion, explicit conversations, and a deterministic local MCP tool loop. A bounded, per-context, core-unit-test-only loopback fixture skips availability without weakening profile, routing, credential, provenance, or terminal-model checks. An ordinary opaque transport cannot grant test availability. No runtime configuration or feature flag enables the candidate, and startup, external commit, and compaction gates remain closed. This is offline executor acceptance, not live gateway or actual HTTP/WebSocket handler qualification.RecordedOutputPrefixbookkeeping owned by the turn rather than its session lease. Both persistence modes avoid duplicated public projections while retaining MCP discovery metadata. Default vLLM persistence behavior remains unchanged; no existing history is rewritten.low/concisereasoning flow, bounded output tokens, and function or local MCP normalization with explicitrequire_approval: never. It rejects unqualified reasoning/sampling/text settings, automatic truncation, vLLM-only fields, unsupported tool kinds and lossy function extensions with static parameter-specific 400 errors. Default vLLM behavior is unchanged; the availability gate remains closed. The HTTP/WebSocket profile guard rejects unknown and duplicate top-level keys and unknown reasoning keys. Closed nested schemas now cover the pinned user/assistant message, opaque reasoning, function call/output, function/MCP tool, and named-function-choice shapes. Bounded visitors reject duplicate keys inside open function JSON Schema and MCP header documents; empty replay metadata arrays and explicit list caps match the captures. Core preflight applies the same typed input surface to direct callers and rehydrated history. All 18 requests in the six recorder-generated pinned cassettes pass the guard. The availability gate remains closed.Storage and compatibility
Migration
0006_reasoning_provenance.sqladds nullableitems.reasoning_provenanceTEXT without backfilling legacy rows. Startup/readiness require the column, including supervisor-managed schemas. Invalid, oversized, unknown-version, or wrong-item-kind provenance fails closed. Fixed non-wire provenance charges participate in response/checkpoint budgets. Reported-model propagation requires no additional migration and does not rewrite old unknown-model fingerprints.Public JSON/OpenAPI remain free of provenance fields; public completed-response model naming is unchanged. Rust struct-literal callers must supply the new policy/profile/provenance fields and the typed optional model in
EventPayload::Response. Raw item insertion is crate-private; typed stores remain the persistence interface.UpstreamRequestimport paths remain available; Rust struct literals now requirestore: Nonefor the default contract and.into()to convertCow<ResponsesInput>into the typed input view. No additional storage migration is needed for this adapter, stream-owned producer lifecycle, assistant phase, or canonical round-ordering fix. Public RustRequestContextstruct literals must now initializerecorded_output_prefixwithRecordedOutputPrefix::default(); split contexts do so without serializing the marker. Item JSON preserves phase directly; publicInputMessageandOutputMessagestruct literals must now supplyphase, while constructors default it toNoneand existing import paths remain available. Design and migration details are indocs/design/provider-aware-reasoning.md.Remaining #335 work
store: falsecannot bypass executor preflight through transparent HTTP proxying. Ambiguous WebSocket wire failures carry no stream or previous-response ID, so they cannot select a lane or cached checkpoint for eviction. Existing gpt-5.6 captures remain regression evidence, not pinned-model qualification.context_management; provider compaction remains unsupported.Test Plan
0621bce): the complete core/server suites passed at the pushed head withcargo test -p agentic-server-core -p agentic-server --quiet; workspace all-target Clippy with-D warningsand all applicable staged-file pre-commit hooks passed. The server guard replays all 18 requests from six recorder-generated pinned cassettes; HTTP/WebSocket integration rejects nested unknown and duplicate fields, including duplicate keys in open JSON Schema, without leaking caller content. Core tests reject unqualified input shapes and excess list sizes before the closed gate. No live provider calls or cassette edits were made.be77904):cargo test -p agentic-server-core -p agentic-serverpassed before the request-parser extraction, and the complete server suite passed again afterward. Workspace all-target Clippy with-D warnings, format, and all applicable staged-file pre-commit hooks passed. HTTP and WebSocket integration tests cover unknown/duplicate fields, redacted errors, the closed gate,store: falseexecutor routing, and unchanged default proxying; core preflight rejects unknown input-item kinds. No live provider calls or cassette changes.c825a66): the full core suite passed withcargo test -p agentic-server-core --offline --quiet --no-fail-fast; workspace all-target Clippy with-D warnings, formatting and applicable staged-file pre-commit hooks passed. A new JSON/SSE matrix rejects 20 unqualified modeled request cases before missing-history lookup or network access, with exact typed parameter errors. A positive unit case preserves the pinned settings without opening the gate; prior recorder-backed executor tests remain green. No provider calls or cassette edits were made.4a617d5): nine offline full-executor acceptance tests pass across pinned JSON/SSE recordings. Cover durable continuations/branches, transient fork isolation and promotion, origin and rotated-credential rejection before network access, ordinary-transport fail-closed behavior, unpolled/active drop, malformed or truncated upstream completion, failed-fork recovery, and two-round MCP tool execution with durable/conversation/session history ordering. A second-round model mismatch persists no response after a completed tool; remote tool side effects are explicitly not rolled back. The turn-prefix unit test checks monotonic deduplication and discovery retention.cargo test --workspace --offline --quiet --no-fail-fast, including storage, reasoning, stateful responses, sessions, HTTP/WebSocket, OpenAPI, and compaction/cassette suites.cargo clippy --workspace --all-targets --offline -- -D warningsand all applicable staged-file pre-commit hooks, including formatting and Rust file-size checks.AgentPipelinebefore promotion. Four reference tests verify exact terminal model evidence, opaque bytes, summaries, phase, item order, typed request projection, branch sources, and in-memory missing-terminal/duplicate-completion/wrong-index faults. The files match recorder staging byte-for-byte; no captured YAML was hand-authored or edited. These references do not qualify live gateway execution.sha2library is now also a direct core dependency.