diff --git a/docs/architecture.md b/docs/architecture.md index c10f25e..6b4434a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4,7 +4,7 @@ doc_type: spec status: draft owner: B0 created: 2026-07-25 -updated: 2026-08-12 +updated: 2026-08-16 confidence: HIGH supersedes: null sources_verified: true @@ -33,6 +33,8 @@ than resolved here. Solid edges are wired in code. **Dashed red edges are hops that do not exist yet** — the artifact on the left is never handed to the box on the right by any code path outside tests. +There are none left: the last one, bundle → cache, was drawn solid by +[#166](https://github.com/DevToolie/Paragent/issues/166). ```mermaid flowchart LR @@ -59,7 +61,7 @@ flowchart LR TRAJ -->|"trajectory.schema.json"| COMP COMP -->|"cache-row.schema.json + assertion.schema.json
wrapper has no $id"| BUNDLE - BUNDLE -.->|"NOT WIRED — see break 1"| CACHE + BUNDLE -->|"paragent compile --to-cache
src/cache/ingest.ts → writeCacheRow (#166)"| CACHE CACHE -->|"cache-row.schema.json"| POOL CACHE -->|"cache-row.schema.json"| TEN @@ -72,8 +74,6 @@ flowchart LR NDJSON -->|"PRD §9 aggregates, no_data-safe"| REPORT INTENT -->|"resolveTaskIntent() → task_key or MISS
src/recorder/select-task.ts (#124)"| REC - - linkStyle 4 stroke:#c00,stroke-width:2px ``` ### Reading the diagram @@ -87,12 +87,25 @@ synthesized assertion and writes a `compiled_trajectory` bundle to `artifacts/co **The two breaks.** Two hops the prose elsewhere implies are wired are not: -1. **The bundle never reaches the cache.** Nothing outside `src/cache/` and `tests/` imports - the cache package — verified by grepping every import in `src/` and `experiments/`. The - `pool_eligible` flag on a bundle row comes from the compiler's own pre-check - (`src/compiler/pool.ts`, `decidePoolEligibility`), *not* from the authoritative write-time - boundary (`src/cache/write.ts`, `writeCacheRow`). Both fail closed, and the compiler's own - doc calls itself a pre-check — but today nothing calls the authority. +1. ~~**The bundle never reaches the cache.**~~ **Closed by + [#166](https://github.com/DevToolie/Paragent/issues/166).** `writeCacheRow` had no caller + outside `tests/`: the read half landed with [#118](https://github.com/DevToolie/Paragent/issues/118) + (`gate:matrix --from-cache`), and the write half did not, so the flag reaching disk came from + the compiler's pre-check (`src/compiler/pool.ts`, `decidePoolEligibility`) while the authority + (`src/cache/write.ts`, `writeCacheRow`) was defended by the canary suite and called by nothing. + `paragent compile --to-cache ` now routes every row through the authority via + `src/cache/ingest.ts`, which is the directory `--from-cache` reads. + + **The two implementations disagree, and the pre-check is the stricter one.** On the committed + 12-step live bundle the compiler marks 1 row poolable and the authority marks 7: where a row's + whole locator chain is tainted but it carries `flow_topology`, `buildPoolRow` degrades it to a + `topology_only` pool row — carrying no locator at all — while `decidePoolEligibility` refuses + it outright as `topology_only_degraded`. That direction is legal (a pre-check may be stricter, + never looser) and the dangerous direction is pinned by + `tests/integration/live-bundle-pool.test.ts`. The pre-check is deliberately left as-is here: + changing it changes what every committed bundle artifact claims about pool eligibility, which + is a privacy-adjacent decision that wants its own ADR rather than a rider on a wiring fix — + tracked in [#170](https://github.com/DevToolie/Paragent/issues/170). 2. ~~**The bundle never reaches the runner.**~~ **Closed by [#62](https://github.com/DevToolie/Paragent/issues/62).** The runner consumes `CompiledProgram` (`src/runner/types.ts`), a different shape from @@ -101,8 +114,9 @@ synthesized assertion and writes a `compiled_trajectory` bundle to `artifacts/co hand-written `experiments/gate-v1/fixtures/compiled-program.json` remains the default, because it is the only program that runs without a recording. -Issue [#52](https://github.com/DevToolie/Paragent/issues/52) (end-to-end integration test: -record → compile → cache-write → replay) is the issue that closes both. +Both are now closed, and the end-to-end path they blocked — +record → compile → cache-write → resolve → replay — is covered by +`tests/integration/cache-ingest-bundle.test.ts` and `tests/integration/cache-resolve-program.test.ts`. **A new entry point, ahead of the recorder.** `src/intent/` ([#124](https://github.com/DevToolie/Paragent/issues/124), [ADR-0015](./decisions/ADR-0015-task-identity-and-intent-resolution.md)) resolves a @@ -113,7 +127,7 @@ one a human had typed. `src/recorder/cli.ts` calls it through `--task-key`. **Not shown as an edge into `CACHE`:** the more obviously cache-shaped call site — resolving intent in front of `gate:matrix --from-cache`, which already looks up a program by `(site_key, task_key)` — is not wired yet (ADR-0015 Open Questions); drawing that edge now would -claim a hop that does not exist, the same reason break 1 above is dashed rather than solid. +claim a hop that does not exist — the standard break 1 above had to meet before it could be drawn solid. --- @@ -124,8 +138,8 @@ claim a hop that does not exist, the same reason break 1 above is dashed rather | `src/intent/` | Resolve a natural-language goal to a `task_key`, or a typed MISS — normalized exact match against a known-task catalog, behind a swappable `IntentMatcher` (#124) | `src/intent/index.ts` (library only — called from `src/recorder/select-task.ts`) | none | none — `task_key` is an opaque string handed to a caller, not a contract field this package owns | [decisions/ADR-0015](./decisions/ADR-0015-task-identity-and-intent-resolution.md) | | `src/testbed/` | Boot + seed Grafana OSS at a pinned tag: compose project, provisioning overlay, HTTP seed | `src/testbed/index.ts`; CLI `src/testbed/cli.ts` (`npm run testbed`) | `scripts/testbed/matrix.json` (not a JSON Schema) | none | [gate/testbed.md](./gate/testbed.md) | | `src/recorder/` | Capture a Playwright run as parameterised steps with ranked locator candidates; refuse literal secrets | `src/recorder/index.ts`; CLI `src/recorder/cli.ts` (`npm run recorder`) | none | `trajectory.schema.json` | [gate/recorder.md](./gate/recorder.md) | -| `src/compiler/` | One cache row per step: locator fallback chain, synthesized assertion, fail-closed `pool_eligible` pre-check | `src/compiler/index.ts`; CLI `src/compiler/cli.ts` (`npm run compile`) | `trajectory.schema.json` | `cache-row.schema.json`, `assertion.schema.json` | [gate/compiler.md](./gate/compiler.md) | -| `src/cache/` | Write-time privacy boundary: allowlist, locator taint, pool/tenant row split, canary pipeline; append-only JSONL store (#63) | `src/cache/index.ts` (library only — no CLI) | `cache-row.schema.json`, `assertion.schema.json` (inspected for pool safety) | `cache-row.schema.json` | [privacy/boundary-spec.md](./privacy/boundary-spec.md) | +| `src/compiler/` | One cache row per step: locator fallback chain, synthesized assertion, fail-closed `pool_eligible` pre-check | `src/compiler/index.ts`; CLI `src/compiler/cli.ts` (`npm run compile`, `--to-cache` to populate a cache) | `trajectory.schema.json` | `cache-row.schema.json`, `assertion.schema.json` | [gate/compiler.md](./gate/compiler.md) | +| `src/cache/` | Write-time privacy boundary: allowlist, locator taint, pool/tenant row split, canary pipeline; append-only JSONL store (#63); bundle ingest through the authority (#166) | `src/cache/index.ts` (library only — no CLI of its own; `paragent compile --to-cache` calls `ingestBundle`) | `cache-row.schema.json`, `assertion.schema.json` (inspected for pool safety) | `cache-row.schema.json` | [privacy/boundary-spec.md](./privacy/boundary-spec.md) | | `src/runner/` | Replay a compiled program in Playwright; repair actions only on failure; emit measured metrics | `src/runner/index.ts` (library only — driven by `experiments/gate-v1/run-matrix.ts`) | `cache-row.schema.json`, `assertion.schema.json` shapes (via `CompiledProgram`) | none directly — emits through `src/metrics/` | [gate/runner.md](./gate/runner.md) | | `src/metrics/` | Cost arithmetic, NDJSON emitter, PRD §9 aggregates that report `no_data` on an empty denominator | `src/metrics/index.ts` (library only) | `metrics.schema.json` (`readMetricNdjson`) | `metrics.schema.json` | §9 sections in [prd/PRD-trajectory-cache-v0.2.md](./prd/PRD-trajectory-cache-v0.2.md) | | `src/shared/` | **Not a pipeline stage.** In-page JS source strings two capture sites must run identically — today the `visible_landmarks` enumeration | `src/shared/index.ts` (library only) | none | none — feeds the `trajectory.schema.json` `visible_landmarks` field written by the recorder | see below | diff --git a/docs/gate/compiler.md b/docs/gate/compiler.md index e7d358c..1fe83fc 100644 --- a/docs/gate/compiler.md +++ b/docs/gate/compiler.md @@ -4,7 +4,7 @@ doc_type: spec status: draft owner: B3 created: 2026-07-25 -updated: 2026-08-12 +updated: 2026-08-16 confidence: MED supersedes: null sources_verified: true @@ -260,14 +260,41 @@ Loosening a privacy rule is B5's call and does not belong in a compiler PR — f here, not fixed. Note the direction: B5 refusing too much is safe; the compiler claiming too much was not. +**Measured, once the authority actually ran (#166).** `paragent compile --to-cache` puts every +row through `writeCacheRow`, so the two implementations can now be compared on real data instead +of in principle. On the committed 12-step live bundle the compiler marks **1** row poolable and +B5 marks **7**. The gap is one rule, in the safe direction: when a row's whole locator chain is +tainted but the row carries `flow_topology`, `buildPoolRow` degrades it to a `topology_only` +pool row — a row carrying no locator at all, only "a click happened here, in `main`, between a +click and a fill" — whereas `decidePoolEligibility` refuses it outright as +`topology_only_degraded`. Nothing tenant-derived escapes either way; the pre-check simply +declines to pool a row B5 is willing to strip and pool. + +That divergence is **not** reconciled here, for the reason the paragraph above gives about the +URL path: it changes what every committed bundle artifact claims about pool eligibility, and the +`pool_eligible` flag in a bundle file is no longer what reaches disk anyway. The number to watch +is the one `--to-cache` prints (`authority pooled N step(s) the compiler pre-check did not`). +Filed as [#170](https://github.com/DevToolie/Paragent/issues/170), which lays out the three ways +it could go and why each is a decision rather than a repair. + ## CLI ```bash npm run compile -- --in contracts/examples/trajectory.example.json # writes artifacts/compiled/.bundle.json + +# ...and populate the cache gate:matrix --from-cache reads (#166) +npm run compile -- --in --to-cache .cache/paragent ``` -Options: `--out `, `--no-validate`, `--help`. +Options: `--out `, `--to-cache `, `--no-validate`, `--help`. + +`--to-cache` writes every row through `writeCacheRow` — the authority, not this package's +pre-check — into ``, which is what `gate:matrix --from-cache --site-key +--task-key ` resolves a program out of. It is all-or-nothing: a bundle carrying a claim the +boundary refuses is rejected whole, so a rejected step cannot leave a resolvable-looking prefix +on disk. See [`src/cache/ingest.ts`](../../src/cache/ingest.ts) for why the compiler CLI owns +this write rather than `gate:matrix` or a separate `cache write` command. ## Known blind spots (cannot yet assert) diff --git a/docs/gate/runner.md b/docs/gate/runner.md index e393ce7..3d6369d 100644 --- a/docs/gate/runner.md +++ b/docs/gate/runner.md @@ -4,7 +4,7 @@ doc_type: spec status: draft owner: B4 created: 2026-07-24 -updated: 2026-08-11 +updated: 2026-08-16 confidence: MED supersedes: null sources_verified: true @@ -331,6 +331,19 @@ npm run gate:report network call unless asked. **Still unmeasured:** no live repair has been observed. The client is covered by 21 mocked-SDK tests; a self-heal rate remains structurally 0 until someone runs it with a real key, which is the exit criterion #27 names and this repo will not fabricate. + + **That opt-in did not actually work until + [#165](https://github.com/DevToolie/Paragent/issues/165).** `--repair-model` was accepted by + the parser, documented in `usage()`, and dropped twice over: `assignValue` had no branch for + it, and nothing in `run-matrix.ts` read `args.repairModel` to build a client. So a run asking + for the real model got `StubRepairModelClient` — a null proposal and zero tokens — and + reported a self-heal rate of 0 and a `cost_repair` of zero that both look measured. The client + refuses to degrade silently (it throws when `ANTHROPIC_API_KEY` is unset); the CLI in front of + it degraded anyway and never reached that constructor. Both halves are wired now, and the + construction happens before anything boots, so a missing key is a named refusal rather than a + stub run. **Any pre-#165 artifact produced with `--repair-model` reported stub numbers** — a + stub run has `cost_repair` all-zero on every row and no `repair_context_level`, so check the + rows rather than the command that was typed. - Whether `compiled_trajectory` bundle `$id` becomes a first-class contract (B3 packaging convention today). - Fresh-reasoning cost capture for `cost_fresh` — measured separately; defaults to zeros when unwired. Since [#123](https://github.com/DevToolie/Paragent/issues/123) this field means the diff --git a/experiments/gate-v1/run-matrix.ts b/experiments/gate-v1/run-matrix.ts index 1e05422..9805b0a 100644 --- a/experiments/gate-v1/run-matrix.ts +++ b/experiments/gate-v1/run-matrix.ts @@ -27,6 +27,8 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { MetricsEmitter, readMetricNdjson } from "../../src/metrics/emitter.js"; import { ReplayRunner } from "../../src/runner/replay.js"; +import type { RepairModelClient } from "../../src/runner/repair.js"; +import { AnthropicRepairModelClient } from "../../src/runner/repair-anthropic.js"; import { bundleToProgram, isCompiledBundle, rowsToProgram } from "../../src/runner/program.js"; import { JsonlCacheStore } from "../../src/cache/store.js"; import { resolveProgram } from "../../src/cache/resolve.js"; @@ -92,7 +94,7 @@ export const DRY_RUN_PARAMS: ParamBindings = { resource_label: "widget", }; -interface Args { +export interface Args { dryRun: boolean; help: boolean; headed: boolean; @@ -142,6 +144,69 @@ interface Args { costFresh?: string; } +/** + * Every value-taking flag, and the one place each is assigned (#165). + * + * This used to be two lists that had to agree and did not: a `valued` set here + * in the parser, and an `else if` chain in `assignValue`. The parser consumed a + * flag's value, handed it over, and the chain fell off its end without matching + * — so `--from-cache`, `--site-key`, `--task-key` and `--repair-model` were + * accepted, documented in `usage()`, and **silently dropped**. `--repair-model` + * was the dangerous one: a run asking for the real repair client got + * `StubRepairModelClient`, which returns a null action and zero tokens, so the + * run reported a self-heal rate of 0 and a `cost_repair` of zero that both look + * like measurements. `AnthropicRepairModelClient` throws at construction rather + * than degrade to a no-op for exactly this reason; the CLI in front of it + * degraded anyway and never reached that constructor. + * + * One table closes the class rather than the four instances. A flag exists here + * or it does not exist at all: the parser derives its accepted set from these + * keys, so an entry cannot be accepted-but-unassigned, and an assigner cannot be + * unreachable. `tests/unit/gate-matrix.test.ts` walks the table and asserts each + * key lands in `Args`. + */ +const VALUED_FLAGS = { + versions: (args, value) => { + args.versions = value; + }, + program: (args, value) => { + args.program = value; + }, + param: (args, value) => { + const eq = value.indexOf("="); + if (eq <= 0) throw new Error(`--param expects key=value, got: ${value}`); + args.params[value.slice(0, eq)] = value.slice(eq + 1); + }, + runs: (args, value) => { + const n = Number.parseInt(value, 10); + if (!Number.isFinite(n) || n < 1) throw new Error(`--runs must be >= 1, got: ${value}`); + args.runs = n; + }, + port: (args, value) => { + const n = Number.parseInt(value, 10); + if (!Number.isFinite(n) || n <= 0) throw new Error(`invalid --port: ${value}`); + args.port = n; + }, + "from-cache": (args, value) => { + args.fromCache = value; + }, + "site-key": (args, value) => { + args.siteKey = value; + }, + "task-key": (args, value) => { + args.taskKey = value; + }, + "repair-model": (args, value) => { + args.repairModel = value; + }, + "cost-fresh": (args, value) => { + args.costFresh = value; + }, +} satisfies Record void>; + +/** Flag names without the leading `--`, in declaration order. */ +export const VALUED_FLAG_NAMES = Object.keys(VALUED_FLAGS) as (keyof typeof VALUED_FLAGS)[]; + function parseArgs(argv: string[]): Args { const args: Args = { dryRun: false, @@ -153,18 +218,7 @@ function parseArgs(argv: string[]): Args { runs: DEFAULT_RUNS_PER_VERSION, poolOnly: false, }; - const valued = new Set([ - "--versions", - "--program", - "--port", - "--param", - "--runs", - "--from-cache", - "--site-key", - "--task-key", - "--repair-model", - "--cost-fresh", - ]); + const valued = new Set(VALUED_FLAG_NAMES.map((name) => `--${name}`)); for (let i = 0; i < argv.length; i++) { const a = argv[i] ?? ""; if (a === "--dry-run") args.dryRun = true; @@ -188,23 +242,15 @@ function parseArgs(argv: string[]): Args { } export function assignValue(args: Args, key: string, value: string): void { - if (key === "versions") args.versions = value; - else if (key === "program") args.program = value; - else if (key === "param") { - const eq = value.indexOf("="); - if (eq <= 0) throw new Error(`--param expects key=value, got: ${value}`); - args.params[value.slice(0, eq)] = value.slice(eq + 1); - } else if (key === "runs") { - const n = Number.parseInt(value, 10); - if (!Number.isFinite(n) || n < 1) throw new Error(`--runs must be >= 1, got: ${value}`); - args.runs = n; - } else if (key === "port") { - const n = Number.parseInt(value, 10); - if (!Number.isFinite(n) || n <= 0) throw new Error(`invalid --port: ${value}`); - args.port = n; - } else if (key === "cost-fresh") { - args.costFresh = value; - } + const assign = Object.prototype.hasOwnProperty.call(VALUED_FLAGS, key) + ? VALUED_FLAGS[key as keyof typeof VALUED_FLAGS] + : undefined; + // Throwing rather than returning matches the parser's own posture two lines + // up — it throws on `unknown argument`. A flag the parser accepted and nobody + // consumed is the same caller error one layer in, and silence there is what + // made #165 invisible for four flags. + if (!assign) throw new Error(`unhandled valued flag: --${key}`); + assign(args, value); } function usage(): void { @@ -393,6 +439,11 @@ interface WalkOptions { shouldStop: () => boolean; /** Measured fresh-baseline (#39), attached to every LIVE run row's cost_fresh. */ costFresh?: Cost; + /** + * Real repair client (#27), built from `--repair-model`. Absent means the + * stub — `ReplayRunner`'s default, and what every run got before #165. + */ + repairClient?: RepairModelClient; } /** @@ -453,6 +504,7 @@ async function walkVersions( await opts.persist(); }, shouldStop: opts.shouldStop, + ...(opts.repairClient ? { repairClient: opts.repairClient } : {}), ...(baseline ? { baseline } : {}), ...(opts.costFresh ? { costFresh: opts.costFresh } : {}), }); @@ -697,6 +749,41 @@ async function main(): Promise { } } + // #165: `--repair-model` reached `Args` and stopped there — nothing built a + // client from it, so `ReplayRunner` fell back to `StubRepairModelClient` and + // the run reported a self-heal rate of 0 and zero repair cost that both look + // measured. Assigning the flag was only half the bug; this is the half that + // makes the flag mean what `usage()` says it means. + // + // Constructed here, before anything boots, for the same reason --cost-fresh + // is loaded here: the client throws when ANTHROPIC_API_KEY is unset rather + // than degrading to a no-op, and a caller who asked for a real model should + // learn that from a named refusal on line one, not after the first container + // is up. + let repairClient: RepairModelClient | undefined; + if (args.repairModel !== undefined) { + if (args.dryRun) { + // A dry run hard-codes every outcome, so no step ever fails and the + // repair loop is never entered. Silently accepting the flag would let a + // caller believe they had priced a real repair. + console.error( + "gate:matrix: --repair-model is meaningless under --dry-run — no step " + + "fails, so the repair loop never runs and no token is ever spent. " + + "Drop one of the two flags.", + ); + process.exit(2); + return; + } + try { + repairClient = new AnthropicRepairModelClient({ model: args.repairModel }); + console.log(` repair-model: ${args.repairModel} (REAL client — this run spends tokens)`); + } catch (err) { + console.error(`gate:matrix: ${errMessage(err)}`); + process.exit(2); + return; + } + } + const mode = args.dryRun ? "dry-run" : "live"; const plannedRuns = walked.length * args.runs; const plannedSteps = plannedRuns * program.steps.length; @@ -755,6 +842,7 @@ async function main(): Promise { persist, shouldStop: () => stopRequested, ...(costFresh ? { costFresh } : {}), + ...(repairClient ? { repairClient } : {}), }); await persist(); diff --git a/src/cache/index.ts b/src/cache/index.ts index 7e08529..db7e6ee 100644 --- a/src/cache/index.ts +++ b/src/cache/index.ts @@ -42,6 +42,12 @@ export { type CacheUpdateResult, type StepOutcomeReport, } from "./update.js"; +export { + ingestBundle, + type IngestableBundle, + type IngestOptions, + type IngestSummary, +} from "./ingest.js"; export { resolveProgram, type ProgramKey, diff --git a/src/cache/ingest.ts b/src/cache/ingest.ts new file mode 100644 index 0000000..ad7744f --- /dev/null +++ b/src/cache/ingest.ts @@ -0,0 +1,195 @@ +/** + * Compiled bundle → cache, through the write-time authority (issue #166). + * + * ## The break this closes + * + * `writeCacheRow()` is the authoritative privacy boundary — invariant 2 in + * `docs/architecture.md` — and until this module it had **no caller outside + * `tests/`**. The only runtime code that touched the cache at all was + * `experiments/gate-v1/run-matrix.ts`, and it only *read*: `--from-cache` + * resolved a program out of a `JsonlCacheStore` in a directory that no shipped + * code path ever populated. `cacheHitRate()` is a reported §9 section whose + * denominator could only be non-empty if a human hand-wrote JSONL first. + * + * Two consequences, and the second is the quieter one: + * + * 1. A §9 metric could not produce a number for a reason that had nothing to + * do with the experiment — nothing could fill the cache. + * 2. The `pool_eligible` flag that reached disk was decided by + * `src/compiler/pool.ts::decidePoolEligibility`, the compiler's own + * **pre-check**, while the canary suite defended a function nothing called. + * Both fail closed, so this was never a live leak; it was an authority and + * a lookalike, with the lookalike doing the work. + * + * ## Why the compiler CLI owns the write + * + * `paragent compile --to-cache `, rather than a `gate:matrix` that + * populates on first run or a separate `paragent cache write`: + * + * - **The compiler is the only stage holding `steps_total`.** `resolveProgram` + * refuses to return anything it cannot prove complete (ADR-0013), and the + * `ProgramRef` carrying that proof is written by the compiler. Whoever writes + * rows has to already hold the whole bundle; the compiler does, by + * construction. + * - **A measurement harness should not also be a data producer.** If + * `gate:matrix` populated on first run, the populating run would differ from + * every run after it — the first would be a file replay and the rest cache + * hits, inside one reported sample. That is precisely the "comparing two + * different things" hazard #39 warns about for the fresh/repair ratio. + * - **A separate `cache write` command would need a third reader of the bundle + * shape.** The compiler already parses, validates and emits it. + * + * This module lives in `src/cache/` and takes a structurally-typed bundle, so + * the dependency runs one way — the compiler's CLI calls the cache, the cache + * knows nothing about the compiler. `src/cache/` stays library-only; it gains a + * caller, not a CLI. + * + * ## What it does not do + * + * It does not renegotiate what `writeCacheRow` permits. Every row goes through + * the authority with its fail-closed checks intact, and a bundle that claims + * pool eligibility the authority refuses raises `CacheWriteRejectedError` — + * unchanged behaviour, now reachable from a shipped path instead of only from a + * test. + */ + +import { writeCacheRowPair, CacheWriteRejectedError, type WriteLogSink } from "./write.js"; +import type { CacheStore } from "./store.js"; +import type { CacheRow, CacheRowCandidate } from "./types.js"; + +/** + * The bundle shape this module needs, declared structurally. + * + * Deliberately not an import of `CompiledTrajectoryBundle`: `CacheRow` is + * declared twice in this repo — once here and once in `src/compiler/types.ts` — + * specifically so the compiler does not depend on the cache package. Importing + * the compiler's types here would close that loop from the other side. + */ +export interface IngestableBundle { + site_key: string; + task_key: string; + rows: readonly CacheRowCandidate[]; +} + +export interface IngestOptions { + store: CacheStore; + log?: WriteLogSink; +} + +export interface IngestSummary { + site_key: string; + task_key: string; + /** From the rows' `ProgramRef`, when they carry one. */ + program_id?: string; + steps: number; + /** Pool rows the authority judged shareable. */ + pool_eligible: number; + /** Steps the authority kept tenant-scoped, with its reason. */ + tenant_only: { step_index: number; reason: string }[]; + /** + * Steps where the compiler's pre-check said "not poolable" and the authority + * said otherwise. + * + * This direction is legal — a pre-check may be stricter than the authority, + * never looser — but it is not nothing: it means the flag on disk before this + * module existed was more conservative than the boundary requires. Reported + * rather than smoothed over, because two fail-closed implementations of one + * rule drifting apart is exactly how the pre-check stops being a pre-check. + */ + widened: number[]; +} + +/** Steps that carry no `program` ref cannot be resolved back. Refuse early. */ +function assertResolvable(bundle: IngestableBundle): string | undefined { + const missing = bundle.rows + .filter((r) => !r.program) + .map((r) => r.step_index); + if (missing.length > 0) { + throw new Error( + `bundle rows ${missing.join(", ")} carry no program ref (ADR-0013), so ` + + "resolveProgram() would report them as no_program_ref forever. " + + "Recompile the trajectory with a current compiler before caching it.", + ); + } + const ids = new Set(bundle.rows.map((r) => r.program?.program_id)); + if (ids.size > 1) { + throw new Error( + `bundle mixes program ids (${[...ids].join(", ")}); refusing to write a ` + + "cache whose rows disagree about which program they belong to.", + ); + } + return bundle.rows[0]?.program?.program_id; +} + +/** + * Write every row of a compiled bundle through `writeCacheRowPair`. + * + * All-or-nothing: the authority runs over the whole bundle **before** anything + * is persisted, so a rejection on step 7 cannot leave steps 0-6 on disk. A + * partial write would not be a correctness hazard — `resolveProgram` fails + * closed on a prefix and reports `incomplete` — but "the cache holds a program + * or it does not" is a cheaper thing to reason about than a truncation that + * resolves as a miss for a reason unrelated to what the caller did wrong. + * + * The cost is that the authority runs twice per row. That is deliberate and it + * is the point: the second pass is the one that persists, and it is the same + * call, so nothing can decide `pool_eligible` on the way to disk except + * `writeCacheRow`. + * + * @throws CacheWriteRejectedError when a row claims pool eligibility the + * authority refuses, or when a tenant row would reach the pool file. + */ +export function ingestBundle( + bundle: IngestableBundle, + options: IngestOptions, +): IngestSummary { + const programId = assertResolvable(bundle); + + // Pass 1 — decide, persist nothing. `writeCacheRowPair` with no `store` runs + // every fail-closed check and returns the rows it would have written. + const decided: { candidate: CacheRowCandidate; pool: CacheRow; tenant: CacheRow }[] = []; + for (const candidate of bundle.rows) { + try { + const { pool, tenant } = writeCacheRowPair(candidate); + decided.push({ candidate, pool, tenant }); + } catch (err) { + if (err instanceof CacheWriteRejectedError) { + throw new CacheWriteRejectedError( + `step ${candidate.step_index}: ${err.message}`, + err.reason, + ); + } + throw err; + } + } + + // Pass 2 — same call, now persisting. Nothing reached disk until every row + // was known to be acceptable. + const widened: number[] = []; + const tenantOnly: { step_index: number; reason: string }[] = []; + let poolEligible = 0; + for (const { candidate, pool } of decided) { + writeCacheRowPair(candidate, { store: options.store, ...(options.log ? { log: options.log } : {}) }); + if (pool.pool_eligible) { + poolEligible++; + // The compiler's claim travelled on the candidate; the authority ignored + // it and decided for itself. Where the two differ in this direction, say so. + if (candidate.pool_eligible === false) widened.push(candidate.step_index); + } else { + tenantOnly.push({ + step_index: candidate.step_index, + reason: pool.pool_ineligible_reason ?? "other", + }); + } + } + + return { + site_key: bundle.site_key, + task_key: bundle.task_key, + ...(programId ? { program_id: programId } : {}), + steps: bundle.rows.length, + pool_eligible: poolEligible, + tenant_only: tenantOnly, + widened, + }; +} diff --git a/src/compiler/cli.ts b/src/compiler/cli.ts index 3dfb412..ea3d9ab 100644 --- a/src/compiler/cli.ts +++ b/src/compiler/cli.ts @@ -10,16 +10,28 @@ import path from "node:path"; import { compileTrajectory } from "./compile.js"; import type { Trajectory } from "./types.js"; import { validateCompiledBundle } from "./validate.js"; +import { ingestBundle, type IngestableBundle } from "../cache/ingest.js"; +import { CacheWriteRejectedError } from "../cache/write.js"; +import { DEFAULT_CACHE_DIR, JsonlCacheStore } from "../cache/store.js"; function usage(): never { console.log(`paragent compiler (B3) Usage: paragent compile --in [--out ] [--no-validate] + paragent compile --in --to-cache npm run compile -- --in [--out ] (from a clone) Reads a trajectory conforming to contracts/trajectory.schema.json and emits a -compiled_trajectory bundle (one cache-row per step).`); +compiled_trajectory bundle (one cache-row per step). + + --to-cache Also write every row through the cache's write-time privacy + boundary into (#166) — the directory + \`gate:matrix --from-cache\` reads. Conventionally + ${DEFAULT_CACHE_DIR}. pool_eligible on disk is decided by + writeCacheRow(), not by the compiler's pre-check; a bundle + claiming eligibility the boundary refuses is rejected and + nothing is written.`); process.exit(0); } @@ -75,6 +87,52 @@ async function main(): Promise { console.log( `rows=${bundle.rows.length} pool_eligible=${bundle.rows.filter((r) => r.pool_eligible).length}`, ); + + // #166: the compiled bundle is a file until something puts it where the + // replay path looks. `writeCacheRow()` had no caller outside tests, so + // `gate:matrix --from-cache` read a directory nothing populated. + const cacheDir = getArg(args, "--to-cache"); + if (cacheDir !== undefined) { + const absCache = path.resolve(process.cwd(), cacheDir); + const store = new JsonlCacheStore({ dir: absCache }); + let summary; + try { + summary = ingestBundle(bundle as unknown as IngestableBundle, { store }); + } catch (err) { + if (err instanceof CacheWriteRejectedError) { + // Not a crash to smooth over: the compiler's pre-check claimed pool + // eligibility the boundary refused, which is the one direction that is + // never allowed. Nothing was written. + console.error(`cache write refused (${err.reason}): ${err.message}`); + console.error(" nothing was written — the whole bundle is rejected, not the row."); + process.exit(1); + } + throw err; + } + const rel = path.relative(process.cwd(), absCache).replace(/\\/g, "/"); + console.log( + `cached ${summary.steps} steps to ${rel} ` + + `(${summary.pool_eligible} pool-eligible, ${summary.tenant_only.length} tenant-only)`, + ); + if (summary.program_id) { + console.log( + ` resolve with: --from-cache ${rel} ` + + `--site-key ${summary.site_key} --task-key ${summary.task_key}`, + ); + } + if (summary.widened.length > 0) { + // Legal (a pre-check may be stricter) but worth saying out loud — it + // means the flag the compiler wrote into the bundle file is more + // conservative than the boundary itself. + console.log( + ` note: authority pooled ${summary.widened.length} step(s) the compiler ` + + `pre-check did not: ${summary.widened.join(", ")}`, + ); + } + for (const t of summary.tenant_only) { + console.log(` step ${t.step_index}: tenant-only (${t.reason})`); + } + } } main().catch((err: unknown) => { diff --git a/tests/integration/cache-ingest-bundle.test.ts b/tests/integration/cache-ingest-bundle.test.ts new file mode 100644 index 0000000..df6d294 --- /dev/null +++ b/tests/integration/cache-ingest-bundle.test.ts @@ -0,0 +1,284 @@ +/** + * `paragent compile --to-cache` end to end (issue #166). + * + * `cache-resolve-program.test.ts` already drives compile → write → resolve, but + * it does so by calling `writeCacheRowPair` itself, row by row, from the test. + * That is the seam; it is not the **shipped path**. Until #166 there was no + * shipped path: `writeCacheRow()` had no caller outside `tests/`, so + * `gate:matrix --from-cache ` read a directory nothing in the product + * could populate, and `cacheHitRate()` could only ever report `no_data` for a + * reason that had nothing to do with the experiment. + * + * So what is under test here is the hop itself — `ingestBundle`, the function + * the compiler CLI calls — and the two properties that make it safe to put in + * front of the privacy boundary: + * + * 1. **The authority decides `pool_eligible`, not the compiler's pre-check.** + * A pre-check may be stricter than `writeCacheRow`; it may never be looser, + * and a bundle that claims otherwise is refused rather than written. + * 2. **All or nothing.** A rejection part-way through cannot leave a prefix on + * disk. `resolveProgram` would fail closed on a prefix anyway (`incomplete` + * is a miss, not a truncated replay), but a cache that half-holds a program + * is a worse thing to debug than one that does not hold it. + * + * The last test runs the **committed 12-step live gate bundle** rather than a + * synthetic one, for the same reason `live-bundle-pool.test.ts` does: synthetic + * fixtures are what let the pre-check/authority divergence through in the first + * place. + */ + +import { existsSync, mkdtempSync, readdirSync, rmSync } from "node:fs"; +import { readFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { compileTrajectory } from "../../src/compiler/compile.js"; +import type { Trajectory } from "../../src/compiler/types.js"; +import { ingestBundle, type IngestableBundle } from "../../src/cache/ingest.js"; +import { CacheWriteRejectedError } from "../../src/cache/write.js"; +import { JsonlCacheStore, POOL_FILE, TENANT_FILE } from "../../src/cache/store.js"; +import { resolveProgram } from "../../src/cache/resolve.js"; +import { rowsToProgram } from "../../src/runner/program.js"; + +const SITE = "fixture@local"; +const TASK = "ingest-three-step"; + +const LIVE_BUNDLE = path.join( + process.cwd(), + "artifacts/compiled/traj-gate-live-create-stat-dashboard-from-testdata-9.5.21.bundle.json", +); + +const fingerprint = (url: string) => ({ + url_template: url, + title_template: "Fixture", + dom_digest: "digest", + visible_landmarks: ["main"], + network_idle: true, +}); + +function trajectory(): Trajectory { + return { + schema_version: "1.0.0", + trajectory_id: "traj-ingest-integration", + site_key: SITE, + task_key: TASK, + recorded_at: "2026-08-11T10:00:00.000Z", + base_url_template: "http://{host}:{port}/", + provenance: { recorder: "test", agent_model: "human", testbed_version: "fixture-v1" }, + parameters: { host: "string", port: "integer", username: "string" }, + steps: [ + { + step_index: 0, + intent: "Open the app", + action: { type: "navigate" as const, url_template: "http://{host}:{port}/" }, + locator_candidates: [], + pre_state: fingerprint("http://{host}:{port}/"), + post_state: fingerprint("http://{host}:{port}/"), + timing_ms: { started_offset_ms: 0, duration_ms: 5 }, + }, + { + step_index: 1, + intent: "Type the username", + action: { type: "fill" as const, param_refs: ["username"] }, + locator_candidates: [ + { strategy: "role_name" as const, rank: 0, role: "textbox", name: "Username" }, + ], + pre_state: fingerprint("http://{host}:{port}/"), + post_state: fingerprint("http://{host}:{port}/"), + timing_ms: { started_offset_ms: 5, duration_ms: 5 }, + }, + { + step_index: 2, + intent: "Save", + action: { type: "click" as const }, + locator_candidates: [ + { strategy: "role_name" as const, rank: 0, role: "button", name: "Save" }, + ], + pre_state: fingerprint("http://{host}:{port}/"), + post_state: fingerprint("http://{host}:{port}/saved"), + timing_ms: { started_offset_ms: 10, duration_ms: 5 }, + }, + ], + } as unknown as Trajectory; +} + +const asIngestable = (bundle: unknown) => bundle as unknown as IngestableBundle; + +/** + * Make one row claim pool eligibility the authority will refuse. + * + * Via the assertion rather than the locator chain, deliberately: a tainted + * locator on a row that carries `flow_topology` legally degrades to a pooled + * `topology_only` row, so it is *not* a refusal on every step. A tenant literal + * in `expected.template` is checked first in `buildPoolRow` and short-circuits + * before that fallback, which makes the violation independent of which step it + * is applied to — the property this helper's callers actually need. + */ +function poison(bundle: { rows: unknown[] }, stepIndex: number): { rows: unknown[] } { + const rows = structuredClone(bundle.rows) as Record[]; + const row = rows[stepIndex] as { + pool_eligible: boolean; + assertion: { expected?: Record }; + }; + row.pool_eligible = true; + row.assertion.expected = { + ...row.assertion.expected, + template: "Acme Widgets Inc invoice #4471", + }; + return { ...bundle, rows }; +} + +describe("compiled bundle → cache, through the write authority (#166)", () => { + let root: string; + let n = 0; + const freshDir = () => path.join(root, `case-${n++}`); + + beforeAll(() => { + root = mkdtempSync(path.join(tmpdir(), "paragent-ingest-")); + }); + + afterAll(() => { + rmSync(root, { recursive: true, force: true }); + }); + + it("populates a cache that resolveProgram then returns as a complete program", () => { + const dir = freshDir(); + const store = new JsonlCacheStore({ dir }); + const bundle = compileTrajectory(trajectory()); + + const summary = ingestBundle(asIngestable(bundle), { store }); + expect(summary.steps).toBe(3); + expect(summary.program_id).toBe("prog-traj-ingest-integration"); + + // The claim that matters: a *different* store, opened on the same directory, + // resolves the whole program. That is the `--from-cache` path exactly — it + // does not share process state with whoever wrote the rows. + const reader = new JsonlCacheStore({ dir }); + const resolution = resolveProgram(reader, { site_key: SITE, task_key: TASK }); + expect(resolution.status).toBe("hit"); + if (resolution.status !== "hit") return; + + expect(resolution.steps_total).toBe(3); + expect(resolution.rows.map((r) => r.step_index)).toEqual([0, 1, 2]); + expect(resolution.program_id).toBe("prog-traj-ingest-integration"); + + // And it is replayable — `required_params` derived from the rows, which is + // what a caller is told to bind before anything opens a browser. + const program = rowsToProgram(resolution, "11.0.0"); + expect(program.steps).toHaveLength(3); + }); + + it("writes rows to disk, not just into an in-process index", () => { + const dir = freshDir(); + ingestBundle(asIngestable(compileTrajectory(trajectory())), { + store: new JsonlCacheStore({ dir }), + }); + + // Both files, because writeCacheRowPair persists the tenant row too and the + // store routes it by `pool_eligible`. A pool row in the tenant file (or the + // reverse) is the leak `tests/canary/store-leak.test.ts` guards from disk. + expect(existsSync(path.join(dir, POOL_FILE))).toBe(true); + expect(existsSync(path.join(dir, TENANT_FILE))).toBe(true); + }); + + it("lets the authority, not the compiler pre-check, decide pool_eligible", () => { + const dir = freshDir(); + const store = new JsonlCacheStore({ dir }); + const bundle = compileTrajectory(trajectory()); + + const summary = ingestBundle(asIngestable(bundle), { store }); + + // Every pool row on disk carries the authority's verdict. Whatever the + // compiler wrote into the bundle file is a pre-check and does not travel. + // + // Filtered rather than `list().filter(...)`: the default view merges pool + // and tenant per key and the tenant row wins, because it is written second. + // Asking the store for pool rows is what `--pool-only` does. + const pooled = store.list({ pool_eligible: true }); + expect(pooled).toHaveLength(summary.pool_eligible); + for (const row of pooled) { + for (const loc of row.compiled_action.locator_fallback_chain) { + expect(loc.tenant_scoped).not.toBe(true); + expect(loc.strategy).not.toBe("text"); + } + } + // Reported, not smoothed over — see IngestSummary.widened. + expect(Array.isArray(summary.widened)).toBe(true); + }); + + it("refuses the whole bundle, writing nothing, when a row claims eligibility the authority denies", () => { + const dir = freshDir(); + const store = new JsonlCacheStore({ dir }); + const bundle = compileTrajectory(trajectory()); + + // A tenant literal in the assertion can never be pool-safe; claiming it is, + // is the one direction a pre-check is never allowed to be wrong in. + expect(() => + ingestBundle(asIngestable(poison(bundle, 2)), { store }), + ).toThrow(CacheWriteRejectedError); + + // All-or-nothing: step 2 was rejected, so steps 0 and 1 must not be on disk. + // Without the two-pass write this directory holds a resolvable-looking + // prefix of a program that was refused. + const onDisk = existsSync(dir) ? readdirSync(dir) : []; + expect(onDisk).toEqual([]); + expect(resolveProgram(new JsonlCacheStore({ dir }), { site_key: SITE, task_key: TASK }).status).toBe("miss"); + }); + + it("names the offending step in the rejection", () => { + // `writeCacheRowPair` knows the row but not its position in a bundle, so a + // bare rejection reads the same for all twelve steps of the live task. + const bundle = compileTrajectory(trajectory()); + expect(() => + ingestBundle(asIngestable(poison(bundle, 1)), { + store: new JsonlCacheStore({ dir: freshDir() }), + }), + ).toThrow(/step 1:/); + }); + + it("refuses rows with no program ref rather than caching something unresolvable", () => { + // Without ADR-0013 identity these rows resolve as `no_program_ref` forever. + // Writing them would produce a cache that is populated and permanently + // useless — the worst of both, and invisible until someone reads a miss reason. + const bundle = compileTrajectory(trajectory()); + const rows = structuredClone(bundle.rows) as unknown as Record[]; + for (const r of rows) delete r.program; + + expect(() => + ingestBundle(asIngestable({ ...bundle, rows }), { + store: new JsonlCacheStore({ dir: freshDir() }), + }), + ).toThrow(/no program ref/); + }); + + it("round-trips the committed 12-step live gate bundle", async () => { + // The real artifact, not a synthetic one. This is the bundle whose + // url-matches rows the compiler once called poolable and the authority + // refused (#25) — so it is the one that proves the shipped path survives + // real data rather than fixtures built to pass. + const bundle = JSON.parse(await readFile(LIVE_BUNDLE, "utf8")) as { + site_key: string; + task_key: string; + rows: { step_index: number }[]; + }; + const dir = freshDir(); + const summary = ingestBundle(asIngestable(bundle), { + store: new JsonlCacheStore({ dir }), + }); + + expect(summary.steps).toBe(12); + + const resolution = resolveProgram(new JsonlCacheStore({ dir }), { + site_key: bundle.site_key, + task_key: bundle.task_key, + }); + expect(resolution.status).toBe("hit"); + if (resolution.status !== "hit") return; + expect(resolution.rows).toHaveLength(12); + + // The point of the exercise, stated as a number: before #166 this + // denominator could not be non-empty without a human hand-writing JSONL. + expect(summary.pool_eligible + summary.tenant_only.length).toBe(12); + }); +}); diff --git a/tests/unit/gate-matrix.test.ts b/tests/unit/gate-matrix.test.ts index f6eba27..3aa61d6 100644 --- a/tests/unit/gate-matrix.test.ts +++ b/tests/unit/gate-matrix.test.ts @@ -26,7 +26,13 @@ import { runsToClearSection9, substituteRunIndex, } from "../../experiments/gate-v1/live-run.js"; -import { buildSection9Floor, loadCostFreshBaseline } from "../../experiments/gate-v1/run-matrix.js"; +import { + assignValue, + buildSection9Floor, + loadCostFreshBaseline, + VALUED_FLAG_NAMES, + type Args as MatrixArgs, +} from "../../experiments/gate-v1/run-matrix.js"; import { perVersionBreakdown, section9SampleFloor, @@ -541,3 +547,87 @@ describe("loadCostFreshBaseline (#39)", () => { await expect(loadCostFreshBaseline(file)).rejects.toThrow(/missing mean_cost_fresh/); }); }); + +// --------------------------------------------------------------------------- +// #165 — every value-taking flag must land somewhere in `Args`. +// +// The bug was structural, not four typos: `parseArgs` held a `valued` set and +// `assignValue` held an `else if` chain, and the two disagreed. A flag in the +// first but not the second was consumed, dropped, and never complained about — +// it did not even trip `unknown argument`, because the parser had accepted it. +// +// So this walks the flag table rather than naming the four that were broken. A +// new flag that reaches the parser without an assigner fails here, and one +// added without a sample below fails the coverage check. +// --------------------------------------------------------------------------- + +describe("gate:matrix valued flags (#165)", () => { + /** A representative value per flag, and what it must produce in `Args`. */ + const SAMPLES: Record< + (typeof VALUED_FLAG_NAMES)[number], + { value: string; expect: (args: MatrixArgs) => unknown; want: unknown } + > = { + versions: { value: "11.0.0", expect: (a) => a.versions, want: "11.0.0" }, + program: { value: "/tmp/p.json", expect: (a) => a.program, want: "/tmp/p.json" }, + param: { value: "resource_label=widget", expect: (a) => a.params, want: { resource_label: "widget" } }, + runs: { value: "7", expect: (a) => a.runs, want: 7 }, + port: { value: "3100", expect: (a) => a.port, want: 3100 }, + "from-cache": { value: "/tmp/cache", expect: (a) => a.fromCache, want: "/tmp/cache" }, + "site-key": { value: "grafana-oss@example", expect: (a) => a.siteKey, want: "grafana-oss@example" }, + "task-key": { value: "open-dashboards-list", expect: (a) => a.taskKey, want: "open-dashboards-list" }, + "repair-model": { value: "claude-opus-5", expect: (a) => a.repairModel, want: "claude-opus-5" }, + "cost-fresh": { value: "/tmp/baseline.json", expect: (a) => a.costFresh, want: "/tmp/baseline.json" }, + }; + + const emptyArgs = (): MatrixArgs => ({ + dryRun: false, + help: false, + headed: false, + keepUp: false, + preamble: true, + params: {}, + runs: 3, + poolOnly: false, + }); + + it("has a sample for every flag the parser accepts", () => { + // Guards the guard: without this, adding a flag and forgetting its sample + // would leave the table-driven test below silently not covering it. + expect(Object.keys(SAMPLES).sort()).toEqual([...VALUED_FLAG_NAMES].sort()); + }); + + it.each(VALUED_FLAG_NAMES)("--%s is assigned, not silently dropped", (flag) => { + const sample = SAMPLES[flag]; + const args = emptyArgs(); + assignValue(args, flag, sample.value); + expect(sample.expect(args)).toEqual(sample.want); + }); + + it("leaves the four flags #165 dropped actually reaching Args", () => { + // The regression in its original terms. `--repair-model` is the one that + // mattered: dropped, the run used StubRepairModelClient and reported a + // self-heal rate of 0 and zero repair cost that both looked measured. + const args = emptyArgs(); + for (const flag of ["from-cache", "site-key", "task-key", "repair-model"] as const) { + assignValue(args, flag, SAMPLES[flag].value); + } + expect(args.fromCache).toBe("/tmp/cache"); + expect(args.siteKey).toBe("grafana-oss@example"); + expect(args.taskKey).toBe("open-dashboards-list"); + expect(args.repairModel).toBe("claude-opus-5"); + }); + + it("throws on a valued flag with no assigner instead of dropping it", () => { + // The structural half: silence here is what made the original bug + // invisible, and the parser already throws for `unknown argument`. + expect(() => assignValue(emptyArgs(), "not-a-flag", "x")).toThrow( + /unhandled valued flag: --not-a-flag/, + ); + }); + + it("still validates values it does assign", () => { + expect(() => assignValue(emptyArgs(), "runs", "0")).toThrow(/--runs must be >= 1/); + expect(() => assignValue(emptyArgs(), "port", "-1")).toThrow(/invalid --port/); + expect(() => assignValue(emptyArgs(), "param", "novalue")).toThrow(/expects key=value/); + }); +});