Repository navigation
fix(test): unbreak main — admit the adoption-replay create fixture through the structured-chat gate - #19246
Conversation
…tured gate Semantic conflict between two green PRs. #19176 added this replay test while `agentSession.*` still admitted a `runtime` client on its negotiated capability alone; #18700 then made `experimentalStructuredNativeChat` one rule for every caller. Neither branch saw the other, and main runs no post-merge test gate, so `agentSession.create` started refusing at the envelope level and the test's `ok: true` expectation broke. #18700's rule is the intended behaviour and `create` starts work, so it belongs behind the gate. The fixture is what is stale: it builds a real `OrcaRuntimeService` whose client settings are unset. Enable the setting the way #18700 already did for the sibling pre-commit fixture. The assertions about durable-identity replay are untouched and now actually run.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe replay test now mocks Merge Risk: ⚪ Minimal · up to The replay test now enables the required structured-chat setting so it exercises durable-identity replay rather than being rejected at the RPC gate. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the failure, root cause, rationale, scope, and verification. However, it omits several template sections, including the required Linked Issue, ELI5, Visual Proof with an N/A explanation, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Resolution Add the missing template sections. Include a linked issue after “Fixes #”, state “N/A” with a reason under Visual Proof because this is not a UI change, complete or explicitly mark AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist, and preserve the existing failure analysis and verification details.
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The PR is a five-line, test-only fix that admits the adoption-replay agentSession.create fixture through the settings gate so the durable-identity replay body it exists to cover actually runs.
getClientSettingsadmission spy —structured-agent-session-adoption-replay.test.tsnow stubsgetClientSettingsto reportexperimentalStructuredNativeChat: true, matching the siblingstructured-agent-session-precommit-refusal.test.tsfixture that #18700 already updated. The gate (supportsStructuredAgentSessions→isStructuredNativeChatEnabled) now admits the call, and the previously-unreached replay assertions (acquireonce,publishStructuredAgentSessionTabtwice,selectAccountHomeonce, twoagent_session_identity_requiredrefusals) genuinely execute.vi.restoreAllMocks()inafterEachcovers the spy.
I confirmed the root cause against the gate in structured-agent-session-policy.ts and ran the test file in isolation (1 passed). The change is minimal, consistent with the documented one-rule-for-every-caller behavior, and touches no production code.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
…tured gate (stablyai#19246) Semantic conflict between two green PRs. stablyai#19176 added this replay test while `agentSession.*` still admitted a `runtime` client on its negotiated capability alone; stablyai#18700 then made `experimentalStructuredNativeChat` one rule for every caller. Neither branch saw the other, and main runs no post-merge test gate, so `agentSession.create` started refusing at the envelope level and the test's `ok: true` expectation broke. stablyai#18700's rule is the intended behaviour and `create` starts work, so it belongs behind the gate. The fixture is what is stale: it builds a real `OrcaRuntimeService` whose client settings are unset. Enable the setting the way stablyai#18700 already did for the sibling pre-commit fixture. The assertions about durable-identity replay are untouched and now actually run.

What broke
src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.tsfails onorigin/main:The
agentSession.createcall is refused at the RPC envelope level (ok: false,structured_agent_session_unsupported) instead of succeeding at the envelope level andrefusing at the result level with
agent_session_operation_unknown.Semantic conflict, not a bad PR
Both sides were green on their own branches; the failure only exists after both landed.
1ae7aa8bb4f, "resume an Agent Session History row into a new structured chat")added the test. It passed there.
ba4e79c2504, "fix(runtime): apply the structured-chat setting to every RPCcaller") broke it. The test passes at its parent
c300913f902.Neither branch could see the other, and main runs no typecheck/test gate after merge, so it
landed silently red.
Root cause
Before #18700,
supportsStructuredAgentSessionsonly consultedexperimentalStructuredNativeChatwhenclientKind === 'mobile'. Aruntimeclient wasadmitted on its negotiated
agent-session.structured.v1capability alone. #18700 made the hostsetting one rule for every caller.
The adoption-replay fixture builds a real
OrcaRuntimeServicefrom a settings-service stubthat only supplies
agentDefaultEnv, sogetClientSettings()never reports the setting on.Under the new rule the gate refuses before the create handler runs, and the whole
durable-identity replay body — the thing this test exists to cover — never executes.
Which side I fixed, and why
The test. #18700's rule is the intended product behaviour, and it is documented in the gate
itself: methods that start, extend, retain or read work go behind
requireStructuredHost;only stop/retire methods (
close,cancel,unsubscribe,release) use the cleanup gate.agentSession.createstarts work, so it belongs behind the gate — loosening it would undo theone-rule-for-every-caller fix and re-open the desktop/mobile asymmetry #18700 removed.
Nothing in #19176's test is about admission; it is about republishing from durable identity
after the transcript disappears and account selection drifts. It just needs an admitted caller.
This is also exactly what #18700 already did for the sibling fixture in the same directory —
structured-agent-session-precommit-refusal.test.tsgotgetClientSettings: () => ({ experimentalStructuredNativeChat: true })with the same rationale.This test was simply not in that branch's tree to be updated.
The fix is five lines and touches no production code. Every replay assertion (
acquirecalledonce,
publishStructuredAgentSessionTabcalled twice,selectAccountHomecalled once, the twoagent_session_identity_requiredrefusals) is unchanged and now genuinely runs — the test waspreviously exiting at its first
expect.Verification
pnpm tc— exit 0npx vitest run --config config/vitest.config.ts src/main/runtime/rpc/methods/structured-agent-session-adoption-replay.test.ts— 1 passednpx vitest run --config config/vitest.config.ts src/main/runtime/rpc/methods/ src/main/native-chat/agent-session-wire/ --exclude "**/*.electron.test.ts"— 2021/2022 passed. The one failure,structured-agent-session-handoff-options.test.ts > keeps a picked model through a native to TUI to native round trip, is an unrelatedvi.waitFortimeout under 233-file parallel load; it passes in isolation and touches none of this code.pnpm run check:code-quality:changed— 0 new findings