feat(results): record the environment knobs and declared config a run actually used - #264
Open
fcostaoliveira wants to merge 22 commits into
Open
feat(results): record the environment knobs and declared config a run actually used#264fcostaoliveira wants to merge 22 commits into
fcostaoliveira wants to merge 22 commits into
Conversation
Docker Build ValidationPlatforms tested:
Tests performed:
|
This was referenced Aug 8, 2026
… actually used The results JSON carried the configuration *name* and nothing else about how the run was set up: no `collection_params`, no `upload_params`, none of the ~104 environment knobs the engines read, and not even `--host`. Two runs of one committed configuration could differ by `OPENSEARCH_SEARCH_RETRY_BUDGET_MS`, or run against two different servers, report materially different `p99_time`/`rps`/`upload_time`, and produce byte-identical `params` blocks. Adds a per-experiment recorder (`effective_config`) that captures values **at the point they are resolved**, emitted as `engine_params` in the `*-search-*` / `*-upload-*` files (under `params`) and in `*-summary.json` (top level): declared collection_params / upload_params as the FILE spells them effective what the run used, after env + defaults + in-code pinning env which variables were CONSULTED, and what was there invocation --host, --skip-upload and the other measuring flags overridden declared value + used value + why they differ ignored_declared_keys known-unread knobs, marked non-exhaustive phase which phase's resolved state this block describes Recording the declared config would reproduce the failure this repo keeps hitting, so the emphasis is on divergence: * `env_parsed` / `env_or` / `env_flag` / `env_opt` / `env_present` resolve AND record in one step, so the artifact value is by construction the value the caller received. Text that does not parse, or a flag value that is not recognised, leaves the run on its default and lands in `overridden` with both sides. `VALKEY_PROTOCOL=" resp3 "` selects RESP2 and now says so. * Only variables the run actually read appear; one that was read and unset appears as `null` with its defaulted result. * Elasticsearch pins `number_of_shards` in code, and `config::KNOWN_UNREAD` is the CI-asserted inventory of knobs shipped configs declare and engines ignore. Both now reach the artifact, which used to say nothing while the guard said "documented debt (#245)". * `declared` is the raw configuration-file JSON (`EngineConfig::raw`), not a serde round-trip: the round-trip injected nulls, normalised `m`/`ef_construct` to `M`/`EF_CONSTRUCTION`, and silently dropped `index_options.type` and `index_options.confidence_interval`. * Credentials are recorded as *set* and never disclosed. Redaction lives in `Recorder::record_effective`, one choke point, and `snapshot()` asserts over every map that no credential-named key is published in the clear — a leak was otherwise one `env_or` -> `env_parsed::<String>` edit away, with tests green. Three guards keep it from decaying, each asserted to bite: 1. no `std::env::var` in production code outside the recorder (a raw read is invisible to the artifact, and an early return past a recorded read makes the file affirmatively deny a knob that was consulted); 2. `KNOWN_UNRECORDED` lists every remaining raw-text `env_var` site with a reason, asserted in BOTH directions so migrating one forces deleting its row; 3. `experiment::run` still begins a recording before building the engine. `metrics_schema_version` is deliberately NOT bumped: it versions the metric definitions, which did not move. `engine_params.schema_version` is the marker for this block, and it is emitted in all three file kinds. Engine edits are the mechanical `std::env::var` -> recorder swap plus 14 value-transforming knobs moved onto recording helpers. #246's `parse_env_secs` is routed through the recorder too, so `OPENSEARCH_FORCE_MERGE_*` is recorded. Recording the resolved INDEX params (`m`, `ef_construction`, the qdrant HNSW diff) is deliberately excluded and tracked in #273: doing it correctly requires reading them back off the server after `configure()`, because a `--skip-upload` run or a shared ES/OS index makes the process's intent diverge from what the server has, and a field named `effective` asserting a value the server does not have is worse than recording nothing. Closes #212 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fcostaoliveira
force-pushed
the
feat/record-effective-config-212
branch
from
August 8, 2026 14:21
a6eb9e4 to
e5f4f02
Compare
…orced
Review found that the redaction invariant was checked over two flat maps
(`env`, `effective`) while `snapshot()` emitted five value-bearing members. The
three never inspected — `invocation`, `declared`, `overridden` — were exactly
the three the previous commit added, and two leaked live with the unmodified
binary:
--host 'default:PW@127.0.0.1' -> "invocation": {"host": "default:PW@..."}
config file api_key / auth_token -> "declared": {... "api_key": "PW" ...}
both three keys away from a working `<redacted:set>`. Reproduced, then fixed:
* `scrub_value` walks the ASSEMBLED document, so every member is covered and a
member added later is covered by construction. A key is secret if IT or any
ancestor is credential-named, which also closes the nested-object bypass
(`record_effective("PASSWORD", json!({"value": s}))`).
* `overridden` entries name their knob in a sibling `key` field, so that field
seeds secret-ness for the entry.
* `strip_userinfo` handled neither a scheme-less authority (`--host` is exactly
that) nor a password containing `/`, `?` or `#`, which pushed the `@` past a
boundary computed too early and returned the URL untouched.
* `strip_dsn_secrets` blanks `password=` inside opaque connection strings, where
the secret sits under a key that is not itself credential-named.
* Markers widened: `"AUTH_TOKEN".contains("_AUTH")` is false, which is how
`auth_token` slipped through. Now covers AUTH/TOKEN/PASS/ACCESS_KEY/
PRIVATE_KEY/SIGNING_KEY/SESSION.
* An unset credential's built-in default records `<redacted:default>`, not
`<redacted:set>` beside `env: null` — that was the self-contradiction class
this block exists to remove.
Separately, "forgetting the reset is not expressible" was false: four mutations
reintroduced the bug with 776/776 green, and one made a config publish the
PREVIOUS config's `declared` block — strictly worse than pre-#212, where the
artifact merely said nothing. `begin_experiment` now returns a move-only
`Recording` that `run_single_experiment` consumes, so commenting the call out,
calling it conditionally, and hoisting it out of the per-experiment loop are all
compile errors (each verified). Dropping `reset()` from inside `begin_experiment`
still compiles and is caught by a test — a pure `Recorder`-level one now, because
the global-state version passed 9 runs in 11 and 3/3 under --test-threads=1.
Guards widened: guard 1 probes vars_os/temp_dir/current_dir and aliased imports,
and scopes the `src/config.rs` exemption to five named calls instead of the whole
file; guard 2 pins the count of non-literal `env_var` sites; guard 3 strips
comments first. `invocation` is now CI-asserted against every `Args` boolean —
it had drifted by five flags including `skip_vector_index`.
NOTE FOR THE MERGE COMMIT: the previous commit's `Closes #212` trailer is
SUPERSEDED. This PR delivers `number_of_shards` and the retry budgets but not
`m` / `ef_construction`, two of #212's four minimum-viable knobs, so it is
`Refs #212`. `Closes #212` belongs on #273. Do not carry the old trailer into a
squash message.
Refs #212
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged rather than rebased: the branch is already pushed and this repo's standing rule is to add a commit, never to force-push.
Docker Build ValidationPlatforms tested:
Tests performed:
|
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 similar comment
Docker Build ValidationPlatforms tested:
Tests performed:
|
…eal reset guard
Three blockers from review, each reproduced before fixing.
1. `strip_dsn_secrets` — the function added to close the libpq case — split on
whitespace before parsing, so a legal quoted value leaked everything after
the first space:
password='LEAKONE LEAKTWO LEAKTHREE'
-> password=<redacted:set> LEAKTWO LEAKTHREE'
Replaced with a quote-aware scanner that honours single/double quotes and
backslash escapes.
2. Seeding secret-ness from a sibling `key` field blanked that field and
`reason` as well, so an override on a credential knob recorded four identical
placeholders and nothing else — the entry destroying what it exists to
produce. Same shape cost a `{"key":…, "value":…}` header list its names.
`key` and `reason` are now exempt in BOTH the scrubber and the assertion,
which had to move together: the assertion REQUIRED the blank and panicked on
a correctly-scrubbed document.
3. The reset was not guarded. `begin_clears_every_accumulating_field` exercises
`Recorder::begin`, which the mutation does not touch, and the only test that
ever failed relied on residue other tests left in the global recorder:
9 green / 2 red at default threads, 3/3 green under --test-threads=1, and
3/3 red only in isolation. Replaced with a test that drives `begin_experiment`
itself and seeds every accumulating field explicitly. With `reset()` deleted
it now fails 7/7 — alone, in-suite, and single-threaded.
Also: this commit had introduced a guard blind spot. A `#[cfg(test)] const` has
no brace, so `strip_test_modules` brace-counted from the next `{` — the
following function's body — and deleted ~25 lines of production text from the
scan; a raw `std::env::var` planted in `parse_update_search_ratio` was missed
with 786/786 green. `strip_test_modules` is now indentation-based, which also
fixes the pre-existing failure on the five files whose string literals contain
braces, and it strips comments first so a doc comment mentioning `#[cfg(test)]`
is not mistaken for an item. Verified: the planted probe is now caught.
Claims corrected to match the code:
* `Recording` proves a recording BEGAN, not that it began for this (config,
dataset) pair — a token minted elsewhere still compiles.
* `EXEMPT_CALLS` was documented as asserted-present and was only a skip-list.
It now has the assertion its docstring promised.
* `invocation` is bidirectional over `bool` fields ONLY. `--repetitions` and
`--warmup-seconds` both change what is measured and are now recorded by hand;
widening the guard past booleans is #274.
* The scrub changes a JSON type (a number under a credential-named key becomes
a string). Zero false positives across all 31 shipped configs and all 104
recorded knob names today, so this is drift risk, not a live defect — noted at
the line that does it.
Refs #212
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push. #271 adds a `corpus_reuse` argument to `build_search_result_json`; kept alongside `engine_params`. It also adds `--allow-partial-corpus", which the invocation guard immediately flagged as unrecorded — the first live catch by that guard, and exactly the flag whose presence an artifact most needs to state, since it suppresses the short-corpus refusal and therefore permits a wrong recall under a config name that claims otherwise.
fcostaoliveira
added a commit
that referenced
this pull request
Aug 8, 2026
…claims, and correct this PR's own test-teeth story Claim accuracy only — no behaviour change. Every item below is a place where the prose asserted more than the code delivers, including two I wrote myself. BLOCKER 1 — the fourth stale README place, twin of one I had already fixed. README's "cardinality is not identity" paragraph still read: "Only the FOUR Redis-wire engines address a per-config index and keyspace … MongoDB, pgvector, Elasticsearch/OpenSearch, Qdrant, Milvus AND VECTORSETS each have ONE corpus object per server, shared by every config and every dataset — so a full-size corpus uploaded by a sibling config … passes." That is verbatim the assertion I rewrote in `engine/mod.rs`'s trait doc. I fixed the Rust twin and missed the Markdown one, then claimed in the PR body that all three stale places were fixed; there were four. Post-PR it was false three ways: VectorSets is per-config, a sibling's corpus does NOT pass (this PR's own `test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus` proves it), and "four" is "five". It also contradicted my new text 50 lines below, which a reader reaches second. Rewritten to five/ten, with the limit that actually survives on the per-config engines spelled out: a sibling CONFIG no longer passes, but the same config re-run against a DIFFERENT DATASET of equal size still does, because the key derives from the config name alone. BLOCKER 2 — the #230 section was chronologically impossible. It said a pre-#230 corpus "is found, the run succeeds, and only the recall number is wrong … the one failure mode left here", while the next paragraph said a pre-#236 corpus is not found and is rejected. `git merge-base --is-ancestor 66fcaaa fd92603` is true: #230's fix predates this branch, so pre-#230 ⊂ pre-#236 and the two halves are mutually exclusive. After this PR the "right size, wrong encoding, silently found" case is unreachable by any released binary. Now scoped: the wrong-encoding case is reachable only by pinning `VECTORSETS_INDEX_NAME_EXACT=1` at the base `idx`, or by `RENAME idx idx:<config>` — both named, and both explicitly flagged as not migration paths this README recommends. S1 — this PR's own account of where its test's teeth are was wrong. Three places (the merge commit message, the test's doc comment, the PR body) credited "seeds the legacy key with EXACTLY the expected row count" as what makes the migration test sharp — the idea being that a probe hardcoded to `idx` would read 400 of 400 and wave the run through. It would not. That test runs over a PRIVATE base (`idx236mig`, chosen so it cannot race the coexistence test over the global `idx`), so a hardcoded probe reads an empty `idx`, gets 0 of 400, and hard-errors — passing the negative half for the wrong reason. Verified by counterfactual on this tree, `corpus_row_count` patched back to `.arg("idx")`, live, `--test-threads=1`: test_vectorsets_corpus_row_count_tracks_the_live_key ... FAILED (:869) test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus ... FAILED (:777) 10 passed; 2 failed `:869` is `holds 200 of the 400 rows` — a direct wrong-key detector. `:777` is the POSITIVE CONTROL ("--skip-upload against this config's OWN corpus must succeed"), not any seeded-negative assertion. So the merge hazard is caught by CI, by both tests, but not by the mechanism I named. The doc comment now says so explicitly, including why the seeding still earns its place (it makes the scenario realistic — rejected even with a full-size corpus sitting on the server — rather than merely absent). S3 — the headline new metric was undocumented, under a heading that contradicted it. The "Per-config index isolation" heading gained VectorSets, but the line under it still said memory "is reported per-index via FT.INFO", which VectorSets does not use; and `index_memory_bytes` appeared nowhere in any .md. Added a table giving both fields, their sources (`FT.INFO` vs `MEMORY USAGE <key>`) and their scopes, plus the two consequences a reader needs: `used_memory` from a pre-#236 run and from a current sweep are not comparable quantities, and an orphaned legacy `idx` keeps inflating `used_memory` in every later result file until it is deleted. S4 — `experiment.rs`: both stale comments fixed after all. `:734`'s historical measurement record (`VCARD idx`) and `:797`'s destructive-call list (`DEL idx`) are now `VCARD idx:<config>` and `DEL <key>`, matching the README line I had already updated. Comment-only, two words; #264 merges clean over them (re-checked with `git merge-tree`). My earlier disclosure said `:796` — the `DEL idx` line is `:797`. Test: the memory test now asserts SCALING, not just presence. Every previous assertion was satisfied by a fixed per-key overhead figure, which is the plausible way `MEMORY USAGE` could be meaningless (a module type with no `mem_usage` callback reports bare key overhead). It now compares the 400-element corpus against a 1-element vector set of the same dimensionality created on the same server, and requires >10×. Artifact for a claim that had none. The README asserted "measured 437 → 3413 QPS" for the empty-corpus case with no transcript in the PR. Re-measured on this tree, 400-vector corpus, ef=400 parallel=1, same server, via `--skip-upload --allow-partial-corpus` after deleting the key: FULL VCARD=400 rps=637.55 recall=1.0 failed_queries=0 EMPTY VCARD=0 rps=2679.26 recall=0.0 failed_queries=0 4.2× faster for measuring nothing, `failed_queries: 0` in both. README now carries these numbers instead of the unsourced pair. Master baseline is a RACE, not a number. I reported "1 passed / 6 failed, three times". Re-measured 11 times at default threads, dedicated container, FLUSHALL between runs, at `fd92603`: 0 passed / 7 failed ×3 1 passed / 6 failed ×7 2 passed / 5 failed ×1 So it is 0–2 of 7 depending on interleaving. `mixed_benchmark` is the most frequent survivor but not the only one (`datetime_keyword_schema` also survived once), so "the one that survives" was wrong twice over. This branch: 12/12, three consecutive runs, no variance. Gates (baseline: master fd92603 = 872): cargo test --lib --bins → 874 passed, 0 failed cargo clippy --all-targets -- -D warnings → clean cargo fmt --check → clean cargo test --test integration_vectorsets → 12/12 ×3 at default threads cargo test --test overhead_invariants → 9/9 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er-stripping
B1 — a password in a URL QUERY STRING was published verbatim in
`invocation.host`, in all three file kinds:
mongodb://h.example/db?w=majority&password=CANARY -> unchanged
redis://h:6379/0?db=1&password=CANARY -> unchanged
`strip_userinfo` declines (no `@`) and the whitespace-splitting DSN scanner read
the key as `mongodb://h.example/db?w`, which is not credential-named, so the
whole tail came through. `?authSource=admin&password=X` only LOOKED fixed: the
first key contains AUTH and blanked the tail by accident, and any benign leading
option (`w=`, `retryWrites=`, `ssl=`, `db=`) restored the leak. The tests
therefore pin the benign-leading-option cases specifically.
Rewritten as a proper key=value scanner: separators are whitespace, `&`, `;` and
`?`; spaces are allowed around `=` (PostgreSQL documents them as optional);
values may be quoted with `'` or `"` honouring backslash AND SQL doubled-quote
escapes. This also closes the other eight shapes a 44-case corpus found —
`spaces-around-eq`, `space-before-eq`, `space-after-eq`, `sql-doubled-quote`,
`pwd=` (a plain marker gap: password/passwd/PGPASSWORD matched, pwd did not),
`jdbc:`, `&`-separated and both ODBC `;` forms. Non-secret pairs are reproduced
byte for byte, verified including `options='-c statement_timeout=5s'`.
S2/S3 — `strip_test_modules` over-stripped in two more shapes, each proven with
a planted probe that the guard missed before and catches now:
* a BODILESS `#[cfg(test)] mod filter_guard;` (engine/mod.rs:12) has no block,
so the scan hunted for the next column-0 `}` and deleted lines 13-60;
* a trailing comment on a closer (`} // end of the config tests`) left `"} "`
after comment-stripping, never matched, and ran to EOF — silently disabling
the guard for the rest of the file.
Both fixed, plus a fail-safe: an unterminated module now emits its lines rather
than deleting to EOF, because over-scanning is a visible false positive and
under-scanning is invisible.
S4 — three claims that were false, two of them shipped in the repo:
* `--repetitions` and `--warmup-seconds` were asserted to be recorded and were
not — an earlier edit aborted before writing and the claim survived in a
comment that the guard's boolean-only limit leans on. Now actually recorded.
* the `env::current_dir()` waiver cited `invocation.configurations_dir` as its
compensating control; that field did not exist. Now emitted (with
`results_dir`), and a new test asserts the cited record exists — the old
assertion only checked the excused CALL was still there.
* `begin_experiment`'s docstring still claimed forgetting the reset is "not
expressible". Deleting `reset()` compiles; it is caught by a test. Corrected,
and it no longer contradicts `Recording`'s docstring in the same file.
Also: `invocation`'s guard now bites both ways (a fabricated key mapping to no
`Args` field was green), and `EXEMPT_CALLS` matches STRIPPED production text —
which immediately found a stale row of its own, `src/config.rs: env::var(k)`,
excusing a read that lives in a `#[cfg(test)]` helper. Deleted.
Refs #212
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker Build ValidationPlatforms tested:
Tests performed:
|
… instead Seven more credential shapes reached `results/*.json` at b9d4e02 — percent-encoded (`%70assword=`), Cyrillic homoglyph (`pаssword=`), `AccountKey=`, `bearer=`, `Pwd=` inside quotes, and any pair nested in a quoted run under a non-secret key. All one class: TOKENISER DESYNC. A block-list over connection strings is a block-list over an unbounded input language, and a single unbalanced quote disabled the redactor for the remainder of the string. Three rounds each closed the demonstrated case and left the class open. So the default is inverted. `redact_connection_like` REBUILDS a connection-like value from positively recognised components — scheme, host, port, path, and allow-listed parameters whose values are simple — and drops everything else, echoing only the sanitised parameter NAME (`password=<dropped>`). Nothing reaches the artifact unless it was recognised, so a desync loses benign detail instead of publishing a credential. None of the seven shapes had to be predicted: their parameter names are simply not on the allow-list. `options` is deliberately excluded because its value can itself contain pairs. A string with none of `://`, `@`, `=` is returned unchanged, which is the overwhelming majority of what gets recorded (`localhost`, `idx:redis-…`, `FLOAT32`, paths). Authored prose (`reason`, `covers`) is exempt from reconstruction: `;` alone is no longer treated as a connection-string signal after it turned our own explanations into rubble. The shipped comment claiming "every client rejects `?password=` as an unknown option before a file is written" is DELETED. It is false — `REDIS_URI=redis://127.0.0.1:6411/?password=X` completes with rc=0 and writes three files — and building the test on that premise is what let seven shapes survive. The corpus test now writes real files and greps the bytes on disk, across search, upload and summary. `strip_test_modules` had three more over-strips, and it had never had a test despite six such bugs found by hand across four passes. Now pinned by a table-driven test, and fixed: `//` inside a string literal (93 lines under `src/`), a block-commented fake module, and a block comment after a bodiless `mod X;`. Writing that test immediately exposed a seventh — a `///` doc comment mentioning `experiments/configurations/*.json` contains `/*`, so stripping block comments first ran to EOF and blanked 2300 lines of config.rs, including `project_root`'s `env::current_dir()`. Caught by the exemption assertion added last round. Also fixed: * `repetitions` / `warmup_seconds` were recorded but UNGUARDED — deleting either left the suite green while `NON_BOOL_INVOCATION_KEYS` made them look covered. Now asserted present. * `NON_LITERAL_ENV_VAR_SITES` deduped file paths, so a second dynamic `env_var(x)` in the same file passed. No longer deduped, and the named function must exist. * Five false shipped comments corrected: the stale `format!`/base-var claim that survived beside its own correction 25 lines away; two different figures for one 11-run measurement; the `src/config.rs` whole-file exemption prose left after the list changed; `start_gate.rs` named as containing `{` literals when it has none; and "104 recorded knob names", where the derived figure is 97. Refs #212 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push.
Docker Build ValidationPlatforms tested:
Tests performed:
|
…corder #278's two new `index_naming` tests resolve through the recorder (`derive_index_name` -> `env_or` / `env_flag`) without holding `test_lock`, so an observation can land between another test's `begin_experiment` and its `snapshot()`. Six consecutive green suites did not disprove it — the window is a few instructions wide — and "measured green" is precisely the evidence that was wrong about this test last time. Closed two ways rather than one: * those tests now take `effective_config::test_lock()`, which any test driving the global recorder must; * `begin_experiment_clears_the_previous_experiments_accumulation` now asserts the keys it SEEDED are gone rather than asserting total emptiness, so a concurrent writer cannot fail it. Equally strong against the mutation, since deleting `reset()` leaves exactly those keys behind — re-verified 5/5 red (3/3 alone, 1/1 in-suite, 1/1 `--test-threads=1`). Total clearing stays covered deterministically by `begin_clears_every_accumulating_field`, which owns a private `Recorder`. These changes were written before the merge commit and lost to a `git checkout HEAD -- src/` used to measure the master baseline; recommitted here rather than amending, since the branch is published. Refs #212
Docker Build ValidationPlatforms tested:
Tests performed:
|
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push.
Docker Build ValidationPlatforms tested:
Tests performed:
|
2 similar comments
Docker Build ValidationPlatforms tested:
Tests performed:
|
Docker Build ValidationPlatforms tested:
Tests performed:
|
This was referenced Aug 8, 2026
`UNCOMPILED` was the one inventory in this module with a skip filter and no assertion. `KNOWN_UNRECORDED` and `EXEMPT_CALLS` are both bidirectional — the latter was upgraded precisely "so a removed one cannot leave a stale excuse" — and the same reasoning applies here with more force: a stale skip row silently WIDENS what the scanner ignores, which is the direction that hides violations rather than surfacing them. `src/vectorsets/` is being deleted (#297) and `src/redisearch/` next (#296), so the row would have rotted unnoticed. Two legs, each proven to bite: * the directory must still EXIST. Simulating #297 by moving `src/vectorsets/` away fails with "UNCOMPILED lists `src/vectorsets/`, which no longer exists — delete the row … Tracked in #275 / #296 / #297". * no crate root may DECLARE it, or the code is compiled, can read the environment at runtime, and is being skipped anyway. Adding a row for `src/readers/` (which lib.rs does declare) fails with "IS declared as a module and therefore compiled". Leg 2 could not be demonstrated by declaring `vectorsets` itself: adding `pub mod vectorsets;` to lib.rs does not COMPILE, which is independent evidence for #297's premise that the directory is dead. The four `src/config.rs` exemption reasons justified themselves by naming `src/redisearch/` and `src/vectorsets/` as `RedisConfig::from_env`'s only callers — half-true once #297 lands, false once #296 does. They now state the durable property instead ("no caller compiled into any binary") and `the_exempted_dead_constructor_has_no_live_caller` asserts it; planting a real call in `engine/redis.rs` fails it. After both deletions the constructor has zero callers and the right move is deleting it, which the reason now says. Citations cross-linked: #275 covers both directories and the constructor, and is superseded piecemeal by #291/#296/#297. Refs #212
…and declared keys Three blockers, one of them a regression against this branch's own round-2 fix. B1 — `redact_connection_like` sliced the authority on `/?#` BEFORE looking for `@`, so a password containing any of those pushed the `@` out of the slice, `rfind` found nothing, and `admin:LEAKCANARY` fell through to the host slot and was published. b9d4e02 had an explicit "malformed-but-dangerous fallback" for exactly this, added in round 2 and cited in the PR body; the round-5 rewrite dropped it and reproduced the bug verbatim. Six shapes, 3/3 files each, plus `REDIS_KEY_PREFIX` through the same choke point. Now the `@` is located over the WHOLE remainder before any slicing, and the host slot is itself allow-listed: `scrub_authority` publishes only `host` or `host:port` with a numeric port, so a stray `:` makes the token unrecognised rather than printable. B2 — `value[..i+3]` was pushed out as "the scheme". It is all text before the first `://`, so `password=LEAKCANARY;endpoint=x://h` was republished byte for byte: the presence of a `://` anywhere disabled the parameter path for the whole prefix. Schemes are now allow-listed (`SAFE_SCHEMES`); a well-formed but unknown scheme becomes `<dropped>://`, and a prefix that is not a scheme at all makes the whole string parse as a parameter blob so the allow-list runs over it. B3 — the inversion had been applied to connection-string VALUES but not to JSON KEYS. `declared` copies `collection_params`/`upload_params` in as raw file JSON, so any key not matching SECRET_MARKERS whose value lacked `://`, `@` or `=` was published untouched: `bearer`, `accountkey`, `jwt`, `pw`, `creds_blob`, `signature`, `cookie`, and colon-delimited `Server:h;Pwd:X`, which the connection-string fast path never sees. `bearer` and `AccountKey` were documented as "closed by construction" — true as parameter names, wide open as key names. Declared keys are now allow-listed too (`DECLARED_KEY_ALLOWLIST`, derived from all 47 keys the shipped configs use); numbers and booleans pass regardless of key since they cannot carry a credential. Measured: 0 of 212 shipped config entries lose a value. The structural fix, which matters more than the three: * `CONNECTION_SHAPE_CORPUS` is APPEND-ONLY, 44 entries, floor asserted. Five rewrites each closed what was just reported and lost something closed earlier because the corpus only grew by the latest finding. * Every entry carries a canary on BOTH sides of its delimiters. Round 5's `userinfo-slash-pw` had the canary only after the `/` — in the half that gets dropped — so the two rows naming the shape were the two hiding it. The `@` rule is asserted mechanically. Also: `SAFE_PARAM_KEYS` and `is_secret` contradicted each other in one artifact (`?authSource=admin` published, `"authSource": "admin"` redacted). `authsource`/`target_session_attrs` are dropped rather than exempted so `no_allow_listed_parameter_is_credential_named` can be enforced — which is what blocks `sslpassword`, `passfile`, `proxyPassword` and `authMechanismProperties` from being added in good faith later. NIT: `configurations_dir`/`results_dir` published `/home/<username>/…` in every artifact; now `~`-relative. Reading `HOME` is scope-exempted with the reason that recording it would republish the value the call exists to remove. Refs #212
…st for the rest A generative campaign (823,920 cases, 116,434 leaking, 5 root causes) found in one pass what six hand-picked rounds did not. Every finding was a desync state of my tokeniser rather than a missing rule, so the tokeniser is gone. * `scheme://` values are parsed by the `url` crate and rebuilt from the STRUCTURED fields it returns. Userinfo is never one of them, so a `@` inside a query value cannot promote that value's tail into the host slot (L1) — the worst of the five, because the leaked bytes sat immediately beside `<redacted:userinfo>`, asserting redaction at the exact point it failed. `redis://[admin:LEAK]` now fails to parse and is digested rather than passing a bracket check that never looked inside the brackets (L3). * Everything that is not a parseable URL with a recognised scheme and a real host becomes `<redacted:opaque sha256=… len=…>` — identity without disclosure. L2 (`Server:h;Pwd:X`, no `://`/`@`/`=` at all) and L4 (a quoted fragment that looks like an allow-listed key) existed only because the code was trying to salvage readable output from arbitrary DSNs. Nobody reproducing a benchmark needs the ODBC string verbatim; they need to know it was the same one. * A scheme-LESS authority (`--host default:pw@node`, the commonest form) is parsed with a synthetic scheme so `url`, not a tokeniser, decides where the userinfo ends. Two ambiguity gates: an `@` that delimited no userinfo, and userinfo followed by an implausible host, both go to a digest — otherwise `--host p@ssword` publishes `ssword` in the host slot. Permanent generative property test, fixed seed, 4000 bounded cases. Ground truth by construction: the generator knows which spans it put in permitted slots (host, port, path, allow-listed values) and which in forbidden ones (userinfo, non-allow-listed parameter values); forbidden spans carry a canary and the oracle is that no canary survives, through `redact_connection_like` AND through the full snapshot path. Asserted non-vacuous (>2000 cases must actually contain a forbidden span) and mutation-checked: publishing query values verbatim fails it in 2 cases. The append-only corpus makes a LOST fix fail; only a generator makes an UNANTICIPATED shape fail. Both are now in place, and the reviewer's point stands that they are orthogonal — the both-sides-of-a-delimiter rule was satisfied by every corpus row and was simply the wrong dimension for L1. Over-redaction casualty fixed rather than accepted: replica-set seed lists (`mongodb://h1:27017,h2:27017/db`) and SQL Server `host,port` survive, via an explicit host-list check applied only when every element parses as `host[:digits]`. Refs #212
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 similar comment
Docker Build ValidationPlatforms tested:
Tests performed:
|
Eight rounds, seven leaked. The re-run found six more classes in 381k cases —
and BLOCKER 1 was the host-list feature added LAST round to recover provenance:
`split_authority` searched for `@` across the whole remainder, so a `@` in a
query value made the text after it the "authority", and if it contained a comma
it was printed verbatim beside a `<dropped>` claiming redaction, replacing the
real host.
That is the pattern, not an accident: every mechanism that preserved readable
output was a parser, and every parser had a desync. So the parser is gone.
`redact_connection_like` is now two outcomes and no third path:
* a plain token — `[A-Za-z0-9._+\-/~]` only — is the identity;
* everything else is `<redacted:opaque sha256=…>`.
No URL parse, no authority split, no host display, no query-pair echo, no scheme
or parameter allow-list. B1-B3, SF4 and SF5 are unreachable by construction:
there is no slice of the input on any path to the artifact. ~200 lines deleted.
`:` is NOT in the plain alphabet. No shape test can tell `admin:Sup3rSecret`
from `idx:redis-docker-test`, so index names digest too; they are derivable from
`params.experiment`, and a published password is not derivable from anything.
Generator fixed — it was the reason these survived:
* the oracle was whole-canary containment while EVERY live class leaked a
FRAGMENT (round 6's own `p@ssX -> ssX` would have passed it). It is now a
5-char contiguous-run check. Mutation-checked twice: republishing the tail
after `@` and leaking a bare 6-char suffix are both caught;
* delimiters are now placed INSIDE the secret, not only after it;
* commas exist at all — the dimension round 8's blocker came from;
* secrets use a G-Z alphabet, disjoint from lowercase hex, after the first
run produced a false positive against the digest's own characters.
Corpus +11 rows (floor 44 -> 55): bracketed authority (L3, demonstrated in
round 7 and never appended despite the append-only rule), the comma and
host-list shapes, `Server:h,Pwd:X`, `Server:h Pwd:X`, bare `admin:secret`, and
`?k=v&secret` with no `=`. The per-row both-sides rule is now a weak per-row
check plus a corpus-wide set property: an `@` cannot be classified soundly per
row (a `/` inside the password moves the apparent authority), but the corpus
must contain rows probing BOTH halves — which is the dimension it lacked.
Smaller items from the review:
* `sha2 = "0.11"`, matching the lock. `"0.10"` had pulled in six crate
versions including `generic-array`, which was not in the lock at all.
* 8 digest bytes, not 4: 4 collided 5 times in 200k values.
* `len=` dropped — it disclosed the credential's exact character count.
* `url` removed again; nothing parses now.
Refs #212
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push.
Docker Build ValidationPlatforms tested:
Tests performed:
|
fcostaoliveira
added a commit
that referenced
this pull request
Aug 9, 2026
…nce Feb 2026 (#297) Closes #291. `src/vectorsets/` was a PyO3 extension that has not compiled since February 2026. `ebd4a5c` (2026-02-27) moved it out of a crate root that declared it and, in the same commit, added the real `engine/vectorsets.rs`; the new `src/lib.rs` never carried the declaration over. Appending `pub mod vectorsets;` today gives **190 errors**, the first being `error[E0433]: cannot find module or crate 'pyo3'` — and `pyo3` is in neither `Cargo.toml` nor `Cargo.lock`. The directory is unbuildable, not merely unreferenced. It still contained `VADD idx` / `VSIM idx`, the hardcoded key #236 spent a PR removing everywhere else, and it kept attracting edits: two repo-wide refactors touched it after it died, the most recent (`1282eda`, #249) **162 days** later. Deadness established six ways and independently re-derived: nothing declares it (`src/vectorsets/mod.rs` exists, so a declaration *would* pick it up — hence the compile proof); no reference to any of its three public types anywhere, including `tests/`, `v0/` and the `fuzz/` crate, which has its own `[workspace]` and path-depends on the root lib so it can only reach what `src/lib.rs` exports; no packaging or CI path picks it up. The one literal `src/vectorsets` hit outside the module is `Dockerfile:26`, a `mkdir -p` fabricating a stub tree for dependency caching, before `rm -rf src` at :39 and the real `COPY src` at :43 — confirmed by an actual `docker build`, which passes. Also removes the module's own entry from `memory-bank/standards/coding-standards.md`'s tree map, which would otherwise describe a directory that no longer exists. `src/redisearch/`'s block stays — that directory still exists and goes with #296. Delta zero on every gate, which is the correct signature for deleting never-compiled files: 919 unit tests before and after, `overhead_invariants` 9/9 both sides, `integration_redis` 41 both sides, fmt and clippy clean. Files under `src/` go 62 → 58 against the guard's `> 20` floor; backslash-continuations are unchanged at 307 because the deleted files contain none. **One thing here is not delta-zero.** #264 carries `const UNCOMPILED = &["src/redisearch/", "src/vectorsets/"]` with a bidirectional assertion added after this PR was opened, so once this lands #264 must delete the `"src/vectorsets/"` row before it can go green. That is one string, and it is recorded on both PRs. #275 remains open as the umbrella: #291 and #296 are the two halves of its item 1, and its items 2-4 — `RedisConfig::from_env`, `WEAVIATE_USE_GRAPHQL`, a stale comment — are untouched. `from_env`'s remaining ten non-test call sites are all inside `src/redisearch/`, so its item 2 unblocks with #296, not here. Reviewed by three adversarial agents. 5 files, 1,426 deletions, **zero additions**.
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push.
…ps printing the leak #297 landed, so `UNCOMPILED` lost its `src/vectorsets/` row — the bidirectional assertion added when #297 was opened failed on the merge and named the string to delete. Contract working as designed. SF1 — a third identity path published operator input verbatim. `prose` was derived by matching the JSON key name against `PROSE_KEYS = ["reason","covers"]` at ANY depth, and `set_declared` forwards a raw un-allow-listed config subtree into `note_override`. A config placing a `{"reason": …}` object at one of three CI-pinned paths therefore published it in the clear — the same value `<dropped>` on the allow-listed path and cleartext two keys away, which is exactly the "artifact asserts a redaction it did not perform" class this series exists to kill. `assert_no_cleartext_secrets` could not see it: `reason` is not credential-named. `PROSE_KEYS` and the prose arm are deleted. `reason` is now `&'static str` on `Override`, held OUTSIDE the scrubbed tree and spliced in by position after the scrub; `covers` is a hoisted literal spliced the same way. Only a string literal can reach either, which a key name cannot forge. Regression test plants four canaries through both routes. SF2 — `assert_no_cleartext_secrets` printed the credential it caught, under no `cfg`, so in release too. It fires only when the redactor has a bug, i.e. the moment the value must not be written anywhere, and benchmark stderr gets pasted into issues. The offending value is now digested in the message. Generator and corpus: * the `canary_before_at >= 5` floor had slack 9 against an actual 14, so nine `@`-rows could be defanged unnoticed — including `userinfo-slash-pw`, the row that memorialises round 5's regression, whose defanging survived 850/850. Now asserted per-row for every `userinfo*` label; verified the exact mutation fails. * the plain-token alphabet is pinned by `plain_token_alphabet_is_pinned`. Adding `@`, `;`, `&`, `#`, `?`, `%`, `,`, `*` or `!` each survived the whole suite; only `:` and `=` were covered. * `assert!(checked > 5000)` was tautological (incremented once per iteration); it now counts cases that actually reached the digest path. * the doc states where the generator's coverage STOPS — it drives `redact_connection_like` and `invocation.host` only, so no seed of it could have produced SF1. Documented rather than changed: `is_plain_token` protects nothing punctuation-free (a bare `hunter2`, a UUID, `ghp_…` are published; `is_secret` on the key is the sole defence for those), and the digest is an UNSALTED hash and therefore a commitment — a guessed endpoint can be confirmed offline. Both are the intended trade; neither was written down. Also: Cargo.toml's comment still advertised the deleted URL parser; `shared_corpus_key_prefix`'s digest buys no secrecy while `env.REDIS_KEY_PREFIX` shows the same text; paths outside `$HOME` stay absolute. Refs #212
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 similar comment
Docker Build ValidationPlatforms tested:
Tests performed:
|
SF1 — `<dropped>` and `<redacted:default>` never reached an artifact, and eight
comments plus a commit message said they did. `snapshot` scrubs the whole
assembled document, so both were re-processed: `<dropped>` is not a plain token
and digested to the hash of the literal `<dropped>`, and `<redacted:default>`
under a credential-named key fell through to `<redacted:set>`.
The second is the damaging one and is the prose bug inverted. An ES/OS/pgvector
run with NO password set published
effective.ELASTIC_PASSWORD = "<redacted:set>"
— the artifact asserting a credential was supplied when the built-in default was
used, which misleads exactly the operator auditing which credentials a run
actually had. `record_effective_raw` existed to prevent this and was defeated,
so `env_or`'s `is_secret(name) && !was_set` branch was dead as far as the
artifact was concerned.
`scrub_value` now passes an already-emitted sentinel through unchanged. Verified
live: with no password set the artifact reads `<redacted:default>`, an unknown
declared key reads `<dropped>`, and `m`/`ef_construction` still read normally.
Two tests pin it — the write path and `env_or`'s set-vs-default branch end to
end — and removing the arm fails them.
SF2 — the splice's positional coupling between `assemble` and `snapshot` was a
convention shared by two functions and asserted nowhere: reversing the iteration
in `assemble` mis-attributes every reason and left all 852 tests green, because
no test had two overrides with distinct reasons. Now three, checked pairwise;
the `.rev()` mutation fails it.
NITs:
* round 5's exact shape was still defangable on the row that memorialises it.
"Some canary before the `@`" passed for
`redis://admin:pw/CANARY@10.0.0.5:6379/0` — the canary sitting in the half
the buggy code dropped. The rule is now "in the userinfo, before its first
internal delimiter", and that mutation dies.
* the docstring claimed "a canary on BOTH sides" while the code three lines
down required some side; it now describes what is actually asserted.
* `MINIMUM_SHAPES` was 55 against an actual 56 — one row deletable silently.
* `mongodb-srv` had userinfo but its label skipped the per-row loop; renamed
and given a two-part canary.
* the #297 remediation was half-followed: the row went, the prose did not.
Three passages still described `src/vectorsets/` as live or its deletion as
pending.
Refs #212
The sentinel pass-through added last commit was a bypass. `is_sentinel` matched
three placeholders exactly and the digest by PREFIX, and its arm sat above both
the credential-key guard and the redactor, so a value merely beginning with
`<redacted:opaque sha256=` was published verbatim. Confirmed on disk before
fixing: `--host '<redacted:opaque sha256=x> password=FORGEDCANARY'` round-tripped
into all three artifacts.
This is the guards-one-notch-weak pattern inside the fix for the previous
instance of it — three sentinels compared exactly, the fourth by prefix because
its content varies, and nobody varied the suffix.
Fixed structurally rather than by tightening the predicate. A `String` that
merely LOOKS like this module's output is exactly the ambiguity that has cost
this PR ten rounds, so there is no longer anything that asks the question:
* the write path stores RAW values — `observe_env`, `record_effective`,
`note_override` and `set_declared` no longer scrub;
* `snapshot` scrubs ONCE, per member, over values this module has never
processed before, with the key in hand;
* the declared-key allow-list is folded into that same traversal, so
`<dropped>` is emitted by a terminal arm instead of by an earlier pass whose
output the scrub then re-read;
* `<redacted:default>` is a marker on the KEY (`from_default: BTreeSet`), not
a magic string in the value, so `record_effective_raw` and `is_sentinel` are
both deleted along with `redact` and `allowlist_declared`.
A credential-named declared key now reads `<redacted:set>` rather than
`<dropped>` — "we removed a secret" and "this knob is not one we read" are
different facts, and the leak guard was right to reject the second standing in
for the first while I had them conflated.
Corpus +6 forged-sentinel rows (floor 56 -> 62), each a sentinel prefix followed
by a canary, because the SUFFIX is the dimension an exact-match test passes and
a prefix-match test leaks. Verified: reintroducing the prefix bypass fails
`no_connection_string_shape_publishes_its_credential` on `forged-digest-prefix`.
Re-verified live: the reproducer is 0/3, and a run with no password set still
publishes `<redacted:default>` while an unknown declared key reads `<dropped>`
and `m`/`ef_construction` read normally.
Refs #212
Docker Build ValidationPlatforms tested:
Tests performed:
|
…n it CORRECTION to 3b27709's commit message, which named the wrong guard for its own verification. It claimed reintroducing the prefix bypass fails `no_connection_string_shape_publishes_its_credential`. It does NOT: that test calls `redact_connection_like` directly and the bypass arm sat ABOVE it in `scrub`, so the mutation passed 855/855. The only guard that fails is `experiment::tests::engine_params::no_shape_reaches_the_files_on_disk`, on `[forged-digest-prefix]`. Naming the wrong guard is this PR's recurring failure mode and it matters here: a future reader deleting that "redundant" on-disk test would remove the sole coverage for the class. SF1 — `snapshot`'s doc still claimed scrubbing the assembled document made "cannot miss a map somebody adds later" true rather than aspirational. Removing the double pass put the manual enumeration back and the claim went stale with it; three members already sat outside it (`phase`, `overridden[].key`, `ignored[]`). No disclosure — all three carry names — but the doc asserted a guarantee the code no longer provided, which is the exact failure this module exists to prevent. Both fixed rather than just the doc: * `phase` is `&'static str`, so only a literal can reach it; * `overridden[].key` and `ignored_declared_keys.keys` go through the same value scrub — a name is not exempt for being a name; * `member_set_is_pinned_so_a_new_one_must_be_scrubbed_deliberately` replaces the guarantee the restructure removed: adding a member is now a build failure naming the fix. Verified — an unscrubbed member fails it. And a hole the audit surfaced while proving the three were safe: JSON map KEYS are never scrubbed anywhere. Correct — they are structure, not data — except under `declared`, whose keys come from the configuration file, where `{"password=SECRET": 1}` publishes the secret in a position no value scrub can see. A non-plain-token declared key is now digested. All 47 shipped keys are plain tokens, so this costs nothing; pinned by `a_secret_in_a_declared_key_name_is_not_published`. SF3 — `record_effective`'s doc still opened "Redaction happens HERE, not in the callers" in a function whose body is now a bare insert. Contradicted by the same commit that wrote it. NITs: `BEARER` added to `SECRET_MARKERS` — the corpus carried `bearer=` as a credential shape while the marker list did not, so `index_params.bearer` read `<dropped>` rather than `<redacted:set>`; harmless while dropped and a live leak the moment anyone allow-lists the key. `begin()`'s doc said "four of the seven fields" and is now five of eight, with `from_default` asserted. Dead `let _ = is_secret(key);` removed. Refs #212
Docker Build ValidationPlatforms tested:
Tests performed:
|
2 similar comments
Docker Build ValidationPlatforms tested:
Tests performed:
|
Docker Build ValidationPlatforms tested:
Tests performed:
|
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push.
Merged, not rebased: the branch is published and the standing rule here is to add a commit rather than force-push. The merge was textually clean and did NOT compile. #298 added two tests calling `build_search_result_json` with master's 11 arguments; this branch's twelfth is `engine_params`. git reported no conflict because the two sides touched different lines of the same call. Both call sites fixed — this is the class the "a clean merge is not evidence" rule exists for, and it is the second time this cycle it has bitten in exactly this way. Checked rather than assumed: * the recorder wiring survived — `begin_experiment` at experiment.rs:366 still precedes `create_engine` at :372, with #298's `gate_update_attribution` landing at :1043 and its call site at :1585, well after; * no raw `env::var`/`vars`/`var_os`/`temp_dir`/`current_dir` was reintroduced by #298 anywhere under `src/`; * planted probes CAUGHT in all six files #298 touched (`engine/mod.rs`, `experiment.rs`, redis, valkey, vectorsets, mongodb). The three new artifact fields do not touch the scrub path, verified rather than assumed: `update_failures` and `update_unattributed` are counters, `update_attribution` is a closed three-value enum rendered through `as_str() -> &'static str`, and `update_attribution_detail` is a per-engine literal at all five call sites. They land in `results`, not `params`. That prompted the one substantive change here. The member-set guard now states its SCOPE: it pins `engine_params` and nothing else, and extending it artifact-wide is not viable — `server_metadata` alone contributes ~1,400 server-supplied strings from a full INFO/CONFIG dump that churn with every engine version. Checked while establishing that: `server_metadata` carries its own redaction and emits a distinct `<redacted>` for `requirepass`, `masterauth` and `tls-*-pass`, so that surface is covered by its own mechanism rather than uncovered. Saying so beats leaving a reader to assume the guard spans the file. Baseline derived in an isolated worktree: master e99d2b5 = 937 (129 lib + 2 generate_dataset + 806 main), branch = 1006, +69 / -0.
Docker Build ValidationPlatforms tested:
Tests performed:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
A result file recorded the configuration name and nothing else. No
collection_params, noupload_params, none of the ~104 environment knobs the engines read, and not even--host. Onorigin/master, four runs differing inREDIS_QUERY_TIMEOUTand in which server they hit produce this, four times, byte for byte:{"dataset":"random-100","experiment":"redis-docker-test","parallel":1,"top":1, "search_params":{"ef":128},"target_qps":null,"duration_seconds":null,"max_lateness_ms":null}What lands in the artifact
engine_params, in*-search-*.json/*-upload-*.json(underparams) and in*-summary.json(top level — soparamsis not the only place this PR writes, thoughresultsis untouched).declaredcollection_params/upload_paramsas the configuration file spells themeffectiveenvnull= read and unset)invocation--host,--skip-upload, and every other measuring flagoverriddenignored_declared_keys{exhaustive: false, covers: …, keys: [...]}phase/schema_versionLive evidence
Engines exercised live: redis and elasticsearch only. The other 13 rest on source reading, the guards, and unit tests. Everything below is one of those two.
Same config, four runs, only the environment and
--hostdiffer:C is the case that matters most: the run used 90000, somebody tried to set something else, and the file says both.
IGNORED_VAR=1, exported for run A and read by no engine, appears nowhere.declaredis the file text, not a serde round-trip —index_options.typeandconfidence_intervalsurvive (the round-trip dropped both), casing is as written, nonulls injected.overriddencarries the ES pinned shard count (declared 3 / effective 1) and an entry fromconfig::KNOWN_UNREAD, the repo's CI-asserted inventory, now wired in — the guard used to say "documented debt (#245)" while the artifact for that same config said nothing.Server read-back (elasticsearch): recorded
number_of_shards: 1andELASTIC_TIMEOUT: 300; live_settings{"refresh_interval":"-1","number_of_shards":"1","number_of_replicas":"0"}. RedisFT.INFOconfirms the recorded env knobs and the derived index/prefix names.mandef_constructionaredeclaredonly and are not asserted against the server — that is #273, and "FT.INFO agrees with declared" is precisely the inference #273 exists to argue is unsafe.Two credential leaks this PR introduced, found in review, fixed and re-verified
The invariant was checked over two flat maps while
snapshot()emitted five value-bearing members. The three never inspected —invocation,declared,overridden— were exactly the three this block added.--host 'default:PW@127.0.0.1'(run succeeds, password authenticates)"host": "default:PLAINTEXT-CANARY-hunter2@127.0.0.1"in 3/3 files"host": "<redacted:set>@127.0.0.1", 0/3api_key/auth_token/endpointwith inline userinfovendor.cloud/v1retainedFixes, all at the choke point rather than per-caller:
scrub_valuewalks the assembled document, so every member is covered and one added later is covered by construction. A value is secret if its key or any ancestor key is credential-named — which also closesrecord_effective("PASSWORD", json!({"value": s})).overriddenentries name their knob in a siblingkeyfield, so that field seeds secret-ness for the entry.strip_userinfowas wrong twice: it requiredscheme://(but--hostis a bare authority, and that is how the Redis-wire and Mongo engines take a password), and it computed the authority boundary before looking for@, soredis://admin:hunt/er2@10.0.0.5:6379/0came back untouched.strip_dsn_secretsblankspassword=inside opaque connection strings, where the secret sits under a key that is not itself credential-named."AUTH_TOKEN".contains("_AUTH")is false, which is howauth_tokenslipped through. Now AUTH/TOKEN/PASS/ACCESS_KEY/PRIVATE_KEY/SIGNING_KEY/SESSION.<redacted:default>for an unset credential's built-in default, so the block no longer saysenv: nullandeffective: "<redacted:set>"about the same knob.usernames_are_deliberately_recorded_in_the_clear): provenance, not a credential. Flip that test to flip the policy.The wiring claim, withdrawn and re-earned
"Forgetting the reset is not expressible" was false. Four mutations reintroduced the bug with 776/776 green, and one —
if engine_idx == 0 { begin_experiment(…) }— made the second config publish the first config's entiredeclaredblock andeffectiveknobs. That is strictly worse than pre-#212: the artifact now affirmatively asserts a configuration that never ran, where master merely said nothing.begin_experimentnow returns a move-onlyRecordingthatrun_single_experimentconsumes. Verified:// TODO(#212): re-enable begin_experiment(…)error[E0425]: cannot find value 'recording'if engine_idx == 0 { … }error[E0425]: cannot find value 'recording'error[E0382]: use of moved value: 'recording'begin_experimentstops callingreset()--test-threads=1)To be accurate about the last one: deleting
reset();from insidebegin_experimentstill compiles. It is killed by a test, not by inexpressibility — a pureRecorder-level test now, because the global-state version was masked by residual state from a previously-scheduled test (observe_envis first-write-wins). Only the three call-site mutations are inexpressible.Root cause of the flake, now documented in the code:
declared/invocation/phaseare overwritten each experiment and self-heal, whileenv/effective/overridden/ignoredonly accumulate and are cleared byreset()alone.Guards
src/**, strips#[cfg(test)], probesvar/var_os/vars/vars_os/temp_dir/current_dirplus aliased imports. Thesrc/config.rsexemption is now scoped to five named calls instead of the whole file. Proven with the drift probe.KNOWN_UNRECORDED— 29 rows, 29 distinct literalenv_varnames, asserted both ways (stale row and missing row both fail with their own message). Also pins the count of non-literalenv_varsites: there is exactly one (opensearch::parse_env_secs, a plain variable, noformat!), and it is tier 1 because it callsrecord_effective.the_experiment_loop_begins_a_recording_before_building_the_engine— strips comments first, and is now only a backstop for the ordering; the type does the rest.every_measuring_flag_is_recorded_in_the_invocation—invocationwas the one hand-maintained inventory here and had drifted by five flags includingskip_vector_index. Now CI-asserted against everyArgsboolean, withverbosethe only documented exclusion.Not breaking existing consumers
plot.rsproduction code byte-identical to master; over a 28-summary corpus (15 unversioned, 13 v2) master's chart and this branch's arediff -qidentical.v0_check.shreads six keys out ofresults, which this PR does not touch.metrics_schema_versionstays 2 — it versions the metric definitions, which did not move.engine_params.schema_versionis this block's marker.Corrections to earlier revisions of this body
e99d2b5) 937, branch 1006, +69, 0 master tests removed. (An earlier revision said 934/+62 againstfd92603; eight tests written for the block-list scanner went with it when the scanner was replaced. No test that exists on master was removed.)opensearch.rsis six hunks (new,build_base_url,upload_bulk_batch,RetryPolicy::from_env,parse_env_secs,SearchRetryPolicy::from_env) — the earlier "8 mechanical lines" and the correction's "five hunks" were both wrong.KNOWN_UNRECORDEDis 29 rows, not 30.record_effective— not "14".a3c6146,rustc 1.97.1.snapshot_carries_schema_version_one.number_of_shardsfor elasticsearch.Known, tracked, and NOT fixed here
effective.number_of_shardsstill asserts a value the server does not have. With--skip-upload, an OpenSearch config declaring1publisheseffective.number_of_shards: 1against a server holding 3,overridden: []. The value is pre-existing (#211) but this PR promoted it into a block whose contract forbids exactly that. It is the one surviving instance of the bug that motivated the split — written up on #273, which fixes it with the same read-back.Third review pass — three more blockers, fixed
1.
strip_dsn_secretsleaked. The function added to close the libpq case split on whitespace before parsing, so a legal quoted value published everything after the first space:Replaced with a quote-aware scanner honouring single/double quotes and backslash escapes. The shapes that already worked (bare, leading, tab-separated, double-space, uppercase
PASSWORD=,api_key=,auth_token=) still do, andoptions='-c statement_timeout=5s'is untouched.2. An
overriddenentry on a credential knob recorded nothing. Seeding secret-ness from the siblingkeyfield blanked that field andreasontoo, so the entry rendered as four identical placeholders — destroying exactly what it exists to produce. Same shape cost a{"key":…, "value":…}header list its names. Fixed in the scrubber and the assertion together: the assertion required the blank and panicked on a correctly-scrubbed document.3. The reset guard was not a guard.
begin_clears_every_accumulating_fieldexercisesRecorder::begin, which the mutation does not touch. The only test that ever failed relied on residue other tests left in the global recorder — 9 green / 2 red at default threads, 3/3 green under--test-threads=1, 3/3 red only in isolation. Replaced with a test that drivesbegin_experimentitself and seeds every accumulating field explicitly: withreset()deleted it now fails 7/7 — alone, in-suite, and single-threaded.And this branch had introduced a guard blind spot. A
#[cfg(test)] consthas no brace, sostrip_test_modulesbrace-counted from the next{— the following function's body — deleting ~25 lines from the scan; a rawstd::env::varplanted inparse_update_search_ratiowas missed with 786/786 green.strip_test_modulesis now indentation-based, which also fixes the pre-existing failure on the five files whose string literals contain braces, and strips comments first. Verified: the planted probe is caught.Claims corrected rather than defended
Recordingproves a recording BEGAN, not that it began for this pair. A token minted for a different experiment compiles and passes. The compile errors are real (E0425×2,E0382); the identity claim was not.EXEMPT_CALLSwas documented as asserted-present and was only a skip-list. It now has the assertion its docstring promised — which then caught two real problems of its own: a stale row, and astrip_test_modulesbug that was blanking 2300 lines ofconfig.rs.invocationis bidirectional overboolfields only.--repetitionsand--warmup-secondschange what is measured, are now recorded by hand, and the guard's limit is stated in its own docstring. Widening past booleans is results JSON is content-attributable but not reproducible: no tool SHA, config file identity, dataset checksum or connection_params #274.use std::env::{var},env_var(&…)).The invocation guard earned itself on the first merge
Merging
origin/master(#271) immediately failed it:--allow-partial-corpussuppresses the short-corpus refusal, so it permits a wrong recall under a config name that claims otherwise — precisely the flag an artifact most needs to state. Now recorded.Fourth review pass — a second form of the same leak, fixed
BLOCKER: a password in a URL query string was published verbatim, on the same
--hostflag the "two leaks fixed" table above is about. That table's demo (default:PW@127.0.0.1) is the userinfo form and was genuinely fixed; this is a different form:strip_userinfodeclines (no@), and the whitespace-splitting DSN scanner read the key asmongodb://h.example/db?w— not credential-named — so the tail came through intoinvocation.hostin all three file kinds.Worth understanding before trusting the fix:
?authSource=admin&password=Xwas redacted, but only by accident — the first key containsAUTH, which blanked the whole tail. Any benign leading option restored the leak. The regression test therefore pins the benign-leading-option cases explicitly, so accidental masking cannot make a future regression look green.Rewritten as a real
key=valuescanner — separators whitespace/&/;/?, spaces allowed around=(PostgreSQL documents them as optional), quoted values honouring backslash and SQL doubled-quote escapes. That also closes the eight other shapes the 44-case corpus found:spaces-around-eq,space-before-eq,space-after-eq,sql-doubled-quote,pwd=(a plain marker gap —password/passwd/PGPASSWORDmatched,pwddid not),jdbc:,&-separated, and both ODBC;forms. Non-secret pairs are reproduced byte for byte,options='-c statement_timeout=5s'included.Verification note: this shape cannot be driven end-to-end through the binary — every client rejects
?password=as an unknown option before any file is written, which is exactly why the redactor must not depend on that. It is asserted on the serialized result document, which is byte-identical to whatsave_search_resultswrites, plus throughRecorder::snapshot()directly. The userinfo form is still verified end-to-end on disk.Two more scanner over-strips, each proven with a planted probe
#[cfg(test)] mod filter_guard;(engine/mod.rs:12) has no block, so the scan hunted for the next column-0}and deleted lines 13–60 — the#[cfg(test)] constbug in a different item shape, and it contradicted the comment I had shipped saying non-module items are never deleted.} // end of the config tests) left"} "after comment-stripping, never matched, and ran to EOF — silently disabling guard 1 for the rest of the file.Both fixed and both probes now caught. Added a fail-safe: an unterminated module emits its lines rather than deleting to EOF, since over-scanning is a visible false positive and under-scanning is invisible.
Three false claims, two of them shipped in the repo
--repetitions/--warmup-secondswere not recorded. An earlier edit aborted before writing and the claim survived in the PR body, a commit message, and a code comment that the guard's boolean-only limit leans on. Now actually recorded.invocation.configurations_dirdid not exist — it was cited as the compensating control waiving guard 1 forenv::current_dir(). Now emitted (withresults_dir), and a new test asserts the cited record exists; the old assertion only checked the excused call was still there.begin_experiment's docstring still claimed inexpressibility. Corrected in the code, where it had been contradictingRecording's own docstring in the same file.Also: the
invocationguard now bites both ways (a fabricated key mapping to noArgsfield was green), andEXEMPT_CALLSmatches stripped production text — which immediately caught a stale row of its own,src/config.rs: env::var(k), excusing a read that lives in a#[cfg(test)]helper. Deleted.Fifth review pass — the design change
Seven more credential shapes reached
results/*.json: percent-encoded (%70assword=), Cyrillic homoglyph (pаssword=),AccountKey=,bearer=,Pwd=inside quotes, and any pair nested in a quoted run under a non-secret key. Every one is the same class — tokeniser desync. A single unbalanced quote disabled the redactor for the rest of the string.Three rounds each closed the demonstrated case and left the class open, so the fix is a design change rather than an eighth patch: the default is inverted.
redact_connection_likeREBUILDS a connection-like value from positively recognised components — scheme, host, port, path, and allow-listed parameters whose values are simple — and drops everything else, echoing only the sanitised parameter name:Nothing reaches the artifact unless it was recognised, so a desync loses benign detail instead of publishing a credential. None of the seven shapes had to be predicted — their parameter names simply are not on the allow-list.
optionsis deliberately excluded because its value can itself contain pairs (options='-c password=X'is a live libpq shape).A string containing none of
://,@,=is returned unchanged, which is the overwhelming majority of what gets recorded (localhost,idx:redis-docker-test,FLOAT32, filesystem paths).Live, all nine reported shapes, isolated Redis, artifacts on disk:
The false claim that caused this
My shipped comment said "every client rejects
?password=as an unknown option before a file is written". It is false — redis-rs ignores the unknown param and the run completes with rc=0 and three files. I used it to justify asserting on the in-memory document instead of on disk, and that is exactly what let seven shapes survive a round. The comment is deleted and the corpus test now writes real files and greps the bytes, across search, upload and summary artifacts.strip_test_modules: pinned at lastThree more over-strips (a
//inside a string literal — 93 lines undersrc/— a block-commented fake module, and a block comment after a bodilessmod X;). Six such bugs had been found by hand across four passes and none was pinned. It now has a table-driven test, which immediately exposed a seventh: a///doc comment mentioningexperiments/configurations/*.jsoncontains/*, so stripping block comments first ran to EOF and blanked 2300 lines ofconfig.rs—project_root'senv::current_dir()among them. Caught by the exemption assertion added the previous round.Also fixed
repetitions/warmup_secondswere recorded but unguarded — deleting either left the suite green whileNON_BOOL_INVOCATION_KEYSmade them look covered. Now asserted present.NON_LITERAL_ENV_VAR_SITESdeduped file paths, so a second dynamicenv_var(x)in one file passed. No longer deduped; the named function must exist too.format!/base-var claim that survived 25 lines from its own correction; two different figures for one 11-run measurement; thesrc/config.rswhole-file exemption prose left behind after the list changed;start_gate.rsnamed as containing{literals when it has none; and "104 recorded knob names" where the derived figure is 97.EXEMPT_CALLSis four scoped rows, not five (the previous commit deleted the fifth); andqueries,timeout,search_timeout,upload_start_idx,upload_end_idx,parallels,ef_runtime,update_search_ratioreach no invocation key — the boolean-only limit is stated correctly further down and the summary contradicted it.Merged
origin/master(#278cff7e3a, then #2867ed81eb)Merged, not rebased. Verified rather than trusted, since a clean
git mergeis not evidence — when #271 merged into #236 the merge was clean and the run publishedRecall: 0.0000at exit 0:begin_experiment(experiment.rs:366) still precedescreate_engine(:372), so--hostis still recorded before any client can reject it — the property the redaction has to hold under.index_naming.rscomposed correctly: fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other #278's docs and two tests came in, my recorder wiring (env_or/env_flag) stayed. No rawstd::env::varreintroduced in any of the three engine files fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other #278 touched.corpus_row_countnamesself.config.key, not the hardcodedidxof the fix(skip-flags): --skip-upload must reuse the corpus, not destroy it (#238) #271×VectorSets uses a hardcodedidxkey — it never got the #151-4 per-config namespacing, so concurrent runs clobber each other #236 hazard; fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other #278's newMERGE HAZARDcomment carries no/*that would trip the comment-stripping fix from the previous commit.vectorsets.rs: planted probes at four depths (24, 300, 537, 900) are all caught.invocation.host=<redacted:userinfo>@127.0.0.1. A two-config vectorsets sweep isolatesidx:vs-a/idx:vs-b, both recall 1.000, and their artifacts correctly attribute M=16 vs M=32 — results JSON does not record collection_params or env-derived engine knobs, so tuned runs are indistinguishable #212's capability working on the engine fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other #278 rewrote.One real defect the merge introduced, fixed
#278's two new
index_namingtests resolve through the process-wide recorder without holdingtest_lock, so an observation could land between another test'sbegin_experimentand itssnapshot(). Six consecutive green suites did not disprove it — the window is a few instructions — and "measured green" is exactly the evidence that was wrong about this test before. Closed two ways: those tests now take the lock, and the sweep-bleed assertions now name the keys they seeded instead of asserting total emptiness. Equally strong against the mutation (deletingreset()still caught 5/5: 3/3 alone, in-suite, and--test-threads=1), and immune to a concurrent writer.#286 (
7ed81eb) — geo-radius filters#286 added
engine/geo.rsand rewrote comment blocks across eight engine files, all of which are new input to the source-scanning guards.KNOWN_UNRECORDEDnorEXEMPT_CALLSneeded a row — and since both assert bidirectionally, a missing one would have failed loudly rather than silently.geo.rscarries none of the shapes that broke the scanner before: no/*inside a///doc comment, and its// no radius-style trailing comments sit outside string literals, which the literal-aware stripper handles.std::env::varatgeo.rs:100and:281→ both CAUGHT; at:400, inside the#[cfg(test)] mod teststhat starts at 282 → correctly not scanned. Probes in the rewrittenmongodb_engine.rsandmilvus.rs→ CAUGHT. All six guards pass.begin_experimentstill precedescreate_engine.invocation.host=<redacted:userinfo>@127.0.0.1; two-config vectorsets sweep isolatesidx:vs-a/idx:vs-b, both recall 1.000, artifacts attributing M=16 vs M=32.The one inventory that was not bidirectional
UNCOMPILED— the list of directories the source scanners skip because nothing compiles them — had a skip filter and no assertion, whileKNOWN_UNRECORDEDandEXEMPT_CALLSare both asserted both ways. A stale row there is the worse kind: it silently widens what the scanner ignores. Withsrc/vectorsets/(#297) andsrc/redisearch/(#296) both queued for deletion, it would have rotted unnoticed.Now asserted, both legs proven to bite:
src/vectorsets/away (simulating chore: delete src/vectorsets/, a PyO3 module that has not compiled since Feb 2026 #297) fails with an actionable message naming the row to delete;src/readers/, whichlib.rsdoes declare, fails with "IS declared as a module and therefore compiled". This is the leg that matters: addingmod vectorsets;would otherwise bring 1,400 unscanned lines into the build.Leg 2 could not be shown using
vectorsetsitself, because addingpub mod vectorsets;tolib.rsdoes not compile — independent support for #297's premise that the directory is dead, separate from itsdocker buildevidence.The four
src/config.rsexemption reasons justified themselves by naming those two directories asRedisConfig::from_env's only callers — half-true after #297, false after #296. They now assert the durable property instead (the_exempted_dead_constructor_has_no_live_caller; planting a real call inengine/redis.rsfails it), and say that after both deletions the right move is deleting the constructor rather than exempting it.Sixth review pass — a regression against this branch's own round-2 fix
B1.
redact_connection_likesliced the authority on/?#before looking for@, so a password containing any of those pushed the@out of the slice andadmin:LEAKCANARYfell through to the host slot.b9d4e02had an explicit "malformed-but-dangerous fallback" for exactly this — added in round 2, cited in this body — and the round-5 rewrite dropped it and reproduced the bug verbatim. Six shapes, 3/3 files each, plusREDIS_KEY_PREFIXthrough the same choke point.Fixed by locating
@over the whole remainder before any slicing, and allow-listing the host slot:scrub_authorityemits onlyhostorhost:portwith a numeric port, so a stray:makes the token unrecognised rather than printable.B2.
value[..i+3]was emitted as "the scheme" — actually all text before the first://, sopassword=LEAKCANARY;endpoint=x://hwas republished byte for byte. Schemes are now allow-listed; an unknown-but-well-formed scheme becomes<dropped>://, and a prefix that is not a scheme makes the whole string parse as a parameter blob so the allow-list runs.B3. The inversion had been applied to connection-string values but not to JSON keys.
bearerandAccountKeywere described in this body as "closed by construction" — true as parameter names, wide open as key names. Declared keys are now allow-listed too (DECLARED_KEY_ALLOWLIST, derived from all 47 keys the shipped configs use); numbers and booleans pass regardless of key.Live, real binary, exit 0 — all previously-leaking inputs now 0/3:
The structural fix, which matters more than the three
CONNECTION_SHAPE_CORPUSis append-only, 44 entries, floor asserted. Five rewrites each closed what was just reported and lost something closed earlier, because the corpus only ever grew by the latest finding.@. Round 5'suserinfo-slash-pwhad its canary only after the/, in the half that gets dropped — the two rows naming the shape were the two hiding it.Also
SAFE_PARAM_KEYSandis_secretcontradicted each other in one artifact (?authSource=adminpublished,"authSource": "admin"redacted).authsource/target_session_attrsare dropped rather than exempted, sono_allow_listed_parameter_is_credential_namedcan be enforced — which is what blockssslpassword,passfile,proxyPasswordandauthMechanismPropertiesfrom being added in good faith later.Over-redaction stays clean: 0 of 212 shipped config entries lose a value.
configurations_dir/results_dirare now~-relative instead of publishing/home/<username>/….Seventh pass — the generative campaign, and the end of the hand-rolled parser
A generative campaign (823,920 cases, 116,434 leaking, 5 root causes) found in one pass what six hand-picked rounds did not. Every finding was a desync state of my tokeniser, not a missing rule, so the tokeniser is gone:
scheme://values are parsed by theurlcrate and rebuilt from the structured fields it returns. Userinfo is never one of them, so a@inside a query value cannot promote that value's tail into the host slot (L1 — the worst, because the leaked bytes sat immediately beside<redacted:userinfo>, asserting redaction at the exact point it failed).redis://[admin:LEAK]now fails to parse and is digested rather than passing a bracket check that never looked inside the brackets (L3).<redacted:opaque sha256=… len=…>— identity without disclosure. L2 (Server:h;Pwd:X— no://,@or=at all) and L4 (a quoted fragment resembling an allow-listed key) existed only because the code tried to salvage readable output from arbitrary DSNs.--host default:pw@node, the commonest form) are parsed with a synthetic scheme sourldecides where userinfo ends. Two ambiguity gates — an@delimiting no userinfo, and userinfo followed by an implausible host — both digest, so--host p@sswordno longer publishesssword.Live, real binary, exit 0:
The permanent generator
Fixed seed, 4000 bounded cases, in CI. Ground truth by construction: the generator knows which spans it placed in permitted slots (host, port, path, allow-listed values) and which in forbidden ones (userinfo, non-allow-listed parameter values). Forbidden spans carry a canary; the oracle is that no canary survives — through
redact_connection_likeand through the full snapshot path. Asserted non-vacuous (>2000 cases must actually contain a forbidden span) and mutation-checked: publishing query values verbatim fails it in 2 cases.The reviewer's framing is the right one and is now reflected in the code: the append-only corpus makes a lost fix fail; only a generator makes an unanticipated shape fail. They are orthogonal — the both-sides-of-a-delimiter rule was satisfied by every corpus row and was simply the wrong dimension for L1.
Over-redaction fixed, not accepted
Replica-set seed lists (
mongodb://h1:27017,h2:27017/db) and SQL Serverhost,portsurvive, via an explicit host-list check applied only when every element parses ashost[:digits]. The previous version dropped the whole host list.One new direct dependency:
sha2 = "0.11", matching the version already in the lock. (urlwas added and then removed again — nothing parses now.)Eighth pass — the parser is deleted
The re-run found six more classes in 381k cases, and BLOCKER 1 was the host-list feature added the previous round to recover provenance:
split_authoritysearched for@across the whole remainder, so a@in a query value made the text after it the "authority" — printed verbatim beside a<dropped>claiming redaction, and replacing the real host.That is the pattern rather than an accident. Eight rounds, seven leaked; every mechanism that preserved readable output was a parser and every parser had a desync. So
redact_connection_likeis now two outcomes and no third path:[A-Za-z0-9._+\-/~]only) is the identity;<redacted:opaque sha256=…>.No URL parse, no authority split, no host display, no query-pair echo, no scheme or parameter allow-list. ~200 lines deleted. B1–B3, SF4 and SF5 are unreachable by construction: there is no slice of the input on any path to the artifact.
:is deliberately not in the plain alphabet — no shape test distinguishesadmin:Sup3rSecretfromidx:redis-docker-test, so index names digest too. They are derivable fromparams.experiment; a published password is not derivable from anything.Live, real binary, exit 0 — every reported shape 0/3:
The generator was the reason these survived
p@ssX -> ssXwould have passed it. Now a 5-char contiguous-run check, mutation-checked twice (republishing the tail after@, and leaking a bare 6-char suffix — both caught).Corpus +11 (floor 44 → 55)
Bracketed authority (L3 — demonstrated in round 7 and never appended, despite the append-only rule), the comma/host-list shapes,
Server:h,Pwd:X,Server:h Pwd:X, bareadmin:secret, and?k=v&secretwith no=.The both-sides rule is now a weak per-row check plus a corpus-wide set property. An
@cannot be classified soundly per row — a/inside the password moves the apparent authority boundary — but the corpus must contain rows probing both halves, which is the dimension it actually lacked.Smaller items
sha2 = "0.11"matching the lock ("0.10"had pulled in six crate versions includinggeneric-array, absent from the lock entirely). 8 digest bytes, not 4 — 4 collided 5 times in 200k values.len=dropped: it disclosed the credential's exact character count.The cost, stated plainly
Readable endpoints are gone from artifacts.
REDIS_URI,--host,index_name,key_prefixand every DSN now render as a digest. The digest still answers "did these two runs use the same endpoint?", which is the question #212 is about; the endpoint itself is in the operator's shell history. Recovering structured-but-safe output later is possible, but only behind a parser, and the record here is seven leaks in eight attempts.Ninth pass — the design held; two paths it did not cover
The adversarial campaign on digest-everything came back clean: 961,344 cases through twelve injection points, zero leaks, all six prior classes unreachable or fixed, 1,000,000 distinct DSNs with 0 digest collisions, and over-redaction measured at 1 of 246 shipped-config string values. Two real findings remained.
A third identity path published operator input verbatim.
prosewas derived by matching the JSON key name against["reason","covers"]at any depth — andset_declaredforwards a raw, un-allow-listed config subtree intonote_override. A config placing a{"reason": …}object at one of three CI-pinned paths therefore published it in the clear: the same value<dropped>on the allow-listed path and cleartext two keys away, which is precisely the class this whole series exists to kill.PROSE_KEYSis deleted.reasonis now&'static stron anOverridestruct, held outside the scrubbed tree and spliced in by position after the scrub;coversis a hoisted literal handled the same way. Only a string literal can reach either — a property of the type, which a key name cannot forge.The leak guard printed the credential it caught, under no
cfg, so in release too. It fires only when the redactor has a bug — the exact moment the value must not be written anywhere — and benchmark stderr gets pasted into issues. The value is now digested in the message.Generator and corpus
canary_before_at >= 5floor had slack 9 against an actual 14, so nine@-rows could be silently defanged — includinguserinfo-slash-pw, the row that memorialises round 5's regression, whose defanging survived 850/850. Now asserted per row for everyuserinfo*label; the exact mutation is verified to fail.@,;,&,#,?,%,,,*or!each survived the whole suite; only:and=were covered by anything.assert!(checked > 5000)was tautological — it counted loop iterations. It now counts cases that actually reached the digest path.redact_connection_likeandinvocation.hostonly, so no seed of it could have produced the prose finding.Documented rather than changed
is_plain_tokenprotects nothing punctuation-free — a barehunter2, a UUID,ghp_…,AKIA…are all published, andis_secreton the key is the sole defence for those. And the digest is an unsalted hash, so it is a commitment: a guessed endpoint can be confirmed offline. Both are the intended trade for "same connection ⇒ same token across runs"; neither had been written down.Merged
caefe0f(#297)UNCOMPILEDlost itssrc/vectorsets/row — the bidirectional assertion added when #297 was opened failed on the merge and named the exact string to delete. Planted-probe checks re-run across the merged tree (59 files):experiment.rs,engine/mod.rs,engine/vectorsets.rs,engine/geo.rs,src/config.rs,src/start_gate.rsall CAUGHT.Tenth pass — the sentinels were fiction
<dropped>and<redacted:default>never reached an artifact.snapshotscrubs the whole assembled document, so both were re-processed:<dropped>is not a plain token and digested to the hash of the literal<dropped>, and<redacted:default>under a credential-named key fell through to<redacted:set>.The second is the prose bug inverted. A run with no password set published
— the artifact asserting a credential was supplied when the built-in default was used.
record_effective_rawexisted to prevent exactly that and was defeated, soenv_or's set-vs-default branch was dead as far as the artifact was concerned. Eight comments and a commit message asserted the behaviour; nothing asserted it in a test, which is why it drifted.scrub_valuenow passes an already-emitted sentinel through unchanged. Live, real binary:Both pinned by tests; removing the arm fails them.
The splice's positional coupling between
assembleandsnapshotwas a convention shared by two functions and asserted nowhere — reversing the iteration mis-attributes everyreasonand left all 852 tests green, because no test had two overrides with distinct reasons. Now three, checked pairwise; the.rev()mutation fails it.NITs
@" passed forredis://admin:pw/CANARY@10.0.0.5:6379/0— the canary sitting in the half the buggy code dropped. The rule is now "in the userinfo, before its first internal delimiter"; that mutation dies.MINIMUM_SHAPESwas 55 against an actual 56 — one row deletable silently.mongodb-srvhad userinfo but its label skipped the per-row loop; renamed with a two-part canary.src/vectorsets/as live or its deletion as pending.Eleventh pass — the sentinel pass-through was a bypass
is_sentinelmatched three placeholders exactly and the digest by prefix, and its arm sat above both the credential-key guard and the redactor. Confirmed on disk before fixing:This is the guards-one-notch-weak pattern inside the fix for the previous instance of it — three sentinels compared exactly, the fourth by prefix because its content varies, and nobody varied the suffix.
Fixed structurally rather than by tightening the predicate, because a
Stringthat merely looks like this module's output is the ambiguity that has cost this PR ten rounds. There is no longer anything that asks the question:observe_env,record_effective,note_override,set_declaredno longer scrub;snapshotscrubs once, per member, over values this module has never processed before, with the key in hand;<dropped>comes from a terminal arm rather than an earlier pass whose output the scrub re-read;<redacted:default>is a marker on the key (from_default: BTreeSet), not a magic string in the value.is_sentinel,record_effective_raw,redactandallowlist_declaredare all deleted.A credential-named declared key now reads
<redacted:set>rather than<dropped>— "we removed a secret" and "this knob is not one we read" are different facts, and the leak guard was right to reject the second standing in for the first.Corpus +6 forged-sentinel rows (floor 56 → 62), each a sentinel prefix followed by a canary, because the suffix is the dimension an exact-match test passes and a prefix-match test leaks. Verified: reintroducing the prefix bypass fails
no_connection_string_shape_publishes_its_credentialonforged-digest-prefix.Live: reproducer 0/3; a run with no password set still publishes
<redacted:default>, an unknown declared key reads<dropped>,m/ef_constructionread normally.Twelfth pass — the doc outlived the property
Correction to
3b27709's commit message, which named the wrong guard for its own verification. It claimed reintroducing the prefix bypass failsno_connection_string_shape_publishes_its_credential; it does not — that test callsredact_connection_likedirectly while the bypass arm sat above it inscrub, so the mutation passed 855/855. The only guard that fails isexperiment::…::no_shape_reaches_the_files_on_diskon[forged-digest-prefix]. That matters beyond bookkeeping: a future reader deleting the "redundant" on-disk test would remove the sole coverage.SF1.
snapshot's doc still claimed the assembled-document scrub made "cannot miss a map somebody adds later" true rather than aspirational. Removing the double pass put the manual enumeration back and the claim went stale with it — three members already sat outside it (phase,overridden[].key,ignored[]). No disclosure, since all three carry names, but the doc asserted a guarantee the code no longer provided, which is the exact failure this module exists to prevent.Fixed on both sides rather than just the doc:
phaseis&'static str; the two name members go through the same value scrub; andmember_set_is_pinned_so_a_new_one_must_be_scrubbed_deliberatelyreplaces the guarantee the restructure removed — adding a member is now a build failure that names the fix. Verified with an unscrubbed member.A hole surfaced while proving those three were safe: JSON map keys are never scrubbed anywhere. Correct in general — they are structure, not data — except under
declared, whose keys come from the configuration file, where{"password=SECRET": 1}publishes the secret in a position no value scrub can see. A non-plain-token declared key is now digested; all 47 shipped keys are plain tokens so it costs nothing.SF3.
record_effective's doc still opened "Redaction happens HERE, not in the callers" in a function whose body is a bare insert — contradicted by the same commit that wrote it.NITs.
BEARERadded toSECRET_MARKERS(the corpus carriedbearer=as a credential shape while the marker list did not, soindex_params.bearerread<dropped>rather than<redacted:set>— harmless while dropped, a live leak the moment anyone allow-lists the key).begin()'s "four of the seven fields" is now five of eight withfrom_defaultasserted. Deadlet _ = is_secret(key);removed.Merged
e99d2b5(#298) — a clean merge that did not compilegit reported no conflict, and the tree did not build: #298 added two tests calling
build_search_result_jsonwith master's 11 arguments while this branch's twelfth isengine_params. The two sides touched different lines of the same call. Second time this cycle in exactly that shape, and the reason "a clean merge is not evidence" is worth repeating.Checked rather than assumed: the recorder wiring survived (
begin_experiment:366 still precedescreate_engine:372; #298'sgate_update_attributionlands at :1043 with its call site at :1585); no raw environment read was reintroduced anywhere undersrc/; planted probes CAUGHT in all six files #298 touched.The three new artifact fields do not touch the scrub path, verified:
update_failures/update_unattributedare counters,update_attributionis a closed three-value enum viaas_str() -> &'static str, andupdate_attribution_detailis a per-engine literal at all five call sites. They land inresults, notparams.That prompted the one substantive change: the member-set guard now states its scope. It pins
engine_paramsand nothing else, and extending it artifact-wide is not viable —server_metadataalone contributes ~1,400 server-supplied strings from a full INFO/CONFIG dump. Checked while establishing that:server_metadatacarries its own redaction, emitting a distinct<redacted>forrequirepass,masterauthandtls-*-pass, so that surface is covered by its own mechanism rather than uncovered. Documented, so nobody reads the guard as spanning the file.Deliberately left out (all filed)
vectorsetsfirst (it drops lowercasemsilently and is the only engine file untouched here). CarriesCloses #212.connection_params, summary timestamp, README artifact contract.is_secretover-matching, the JSON type change.src/redisearch/+src/vectorsets/andRedisConfig::from_env;WEAVIATE_USE_GRAPHQLsemantics.SearchParams::knob()— engines also reach typed fields directly, so it would report false ignores.Gates
cargo fmt --checkclean ·cargo clippy --all-targets -- -D warningsclean ·cargo test --lib --bins1006 passed, 0 failed: lib 129,vector_db_benchmark875,generate_dataset2,bench_hdf5/bench_jsonl/bench_npy0 each. Against master7ed81eb(905, measured in an isolated worktree). All 22 test binaries compile.Refs #212
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com