diff --git a/.agents/skills/rea-ci-tests-audit/SKILL.md b/.agents/skills/rea-ci-tests-audit/SKILL.md new file mode 100644 index 000000000..4d7b44bf5 --- /dev/null +++ b/.agents/skills/rea-ci-tests-audit/SKILL.md @@ -0,0 +1,112 @@ +--- +name: rea-ci-tests-audit +description: Audit REA CI and test suites for correctness, fixture drift, false passes, and measured performance; use for suite or workflow investigations and efficiency changes, rather than ordinary feature testing or a single test failure. +--- + +# REA CI and tests audit + +Establish which checks still catch real regressions and which costs can be +removed without losing that protection. The outcome may be a verified fix, +an evidence-backed recommendation, or an explicit decision to stop. + +Follow the checkout's `AGENTS.md`, [testing guide](../../../docs/testing.md), +and [contribution rules](../../../CONTRIBUTING.md). Use their current commands +and pinned toolchain, rather than commands remembered from an earlier audit. +Preserve the requested scope: investigation alone does not authorize edits; +this skill does not grant permission to publish issues, push, or merge. Existing +user authorization still applies. Keep audit notes and raw results outside the +tracked tree, normally in a task-specific `/tmp` directory. + +## Establish the executed baseline + +Fetch when the user asks for latest main. Pin the baseline SHA and inspected +workflow revision; use an isolated worktree when changing or comparing code. +Do not modify another task's checkout. Record dirty changes and the exact +command, toolchain, host, selected tests, instrumentation, and prerequisites. +An old run is evidence for its executed revision, even if main advances. + +Trace workflow triggers, changed-path selection, job dependencies, reusable +actions, package scripts, test discovery, artifact producers, and report gates +through to the commands that actually execute. Include scripts, generated data, +runtime registration, and subprocess boundaries missing from import graphs. +Check positive and negative selection paths: required files must be discovered +once per intended test configuration; deliberate platform/configuration repeats +are separate coverage. A missing explicit test or missing required report must +not turn green. Optional lanes may skip; preserve their stated prerequisites. + +Use existing timing and failure artifacts to select suspicious families. Run +relevant checks to establish a baseline; reserve a full local suite for broad +CI diagnosis or uncertainty that focused checks cannot resolve. Do not rebuild +or rerun an unchanged complete suite after every documentation edit. + +## Prove the failure or protection + +For each candidate, state the expected observable behavior and its owner, +then construct the smallest reproduction against production behavior. Follow +the producer and affected consumers before deciding where to fix it. + +- For drift, compare fixtures with current admission requirements, protocol + replies, named contracts, tool versions, and actual public entrypoints. A + plausible-looking mock can fail before the behavior its test claims to cover. + Search the whole affected fixture family, including other version strings. + Preserve intentionally malformed fixtures and test their intended rejection. +- For a suspicious green test, perturb the relevant behavior in an isolated + copy or inject a controlled failure. Verify that the existing assertion or + gate detects it. A test may merely repeat its mock, import abandoned code, + or hide a subprocess/checker failure. Restore mutations after the experiment. +- For shared setup or pruning, map distinct assertions to their retained + scenarios. Preserve malformed inputs, cancellation, cleanup failure, + permissions, capacity, identity, and meaningful concurrency interactions. + A faster run that skips a case or weakens its oracle is not an improvement. +- For provider coverage, separate fake-producer, boundary, and actual-engine + evidence. Inspect named skips and prerequisites; fixtures do not establish + real Ghidra, LLDB, browser, or other provider behavior. + +Classify findings as reproduced defects, supported costs, hypotheses, or +unavailable evidence. Record the baseline command, observed output/exit status, +expected result, affected scope, and competing explanation. A failure on main +may be pre-existing; that does not by itself make it irrelevant to a CI audit. +Attribute PR failures using the executed revision and a clean baseline, not +just their resemblance to a previous failure. Retry only to test a specific +transient explanation; retain the original failure and stop when repetition +adds no evidence. A passing retry does not establish a root cause or a fix. + +## Assess efficiency without losing the gate + +Read [measurement and CI checks](references/measurement-and-ci.md) when +investigating slowness, caching, runner waits, shared artifacts, or log noise. +Compare equivalent completed work before claiming a speedup. Distinguish +setup/build time, test execution, coverage instrumentation, dependency waits, +and runner scheduling; state which costs remain unmeasured. + +Prefer removing proven duplicate preparation, reusing immutable setup, and +reducing repeated subprocess work before shrinking correctness coverage. +Preserve independent sessions where lifecycle or concurrency is the oracle. +Keep live tests fresh; a cached previous pass cannot validate current host, +installed provider contents, permissions, or external state. + +## Fix, verify, and hand off within scope + +If implementation is authorized, make the smallest structural change at the +owning boundary and add a regression that fails on the baseline for the right +reason. Retest affected consumers and meaningful negative paths; verify cache +invalidation or discovery when those mechanisms change. Read the installed +Turbo documentation before changing its configuration or commands. + +Run the repository's applicable checks and inspect complete current-head CI +results before an authorized merge. Preserve required coverage and discovery +gates. Do not suppress a failure, add blanket retries, raise timeouts, or delete +distinct assertions solely to make CI green. Changes to these controls need +their own demonstrated reason and retained failure detection. + +Report each confirmed issue with exact revision, reproduction, consequence, +proposed correction, and validation limits. Report performance with workload +and measurements, not just a percentage. Use English issue/PR records when +requested; follow the user's assignment, commit identity, review, and merge +preferences without assuming them for other users. Open ready-for-review PRs +only when that lifecycle is authorized. Resolve necessary review findings and +check final main against the tested tree after merging. + +Stop when the authorized work is verified, remaining benefits are unproven, +or further changes would trade correctness for speculative speed. List genuine +uncertainties without turning them into additional optimization rounds. diff --git a/.agents/skills/rea-ci-tests-audit/agents/openai.yaml b/.agents/skills/rea-ci-tests-audit/agents/openai.yaml new file mode 100644 index 000000000..366dd1229 --- /dev/null +++ b/.agents/skills/rea-ci-tests-audit/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "REA CI and tests audit" + short_description: "Audit CI/test correctness and measured performance" + default_prompt: "Use $rea-ci-tests-audit to investigate CI and test correctness, drift, and performance before proposing changes." diff --git a/.agents/skills/rea-ci-tests-audit/references/measurement-and-ci.md b/.agents/skills/rea-ci-tests-audit/references/measurement-and-ci.md new file mode 100644 index 000000000..e86ba9a4d --- /dev/null +++ b/.agents/skills/rea-ci-tests-audit/references/measurement-and-ci.md @@ -0,0 +1,65 @@ +# Measurement and CI checks + +## Equivalent completed work + +Record the source/workflow SHA, command, test identities and counts, skips, +assertions or output contract, exit status, coverage scope and thresholds, +toolchain/provider versions, host resources, and prepared artifacts. Inspect +both the child result and its report: a profiling wrapper may exit successfully +after the measured command failed. Focused tests can pass while global coverage +thresholds fail; describe both rather than calling the command green. + +Measure wall time, user/system CPU, and peak process-family memory for relevant +completed tasks. Parent-only RSS misses workers and their children. On Linux, +summed RSS can double-count shared pages; report RSS/PSS semantics, sampling +interval, missing samples, and observed cleanup limits. Use equivalent host +tools elsewhere. State unknown costs, including agent tokens; fewer log bytes +alone prove neither fewer tokens nor faster tests. + +Separate cold execution, warm build/file caches, and warm runtime caches. +Use repeated or alternating runs when variance affects the decision, with +enough observations to distinguish the proposed benefit from noise. Keep +workload, instrumentation, and host conditions comparable; record interference. +A run with fewer tests, early rejection, lost coverage, or a different runner +does not prove suite acceleration. CI-wide claims need end-to-end measurements, +not an extrapolation from one improved family. + +## Workflow, caches, and shared artifacts + +Trace each reused result from producer to consumers and identify its lifetime. +Check key/input coverage against the actual file inventory and result-affecting +configuration, environment, generated data, tool versions, and host state. +Exercise an affected input change to prove invalidation; exercise an irrelevant +change to support reuse where useful. Cache deterministic preparation and +static analysis only when their inputs fully describe the result. + +Verify downloaded artifacts match the expected revision, platform, runtime, +and schema. Check restore misses, failed/partial producers, stale reports, +removed output files, and consumers after producer failures. Shared writers +need output ownership or locks; tests need isolated state, not an unnecessary +global lock. Worker counts do not bound children spawned within each test. +Preserve intentionally concurrent scenarios while bounding unrelated batches. + +Check matrix expansion and all job pages before saying CI passed. Inspect +required gates after failed, cancelled, and skipped dependencies. `always()` +alone does not preserve failures; inspect the aggregation logic. Test/report +uploads should retain available diagnostics after failure, while absent or +malformed required results must fail closed. Do not demand an optional report +from a lane intentionally excluded by the selector. + +## Runner waits and console noise + +Use per-job and per-step timestamps where available. Workflow creation to job +start includes dependencies and other delays; it is not necessarily runner +queue time. Identify when the job became eligible before attributing waits to +runner capacity. If eligibility/queue timestamps are unavailable, report the +combined delay and unknown split. Compare equivalent lanes and runner types +across enough runs; a single busy runner is not a workflow regression. + +For console reductions, retain checker exit codes, actionable errors, useful +summary counts, and complete machine-readable artifacts. Verify authored code +remains in the actual checker inventory when excluding generated outputs. +Introduce an authored error as a control; it must still fail. An ignore pattern +can hide real source as well as noise. Show that cached reports are restored, +stale successful reports cannot mask current failures, and report parse errors +remain failures. Measure readability or byte reduction separately from runtime. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 50674afa7..28e8788bd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -202,6 +202,10 @@ CI diagnosis. Packaging/setup/distribution changes additionally require `npm run verify:package` and `npm pack --dry-run`; provider changes require the matching real-provider lane. State which host/provider workflows ran. +For CI/test correctness, drift, or performance investigations, use the +[`rea-ci-tests-audit` contributor skill](.agents/skills/rea-ci-tests-audit/SKILL.md). +It guides reproduction and comparable measurements before changing checks. + `check:pr` reuses build and static-check outputs while executing the test suite fresh, including the fast Python producer checks. `docs:check` also runs the onboarding example consistency suite for README-only changes. diff --git a/src/domain/webBundleAnalyzer.test.ts b/src/domain/webBundleAnalyzer.test.ts index ad82b5730..3361cc366 100644 --- a/src/domain/webBundleAnalyzer.test.ts +++ b/src/domain/webBundleAnalyzer.test.ts @@ -320,7 +320,9 @@ describe("web bundle API classification parity", () => { "users", ]); }); +}); +describe("web bundle configuration and navigation parity", () => { it("keeps configuration setting reads out of endpoints in both analyzers", () => { const source = ` import vscodeDefault from "vscode"; diff --git a/src/ghidra/GhidraFunctionValues.test.ts b/src/ghidra/GhidraFunctionValues.test.ts index e54692840..e7e5d8e9d 100644 --- a/src/ghidra/GhidraFunctionValues.test.ts +++ b/src/ghidra/GhidraFunctionValues.test.ts @@ -117,7 +117,9 @@ describe("Ghidra function-analysis result values", () => { }); }, ); +}); +describe("Ghidra reference and value-flow completeness", () => { it.each(["reference_kinds_available", "unresolved_calls"])( "rejects omitted %s instead of inventing complete reference observations", (field) => { diff --git a/src/process/ProviderDeadline.test.ts b/src/process/ProviderDeadline.test.ts index 0a9448150..2ecd6402c 100644 --- a/src/process/ProviderDeadline.test.ts +++ b/src/process/ProviderDeadline.test.ts @@ -34,14 +34,34 @@ it("aborts a bounded wait when its real monotonic deadline expires", async () => } }); -it("reports no remaining time once its timer fires before the monotonic deadline", () => { +it("does not expire a future monotonic deadline on an early timer callback", async () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); - const now = vi.spyOn(performance, "now").mockReturnValue(1_000); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); const deadline = new ProviderStartupDeadline(40); try { - // Timers may fire while the monotonic clock is still just short of the deadline. - now.mockReturnValue(1_039.6); - vi.advanceTimersByTime(40); + monotonic = 39; + await vi.advanceTimersByTimeAsync(40); + expect(deadline.remainingMs()).toBe(1); + expect(deadline.signal.aborted).toBe(false); + expect(deadline.interruption).toBeUndefined(); + } finally { + deadline.dispose(); + vi.useRealTimers(); + } +}); + +it("aborts when the re-armed timer reaches the absolute deadline", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const deadline = new ProviderStartupDeadline(40); + try { + monotonic = 39; + await vi.advanceTimersByTimeAsync(40); + expect(deadline.signal.aborted).toBe(false); + monotonic = 40; + await vi.advanceTimersByTimeAsync(1); expect(deadline.signal.aborted).toBe(true); expect(deadline.interruption).toBe("timeout"); expect(deadline.remainingMs()).toBe(0); @@ -50,3 +70,112 @@ it("reports no remaining time once its timer fires before the monotonic deadline vi.useRealTimers(); } }); + +it("reports a wait that resolves at the deadline as aborted, not elapsed", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const deadline = new ProviderStartupDeadline(40); + try { + const pending = deadline.wait(500); + monotonic = 40; + await vi.advanceTimersByTimeAsync(40); + await expect(pending).resolves.toBe("aborted"); + } finally { + deadline.dispose(); + vi.useRealTimers(); + } +}); + +it("resolves a polling interval shorter than the budget as elapsed", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const deadline = new ProviderStartupDeadline(40); + try { + const pending = deadline.wait(10); + monotonic = 10; + await vi.advanceTimersByTimeAsync(10); + await expect(pending).resolves.toBe("elapsed"); + expect(deadline.signal.aborted).toBe(false); + expect(deadline.remainingMs()).toBe(30); + } finally { + deadline.dispose(); + vi.useRealTimers(); + } +}); + +it("keeps caller cancellation classified without reporting an exhausted budget", () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const caller = new AbortController(); + const deadline = new ProviderStartupDeadline(40, caller.signal); + try { + monotonic = 20; + caller.abort(new Error("caller stopped startup")); + expect(deadline.signal.aborted).toBe(true); + expect(deadline.interruption).toBe("cancelled"); + expect(deadline.remainingMs()).toBe(20); + } finally { + deadline.dispose(); + vi.useRealTimers(); + } +}); + +// Probe from issue #2760 review: both the abort timer and the clamped polling +// timer can fire early at the same monotonic instant. The full-budget wait must +// stay pending until the absolute deadline expires. +it("keeps a full-budget wait pending when both timers fire early", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const caller = new AbortController(); + const deadline = new ProviderStartupDeadline(40, caller.signal); + let outcome: string | undefined; + const pending = deadline.wait(500).then((result) => { + outcome = result; + return result; + }); + try { + monotonic = 39; + await vi.advanceTimersByTimeAsync(40); + expect(deadline.signal.aborted).toBe(false); + expect(deadline.remainingMs()).toBe(1); + expect(outcome).toBeUndefined(); + monotonic = 40; + await vi.advanceTimersByTimeAsync(1); + await expect(pending).resolves.toBe("aborted"); + expect(deadline.remainingMs()).toBe(0); + } finally { + caller.abort(); + deadline.dispose(); + vi.useRealTimers(); + } +}); + +it("releases the timer and listener on dispose with a pending wait", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + let monotonic = 0; + vi.spyOn(performance, "now").mockImplementation(() => monotonic); + const caller = new AbortController(); + const deadline = new ProviderStartupDeadline(40, caller.signal); + try { + const pending = deadline.wait(500); + deadline.dispose(); + // The external abort listener is removed: caller cancellation after + // dispose must not abort the deadline or classify it as cancelled. + caller.abort(new Error("caller stopped startup")); + expect(deadline.signal.aborted).toBe(false); + expect(deadline.interruption).toBeUndefined(); + monotonic = 100; + await vi.advanceTimersByTimeAsync(100); + expect(deadline.signal.aborted).toBe(false); + // The deadline expired while disposed, so the clamped wait resolves + // "aborted" from the monotonic clock instead of polling past expiry. + await expect(pending).resolves.toBe("aborted"); + } finally { + deadline.dispose(); + vi.useRealTimers(); + } +}); diff --git a/src/process/ProviderDeadline.ts b/src/process/ProviderDeadline.ts index a6f10779a..83e24a807 100644 --- a/src/process/ProviderDeadline.ts +++ b/src/process/ProviderDeadline.ts @@ -61,13 +61,7 @@ export class ProviderStartupDeadline { return; } signal?.addEventListener("abort", this.#onExternalAbort, { once: true }); - this.#timer = setTimeout(() => { - if (this.#controller.signal.aborted) return; - this.#interruption = "timeout"; - this.#abort( - new DOMException("Provider startup deadline elapsed", "TimeoutError"), - ); - }, timeoutMs); + this.#armDeadlineTimer(timeoutMs); } /** Composite signal aborted by either the caller or the absolute deadline. */ @@ -85,18 +79,39 @@ export class ProviderStartupDeadline { /** Milliseconds still available to the next startup phase. */ remainingMs(): number { - // Node timers can fire a fraction of a millisecond before the monotonic - // deadline, so an elapsed timer must not report time left to spend. - if (this.#interruption === "timeout") return 0; return Math.max(0, Math.ceil(this.#deadlineAt - performance.now())); } - /** Wait for at most one polling interval under the shared deadline signal. */ + /** + * Wait for at most one polling interval under the shared deadline signal. + * + * Both timers may fire before their monotonic targets. Retain the earlier + * of the polling target and deadline, and wait again for any residual time. + */ wait(milliseconds: number): Promise { - return waitForAbortableDelay( - Math.min(milliseconds, this.remainingMs()), - this.signal, - ); + const startedAt = performance.now(); + // Absolute target: the earlier of the requested polling interval and the + // absolute monotonic deadline. + const targetAt = Math.min(startedAt + milliseconds, this.#deadlineAt); + const poll = (): Promise => { + const budget = this.remainingMs(); + const residual = Math.max(0, targetAt - performance.now()); + return waitForAbortableDelay( + Math.min(residual, budget), + this.signal, + ).then((result) => { + if (result === "aborted") return "aborted"; + const now = performance.now(); + // The monotonic deadline expired: a full-budget wait resolves as an + // abort so polling stops at the boundary. + if (now >= this.#deadlineAt) return "aborted"; + // The polling interval elapsed without expiring the deadline. + if (now >= targetAt) return "elapsed"; + // A timer fired before the absolute target; keep waiting. + return poll(); + }); + }; + return poll(); } /** Release the deadline timer and external abort listener. */ @@ -115,4 +130,25 @@ export class ProviderStartupDeadline { if (this.#timer !== undefined) clearTimeout(this.#timer); this.#timer = undefined; } + + /** + * Arm (or re-arm) the timer that expires the absolute monotonic deadline. + * + * Re-check the clock before classifying expiry, and retain the residual + * budget when the timer arrives early. + */ + #armDeadlineTimer(milliseconds: number): void { + this.#timer = setTimeout(() => { + this.#timer = undefined; + if (this.#controller.signal.aborted) return; + if (performance.now() >= this.#deadlineAt) { + this.#interruption = "timeout"; + this.#abort( + new DOMException("Provider startup deadline elapsed", "TimeoutError"), + ); + return; + } + this.#armDeadlineTimer(this.remainingMs()); + }, milliseconds); + } } diff --git a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts index ea8fb964c..98c231240 100644 --- a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts +++ b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts @@ -486,7 +486,9 @@ describe("Ghidra Windows launcher", () => { }), ).toThrow(/metacharacters/u); }); +}); +describe("Ghidra Windows launcher runtime", () => { it.skipIf(process.platform !== "win32").each([ { properties: diff --git a/tests/fixtures/javaClassFile.ts b/tests/fixtures/javaClassFile.ts index 30f5ffc37..29fdace98 100644 --- a/tests/fixtures/javaClassFile.ts +++ b/tests/fixtures/javaClassFile.ts @@ -1,5 +1,8 @@ /** Build a valid minimal JVM class file, optionally with an unused UTF-8 constant. */ -export const javaClassFile = (extraUtf8Bytes = 0, fatCpuLookalike = false): Buffer => { +export const javaClassFile = ( + extraUtf8Bytes = 0, + fatCpuLookalike = false, +): Buffer => { const utf8 = (value: string): Buffer => { const bytes = Buffer.from(value, "utf8"); const entry = Buffer.alloc(3 + bytes.length);