Repository navigation
fix(web-shell): stop rendering unrecognized daemon events in transcripts #8812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
defbe4f
2736e7f
bc32742
df0b757
98bc115
c21818d
c9224e5
8c9dbb0
1d8a5d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2522,6 +2522,118 @@ describe('daemon UI normalizer — Wave 3/4 event coverage (PR-A)', () => { | |
| expect(events).toEqual([]); | ||
| }); | ||
|
|
||
| it('stamps debugReason on unrecognized daemon events', () => { | ||
| const events = normalizeDaemonEvent( | ||
| envelopeOf('some_future_event', { sessionId: 's1' }), | ||
| ); | ||
|
|
||
| expect(events).toEqual([ | ||
| expect.objectContaining({ | ||
| type: 'debug', | ||
| debugReason: 'unrecognized_event', | ||
| }), | ||
| ]); | ||
| }); | ||
|
|
||
| it('stamps debugReason on unrecognized session_update kinds', () => { | ||
| const events = normalizeDaemonEvent( | ||
| envelopeOf('session_update', { | ||
| update: { sessionUpdate: 'some_future_kind', payload: { a: 1 } }, | ||
| }), | ||
| ); | ||
|
|
||
| expect(events).toEqual([ | ||
| expect.objectContaining({ | ||
| type: 'debug', | ||
| debugReason: 'unrecognized_session_update', | ||
| }), | ||
| ]); | ||
| }); | ||
|
|
||
| it('classifies a session_update with no usable discriminator as malformed', () => { | ||
| // `getSessionUpdatePayload` accepts any record, so these reach the default | ||
| // branch with `kind === undefined`. They are broken frames, not kinds from | ||
| // a newer daemon — marking them unrecognized would let renderers hide the | ||
| // only diagnostic they produce. | ||
| for (const update of [ | ||
| {}, | ||
| { sessionUpdate: 42 }, | ||
| { sessionUpdate: '' }, | ||
| // Truthy but no more usable than an empty string. | ||
| { sessionUpdate: ' ' }, | ||
| ]) { | ||
| expect( | ||
| normalizeDaemonEvent(envelopeOf('session_update', { update })), | ||
| ).toEqual([ | ||
| expect.objectContaining({ | ||
| type: 'debug', | ||
| debugReason: 'malformed_payload', | ||
| }), | ||
| ]); | ||
| } | ||
| }); | ||
|
|
||
| it('stamps debugReason on malformed payloads of known events', () => { | ||
| const events = normalizeDaemonEvent( | ||
| envelopeOf('memory_changed', { scope: 'not-a-scope' }), | ||
| ); | ||
|
|
||
| expect(events).toEqual([ | ||
| expect.objectContaining({ | ||
| type: 'debug', | ||
| debugReason: 'malformed_payload', | ||
| }), | ||
| ]); | ||
| }); | ||
|
|
||
| it('carries debugReason through the reducer onto the transcript block', () => { | ||
| // The normalizer tests above inspect events directly and the Web Shell | ||
| // adapter tests construct blocks by hand, so neither would notice if the | ||
| // reducer dropped the field on the way across. Production blocks would | ||
| // then lose their classification and Web Shell would render raw JSON | ||
| // again with both suites still green. | ||
| const state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| normalizeDaemonEvent( | ||
| envelopeOf('some_future_event', { sessionId: 's1' }), | ||
| ), | ||
| ); | ||
|
|
||
| expect(state.blocks).toEqual([ | ||
| expect.objectContaining({ | ||
| kind: 'debug', | ||
| debugReason: 'unrecognized_event', | ||
| }), | ||
| ]); | ||
|
Comment on lines
+2602
to
+2607
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in bc32742, and I reproduced your mutant first: |
||
| }); | ||
|
|
||
| it('leaves client-dispatched debug blocks without a debugReason', () => { | ||
| // The mirror of the test above, and the invariant that keeps Web Shell's | ||
| // model-switch summary visible. Without it, defaulting the field in | ||
| // `appendStatusBlock` (e.g. `event.debugReason ?? 'unrecognized_event'`) | ||
| // passes every other test in both suites while silently tagging the | ||
| // summary as unrecognized, which Web Shell then filters out. | ||
| const state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| [ | ||
| { | ||
| type: 'debug', | ||
| text: 'Model switched to qwen3-coder-plus', | ||
| source: 'model_switch_summary', | ||
| }, | ||
| ], | ||
| ); | ||
|
|
||
| expect(state.blocks).toHaveLength(1); | ||
| expect(state.blocks[0]).toEqual( | ||
| expect.objectContaining({ | ||
| kind: 'debug', | ||
| source: 'model_switch_summary', | ||
| }), | ||
| ); | ||
| expect(state.blocks[0]).not.toHaveProperty('debugReason'); | ||
| }); | ||
|
|
||
| it('normalizes memory_changed with closed-enum scope + mode', () => { | ||
| const events = normalizeDaemonEvent( | ||
| envelopeOf('memory_changed', { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
DaemonUiDebugReasonis a new public type of@qwen-code/sdk/daemon, but nothing gates that public surface: no test imports it through this barrel, so if the re-export were dropped or the type renamed without updating the barrel, every suite stays green (the only in-repo referents aretypes.tsitself and the two re-export lines, and an esbuild-strippedexport typecan never fail at runtime) while external consumers lose the documented union type — the first signal would be theirtscerror after upgrade. — Failure scenario: a future refactor drops this re-export → all builds and tests pass → the advertised public API silently shrinks until a consumer's compile breaks. (Note: the innerdaemon/uibarrel IS transitively guarded — this outer barrel re-exports from./ui/index.js, so deleting the inner line fails the sdk build; only the outermost export needs an explicit guard.)中文说明
DaemonUiDebugReason是@qwen-code/sdk/daemon新增的公开类型,但这一公开面无任何守护:没有任何测试经公共 barrel 导入它,因此若某次重构丢失该再导出、或类型改名后忘了同步,所有构建与套件依旧全绿(仓内只有types.ts与两行再导出引用它,而export type会被 esbuild 擦除、运行时永不报错),外部消费者则无声地失去文档承诺的联合类型——第一个信号是他们升级后自己的tsc报错。——失败场景:未来的重构删掉了这一行再导出,构建测试全部通过,公开 API 静默缩水,直到消费者的编译中断。(说明:内层daemon/uibarrel 其实已被传递守护——外层 barrel 从./ui/index.js再导出,删除内层那行会让 sdk 构建直接报错;真正需要显式守护的只有这个最外层导出。)— Kimi-K3 via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and the suggested guard turned out not to hold — worth flagging since it affects other type-only guards in this package.
I implemented it as written first, then mutation-checked by deleting the re-export from
src/daemon/index.ts. Bothvitest run(14 passed) andnpm -w packages/sdk-typescript run typecheckstayed green. Two reasons: vitest transpiles through esbuild, which erasesexport typewithout checking it, and this package's tsconfig isinclude: ["src/**/*.ts"]withexclude: [..., "test"], so nothing type-checks the test file at all.expectTypeOfalone cannot fence this surface here.So in bc32742 the union ships as a closed enum value instead, matching
DAEMON_ERROR_KINDS/DAEMON_APPROVAL_MODES:Re-exported as a value through both barrels, with a runtime
toEqualassertion next to theexpectTypeOfyou suggested. Mutation-checked: dropping the outer re-export now fails that test.