fix: resolve test-app dependabot alerts - #649
Merged
Merged
Conversation
The postcss/uuid overrides added in #464 stopped applying once test-app ended up nested under the repo-root pnpm-workspace.yaml: pnpm only honors overrides from a workspace root, so test-app's package.json `pnpm.overrides` were silently ignored and the lockfile drifted back to vulnerable versions. Move the overrides into a dedicated examples/test-app/pnpm-workspace.yaml so test-app is its own pnpm root and the overrides are honored, and add scoped overrides for the two remaining alerts: - postcss 8.4.49 -> 8.5.12 (XSS in CSS stringify) - uuid 7.0.3 -> 14.0.0 (missing buffer bounds check) - ws@8 8.20.0 -> 8.21.0 (uninitialized memory disclosure) - brace-expansion@5 5.0.5 -> 5.0.6 (ReDoS / max bypass) ws and brace-expansion overrides are scoped to the vulnerable majors so the non-vulnerable ws@7 / brace-expansion@1 copies in the tree are left untouched.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f729f436f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,6 @@ | |||
| overrides: | |||
There was a problem hiding this comment.
The documented install paths still run pnpm install --dir examples/test-app --ignore-workspace from the root script and pnpm install --ignore-workspace in examples/test-app/README.md; --ignore-workspace makes pnpm behave as if no pnpm-workspace.yaml exists, so these newly moved overrides are not applied on the normal setup path. After a contributor runs the documented install, pnpm can either rewrite the lockfile back without the security overrides or report the lockfile overrides/config as mismatched, which defeats the Dependabot-alert fix this commit is trying to make durable.
Useful? React with 👍 / 👎.
- Remove the no-op `lodash-es` override from the root package.json (leftover from #368). lodash-es is no longer in the dependency tree, so the override resolved to nothing; regenerating the root lockfile is a no-op. - Add a comment to examples/test-app/pnpm-workspace.yaml explaining why the file exists, so it isn't "tidied away" and the override drift reintroduced.
|
thymikee
added a commit
that referenced
this pull request
Jul 16, 2026
…rity overrides) The first live run of the layer-3 device path failed at ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, and the cause is a real latent bug rather than a stale lockfile. #649 moved the fixture app's `overrides` into examples/test-app/pnpm-workspace.yaml precisely because pnpm only honors overrides from a workspace root — they pin transitive deps (ws, brace-expansion, xmldom, postcss, uuid, shell-quote) to versions that clear Dependabot alerts. But `test-app:install` passes --ignore-workspace, which ignores that very file, so the overrides are dropped and no longer match the lockfile that has them baked in. It goes unnoticed locally because interactive installs are not frozen, and no CI job has ever installed this app. Dropping --ignore-workspace makes examples/test-app resolve as its own workspace root (it has its own pnpm-workspace.yaml and is not a member of the repo-root workspace), so the overrides apply and a frozen install succeeds. Verified both directions locally: with the flag + --frozen-lockfile reproduces the CI failure; without it, a frozen install completes and the lockfile's overrides stay intact. Note the workaround this replaces would have been actively harmful: installing with --no-frozen-lockfile resolves the mismatch by regenerating the lockfile WITHOUT the overrides, silently reverting the app to the vulnerable transitive versions #649 pinned away.
thymikee
added a commit
that referenced
this pull request
Jul 16, 2026
…ance oracle (#1289) * test: replace the hand-typed Maestro fixture with a generated conformance oracle Closes #1274. The old harness (scripts/maestro-conformance*) compared 5 hand-authored flows against a hand-typed transcription of Maestro 2.5.1's command model. It proved parser self-consistency, not conformance: all four bug classes that cost #1217 days of live debugging slipped past it by construction, and it verified no upstream SHAs despite parsing them. Every expected value here is generated from the pinned upstream artifacts. dev.mobile:maestro-orchestra:2.5.1 is published on Maven Central, so the harness runs the real parser and reads the real bytecode — no full Maestro source build. Layer 1 (parser): a Gradle/Kotlin harness drives the pinned YamlCommandReader over a corpus of 42 vendored maestro-test flows (sha256-recorded) plus authored bug-class, coverage, and invalid flows, capturing each parse. The verifier parses each flow with the live engine and classifies it identical / both-reject / we-reject / mismatch / we-are-lenient. Every non-identical outcome must be a declared divergence, so the 17 we-reject entries in expected-divergence.ts are the mechanical parity backlog (assertTrue, clipboard, travel, killApp, and option-level gaps) rather than silent drift. Layer 2 (semantics): ASM reads static-final constants straight from the pinned bytecode without initializing driver classes (MAX_RETRIES_ALLOWED=3, SCREENSHOT_DIFF_THRESHOLD=0.005, ANIMATION_TIMEOUT_MS=15000, erase cap, and the iOS pre-tap gate we intentionally omit), plus the parser-observed 400ms swipe default. Each is cross-checked against MAESTRO_COMPATIBILITY_PRESETS. Layer 3 (differential): scheduled device scenarios. Cross-engine comparison is outcome parity only and says so; finer behavior is asserted engine-side via invariants over replay-timing.ndjson. Bug class 4's detector — a tap must not consume the whole settle budget, since a full-budget tap means the stability loop never latched while the flow still passes — is pure and unit-tested against synthetic traces; only the device run is scheduled-only. regenerate.mjs verifies the pinned jar SHA-256s before trusting output and is byte-deterministic across runs. Layers 1-2 verify in normal CI via node --test with no Java (the job installs deps: unlike the layering guard it copies, the verifier parses with the live engine, which imports the `yaml` package). Acceptance: the four bug classes each have a fixture; every command in SUPPORTED_MAESTRO_COMMAND_NAMES (the parser's own dispatch table, now exported as the single source of truth) is corpus-covered or listed unverified; the five documented deviations are expected-divergence entries. * fix: address review findings on the conformance oracle P1 — layer-3 scenarios could never run. They pointed at layer-1 corpus flows, which exist only to be PARSED: they name a fictional com.example.app and elements that exist on no device. A device run would have failed before exercising any runtime behavior, making bug class 4's detector silently vacuous. Layer 3 now has its own flows under differential/flows/ driving the real fixture app (examples/test-app, com.callstack.agentdevicelab); the workflow builds and installs it and hard-fails if it is missing. A test enforces the separation so a scenario can never point back at the parse corpus. Nothing else in this repo builds or installs the Expo fixture app, so those steps are new and unproven. The workflow is therefore dispatch-only: the cron is removed until a supervised first run proves the path. A nightly job that fails at 05:00 every day teaches nothing. P2 — layer 3 installed whatever version the online installer served. It now pins MAESTRO_VERSION from pinned-upstream.json, so layer 3 cannot drift from the version layers 1-2 claim, and asserts `maestro --version` matches. P2 — fixture content was not bound to regeneration. CI compared only the embedded upstream metadata, so a hand edit to a captured command or constant passed: the transcription failure mode this oracle exists to remove. Two-layer fix, because per-PR CI must stay Java-free and cannot re-derive: - Each fixture now carries a contentHash seal that the verifier recomputes, so editing a capture breaks the build. Tamper-evident, and tested by actually tampering rather than assuming a hash comparison works. - New scheduled conformance-regenerate job re-runs the harness against the pinned jars and fails on any byte difference. Forgery cannot survive a real re-derivation. This is what makes "generated from upstream" enforced. P3 — boot-ios-test-simulator requires runtime-version; now passed alongside preferred-device-name, as the other iOS workflows do. * tmp: trigger layer-3 differential on this branch to prove the device path workflow_dispatch cannot run pre-merge (it registers from the default branch), so this temporary push trigger exists only to execute the never-run device path on the PR head and capture evidence. Removed before merge. * fix(ci): install the fixture app unfrozen for the layer-3 device run First live run of the device path failed at the very first step: ERR_PNPM_LOCKFILE_CONFIG_MISMATCH. CI implies --frozen-lockfile and the fixture app's lockfile is out of sync with its package.json overrides. No CI job has ever built examples/test-app, so that drift was never surfaced. * fix: drop --ignore-workspace from test-app:install (defeats #649 security overrides) The first live run of the layer-3 device path failed at ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, and the cause is a real latent bug rather than a stale lockfile. #649 moved the fixture app's `overrides` into examples/test-app/pnpm-workspace.yaml precisely because pnpm only honors overrides from a workspace root — they pin transitive deps (ws, brace-expansion, xmldom, postcss, uuid, shell-quote) to versions that clear Dependabot alerts. But `test-app:install` passes --ignore-workspace, which ignores that very file, so the overrides are dropped and no longer match the lockfile that has them baked in. It goes unnoticed locally because interactive installs are not frozen, and no CI job has ever installed this app. Dropping --ignore-workspace makes examples/test-app resolve as its own workspace root (it has its own pnpm-workspace.yaml and is not a member of the repo-root workspace), so the overrides apply and a frozen install succeeds. Verified both directions locally: with the flag + --frozen-lockfile reproduces the CI failure; without it, a frozen install completes and the lockfile's overrides stay intact. Note the workaround this replaces would have been actively harmful: installing with --no-frozen-lockfile resolves the mismatch by regenerating the lockfile WITHOUT the overrides, silently reverting the app to the vulnerable transitive versions #649 pinned away. * fix: make layer-3 scenarios prove what they claim, and parse the Maestro version Run 3 (29497919702) got the whole device path working: Expo build (30m), app installed, simctl check, pinned Maestro CLI install. Only the version ASSERTION failed — `maestro --version` prints an analytics banner before the version, and `tr -d '[:space:]'` mashed banner+version into one string. The CLI was correctly 2.5.1. Match the semver line instead, and set MAESTRO_CLI_NO_ANALYTICS (CI should not phone home). Verified the parse against the exact CI output: banner and clean forms both yield 2.5.1, wrong/empty still fail. tap-retry-if-no-change was vacuous: it tapped a navigating control, so the first tap always succeeded and retryIfNoChange never ran — it passed while proving nothing. It now taps the app's non-interactive title so the screen cannot change and the retry path is forced, and asserts tapRetries >= 1 from the trace (MaestroRuntimeMetrics already records it per step). A new metricAtLeast invariant kind carries the assertion; a test reproduces the old vacuity. percent-swipe no longer claims bug class 1. Truncation vs rounding is a <=1px delta that no app-observable device outcome can distinguish, so pass/pass could never back that claim up. The runtime half is instead pinned exactly by a pure unit test of resolveMaestroCoordinate (it short-circuits on a known viewport, so no device is needed) — verified to catch the regression by flipping trunc->round, which turns 3 of 6 tests red. Truncation had no test coverage at all before this. A test now forbids any device scenario from re-claiming bug class 1. * fix(ci): pass --maestro and match the fixture app's real UI in layer-3 flows Run 4 (29500262301) reached the differential itself — build, install, simctl check and the pinned Maestro 2.5.1 verification all passed — and surfaced two real bugs, both mine: 1. The runner invoked `agent-device test <flow>` without --maestro, so every scenario failed with "test does not support this file type". The repo's own scripts/run-test-app-maestro-suite.mjs passes it; the flag is what routes a .yaml through the Maestro compat engine. 2. settle-after-tap and percent-swipe assumed home-open-form is on screen at launch. It is not: real Maestro reported "Element not found: home-open-form", and the app's own helper flow scrolls it into view first. settle-after-tap now scrolls before tapping, mirroring that helper; percent-swipe no longer navigates at all and swipes the scrollable home screen, so it tests the conversion and nothing else. The remaining two flows already reported maestro=pass, so only the agent-device invocation was wrong for those. Note the settle invariant correctly reported "no-data: no completed tapOn steps" and FAILED rather than passing — a detector that cannot run is a failure, as intended. * feat: declare layer-3 divergences and schedule the differential Layer 3 ran both engines for the first time (29504440599) and immediately found a real engine bug. Blocking the measurement instrument on repairing what it just measured inverts the dependency, so layer 3 now gets the contract layer 1 already had: every divergence is a decision on the record. Adds `knownDivergence: { reason, tracking }` to the scenario type — the layer-3 twin of FLOW_DIVERGENCES. A declared divergence keeps the run green; only UNDECLARED ones fail. Two rules stop that from rotting, both enforced mechanically rather than by prose discipline: - `tracking` is required and must be a real issue URL (run.test.ts), because a declaration with nothing behind it is how "temporarily expected" becomes permanent without anyone deciding to. - a stale declaration FAILS: if a declared-divergent scenario starts passing, the run goes red until the declaration is removed. The fix PR must delete it, and the differential then enforces the gap stays closed — the oracle is the acceptance test for its own findings. Declared: - settle-after-tap -> #1299. Our scrollUntilVisible times out finding home-open-form where Maestro 2.5.1 scrolls to it and passes. Real engine correctness bug in an advertised command, found by this differential. Blocks bug class 4's device detector until fixed. - tap-retry-if-no-change -> #1300. The invariant caught the scenario being vacuous: both engines pass but tapRetries was 0, so retryIfNoChange never ran. Needs an inert fixture control; a scenario defect, not an engine one. Proven green on both engines and enforced now: percent-swipe, optional-warned-not-failed — the latter is real device-verified warned-vs-failed parity. With declarations in place the differential is green, so the schedule goes in (cron 05:00) per #1274. A green run still prints what it is not proving. * fix: park the flaky retry scenario instead of declaring it a divergence Run 29510020718 fired the stale-declaration guard on its first outing and caught my own mistake. tap-retry-if-no-change measured tapRetries=0 in run 29504440599 and tapRetries=1 in 29510020718 — same flow, same commit. So it is not vacuous as #1300 originally claimed: it is NON-DETERMINISTIC. The tap sometimes holds the hierarchy signature still and sometimes does not, because the fixture home screen carries live content. That exposes a real limit of the mechanism added in the previous commit: knownDivergence assumes the divergence REPRODUCES. A declared-but-flaky scenario flips between known-divergence (green) and stale-declaration (red) at random — a coin-flip scheduled job, which is worse than no scenario because it teaches people to ignore the differential. So the scenario is parked, not declared. The flow and the tapRetries invariant stay implemented and unit-tested, so the fix PR only re-adds the scenario once the fixture has an inert control. retryIfNoChange therefore has NO device coverage right now — tracked in #1300 and stated plainly rather than disguised by a green run. A test keeps it out of the active set until then. #1300 updated with the corrected diagnosis and both runs' evidence. Active differential: settle-after-tap (declared divergence, #1299), percent-swipe and optional-warned-not-failed (both enforced, pass/pass on real devices). * fix: make a knownDivergence waiver cover exactly one failure, not any failure P1 from re-review, and a real flaw: the code did not do what its own comment claimed. runScenario() collapsed every unexpected outcome and every invariant failure into `misbehaved`, then turned ANY of them green if the scenario carried a declaration. So while the #1299 scrollUntilVisible waiver is open, upstream Maestro could start failing too — or a different invariant could break — and the scheduled job would still report known-divergence and pass. A waiver for one bug was silently amnesty for the next. That is the exact failure this oracle exists to prevent, committed one commit after building the guard against it. knownDivergence now requires an `expected` signature: both engines' outcomes plus each declared invariant's status. The runner matches it exactly — - matches -> known-divergence (green, tracked) - misbehaves differently -> failed (red): not the failure the waiver covers - stops misbehaving -> stale-declaration (red): remove the declaration #1299's signature pins what runs 29504440599/29510020718 actually observed: maestro=pass, agent-device=fail, settle invariant no-data. Tests prove unrelated failures stay red under an open waiver: upstream also failing, our engine unexpectedly passing, a different invariant status, and a new invariant appearing are each NOT covered. A signature where both engines pass is rejected outright as describing no divergence. Also retains replay-timing.ndjson as a run artifact (review evidence note): the invariants are computed from that trace, so a report saying "tapRetries was 0" cannot be audited once the runner is gone without it. * perf(ci): cache the fixture app build for the layer-3 differential The differential job took ~30 minutes, of which 1331s (22 min, 79%) was building the Expo fixture app and only 347s was the differential itself — rebuilt from scratch on every run for an app that changes almost never. Cache the built .app, keyed on everything that can change the binary: the app's sources, native config, dependency graph, the build step itself, the iOS runtime, and the Xcode version. Mirrors the existing setup-apple-replay prebuilt-runner cache (same action pin, same Xcode-key + source-hash shape). On a hit the build is skipped entirely and the bundle is installed straight onto the booted simulator (~seconds), taking the job to roughly 8 minutes. On a miss it falls back to exactly the previous behaviour and repopulates, so the worst case is unchanged. The existing simctl verification still gates both paths, so a bad cache cannot produce a vacuous green: if the app is not installed, the job fails loudly rather than running scenarios against nothing. Note the first run after this lands is necessarily a miss. * refactor(ci): extract setup-fixture-app so any job can use the cached app The fixture-app build + cache was inline in the differential workflow, so nothing else could reach it. Extracted to a composite action mirroring setup-apple-replay, because the capability is what #320 has been missing: it wants replay coverage moved off Apple system apps onto a controlled fixture with stable ids, and that fixture (examples/test-app) already exists — CI just had no way to build and install it. The cache is genuinely shared. GitHub caches are per-repository and readable across workflows, and a run restores from its own branch or the default branch, so once a run on main populates it every workflow gets the hit and only the first one pays the ~22 minutes. The key is computed inside the action from a fixed input list and deliberately contains nothing caller-specific — folding a caller's workflow path into it would silently unshare the cache. Also removes a duplication risk: the action reads the bundle id from the built app's Info.plist rather than hardcoding it, so it cannot drift from what was actually built, and it fails loudly if the app is not installed. The conformance workflow keeps its own narrower assertion — that the installed id is the one its scenarios target — since that is its concern, not the action's. Usage: - uses: ./.github/actions/setup-fixture-app with: runtime-version: ${{ env.IOS_RUNTIME_VERSION }} # outputs: app-path, app-id, cache-hit * chore(ci): remove the temporary branch push trigger Run 29519848340 on this head executed both engines against the real fixture app and came back green, so the trigger that existed only to prove the never-run device path has done its job. Merged config is now cron (05:00) + workflow_dispatch, as required by #1274. known-divergence settle-after-tap maestro=pass agent-device=fail (#1299) ok percent-swipe maestro=pass agent-device=pass ok optional-warned-not-failed maestro=pass agent-device=pass This commit will not itself trigger a run: GitHub evaluates triggers at the pushed commit, and the push trigger is gone in it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves all 4 open Dependabot alerts, all transitive deps in
examples/test-app/pnpm-lock.yaml.The
postcss/uuidoverrides added in #464 had stopped applying: once test-app ended up nested under the repo-rootpnpm-workspace.yaml, pnpm only honored overrides from a workspace root, so test-app'spackage.jsonpnpm.overrideswere silently ignored and the lockfile drifted back to vulnerable versions.Fix
examples/test-app/pnpm-workspace.yamlso test-app is its own pnpm root and the overrides are actually honored (the canonical location in pnpm 10+).pnpmfield frompackage.json(single source of truth).postcssuuidws@8brace-expansion@5maxbypasswsandbrace-expansionoverrides are scoped to the vulnerable majors (ws@8,brace-expansion@5) so the non-vulnerablews@7/brace-expansion@1copies elsewhere in the tree are left untouched.Notes
uuid7→14 (forced by the existing override, used byxcode). API-compatible —uuid.v1()/v4()named exports work viarequireacross v7→v14.