mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-19 16:28:28 +00:00
* fix(core): wrap hasOwnProperty.call inside try + add post-abort PTY data assertion Two non-blocking review notes from @tanzhenxin's PR-1 approval (#3842, post-merge follow-up): - **Note 2 (real bug)**: `getShellAbortReasonKind` had `Object.prototype.hasOwnProperty.call(reason, 'kind')` outside the try/catch. `hasOwnProperty.call` triggers the `[[GetOwnProperty]]` Proxy trap (`getOwnPropertyDescriptor` handler). A Proxy whose `getOwnPropertyDescriptor` throws — separate from a throwing `get` trap, which the prior commit already covered — would propagate past the helper, leaving the abort handler's switch on `kind` to throw through `addEventListener` (which doesn't await async listener return values), so the shell process would stay alive instead of being killed on cancel. Moved the descriptor probe inside the same try block as the value read. My own multi-round audit covered six attack vectors but missed this one — only `get` trap throws were considered. The reviewer caught it on first pass. - **Note 3 (test parity)**: the PTY post-promotion handoff test asserted `dispose` was called but never re-invoked the data callback to verify the foreground `onOutputEvent` actually stops firing — the child_process equivalent has that assertion. Mirrored it: emit data AFTER abort by re-invoking the captured `dataCallback` reference, and assert `onOutputEventMock.mock.calls.length` does NOT increase past the moment of promote. Exercises the production `listenersDetached` guard inside the chain callback, which the bare dispose-was-called check didn't. Note 1 from the same review (the `aborted: true + promoted: true` shape forcing PR-2 callers to check `promoted` before `aborted`) is deliberately NOT addressed here — it's a contract simplification that affects PR-2's branching, so it belongs in PR-2 along with the caller-side decision on whether to flip `aborted` for promoted results. Added a TODO upstream in #3831 (PR-2 design) to track. 70 / 70 tests pass (69 baseline + 1 new helper boundary for the throwing-getOwnPropertyDescriptor case). tsc + ESLint clean. * test(core): fix tautological PTY post-promote assertion (audit follow-up) The PTY post-promotion handoff test added in the previous commit copied the child_process equivalent's pattern verbatim — sync \`expect(count).toBe(countAtPromote)\` immediately after dataCallback returns. That works for child_process because its \`handleOutput\` is fully synchronous (sniff → decoder → emit, all on the same call stack), so the count change happens BEFORE the assertion. PTY's \`handleOutput\` is async — \`processingChain.then(...)\` queues a microtask that does the sniff + write + render-then-emit work. The sync assertion captures both \`countAtPromote\` and the post-emit count BEFORE the chain microtask ever runs, so both reads return whatever happened before the assert (typically 0). The test would tautologically pass even if the production \`listenersDetached\` guard were removed — i.e., it didn't actually verify the guard. Restructured to: 1. Drive the PTY through \`simulateExecution\` so \`await handle.result\` forces all queued microtasks (including pre-promote chain items AND the abort handler's drain) to settle. 2. Capture \`eventCountAfterSettle\` once everything has stabilized. 3. Re-invoke the captured \`dataCallback\` with post-promote data, await two more macrotask boundaries to let the new chain item fully run. 4. Assert the count hasn't moved. If the production \`listenersDetached\` guard is removed, the post-promote chain item emits, count increases past \`eventCountAfterSettle\`, and this assertion fails. So the test actually exercises the guard now. Found in self-audit while reviewing my own follow-up commit. Caught because audit was paranoid about *whether the test verifies what it claims to verify*, not just whether it passes. 70 / 70 tests pass; tsc + ESLint clean. * test(core): pin eventCountAfterSettle === 0 in PTY post-promote test The previous fix asserted post-promote count equals the \`eventCountAfterSettle\` baseline, but didn't pin the baseline itself. With the production \`listenersDetached\` guard intact, both halves (pre-promote chain and post-promote chain) suppress emit, so \`eventCountAfterSettle === 0 === post-count\` and the relative comparison is vacuously true. If a future refactor changed the production guard semantics so the pre-promote chain item DID emit (count becomes 1+ after settle), the relative-comparison test would still pass as long as post-promote also emitted the same number — that's a regression the test should catch. Adding \`expect(eventCountAfterSettle).toBe(0)\` makes the contract explicit: once \`listenersDetached\` is set during the abort handler's sync part, BOTH the in-flight chain item (pre-promote) and the future chain item (post-promote) skip emit. Found in another self-audit pass — even after fixing the tautological assertion, the test could still mask certain future regressions. 70 / 70 tests pass; tsc + ESLint clean. * test(core): drop dataCallbackHolder pattern, read mock.calls directly The dataCallbackHolder pattern (capturing the onData callback inside simulateExecution then invoking it after via a closure-shared object) was unnecessary indirection — \`mockPtyProcess.onData.mock.calls[0][0]\` reads the same callback reference whether you read it inside or outside the simulation closure. Vi's mock.calls array is per-mock-instance and beforeEach re-creates mockPtyProcess + .onData freshly, so there's no stale-reference risk in the simpler form. No behavior change in what's tested. 70 / 70 pass. * chore(core): drop in-source @-mention attribution + dead Proxy.get handler Two cosmetic cleanups found in another self-audit pass: - **Helper comment**: removed the parenthetical attribution ("Caught by @tanzhenxin in the PR-1 review; my own audit only covered \`get\` trap throws."). The technical content of the comment — explaining why both the descriptor probe and the value read live inside the try — stands on its own. The reviewer credit lives in commit history / PR description, where it belongs; an in-source @-mention ages poorly (handles change, the relevant person may move on) and doesn't help future readers reason about the code. - **Test Proxy**: \`throwingDescriptorProxy\` declared a \`get()\` handler that always returned \`undefined\`. The descriptor probe throws before the helper ever reaches the value read, so the \`get\` handler is unreachable — dropped it and added a one-line comment explaining why no \`get\` handler is needed for this test. Mirror test (\`throwingReason\` with throwing accessor + \`proxyReason\` with throwing \`get\`) keeps the symmetric "throwing-`get`" coverage. 70 / 70 tests pass; tsc + ESLint clean.
This commit is contained in:
Shaojin Wen
GitHub
parent
93139d0eb0
commit
ddd21b5330
2 changed files with 85 additions and 21 deletions
|
|
@ -679,19 +679,57 @@ describe('ShellExecutionService', () => {
|
|||
// listeners now). Without dataDisposable.dispose() in the abort
|
||||
// handler, the listener-retention bug would let post-promote bytes
|
||||
// leak into the foreground consumer.
|
||||
//
|
||||
// Implementation note: PTY's handleOutput is async (`processingChain`
|
||||
// queues microtasks for headlessTerminal.write callbacks), unlike
|
||||
// child_process's sync handleOutput. Sync `expect` immediately
|
||||
// after emit-then-abort would only see the call count BEFORE chain
|
||||
// items run — both pre and post would read 0 and the assertion
|
||||
// would tautologically pass without exercising the
|
||||
// `listenersDetached` guard. We drive the test using the
|
||||
// `simulateExecution` helper, which awaits `handle.result` — by
|
||||
// the time the result resolves, the abort handler has run its
|
||||
// drain (so all queued chain items have settled) and we can read
|
||||
// the final emit count.
|
||||
const { result } = await simulateExecution(
|
||||
'tail -f /tmp/never.log',
|
||||
(pty, abortController) => {
|
||||
// Data BEFORE promote — fed via the live onData listener so it
|
||||
(pty, ac) => {
|
||||
// Pre-promote data — fed via the live onData listener so it
|
||||
// reaches the foreground onOutputEvent normally.
|
||||
pty.onData.mock.calls[0][0]('pre-promote-data\n');
|
||||
abortController.abort({
|
||||
ac.abort({
|
||||
kind: 'background',
|
||||
shellId: 'bg_test123',
|
||||
} satisfies ShellAbortReason);
|
||||
},
|
||||
);
|
||||
expect(result.promoted).toBe(true);
|
||||
// Snapshot the count after promote settled. Pre-promote chain
|
||||
// item ran AFTER abort set `listenersDetached = true` (chain
|
||||
// items queue at handleOutput time but only execute when their
|
||||
// .then microtask runs, which is after the sync abort dispatch
|
||||
// that set the flag), so the pre-promote emit was already
|
||||
// suppressed by the guard. Asserting `0` here pins both halves
|
||||
// of the contract — pre-promote AND post-promote bytes are both
|
||||
// suppressed once `listenersDetached` is set. Without the guard,
|
||||
// pre-promote's render path would emit a `'data'` event into
|
||||
// `onOutputEventMock` and this would be `>= 1`, failing the
|
||||
// assertion.
|
||||
const eventCountAfterSettle = onOutputEventMock.mock.calls.length;
|
||||
expect(eventCountAfterSettle).toBe(0);
|
||||
// Drive the data callback again after promote: production-side
|
||||
// dataDisposable was disposed, but the mock stub doesn't actually
|
||||
// detach the callback (the disposable returned by `vi.fn()` is a
|
||||
// no-op). Re-invoking via `mock.calls[0][0]` exercises the
|
||||
// production-side `listenersDetached` guard inside the chain
|
||||
// callback, which is the real backstop against post-promote
|
||||
// bytes leaking to the foreground onOutputEvent.
|
||||
mockPtyProcess.onData.mock.calls[0][0]('post-promote-data\n');
|
||||
// Wait one macrotask + a microtask flush to let any chain items
|
||||
// queued by the post-promote dataCallback fully settle.
|
||||
await new Promise((res) => setImmediate(res));
|
||||
await new Promise((res) => setImmediate(res));
|
||||
expect(onOutputEventMock.mock.calls.length).toBe(eventCountAfterSettle);
|
||||
|
||||
// The disposable returned by mockPtyProcess.onData was disposed by
|
||||
// the abort handler — verify by calling .dispose's mock.
|
||||
|
|
@ -1880,6 +1918,27 @@ describe('getShellAbortReasonKind (defensive abort-reason read)', () => {
|
|||
expect(getShellAbortReasonKind(proxyReason)).toBe('cancel');
|
||||
});
|
||||
|
||||
it("returns 'cancel' when the `getOwnPropertyDescriptor` Proxy trap throws", () => {
|
||||
// `Object.prototype.hasOwnProperty.call(reason, 'kind')` triggers
|
||||
// the `[[GetOwnProperty]]` Proxy trap. A Proxy whose
|
||||
// `getOwnPropertyDescriptor` handler throws (separate from the
|
||||
// `get` trap covered by the test above) used to propagate past
|
||||
// the helper because `hasOwnProperty.call` was outside the try.
|
||||
// Now the helper wraps both the descriptor probe and the property
|
||||
// read, so this also falls back to 'cancel'. (No `get` handler on
|
||||
// the proxy: `hasOwnProperty.call` throws before the helper ever
|
||||
// tries to read `kind`.)
|
||||
const throwingDescriptorProxy = new Proxy(
|
||||
{},
|
||||
{
|
||||
getOwnPropertyDescriptor() {
|
||||
throw new Error('getOwnPropertyDescriptor blew up');
|
||||
},
|
||||
},
|
||||
);
|
||||
expect(getShellAbortReasonKind(throwingDescriptorProxy)).toBe('cancel');
|
||||
});
|
||||
|
||||
it("returns 'background' for the canonical happy-path reason", () => {
|
||||
expect(getShellAbortReasonKind({ kind: 'background' })).toBe('background');
|
||||
expect(
|
||||
|
|
|
|||
|
|
@ -62,26 +62,31 @@ const PROMOTE_DRAIN_TIMEOUT_MS = 200;
|
|||
export function getShellAbortReasonKind(
|
||||
reason: unknown,
|
||||
): ShellAbortReason['kind'] {
|
||||
if (
|
||||
reason !== null &&
|
||||
typeof reason === 'object' &&
|
||||
Object.prototype.hasOwnProperty.call(reason, 'kind')
|
||||
) {
|
||||
if (reason !== null && typeof reason === 'object') {
|
||||
try {
|
||||
const kind = (reason as { kind?: unknown }).kind;
|
||||
// INVARIANT — three points must be kept in sync when extending
|
||||
// `ShellAbortReason`:
|
||||
// (1) the discriminated union below (`type ShellAbortReason`),
|
||||
// (2) the value-equality whitelist on this line, and
|
||||
// (3) the `case` arms in both abort-handler switches (the
|
||||
// `default: { const _exhaustive: never = kind; ... }`
|
||||
// statically forces #3 when #1 grows, but #2 has no
|
||||
// compile-time tie to the union; if you forget to extend
|
||||
// it the new variant silently degrades to 'cancel' here
|
||||
// and the `case` you added in #3 is never reached).
|
||||
if (kind === 'background' || kind === 'cancel') return kind;
|
||||
// Both `hasOwnProperty.call` AND the `kind` read are inside the
|
||||
// try: `hasOwnProperty.call` triggers the `[[GetOwnProperty]]`
|
||||
// Proxy trap (`getOwnPropertyDescriptor` handler), so a Proxy
|
||||
// whose `getOwnPropertyDescriptor` throws — separate from a
|
||||
// throwing `get` trap — would otherwise propagate past the
|
||||
// helper.
|
||||
if (Object.prototype.hasOwnProperty.call(reason, 'kind')) {
|
||||
const kind = (reason as { kind?: unknown }).kind;
|
||||
// INVARIANT — three points must be kept in sync when extending
|
||||
// `ShellAbortReason`:
|
||||
// (1) the discriminated union below (`type ShellAbortReason`),
|
||||
// (2) the value-equality whitelist on this line, and
|
||||
// (3) the `case` arms in both abort-handler switches (the
|
||||
// `default: { const _exhaustive: never = kind; ... }`
|
||||
// statically forces #3 when #1 grows, but #2 has no
|
||||
// compile-time tie to the union; if you forget to extend
|
||||
// it the new variant silently degrades to 'cancel' here
|
||||
// and the `case` you added in #3 is never reached).
|
||||
if (kind === 'background' || kind === 'cancel') return kind;
|
||||
}
|
||||
} catch {
|
||||
// Throwing accessor / Proxy trap — fall back to safe kill below.
|
||||
// Throwing accessor / Proxy trap (either `get` or
|
||||
// `getOwnPropertyDescriptor`) — fall back to safe kill below.
|
||||
}
|
||||
}
|
||||
return 'cancel';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue