perf(ci): parallelize repo audits, guard env-dependent tests, and fix the docs generator - #6358
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Tests add Docs generation fixes MDX breakage from unescaped angle brackets in tool prose, reads trigger config and outputs from the evaluated registry (with a converter for nested trigger output shapes), and renders action headings from tool display names (e.g. Reviewed by Cursor Bugbot for commit 19cd7ea. Configure here. |
Greptile SummaryThe PR parallelizes repository audits, makes tool-dependent tests fail safely in CI while skipping unsupported local environments, and refactors documentation generation to use evaluated registry metadata.
Confidence Score: 5/5The PR appears safe to merge. The prior Python-version issue is resolved: the guard now requires Python 3.12, all tests implicated by that thread invoke it before executing the affected syntax, and unsupported CI environments fail rather than silently skipping coverage.
|
| Filename | Overview |
|---|---|
| packages/testing/src/environment/index.ts | Introduces memoized Python 3.12 and ripgrep availability guards, with explicit local skips and CI failures; the previously reported Python-version floor is fixed. |
| scripts/run-audits.ts | Runs independent zero-argument repository audits concurrently while preserving failure output and status. |
| scripts/generate-docs.ts | Refactors documentation generation around evaluated registry metadata and normalizes trigger-output nesting. |
| .github/workflows/test-build.yml | Replaces sequential audit steps with the consolidated audit runner and removes the inactive coverage upload. |
| apps/sim/lib/execution/code-placeholders/compiler.test.ts | Guards tests that execute Python 3.12-only generated syntax using the corrected environment check. |
Reviews (2): Last reviewed commit: "refactor(ci): read package.json once in ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The 21 independent audits ran as 21 sequential CI steps, each a single-threaded read-only walk of the tree. scripts/run-audits.ts runs them concurrently: 28s serial -> 5.0s wall locally at 13-way. It buffers each audit's output and replays only failures, so a green run stays quiet and a red one still names the audit and shows why. Audits needing a git base ref (block registry, migration safety) or that write files (drizzle generate) stay as their own steps. Also fixes 5 tests that fail for every macOS dev and are invisible in CI. They shell out to python3 using `match` statements and 3.12 f-string nesting, which need >= 3.10; stock macOS ships 3.9.6, so `bun run test` produced raw Python SyntaxErrors with no guard and nothing tying them to a missing tool. One also needs ripgrep, which CI installs and a Mac usually does not. @sim/testing/environment detects both and the tests skip with a reason via vitest's ctx.skip(). Under CI it throws instead: these suites deliberately run the real helper rather than a mock -- the cloud-review path/read-size bounds and the placeholder compiler's generated Python are only observable that way -- so a missing tool in CI means a security boundary silently stopped being covered, which is worse than a red build. Drops the Codecov upload. The workflow already documented it as a dead path: nothing generates apps/sim/coverage, vitest runs without --coverage, and fail_ci_if_error hides it, so it reported green having uploaded nothing.
…ializing the batch
Two review findings, both real.
MIN_PYTHON was 3.10, chosen for the `match` statements the compiler suite
generates. But two of the three guarded tests also use PEP 701 f-strings --
reusing the outer quote, and embedding `#` -- which are 3.12. Verified on a real
3.11 interpreter: the match-guard test passes, the other two fail with
`f-string: unmatched '('` and `f-string expression part cannot include '#'`,
which is exactly the raw SyntaxError the guard exists to prevent. A 3.10 floor
let them through and failed anyway.
The audit parallelization did not speed CI up -- it slowed it down. Serially the
21 audits took ~31s; concurrently the batch took 39.2s wall, because
check:desktop-bridge went from 1s to 39.2s and became the entire wall clock while
the other 20 finished in 9s. It is the only audit that shells out through `bunx`,
which re-resolves the package against the shared install cache -- a network-backed
sticky-disk mount on CI. Cheap when it runs alone, serialized behind the others
when they run together. Spawning the resolved compiler entry point directly
removes that layer.
Verified the audit still fails on a breaking bridge change rather than passing
faster by doing less.
ab592b0 to
a23f212
Compare
…istry
The docs build has been failing on staging since the Smartlead merge:
./apps/docs/content/docs/en/integrations/smartlead.mdx
Expected a closing tag for `<original>` before the end of `paragraph`
Tool descriptions are emitted as prose, and that path escaped only braces --
every table-cell path already escaped angle brackets. MDX reads `<` as the start
of a JSX tag, so a description like 'The copy is named "<original> - copy"' fails
the build outright. escapeMdxProse handles the MDX-hostile characters and leaves
pipes, parens and brackets alone, which are legal in prose and whose escaping
would mangle markdown links.
Trigger configuration now comes from the evaluated registry instead of regex over
source. Static parsing silently dropped every field whose builder assembled its
array imperatively or took a description as a parameter -- all ten Jira triggers
lost `webhookSecret` and `jqlFilter` that way, and Monday lost its config too, so
regenerating the docs was destructive. Reading real objects also deletes 232 lines
of parsing. Note `required` may be a condition object rather than `true`; only an
unconditional `true` renders as Required, matching the previous behavior.
Tool headings now show the tool's name ("A2A Send Message") rather than its id
(`a2a_send_message`), unformatted, across 241 generated pages. Names come from
tools/generated/tool-metadata.ts, which CI keeps in sync. These headings feed each
page's table of contents. a2a.mdx is hand-written, so its headings were updated
directly.
Also consolidates five hand-inlined copies of the escape chain into the
escapeMdxCell that already existed, and drops 44 comments that restated the line
below them. Generator: 4306 -> 4069 lines.
Every refactor step was verified against a golden manifest of all 289 generated
files -- proven deterministic across runs and proven to catch a one-character
change -- so the only output differences are the intended ones.
KNOWN GAP: extractTriggerOutputs still parses source and has the same blind spot;
it already drops one Jira output section on main. Regenerating is now safe for
trigger config but still lossy for trigger outputs.
waleedlatif1
changed the title
Review pass over the audit runner and the tool guards. The audit list was hand-maintained alongside package.json with nothing linking them, and it had already drifted: check:cron-parity exists, passes, and ran in no CI step at all. The list is now derived from the check:* scripts with an explicit exclusion map, so a new audit is opted out deliberately rather than forgotten. That picks up cron-parity — 22 audits now, not 21. check-realtime-prune-graph.ts still shelled out through `bunx turbo`, the same pattern that took the bridge audit from 1s to 39s once the audits ran concurrently. Both now go through scripts/local-bin.ts, which resolves node_modules/.bin — the same path check:native-typecheck asserts is the native TypeScript 7 compiler, so the one guarded path is the one that runs. Audits are spawned as their script rather than `bun run <name>`, which started a bun process only to read package.json and start a second one. Tool detection is memoized per process; it was re-spawning python3 on each of the 5 call sites, in every vitest worker. The CI throw is deliberately NOT memoized — memoizing it would turn every call after the first into a silent skip, which is the failure mode the guard exists to prevent. Verified it still throws for all three guarded tests, not just the first. Also: dropped the environment module from the @sim/testing barrel so node:child_process stays out of unrelated consumers' module graphs, restored the per-audit reporting the 21 separate steps used to give (collapsible groups, error annotations, and a timing table they never had), and trimmed comments that restated their code or duplicated the runner's own docs.
|
Pushed a review pass ( The audit list had already drifted. It was hand-maintained alongside The Memoizing the tool probe could have weakened the CI gate. Detection is now cached per process (it was re-spawning Also restored the per-audit reporting the 21 separate steps used to give: collapsible Skipped two findings: resolving the native tsc binary directly to skip its JS shim (couples to Also folded in the docs work from #6360 — the MDX build fix and the generator cleanup — so it all lands together. |
|
@cursor review |
|
This is a clean review pass — all three substantive findings are addressed correctly, and the design decisions hold up well. Derived audit list (
Memoization design: The split between memoizing result ??= detect() // spawn once
if (result.ok) return true
if (process.env.CI) {
throw new Error(...) // re-throws on every call — NOT memoized
}One minor thing worth noting in Everything else looks good — the |
Every Devin tool had its id as its `name` (`list_session_messages`), so the generated docs rendered `### list_session_messages` where every other integration renders a human name. It was the only integration doing this -- 11 of 4427 tools. Names take the service prefix, matching the majority convention (3200 of 4416 names start with their service). Also points the ship skill at check:audits instead of hand-listing the audits. That copy had drifted five behind package.json: cron-parity, import-specifiers, sql-date-binding, trigger-block-cycle and native-typecheck were all missing, so shipping never ran them. It was the third copy of that list; there is now one.
Closes the gap left by the config fix: extractTriggerOutputs still parsed source,
so triggers whose outputs come from a builder call lost their tables. jira_webhook
had no output section at all.
The registry was not a drop-in, which is why the naive swap deleted 10,298 lines
earlier. The two sides encode nesting differently. A TriggerOutput marks a group
by OMITTING type and holding children as sibling keys:
issue: { id: { type: 'number' }, title: { type: 'string' } }
while the renderer walks the JSON-Schema-ish shape the parser used to synthesize:
issue: { type: 'object', properties: { id: …, title: … } }
formatOutputStructure only descends into .properties, so handing it the raw
registry value collapsed every nested group to one untyped row and dropped its
children. normalizeTriggerOutputs converts between the two, preserving leaves
that already declare properties/items and merging the 13 hybrid nodes that carry
both a type and inline children.
Measured across all 368 triggers before changing anything: 155 identical, 213
divergent, and the divergence was purely the nesting encoding — no node has a
non-string type, and a group never carries its own string description, so
leaf-vs-group classification is unambiguous. That is what makes a nested property
literally named 'description' (42 of them) survive.
Deletes the static path: extractTriggerOutputs, resolveTriggerBuilderFunction,
resolveTriggerOutputsConstant, readTriggerSiblingModules,
getWebhookProviderConstants, plus resolveConstStringValue and matchQuotedProperty
which the config fix had already stranded.
20 output sections recovered (linear 79->93, tiktok 6->11, jira 44->45) and 1698
rows. Verified independently: zero sections lost across all 289 generated files,
no file lost rows, output deterministic across regeneration.
The 96 deletions are all corrections, not losses. 70 are confluence fields the
parser flattened out of `comment: { ...buildContentEntityFields(), parent: {…} }`
and rendered as top-level trigger outputs; they reappear nested under their
parent in the same hunk. 8 are greenhouse key ordering, 6 are intercom
descriptions the parser had dropped, 1 is a vercel row moving position.
Generator: 4069 -> 3903 lines.
…nfig @vitejs/plugin-react v4 targets pre-rolldown Vite: it sets `esbuild.jsx` and `optimizeDeps.rollupOptions`, both deprecated under Vite 8's oxc pipeline, and self-reports that plugin-react-oxc should be used instead. v6 is that plugin merged back under the original name — it requires Vite ^8, drops Babel entirely, and emits none of those options. Vite 8 also resolves tsconfig paths natively, so vite-tsconfig-paths is replaced by `resolve.tsconfigPaths`. Full apps/sim suite unchanged: 1483 passed / 2 skipped files, 20415 passed / 30 skipped tests.
Second pass over the generator, e.g. `// Copy icons from sim app to docs app` above `copyIconsFile()`. Kept the multi-line runs (those carry reasoning), the ones with concrete examples, and the one marking a deliberate empty catch. Verified byte-identical output across all 289 generated files. Generator: 3903 -> 3870 lines, 4306 at the start of this branch.
auditScripts() re-read the manifest the module body had already loaded.
|
Pushed four more commits. Two close gaps I'd flagged as open, two came from review. The trigger-outputs gap is closed ( Devin tool names ( Vite 8 warnings ( And the thing I got wrong earlier is now measured right. I reported the audit parallelization as a 5.8x win from my laptop (13-way, warm cache). On CI's 8 vCPU with cold I/O it was actually a regression — 31s serial → 39.2s — because one audit's |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 21dbe2f. Configure here.
People Data Labs declared `pdl_*` tool ids under `tools/peopledatalabs/`. Every other integration names the directory after its id prefix -- 259 of 260 before this, and PDL was the only exception. The docs generator locates a tool's definition by deriving the directory from the id prefix, so it looked in `tools/pdl/`, found nothing, and returned null for all 11 tools. peopledatalabs.mdx rendered eleven bare `###` headings with no description, no Input table and no Output table. Renaming the directory rather than the ids: tool ids are persisted in saved workflows, so renaming those would break existing users. The directory is internal -- 15 files' imports. Fixed at the source rather than teaching the generator a fallback. A special case would have left the invariant broken and the next integration free to break it again; now 260 of 260 hold, and the generator needs no exception. peopledatalabs.mdx: 11 empty headings -> 456 lines. Repo-wide: zero pages with an empty action body.
|
One more, from a real gap you'd never see in the diff: Root cause: PDL declared I fixed the naming rather than the generator. My first instinct was a fallback that indexes what each file actually declares — but that would have left the invariant broken and let the next integration break it again. Measured it first: 259 of 260 tool directories already name themselves after their id prefix. PDL was the sole exception, so it's a violation to correct, not a rule to soften. Now 260 of 260, and the generator needs no special case. Renamed the directory, not the ids — tool ids are persisted in saved workflows, so renaming those would break existing users. The directory is internal: 15 files' imports.
Worth a reviewer's eye: |
|
@cursor review |
|
The fix is correct and the reasoning is sound. Renaming the directory to match the The rename-not-the-ids call is the right one too. Tool ids are external state (persisted in saved workflows); directory paths are internal. 15 import sites is an acceptable churn for a correctness fix. The LGTM on this piece. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 19cd7ea. Configure here.
1 participant
Three related pieces of CI/docs work. All three came out of chasing why type-checking was slow.
1. Parallelize the repo audits — and actually make them faster
21 independent read-only tree walks ran as 21 sequential CI steps.
scripts/run-audits.tsruns them concurrently and replays output only for failures, so a green run stays quiet and a red one still names the audit.The first version of this made CI slower, not faster — 31s serial → 39.2s wall.
check:desktop-bridgewent from 1s to 39.2s and became the entire wall clock while the other 20 finished in 9s. It's the only audit that shells out throughbunx, which re-resolves the package against the shared install cache — a network-backed sticky-disk mount on CI. Cheap alone, serialized behind the others when run together. Spawning the resolved compiler entry directly removes that layer.2. Guard tests that depend on tools the repo doesn't vendor
5 tests fail for every macOS dev and are invisible in CI. They shell out to
python3using PEP 701 f-strings (3.12+); stock macOS ships 3.9.6, sobun run testproduced raw PythonSyntaxErrors with nothing tying them to a missing tool. One also needsripgrep.These suites deliberately run the real helper rather than a mock — the cloud-review path/read-size bounds and the placeholder compiler's generated Python are only observable that way. So
@sim/testing/environmentskips them with a reason locally viactx.skip(), but throws underCI: a missing tool on the runner means a security boundary silently stopped being covered.Reviewers correctly caught that the floor should be 3.12, not 3.10. Verified on a real 3.11 interpreter — the
match-guard test passes, but the two f-string tests fail withf-string: unmatched '('andf-string expression part cannot include '#'.Also drops the Codecov upload, which the workflow itself documented as a dead path.
3. Unbreak the docs build + clean up the generator
The docs build has been failing on staging since the Smartlead merge:
Expected a closing tag for``. Tool descriptions are emitted as prose, and that path escaped only braces while every table-cell path already escaped angle brackets.Trigger config now comes from the evaluated registry, not regex over source. Static parsing silently dropped every field whose builder assembled its array imperatively or took a description as a parameter — all ten Jira triggers lost
webhookSecret/jqlFilter, Monday lost its config too. Regenerating the docs was destructive. Deletes 232 lines of parsing.Tool headings show the tool's name, not its id —
### \a2a_send_message`→### A2A Send Message`, across 241 pages. These headings feed each page's table of contents.Generator: 4,306 → 4,069 lines.
Type of Change
Testing
The generator refactor was verified against a golden manifest of all 289 generated files, itself validated both ways — deterministic across runs, and proven to catch a one-character change. Only intended output changed.
CI=true, python 3.9.6CI=true, python 3.13apps/simsuiteVerified the failure paths too: broke an audit (runner named it, replayed output, exit 1), and broke the bridge contract (audit still fails after the
bunxchange — it isn't passing faster by doing less).Things I built, measured, and threw away rather than ship: a regex fix for the imperative-array pattern (invented
fieldFilterson 9 triggers that don't declare it); sourcing triggeroutputsfrom the registry (deleted 10,298 lines across 53 files);isolate: falsein vitest (1.94x faster, 429 test failures).Known gaps — please don't read this as "all clear"
extractTriggerOutputsstill parses source and has the same blind spot as the config parser. It already drops one Jira#### Outputsection onmain— verified as pre-existing. Regenerating is now safe for trigger config, still lossy for trigger outputs.openai.mdxis a stale orphan with no backing block since feat(embeddings): multi-provider Embeddings block on a shared core #6317. The generator doesn't rewrite it and the stale-doc cleanup doesn't delete it. Probably just wants deleting.Checklist
Added since the first review round
Trigger outputs — the known gap above is now closed.
extractTriggerOutputsalso parsed source, sojira_webhookhad no output section at all. The registry wasn't a drop-in: aTriggerOutputmarks a nested group by omittingtypeand holding children as sibling keys, while the renderer walks a{type, description, properties}shape the parser used to synthesize. Handing it the raw value collapsed every group to one untyped row — that's the 10,298-line deletion an earlier attempt produced. A converter between the two encodings is the real fix. 20 output sections and 1,698 rows recovered; independently verified zero lost across all 289 files.The 96 deletions are corrections: 70 are confluence fields the parser flattened out of
comment: { ...buildContentEntityFields(), parent: {…} }and published as top-level trigger outputs — they reappear nested under their parent, and the source confirms the nesting.Devin tool names. All 11 Devin tools had their id as their
name, so docs rendered### list_session_messages. Audited all 4,427 tools — Devin was the only one. Fixed at the source; zero id-shaped names remain.shipskill →check:audits. It had a third hand-maintained copy of the audit list, five behindpackage.json(cron-parity,import-specifiers,sql-date-binding,trigger-block-cycle,native-typechecknever ran when shipping).Vite 8 deprecation warnings. Five warnings on every test run, all from⚠️ Requires
@vitejs/plugin-react@4being pre-rolldown on Vite 8 — it sets deprecated options unconditionally, so no config change suppresses them.@vitejs/plugin-react^4.3.4 → ^6.0.5 (v6 isplugin-react-oxcmerged back;plugin-react-oxcitself doesn't support Vite 8). v6 drops Babel support — verified the repo callsreact()with no options and has no Babel plugins. Full suite before/after: identical counts (20,415 passed / 30 skipped), 128.6s vs 129.8s — no speedup, and I'm not claiming one.plugin-reactdoes little in anenvironment: 'node'run.Generator: 4,306 → 3,870 lines, every step verified byte-identical against the 289-file manifest.
Measured CI result
Repo auditsstepbunxcall serialized the batch)Lint and Testjob: 373s → 220s (−41%).