From e9b180685b8070c2f01f96fc298970f88100dc4a Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 21 Sep 2026 00:36:51 -0400 Subject: [PATCH 1/3] feat(mobile): render Mermaid diagrams on the page from one deferred engine artifact (OTA phase C, C7.10 B) (#21871) * test(mobile): measure mermaid rendered in the page Red-first for C7.10 item B. The check mounts the real web sibling in chromium and webkit under the shipped shell CSP and asks four things of it: that a diagram renders with zero policy violations and zero eval / new Function calls, that the SVG is the native buildHtml's own output once the diagram id and xmlns:xlink are normalised away, that a hostile diagram lands inert, and that a source change, an unmount and a remount leave exactly one SVG and no listener of the first mount. The equality oracle is buildHtml itself, bundled for Node behind a Proxy stub for its native imports and served as its own document in the same browser, so neither side of the comparison is retyped. All eight cases fail on this commit: the sibling is still the labelled source box. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): fence the session download rather than its module list Ruling 28. mobileWebAppRouteClosure reads metafile.inputs, which holds dynamically imported modules under splitting: true exactly as it does under splitting: false, so it cannot say "on demand" about anything: an on-demand mermaid moves the session route's module list 4320 -> 6362 while its download does not move at all. So the fence moves to entryStaticClosure. The new helper walks the emitted chunks from the output the route's own module landed in and follows import-statement edges only, and hands back both halves, because mermaid's absence from the download is only a measurement while its 66 files are present in the deferred half. The module list's new total is recorded in the docstring with its reason and asserted beside the engine's own file count, which moves only when the pinned mermaid version does. Red on this commit: no mermaid in the closure yet. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): render mermaid in the page The web sibling stops being a source box. mermaid is a browser library, so the page imports it inside the render effect and draws the diagram in this document: no WebView, no 3.7 MB engine string, and nothing of the engine downloaded by a session with no diagram on it. What replaces the sandbox is mermaid's own securityLevel: 'strict', which runs its serialized SVG through DOMPurify. The native path's escaping has no analogue here and needs none, because the source is a JS string argument rather than text spliced into an inline script. Measured in both engines: a script in a label, a , an onerror and a javascript: click all land inert. The configuration is now one object both hosts read, so the theme cannot drift between the page and the phone; buildHtml serializes it instead of holding a second copy. It gains suppressErrorRendering, because mermaid otherwise draws its own error diagram into a temporary element and leaves that element behind when it rethrows -- an orphan SVG on the page, and on native a diagram the component is about to replace with the source box anyway. The dispose clears the host on unmount and on a source change; the id is a useId, because mermaid writes it into the stylesheet inside the SVG and it has to be a CSS identifier. Also re-records the closure total the previous commit pinned: with the real component the session route's module list is 6376, not the design probe's 6362, and the reason is in that file's docstring. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): budget the deferred engine's chunks apart from the routes Putting mermaid on the page took the app bundle from 69 emitted scripts to 172, and the asset budget failed: 215 assets against a ceiling of 115. The cause is not a page split running away, which is what that ceiling is for -- it is that mermaid lazily imports each of its own diagram types, so one import() lands 103 scripts no route count predicts. So the ceiling gains a second term, named and measured (172 scripts with mermaid against 69 with it aliased to a stub, at 11.17.2), rather than the route term being raised to cover it. A page split running away still fails on the route term, and the failure still says which of the two grew. The consequence is worth reading twice: the derived ceiling has to stay inside the 256 assets the shell will load, and with 42 images it now crosses that at 24 routes instead of 50. The bundle is at 215 today with 14 routes, so there is room for about ten more routes before a green build produces a manifest no phone will open. Measured by the config/scripts suite failing on this head, not predicted. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): pre-bundle the page's mermaid into one artifact import('mermaid') from inside the app bundle emitted 103 scripts, not one: mermaid lazily imports each of its own diagram types and esbuild splits along those boundaries. Every one of those scripts sits inside the OTA generation the phone has already downloaded, so the split moved no bytes over the wire and spent 103 of the 256 manifest assets the shell will load -- which is the scarce resource here, and the reason the previous commit had to invent a second ceiling term. So a sibling generator bundles the package into one ESM module beside the WebView engine it already builds, emitted by the same postinstall run, gitignored and lint-ignored with the others. The page imports that artifact on demand instead, through a loader whose return type names the two calls the component makes -- checked against the artifact's own inferred export rather than cast to it. Measured, at 14 routes: emitted scripts 172 -> 69 (68 with no deferred engine at all) manifest assets 215 -> 112 (111 with none) session modules 6376 -> 4323 (+3 over main: config, loader, artifact) chunks fetched for one graph TD 27 -> 1 bytes fetched 837,530 -> 3,482,965 The static-closure fence is unchanged in meaning and now reads on the artifact: absent from every chunk the route reaches by an import statement, present in the deferred half. The rendered SVG is byte-for- byte what it was, so the equality against the native document still holds on both engines. Also adds the diagram to the webview-consumers list, which is what that list means: its native component imports the package and its sibling is what the builder resolves instead. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * revert(mobile): drop the deferred-engine ceiling term, keep the control With the engine pre-bundled into one artifact the bundle emits 69 scripts at 14 routes against the route term's 72, so the second term this series added has nothing left to do and the route count is the only term again. mobileWebAppBundleMaxChunks and the asset ceiling derived from it are back to what main has; the shell's 256 assets are crossed at 50 routes again rather than at 24. What stays is why. A ceiling raised to admit 172 scripts would have admitted any split at all, so the budget test gains the control that holds the line: the single-artifact count passes the ceiling and the lazily-chunked count fails it, both measured at 14 routes, with mermaid named as what produced the second. Red before the term came out: the control failed asserting 172 > 175. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): keep build output out of the raw-request-port census The census walks mobile/src for AST reaches into the unvalidated request port, and the pre-bundled mermaid artifact is the first generated file under src that is executable code rather than a string literal. Two of its own vendored dependencies contain the token `sendRequest`, so the walk read minified third-party code as a new call site and asked for an inventory line nobody can ever migrate. So `*.generated.ts` joins node_modules and test files in that file's stated list of what it does not scan, with the reason. The scripts that emit those artifacts are ordinary source and are still scanned, which is where a real reach would be. Two halves to the new control, because a filter that skipped everything would satisfy either alone: nothing generated is left in the scan, and the matcher still finds the port when handed one line of code. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): escape the shared config into the native inline script buildHtml spliced JSON.stringify(MERMAID_DIAGRAM_CONFIG) straight into the inline ` in a label, an `onerror` attribute and a `javascript:` click. + * + * The native path escapes `<`, `>` and the line separators because the source is spliced into an + * inline `"]' + +/** Not a diagram in any grammar mermaid has, so `render` rejects and the component falls back. */ +const BROKEN = 'graph TD\n A[[[unclosed' + +const ENGINES = [ + { + name: 'chromium', + // CI runs this against the runner's Google Chrome rather than paying for a download, the same + // override shape as every other render check here. + launch: () => { + const executablePath = process.env.ORCA_MOBILE_WEB_RENDER_BROWSER + return chromium.launch({ headless: true, ...(executablePath ? { executablePath } : {}) }) + } + }, + { name: 'webkit', launch: () => webkit.launch({ headless: true }) } +] + +/** + * The page under test: the real component, mounted by the real React, with a handle on its props. + * + * Not a re-implementation of what the component does — the dispose, the fallback and the remount + * are the behaviour under test, and a probe that called `mermaid.render` itself would prove + * nothing about any of them. + */ +const PAGE_ENTRY = ` +import { createElement, useEffect, useState } from 'react' +import { createRoot } from 'react-dom/client' +import { MermaidDiagram } from './MermaidDiagram' + +function Harness() { + const [state, setState] = useState({ mounted: false, source: '' }) + useEffect(() => { + globalThis.__orcaMermaidSet = setState + document.body.setAttribute('data-ready', 'yes') + }, []) + return state.mounted ? createElement(MermaidDiagram, { source: state.source, base: 15 }) : null +} + +createRoot(document.getElementById('root')).render(createElement(Harness)) +` + +/** + * A stand-in for React, React Native and `react-native-webview`, so the native module can be + * bundled for Node to get its HTML. + * + * `buildHtml` is a pure function of the source and the theme, but it lives beside a component + * whose other imports are all native. CommonJS with a Proxy rather than a list of named exports: + * what that component reaches for is its own business, and none of it is called here. + */ +const IMPORT_STUB = ` +const identity = (value) => value +module.exports = new Proxy( + { + StyleSheet: { create: (styles) => styles, hairlineWidth: 1 }, + memo: identity, + default: identity + }, + { get: (target, key) => (key in target ? target[key] : identity) } +) +` + +const bundles = mobileWebAppDependenciesPresent() +const describeMermaid = bundles ? describe : describe.skip + +let scratch = null +let server = null +let origin = null + +/** The native document's own render of `FIXTURE`, built from `buildHtml` and served as a page. */ +async function buildNativeDocument(outDir) { + const stubPath = join(scratch, 'import-stub.cjs') + await writeFile(stubPath, IMPORT_STUB, 'utf8') + const nativeHtmlModule = join(scratch, 'native-html.mjs') + await esbuild.build({ + absWorkingDir: mobileDir, + stdin: { + contents: "export { buildHtml } from './MermaidDiagram'\n", + resolveDir: diagramDir, + loader: 'ts', + sourcefile: 'native-html-entry.ts' + }, + bundle: true, + format: 'esm', + platform: 'node', + outfile: nativeHtmlModule, + target: ['node20'], + jsx: 'automatic', + logLevel: 'silent', + nodePaths: [join(mobileDir, 'node_modules')], + // `resolveExtensions` is left at its default here, with no `.web.*`, so `./MermaidDiagram` + // resolves to the file the phone builds rather than to the sibling under test. + alias: { + react: stubPath, + 'react/jsx-runtime': stubPath, + 'react-native': stubPath, + 'react-native-webview': stubPath + }, + define: { __DEV__: 'false', 'process.env.NODE_ENV': '"production"' } + }) + const { buildHtml } = await import(pathToFileURL(nativeHtmlModule).href) + await writeFile(join(outDir, 'native.html'), buildHtml(FIXTURE), 'utf8') + // The same document for a diagram that throws, because the shared config the page introduced + // reaches the phone too and one of its keys changes what mermaid does on that path. + await writeFile(join(outDir, 'native-broken.html'), buildHtml(BROKEN), 'utf8') +} + +beforeAll(async () => { + if (!bundles) { + return + } + // Inside mobile/ rather than the system temp dir: the entry resolves the component beside it, + // and esbuild resolves a bare specifier from the importer upward. + await mkdir(join(mobileDir, '.tmp'), { recursive: true }) + scratch = await mkdtemp(join(mobileDir, '.tmp', 'mermaid-render-')) + const outDir = join(scratch, 'bundle') + await mkdir(outDir, { recursive: true }) + await esbuild.build({ + absWorkingDir: mobileDir, + stdin: { + contents: PAGE_ENTRY, + resolveDir: diagramDir, + loader: 'ts', + sourcefile: 'mermaid-check.ts' + }, + bundle: true, + // esm with splitting, because `import('mermaid')` has to be a chunk the browser fetches when + // the diagram renders. An iife would inline the engine into the entry, which is the one shape + // this item exists to avoid. + format: 'esm', + splitting: true, + // Minified, like the bundle the shell serves: the chunk count and the bytes one render fetches + // are numbers this file records, and an unminified bundle records neither. + minify: true, + outdir: outDir, + entryNames: 'mermaid-check', + chunkNames: 'chunk-[hash]', + target: ['es2022'], + jsx: 'automatic', + logLevel: 'silent', + nodePaths: [join(mobileDir, 'node_modules')], + alias: { 'react-native': 'react-native-web' }, + // The web sibling is what the page runs; the native file reaches a WebView that a browser + // renders as a line of text. + resolveExtensions: ['.web.tsx', '.web.ts', '.web.js', '.tsx', '.ts', '.js'], + define: { __DEV__: 'false', 'process.env.NODE_ENV': '"production"' } + }) + await writeFile( + join(outDir, 'index.html'), + '
' + + '' + ) + await buildNativeDocument(outDir) + const served = await createBundleServer({ outDir, cspHeader: await readShellCsp() }) + server = served.server + origin = served.origin +}, 600_000) + +afterAll(async () => { + server?.close() + if (scratch) { + // This run's directory only: `mobile/.tmp` is a shared ignored root and another suite may hold + // one of its own. + await rm(scratch, { recursive: true, force: true }) + } +}) + +/** + * Every `eval` and `new Function` attempted on the page, with the stack that asked for it. + * + * `script-src 'self'` carries no `'unsafe-eval'`, so a JIT call raises a violation too — but a + * library that catches its own `EvalError` and takes a slower path would leave that violation + * looking like noise from elsewhere. The stack is what names the caller, and it has to, because + * Playwright evaluates every one of this file's own page functions through `eval`: the calls on + * this list are mostly the harness's, and only the ones from the bundle's scripts are the page's. + */ +function installJitRecorder() { + globalThis.__orcaJit = [] + const record = (kind, source) => { + // Line 0 is the error's own header and line 1 is this recorder; the rest is whoever asked. + const stack = (new Error('jit').stack ?? '').split('\n').slice(2).join(' | ') + globalThis.__orcaJit.push({ kind, source: String(source).slice(0, 60), stack }) + } + // oxlint-disable-next-line eslint/no-eval -- SAFETY: the recorder holds the real eval so it can count and forward calls; naming it is this function's whole purpose. + const realEval = globalThis.eval + // oxlint-disable-next-line eslint/no-eval -- SAFETY: replacing eval with a counting wrapper is the measurement, not a call. + globalThis.eval = function (source) { + record('eval', source) + return realEval.call(globalThis, source) + } + const RealFunction = globalThis.Function + function PatchedFunction(...args) { + record('Function', args.map((one) => String(one).slice(0, 40)).join('|')) + return RealFunction.apply(this, args) + } + PatchedFunction.prototype = RealFunction.prototype + globalThis.Function = PatchedFunction +} + +/** + * The JIT calls that came from the bundle rather than from the harness driving it. + * + * `__orcaJit` being non-empty is the precondition: an attribution filter over a list nothing ever + * wrote to answers "none from the page" for a recorder that was never installed. + */ +async function pageJitCalls(page) { + const all = await page.evaluate(() => globalThis.__orcaJit) + expect(all.length).toBeGreaterThan(0) + return all.filter((one) => /mermaid-check\.js|\/chunk-/.test(one.stack)) +} + +/** What the component has on the page: its frame, the SVG under it, and every SVG anywhere. */ +function readDiagram() { + const frame = document.querySelector('[data-testid="mermaid-diagram"]') + const svg = frame?.querySelector('svg') ?? null + return { + framed: frame !== null, + inFrame: frame ? frame.querySelectorAll('svg').length : -1, + // Every SVG in the document, not only the framed one: mermaid renders into a temporary + // element of its own, and an orphan left in the body is invisible to a count under the host. + inDocument: document.querySelectorAll('svg').length, + sourceBox: document.querySelector('[data-testid="mermaid-diagram-source"]') !== null, + id: svg?.id ?? null, + html: svg?.outerHTML ?? null, + text: frame?.textContent ?? null + } +} + +async function openPage(browser) { + const page = await browser.newPage({ viewport: { width: 390, height: 844 } }) + const consoleErrors = [] + page.on('console', (message) => { + if (message.type() === 'error') { + consoleErrors.push(message.text()) + } + }) + page.on('pageerror', (error) => consoleErrors.push(`pageerror: ${error.message}`)) + await page.addInitScript(installJitRecorder) + await page.addInitScript(installCspViolationRecorder) + await page.addInitScript(installListenerRecorder) + await page.goto(`${origin}/`, { waitUntil: 'domcontentloaded' }) + await page.waitForFunction(() => document.body.dataset.ready === 'yes') + return { page, consoleErrors } +} + +/** + * Drives the harness and waits for the component to settle into a diagram or a fallback. + * + * `contains` is for a source change, where "an SVG is present" is already true of the diagram being + * replaced: naming a label only the new diagram carries is what makes the wait about the new one. + */ +async function show(page, source, contains = null) { + await page.evaluate( + (next) => globalThis.__orcaMermaidSet({ mounted: true, source: next }), + source + ) + await page.waitForFunction((needle) => { + const frame = document.querySelector('[data-testid="mermaid-diagram"]') + if (frame === null) { + return false + } + const svg = frame.querySelector('svg') + if (needle !== null) { + return (svg?.textContent ?? '').includes(needle) + } + return svg !== null || document.querySelector('[data-testid="mermaid-diagram-source"]') !== null + }, contains) +} + +async function unmount(page) { + await page.evaluate(() => globalThis.__orcaMermaidSet({ mounted: false, source: '' })) + await page.waitForFunction( + () => document.querySelector('[data-testid="mermaid-diagram"]') === null + ) +} + +/** + * The two strings, with the only two differences the design measured taken out: the diagram id, + * which each host generates its own way, and the `xmlns:xlink` the native serialization adds. + * + * The id is read off the element rather than matched by a pattern, so a host that changes its id + * scheme normalises correctly instead of comparing an unreplaced string. + */ +function normaliseSvg(html, id) { + return html + .split(id) + .join('ID') + .replace(/ xmlns:xlink="[^"]*"/g, '') +} + +/** + * The native document, loaded in the same browser, with the host it posts to standing in. + * + * `window.ReactNativeWebView` is what the WebView injects; the document's `post` is a no-op + * without it, so the message that drives the component's fallback would be unobservable. Recorded + * as a list because the two outcomes are told apart by what it posts: a height, or `error`. + */ +async function readNativeDocument(browser, file) { + const page = await browser.newPage({ viewport: { width: 390, height: 844 } }) + try { + await page.addInitScript(() => { + globalThis.__orcaNativePosts = [] + globalThis.ReactNativeWebView = { + postMessage: (message) => globalThis.__orcaNativePosts.push(String(message)) + } + }) + await page.goto(`${origin}/${file}`, { waitUntil: 'load' }) + await page.waitForFunction(() => globalThis.__orcaNativePosts.length > 0, null, { + timeout: 120_000 + }) + return await page.evaluate(() => { + const svg = document.querySelector('#c svg') + return { + posts: globalThis.__orcaNativePosts, + svgs: document.querySelectorAll('svg').length, + html: svg?.outerHTML ?? null, + id: svg?.id ?? null + } + }) + } finally { + await page.close() + } +} + +describeMermaid( + 'mermaid on the page', + () => { + for (const engine of ENGINES) { + describe(engine.name, () => { + let browser = null + + beforeAll(async () => { + browser = await engine.launch() + }, 180_000) + + afterAll(async () => { + await browser?.close() + }) + + it('paints the diagram the native document paints, with no violation and no JIT', async () => { + const { page, consoleErrors } = await openPage(browser) + const fetched = [] + page.on('response', (response) => fetched.push(response.url())) + try { + await show(page, FIXTURE) + const shown = await page.evaluate(readDiagram) + expect(shown.inFrame).toBe(1) + // The precondition the absences below need: a diagram rendered, and it is mermaid's. + expect(shown.html).toContain('aria-roledescription="flowchart-v2"') + expect(await pageJitCalls(page)).toEqual([]) + expect(await page.evaluate(() => globalThis.__orcaCspViolations)).toEqual([]) + expect(consoleErrors).toEqual([]) + // A CSS identifier, because mermaid writes `#` into the stylesheet it puts inside + // the SVG; an id spelled `«r0»` would leave every one of those rules inert. + expect(shown.id).toMatch(/^[A-Za-z_][\w-]*$/) + // On demand, from here, and in one piece: the engine arrived as exactly one chunk the + // render asked for, and nothing was fetched off this origin. The count is the claim — + // importing the package rather than the artifact fetched 27 here and emitted 103 in + // the app bundle, which is what spends the shell's asset budget. + expect(fetched.filter((url) => url.includes('/chunk-'))).toHaveLength(1) + expect(fetched.filter((url) => !url.startsWith(origin))).toEqual([]) + + const native = await readNativeDocument(browser, 'native.html') + expect(normaliseSvg(shown.html, shown.id)).toBe(normaliseSvg(native.html, native.id)) + } finally { + await page.close() + } + }, 600_000) + + it('leaves the native document reporting a diagram that throws, with nothing drawn', async () => { + // The page's shared config reaches the phone as well, and `suppressErrorRendering` is a + // key the native path did not have before it. What must not change is that the component + // above the WebView still hears about a diagram that throws: `run` rethrows, the + // document's own catch posts `error`, and the component swaps in the source box. + const broken = await readNativeDocument(browser, 'native-broken.html') + expect(broken.posts).toEqual(['error']) + // And what the key does change: mermaid draws no error diagram of its own, so the + // document is empty behind the fallback rather than showing a diagram for a moment. + expect(broken.svgs).toBe(0) + + // The control, the same document for a diagram that parses: a height, not `error`. + const rendered = await readNativeDocument(browser, 'native.html') + expect(rendered.posts).not.toContain('error') + expect(Number(rendered.posts[0])).toBeGreaterThan(0) + expect(rendered.svgs).toBe(1) + }, 600_000) + + it('leaves one SVG across a source change, an unmount and a remount', async () => { + const { page, consoleErrors } = await openPage(browser) + const listeners = () => page.evaluate(() => globalThis.__orcaListeners.snapshot()) + try { + const beforeAnyMount = await listeners() + await show(page, FIXTURE) + const first = await page.evaluate(readDiagram) + await unmount(page) + // A first mount installs listeners no dispose can take off, and they are not a leak: + // mermaid's own `window` `load` (inert under `startOnLoad: false`, and the module's + // rather than the mount's) and react-native-web's responder system, which arms itself + // on the first `View` the page renders. So the baseline a per-mount leak would move is + // the snapshot after one whole cycle, not the one before it — with mermaid's named, + // because a cycle that installed nothing would make the comparison below vacuous. + const afterEngineLoaded = await listeners() + expect( + Object.keys(afterEngineLoaded).filter((key) => !(key in beforeAnyMount)) + ).toContain('window load') + + await show(page, FIXTURE) + await show(page, SECOND, 'Three') + const changed = await page.evaluate(readDiagram) + // One in the frame and one in the document: a diagram the first source left behind + // would be the second, wherever it hung. + expect(changed.inFrame).toBe(1) + expect(changed.inDocument).toBe(1) + expect(changed.html).not.toBe(first.html) + + await unmount(page) + const gone = await page.evaluate(readDiagram) + expect(gone.framed).toBe(false) + expect(gone.inDocument).toBe(0) + // Two mounts and a source change later, the page is listening to exactly what it was + // after the first of them. A mount that registered anything of its own would show up + // here as the third. + expect(await listeners()).toEqual(afterEngineLoaded) + + await show(page, FIXTURE) + const again = await page.evaluate(readDiagram) + expect(again.inFrame).toBe(1) + expect(again.inDocument).toBe(1) + expect(normaliseSvg(again.html, again.id)).toBe(normaliseSvg(first.html, first.id)) + expect(await page.evaluate(() => globalThis.__orcaCspViolations)).toEqual([]) + expect(consoleErrors).toEqual([]) + } finally { + await page.close() + } + }, 600_000) + + it('renders a hostile diagram inert', async () => { + const { page } = await openPage(browser) + try { + await show(page, HOSTILE) + const inert = await page.evaluate(() => { + const frame = document.querySelector('[data-testid="mermaid-diagram"]') + return { + rendered: frame?.querySelector('svg') !== null, + scripts: frame.querySelectorAll('script').length, + inlineHandlers: [...frame.querySelectorAll('*')].filter((element) => + [...element.attributes].some((attribute) => attribute.name.startsWith('on')) + ).length, + javascriptHrefs: [...frame.querySelectorAll('[*|href]')] + .map( + (element) => + element.getAttribute('href') ?? element.getAttribute('xlink:href') ?? '' + ) + .filter((href) => href.toLowerCase().startsWith('javascript:')).length, + pwned: globalThis.__pwned ?? null + } + }) + // Rendered rather than refused, which is the whole point: the payload is carried into + // the document as data and does nothing there. + expect(inert.rendered).toBe(true) + expect(inert.scripts).toBe(0) + expect(inert.inlineHandlers).toBe(0) + expect(inert.javascriptHrefs).toBe(0) + expect(inert.pwned).toBeNull() + expect(await pageJitCalls(page)).toEqual([]) + } finally { + await page.close() + } + }, 600_000) + + it('falls back to the source when the diagram throws, and recovers from it', async () => { + const { page } = await openPage(browser) + try { + await show(page, BROKEN) + const failed = await page.evaluate(readDiagram) + expect(failed.sourceBox).toBe(true) + expect(failed.inFrame).toBe(0) + expect(failed.text).toContain('unclosed') + // The fallback is a state of this component, not a page with a diagram left on it: + // mermaid draws its own error diagram unless it is told not to. + expect(failed.inDocument).toBe(0) + + // The control: the same component, the same mount, a diagram that parses. Named, + // because the fallback it is replacing already satisfies a bare settle. + await show(page, FIXTURE, 'Ship it') + const recovered = await page.evaluate(readDiagram) + expect(recovered.sourceBox).toBe(false) + expect(recovered.inFrame).toBe(1) + } finally { + await page.close() + } + }, 600_000) + }) + } + }, + 3_600_000 +) diff --git a/config/scripts/mobile-web-app-route-chunk-closure.mjs b/config/scripts/mobile-web-app-route-chunk-closure.mjs new file mode 100644 index 00000000000..b4cee5aaf27 --- /dev/null +++ b/config/scripts/mobile-web-app-route-chunk-closure.mjs @@ -0,0 +1,46 @@ +import { basename, join, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' +import * as esbuild from 'esbuild' +import { entryStaticClosure, mobileWebAppBuildOptions } from './build-mobile-web-app-bundle.mjs' +import { collectMobileWebAppRoutes } from './mobile-web-app-route-manifest.mjs' + +const mobileDir = fileURLToPath(new URL('../../mobile', import.meta.url)) + +/** + * What a browser must download before one page route can paint, and what it may defer. + * + * `mobileWebAppRouteClosure` answers a different question: it reads `metafile.inputs`, which holds + * every module an entry reaches including the ones behind `import()`, so it cannot say "on + * demand" about anything (ruling 28). This walks the emitted chunks instead, from the output the + * route's own module landed in, and follows only `import-statement` edges — which is exactly where + * a dynamic import stops being part of the download. + * + * Both halves come back, because the interesting claim is always a difference: `staticInputs` is + * what the route costs to open, `deferredInputs` is everything else the bundle emitted, and a + * module absent from the first is only meaningful while it is present in the second. + */ +export async function mobileWebAppRouteChunkClosure(routeModule) { + const routes = await collectMobileWebAppRoutes(join(mobileDir, 'app')) + const { metafile } = await esbuild.build({ + ...mobileWebAppBuildOptions(routes), + metafile: true, + write: false + }) + const routePath = resolve(mobileDir, routeModule) + const owner = Object.entries(metafile.outputs).find(([, output]) => + Object.keys(output.inputs ?? {}).some((input) => resolve(mobileDir, input) === routePath) + ) + if (!owner) { + throw new Error(`[mobile-web-app-route-chunk-closure] ${routeModule} reached no output`) + } + const reached = entryStaticClosure(metafile, owner[0]) + const inputsOf = (outputs) => + outputs.flatMap((output) => Object.keys(metafile.outputs[output]?.inputs ?? {})) + const every = Object.keys(metafile.outputs).filter((output) => output.endsWith('.js')) + return { + routeChunk: basename(owner[0]), + staticChunks: [...reached].map((output) => basename(output)), + staticInputs: inputsOf([...reached]), + deferredInputs: inputsOf(every.filter((output) => !reached.has(output))) + } +} diff --git a/config/scripts/mobile-web-app-session-terminal-closure.test.mjs b/config/scripts/mobile-web-app-session-terminal-closure.test.mjs index 34d51a42244..08ab772d7e2 100644 --- a/config/scripts/mobile-web-app-session-terminal-closure.test.mjs +++ b/config/scripts/mobile-web-app-session-terminal-closure.test.mjs @@ -3,6 +3,7 @@ import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' import { mobileWebAppRouteClosure } from './build-mobile-web-app-bundle.mjs' import { mobileWebAppDependenciesPresent } from './mobile-web-app-bundle-dependencies.mjs' +import { mobileWebAppRouteChunkClosure } from './mobile-web-app-route-chunk-closure.mjs' import { textInputFontSizeOffenders, unresolvedTextInputStyles @@ -32,6 +33,32 @@ import { * -47,255 at 51ae7b1b03 and -55,561 at 0ce0fc99a2. They differ because C7.1's own round-1 fold * deleted `URL_TAP_WEBVIEW_JS` from a module only the page's component brings into this closure, * so the saving lands on the after side and no base can show it. + * + * Then C7.10 item B put mermaid on the page, and the module list moved again: + * + * modules 4320 -> 4323 (+3) + * local modules 970 -> 973 (+3) + * + * Three modules: the configuration both hosts read, the loader, and the pre-bundled engine the + * loader imports on demand. The engine's own 66 files and the d3, dagre, katex and cytoscape trees + * under them are inside that one artifact rather than in this graph, which is why the count barely + * moves. Importing the package here instead read +2,056 and emitted 103 scripts, a package + * splitting along its own lazy diagram-type boundaries — every one of them inside the OTA generation + * the phone had already downloaded, so the split moved no bytes and spent 103 of the 256 manifest + * assets the shell will load. One artifact costs one script and one module. + * + * What the generation weighs, because every chunk ships in it whether or not a phone ever fetches + * one: the built bundle is 8,016,714 bytes across 112 assets, against the 9 MiB ceiling in + * `verify-mobile-web-app-bundle.mjs`. That is 84.9% of it, with 1,420,470 bytes left for the rest + * of C7.10 and for C7.7. Before item B the same bundle was 4,539,090 bytes, and the engine is the + * difference — deferring it defers evaluation and a fetch, never the download. + * + * `mobileWebAppRouteClosure` reads `metafile.inputs`, which holds dynamically imported modules + * under `splitting: true` just as it does under `splitting: false`, so it cannot express "on + * demand" about anything. Ruling 28: the fence for this route is `entryStaticClosure`, which + * follows `import-statement` edges only, and the module list's total is a recorded number rather + * than a budget. It moves whenever main adds a module this route reaches, and is re-recorded rather + * than argued with. */ const projectDir = fileURLToPath(new URL('../..', import.meta.url)) @@ -75,6 +102,16 @@ const XTERM_PACKAGES = ['@xterm/xterm', '@xterm/addon-unicode11', '@xterm/addon- */ const EXPECTED_OFFENDERS = 0 +/** The deferred engine, as the page reaches it: one artifact, not the package's own file tree. */ +const MERMAID_PAGE_ENGINE = 'src/components/pr-sidebar/mermaid-page-engine.generated.ts' +const MERMAID_PACKAGE = 'node_modules/mermaid/' + +/** The module list with mermaid on the page, recorded at the base in the docstring above. */ +const MODULES_WITH_MERMAID = 4323 + +const artifactModules = (inputs) => inputs.filter((input) => input.includes(MERMAID_PAGE_ENGINE)) +const packageModules = (inputs) => inputs.filter((input) => input.includes(MERMAID_PACKAGE)) + const bundles = mobileWebAppDependenciesPresent() const describeClosure = bundles ? describe : describe.skip @@ -103,6 +140,27 @@ describeClosure( expect(documentModules).toContain('src/terminal/document/page-document-modules.ts') }, 300_000) + it('reaches the engine as one deferred module and never as part of the download', async () => { + const { modules } = await mobileWebAppRouteClosure(SESSION_ROUTE) + // The engine is here, as the one artifact the loader imports. + expect(artifactModules(modules)).toHaveLength(1) + // And the package's own file tree is not, anywhere: it is inside that artifact. Meaningful + // only beside the line above, which is why the two sit together. + expect(packageModules(modules)).toEqual([]) + expect(modules).toHaveLength(MODULES_WITH_MERMAID) + + const download = await mobileWebAppRouteChunkClosure(SESSION_ROUTE) + // The fence: nothing of the engine is reachable from the route's own chunk by an import + // statement, so opening the session pays none of it. + expect(artifactModules(download.staticInputs)).toEqual([]) + // The precondition that absence needs. The artifact is in the bundle, in a chunk the route + // reaches by a `dynamic-import` edge instead -- a deferred engine, not a dropped one. + expect(artifactModules(download.deferredInputs)).toHaveLength(1) + // And the walk read a real download rather than one chunk: the route's own chunk is in it. + expect(download.staticChunks).toContain(download.routeChunk) + expect(download.staticInputs.length).toBeGreaterThan(1000) + }, 600_000) + it('leaves the 16px seam census exactly where C7.2 left it', async () => { const closure = await mobileWebAppRouteClosure(SESSION_ROUTE) // Two preconditions, because zero offenders is what a walk that read nothing also reports: diff --git a/config/scripts/mobile-web-app-session-webview-consumers.test.mjs b/config/scripts/mobile-web-app-session-webview-consumers.test.mjs index 47e132a0737..a303f955670 100644 --- a/config/scripts/mobile-web-app-session-webview-consumers.test.mjs +++ b/config/scripts/mobile-web-app-session-webview-consumers.test.mjs @@ -11,8 +11,10 @@ * this closure reaches that package from nowhere at all. * * An empty list is also what a scan that read nothing reports, so the control below no longer - * uses the list — it runs the same walk over three native modules that do import the package and - * over the three web siblings that replace them. + * uses the list — it runs the same walk over the four native modules that do import the package and + * over the four web siblings that replace them. The diagram is the fourth: its native component + * seals untrusted source in a `WebView` and its sibling renders the same diagram in the document + * (C7.10 item B), which is the same substitution the other three are. */ import { readFileSync } from 'node:fs' import { join } from 'node:path' @@ -29,17 +31,19 @@ const SESSION = 'app/h/[hostId]/session/[worktreeId].tsx' /** Nothing: every consumer this closure had now resolves to a web sibling that needs no WebView. */ const REMAINING = [] -/** The three answered, whose `.web.tsx` the builder resolves instead of the native file. */ +/** The four answered, whose `.web.tsx` the builder resolves instead of the native file. */ const ANSWERED = [ 'src/components/MobileRichMarkdownEditor.web.tsx', 'src/components/MobileHtmlPreview.web.tsx', + 'src/components/pr-sidebar/MermaidDiagram.web.tsx', 'src/terminal/TerminalWebView.web.tsx' ] -/** The native files behind those three, which do import the package. The scan's own control. */ +/** The native files behind those four, which do import the package. The scan's own control. */ const NATIVE_CONSUMERS = [ 'src/components/MobileRichMarkdownEditor.tsx', 'src/components/MobileHtmlPreview.tsx', + 'src/components/pr-sidebar/MermaidDiagram.tsx', 'src/terminal/TerminalWebView.tsx' ] diff --git a/config/scripts/verify-mobile-web-app-bundle.mjs b/config/scripts/verify-mobile-web-app-bundle.mjs index f5f0e1e3a39..ba39a92a7ec 100644 --- a/config/scripts/verify-mobile-web-app-bundle.mjs +++ b/config/scripts/verify-mobile-web-app-bundle.mjs @@ -40,10 +40,22 @@ export const MOBILE_WEB_APP_BUNDLE_MAX_TOTAL_BYTES = 9 * 1024 * 1024 /** * How many scripts the page may be cut into, for a given number of routes. A chunk is emitted per - * distinct set of importers rather than per route, so the count is combinatorial in what the - * routes share: 8 routes measure 23 chunks, 10 measure 40, 12 measure 47, 14 measure 53, about - * three more per route at the top. Four per route with a flat 16 leaves the next few routes room, - * so a route added in C2 fails on its own weight and not on a number measured before it existed. + * distinct set of importers rather than per route, so this is not a function of the route count + * alone — it depends on what the routes in the tree happen to share. Measured on the head that + * wrote this, dropping routes from the end of the sorted key list: 8 routes emit 32 scripts, 10 + * emit 43, 12 emit 61, 14 emit 69. That is between four and nine more per route depending on which + * route, so four per route with a flat 16 is a bound rather than a fit. + * + * Read the headroom before adding a route: 14 routes measure 69 against this ceiling's 72, and the + * last two of them cost 8 — exactly the 8 the ceiling grants for two. The fence is at break-even, + * so the next route that shares less than its neighbours breaches it. That is the failure it is + * for; it names the split, where the asset count alone would name nothing. + * + * The route count is the only term, deliberately. A deferred engine belongs inside one artifact and + * costs one script: C7.10 item B first reached mermaid with `import('mermaid')`, which emitted 103 + * more because mermaid lazily imports each of its own diagram types, and a second term admitting + * those would have raised this fence far enough to admit any split at all. The build test's control + * is what holds that line. * * This is the ceiling that catches a split running away; MOBILE_WEB_APP_BUNDLE_MAX_ENTRY_BYTES * below is the one that catches it collapsing, and it is the real budget of the two. diff --git a/mobile/.gitignore b/mobile/.gitignore index fc97af19a22..474807b74b5 100644 --- a/mobile/.gitignore +++ b/mobile/.gitignore @@ -3,6 +3,7 @@ src/terminal/terminal-webview-engine.generated.ts src/terminal/terminal-webview-engine-css.generated.ts src/terminal/terminal-webview-document-script.generated.ts src/components/pr-sidebar/mermaid-webview-engine.generated.ts +src/components/pr-sidebar/mermaid-page-engine.generated.ts .expo/ dist/ /android/ diff --git a/mobile/.oxlintrc.json b/mobile/.oxlintrc.json index aa73bfe8583..442c50256bb 100644 --- a/mobile/.oxlintrc.json +++ b/mobile/.oxlintrc.json @@ -1,7 +1,10 @@ { "$schema": "./node_modules/oxlint/configuration_schema.json", "extends": ["../.oxlintrc.json"], - "ignorePatterns": ["src/terminal/terminal-webview-engine.generated.ts"], + "ignorePatterns": [ + "src/terminal/terminal-webview-engine.generated.ts", + "src/components/pr-sidebar/mermaid-page-engine.generated.ts" + ], "rules": { "react-hooks/exhaustive-deps": "off", "react/no-unescaped-entities": "off", diff --git a/mobile/package.json b/mobile/package.json index d89f04ff36d..f1359d96a7b 100644 --- a/mobile/package.json +++ b/mobile/package.json @@ -7,7 +7,7 @@ "start": "node scripts/start-expo.mjs", "android": "expo run:android", "ios": "expo run:ios", - "postinstall": "node scripts/build-terminal-webview-engine.mjs && node scripts/build-mermaid-webview-engine.mjs && node scripts/build-terminal-document-script.mjs", + "postinstall": "node scripts/build-terminal-webview-engine.mjs && node scripts/build-mermaid-webview-engine.mjs && node scripts/build-mermaid-page-engine.mjs && node scripts/build-terminal-document-script.mjs", "test": "vitest run", "typecheck": "tsc --noEmit", "typecheck:tests": "tsc --noEmit -p tsconfig.test.json", diff --git a/mobile/scripts/build-mermaid-page-engine.mjs b/mobile/scripts/build-mermaid-page-engine.mjs new file mode 100644 index 00000000000..ca7a9b2052b --- /dev/null +++ b/mobile/scripts/build-mermaid-page-engine.mjs @@ -0,0 +1,61 @@ +import { writeFile } from 'node:fs/promises' +import path from 'node:path' +import { createRequire } from 'node:module' +import * as esbuild from 'esbuild' + +const require = createRequire(import.meta.url) +const mobileRoot = path.resolve(import.meta.dirname, '..') +const outputPath = path.join( + mobileRoot, + 'src', + 'components', + 'pr-sidebar', + 'mermaid-page-engine.generated.ts' +) + +// Why: the page imports mermaid on demand, and `import('mermaid')` from inside the app bundle lands +// 103 emitted scripts rather than one -- mermaid lazily imports each of its own diagram types, so +// esbuild splits along those boundaries. All 103 sit inside the OTA generation the phone has +// already downloaded, so the split buys nothing in transfer and spends 103 of the 256 manifest +// assets the shell will load. Bundled here into one artifact, which the page imports on demand +// exactly as it imported the package. +// +// Unlike the sibling that builds the WebView engine, this is not a string: it is the module the +// page evaluates, so it is emitted as source and `mermaid-page-engine.ts` is what types it. +async function main() { + const { version } = require(require.resolve('mermaid/package.json')) + const { outputFiles } = await esbuild.build({ + absWorkingDir: mobileRoot, + stdin: { + contents: "export { default } from 'mermaid'\n", + resolveDir: mobileRoot, + loader: 'ts', + sourcefile: 'mermaid-page-engine-entry.ts' + }, + bundle: true, + format: 'esm', + // The whole point: one file, so the app bundle emits one deferred chunk for it. + splitting: false, + minify: true, + // The floor the shell's WebViews hold, the same pair the terminal engine is built for. + target: ['chrome74', 'safari15'], + write: false, + charset: 'utf8', + legalComments: 'none', + logLevel: 'silent', + nodePaths: [path.join(mobileRoot, 'node_modules')], + define: { 'process.env.NODE_ENV': '"production"' } + }) + const bundle = Buffer.from(outputFiles[0].contents).toString('utf8') + const source = [ + '// Generated by scripts/build-mermaid-page-engine.mjs.', + `// Package: mermaid@${version}, bundled as one ESM module for the page.`, + '// Do not edit by hand; regenerate via pnpm postinstall.', + '// @ts-nocheck -- minified vendor output; mermaid-page-engine.ts is where this is typed.', + bundle, + '' + ].join('\n') + await writeFile(outputPath, source) +} + +await main() diff --git a/mobile/src/components/pr-sidebar/MermaidDiagram.tsx b/mobile/src/components/pr-sidebar/MermaidDiagram.tsx index a9df89766a0..ac5dd9aa3b9 100644 --- a/mobile/src/components/pr-sidebar/MermaidDiagram.tsx +++ b/mobile/src/components/pr-sidebar/MermaidDiagram.tsx @@ -2,6 +2,7 @@ import { memo, useMemo, useState } from 'react' import { ScrollView, StyleSheet, Text, View } from 'react-native' import { WebView } from 'react-native-webview' import { colors, radii, spacing, typography } from '../../theme/mobile-theme' +import { MERMAID_DIAGRAM_CONFIG } from './mermaid-diagram-config' import { MERMAID_ENGINE_JS } from './mermaid-webview-engine.generated' export type MermaidDiagramProps = { @@ -79,19 +80,31 @@ function MermaidFallback({ source, base }: MermaidDiagramProps) { } // JSON.stringify escapes quotes and control chars but leaves `<`, `>`, `&`, and -// the U+2028/U+2029 line separators raw — so a source containing `` -// would close this inline ` would +// close the inline ` and the - * line separators would have to be replaced by whatever the DOM path needs. That is its own change - * with its own proof, so this series ships the degradation the component already defines and says - * so, rather than a second renderer nobody has tested against hostile diagram source. + * What replaces the sandbox is mermaid's own `securityLevel: 'strict'`, which runs the serialized + * SVG through DOMPurify before handing it back — a `` escaping has no analogue here and does not need one: the source is a + * JS string argument, not text spliced into an inline `' } + })) + try { + const hostile = await import('./MermaidDiagram') + const countClosers = (html: string) => (html.match(/<\/script>/gi) ?? []).length + const benign = countClosers(buildHtml('graph TD; A-->B')) + const built = hostile.buildHtml('graph TD; A-->B') + expect(countClosers(built)).toBe(benign) + expect(built).toContain('\\u003c/script') + } finally { + vi.doUnmock('./mermaid-diagram-config') + vi.resetModules() + } + }) + it('escapes the U+2028/U+2029 line separators that would break the JS literal', () => { const payload = `a${String.fromCharCode(0x2028)}b${String.fromCharCode(0x2029)}c` const html = buildHtml(payload) diff --git a/mobile/src/components/pr-sidebar/mermaid-page-engine.ts b/mobile/src/components/pr-sidebar/mermaid-page-engine.ts new file mode 100644 index 00000000000..807b5902acf --- /dev/null +++ b/mobile/src/components/pr-sidebar/mermaid-page-engine.ts @@ -0,0 +1,42 @@ +import type { Mermaid, MermaidConfig } from 'mermaid' + +/** + * The two calls the page makes of mermaid, named rather than cast. + */ +export type PageMermaid = { + initialize: (config: MermaidConfig) => void + render: (id: string, text: string) => Promise<{ svg: string }> +} + +/** + * The package's own API satisfies the type above, asserted at compile time. + * + * The loader's return does not assert it. The artifact is minified vendor output and both members + * measure as `any` there (a probe assigning `engine.render` to a `number` compiles), and `any` + * satisfies every signature, so returning it as `PageMermaid` checks the two names and nothing + * about their shapes. This does: `Mermaid` is precise, so a `PageMermaid` member whose signature + * the engine does not really have fails here instead of at a call the page makes. It lives beside + * the type it constrains rather than in a test: `mobile/tsconfig.json` excludes `*.test.ts`, so the + * app's own typecheck would not cover it there. Tests are typechecked too, by `tsconfig.test.json` + * through the tests-typecheck ratchet, but that is a second program with a grandfathered baseline + * and a few files held outside it on purpose, and it is not the gate the shipped build rests on. + * + * What no type can check is that the bundle behaves like the package. The render check is that, in + * both engines, against the native document's own bytes. + */ +const _packageSatisfiesPageMermaid: (engine: Mermaid) => PageMermaid = (engine) => engine + +/** + * The page's mermaid, loaded on demand from one pre-bundled artifact. + * + * `import('mermaid')` from inside the app bundle would emit 103 scripts, because mermaid lazily + * imports each of its own diagram types and esbuild splits along those boundaries. Every one of + * them ships inside the OTA generation the phone has already downloaded, so the split moves no + * bytes over the wire and spends 103 of the 256 manifest assets the shell will load. The artifact + * is the same engine in one file, and this import is still the deferred one: a session with no + * diagram on it evaluates none of it. + */ +export async function loadPageMermaid(): Promise { + const engine = await import('./mermaid-page-engine.generated') + return engine.default +} diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts b/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts index 2ed1917c948..e22971534dd 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts +++ b/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts @@ -1,6 +1,7 @@ -import { readdirSync, readFileSync } from 'node:fs' -import { join } from 'node:path' +import { readFileSync } from 'node:fs' +import { join, relative } from 'node:path' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' /** * The hybrid shell flag is the whole of what keeps this feature dark, so who touches it is a @@ -34,16 +35,9 @@ const TREES = { src: 200, app: 10, modules: 1 } const SHELL_VIEW = 'modules/orca-mobile-web-shell/src/index.ts' function sourceFiles(directory: string): string[] { - const found: string[] = [] - for (const entry of readdirSync(join(MOBILE_ROOT, directory), { withFileTypes: true })) { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - found.push(...sourceFiles(path)) - } else if (/\.tsx?$/.test(entry.name) && !entry.name.includes('.test.')) { - found.push(path) - } - } - return found + return censusSourceFiles(join(MOBILE_ROOT, directory)) + .map((path) => relative(MOBILE_ROOT, path)) + .filter((path) => /\.tsx?$/.test(path) && !path.includes('.test.')) } const SOURCES = Object.keys(TREES) diff --git a/mobile/src/reanimated-web-mapper-deps.test.ts b/mobile/src/reanimated-web-mapper-deps.test.ts index c03152a8f6d..7b62ace7821 100644 --- a/mobile/src/reanimated-web-mapper-deps.test.ts +++ b/mobile/src/reanimated-web-mapper-deps.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from './test-support/census-source-files' const mobileDirectory = fileURLToPath(new URL('..', import.meta.url)) const scanned = ['src', 'app'] @@ -29,16 +30,6 @@ const MAPPER_HOOKS = new Map([ ['useAnimatedReaction', { updaters: [0, 1], dependencies: 2 }] ]) -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return sourceExtensions.has(extname(entry.name)) ? [path] : [] - }) -} - /** Whether this `X.value` is being written rather than read. A write is an output, not an input. */ function isWriteTarget(node: ts.PropertyAccessExpression): boolean { const parent = node.parent @@ -200,11 +191,13 @@ describe('reanimated mapper hooks in the web bundle', () => { it('are all given a dependency array, because esbuild writes no worklet closure', () => { const found: string[] = [] const missing = scanned.flatMap((directory) => - sourceFiles(join(mobileDirectory, directory)).flatMap((path) => - path.endsWith('.test.ts') || path.endsWith('.test.tsx') - ? [] - : callsMissingDependencies(path, readFileSync(path, 'utf8'), found) - ) + censusSourceFiles(join(mobileDirectory, directory)) + .filter((path) => sourceExtensions.has(extname(path))) + .flatMap((path) => + path.endsWith('.test.ts') || path.endsWith('.test.tsx') + ? [] + : callsMissingDependencies(path, readFileSync(path, 'utf8'), found) + ) ) // The precondition the empty list above rests on. Binding resolution means a broken resolver // reports nothing at all, which would read exactly like a clean tree. diff --git a/mobile/src/rpc-params-contract-type-only-boundary.test.ts b/mobile/src/rpc-params-contract-type-only-boundary.test.ts index bc9088bea0b..7952c38f1a8 100644 --- a/mobile/src/rpc-params-contract-type-only-boundary.test.ts +++ b/mobile/src/rpc-params-contract-type-only-boundary.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative, resolve } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from './test-support/census-source-files' // Why: src/shared/rpc-contract/*-params.ts hold the host's zod schemas. Bundling one // into the app would let client code call parse(), and requiredString is @@ -13,16 +14,6 @@ const contractRoot = resolve(mobileRoot, '..', 'src', 'shared', 'rpc-contract') const scannedRoots = ['app', 'src'].map((directory) => join(mobileRoot, directory)) const sourceExtensions = new Set(['.js', '.jsx', '.ts', '.tsx']) -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function targetsContract(path: string, specifier: string): boolean { if (!specifier.startsWith('.')) { return false @@ -131,7 +122,7 @@ describe('RPC params contract boundary', () => { it('keeps every mobile import of the params contract type-only', () => { const offenders = scannedRoots - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => sourceExtensions.has(extname(path))) .flatMap((path) => contractValueImports(path, readFileSync(path, 'utf8')).map( diff --git a/mobile/src/test-support/census-source-files.test.ts b/mobile/src/test-support/census-source-files.test.ts new file mode 100644 index 00000000000..67c179bd498 --- /dev/null +++ b/mobile/src/test-support/census-source-files.test.ts @@ -0,0 +1,109 @@ +import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, relative } from 'node:path' +import { fileURLToPath } from 'node:url' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { censusSourceFiles, isGeneratedSource } from './census-source-files' + +const mobileRoot = fileURLToPath(new URL('../..', import.meta.url)) + +let scratch = '' + +beforeEach(() => { + scratch = mkdtempSync(join(tmpdir(), 'orca-census-source-files-')) +}) + +afterEach(() => { + rmSync(scratch, { recursive: true, force: true }) +}) + +function plant(relativePath: string, source: string): void { + const absolute = join(scratch, relativePath) + mkdirSync(join(absolute, '..'), { recursive: true }) + writeFileSync(absolute, source, 'utf8') +} + +describe('the source files a census reads', () => { + it('leaves out build output that would be read as a violating import', () => { + // The shape that started this: a generated file whose text holds exactly what a census is + // looking for. Every one of them is minified vendor output, so the match is a token in + // somebody else's code and the census has no line to offer anybody. + const violating = "import { requiredString } from '../../src/shared/rpc-contract/params'\n" + plant('src/components/engine.generated.ts', violating) + plant('src/components/Diagram.tsx', violating) + + const walked = censusSourceFiles(join(scratch, 'src')).map((path) => relative(scratch, path)) + expect(walked).toEqual([join('src', 'components', 'Diagram.tsx')]) + // Both halves: the generated file is gone, and the file beside it carrying the same text is + // not — a walk that returned nothing at all would satisfy the first line on its own. + expect(readFileSync(join(scratch, 'src/components/engine.generated.ts'), 'utf8')).toBe( + violating + ) + }) + + it('leaves out node_modules, and keeps everything else', () => { + plant('src/a.ts', '') + plant('src/node_modules/dep/index.ts', '') + plant('src/deep/b.tsx', '') + plant('src/notes.md', '') + expect( + censusSourceFiles(join(scratch, 'src')) + .map((path) => relative(scratch, path)) + .sort() + ).toEqual([join('src', 'a.ts'), join('src', 'deep', 'b.tsx'), join('src', 'notes.md')].sort()) + }) + + it('names build output by the suffix the generators write, and nothing else', () => { + expect(isGeneratedSource('mermaid-page-engine.generated.ts')).toBe(true) + expect(isGeneratedSource('route-manifest.generated.tsx')).toBe(true) + expect(isGeneratedSource('generated.ts')).toBe(false) + expect(isGeneratedSource('rpc-client.ts')).toBe(false) + expect(isGeneratedSource('generated-goldens.ts')).toBe(false) + }) + + it('covers every artifact the tree generates, read from the ignore file that lists them', () => { + // The list is `mobile/.gitignore`, because that is what the generators and the build agree on. + // A sixth artifact landing under a name this predicate does not match would put a multi-megabyte + // vendor bundle back into every census, which is the failure this module exists for. + const ignored = readFileSync(join(mobileRoot, '.gitignore'), 'utf8') + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.endsWith('.generated.ts')) + expect(ignored.length).toBeGreaterThanOrEqual(5) + for (const entry of ignored) { + expect(isGeneratedSource(entry), entry).toBe(true) + } + }) + + it('is the only walk of its kind left in the tree', () => { + // The line every census used to hold a copy of. One spelling, so a tenth census cannot quietly + // reintroduce the cost by pasting the walk rather than importing it. + const copies: string[] = [] + const walk = (directory: string): void => { + for (const entry of readdirSync(directory, { withFileTypes: true })) { + const path = join(directory, entry.name) + if (entry.isDirectory()) { + if (entry.name !== 'node_modules') { + walk(path) + } + continue + } + if (!/\.tsx?$/.test(entry.name) || isGeneratedSource(entry.name)) { + continue + } + if (readFileSync(path, 'utf8').includes("entry.name === 'node_modules' ? []")) { + copies.push(relative(mobileRoot, path)) + } + } + } + walk(join(mobileRoot, 'src')) + walk(join(mobileRoot, 'app')) + // This file is in the list because it carries the line as the text it greps for; the module + // beside it is the walk itself. Named rather than filtered out, so a third entry is a failure + // that reads as one. + expect(copies.sort()).toEqual([ + 'src/test-support/census-source-files.test.ts', + 'src/test-support/census-source-files.ts' + ]) + }) +}) diff --git a/mobile/src/test-support/census-source-files.ts b/mobile/src/test-support/census-source-files.ts new file mode 100644 index 00000000000..48ad1e087a4 --- /dev/null +++ b/mobile/src/test-support/census-source-files.ts @@ -0,0 +1,37 @@ +import { readdirSync } from 'node:fs' +import { join } from 'node:path' + +/** + * Build output, which a source census reads as source and must not. + * + * `mobile/.gitignore` is the list: five `*.generated.ts` files under `mobile/src`, written by the + * postinstall generators. Two are vendored engines — 3.7 MB of mermaid for the native WebView and + * 3.5 MB of it for the page — and 7.9 MB of what a walk over this tree returns is generated. A + * census that parses them parses minified third-party code looking for call sites nobody in this + * repo wrote and nobody can move, and pays the whole parse to find them: five of those files is + * what took `rpc-params-contract-type-only-boundary` from 1.5 s to over its 5 s timeout in CI. + * + * The generator that writes each one is ordinary source and is still walked, which is where a real + * reach into whatever a census is fencing would be. + */ +export function isGeneratedSource(name: string): boolean { + return /\.generated\.tsx?$/.test(name) +} + +/** + * Every file under `directory`, absolute, without `node_modules` or build output. + * + * Nine censuses in this tree held a copy of this walk, and two of them had grown a private opinion + * about generated files while the rest had none. What each census counts as *interesting* — which + * extensions, whether test files are in — stays its own business, because they genuinely disagree; + * what counts as a source file at all does not. + */ +export function censusSourceFiles(directory: string): string[] { + return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const path = join(directory, entry.name) + if (entry.isDirectory()) { + return entry.name === 'node_modules' ? [] : censusSourceFiles(path) + } + return isGeneratedSource(entry.name) ? [] : [path] + }) +} diff --git a/mobile/src/transport/lifecycle-owner.test.ts b/mobile/src/transport/lifecycle-owner.test.ts index 3a5d8aca82f..8f7a5c8fb80 100644 --- a/mobile/src/transport/lifecycle-owner.test.ts +++ b/mobile/src/transport/lifecycle-owner.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative, resolve } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' import { GenerationScopedRequestOwner, type LoadedRequest, @@ -370,16 +371,6 @@ function importsOwner(path: string, source: string): boolean { }) } -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function declaredInside(callback: ts.Node): Set { const names = new Set() const bind = (name: ts.BindingName): void => { @@ -492,7 +483,7 @@ function loaderWrites(path: string, source: string): string[] { describe('loader write fence', () => { const holders = ['app', 'src'] .map((directory) => join(mobileRoot, directory)) - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => ['.ts', '.tsx'].includes(extname(path))) .filter((path) => !/\.test\.tsx?$/.test(path)) .map((path) => ({ path, source: readFileSync(path, 'utf8') })) diff --git a/mobile/src/transport/rpc-operation-cast-fence.test.ts b/mobile/src/transport/rpc-operation-cast-fence.test.ts index 83d76eb87ab..df692012952 100644 --- a/mobile/src/transport/rpc-operation-cast-fence.test.ts +++ b/mobile/src/transport/rpc-operation-cast-fence.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative, resolve } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' /** * Bans the escapes that would make the typed boundary decorative. @@ -68,16 +69,6 @@ const CAST_FENCE_EXCEPTIONS: readonly CastFenceException[] = [ // inside a string literal therefore reads as one — which fails closed. const SUPPRESSION = /@ts-(?:expect-error|ignore|nocheck)\b/ -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function parse(path: string, source: string): ts.SourceFile { const extension = extname(path) return ts.createSourceFile( @@ -150,7 +141,7 @@ function moduleEdges(path: string, source: string): { imports: string[]; reExpor } const scanned = scannedRoots - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => sourceExtensions.has(extname(path))) .filter((path) => !/\.test\.tsx?$/.test(path)) diff --git a/mobile/src/transport/rpc-subscription-boundary.test.ts b/mobile/src/transport/rpc-subscription-boundary.test.ts index 2bc9239c613..6232212fbec 100644 --- a/mobile/src/transport/rpc-subscription-boundary.test.ts +++ b/mobile/src/transport/rpc-subscription-boundary.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative, resolve } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' import { readScenarios } from '../test-support/rpc-recording/scenario-input' import { RPC_SUBSCRIPTION_SITES, type RpcSubscriptionSite } from './rpc-subscription-inventory' @@ -38,16 +39,6 @@ const sourceExtensions = new Set(['.js', '.jsx', '.ts', '.tsx']) /** The port's own implementation and the oracle that scripts it. Neither consumes a stream. */ const EXCLUDED_DIRECTORIES = ['src/transport/', 'src/test-support/'] -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function parse(path: string, source: string): ts.SourceFile { const extension = extname(path) return ts.createSourceFile( @@ -80,7 +71,7 @@ export function subscribedMethods(path: string, source: string): string[] { } const scanned = scannedRoots - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => sourceExtensions.has(extname(path))) .filter((path) => !/\.test\.tsx?$/.test(path)) .map((path) => relative(mobileRoot, path).split(/[/\\]/).join('/')) diff --git a/mobile/src/transport/timer-receiver-census.test.ts b/mobile/src/transport/timer-receiver-census.test.ts index 8ff818a139b..d6cc23bee2f 100644 --- a/mobile/src/transport/timer-receiver-census.test.ts +++ b/mobile/src/transport/timer-receiver-census.test.ts @@ -1,7 +1,9 @@ -import { readdirSync, readFileSync } from 'node:fs' +import { readFileSync } from 'node:fs' +import { relative } from 'node:path' import { fileURLToPath } from 'node:url' import ts from 'typescript-api' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' const SOURCE_ROOT = fileURLToPath(new URL('..', import.meta.url)) const TIMER_GLOBALS = new Set(['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval']) @@ -28,9 +30,9 @@ const PARKING_OPERATORS = new Set([ type Census = { parked: string[]; shared: string[] } function productFiles(): string[] { - return readdirSync(SOURCE_ROOT, { recursive: true, encoding: 'utf8' }) - .filter((entry) => /\.tsx?$/.test(entry) && !/\.test\.tsx?$|\.generated\.ts$/.test(entry)) - .map((entry) => entry.replaceAll('\\', '/')) + return censusSourceFiles(SOURCE_ROOT) + .map((path) => relative(SOURCE_ROOT, path).replaceAll('\\', '/')) + .filter((entry) => /\.tsx?$/.test(entry) && !/\.test\.tsx?$/.test(entry)) } function timerName(node: ts.Node): string | null { diff --git a/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts b/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts index efdc426cabf..f452a7bd564 100644 --- a/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts +++ b/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' import { UNCHECKED_RPC_READERS, type UncheckedRpcReaderEntry @@ -46,16 +47,6 @@ const UNCHECKED_READER_NAMES = new Set([ // them in prose alone. const SELF_FILES = new Set(['src/transport/rpc-reader-payload.ts']) -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function parse(path: string, source: string): ts.SourceFile { const extension = extname(path) return ts.createSourceFile( @@ -85,7 +76,7 @@ function uncheckedReaderCount(path: string, source: string): number { } const scanned = scannedRoots - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => sourceExtensions.has(extname(path))) .filter((path) => !/\.test\.tsx?$/.test(path)) .map((path) => relative(mobileRoot, path).split(/[/\\]/).join('/')) diff --git a/mobile/src/transport/unvalidated-rpc-request-port-boundary.test.ts b/mobile/src/transport/unvalidated-rpc-request-port-boundary.test.ts index d0025e858ac..5f8db2935a6 100644 --- a/mobile/src/transport/unvalidated-rpc-request-port-boundary.test.ts +++ b/mobile/src/transport/unvalidated-rpc-request-port-boundary.test.ts @@ -1,8 +1,9 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { extname, join, relative, resolve } from 'node:path' import ts from 'typescript' import { describe, expect, it } from 'vitest' +import { censusSourceFiles } from '../test-support/census-source-files' import { UNVALIDATED_RPC_REQUEST_PORT_OWNERS, UNVALIDATED_RPC_REQUEST_PORT_PENDING, @@ -33,6 +34,10 @@ import { * - Test files. `*.test.ts(x)` is not scanned: faking the port is how these suites work, and a * test does not ship. A non-test file that fakes it (tsconfig excludes tests, so some do) is * scanned and listed. + * - Build output. `censusSourceFiles` leaves every `*.generated.ts` out, and one of them is a + * bundled vendor engine whose own dependencies contain the token `sendRequest` — minified + * third-party code, not a call site anybody in this repo wrote or can move onto an + * RpcOperation. The script that emits each of them is ordinary source and is walked. * A compile-time fence would catch the first two. That needs `RpcClient` to stop carrying the * port, which needs the call sites migrated first — the thing this list is counting down. */ @@ -51,16 +56,6 @@ const SELF_FILES = new Set([ /** The coalescing second sender: same unchecked string in, same unread envelope out. */ const SECOND_SENDER = 'sendSingleFlightRequest' -function sourceFiles(directory: string): string[] { - return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { - const path = join(directory, entry.name) - if (entry.isDirectory()) { - return entry.name === 'node_modules' ? [] : sourceFiles(path) - } - return [path] - }) -} - function parse(path: string, source: string): ts.SourceFile { const extension = extname(path) return ts.createSourceFile( @@ -144,7 +139,7 @@ const inventory: readonly UnvalidatedRpcRequestPortEntry[] = [ ] const scanned = scannedRoots - .flatMap(sourceFiles) + .flatMap(censusSourceFiles) .filter((path) => sourceExtensions.has(extname(path))) .filter((path) => !/\.test\.tsx?$/.test(path)) .map((path) => relative(mobileRoot, path).split(/[/\\]/).join('/')) diff --git a/mobile/web-entry/web-overrides.json b/mobile/web-entry/web-overrides.json index 2527e79112e..07c2d8fe7e1 100644 --- a/mobile/web-entry/web-overrides.json +++ b/mobile/web-entry/web-overrides.json @@ -63,7 +63,7 @@ }, { "file": "src/components/pr-sidebar/MermaidDiagram.web.tsx", - "reason": "The native component renders the diagram inside a sandboxed WebView, and react-native-webview is a native component with no browser counterpart: importing it runs a codegen lookup that throws, and the route manifest imports every route, so one such import takes the whole page down rather than one diagram. This one renders the labelled source box the native component already falls back to on a parse or render error. A real browser renderer is reachable — mermaid is a browser library and the engine bundle is vendored — but it is a different shape rather than a smaller one: with no WebView to sandbox untrusted diagram source in, the escaping buildHtml does for and the line separators has to be replaced by whatever the DOM path needs, which is its own change with its own proof." + "reason": "The native component seals the diagram inside a WebView whose document embeds the whole mermaid bundle as a string, because react-native-webview is a native component with no browser counterpart: importing it runs a codegen lookup that throws, and the route manifest imports every route, so one such import takes the whole page down rather than one diagram. This one renders the same diagram in this document instead — mermaid is a browser library, so it is an import() inside the render effect rather than a 3.7 MB literal, and the two hosts share one MERMAID_DIAGRAM_CONFIG. What replaces the sandbox is mermaid's own securityLevel: 'strict', which runs the serialized SVG through DOMPurify; the native path's escaping has no analogue because the source is a JS string argument rather than text spliced into an inline script. Both are measured in both engines under the shipped CSP by config/scripts/mobile-web-app-mermaid-render.test.mjs, which also pins the rendered SVG byte for byte against the native document's own render." }, { "file": "app/h/[hostId]/tasks.web.tsx", From 35005fb65c9d4d72244a830ade1dd35e6ca446a5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 20 Sep 2026 21:38:38 -0700 Subject: [PATCH 2/3] fix(pi): keep panes working while async subagents run (#21882) * fix(pi): wait for async subagents before settling pane * fix(pi): handle subagent event aliases and reloads * test(pi): assert lifecycle listener cardinality --- ...ent-status-extension-omp-lifecycle.test.ts | 47 +++++++++++++++++++ .../pi/agent-status-extension-test-harness.ts | 9 ++++ src/main/pi/agent-status-handler-source.ts | 39 +++++++++++++-- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/main/pi/agent-status-extension-omp-lifecycle.test.ts b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts index 89f8c94d9a5..5d89d31aec1 100644 --- a/src/main/pi/agent-status-extension-omp-lifecycle.test.ts +++ b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts @@ -15,6 +15,53 @@ const OMP_RUNTIME_CASES = [ ] as const describe('OMP agent_end contract', () => { + it('keeps a Pi pane working until async subagents finish', async () => { + const harness = createAgentStatusExtensionHarness({ kind: 'pi' }) + + await harness.callHook('agent_start') + harness.emitPiEvent('task:subagent:lifecycle', { id: 'child-1', status: 'started' }) + await harness.callHook('agent_settled', undefined, { isIdle: () => true }) + + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start']) + + harness.emitPiEvent('task:subagent:lifecycle', { id: 'child-1', status: 'completed' }) + await vi.waitFor(() => + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start', 'agent_end']) + ) + }) + + it('ignores malformed or unknown Pi subagent lifecycle events', async () => { + const harness = createAgentStatusExtensionHarness({ kind: 'pi' }) + harness.emitPiEvent('task:subagent:lifecycle', {}) + harness.emitPiEvent('task:subagent:lifecycle', { id: 'child-1', status: 'paused' }) + await harness.callHook('agent_start') + await harness.callHook('agent_settled') + await vi.waitFor(() => + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start', 'agent_end']) + ) + }) + + it('keeps one lifecycle subscription across extension reloads', async () => { + const harness = createAgentStatusExtensionHarness({ kind: 'pi' }) + harness.reload() + expect(harness.piEventListenerCount('task:subagent:lifecycle')).toBe(1) + expect(harness.piEventListenerCount('subagent:async-started')).toBe(1) + expect(harness.piEventListenerCount('subagent:async-complete')).toBe(1) + harness.emitPiEvent('task:subagent:lifecycle', { id: 'child-1', status: 'started' }) + await vi.waitFor(() => expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start'])) + }) + + it('accepts the pi-subagents async lifecycle aliases', async () => { + const harness = createAgentStatusExtensionHarness({ kind: 'pi' }) + harness.emitPiEvent('subagent:async-started', { id: 'child-1' }) + await harness.callHook('agent_settled') + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start']) + harness.emitPiEvent('subagent:async-complete', { id: 'child-1' }) + await vi.waitFor(() => + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start', 'agent_end']) + ) + }) + it.each(OMP_RUNTIME_CASES)( 'keeps %s working when agent_end will continue', async (_name, args) => { diff --git a/src/main/pi/agent-status-extension-test-harness.ts b/src/main/pi/agent-status-extension-test-harness.ts index 696153064f1..0c07caf8256 100644 --- a/src/main/pi/agent-status-extension-test-harness.ts +++ b/src/main/pi/agent-status-extension-test-harness.ts @@ -46,6 +46,8 @@ export type AgentStatusExtensionHarness = { handlers: Record processEnv: Record callHook: (name: string, event?: unknown, context?: HookContext) => Promise + emitPiEvent: (name: string, event: unknown) => void + piEventListenerCount: (name: string) => number // Re-invoke the extension factory in the same process (as Pi does on an // in-process extension reload), swapping in the freshly registered handlers. reload: () => void @@ -130,6 +132,7 @@ export function createAgentStatusExtensionHarness(args: { command: { handler: (args: string, context: HookContext) => Promise } ) => void setModel: (model: unknown) => Promise + events?: EventEmitter }) => void } } = { exports: {} } @@ -191,6 +194,7 @@ export function createAgentStatusExtensionHarness(args: { } const handlers: Record = {} + const piEvents = new EventEmitter() const commands: AgentStatusExtensionHarness['commands'] = {} const setModelMock = vi.fn(async (_model: unknown) => true) const registerInto = (target: Record): void => { @@ -199,6 +203,7 @@ export function createAgentStatusExtensionHarness(args: { commands[name] = command }, setModel: setModelMock, + events: piEvents, on(name: string, handler: HookHandler) { target[name] = handler } @@ -219,6 +224,10 @@ export function createAgentStatusExtensionHarness(args: { callHook: async (name, event, hookContext) => { await handlers[name]?.(event, hookContext) }, + emitPiEvent: (name, event) => { + piEvents.emit(name, event) + }, + piEventListenerCount: (name) => piEvents.listenerCount(name), reload: () => { for (const key of Object.keys(handlers)) { delete handlers[key] diff --git a/src/main/pi/agent-status-handler-source.ts b/src/main/pi/agent-status-handler-source.ts index b3bd70e28a4..1d339e670ae 100644 --- a/src/main/pi/agent-status-handler-source.ts +++ b/src/main/pi/agent-status-handler-source.ts @@ -129,13 +129,27 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] ' if (ownerPid && ownerPid !== selfPid && isStatusOwnerAlive(ownerPid)) return', ` process.env.${ownerEnv} = selfPid`, ' resetPostQueue()', + ' const piEventBus = (pi as { events?: { on?: (name: string, handler: (event: unknown) => void) => void } }).events', + ' const lifecycleState = (piEventBus as { __orcaPiSubagents?: { active: Set; waiting: boolean; onEvent?: (event: unknown, forcedStatus?: string) => void; listener?: (event: unknown) => void } } | undefined)?.__orcaPiSubagents ?? { active: new Set(), waiting: false }', + ' if (piEventBus) (piEventBus as { __orcaPiSubagents?: unknown }).__orcaPiSubagents = lifecycleState', + ' if (piEventBus?.on && !(lifecycleState as { listener?: unknown }).listener) {', + ' const listener = (event: unknown) => lifecycleState.onEvent?.(event)', + ' lifecycleState.listener = listener', + " piEventBus.on('task:subagent:lifecycle', listener)", + " piEventBus.on('subagent:async-started', (event: unknown) => lifecycleState.onEvent?.(event, 'started'))", + " piEventBus.on('subagent:async-complete', (event: unknown) => lifecycleState.onEvent?.(event, 'completed'))", + ' }', ...(kind !== 'pi' - ? [" pi.on('session_shutdown', () => { resetPostQueue(); clearPendingAgentEndCheck() })"] + ? [ + " pi.on('session_shutdown', () => { lifecycleState.active.clear(); lifecycleState.waiting = false; resetPostQueue(); clearPendingAgentEndCheck() })" + ] : []), ...(kind !== 'prime-agent' ? [ " pi.on('session_switch', (_event, ctx) => {", ' if (!isOmpRuntime()) return', + ' lifecycleState.active.clear()', + ' lifecycleState.waiting = false', ' resetPostQueue()', ' clearPendingAgentEndCheck()', ' updateRuntimeOmpSessionMetadata(ctx)', @@ -154,6 +168,7 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] ` onStatus('agent_start', (${bareCtxParams}) => {`, ...captureSessionMetadata, ' clearPendingAgentEndCheck()', + ' lifecycleState.waiting = false', ' runGeneration += 1', // Why: a turn cannot begin under a dialog holding input focus, so this is the one // boundary that can recover a modal whose close never arrived. @@ -224,11 +239,25 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] ' pendingAgentEndCheck = null', ' pendingAgentEndContext = null', ' }', - '', - ' // Why: isIdle flips before agent_settled handlers run, so both paths', - ' // share a guard instead of racing duplicate completion posts — one keyed on the', - ' // generation of the run that ENDED, so a later run still reports its own end.', + ' // Defer completion while live child work remains.', + ' lifecycleState.onEvent = (event: unknown, forcedStatus?: string): void => {', + " if (!event || typeof event !== 'object') return", + " const id = typeof (event as { id?: unknown }).id === 'string' ? (event as { id: string }).id : ''", + ' const status = forcedStatus ?? (event as { status?: unknown }).status', + ' if (!id) return', + " if (status === 'started') { lifecycleState.active.add(id); post('agent_start'); return }", + " if (status !== 'completed' && status !== 'failed' && status !== 'aborted') return", + ' lifecycleState.active.delete(id)', + ' if (lifecycleState.active.size === 0 && lifecycleState.waiting) {', + ' lifecycleState.waiting = false', + ' postAgentEndOnce()', + ' }', + ' }', ' function postAgentEndOnce(): void {', + ' if (lifecycleState.active.size > 0) {', + ' lifecycleState.waiting = true', + ' return', + ' }', ' if (completionPostedGeneration === endedRunGeneration) return', ' completionPostedGeneration = endedRunGeneration', // Why: distinct from the completion guard, which holds the generation of the posted run From 27a0889dcfda7a37f256a024d9f88fde64b3d3f7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 20 Sep 2026 21:41:58 -0700 Subject: [PATCH 3/3] test(relay): account for OpenCode marker in OMP launch environment (#21907) --- src/relay/omp-fresh-launch-environment.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/relay/omp-fresh-launch-environment.test.ts b/src/relay/omp-fresh-launch-environment.test.ts index 8697368567f..5f7cd57681f 100644 --- a/src/relay/omp-fresh-launch-environment.test.ts +++ b/src/relay/omp-fresh-launch-environment.test.ts @@ -91,7 +91,7 @@ it('prepares the execution host OMP config and status extension for a guarded la source.mockReturnValue(false) expect( await augment.mock.calls[1][0]({ id: 'other', shell: '/bin/bash', env: {}, command: 'codex' }) - ).toEqual({}) + ).toEqual({ ORCA_OPENCODE_AGENT: 'opencode' }) } finally { runtime.stop() dispatcher.dispose()