feat(protocols): implement P3 EasyInputMessage.phase - #1281
Merged
Merged
Conversation
slin1237
requested review from
CatherineSue,
key4ng,
zhaowenzi and
zhoug9127
as code owners
April 21, 2026 04:50
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
grpc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughA new public Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Persistence as Persistence_Utils
participant DB
Client->>Router: request (load/build conversation)
Router->>Persistence: fetch stored items
Persistence->>DB: read stored conversation items
DB-->>Persistence: stored item (legacy or {content, phase})
Persistence->>Persistence: split_stored_message_content -> (content_value, stored_phase)
Persistence-->>Router: deserialized content + stored_phase
Router->>Client: constructed ResponseInputOutputItem (phase set or None)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add an `Option<MessagePhase>` field carrying `"commentary" | "final_answer"`
to the three message variants that map to OpenAI's `EasyInputMessage` and
`ResponseOutputMessage`:
- `crates/protocols/src/responses.rs`:
- New `MessagePhase` enum (`Commentary` / `FinalAnswer`,
`#[serde(rename_all = "snake_case")]`).
- Add `phase: Option<MessagePhase>` to
`ResponseInputOutputItem::SimpleInputMessage` (spec `EasyInputMessage`),
`ResponseInputOutputItem::Message` (so assistant replays round-trip
phase from the output side), and `ResponseOutputItem::Message`
(spec `ResponseOutputMessage`).
- All three fields `#[serde(default, skip_serializing_if = "Option::is_none")]`
so absent-on-wire stays absent and existing clients keep working.
- `normalize_input_item` forwards phase when converting SimpleInputMessage
into the typed Message variant.
- `ResponseOutputItem::new_message` keeps its existing signature (phase
defaults to `None`); added `new_message_with_phase` for callers that
need to set it explicitly.
- `crates/protocols/src/builders/responses/response.rs`: update the two
struct literals in the builder tests.
- `crates/mcp/src/core/session.rs`: update the two struct literals in the
session ordering test.
- `model_gateway/src/routers/common/persistence_utils.rs`:
- `extract_input_items` carries `phase` into the normalized message JSON
for `SimpleInputMessage`.
- `item_to_new_conversation_item` stores message items as
`{content: [...], phase: "..."}` when phase is present, and leaves the
legacy bare-array shape untouched otherwise so existing rows deserialize.
- `item_to_json` recognizes both shapes when reconstructing a message
item and hoists `phase` back to the item level.
- New `pub fn split_stored_message_content(Value) -> (Value, Option<MessagePhase>)`
helper shared with router load paths.
- `model_gateway/src/routers/openai/responses/history.rs`: load path for
conversation items uses `split_stored_message_content` and populates the
new `phase` field; existing text-only append paths pass `phase: None`.
- `model_gateway/src/routers/grpc/regular/responses/common.rs`: same
treatment for the gRPC-regular load path (`load_conversation_history`)
plus the three text → `Message` literals in that module.
- `model_gateway/src/routers/grpc/regular/responses/{streaming,conversions}.rs`,
`model_gateway/src/routers/grpc/harmony/responses/{common,non_streaming}.rs`,
`model_gateway/src/routers/grpc/harmony/processor.rs`,
`model_gateway/benches/routing_allocation_bench.rs`,
`model_gateway/tests/spec/responses.rs`: add `phase: None` to the
remaining struct literals so the crate continues to compile.
The Responses API spec (verified against
`.claude/_audit/openai-responses-api-spec.md`, lines 102-111 and 119-135)
declares an optional `phase` discriminator on `EasyInputMessage` and
`ResponseOutputMessage`. `crates/protocols/src/responses.rs` had zero
references to `phase`, so any phase value supplied by a client was
dropped on the way in and never re-emitted to the upstream model.
For gpt-5.3-codex+ this is a quality regression: the spec note
explicitly states that dropping `phase` across turns causes latency /
quality degradation because the model uses it to separate commentary
reasoning from the final answer. P3 restores the field on the wire and
through the gateway's persistence layer so that a phase attached to a
stored assistant message survives store → list-items → re-inject as
input across subsequent turns.
- `MessagePhase` is a plain `Copy` enum with serde `snake_case` renaming,
matching the `"commentary" | "final_answer"` wire values from the spec.
- `phase` is `Option<_>` with `#[serde(default, skip_serializing_if)]`
on every added field so backward-compat is preserved for both
serialization (absent when `None`) and deserialization (absent JSON
→ `None`).
- Conversation-item storage uses a `Value` column; rather than migrate
the schema, messages that carry phase are stored as an object
`{"content": [...], "phase": "..."}`. Legacy rows that stored just the
content array continue to deserialize via `split_stored_message_content`,
which detects the object shape and otherwise returns the raw input.
- All consumer call sites in the gateway use `..` patterns and are
unaffected; only struct-literal constructors needed updates.
- `CARGO_TARGET_DIR=/tmp/p3-target cargo check --workspace --tests --benches`
— clean (isolated target dir per tech-lead environment advisory).
- `CARGO_TARGET_DIR=/tmp/p3-target cargo clippy -p smg -p openai-protocol
-p smg-mcp --lib --bins --tests -- -D warnings` — clean.
- `cargo test -p openai-protocol` — 82/82 pass (55 + 8 + 18 + 1 doc).
- `cargo test -p smg --lib` — 553/553 pass.
- `cargo test -p smg-mcp --lib` — 177/177 pass.
Refs: P3
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…e 2) Tech Lead P3 cycle-1 REJECT: `pub fn new_message_with_phase` had zero callsites — §7 "No new helper used by one callsite. Inline." Removed. Callers that need to set phase use the struct-literal pattern directly, matching existing constructor patterns in the file. All other P3 substance preserved: phase field across 3 variants, persistence migration, all 6/6 spec roundtrips still green. Refs: P3 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Addresses a cycle-1 fmt artifact missed in P3 Tech Lead review: the let-chain at persistence_utils.rs:315 collapses to a single line under nightly rustfmt. Pure whitespace normalization; no semantic change. Refs: P3 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
feat/audit-p3-phase
branch
from
April 21, 2026 05:40
668cdcc to
2a3588e
Compare
mergify
Bot
removed
the
needs-rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements audit task P3: add the typed
phasefield (MessagePhase { Commentary, FinalAnswer }) toEasyInputMessage/ResponseInputOutputItem::Message/ResponseOutputItem::Messageper spec, and thread it end-to-end through conversation persistence sophasesurvives multi-turn store + retrieve round-trips. Unblocks audit task I1 (which depends on the typed Message variant distinction).What changed
Commits on branch (
main..HEAD):b4ca9343—feat(protocols): implement P3 EasyInputMessage.phase(substance)e2d308fc—refactor(protocols): drop dead new_message_with_phase helper (P3 cycle 2)(cycle-1 REJECT remediation — removed 0-callsitepub fnper §7)668cdcc5—style(protocols): rustfmt remediation for P3 cycle-1 persistence_utils(cycle-2 fmt fix for a cycle-1 artifact caught by the cycle-2 Lead-requested gate expansion)Diff totals (vs main): 13 files, +146 / -10.
crates/protocols/src/responses.rs:MessagePhaseenum (commentary | final_answerwith serde renames),phase: Option<MessagePhase>added to 3 message variants per spec,normalize_input_itemforwards phase,new_messagebuilder preserves signature (phase: Nonedefault), 2 existing constructor helpers threaded through.crates/protocols/src/builders/responses/response.rs: 2 test-literalphase: Noneadditions (compile-forced).crates/mcp/src/core/session.rs: 2 test-literalphase: Noneadditions in#[cfg(test)]block (compile-forced).model_gateway/benches/routing_allocation_bench.rs: 1phase: Nonein bench (verified compile-forced by revert →error[E0063]: missing field 'phase').model_gateway/tests/spec/responses.rs: 2 test-literalphase: Noneadditions (compile-forced).model_gateway/src/routers/grpc/harmony/{processor.rs, responses/common.rs, responses/non_streaming.rs}: 5phase: Noneadditions (compile-forced).model_gateway/src/routers/grpc/regular/responses/{common.rs, conversions.rs, streaming.rs}: 7phase: Noneadditions + load path viasplit_stored_message_content(compile-forced + AC-required).model_gateway/src/routers/openai/responses/history.rs: load path viasplit_stored_message_content+ 1phase: None(AC-required + compile-forced).model_gateway/src/routers/common/persistence_utils.rs:split_stored_message_contenthelper (3 callsites → §7 passes) + wrapping-object persistence to carryphasealongside content array + cycle-2 rustfmt on let-chain at L315.Why
OpenAI Responses API spec requires
phase ∈ {commentary, final_answer}on message items to distinguish intermediate scaffolding text from the final answer surface. smg previously had no such field, so round-tripping a spec-compliant assistant message through conversation storage lost the phase tag silently. This PR:{"content":[...], "phase":"..."}when phase is present, with a backward-compat decode path for legacy bare-array rows (zero-schema-migration on the data-connector side; content column stays aValueblob).#[serde(default, skip_serializing_if = "Option::is_none")]so absent phase never emits"phase": nullonto user/system messages (verified via 6/6 spec roundtrips including a dedicated user-role-no-phase fixture).Storage wrapping was evaluated as the minimum-scope alternative to a
data_connectorschema migration (which would have been far larger blast radius). Legacy bare-array rows still decode via the newsplit_stored_message_contenthelper; no data migration required.Verification
cargo check --workspace --tests --benchesclean (isolatedCARGO_TARGET_DIR=/tmp/p3-c2-lead-targetto avoid worktree cache collision)cargo test -p openai-protocol→ 82/82 (55 + 8 + 18 + 1)cargo test -p smg --lib→ 553/0 (4 ignored)cargo test -p smg-mcp --lib→ 177/0cargo clippy -p openai-protocol -p smg -p smg-mcp --lib --bins --tests -- -D warningscleancargo +nightly fmt --all -- --checksilent (cycle-2 commit B addresses cycle-1 persistence_utils.rs:315 let-chain artifact)"thinking"→ rejects at deserialize (no silent#[serde(other)]swallow)routing_allocation_bench.rs:95withoutphase: None→error[E0063]: missing field 'phase' in initializer of ResponseInputOutputItem→ confirmed compile-forcedsplit_stored_message_contenthas 3 prod callsites (persistence_utils internal + openai/responses/history.rs:156 + grpc/regular/responses/common.rs:279) → §7 "no new helper used by one callsite" PASSESnew_message_with_phaseremoved (was 0 callsites; cycle-1 §7 blocker) →grep -rn 'new_message_with_phase'zero hitsunavailable(harness skill permission; Lead proceeded solo per playbook §8 fallback after own fixtures passed both cycles)Blast radius
13 files. Breakdown: protocols schema (2: responses.rs, builders/responses/response.rs), gateway routers + persistence (6), tests + bench (3: mcp session, bench, spec test), rustfmt cleanup (1: persistence_utils.rs).
Every non-responses.rs edit verified compile-forced via revert-and-test, or justified as AC-required (persistence wrapping for "phase survives store+retrieve"). No forbidden files touched.
Out of scope
phasefrom upstream SGLang/vLLM harmonizer → future follow-up tied to backend-specific structural tag emission.Messagevariant (spec distinguishes taggedMessageuser/system/developer vs taggedResponseOutputMessageassistant-with-phase) → I1's scope, not P3.data_connectorto make phase a first-class column → evaluated and rejected as over-scope; wrapping-object persistence with legacy decode is the minimum path.Refs: audit task P3 ·
.claude/_audit/responses-api-gap-audit.mdSummary by CodeRabbit
New Features
Chores
Tests