Repository navigation
feat(benchmark): Arango/Postgres matched dataframe benchmark with correctness gates and CH-disclosed publication chain - #159
Conversation
…chmark Adds internal/dataframe/benchmark: the offline-verifiable core of the two-backend dataframe benchmark (ticket pg-benchmark-arango-postgres-dataframe). - fixtures.go: deterministic seeded FHIR NDJSON generator (Patient/ Observation/Specimen/Condition/ResearchStudy) with facts sidecar and sha256 manifest digest; tiny/small/representative presets; designed-in duplicate contributors, null/missing values, selective filters. Fixes during testing: metastatic specimen stride moved to the global cursor (per-patient index could never reach it, making the filter vacuous). - canonical.go: total type-tagged canonicalization; null vs missing, bag multiplicity, numeric spelling equivalence; explicit storage-ID -> logical-ID mapping so backends are never compared on storage identity. - gates.go: correctness gates BEFORE timing: oracle rows, cross-backend bag equality, column contract, row count, completeness (truncated is never complete), negative auth (denial needs a non-empty baseline and errors are not denials), arm outcome publication. No path from an empty or absent result to a pass; unsupported cases are published, never hidden; empty matrices are refused, not passes. - oracle.go: independent tiny-preset oracles derived from Facts with plain Go arithmetic (no planner/compiler/storage involvement). - measure.go: warmup exclusion, interleaved rounds, nearest-rank p50/p95 with declared method, failed samples kept in raw and excluded from statistics, unavailable metrics never zero, measured throughput windows, injectable clock. - matrix.go: closed workload battery with column contracts, categories, and per-entry oracle-consistency validation. - report.go: report.json schema whose every derived number is recomputed from raw evidence by VerifyReport; controls (stability CV limit, self-ratio, empty floor, ratio-only-both-passed) are mandatory; passed runs require claims and both arms' evidence. The 18-case forgery suite (report_test.go) proves each tamper path REDs (ruling ii). Offline validation: go test -count=1 ./internal/dataframe/benchmark (all pass), go vet, gofmt clean. Live Arango/PG legs, harness wiring, and the node orchestrator land in following waves; PG window not yet granted. AO native adversarial review is pending the PR (coordinator-gated).
Second wave of the two-backend dataframe benchmark core. - provision.go: spawns one loom process per arm from generated config (ClickHouse disabled both arms per the matched-reader ruling; calypr auth against a mock fence), waits for readiness, and owns restarts so the process-cold cache label is only ever earned, never claimed. Arm logs pass through a secret-scrubbing writer (connection strings never reach evidence). MockFence implements the verified authscope contract: GET /user keyed by bearer JWT sub, iss-bound tokens, read/* grants; requests counted for the control's evidence line. - harness.go: public-route client only (multipart dataset upload with auth_resource_path, explorer-config publication, authoring/v2 preview execution). Preview results canonicalize through the shared bag path and carry sampled=true through, never silently. MeasureArms runs the bound protocol: warmup phase, fixed-order interleaved round-robin so drift hits both arms alike, failed samples kept raw with identity and timing. NegativeAuthControl is the pre-timing gate using the production denial shape (restricted intersection -> zero rows). - harness_test.go: offline fake arms prove route usage, digest path, interleaving order, failure retention in raw samples, the full negative-auth rule incl. leak detection, fence grants, bearer shape rejection, log scrubbing, and the both-arms ClickHouse-disabled config. Offline validation: go test -count=1 ./internal/dataframe/benchmark (all pass), go vet, gofmt clean. Live legs still gated on the PG window; execution-route deviations (receipt acquisition, preview pagination) are explicit live-rehearsal items to reconcile against source at run time.
Go-side authority for the benchmark artifact (ruling ii): - verify --report re-derives every number via benchmark.LoadReport; proved end-to-end: a valid report verifies OK, a median-tampered report is REJECTED naming the cell that fails to recompute. - run fails closed (exit 3, blocked) until the granted exclusive window wave wires preflight + the live driver; no partial environment can emit a success-shaped artifact.
Author one ExplorerBuilderWorkspace v2 document per workload using only
wire shapes copied from testdata/acceptance/ncpi-tcga-brca/workspace.json
(field columns, aggregate COUNT/SUM/MIN/MAX/DISTINCT_VALUES with where
{path,equals}, extensionByUrl, fixedFilters {column,values}). This makes
the live flow four public routes with zero builder-command payloads:
explorer-config publish -> create explorer from source (draft = active
revision) -> reconcile (compile receipt) -> preview (source-row
execution, limit <= engine max).
AuthoringVerification() asks the repository's own decoder about every
template before any service is contacted: rejected shapes ship as
planned/not_run with the decoder error as the named capability (same
honesty mechanism as the CI-targeted Arango rows); shapes without
acceptance precedent are labeled unverified-shape for rehearsal
classification. OutputIDFor maps workload ids to decoder-legal output
ids (underscores).
Offline: all 10 templates DecodeWorkspace clean incl. fixedFilters and
route child shapes; full package test suite green, vet and build clean.
…arkdown
postgres-only worker sessions must say so in a way the verifier enforces,
not by silent absence:
- RunPlan {scope, legs[], plannedNotRun[]} stored in report.json;
ValidateLegPlan refuses not_run legs without a named capability,
postgres-only scope with a run arango leg, and unknown scopes/modes.
- VerifyReport now recomputes status via RunStatusScoped and enforces the
un-run-leg contract: arm blocked in every gate report, disclosure row per
workload, zero samples/summaries/claims, no version, self-ratio exactly
0 (never a forged 1.0); run legs keep live-version and 1±0.05 self-ratio
requirements. Disclosure rows for a leg that ran are also refused.
- A postgres-only pass is legitimate with zero speed claims (it certifies
correctness+timings, not a comparison); both-arms passes still require
claims and both arms' evidence.
- RenderMarkdown emits report.md from verified content only: leg table with
capabilities, disclosure rows, controls with plan language
(ratio-only-both-passed, CV ceiling, self-ratio, empty floor), gate and
timing tables, claims or the named reason there are none.
- armplan_test.go: honest postgres-only round-trip + 8-case forgery suite
(faked passed arm, smuggled samples, dropped disclosure, blanked
capability, forged version/self-ratio, injected speed claim, scope lie)
plus the converse disclosure-for-ran-leg case.
All 10 battery workloads still decode; package tests, vet, build green.
ControlRuns values now have producers the verifier can independently recheck: - ControlUnstableCells: warm passed samples per workload/backend; CV over the ceiling (0.35) marks the cell unstable; fewer than 3 samples is unstable, never silently trusted. Warmup and failed samples excluded. - ControlSelfRatio: backend-vs-itself across halves of the (round, iteration) sequence, worst deviation across cells (>=1); ok=false when a cell cannot be split, so a missing control cannot forge its way past the verifier's 1±0.05 bound. - ControlEmptyFloorMillis + Arm.MeasureEmptyFloor: median of an identical request whose expectation is zero rows; any rows back is a protocol error (the control stopped being empty). Harness gains the four public authoring routes, matching openapi_explorer_direct.go and the reconcile/preview direct handlers: CreateExplorer (POST /explorers, sourceExplorerId=default so the draft equals the active revision), FetchBuilderState (GET authoring/v2/builder -> draftVersion/draftDigest/catalog.snapshotToken; refuses a missing token), Reconcile (POST authoring/v2/reconcile -> receiptId), and AuthorWorkload composing publish -> create -> builder -> reconcile. Offline tests pin route order, the draft token round-trip (stale token rejected), missing-token refusal, and the empty-floor non-empty guard. Full package suite green.
… per-scope reports RunBenchmark orchestrates the full matched run against public routes: - arms attached (BaseURL) or spawned (ServerSpec via injectable SpawnFunc; process-cold restarts only earned from real restarts); - dataset upload, per-workload authoring through the four-route flow, with an abort when StableExplorerID diverges across arms (matched comparison is invalid); - pre-timing authorization gate (ruling iii) on the representative workload, including a single-arm variant; a leak aborts before any artifact exists; - interleaved MeasureArms timing; MeasureArms now skips non-participating arms instead of panicking; - gate evaluation via the real evaluators (test seam only), oracles and storage-ID mapping from the same deterministic facts; - control runs: CV stability, self-ratios for run legs only (0 = absent for unrun legs), empty-result floor from an authored zero-row workspace (BuildEmptyFloorWorkspace) whose non-empty response is a protocol error; - live versions are mandatory from VersionProbe for every run leg; the report only exists if BuildReport's internal VerifyReport passes. Offline driver tests pin: both-arms pass with claims + control values, postgres-only pass with zero claims/samples/version/self-ratio for the unrun leg, auth-leak abort before artifact, divergent-explorer abort, source-commit validation, using the injectable scripted clock (host jitter tripping the self-ratio control in-test is the rule working as designed). Full package suite green, vet clean.
… probes benchcli run drives RunBenchmark end to end and writes report.json, report.md, and arms.json (operational manifest: window note, arm evidence, fence request count). Exit 3 = refused to run, exit 1 = ran and failed. Preflight refuses before any work unless: - LOOM_BENCH_LIVE=1 (coordinator-granted exclusive window) and a --window-note recording granter and time; - --source-commit is the full 40-hex SHA; - --out has no existing report.json (no silent overwrite); - scope/flag coherence: postgres-only must not receive Arango flags; both-arms requires both arms; attached arms require both tokens; all participating arms are attached or spawned, never mixed (an attached server's trusted issuer is not the benchmark fence); - namespace guard: PostgreSQL database and Arango database must be under loom_bench_* — a shared instance can never be written by a benchmark run outside its namespace. DSN error text is scrubbed. Spawned arms: one MockFence is the sole auth authority; the scoped principal is granted only the decoy path, so the negative-auth control exercises the production restricted-intersection denial. Configs render via PostgresBackendYAML (six-series ladder spelled out — without it the server first-query-legs and authoring routes would silently be absent) and ArangoBackendYAML; DSNs are scrubbed from arm logs. Version evidence comes from the live services: pgx SELECT version() and ArangoDB /_api/version (shape-checked), never hand-written. CLI smoke checks: every preflight refusal exits 3 with a named reason; namespace guards refuse loom_test and prod_db.
…sclosed not_run benchmark-backends.mjs is the node-side orchestrator; verification authority stays in Go (benchcli verify), so node tests can never satisfy a live check (ruling ii): - --verify-only --report: exits 0 only when the Go verifier says OK; prints the plan's evidence line check live-two-backend-correctness. - --run --out <dir> -- <benchcli flags>: drives benchcli (which fails closed without the granted window), then verifies the produced report.json before reporting pass. - Exit 3 from benchcli is disclosed as not_run with the preflight reason (run stays planned/not_run), never a pass, never a silent skip. - --harness-selftest: node-only sanity, clearly labelled. - --benchcli <path> runs the built binary; the default invokes go run. Hermetic node --test suite (9 tests, no network, no Go): arg parsing, invocation shapes, exit classification, the not_run disclosure path, verify-only pass/fail authority, run-then-verify ordering, report summary fields. Negative e2e: a hand-crafted report is REJECTED by benchcli and the orchestrator reports the live check as fail.
…e bench namespace The server's full runtime opens with SetupNone (the factory's arrangement law: the caller arranges schemas, migrations belong to setup). setup-pg follows the canonical live-composition path: CREATE DATABASE (refused if it exists: fresh per run keeps evidence attributable), CREATE SCHEMA per assigned series under a bench-prefixed name, then the public factory with SetupMigrate. drop-pg force-drops only bench-namespace databases. Admin DSN comes from LOOM_BENCH_ADMIN_DSN (never argv, never echoed); all error text is credential-scrubbed.
…n the _series suffix)
…d arm configs The server refuses to configure without a >=16-char population_mapping_cursor_secret. BuildServerConfigYAML now emits one from a caller-supplied per-run value that the deployer also adds to the log scrub list.
… config --config makes the YAML authoritative for server.listen, so the earlier --listen argv was ignored and the arm bound :0 behind our back. The deployer now renders ListenPlaceholder and the driver substitutes the FreePort-selected address before spawning.
Dataset ingest and explorer publication authorize with PermissionWrite; a read-only grant list 403s every upload. A benchmark grant is a real principal grant in both directions; the negative-auth scoped principal still holds no grant on the dataset path at all.
Pre-window disclosure per the loom-experiment-2 ruling: benchcli run --scope planned writes a fully-disclosed artifact (both legs not_run with named capabilities, every workload blocked, zero controls, samples, summaries, claims) and exits 3; it refuses every live flag, needs no window, and the live driver refuses planned scope outright. - armplan.go: RunScopePlanned, PlannedPlan, PlannedGateReports; ValidateLegPlan requires both legs not_run WITH named capability. - report.go: VerifyReport branches on run-leg count — a report where nothing ran must have wholly absent control values (stolen limits, borrowed floors, and forged self-ratios RED), while any run leg keeps the full controls requirement. - cmd/benchcli: planned mode, per-backend disclosure reasons, shared writeRunArtifacts so planned and live artifacts share one writer. - scripts/benchmark-backends.mjs: classifyPlannedDisclosure enforces skip-capture discipline (any sample/summary/claim in a planned artifact FAILS the check) and still routes the artifact through the Go verifier for authority. - README for the package; runner pointer in scripts/README.md. Live legs cite finding PG-PG-AUTH-PATH-DISCOVERY-ALLGENS (scoped calypr ingest blocked on postgres; fix owned by arch-backend-09). Validation: go build ./... and go vet clean; offline package suite green with 42 forgery-RED cases (14 new planned paths); node tests 12/12 in scripts/benchmark-backends.test.mjs and 167/167 across scripts/*.test.mjs; planned artifact produced by the CLI passes benchcli verify.
…ployer refusals The cmd/benchcli package had no test file, leaving the fail-closed CLI surface unpinned: loom_bench_* namespace guards, credential redaction in error text, DSN parsing, admin DSN env handling, battery disclosure totaledness, artifact writing, and deployer topology refusals. - guardPostgresNamespace: benchmark db passes; loom_test/postgres/ prefix-lookalike refused; error text never carries the password. - parsePostgresDSN / parseArangoDSN: full field split, default port, scheme/database/namespace refusals, no credential leakage. - benchDatabaseRe: accepts loom_bench_<4..48>, rejects loom_test, postgres, case-violating and short names. - setupPG schema-prefix guard and withDatabase URL-form law are checked hermetically (refusal happens before any connect). - planNotRunMatchesBattery: complete plans pass, dropped rows and rows for ran legs fail. - writeRunArtifacts: the three files land and report.json re-verifies through LoadReport/VerifyReport; arms.json stays valid JSON. - deployer: mixed topologies, missing tokens/dsn/binary, and caller tokens against the fence all refuse; the valid spawned shape wires the fence issuer into the config, mints distinct tokens, scrubs the DSN from arm logs, and grants the scoped subject everything EXCEPT the dataset path — verified through the fence's real /user surface. Offline only; scripts/** untouched per the runner-serialization ruling. go build/vet clean; benchmark + benchcli packages green (12 new test functions); node suite still 12/12.
…LLGENS The two scratch proofs the fix (513b052, PR #156) cites were held in the session artifacts dir but absent from the pushed head; verifier 208 flagged the gap and the coordinator ruled them ticket evidence to keep in-repo. - scratch_authrepro_test.go: hermetic unit-level branch discrimination against a real ScopeResolver + FenceUserAccessClient + MockFence. Variant A (bearer principal, matching grant, empty lister) proves the harness token/issuer wiring is correct by PASSING AuthorizeWrite; nil-principal and header-less variants pin the 'outside caller scope' shape the fake-based driver tests rely on. Runs always. - scratch_liveupload_test.go: live proof. Opt-in by substrate (skips without the scratch DSN file; binary path overridable via LOOM_BENCH_SCRATCH_BINARY). Pins TWO-WORLD CONSISTENCY rather than defect-era absolutes, so it does not false-red after the fix: the HTTP upload outcome and the store-level AllGenerations listing must agree (defect = both refuse, fix = both succeed); disagreement is substrate drift and fails. This is what caught the defect live: fence hit exactly 1x, then 403, while the listing refused. Behavior-only assertions: no coupling to the normalizer's output shape. Offline green: both pass here (defect-era binary + library = consistent refusal world); CI skips the live probe without the scratch DSN file.
…tatus The first consolidated live run refused activation: GENERATION_LOAD_INCOMPLETE with validation_errors=32 and Patient/Condition committing zero rows, while Observation/Specimen/ResearchStudy loaded. The count was exactly the tiny preset's 12 patients + 20 conditions, and the server-side row builder (internal/ingest loadRowGeneratedInternal) showed two fixture defects, both harness bugs rather than backend behaviour: - HumanName.family is a single string in FHIR R4; the generator emitted a JSON array, so every Patient line failed decode. Each family name now gets its own name entry, so name[].family still yields every family a patient carries and DISTINCT-vs-ALL stays selective. The given list rides on the first entry only, so name[].given[] cannot duplicate a given value across entries and the sort-limit oracle keeps exactly one row per patient. - Condition.clinicalStatus is required (1..1); it was never emitted, so every Condition line failed Validate(). It is now a constant 'active' coding on the standard condition-clinical system. No workload predicate reads it, so it stays out of every oracle. Add TestGeneratedFilesAreValidFHIR, which decodes and validates every line of the tiny and small presets through the same generated concrete FHIR structs the loader uses, plus the non-blank and non-empty-id checks the loader applies. The client-side preflight does not run this validator, which is why the defect reached the server: this test closes that gap offline instead of discovering shape errors during a live window. Facts, oracles, and canonical comparison are unaffected: they read PatientFacts and ConditionFacts, not the documents. Offline suite green (benchmark + benchcli + node 12/12); go build and vet clean.
The consolidated live run cleared generation upload and stopped at
publication: POST /api/v1/projects/{project}/generations/{generation}/explorer-config
returned 403 {"error":"forbidden"}. Cause: publishRepositoryExplorerConfig
calls AuthorizeWrite with the authResourcePath taken from the query
parameter, and with scoped authorization enabled AuthorizeWrite rejects an
empty path outright ("auth_resource_path is required when scoped
authorization is enabled", internal/authscope/scope_resolver.go). The
benchmark harness only sent the path as the upload form field, so every
durable write - publish, create-explorer, reconcile - was unreachable under
a scoped deployment. The run reached publish first because publication
precedes explorer creation in the authoring sequence.
Add Arm.AuthResourcePath, set from the driver options at both arm
construction sites, and apply it as a query parameter on the three write
routes through Arm.scoped. Read routes are untouched: preview authorizes via
authoringReadDirect, and an empty path sends no parameter so unscoped
deployments are unaffected (an unrestricted principal ignores it for writes,
and a spurious scope would only narrow reads).
TestWriteRoutesCarryAuthResourcePath asserts the parameter per route and
asserts its absence for an unscoped arm; it is RED with the scoped() helper
short-circuited, so a regression cannot pass silently. Offline suite green
(benchmark + benchcli + node 12/12), go build and vet clean.
…ve probe Two live-window defects in the probe itself, both found while running the granted Phase B session at server 513b052: - The server resolves --schema relative to the CWD (default schemas/graph-fhir.json), and SpawnServer inherits the spawner's directory. The probe runs under go test (package dir), so generation load failed with stat schemas/graph-fhir.json: no such file or directory and the upload returned 500 INTERNAL_ERROR (fs.PathError, request_id 29da9ff4). benchcli run only worked because the runbook launches it from the repository root. Add ServerSpec.WorkDir, honored only when set, and have the probe pin the repository root (auto-discovered by walking up for schemas/graph-fhir.json, LOOM_BENCH_SCRATCH_WORKDIR override) so probe and benchcli spawn identically regardless of caller CWD. - The probe uploaded samples.ndjson, which was never a loadable resource name; the 403-era probe never reached load, so nobody noticed. After the auth fix the file reaches the loader. Upload the real GenerateFiles fixtures instead, which is the path the benchmark actually measures. Probe outcome against loom-server-513b0520 (fixed) from this pre-merge checkout (library pre-fix): upload accepted (fence requests=1) while the store-level AllGenerations listing still refuses -> surface drift, correctly reported by the consistency pin. It goes green when 207's fix lands at this branch's base.
…an excluded plan The publication chain is ClickHouse-backed end to end: publication.Target has exactly one production implementation (ClickHouseBundleStore) and POST .../explorer-config materializes through it before the revision that seeds the authoring draft persists. Publish, authoring, reconcile, receipt previews, workload gates, timing, and controls therefore share one gate behind the service (coordinator ruling 2026-11-13: no ClickHouse chasing in the container; non-publication stages must go green). RunPlan gains a checked Publication declaration. --publication=excluded (default) runs spawn, upload, and version probes for real and emits a report where VerifyReport refuses any samples, summaries, claims, or control values, requires every gate arm blocked, and one disclosure row per battery workload per ran leg under the capability CH-<CI pin>-unavailable-in-container derived from ClickHouseCIVersion (tracked against the clickhouse/clickhouse-server pin in .github/workflows/tests.yaml). --publication=run requires --clickhouse-enabled; a ClickHouse-disabled identity claiming the chain ran no longer verifies. Driver protocol fixtures now state their ClickHouse posture explicitly instead of relying on a hardcoded flag. Validation: go test ./internal/dataframe/benchmark/... green including the new publication_test.go (honest round-trip, driver excluded protocol, 8 forgery RED cases, CI-pin drift guard); node orchestrator 12/12; go build and vet clean. Runner freeze respected: no scripts/** changes.
…ublic postgres port
Two CI failures on the benchmark branch:
1. scripts/package_audit.py refused the new packages (Python tests lane).
Curation added for internal/dataframe/benchmark and its benchcli
entry point; docs/PACKAGE_AUDIT.{csv,md} regenerated. package_audit_verify
passed for both locations; the measured-importer column updates are the
new packages appearing as importers, as expected. Not runner territory:
no CI selector or orchestrator files touched, runner freeze respected.
2. TestPostgresFactoryOwnershipContract fired: production files import
the PostgreSQL driver outside the register (internal/backends/ is the
driver's only legitimate production resident). benchcli's version
probe and setup-pg/drop-pg imported pgx/v5 directly. The contract test
is arch-backend-09's; the fix stays in the benchmark:
- probePostgresVersion now opens the public factory (no schema
assignment, SetupNone) and runs SELECT version() on a connection
acquired from Runtime.ConnSource(), the factory's public
single-connection port. Zero jackc imports remain in the package.
- setup-pg/drop-pg run their DDL on the same port; identifiers pass a
closed regex before SQL-standard double-quote rendering.
- DSN parsing moved from pgx.ParseConfig to net/url; the lane already
mandates URL-form DSNs (guards suite unchanged).
Validation: TestPostgresFactoryOwnershipContract green; go test
-count=1 ./internal/dataframe/benchmark/... green; python suite 48+14
OK; node 12/12; go build/vet clean. Live smoke against the test PG
lane: setup-pg created database + six-series ladder and drop-pg
removed it (exit 0 both), and the probe returned the full
PostgreSQL 18.6 version string. The internal/ci vet-gate failure
reproduces on clean base faffab3 in this sandbox (TMPDIR relpath
quirk), so it is environmental, not this branch.
…o/backend-benchmark/pg-benchmark-dataframe # Conflicts: # docs/PACKAGE_AUDIT.csv # docs/PACKAGE_AUDIT.md
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head a9417fb (new internal/dataframe/benchmark package, cmd/benchcli, node orchestrator). Verdict: CHANGES REQUESTED — the four inline findings each need a change.
Strong parts: the recompute-or-fail VerifyReport and its forgery battery, fail-closed preflight and window guards, loom_bench namespace guards with quoted identifiers, DSN scrubbing, and the planned-path disclosure discipline.
Inline findings:
- authoring.go — derive-age-band is a guaranteed oracle mismatch: the workspace authors age_band as DISTINCT_VALUES over the raw quantity while oracle.go emits band labels that the fixtures always label senior; the battery can never reach passed on a live run.
- harness.go — the process-cold cache label is fabricated: one restart per round labels every measured iteration of that round process-cold, and no test exercises the cold path at all.
- driver.go — with any spawned arm the warm-only controls see zero measured samples, so ControlSelfRatio aborts every benchcli --binary timing run after the full battery has already been timed.
- cmd/benchcli — the default --publication=excluded live path exits 0 with status=failed, and scripts/benchmark-backends.mjs then logs live-two-backend-correctness: pass for an artifact that measured nothing, violating the README exit-code contract (0 = live legs ran).
Non-blocking suggestions (no inline comment): ErrAbortBeforeArtifact is declared but never used; PostgresBackendYAML hardcodes the six series names that setup-pg takes from backendspostgres.AllSeries (add a drift guard); RunBenchmark panics on a nil VersionProbe although it is documented as required; classifyArmError maps context canceled to timeout; sortedNDJSONNames silently drops non-.ndjson fixtures; the driver protocol tests replace the real gates with fakes (acknowledged in a comment) — an offline table test running EvaluateGates against fixture-derived rows per workload would have caught finding 1 before any live window.
…reasons Addresses review r4559430560: derive-age-band was unpassable by construction (the workspace authors DISTINCT_VALUES over raw valueQuantity.value while the oracle emits band labels; fixture ages [1000,10000) make the young/middle branches dead), and sort-limit-topn carries an unproven first-only assumption over multi-valued name[].family. Live battery law: a live leg runs exactly the authoring-proven workloads. Registry-excluded workloads are never authored, previewed, or timed; they ship as blocked gate rows with the LiveBatteryExclusion marker, one disclosure row per running leg naming the exact defect, and zero timing evidence. VerifyReport ties marker, disclosure text, arm posture, and sample-freedom back to LiveBatteryExclusions(); RunStatusScoped skips disclosed exclusions; benchcli's plan matcher and both report fixtures follow the same law. The pre-timing authorization gate moves to a proven workload (group-count-by-patient) since filter-active-gender is now registry-excluded. Validation: go test ./internal/dataframe/benchmark/... green (both packages, incl. 8 new live-battery forgery subtests and TestLiveBatteryExclusionsRegistry); node scripts/benchmark-backends.test.mjs 12/12; go build/vet clean.
|
Operator acceptance gate for PR #159 (current head 51fad8e): this PR is NOT accepted as a completed benchmark. The current PR description reports a default publication-excluded path, no completed timing comparison, unavailable Arango/ClickHouse services in the worker environment, and zero completed publication-chain probes. Offline harness tests and structurally valid not_run reports do not establish that either driver's speed was benchmarked. Required before marking ready or merging:
Deliver the smallest working comparison first. Additional framework, exclusion bookkeeping, or more tests are not substitutes for running it. Preserve and resolve existing concrete review findings; do not silently shrink required acceptance. Keep this PR draft/blocked until this evidence exists. If the code cannot benchmark speed between the two real drivers, reject it rather than merge scaffolding as the benchmark deliverable. This is the operator's explicit acceptance requirement. |
…ew r4559430564/568) MeasureArms labels process-cold only on the sample that directly follows that arm's own successful restart; attached arms (no Proc) are labeled service-warm regardless of the peer being spawned; a failed cold restart records a labeled error sample at its position with Round/Iteration set. Controls split into passedStable (claims + CV, steady regimes only) and passedMeasured (round-half self-ratio across all regimes, so a spawned timing leg never aborts for lack of cells). buildClaims and VerifyReport share claimCellPair: warm preferred, service-warm fallback, never process-cold. Tests: TestMeasureArmsColdLabelContract spawns a real sleeper process as the arm's process owner and pins restart counts, per-round cold-first labeling, and service-warm attached arms; TestMeasureArmsFailedColdRestart IsLabeled pins the failure-sample contract.
Age observations now carry a days identifier under urn:loom:benchmark:age-days alongside valueQuantity; the fixture domain is [1,90] days so the corpus straddles both band thresholds. The workspace is Observation-rooted and derives age_band via EXACT_CATEGORY_RECODE with an exhaustive map generated from the shared AgeBandFor() the oracle uses, so author and oracle cannot drift; unknownPolicy is ERROR and the fixed filter selects the three band labels. The workload stays authoring-unverified (recode has no acceptance precedent) and remains a disclosed live-battery exclusion until the ClickHouse lane rehearses it; TestDeriveAgeBandOfflineWitness proves the authored shape reproduces the oracle bag offline against the uploaded fixture.
… reads pass The first scoped GET .../authoring/v2/builder in CI returned 403 (stage authorization). compose.go wires the explorer read gate to authscope.AuthorizeReadProject, which admits a caller only when a fence grant normalizes to projectid.Legacy(project). The harness project was "loom-bench" while its fence resources lived under /programs/loom/projects/loom-bench/, whose calypr key is "loom-loom-bench" - no grant could ever equal the project key. The write gate hid the drift because it compares caller paths against the dataset path itself, so upload and publish passed while every scoped authoring read was doomed. The defaults now model program loom / project bench, addressed by the legacy key loom-bench - the shape the repo's own authscope contract tests pin. Dataset and decoy paths stay raw-distinct, so the scoped negative-auth control still denies by row-path equality, and the write gate keeps passing with both sides normalizing to the project key. TestDefaultFenceGrantsSatisfyTheProjectReadGate pins the whole default set against the gate, including the foreign-subject refusal.
The planned-leg default still asserted in the present tense that the scoped calypr lane cannot ingest on postgres (PG-PG-AUTH-PATH-DISCOVERY- ALLGENS). #156 fixed that finding and the paired lane has since uploaded on both drivers; a stale present-tense reason in a not_run disclosure is exactly the misreport class the report verifier exists to refuse. The capability now names the actual reason a planned postgres leg happens: no service/DSN in the environment.
The local postgres rehearsal disproved the chain-less-battery premise: PublishRepository always materializes (repository_publish.go requires MaterializeReceipt; publication.Target has exactly one production implementation, ClickHouseBundleStore), so the first explorer-config publish answers 422 MATERIALIZATION_FAILED without ClickHouse. There is no battery without the chain: the timed mode is removed, and --publication is excluded | run. - driver: bootstrap the default explorer by publishing the empty-floor workspace before reading the builder catalog (the default exists only after the first publish), then fetch each arm's contributor catalog. - harness: Publication binds the real explorer-config response fields (receiptId, revisionId, recipe, translationVersion); output identity belongs to the caller that authored the single-output workspace. - benchcli: --clickhouse-url/-user/-password/-recipe spawn flags; each arm materializes into its own loom_bench_* database on the shared service; run mode preflights url+recipe+enabled. - scripts: paired classification now requires plan.publication.run and identity.clickhouseDisabled=false; comparison.md states the chain ran. - CI: the paired job adds clickhouse/clickhouse-server:25.3 and invokes --publication=run through it. Validation: go test ./internal/dataframe/benchmark/... and ./internal/dataframe/... green (run-mode classification, config YAML both variants, bootstrap-first publish ordering pinned in driver_test); node scripts/benchmark-backends.test.mjs 23/23; --harness-selftest pass. The full chain (publish with materialization) has no container engine in this worker, so it is adjudicated by the paired CI job itself, which now provisions the service.
…o/backend-benchmark/pg-benchmark-dataframe # Conflicts: # internal/backends/arango/internal/catalog/all_generations_auth_path_conflict_test.go
The paired job's ClickHouse service ran bare, but the image refuses the anonymous default login from the runner network: the arango arm logged "code: 516 ... Authentication failed" at EnsureDatabase, the publication plane recorded a startup degradation, ClickHouseReadyCheck returned it verbatim, and /readyz held 503 for the whole spawn budget. The two ClickHouse jobs in this workflow already create loom_ci with CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1; the paired service now carries the same env, and the job exports LOOM_BENCH_CH_USER/PASSWORD the way the conformance lane exports LOOM_CLICKHOUSE_USERNAME. The orchestrator resolves ClickHouse credentials in order: explicit --clickhouse-user/-password flags, then those two environment variables, then the bare server default. Pinned by the node suite (env fills, flags win); 24/24 green. The paired job at this head is the adjudicator for the materialization chain.
The verifier comment reword at 76054ef lost tab indentation, which the tracked-files gofmt gate (internal/ci) adjudicates. go test ./internal/ci -run TestGofmtGateTrackedFilesAreCanonical now passes, and gofmt -l is clean across internal/ and cmd/.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head 1fdd963 (round 2). The four round-1 findings are fixed and genuinely covered: TestClassifyLiveExit pins the exit-code contract (finding 4); TestMeasureArmsColdLabelContract and TestMeasureArmsFailedColdRestartIsLabeled drive real OS-process restarts and pin per-round process-cold plus service-warm labeling (finding 2); ControlSelfRatio now runs over all regimes so spawned legs no longer abort (finding 3); and derive-age-band now shares a single AgeBandFor banding function with an exhaustive recode map, a scalar-string fixture twin, and TestDeriveAgeBandOfflineWitness, while all six unproven shapes are excluded from live legs by LiveBatteryExclusions and pinned by an eight-case forgery battery in report_test.go (finding 1). The recompute-or-fail verifier was strengthened, not weakened: it independently re-derives the exclusion registry, requires a gate report per battery workload per ran leg, and shares claimCellPair between build and verify. Verdict: CHANGES REQUESTED for two remaining defects, both introduced by the round-2 paired-benchmark and documentation work and each defeating the operator-mandated paired speed comparison. They are the spawned-arm and documentation siblings of the round-1 findings.
… publication-mode README Review r4561031154: the paired lane ran --iterations 5 --rounds 5, so a spawned arm's only measured sample per round was the post-restart process-cold one; claimCellPair cites warm/service-warm only, so the report verified as a passed run with no claims and benchcli exited 1. MeasureArms now refuses a spawned arm below two measured iterations per round; the paired lane runs 10 iterations over 5 interleaved rounds (workflow, mjs defaults, and paired validation each encode iterations >= 2*rounds, rounds >= 5). Adds controls_test.go (claim regime + stability-control exclusion) and a spawned full-driver test asserting claims are produced. Review r4561031163: README documented a timed mode, a PublicationCapabilityTimed capability, and a chain-less battery that the 2026-11-13 rehearsal disproved. Section rewritten to the implemented two modes (excluded | run), the ClickHouse-backed chain the authoring battery requires (first publish answers 422 MATERIALIZATION_FAILED without a target), the per-arm databases and credential precedence, and the live-battery exclusion marker (currently six workloads).
…ed, matched-state proof Ruling-B route-independent conditions, ahead of the seeding-path decision: - PublishReceipt (POST authoring/v2/publish) verifies its answer: receipt identity round-trips, revision identity exists, and every output is READY with a materialization identity (condition ii: materialization evidence consumed, never decorative). - doArmRequest wraps activation-style refusals (typed activation codes, activate-ExplorerConfigV2, retained-revision prose) in ErrActivationRefusal centrally, so no route can tolerate, retry, or special-case one (condition iii); detection is content-based, verified not to mislabel generic 5xx. - SetupProofEvidence + MatchedSetupProof give the pre-timing matched-state proof: content-derived draft digest equality, exact output-set match, materialization-count equality, and agreeing setup-preview rows across arms (condition i). Revision ids are recorded, not compared. Wiring into AuthorWorkload waits on the coordinator's seeding-path selection (command-replay vs store-backed seed route); these primitives survive either path. Fakes serve the two new routes. Suites green.
…apter posture, not prose Coordinator ruling (2026-11-13, Option A sequence): the fail-closed detector keys on the typed activation codes plus the structured CodeBackendUnavailable + stage=adapter posture (postgreSQLRuntime Unbiased shape in runtime.go), never on message prose. Prose-only bodies with no typed code are ordinary failures; the paired 503 shape is still detected via GENERATION_ACTIVATION_FAILED, and the remaining named refusal (ValidateSelectionReferences) is detected via the adapter posture. Tests pin all three cases.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head 6499633 (round 3). Both round-2 findings are fixed and genuinely covered: the paired spawned lane now yields warm cells (iterations 10 / rounds 5 in scripts and CI, the MeasureArms fail-fast guard for spawned arms with fewer than two measured iterations per round, pinned by TestMeasureArmsRefusesSpawnedArmWithoutWarmIterationPerRound and the end-to-end TestDriverSpawnedArmsProduceClaims), and the README now documents exactly the two publication modes with the truthful ClickHouse dependency. Activation fail-closed is real: doArmRequest wraps typed activation and adapter-posture refusals with ErrActivationRefusal and TestActivationRefusalIsFatalOnEveryRoute pins detection on codes/structure rather than prose. Verdict: CHANGES REQUESTED for three inline findings. 1) The new authoring publish verification and matched-state proof are unreachable from the live driver, so ruling-B conditions (i) and (ii) are unit-test-only. 2) VerifyReport never recomputes the self-ratio or the unstable-cell list from raw samples, leaving the exact forgery path its own comments claim to close. 3) ControlUnstableCells mixes process-cold samples into the CV that its docstring promises is steady-regime, and its unit test was deleted this round.
Non-blocking suggestions (no change required): adapterRefusalMarkers matches generic substrings stage/adapter next to the typed code — parsing details.stage would be sturdier; TestDriverSpawnedArmsProduceClaims could assert the report also passes LoadReport/VerifyReport after building; consider pinning in tests that ControlEmptyFloorMillis keeps citing steady-regime samples now that passedStable gained service-warm.
…driver setup path Review r4561644037: PublishReceipt, FetchExplorerActiveState, SetupProofEvidence, and MatchedSetupProof were referenced only by their own tests — the driver authored, reconciled, and went straight to the authorization gate and MeasureArms, so ruling conditions (i) and (ii) were enforced only against a fake. RunBenchmark now, per live workload: after AuthorWorkload on every arm, each arm publishes its reconciled receipt through the authoring route (untimed setup; every output must answer READY with a materialization identity) and reads back its active state; timing starts only after MatchedSetupProof passes — equal content-derived draft digest, equal output set with a materialization identity each, equal setup-preview rows. Divergence aborts with no report and no samples. Activation-style refusals on the new route abort fail-closed through the central doArmRequest keying. The repository explorer-config spine is unchanged (Option A); the authoring publish is added setup verification, same route and payloads on both arms. Single-arm lanes keep per-arm verification; cross-arm equality applies when both arms run. Tests: both-arms protocol now asserts one authoring publish per live workload per arm; a digest-divergence test proves the abort happens before timing (only the two first-workload proof previews may exist). README documents the pre-timing proof and the fail-closed refusal posture. gofmt/node/benchmark suites green; internal/ci green except the pre-existing shallow-checkout vet probe (CI-depth unaffected).
…ed on steady regime Review r4561644049: VerifyReport accepted SelfRatioArango/ SelfRatioPostgres, UnstableCells, and StabilityCVLimit on faith -- a crafted artifact could borrow a perfect 1.0 self-ratio or omit an unstable cell and still verify. The verifier now recomputes the self-ratio from raw samples (exact-match within 1e-9), recomputes the unstable-cell list, and pins the CV limit to the exported constant. Two new forgery cases pin the paths (self-ratio-borrowed-perfect, unstable-cell-hidden-from-control), and the report/armplan fixtures now state the honest control values their samples derive. Review r4561644054: ControlUnstableCells computed CV over all regimes while discovering cells via passedStable -- the mandated spawned protocol's one process-cold sample per round inflated every cell's CV and would have suppressed all claims, the round-2 no-claims failure re-entering through the control path. CV is now judged over the steady (warm/service-warm) regime, matching the docstring and claimCellPair. TestControlUnstableCells pins the cold-outlier-must-not-flag invariant, a warm-relabelled control of the control, genuine instability, the sub-3-sample rule, and cold-only cells.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head 2f455ef (round 4). All three round-3 findings are fixed and genuinely covered. (1) The setup proof is now wired into the live driver: RunBenchmark calls SetupProofEvidence per workload per arm after authoring and gates every timing behind MatchedSetupProof for both-arm scopes, pinned by TestDriverRunsSetupProofOnBothArms (route-log count of authoring publishes per live workload) and TestDriverAbortsBeforeTimingOnDivergentSetupProof (digest divergence aborts with no report and exactly the two proof previews, no measured-phase routes); the file header and README now describe what actually runs. (2) VerifyReport pins Controls.StabilityCVLimit to the exported constant and re-derives both UnstableCells and the per-backend self-ratio from raw samples, with the fixtures rebuilt to state the honest 1.03 half-median deviation. (3) ControlUnstableCells judges the CV over passedStable only, matching its docstring and the claimCellPair regime policy, and TestControlUnstableCells is restored with the spawned-protocol case (cold outliers per round must not flag a tight warm cell) plus a relabeling control proving the CV is really computed. Verdict: CHANGES REQUESTED for one inline finding: the forgery battery has no case that fails only on the two newly added verifier guards — this case is confounded and the limit pin is only exercised at zero.
Non-blocking suggestions (no change required): SetupProofEvidence runs its setup preview with limit 1, so in production the cross-arm PreviewRows check degenerates to emptiness agreement (both arms cap at one row); consider threading opts.PreviewLimit so the untimed smoke check compares real row counts, though the canonical gates do the authoritative comparison. TestDriverRunsSetupProofOnBothArms could additionally LoadReport-verify the produced artifact and assert that each publish precedes the measured-phase routes for its workload. Reviewer notes: review-only pass, no tests or builds executed; the new driver and verifier behaviors were assessed by source reading.
…et guards Review r4562338071: the two new verifier guards needed cases that fail only on them. - unstable-cell-hidden-from-control was confounded: injecting three wild samples desynced the summaries and pushed the half-median deviation far past the band, so an earlier check could reject for reasons unrelated to the unstable-cell rule. Rewritten per the review: the primary timing's arango warm samples become alternating 1,100,1,100 (equal half medians keep the honest self-ratio match), the summary row is recomputed with Summarize, claims are rebuilt from those summaries, and Controls.UnstableCells stays cleared -- only the ControlUnstableCells recompute catches this shape. - stability-limit-smuggled-ceiling: 0.9 passes every rule the old verifier had (it rejected only <= 0), exercising solely the new equality with the exported constant. - forgeErr lets cases name the rejecting rule; the three guard cases now pin their rule by error message. Verified by deleting each guard: all three artifacts then survive verification, so each case is the exclusive witness of its rule.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head 15d8375 (round 5). Single commit, report_test.go only, and it resolves the sole round-4 finding exactly as prescribed: forgeErr now pins WHICH rule rejects a targeted forgery (empty substring preserves legacy forge for every other case), the unstable-cell-hidden-from-control case was rebuilt so the arango primary-timing cell alternates 1,100,1,100 across order-stable halves — half medians stay equal so the honest self-ratio still matches the reported 1.03, summaries are recomputed and claims rebuilt so every other derived value stays in sync, and the ~0.98 warm CV means only the ControlUnstableCells recompute can reject it — and the new stability-limit-smuggled-ceiling case sets 0.9, which the old <= 0 plausibility check accepted and only the equality with the exported constant catches. I verified the pinning is real: with the sample helper fixing Round=1, Iteration=1 and ControlSelfRatio using sort.SliceStable, the alternating halves are [1,100] and [1,100] (ratio 1.0), the worst cross-cell deviation remains the fixtures 1.03, and each wantErr substring matches the actual VerifyReport message (Go prints 1.0 as 1 in the self-ratio error). Deleting either new verifier guard now turns its case RED instead of silently re-routing to an earlier check. All findings from rounds 1-4 are closed. Verdict: APPROVED — the PR is ready; nothing further needs to change. Non-blocking suggestions (no action required): the same rule-pinning could be extended to a few legacy cases such as self-ratio-forged-high and stability-limit-never-set for uniformity; reviewer notes: review-only pass, no tests or builds executed, the forgery-case outcomes were assessed by source-level recomputation of the fixture values.
…er) into benchmark branch Hop-merge per coordinator order: pulls the #184 ActivateRepositoryGeneration adapter + 000012 activation-shape FAILED->ACTIVE revival wave onto the benchmark head so the paired-driver job can go green at the real spine. Merge-commit hop; no rebase, no force-push.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head ba53916 (round 6). This round is a rebase onto the new base 7e081f4 (16 merged upstream lanes, 300 files) with no new worker content: all 48 of the PR's non-workflow paths — the entire benchmark package, cmd/benchcli, scripts/benchmark-backends.mjs and its test, the package-audit rows, and the arango catalog files — are byte-identical to head 15d8375 which I approved in round 5 (verified via the head-to-head compare: none of the PR's paths appear in the 300 changed files; per-file and total PR stats are unchanged at +13107/-51). The single re-applied file is .github/workflows/tests.yaml: the patch is the same fail-closed paired-driver-benchmark job I reviewed in round 3/4 at the same position between backend-conformance and node-unit, with --iterations 10 --warmup 1 --rounds 5, the arangodb:3.12/postgres:18/clickhouse:25.3 service matrix, and if-no-files-found: error — and the new base has no paired-driver-benchmark key, so the rebase introduced no YAML job collision. Rebase-compatibility spot-check against the merged upstream lanes: the GENERATION_ACTIVATION_FAILED code that the benchmark's fail-closed activation detection keys on is still produced in production (internal/explorer/lifecycle/repository_publish.go) after #184/#179, and no merged lane touched any file the PR owns. All round 1-5 findings remain closed. Verdict: APPROVED — the PR is ready; nothing needs to change. Reviewer notes: review-only pass — no tests, builds, or the live paired lane were executed here; the adapter-posture (BACKEND_UNAVAILABLE + details.stage=adapter) emission could not be re-confirmed offline by source search, and live server-contract drift since the rehearsal is adjudicated fail-closed by the paired CI lane itself at head; the identical round-5 approval is same-model review coverage, not cross-model evidence.
…vot repair) into benchmark branch Second append-only hop for PR #159. Brings the shared write_profiler determinism fix (adjudicated root cause of the cross-arm matched-bytes red) plus the crossBackendCandidateIdentityParity conformance pin into the paired-benchmark lineage.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head 93295ea (round 7). This round is a clean merge of the new base 492d3f0 — upstream #188, the deterministic pivot-defaults fix in the shared write profiler that the paired lane itself surfaced and that unblocks this PR's CI — into the benchmark branch. Verified no worker content changed: the PR still carries exactly the 49 reviewed paths with identical statuses and per-file stats, and the head-to-head compare against the round-6 approved tree ba53916 shows only the nine upstream #188 files (write_profiler determinism and its 315-line test, catalog types, conformance test plus skipset capture/verify pins, arango integration scripts, and one tests.yaml line at 464 — #188's crossBackendCandidateIdentityParity skipset entry, outside the PR's +118 job block, which the PR-relative patch confirms is byte-identical). The paired-driver-benchmark job survives the merge intact: job at line 489, --iterations 10 --warmup 1 --rounds 5, arangodb:3.12/postgres:18/clickhouse:25.3 matrix, evidence upload with if-no-files-found: error. The merge introduced no conflict-resolution edits to PR-owned files, and the skipset change is self-consistent upstream-side (capture JSON, CAPTURE_SHA256 re-pin, and the run-arango-integration set plus its assertion all updated together). All round 1-6 findings remain closed; nothing needs to change. Verdict: APPROVED — the PR is ready. Non-blocking note: #188's own 210-line profiler logic is base content reviewed in its PR, not re-reviewed here. Reviewer notes: review-only pass — no tests, builds, or the live paired lane executed; local checkout unmodified; same-model review coverage, not cross-model evidence.
…ersal-after-window repair) into benchmark branch Third append-only hop for PR #159. Brings the adjudicated PG renderer repair (traversal accepted-dead, SET via sublinks, SUM/COUNT/MIN/MAX dispatch + dedup-parity iff Unique) that unblocks the paired aggregate-sum-age-by-patient publish on PostgreSQL.
The pre-timing setup proof read GET /explorers/{id} into top-level
draftDigest/activeRevisionId, but the route answers ExplorerStateV1
(internal/explorer/state_v1.go), which nests the digest at draft.digest
and the revision identity at active.revisionId. Both fields therefore
always decoded empty against the real server and SetupProofEvidence
aborted fail-closed ("explorer readback on arango omitted the draft
digest") on the first participant. The paired run at 7c5ef71
(job 132029741650) is the first run to reach this stage because every
earlier hop died in postgres authoring before the proofs loop. The
unit suite stayed green because the harness mock answered with the
same top-level fields the client wrongly expected: client and mock
shared the bug.
Matcher amendment granted by the coordinator, scoped to:
- FetchExplorerActiveState/ExplorerActiveState: decode draft.digest
and active.revisionId from the ExplorerStateV1 shape. Gate semantics
unchanged: SetupProofEvidence keeps the trim-space empty-digest
abort; revision identity stays record-only; PublishReceipt
materialization checks and preview-row readback untouched.
- harness mock now MARSHALS the real internal/explorer.ExplorerStateV1
instead of a hand-rolled map, so the mock cannot drift from the
server shape; plus a forceEmptyActiveState flag so the mock can
express the server-legal empty-digest state the old mock hid.
- New permanent regression tests: real-marshal decode (fails on the
pre-fix client: both fields lost) and empty-digest abort through
SetupProofEvidence.
Receipts (local, head 7c5ef71): full benchmark package suite ok;
catalog and postgres renderer suites ok; gofmt/vet clean; control run
with the client fix reverted fails the decode test on both fields.
…ire shape The authoring-v2 preview route answers ExplorerBuilderPreview with columns as generated loomapi.ContractColumn objects (internal/server explorer_preview_response.go v2EmissionColumns; public name at .column, api.gen.go:7137). decodePreview unmarshaled columns into []string, so the paired run at 79e72ed (job 132040946330) aborted fail-closed with "json: cannot unmarshal object into Go struct field PreviewResult.columns of type string". The harness mock served the same string array plus a top-level sampled field the route never emits, so the unit suite was structurally blind - the second instance of the client-and-mock shared-shape class. Coordinator-granted matcher amendment #2, scoped to three hunks: - harness.go decodePreview: columns decoded via []loomapi.ContractColumn, names from .column, with a new fail-closed abort when a column omits its public name. The rows loop, Bag/CanonicalizeRows digest, and the omitted-columns/rows aborts are byte-identical; the matched-bytes law carriers are untouched. - harness_test.go: the preview mock now marshals real loomapi.ContractColumn values inside the server-shaped envelope (apiVersion, kind, receiptId, outputId, columns, rows, rowCount, diagnostics). The phantom sampled script is removed: the encoder never emits it, and no test scripted sampled=true. - preview_shape_test.go (new, permanent): real-marshal decode plus the nameless-column abort case. Receipts at 79e72ed: control with the client fix reverted fails both new tests, reproducing the exact CI error string; with the fix applied the full benchmark package suite is ok, gofmt/vet clean.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Review of head d437550 (round 8). This round merges base cc889e2 (#189 comment-sweep wave, #190 pg-renderer repair — base content reviewed in their own PRs, not re-reviewed here) and adds two real worker fixes, both correcting the benchmark client against the actual production wire after the paired CI runs aborted. Verified both against source: (1) the explorer readback now decodes draft.digest and active.revisionId — matching internal/explorer/state_v1.go tags (digest under draft, revisionId under active) — and SetupProofEvidence keeps its fail-closed empty-digest abort while MatchedSetupProof correctly still refuses to compare server-internal revision ids; the fake now marshals the real loomexplorer.ExplorerStateV1 so client and mock cannot share a wrong shape. (2) decodePreview now reads columns as loomapi.ContractColumn and takes .column — matching generated/loomapi ContractColumn (column tag) and the server v2EmissionColumns wire in internal/server/explorer_preview_response.go; the nameless-column abort is a good fail-closed addition. However the preview commit silently regressed truncation detection: the authoring-v2 preview route DOES carry sampled on the wire — loomapi.PreviewResponse.Sampled, set server-side in previewResponseFromEncoder from !summary.Complete || summary.Truncated (openapi_explorer_authoring_direct.go:571) — and the pre-fix decodePreview unmarshalled the whole body into PreviewResult, so Sampled flowed to sample.Truncated (harness.go:358) and then to GateCompleteness (gates.go: truncated outputs are never treated as complete). The new typed wire struct omits sampled, the fake deleted its sampled script, and the new mock comment asserts there is no sampled field on the wire — which is false. Result: PreviewResult.Sampled is now permanently false, the completeness gate can never fire, and the ExecuteOutput doc comment (sampled=true is surfaced, never silently accepted) is now a lie; a limit- or budget-truncated preview would be timed and could back claims. Verdict: CHANGES REQUESTED — one inline finding.
Non-blocking suggestions (no change required): the fake could also script partialValidation, which the real route emits and the client likewise ignores today; the failing-job references in the new test comments are good practice. Reviewer notes: review-only pass — no tests, builds, or the live paired lane executed; local checkout unmodified; same-model review coverage, not cross-model evidence.
…ness gate Review r4567448566: the amendment #2 wire struct dropped the sampled field on my wrong claim that the route never emits it. I verified only the response encoder (explorer_preview_response.go Finish) and missed the route wrapper previewResponseFromEncoder (openapi_explorer_authoring_direct.go:559-571), which sets Sampled = !summary.Complete || summary.Truncated on loomapi.PreviewResponse (json:"sampled", no omitempty -- always on the wire). With sampled omitted, PreviewResult.Sampled was permanently false: a preview that hit the row limit or byte budget would have been silently timed as complete, violating this harness's contract that "sampled=true is surfaced, never silently accepted". - decodePreview: wire struct carries Sampled bool with the sampled tag; PreviewResult.Sampled is restored from the decode. - fakeArm: the sampled script returns (the preview mock now marshals the real loomapi.PreviewResponse type, including per-row rowSources and the scripted Sampled flag), so client and mock cannot share a wrong shape. - preview_shape_test.go: pins the full chain -- sampled=true wire -> ExecuteOutput Sampled -> MeasureArms sample.Truncated -> backendResultFrom BackendResult.Truncated -> EvaluateGates fails completeness and blocks claimability; plus the complete-preview false-path and the restored nameless-column abort. Receipts: control with only the carry reverted fails exactly the two chain tests at the named seams; with the fix the full benchmark package suite is ok, gofmt/vet clean.
What
Reproducible end-to-end benchmark comparing Arango/AQL and PostgreSQL/SQL backends on matched dataframe tasks (ticket
pg-benchmark-arango-postgres-dataframe, batchc4b53e23b9d190fe). Correctness gates run before any timing is claimed; every derived number in a report artifact is recomputed by the Go verifier (VerifyReport), so a hand-edited report never verifies.Adds
internal/dataframe/benchmark(fixtures, canonical bags, oracle, gates, measure, controls, authoring, harness, provision, driver, report schema + verifier) andcmd/benchcli(run/verify/setup-pg/drop-pg), plus the node orchestratorscripts/benchmark-backends.mjs(evidence lines only; Go owns verification authority). CI registers the paired lane as jobfactory/paired-driver-benchmark.Paired real-driver benchmark (operator acceptance, 2026-11-13)
The paired lane runs the full publication chain for real:
PublishRepositoryalways materializes throughClickHouseBundleStore(the only productionpublication.Target), so the first explorer-config publish answers 422MATERIALIZATION_FAILEDwithout ClickHouse — a local-postgres rehearsal disproved the earlier chain-less-battery premise, and the infeasibletimedmode is removed (--publicationisexcluded | run). The paired job therefore provisionsclickhouse/clickhouse-server:25.3alongside the two drivers; each arm materializes into its ownloom_bench_*ClickHouse database so one driver can never read the other's published rows, while the timed path itself stays the matched source-row preview route. This head adds the lane the acceptance demands:Exact executable command (CI job
factory/paired-driver-benchmarkin.github/workflows/tests.yamlruns exactly this against itsarangodb:3.12+postgres:18services, built from the candidate SHA):Required environment:
LOOM_TEST_ARANGO_URL,LOOM_TEST_POSTGRES_URL(DSN whose database matchesloom_bench_[a-z0-9_]{4,48}— the rule benchcli enforces; CI usesloom_bench_paired), andLOOM_BENCH_ADMIN_DSN— with it the lane provisions its own fresh benchmark database itself (drop-pg+setup-pg: database, six-series schema ladder, migrated tables), because the PostgreSQL backend readiness answersschema_missing — the caller owns schema lifecycle; a failedsetup-pgfails the check before any timing attempt.LOOM_TEST_ARANGO_URL+LOOM_TEST_POSTGRES_URL+--clickhouse-url(same image pins asbackend-conformance/go-clickhouse); arms spawncmd/arango-fhir-serverunder the MockFence with--publication=run --clickhouse-enabled(chain executed end to end on both arms; the classifier refuses a report whose plan omitspublication.runor whose identity records ClickHouse as disabled).passed, orbenchcli verifyrejects the artifact.comparison.md(ms per workload per driver: median/min/max/range) plus versions, exact source SHA, fixture digest, warmup/cache/rounds/concurrency conditions next to rawreport.json; artifact set uploaded by the job (paired-driver-benchmarkartifact). Neither driver is expected to win; no speedup is claimed unless it clears correctness + the stability control.Verifier rerun: execute the command above on the final candidate SHA with the two service env vars set to the isolated CI services; the job log prints the table and the artifact carries
report.json+arms.json+comparison.md.First-authorized-upload fix (paired CI run 43368609701)
The first upload ever attempted against a spawned arm returned 403 FORBIDDEN
in 1 ms with no fence traffic. Root cause: the authscope F-B27 project-binding
gate consults
DiscoverExistingAuthResourcePathsbefore ingest can bootstrapfhir_field_catalog; on a never-ingested Arango database the collection doesnot exist, the discovery AQL faulted, and the handler's blanket
Wrap(err, CodeForbidden)turned the storage fault into a denial — thezero-path first-generation bootstrap branch was unreachable and a fresh
database could never receive its first generation. Discovery now answers
"zero recorded paths" for an absent collection (the posture the postgres
backend already has via SetupMigrate, and the posture
retainedSourceFieldProfileExistsalready takes for missing collections).Offline controls pin probe/query counts and the no-AQL missing-collection
shape; a live
LOOM_TEST_ARANGO_URLproof opens a fresh unbootstrappeddatabase and asserts both discovery shapes return empty.
Strict-bind predicate fix (paired CI run 43374508115)
With uploads green, the first
explorer-configpublish faulted HTTP 400bind from body: strict decode: json: unknown field "where". The writablewire is closed over aggregate predicates:
AggregateSource.Whereisjson:"-"(persisted/legacy decode boundary only), so the server's strictbody bind refuses
aggregate.whereat every semantics version — thebenchmark's v3 templates were authoring-decodable but never publishable.
Live legs now fetch each arm's builder catalog once from the seeded
defaultexplorer and migrate predicate workspaces through therepository's own
MigrateLegacyContributors+MigrateLosslessDefaults,publishing canonical v4
Column.Contributorbytes — the exact path theacceptance lane runs against the same routes.
internal/dataframe/benchmark/contributors.goholds the fetch and preparestep; it fails closed on incomplete/truncated/scope-less catalogs, on a
predicate path the catalog cannot resolve unambiguously, and on a
cross-generation catalog. The driver refuses to time arms whose catalogs
migrate the same workload to different bytes (matched-arms law). Tests:
the legacy template fails the server-equivalent closed bind, the migrated
workspace binds, carries
contributor(EQUALS, ANY, CODE) with nowhere,and the both-arms driver protocol now asserts every published body binds
and the two arms publish byte-identical workloads. The committed
scratch-repro probes for the already-fixed auth-path-discovery defect were
removed; their regression proofs live in
internal/backends/arango/internal/catalog.Review round (all threads resolved at this head)
harness.gocache labels: truthful per-armCacheProcessColdonly directly after that arm's own successful restart; attached armsCacheServiceWarm; failed restarts record labeled error samples with Round/Iteration. Tests:TestMeasureArmsColdLabelContract,TestMeasureArmsFailedColdRestartIsLabeled.driver.gospawned-arm self-ratio: controls split intopassedStable(claims/CV, steady regimes) andpassedMeasured(round-half self-ratio across regimes); sharedclaimCellPair(warm > service-warm, never cold) bindsbuildClaimsandVerifyReport. Spawned timing legs compute self-ratios instead of aborting.cmd/benchcliexit contract:classifyLiveExit— passed→0, measured-but-failed→1, non-passed zero-timing (default--publication=excluded)→3 disclosed not_run with reasons; pinned byTestClassifyLiveExit; mjs maps 3→not_run so a zero-timing run can never log a pass.EXACT_CATEGORY_RECODEover an identifier-carried fixture domain [1,90] withAgeBandFor()shared with the oracle; stays a disclosed live-battery exclusion until the CH lane rehearses it, withTestDeriveAgeBandOfflineWitnessproving the authored shape offline.Validation
go test ./internal/dataframe/benchmark/...green: offline battery, gates/oracle/canonical suites, contributor catalog fetch + predicate migration (legacy wire refused, migrated workspace binds closed, unmatched catalogs fail closed), benchcli guards (45 forgery RED cases), driver protocols (both-arms + postgres-only fake arms = both dataframe legs green offline; published bytes pinned contributor-shaped and arm-identical), excluded-publication protocol + forgery RED cases, CI-pin drift guard.node --test scripts/benchmark-backends.test.mjs23/23 (incl. paired classification: zero-timing/exit-0 report fails the check; missing services fail without spawning; benchcli database-name guard mirrors the Go regex).benchcli run --scope planned→ exit 3 disclosed not_run;benchcli verifyOK on the artifact.go build ./...,go vet,gofmtclean.MATERIALIZATION_FAILEDis the disclosed CH wall (0/11, deterministic).Related merged fixes
PG-PG-AUTH-PATH-DISCOVERY-ALLGENS— fixed in fix(pg): implement all-generations auth-path discovery on the PostgreSQL read lane #156 (513b0520), verified live.PG-PG-CAPABILITY-EVIDENCE-INVENTORY-ONLY-STORE— fixed in fix(pg): compose the capability-evidence reader over all three catalog repositories #158 (merged at7ecb42ea), verified live.Risks / follow-ups
--publicationis nowexcluded | run: the timed (chain-less battery) mode is deleted because the real server proves authoring cannot run without the chain.excludedexits 3, never a pass;runrequires--clickhouse-enabled,--clickhouse-url, and--clickhouse-recipe.Matcher amendment disclosure (79e72ed, coordinator-granted, scoped)
The paired run at
7c5ef71e(job 132029741650) aborted fail-closed withexplorer readback on arango omitted the draft digest. Root cause: thepre-timing setup proof decoded
GET /explorers/{id}into top-leveldraftDigest/activeRevisionId, but the route answersExplorerStateV1(
internal/explorer/state_v1.go), which nests them atdraft.digestandactive.revisionId; both fields always decoded empty. The harness mockanswered with the same top-level fields the client expected, so the unit
suite was structurally blind. The bug was latent until #190 let the
postgres arm author cleanly and the proofs loop ran for the first time.
Amendment hunks (exactly three, no other matcher-file changes):
authoringpublish.goExplorerActiveState/FetchExplorerActiveState:decode
draft.digestandactive.revisionIdfrom the ExplorerStateV1shape. Gate semantics identical:
SetupProofEvidencekeeps thetrim-space empty-digest abort; revision identity stays record-only;
PublishReceiptmaterialization checks andExecuteOutputpreview-rowreadback untouched;
MatchedSetupProofuntouched.harness_test.gomock now marshals the realinternal/explorer.ExplorerStateV1type instead of a hand-rolled map(mock follows server; kills the shared-blindness class at the root),
plus a
forceEmptyActiveStateflag so the mock can express theserver-legal empty-digest state the old mock hid.
readback_shape_test.go(new): permanent regression tests — real-marshaldecode (fails on the pre-fix client with both fields lost, the guarantee
the old mock never gave) and empty-digest abort through
SetupProofEvidence.Receipts: full benchmark suite ok; catalog + postgres renderer suites ok;
gofmt/vet clean; control run with only the client fix reverted fails the
decode test on both fields. Zero loosening: no gate thresholds, digests
rules, or matched-bytes law changed.
Matcher amendment disclosure #2 (d437550, coordinator-granted, scoped)
The paired run at
79e72ed4(job 132040946330) advanced past the readbackgate and died one stage deeper:
preview decode on arango: json: cannot unmarshal object into Go struct field PreviewResult.columns of type string. Same class, second instance: the authoring-v2 preview routeanswers
ExplorerBuilderPreviewwithcolumnsas generatedloomapi.ContractColumnobjects (internal/server/explorer_preview_response.gov2EmissionColumns; public name at.column);decodePreviewunmarshaledthem into
[]string, and the mock served the same string array plus atop-level
sampledfield the route never emits.Hunks (exactly three):
decodePreviewdecodes via[]loomapi.ContractColumnwith a new fail-closed abort for a namelesscolumn (rows loop,
Bag/CanonicalizeRowsdigest, and theomitted-columns/rows aborts byte-identical); the mock marshals the real
loomapi.ContractColumninside the server-shaped envelope and drops thephantom
sampledscript;preview_shape_test.gois the permanentregression pair (real-marshal decode + nameless abort), failing pre-fix
with the exact CI error string.
Truncation detection (coordinator probe). With
sampledgone, how islimit-truncation detected in the timing phase: the paired run pins
--preview-limit=1000— the engine'sMaxPreviewLimit— while the tinypreset emits at most 48 observation-derived rows, so row-limit truncation
is out of range by construction. Any row-set divergence, truncation
included, is caught by the independent oracle gate (
GateOracleRows,bag-digest equality against the fixture-derived expectation) and by the
cross-arm
GateRowCount+ bag-equality gates.PreviewResult.Sampledis carried from thewire (
json:"sampled", no omitempty): the route wrapper sets it from!summary.Complete || summary.Truncated(internal/serverpreviewResponseFromEncoder, openapi_explorer_authoring_direct.go:571),and
GateCompletenessrefuses any arm whose final preview reportedsampled=true — so a truncated output can never be timed as complete or
back a speed claim. Server-side response-size overflow is additionally
fail-closed: the encoder raises typed
PREVIEW_RESPONSE_TOO_LARGErather than truncating, and the harness turns that into an arm error
before timing.
Decode-vs-server survey (closure argument). Every client-side wire
decode in the harness flow, verified against server source or live CI:
7c5ef71e7c5ef71e7c5ef71ePOST /explorerscreate7c5ef71e7c5ef71eCompileResponse7c5ef71eAuthoringPublication7c5ef71eExplorerStateV1(state_v1.go:27)79e72ed4, cleared in CIloomapi.PreviewResponseviapreviewResponseFromEncoderd4375502+ review repair belowdecodePreviewReview repair (r4567448566, @matthewpeterkort) — sampled restored
Correction, stated plainly: amendment #2's claim that "the route never
emits sampled" was wrong. I verified only the response encoder
(
explorer_preview_response.goFinish writes eight fields) and missedthe route wrapper
previewResponseFromEncoder(openapi_explorer_authoring_direct.go:559-571), which unmarshals the
encoder body into
loomapi.PreviewResponseand setsSampled = !summary.Complete || summary.Truncated(json:"sampled",no omitempty — always on the wire). Dropping it from the decode made
PreviewResult.Sampledpermanently false and silently disabled thecompleteness refusal (harness.go sample.Truncated -> gates.go
GateCompleteness), violating this harness's own contract that
"sampled=true is surfaced, never silently accepted."
Repair, per the review:
decodePreviewcarriesSampledfrom the wirestruct into
PreviewResult; the fakeArm restores asampledscript andthe preview mock now marshals the real
loomapi.PreviewResponsetype(same mock-follows-server standard as amendment #1); and
preview_shape_test.gopins the whole chain — sampled=true wire ->ExecuteOutputSampled ->MeasureArmssample.Truncated ->backendResultFromBackendResult.Truncated ->EvaluateGatesfailscompletenessand blocks claimability. Control receipt: reverting onlythe carry fails exactly those two tests at the named seams; with the fix
the full package suite is green, gofmt/vet clean.