Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion docs/gate/compiler.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ doc_type: spec
status: draft
owner: B3
created: 2026-07-25
updated: 2026-07-27
updated: 2026-07-28
confidence: MED
supersedes: null
sources_verified: true
Expand Down Expand Up @@ -128,6 +128,23 @@ listed and the fallback was useless. It is now genuinely visibility-filtered, an
`page-state` run **one** enumeration (`src/shared/landmarks.ts`) rather than two that agreed
only on markup with redundant `role=` attributes.

### `timeout_ms` is part of strength, not a performance knob

Every synthesized assertion carries `DEFAULT_ASSERTION_TIMEOUT_MS` (5000 ms,
`src/compiler/assertions.ts`) — previously seven separate `5000` literals, now one named
constant, overridable per compile via `CompileOptions.assertionTimeoutMs`.

**The value is deliberately unmoved.** A shorter timeout is a *stricter* check and a longer one
laxer, so "tuning it for speed" would move step-level replay-validity — the number PRD §9 gates
on — while looking like a perf change. That is the shape the assertion-immutability invariant
forbids.

It is also the dominant term in worst-case replay latency, because
`src/runner/assertions.ts` spends the full budget on **failure**: a 12-step task with three
stale locators waits 3 × 5 s before repair even starts. That is a real cost worth revisiting —
but on evidence, after a measurement, not before one. `tests/unit/compiler.test.ts` pins the
emitted default so it cannot drift silently.

**Strength rule:** `strong` = unambiguous proof the step achieved its purpose;
`weak` = consistent with success but also with several failures. Weak is allowed
and **must stay labelled** (`strength` + `notes`). Silent promotion is forbidden
Expand Down
65 changes: 64 additions & 1 deletion docs/gate/runner.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ doc_type: spec
status: draft
owner: B4
created: 2026-07-24
updated: 2026-07-27
updated: 2026-07-28
confidence: MED
supersedes: null
sources_verified: true
Expand All @@ -30,6 +30,60 @@ repairs **actions only** on failure (≤2 repairs/run by default), and emits
| `replay.ts` | `ReplayRunner` — dry-run, repair loop, metrics emission |
| `metrics/` | Sibling package: emitter + §9 aggregates |

## Bounded waits

Every wait the runner performs has an explicit ceiling. One did not: a `wait` step with no
positive duration parameter called `page.waitForLoadState("networkidle")` with **no timeout**,
inheriting Playwright's 30s default — a number nobody here chose. If the page never goes quiet
for 500ms the step burned all 30s and then failed anyway: maximum latency for zero information.

Now bounded by `NETWORK_IDLE_WAIT_MS` (5000ms, `src/runner/actions.ts`), overridable per run via
`ReplayRunnerOptions.networkIdleWaitMs`. Measured in `tests/unit/runner-bounded-wait.test.ts`
against a page that never reaches idle:

| | unbounded (before) | bounded (after) |
| --- | --- | --- |
| default | 30.8 s | 5.0 s |
| 1s override | 30.0 s | 1.0 s |

**Honest scope.** The seeded Grafana dashboard does *not* trigger this — `networkidle` settles
there in ~3 ms (measured on 11.0.0, `/d/paragent-seed`, 2026-07-28), because the seed dashboard
sets no refresh interval and TestData is generated client-side. This is a **latent** worst case,
reachable on any surface with continuous polling, streaming, or websockets — not a hang observed
on the current test-bed. Bounding it is cheap insurance taken before the gate runs, not a fix
for a live symptom.

**It changes which steps pass.** A page that first goes quiet at 12 s held the step until it did
and does not now — at 5 s the step continues and the assertion decides on whatever is on screen.
Deliberate, and cheap *today* because no gate number exists (`gate:matrix` is dry-run only,
[#62](https://github.com/DevToolie/Paragent/issues/62)). After a published measurement it would
be an expensive silent shift.

### Reaching the bound is not a step failure

A parameterless `wait` is a settling **hint**. The step's post-condition is the assertion that
runs immediately after it, with its own `timeout_ms` budget. So when the bound elapses the step
**proceeds** and records `settled: false` (`ActionResult.settled`, surfaced as
`StepAttemptResult.notes`) — the same posture as the 250 ms idle probe in
`src/runner/page-state.ts`, where a timeout means *no claim* rather than failure.

Classifying it as `TIMEOUT` would be worse than slow. `replay.ts` routes every non-`PASS`
outcome into the repair loop, so a never-quiet page would fail deterministically at the bound,
consume both repair attempts, and land on `REPAIR_EXHAUSTED` — and no `corrected_action` can
make a polling page go quiet. The run's `success_with_le_2_repairs` would then be reporting a
scaffolding condition as churn, which is the one thing the gate number must not do. On exactly
the surfaces this bound exists for (polling, streaming, websockets), the bound would otherwise
make a doomed step fail 6× faster without making it any less doomed.

If the page really is broken, nothing is hidden: the assertion fails on its own evidence, and
*that* failure is worth a repair attempt. And a step that genuinely needs idle as its
post-condition can say so — `network-idle` is an assertion type
(`src/runner/assertions.ts`), where a timeout is a real failure because it was a real claim.

`tests/unit/runner-bounded-wait.test.ts` pins both halves: the clock (bounded, not 30 s) and the
classification (`repair_count: 0` on a never-idle page, with the note still recorded). Reverting
either fails it.

## Invariants

1. **Assertions are immutable in repair.** `deepFreeze` + `assertAssertionUnchanged` — proposals may only supply `corrected_action`.
Expand Down Expand Up @@ -78,3 +132,12 @@ npm run gate:report
- Whether walking eight versions changes anything the report can *conclude*. It does not — more
rows over the same hand-written 2-step program is a better-shaped denominator, not a
measurement. That waits on live execution ([#62](https://github.com/DevToolie/Paragent/issues/62)).
- **`settled: false` is recorded but not aggregated.** It reaches `StepAttemptResult.notes` in
memory and stops there: `metrics.schema.json` has no field for it, so nothing counts how often
a wait's hint went unanswered across a matrix run. Adding one is a contract change, and there
is no measurement yet to justify the shape. Until then a reader cannot tell "this run met a
never-quiet page eight times" from "never".
- Whether 5000 ms is the right *bound* rather than merely a chosen one. It equals
`DEFAULT_ASSERTION_TIMEOUT_MS` by coincidence, not by construction — two independent constants
in two packages, nothing enforcing the match. Neither number has been fitted to an observation
because no live run exists yet.
44 changes: 37 additions & 7 deletions src/compiler/assertions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,32 @@ import type {
} from "./types.js";
import { SCHEMA_VERSION } from "./types.js";

/**
* `timeout_ms` written onto every synthesized assertion.
*
* Was seven separate `5000` literals. Naming it is the change; **the value is
* deliberately unmoved.**
*
* A timeout is part of an assertion's *strength*, not a performance knob: a
* shorter one is a stricter check, a longer one a laxer one. Lowering this to
* make replay feel faster would raise the failure rate and move step-level
* replay-validity — the one number PRD §9 gates on — while looking like a perf
* tweak. That is the shape `docs/architecture.md` invariant 1 forbids.
*
* The runner spends this budget on *failure*
* (`src/runner/assertions.ts`), so it is also the dominant term in
* worst-case latency: a 12-step task with three stale locators waits 3 × this
* before repair even starts. Changing it is therefore a real decision, and it
* should follow a measurement rather than precede one. Override per-compile via
* `SynthesizeAssertionOptions.timeoutMs` if you need to explore that.
*/
export const DEFAULT_ASSERTION_TIMEOUT_MS = 5000;

export interface SynthesizeAssertionOptions {
/** Override for {@link DEFAULT_ASSERTION_TIMEOUT_MS}. */
timeoutMs?: number;
}

const ASSERTION_TYPES = new Set<AssertionType>([
"element-visible",
"text-matches",
Expand Down Expand Up @@ -89,7 +115,11 @@ interface SynthesisContext {
}

/** Synthesize one post-condition; expected values are templates with typed holes. */
export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
export function synthesizeAssertion(
ctx: SynthesisContext,
options: SynthesizeAssertionOptions = {},
): Assertion {
const timeout_ms = options.timeoutMs ?? DEFAULT_ASSERTION_TIMEOUT_MS;
const { trajectory, step, locatorChain } = ctx;
const hint = step.assertion_hint;
const primary = pickPrimaryLocator(locatorChain);
Expand Down Expand Up @@ -133,7 +163,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
param_types: { success_message: "string" },
regex_template: templateToRegex(template),
},
timeout_ms: 5000,
timeout_ms,
failure_classification: "assertion_failed",
notes:
"Recorder signalled toast/success copy. Expected text is a typed hole ({success_message}) — never a tenant/product-message literal. Strong when the runner binds a success-pattern allowlist; otherwise runtime bind quality is MED confidence.",
Expand All @@ -157,7 +187,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
template: "{item_count}",
param_types: { item_count: "integer" },
},
timeout_ms: 5000,
timeout_ms,
failure_classification: "assertion_failed",
notes:
"Count asserted via template hole. expected.count=0 is a schema placeholder until B2 emits structured counts in post_state; runner must bind the observed count. Labelled weak: absolute counts drift. Not a gate metric.",
Expand Down Expand Up @@ -210,7 +240,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
strength: "strong",
target: { locator: stripTenantFlagForTarget(primary) },
expected: { visible: false },
timeout_ms: 5000,
timeout_ms,
failure_classification: "assertion_failed",
notes:
"Recorder observed the acted-on control was no longer visible after the action (ADR-0007 post_action_target_visible=false). Asserts only that: the control is gone. Proves the step was not a no-op; proves nothing about downstream state.",
Expand Down Expand Up @@ -278,7 +308,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
strength,
target: { locator: stripTenantFlagForTarget(resolved) },
expected: { visible: true },
timeout_ms: 5000,
timeout_ms,
failure_classification: "assertion_failed",
notes,
};
Expand All @@ -305,7 +335,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
strength: strong ? "strong" : "weak",
target: { url_template },
expected,
timeout_ms: 5000,
timeout_ms,
failure_classification: "assertion_failed",
notes: strong
? "URL template changed (or navigate completed); matching post_state.url_template is strong evidence the step reached the intended surface."
Expand All @@ -319,7 +349,7 @@ export function synthesizeAssertion(ctx: SynthesisContext): Assertion {
assertion_id: assertionId,
type: "network-idle",
strength: "weak",
timeout_ms: 5000,
timeout_ms,
failure_classification: "timeout",
notes:
"network_idle in post_state is weakly consistent with success — idle also occurs on error pages and no-ops. Labelled weak.",
Expand Down
31 changes: 24 additions & 7 deletions src/compiler/compile.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { synthesizeAssertion } from "./assertions.js";
import {
synthesizeAssertion,
type SynthesizeAssertionOptions,
} from "./assertions.js";
import { buildLocatorFallbackChain } from "./locators.js";
import { decidePoolEligibility } from "./pool.js";
import {
Expand Down Expand Up @@ -50,13 +53,17 @@ export function compileStep(
trajectory: Trajectory,
step: TrajectoryStep,
compiledAt: string,
assertionOptions: SynthesizeAssertionOptions = {},
): CacheRow {
const { action, topologyOnly } = buildCompiledAction(step);
const assertion = synthesizeAssertion({
trajectory,
step,
locatorChain: action.locator_fallback_chain,
});
const assertion = synthesizeAssertion(
{
trajectory,
step,
locatorChain: action.locator_fallback_chain,
},
assertionOptions,
);
const pool = decidePoolEligibility({
chain: action.locator_fallback_chain,
assertion,
Expand Down Expand Up @@ -101,6 +108,12 @@ export interface CompileOptions {
compiledAt?: string;
inputPath?: string;
notes?: string;
/**
* Override the `timeout_ms` written onto every synthesized assertion.
* Defaults to `DEFAULT_ASSERTION_TIMEOUT_MS`. Read the note on that constant
* before changing it — it is an assertion-strength knob, not a perf one.
*/
assertionTimeoutMs?: number;
}

export function compileTrajectory(
Expand All @@ -117,9 +130,13 @@ export function compileTrajectory(
}

const compiledAt = options.compiledAt ?? new Date().toISOString();
const assertionOptions: SynthesizeAssertionOptions =
options.assertionTimeoutMs === undefined
? {}
: { timeoutMs: options.assertionTimeoutMs };
const rows = [...trajectory.steps]
.sort((a, b) => a.step_index - b.step_index)
.map((step) => compileStep(trajectory, step, compiledAt));
.map((step) => compileStep(trajectory, step, compiledAt, assertionOptions));

const bundle: CompiledTrajectoryBundle = {
schema_version: SCHEMA_VERSION,
Expand Down
7 changes: 6 additions & 1 deletion src/compiler/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ export type {
Trajectory,
} from "./types.js";
export { compileTrajectory, compileStep } from "./compile.js";
export { synthesizeAssertion, templateToRegex } from "./assertions.js";
export {
DEFAULT_ASSERTION_TIMEOUT_MS,
synthesizeAssertion,
templateToRegex,
} from "./assertions.js";
export type { SynthesizeAssertionOptions } from "./assertions.js";
export {
buildLocatorFallbackChain,
orderLocatorCandidates,
Expand Down
76 changes: 73 additions & 3 deletions src/runner/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,62 @@ export interface ActionResult {
ok: boolean;
outcome?: "LOCATOR_NOT_FOUND" | "TIMEOUT" | "PAGE_ERROR";
message?: string;
/**
* `wait` steps only: whether the `networkidle` fallback actually fired.
* `false` means the bound elapsed and the step proceeded anyway — a settling
* hint that went unanswered, not a failure. See {@link NETWORK_IDLE_WAIT_MS}.
*/
settled?: boolean;
}

/**
* Ceiling for the `networkidle` fallback of a parameterless `wait` step.
*
* This used to be an unbounded `page.waitForLoadState("networkidle")`, which
* inherits Playwright's 30s default — a number nobody in this repo chose. On a
* page that never goes quiet for 500ms, `networkidle` never fires, so the step
* burned the full 30s and then failed anyway: maximum latency for zero
* information. Measured at 30.0s in tests/unit/runner-bounded-wait.test.ts.
*
* **Honest scope of the risk.** The seeded Grafana dashboard does *not* trigger
* it — `networkidle` settles there in ~3ms (measured on 11.0.0, /d/paragent-seed,
* 2026-07-28), because the seed dashboard sets no refresh interval and TestData
* is generated client-side. So this is a latent worst case rather than one the
* current test-bed hits: it becomes reachable on any surface with continuous
* polling, streaming, or websockets. Bounding it is cheap insurance, not a fix
* for an observed test-bed hang.
*
* 5000ms happens to equal the assertion timeout the compiler emits
* (`DEFAULT_ASSERTION_TIMEOUT_MS` in src/compiler/assertions.ts), which keeps a
* step's wait and its post-condition the same order of magnitude. Nothing
* enforces the equality — they are independent constants in different packages,
* and either can move without the other.
*
* **Reaching the bound is not a step failure.** A parameterless `wait` is a
* settling *hint*; the post-condition is the assertion that runs immediately
* after it, with its own budget. So when the bound elapses the step proceeds
* and records `settled: false` — the same posture as the 250ms idle probe in
* page-state.ts, where a timeout means *no claim* rather than failure.
*
* Classifying it as `TIMEOUT` instead would route the step into the repair loop
* (replay.ts sends every non-PASS outcome there), spending repair budget twice
* on a condition no `corrected_action` can fix — "this page never goes quiet"
* is not a locator problem. `success_with_le_2_repairs` would then be reporting
* a scaffolding condition as churn, which is the one thing the gate number must
* not do. If the page really is broken, the assertion says so on its own and
* that failure *is* worth a repair attempt.
*
* **This still changes which steps pass.** A page that first goes quiet at, say,
* 12s used to hold the step until it did; now the step continues at 5s and the
* assertion decides on whatever is on screen. That is deliberate and is a good
* trade *now*, while no gate number exists — see docs/gate/runner.md. It would
* be a bad trade after a measurement had been published against the old value.
*/
export const NETWORK_IDLE_WAIT_MS = 5_000;

export interface ExecuteActionOptions {
/** Override for {@link NETWORK_IDLE_WAIT_MS}. */
networkIdleWaitMs?: number;
}

function isTimeoutError(err: unknown): boolean {
Expand All @@ -39,6 +95,7 @@ export async function executeAction(
page: Page,
action: CompiledAction,
params: ParamBindings = {},
options: ExecuteActionOptions = {},
): Promise<ActionResult> {
try {
switch (action.type) {
Expand Down Expand Up @@ -156,10 +213,23 @@ export async function executeAction(
: 0;
if (Number.isFinite(ms) && ms > 0) {
await page.waitForTimeout(ms);
} else {
await page.waitForLoadState("networkidle");
return { ok: true };
}
const bound = options.networkIdleWaitMs ?? NETWORK_IDLE_WAIT_MS;
try {
await page.waitForLoadState("networkidle", { timeout: bound });
return { ok: true, settled: true };
} catch (err) {
// Only an unanswered settling hint is tolerated here. Anything else
// (a closed page, a navigation error) is a real failure and falls
// through to the outer handler.
if (!isTimeoutError(err)) throw err;
return {
ok: true,
settled: false,
message: `networkidle not reached within ${bound}ms; proceeded — the assertion is the post-condition`,
};
}
return { ok: true };
}

case "upload": {
Expand Down
Loading
Loading