Skip to content

Commit edb7c2b

Browse files
os-salesclaude
andcommitted
test(cli): pin the attach ORDER, the real writer, and a named control
Three review-adopted fixes to the #15564 pin, none of which change `bin/run.js`'s behaviour. F1 — the order case pinned PRESENCE, not order. `LISTENER ATTACHED after N ms` is true for any N inside the probe's 15 s wait, so an attach moved below `await run(…)` would keep every case green while the entry's own claim ("BEFORE `run()`, and that order is the whole point") had stopped being true. The runtime cases cannot see it — one process, `--version` settles oclif in a few hundred ms, both have happened by the time the poll looks — so the order is now read STRUCTURALLY from the entry's comment-masked source, in a case of its own, and the presence case is renamed to claim only what it pins. F2 — the premise case asserted that `serve.ts` contains `process.stderr.write(`. That file holds about a dozen such sites, so it stayed green even if `printDiagnostic` — the one writer the reproduction ran through (#7915) — moved to `console.error` and stopped being able to crash anything. It now anchors on `printDiagnostic`'s own body, located by symbol and brace-matched, with a length bound so a desynchronised match reds instead of reporting green about some other writer. F3 — the unguarded (positive-control) arm called `removeAllListeners('error')`. Equivalent today, but it measures "no listener at all" rather than "the entry's listener absent"; it now removes the guard BY NAME, and marks the resulting `guard=` reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
1 parent a48dd2a commit edb7c2b

2 files changed

Lines changed: 116 additions & 12 deletions

File tree

packages/cli/test/fixtures/published-entry-stderr-error-probe.mjs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,17 @@ const poll = setInterval(() => {
136136
mark(`LISTENERS count=${process.stderr.listenerCount('error')}`);
137137
if (UNGUARDED) {
138138
// The live positive control — in this process only, never on disk.
139-
process.stderr.removeAllListeners('error');
140-
mark(`ARM unguarded listeners=${process.stderr.listenerCount('error')}`);
139+
//
140+
// ⛔ BY NAME, not `removeAllListeners('error')`. The two are equivalent on
141+
// today's tree, but the control has to measure "the ENTRY's listener is
142+
// absent"; clearing the stream measures "no listener at all", and the day
143+
// anything else attaches one here — a library, a future prologue, node
144+
// itself — that would silently become a different experiment from the one
145+
// the driving case claims to run.
146+
for (const fn of process.stderr.listeners('error')) {
147+
if (fn?.name === LISTENER_NAME) process.stderr.removeListener('error', fn);
148+
}
149+
mark(`ARM unguarded listeners=${process.stderr.listenerCount('error')} guard=${guardAttached()}`);
141150
} else {
142151
mark(`ARM guarded listeners=${process.stderr.listenerCount('error')}`);
143152
}

packages/cli/test/published-entry-stderr-error-listener.e2e.test.ts

Lines changed: 105 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@
6161
* case asserting it would smuggle that reading back in.
6262
*
6363
* The reachability half is not left unheld either — the last case below keeps
64-
* the premise the measurement rests on: `serve` still writes to stderr RAW.
64+
* the premise the measurement rests on: `serve`'s `printDiagnostic`, the one
65+
* writer the reproduction ran through, still writes to stderr RAW.
6566
*/
6667

6768
import { spawn } from 'node:child_process';
@@ -166,7 +167,7 @@ afterAll(() => {
166167
});
167168

168169
describe('the published entry point survives a failed stderr write', () => {
169-
it('attaches its OWN listener before anything of its own can write', () => {
170+
it('has its OWN listener on process.stderr by the time the probe looks', () => {
170171
// The probe reports what it SAW rather than being assumed to have found it:
171172
// `LISTENER ABSENT` is the reading when `bin/run.js` stops attaching one,
172173
// and it is a different sentence from "the probe never ran".
@@ -177,6 +178,13 @@ describe('the published entry point survives a failed stderr write', () => {
177178
// `LISTENER ATTACHED after 20 ms` against a tree with the whole block
178179
// deleted — measured, under the ablation below. The count is carried in
179180
// the markers as evidence and decides nothing.
181+
//
182+
// ⛔ PRESENCE ONLY, and the name says so deliberately. `LISTENER ATTACHED
183+
// after N ms` is true for every N inside the probe's 15 s wait, so this
184+
// case cannot tell an attach at the top of `bin/run.js` apart from one
185+
// moved below `await run(…)`. The ORDER is the next case, read from the
186+
// entry's source, because no reading this process can take will ever
187+
// distinguish them.
180188
expect(
181189
guarded.marks,
182190
`the probe never reached the listener check, so it measured NOTHING — a zero reading, not a pass. Markers:\n${guarded.marks}`,
@@ -188,6 +196,46 @@ describe('the published entry point survives a failed stderr write', () => {
188196
).toContain('LISTENER ATTACHED');
189197
});
190198

199+
it('attaches it ABOVE `await run(…)`, which is the order the entry claims', () => {
200+
// ⭐ Why this case exists at all. The case above pins PRESENCE; a refactor
201+
// that moved the attach BELOW `await run(…)` would keep it — and every
202+
// other runtime case here — green, while the entry's own claim ("⚠️ BEFORE
203+
// `run()`, and that order is the whole point") had quietly stopped being
204+
// true. The probe cannot see the difference: it is one process, `--version`
205+
// settles oclif in a few hundred ms, and by the time the poll can look,
206+
// both have already happened. So the order is read STRUCTURALLY, from the
207+
// entry's source, which is the only place it is visible.
208+
//
209+
// What the order buys, in the entry's words: everything the CLI writes to
210+
// stderr is written from inside `run()`, so a listener installed after it
211+
// has already missed the writes it exists to survive.
212+
//
213+
// ⚠️ Both sites are located BY TEXT, never by line number — the docblocks
214+
// around them move whenever anyone edits them. Comments are masked so the
215+
// paragraphs that DISCUSS this order, several of which name `run()`, can
216+
// never answer for the code.
217+
const entry = maskComments(readFileSync(RUN_JS, 'utf8'));
218+
const attaches = [...entry.matchAll(/process\.stderr\.on\s*\(\s*['"]error['"]/g)].map((m) => m.index ?? -1);
219+
expect(
220+
attaches.length,
221+
`${RUN_JS} no longer attaches any \`error\` listener to process.stderr — a failed stderr write is an uncaught ` +
222+
`exception again on the entry point a customer's install runs (#14858, #15564).`,
223+
).toBeGreaterThan(0);
224+
const runCall = entry.search(/\bawait\s+run\s*\(/);
225+
expect(
226+
runCall,
227+
`${RUN_JS} no longer calls \`await run(\`. That is not automatically a defect, but this case can no longer ` +
228+
`read the order it pins — re-locate both sites by text before trusting it.`,
229+
).toBeGreaterThan(-1);
230+
expect(
231+
Math.max(...attaches),
232+
`${RUN_JS} attaches its \`error\` listener at or after \`await run(\` (last attach at offset ` +
233+
`${Math.max(...attaches)}, \`await run(\` at ${runCall}). Every byte this CLI puts on stderr is written from ` +
234+
`inside \`run()\`, so a listener installed there has already missed what it exists to survive — and no runtime ` +
235+
`case in this file can see that, because both have happened by the time the probe looks.`,
236+
).toBeLessThan(runCall);
237+
});
238+
191239
it("keeps the probe's mirror of the listener name equal to the entry's own", () => {
192240
// The probe cannot import the name — `bin/run.js` runs the CLI at module
193241
// top — so it mirrors it, and a mirror with nothing holding it is how a
@@ -224,7 +272,11 @@ describe('the published entry point survives a failed stderr write', () => {
224272
// `console.error` that swallows its own errors, a node that stopped
225273
// reporting EPIPE here — would be green on the guarded arm forever. This
226274
// arm removes the listener IN THE CHILD's process (nothing on disk), so the
227-
// hazard is re-armed against the same tree in the same run.
275+
// hazard is re-armed against the same tree in the same run. It removes the
276+
// entry's listener BY NAME rather than clearing the stream: the control has
277+
// to be "the entry's guard is gone", and a `removeAllListeners('error')`
278+
// would silently become "nothing is listening at all" the day anything else
279+
// attaches one — a different experiment from the one this case claims.
228280
const evidence = `this child ran ${unguarded.elapsedMs} ms. Markers:\n${unguarded.marks}`;
229281
expect(unguarded.marks, `the unguarded arm never made its write. ${evidence}`).toContain('WROTE');
230282
expect(
@@ -238,22 +290,65 @@ describe('the published entry point survives a failed stderr write', () => {
238290
});
239291

240292
describe('the premise the measurement rests on', () => {
241-
it('keeps a RAW stderr write on a long-lived published command', () => {
293+
it('keeps a RAW stderr write inside `printDiagnostic`, the writer it reproduced on', () => {
242294
// ⚠️ The reachability half, held rather than assumed. `serve` is what
243295
// #15564 reproduced on, and only because two things are true of it at once:
244296
// it writes to stderr WITHOUT `console.error`'s `ignoreErrors` guard, and
245-
// it stays alive across the write. If every raw write here were ever routed
297+
// it stays alive across the write. If that raw write were ever routed
246298
// through `console.error`, the measurement in this file's header would no
247299
// longer describe the tree — re-measure before reading the pins above as
248300
// covering a live hazard.
249301
//
250-
// Comments are masked so the docblocks that DISCUSS `process.stderr.write`
251-
// — including the one at the call site — cannot answer for the call itself.
302+
// ⛔ ANCHORED ON `printDiagnostic`'S OWN BODY, not on the file. `serve.ts`
303+
// holds about a dozen `process.stderr.write(` sites, so a whole-file
304+
// `toContain` stays green while the ONE writer this reproduction rests on
305+
// — the boot diagnostic, #7915, the line the crash was measured at — moves
306+
// to `console.error` and stops being able to crash anything.
307+
//
308+
// ⚠️ Located BY SYMBOL, never by line number. Comments are masked so the
309+
// docblocks that DISCUSS `process.stderr.write` — including the one
310+
// directly above this declaration — cannot answer for the call itself.
252311
const code = maskComments(readFileSync(SERVE_COMMAND, 'utf8'));
312+
const decl = code.indexOf('const printDiagnostic =');
313+
expect(
314+
decl,
315+
`${SERVE_COMMAND} no longer declares \`const printDiagnostic =\`. That is not automatically a defect, but it ` +
316+
`is the writer #15564 measured the crash on, so re-locate it by symbol and re-point this case rather than ` +
317+
`widening it back to the whole file.`,
318+
).toBeGreaterThan(-1);
319+
// Brace-matched over comment-masked source: the body is one statement and
320+
// carries no braces of its own today, and the length bound below is what
321+
// keeps a desynchronised match from reporting green against some other
322+
// writer further down the file.
323+
const open = code.indexOf('{', decl);
324+
let depth = 0;
325+
let end = -1;
326+
for (let i = open; i >= 0 && i < code.length; i += 1) {
327+
if (code[i] === '{') depth += 1;
328+
else if (code[i] === '}') {
329+
depth -= 1;
330+
if (depth === 0) {
331+
end = i;
332+
break;
333+
}
334+
}
335+
}
336+
const body = end > open ? code.slice(open, end + 1) : '';
337+
expect(
338+
body.length,
339+
`could not read \`printDiagnostic\`'s body out of ${SERVE_COMMAND} — the brace match ran away (${body.length} ` +
340+
`chars), so this case measured NOTHING. Re-locate the declaration before trusting any verdict from it.`,
341+
).toBeGreaterThan(0);
342+
expect(
343+
body.length,
344+
`\`printDiagnostic\`'s body read back as ${body.length} chars, far past the one statement it is — the brace ` +
345+
`match lost sync, so a hit below would be about some other writer in ${SERVE_COMMAND}.`,
346+
).toBeLessThan(1000);
253347
expect(
254-
code,
255-
`${SERVE_COMMAND} no longer writes to stderr directly. That is not automatically a defect, but it ` +
256-
`removes the reproduction #15564 measured, so the header above needs re-measuring rather than trusting.`,
348+
body,
349+
`\`printDiagnostic\` in ${SERVE_COMMAND} no longer writes to stderr RAW. That is not automatically a defect, ` +
350+
`but \`console.error\` carries \`ignoreErrors\` and cannot crash this process at any size, so it removes the ` +
351+
`reproduction #15564 measured — the header above needs re-measuring rather than trusting.`,
257352
).toContain('process.stderr.write(');
258353
});
259354
});

0 commit comments

Comments
 (0)