Repository navigation
feat(reborn): route chat completions through ProductWorkflow - #4495
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the non-streaming Chat Completions workflow slice backed by ProductWorkflow in the ironclaw_reborn_openai_compat crate. It introduces the OpenAiChatCompletionsWorkflow service, wires it into the axum router, adds comprehensive contract tests, and updates the documentation. The review feedback suggests a minor improvement to simplify UNIX timestamp generation in chat_workflow.rs by leveraging the already-imported chrono::Utc library instead of using SystemTime and manual duration calculations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Draft status / remaining work before this can be considered the final #4444 implementation: This PR currently validates the Chat Completions route-level behavior, but it should not be treated as fully closing #4444 yet. What is covered here:
What is still missing for final #4444 completion:
Dependencies / blockers:
So the current state is: useful draft behavior harness for #4444, but final readiness depends on the #4488 ProductWorkflow facade decision or reviewer agreement that the temporary waiter seam is acceptable for this rollout slice. |
03be9aa to
587aa48
Compare
587aa48 to
b3cc727
Compare
6b84d46 to
e45961b
Compare
b3cc727 to
f02dab6
Compare
2e5539f to
d7d07ac
Compare
think-in-universe
left a comment
There was a problem hiding this comment.
Review pass for the #3283 Chat Completions workflow slice. I found two route-contract issues that should be addressed before treating this draft as ready.
|
Reviewing the current PR, I have 5 findings ordered by priority: P1 / Blocking
P2 / Should fix
P3 / Lower priority
The earlier review items around replay re-submit, newline sanitization, duplicate-chain recursion, created timestamp stability, bind timeout, message count cap, idempotency-key validation, and the |
abbyshekit
left a comment
There was a problem hiding this comment.
Code review — feat(reborn): route chat completions through ProductWorkflow
Multi-agent review (security · bugs · performance · tests · conventions) at 09f22e83. Diff-only mode.
Independent review of an open PR.
7 findings — 4 Medium, 3 Low. Posted as a comment (advisory). Confidence ≥ 50, deduplicated across reviewers.
| Sev | Conf | Reviewer | Location | Finding |
|---|---|---|---|---|
| Medium | 78% | tests | ref_store_contract.rs:466 (in body) |
No test asserts same idempotency-key from two different actors yields two distinct refs (cross-actor replay isolation) |
| Medium | 72% | tests | refs.rs:623 |
InMemoryOpenAiCompatRefStore has zero direct tests; new record_accepted_ack cross-actor denial untested |
| Medium | 70% | tests | chat_workflow.rs:352 |
accepted_ack_from_ack Duplicate-unwrap and CommandResult/NoOp branches are untested |
| Medium | 68% | conventions | chat_workflow.rs:58 |
Scope-mismatch 403 emits an authentication_error body, contradicting the crate's own 403 error-shape convention |
| Low | 80% | performance | lib.rs:113 (in body) |
New-chat hot path issues 2 avoidable storage GET round-trips (discarded RecordVersion) |
| Low | 62% | performance | refs.rs:531 |
Ref store grows unbounded (no TTL/GC) on the now-live chat-completions ingress |
| Low | 60% | conventions | handlers.rs:6 |
Idempotency-key length bound duplicated as a second magic constant and re-validated in the handler, diverging from the constructor's validation |
Findings without an inline anchor (cited lines fall outside the diff hunks)
[Medium · 78%] No test asserts same idempotency-key from two different actors yields two distinct refs (cross-actor replay isolation)
crates/ironclaw_reborn_openai_compat_storage/tests/ref_store_contract.rs:466-508 — tests reviewer
Focus area 2 asks: can actor A replay/bind against actor B's ref? The durable store keys its idempotency index path on a SHA-256 over {owner, surface, key} (storage_lib.rs:552-573), so the same idempotency-key value from two different owners must produce two independent reservations (Created + Created), never a Replayed/Conflict across actors. This isolation property is never directly asserted. The closest test, durable_store_rejects_idempotency_index_pointing_to_other_actor_mapping (line 466), reserves alice with key "same-key" and bob with a DIFFERENT key "bob-key", so it never exercises the same-key-different-actor collision-avoidance path. If the owner were ever dropped from the index digest, alice's prior ref could be returned as a Replayed mapping to bob (cross-tenant disclosure of a prior result) and no test would catch it.
Fix: Add tests::ref_store_contract::durable_store_same_idempotency_key_across_actors_creates_distinct_refs covering: alice reserve("same-key", body) -> Created(id_a); bob reserve("same-key", same body) -> Created(id_b); assert_ne!(id_a, id_b) and assert both outcomes are Created (not Replayed/Conflict); then alice re-reserve("same-key", same body) -> Replayed(id_a) to confirm per-actor replay still binds to the correct owner's ref.
[Low · 80%] New-chat hot path issues 2 avoidable storage GET round-trips (discarded RecordVersion)
crates/ironclaw_reborn_openai_compat_storage/src/lib.rs:113-130 — performance reviewer
On the persistent (libsql/postgres) store, every RootFilesystem::get/put is a separate network/DB round trip (see crates/ironclaw_filesystem/src/root.rs: get/put are distinct async ops, and put returns the new RecordVersion). The new-chat-completion hot path is: reserve_with_cas -> put_mapping(Absent) [PUT, returns version that is DISCARDED at lib.rs:124 Ok(_) => Ok(())], then submit_chat_and_record_ack -> record_accepted_ack_with_cas -> load_mapping_entry [redundant GET, lib.rs:270] + put_mapping(Version) [PUT], then bind_with_cas -> load_mapping_entry [GET, lib.rs:243] + put_mapping(Version) [PUT]. The first GET in record_accepted_ack_with_cas re-reads a mapping whose version was already known from the reserve PUT a few statements earlier. That is one fully avoidable round trip per new chat completion (and the bind step is a second known-stale read in the common no-contention case). This is a HOT path (per chat-completions request) but is dominated by the up-to-30s LLM waiter, so impact is modest — hence Low.
Fix: Have put_mapping return the RecordVersion from filesystem.put instead of discarding it (Ok(version) => Ok(version)), and thread the post-reserve version into submit_chat_and_record_ack so record_accepted_ack_with_cas can attempt its CAS put with the known version, skipping the initial load on the no-contention path (fall back to a re-load only on CasConflict). This removes ~1 GET per new chat completion on the persistent store.
Generated by near-ai-code-review (5 parallel reviewer agents + intent analysis). Diff-only; confidence ≥ 50; ≤ 15 inline comments.
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Route non-streaming POST /v1/chat/completions through ProductWorkflow-backed Reborn API slice with idempotency replay/conflict handling and OpenAI-compatible responses.
Stats: 14 findings (from 19 raw, 14 after dedup) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: 0.
Verdict: REQUEST_CHANGES — 2 High-confidence findings:
- Security/prompt-injection (chat_workflow.rs:411-448):
chat_messages_to_product_text()emits free-formrole: contentjoined by newlines. Even after CR/LF stripping, attacker can embed literalsystem: …in user content (no newlines required) — downstream LLM consumer that splits on": "sees spurious role turn.tool_call_idechoed inline astool_call_id: <value>can re-introduce fake role prefixes. Structured serialisation (JSON-per-message) is the durable fix. - Tests (chat_workflow.rs:354-372):
accepted_ack_from_ackDuplicate / CommandResult / NoOp arms are unreachable from any test. The Duplicate path is the exact code an idempotency replay exercises; a regression that silently misrouted NoOp to 200 (or recursed unboundedly on a deep Duplicate chain) ships unchecked.
Most of the prior reviewer findings (serrrfirat/think-in-universe/gemini) appear addressed by the recent "resolve chat completions review feedback" commits. Findings below are scoped to what remains.
Security
- High Multi-message transcript injected as free-form text with role prefixes attackers control (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:411-448, confidence 75)
chat_messages_to_product_text() serialises every message as "{role}: {content}" joined by newlines. sanitize_product_text_fragment() strips CR/LF from content but leaves role label unconstrained. Injection surface is content: after CR/LF stripping an attacker can embed exact string "system: <malicio…
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:433- Also flagged by: security/Medium — 1000-message cap has no per-message content-length bound, allowing large in-budg
- Medium OpenAiCompatAuthenticatedCaller::new validates subject but not tenant_id, enabling cross-tenant scope injection (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:45-70, confidence 75)
Constructor validates JWT claim subject matches scope's user_id but does not verify scope.tenant_id() matches tenant carried in verified claim. If host middleware mints ProtocolAuthEvidence for tenant A and caller passes OpenAiCompatActorScope scoped to tenant B (same user_id), check at line 58 pass…
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:58 - Medium Timeout on wait_for_chat_completion returns 503 but does not cancel underlying turn — abandoned turns accumulate (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:179-214, confidence 75)
Per CLAUDE.md spec this is intentional: 'Timeout returns retryable sanitized API error and does not cancel or detach underlying product turn.' Each timed-out request leaves a live turn running in product workflow. Attacker can flood POST /v1/chat/completions at rate-limit boundary (60/min), wait for…
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:185- Also flagged by: tests/Medium — bind_internal_refs error propagation path has no test
- Medium record_accepted_ack silently returns Ok(None) on unauthorized access — caller maps to 500 (
crates/ironclaw_reborn_openai_compat/src/refs.rs:620-641, confidence 75)
InMemoryOpenAiCompatRefStore::record_accepted_ack (and symmetrically bind_internal_refs) returns Ok(None) when is_authorized_for fails. Caller in submit_chat_and_record_ack treats Ok(None) as soft error via .ok_or_else(OpenAiCompatHttpError::internal)?, mapping to 500. Caller with valid scope but mi…
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:252- Also flagged by: tests/Low — InMemoryOpenAiCompatRefStore::record_accepted_ack has no unit test in refs_contr
Performance
- Medium std::sync::Mutex held for full async method body in InMemoryOpenAiCompatRefStore (
crates/ironclaw_reborn_openai_compat/src/refs.rs:526-556, confidence 72)
All four OpenAiCompatRefStore impls on InMemoryOpenAiCompatRefStore (reserve, bind_internal_refs, record_accepted_ack, lookup_authorized) call lock_state() acquiring std::sync::Mutex. No .await held inside lock, so deadlock is impossible, but OS mutex blocks calling tokio thread for duration of meth…
anchor:crates/ironclaw_reborn_openai_compat/src/refs.rs:528 - Medium SHA-256 fingerprint computed on unbounded raw_body bytes before any size guard (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:130-130, confidence 68)
OpenAiCompatRequestFingerprint::from_body_bytes(raw_body) hashes full raw HTTP body slice on every POST. parse_chat_request enforces message count but not raw body size — request with 1000 messages each containing megabytes of text passes count check and forces SHA-256 pass over all data. CLAUDE.md …
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:130
Tests
- High accepted_ack_from_ack: Duplicate, CommandResult, NoOp arms have no tests (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:354-372, confidence 85)
accepted_ack_from_ack handles five variants. Only Accepted, DeferredBusy, Rejected exercised. The Duplicate{prior} unboxing loop and CommandResult | NoOp -> internal-error arm never called from any test. Duplicate is exact path exercised on idempotency replay; NoOp is silent mis-wire bug.
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:354- Also flagged by: performance/Low — accepted_ack_from_ack loop follows Duplicate chain with no depth limit
- Medium error_from_rejection: UnknownInstallation, InvalidRequest, PolicyDenied rejections untested (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:374-402, confidence 80)
error_from_rejection maps five ProductRejectionKind variants. Tests exist only for BindingRequired (404) and AccessDenied (403). UnknownInstallation -> 503 retryable, InvalidRequest -> 400, PolicyDenied -> 403 non-retryable never exercised. The 503/retryable flag on UnknownInstallation could silentl…
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:374 - Medium Replayed path with accepted_ack=None (re-submit) has no handler-level test (
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:151-165, confidence 75)
When reserve returns Replayed(mapping) with accepted_ack=None (first request succeeded at submit but record_accepted_ack returned error and client retried), code calls submit_chat_and_record_ack again. No test drives this through full handler.
anchor:crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs:156- Also flagged by: maintainability/Low — Created/Replayed match arms duplicate identical destructuring
Conventions
- Low tower promoted from dev-dependency to unconditional production dependency (
crates/ironclaw_reborn_openai_compat/Cargo.toml:34-34, confidence 80)
Diff moves tower = { version = "0.5", features = ["util"] } from [dev-dependencies] into [dependencies] unconditionally. Only consumers of tower in this crate are tests/chat_workflow_handlers_contract.rs:27 and tests/stub_handlers_contract.rs:7 (use tower::ServiceExt for .oneshot()). No production s…
anchor:crates/ironclaw_reborn_openai_compat/Cargo.toml:34 - Low unix_timestamp_now duplicated across two crates with different time sources (
crates/ironclaw_reborn_openai_compat/src/refs.rs:670-672, confidence 75)
Diff adds unix_timestamp_now() to refs.rs (using chrono::Utc) while identically-named, same-purpose function already exists in ironclaw_reborn_openai_compat_storage/src/lib.rs:491 (using std::time::SystemTime). The two crates share same persisted type (OpenAiCompatResourceMapping.created_at). Splitt…
anchor:crates/ironclaw_reborn_openai_compat_storage/src/lib.rs:491- Also flagged by: maintainability/Low — unix_timestamp_now() duplicated across two crates with different implementations
Local Patterns
- Low Missing blank line between preamble paragraph and ## Boundary heading (
crates/ironclaw_reborn_openai_compat/CLAUDE.md:4-5, confidence 100)
Diff removed blank line that previously separated introductory paragraph (#4443 / #4444.) from ## Boundary section heading. Every other ## section in this file and sibling crate CLAUDE.md files is preceded by blank line. Without it paragraph runs directly into heading.
anchor:crates/ironclaw_reborn_openai_compat_storage/CLAUDE.md:4-6 - Low Two bullet-list items merged onto one line in CLAUDE.md Boundary section (
crates/ironclaw_reborn_openai_compat/CLAUDE.md:9-10, confidence 100)
Line 10 readsfeature-gated axum route fragments for host composition.- It must not bind sockets, ...— period ending first bullet immediately followed by start of second bullet (- It must not) without newline. Diff artifact: original had these as separate list items.
anchor:crates/ironclaw_reborn_openai_compat/CLAUDE.md:10 - Low Missing blank line before ## Do Not Move In Here heading in AGENTS.md (
crates/ironclaw_reborn_openai_compat/AGENTS.md:16-17, confidence 100)
New bullet ends at line 16 and ## Do Not Move In Here follows immediately at line 17 with no blank line. Original file had blank line between ## What This Crate Owns list and ## Do Not Move In Here. Every other ## section in this file and all sibling AGENTS.md files preceded by blank line.
anchor:crates/ironclaw_reborn_openai_compat/AGENTS.md:14-16
| None, | ||
| )); | ||
| }; | ||
| if claim.subject() != scope.user_id().as_str() { |
There was a problem hiding this comment.
Medium — OpenAiCompatAuthenticatedCaller::new validates subject but not tenant_id, enabling cross-tenant scope injection.
Constructor validates JWT claim subject matches scope's user_id but does not verify scope.tenant_id() matches tenant carried in verified claim. If host middleware mints ProtocolAuthEvidence for tenant A and caller passes OpenAiCompatActorScope scoped to tenant B (same user_id), check at line 58 passes. Downstream, ref store isolation, actor refs, conversation scoping all use caller's scope — includes attacker-controlled tenant_id.
Fix: Add tenant_id claim check analogous to subject check: verify claim's tenant matches scope.tenant_id().as_str().
if claim.subject() != scope.user_id().as_str() || claim.tenant_id() != scope.tenant_id().as_str() {
return Err(...);
}There was a problem hiding this comment.
Good catch. I did not add a tenant field to VerifiedAuthClaim in this PR because that changes the shared ironclaw_product_adapters auth-evidence contract and the host-auth minting surface, which is larger than the Chat Completions workflow slice. In the latest commit I tightened the OpenAI compat path by forcing the canonical ProjectionReadRequest returned from ProductWorkflow::read_projection(...) to match the authenticated caller tenant/user/agent/project before the projection reader is invoked. That closes the projection-read cross-tenant path here. The stronger tenant-bearing auth claim should be a separate auth-contract follow-up if we want the constructor itself to enforce it.
There was a problem hiding this comment.
Tracked as follow-up issue #4603: #4603
I agree the stronger long-term contract is tenant-bound host auth evidence. I am keeping #4495 scoped to the OpenAI-compatible route/ProductWorkflow slice and retaining the ProductWorkflow projection-scope match as defense in depth here, while #4603 covers the shared VerifiedAuthClaim/host-auth minting contract change across product adapters.
| UserMessagePayload, | ||
| }; | ||
|
|
||
| const DEFAULT_CHAT_WAIT_TIMEOUT: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
Medium — Timeout on wait_for_chat_completion returns 503 but does not cancel underlying turn — abandoned turns accumulate.
Per CLAUDE.md spec this is intentional: 'Timeout returns retryable sanitized API error and does not cancel or detach underlying product turn.' Each timed-out request leaves a live turn running in product workflow. Attacker can flood POST /v1/chat/completions at rate-limit boundary (60/min), wait for each to time out after 30s, continuously accumulate detached turns. Combined with replay via idempotency-key manipulation becomes sustained resource exhaustion vector.
Fix: Document and enforce per-user cap on live detached turns in ProductWorkflow layer; alternatively, integrate with turn quota or budget checked before submit_inbound.
Also flagged by: tests/Medium
There was a problem hiding this comment.
Agree with the risk. I have kept the documented timeout behavior here: the HTTP wait timeout returns a retryable sanitized 503 and does not cancel the underlying product turn. I did not add a quota in this PR because the enforceable cap belongs in ProductWorkflow/admission or turn quota policy, not in the OpenAI route wrapper. This PR now routes reads through the ProductWorkflow projection door; detached-turn admission should be tracked as a product-workflow/resource-governance follow-up rather than being bolted onto this compat route.
There was a problem hiding this comment.
Tracked as follow-up issue #4586: #4586
I agree this needs an enforceable bound, but the owner should be shared turn admission/resource policy rather than a route-local quota or route-local cancellation path. #4586 now tracks verifying/wiring existing turn-admission reservations for timed-out OpenAI-compatible turns while preserving this PR’s documented timeout-detach behavior.
Review comments already resolved
…4495) * feat(reborn): add OpenAI-compatible API contracts * fix(reborn): tighten OpenAI-compatible response contracts * fix(reborn): address OpenAI compat contract review * fix(reborn): satisfy merged main clippy * feat(reborn): add OpenAI-compatible product refs * fix(reborn): validate OpenAI-compatible ref mappings * fix(reborn): address OpenAI product refs review * feat(reborn): route chat completions through ProductWorkflow * fix(reborn): simplify chat completion timestamps * fix(reborn): use explicit ProductWorkflow submit door * test(reborn): stabilize subagent cancellation propagation * Validate durable OpenAI compat ref records * Fix chat completions idempotency validation * Format OpenAI compat storage review fixes * feat(reborn): add OpenAI-compatible product refs (nearai#4489) * feat(reborn): add OpenAI-compatible product refs * fix(reborn): validate OpenAI-compatible ref mappings * fix(reborn): address OpenAI product refs review * Validate durable OpenAI compat ref records * Format OpenAI compat storage review fixes --------- Co-authored-by: Robert Yan <mstr.raphael@gmail.com> * Address chat completions review comments * Persist chat completion idempotency replay metadata * Export OpenAI compat ack recording request * Persist OpenAI compat accepted workflow acks * fix(reborn): resolve chat completions review feedback * fix(reborn): resolve chat completions review feedback * fix(openai-compat): address chat workflow review comments * fix(openai-compat): address chat ref store review * fix(reborn): resolve chat projections through workflow * feat(reborn): wire OpenAI chat completions route * fix(reborn): centralize chat body validation --------- Co-authored-by: Robert Yan <mstr.raphael@gmail.com> Co-authored-by: Robert Yan <46699230+think-in-universe@users.noreply.github.com>
Summary
POST /v1/chat/completionsthrough a ProductWorkflow-backed Reborn API slice instead of v1 gateway or direct LLM proxy code.chatcmpl-*ref reservation, idempotency replay/conflict handling, bounded waiter timeout, and sanitized OpenAI-compatible responses.Change Type
Linked Issue
Refs #4444
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test -p ironclaw_reborn_openai_compat --features openai-compat-beta,cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold,cargo test -p ironclaw_architecture reborn_boundary_rules_active_crates_are_workspace_memberscargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewAdditional validation:
cargo clippy -p ironclaw_reborn_openai_compat --all-targets --all-features -- -D warningsgit diff --checkSecurity Impact
Yes. This wires the OpenAI-compatible Chat Completions route to require a host-minted authenticated caller before ProductWorkflow submission, rejects auth subject/scope mismatch, keeps public ids opaque, preserves actor-scoped idempotency, rejects streaming before side effects, and avoids v1 gateway/direct LLM proxy paths. Host composition still owns listener binding, bearer/session auth middleware, CORS/origin, body/rate limits, audit, and mounting.
Database Impact
None in this slice. It uses the existing OpenAI-compatible ref store port and in-memory contract tests; durable storage remains in the existing storage crate.
Blast Radius
Limited to
ironclaw_reborn_openai_compatbehindopenai-compat-beta, plus Reborn contract docs. Default router behavior remains fail-closed unless composition injects the Chat Completions workflow state.Rollback Plan
Revert this PR to return
/v1/chat/completionsto the prior fail-closed501 unsupportedbehavior for this Reborn route fragment.Review Follow-Through
This is stacked on #4443. Reviewer judgment requested on whether the waiter port is the right temporary seam until the canonical ProductWorkflow projection/read/subscribe doors from the follow-up design land.
Review track: C