DEV Community: Tan Tran

Peeking at your A/B test turns 5% false positives into 25%, and what mSPRT costs you

Tan Tran — Sun, 01 Nov 2026 15:46:32 +0000

Most teams I have worked with check the experiment dashboard every day. It is just what you do when a campaign is running and someone asks how it is going. The t-test on that dashboard, though, was designed for one look at a pre-agreed sample size, and the daily habit quietly breaks it.

This post measures how badly, shows the fix I used (a mixture sequential probability ratio test, mSPRT), and is explicit about what the fix costs. All numbers come from a seeded script in a public repo, linked at the end.

Why checking daily breaks the t-test

A 5% significance level means: if there is no effect and you run the test once, at the planned sample size, you will wrongly reject 5% of the time. That guarantee is about one decision.

If you look every day and stop the first time p < 0.05, you have given the random walk of your estimate many chances to wander across the line. Under no true effect, the difference between arms drifts up and down as data arrives. The chance that at least one look crosses the line grows with the number of looks, and the nominal 5% describes none of it.

Worse, the failure is silent. The p-value on the screen is computed correctly for a single look. It is the stopping rule that is wrong.

The simulation

I wanted a number, not an argument. The setup, from the repo's sequential report:

Bernoulli outcomes, 10% base rate.
20 looks of 500 users per arm each, so a 10,000-per-arm horizon.
alpha 0.05, 10,000 seeded runs per scenario.
For the A/B scenario, the true effect is the fixed test's 80%-power minimum detectable effect (MDE), +1.19 points.

Three rules are scored on exactly the same simulated data:

Rule	False-positive rate (A/A)	Power (A/B)	Median n per arm at rejection (A/B)
Naive t-test at every look	0.248	0.874	3,000
mSPRT (always valid)	0.0098	0.465	6,500
One test at the horizon	0.0499	0.787	10,000

The single final test lands at 4.99%, as it should. Peeking at every look with the same t-test rejects 24.8% of A/A experiments. A quarter of the experiments where nothing happened would have been reported as wins. With --large (100 looks of 100 users instead of 20 of 500, same horizon, 20,000 runs), the naive rate rises to 0.377 while mSPRT stays at 0.014.

How mSPRT works

The idea is to make the stopping rule part of the statistic, so the guarantee holds however often you look.

Start with the likelihood ratio for "the true difference is exactly delta" against "the difference is 0". You do not know delta, so instead of picking one, average the ratio over a prior on delta. I use a normal prior, delta ~ N(0, tau^2). That average is the mixture likelihood ratio, and for a difference in means it has a closed form. With d the estimated difference and V its estimated variance (s_t^2/n_t + s_c^2/n_c):

Lambda = sqrt(V / (V + tau^2)) * exp(tau^2 * d^2 / (2 V (V + tau^2)))

Two properties do the work:

Under "no effect", Lambda is a non-negative martingale starting at 1. Informally, its expected next value is always its current value, so it has no upward drift.
For such a process, Ville's inequality says the chance it ever reaches 1/alpha is at most alpha.

So the rule "stop and reject the first time Lambda >= 1/alpha" has type-I error at most alpha, however many times you inspect, whenever you stop looking, and with no correction for the number of looks.

One honest caveat: the martingale property is exact for normal data with known variance. The code plugs in sample variances, the usual large-sample approximation, so the guarantee is approximate at small n. The simulation is where I check that it holds in practice.

The code is short. This is the repo's implementation, trimmed to the arithmetic:

import numpy as np

def msprt_log_lr(estimate, variance, tau):
    tau2 = tau * tau
    d = np.asarray(estimate, dtype=np.float64)
    v = np.asarray(variance, dtype=np.float64)
    log_lr = 0.5 * np.log(v / (v + tau2)) + tau2 * d**2 / (2.0 * v * (v + tau2))
    return np.where(v > 0.0, log_lr, np.nan)

def msprt_reject(estimate, variance, tau, *, alpha=0.05):
    log_lr = msprt_log_lr(estimate, variance, tau)
    return np.nan_to_num(log_lr, nan=-np.inf) >= np.log(1.0 / alpha)

The always-valid p-value is the running minimum of 1/Lambda over the looks, so it can only go down, which is exactly what you want from something you glance at every day.

Setting the mixing scale

Validity holds for any tau > 0. The choice only moves power. The mixture concentrates its power near |delta| ~ tau, so a tau that is too small makes early stopping slow, and one that is too large spends evidence on implausibly big effects.

I set tau before the experiment to the effect the test is designed to detect: the MDE. In the simulation that is the fixed test's 80%-power MDE of +1.19 points, fixed before any data is seen. That is a design decision, not something to tune after watching the dashboard. Tuning tau on the data would quietly reintroduce the problem.

I also checked the closed form against numerical integration of the mixture in a test (test_log_lr_equals_numerical_mixture), and a second test confirms that mSPRT keeps type-I error under the null while naive peeking exceeds three times alpha.

The price

Here the table above turns uncomfortable. At the same 10,000-per-arm cap, mSPRT has power 0.465 against 0.787 for the fixed test. You cannot have both "look any time" and "same power at the same sample size".

The repo measures what it takes to get the power back (uplift-sequential-report, the price-of-peeking section):

A larger cap. mSPRT reaches 80% power once the cap is raised to 18,000 per arm, 1.8x the fixed horizon. Its false-positive rate at that cap is 0.016, so the bigger cap costs no validity.
More units on average, not fewer. mSPRT stops as soon as the evidence is strong, but under the alternative it still uses 11,190 per arm on average: about 12% more units than the fixed test.
Near the cap under the null. When there is no effect it rarely stops, so it runs to 17,848 per arm on average (1.78x), close to the cap.

The trade is plain: budget for a larger maximum sample and somewhat more units on average, and in exchange you may look whenever you like without inflating false positives.

The second bullet is the one people do not expect. With this design, centred on the MDE, "sequential tests save sample" does not hold on average. I only claim this design; I have not simulated other effect sizes.

A detour: honest CUPED

The same repo covers CUPED, the other thing teams reach for to get answers faster. It replaces the outcome y with y - (X - mean(X)) @ theta, where X is a pre-experiment covariate and theta its regression coefficient. The estimate stays unbiased and the variance falls by roughly the share of outcome variance the covariate explains.

The word that matters is "predicts". On the Hillstrom e-mail RCT (64,000 customers), the pre-period covariates are past-year spend, recency, product lines, newbie and channel. They explain 2.6% of visit variance, and under 0.3% for conversion and spend. CUPED reduced the variance of the visit estimate by 2.6%. A 10% relative visit lift needs 13,217 users per arm without CUPED and 12,873 with it. The report says this plainly instead of promising a bigger number.

The method is not the problem. In the seeded simulation, where I control how well the covariate predicts the outcome, 1,000 experiments with 2,000 users per arm give:

Correlation rho	Variance ratio CUPED / raw (theory 1 - rho^2)	CI coverage
0.0	0.998 (1.00)	0.948
0.5	0.796 (0.75)	0.939
0.7	0.512 (0.51)	0.957
0.9	0.183 (0.19)	0.944

At rho = 0.7 the variance ratio is 0.512, and the same power needs about half the users. Coverage stays near 95% throughout, so CUPED is not buying precision with validity.

When it helps: a metric with a stable history, such as weekly spend for returning customers, where last period strongly predicts this period. When it does not: the two-week visits of a one-off e-mail, where the pre-period barely says anything about who clicks. Check the correlation before you promise anyone a variance reduction.

The same habit, one level up

The uplift half of the repo has a decision problem of the same shape: four learners (S, T, X and a transformed-outcome baseline) whose test-set Qini coefficients are within noise of each other. Picking the highest point estimate is peeking in a different form: you let the data choose the winner after the fact.

So I wrote the champion rule into code (uplift/champion.py). Test the best learner against each rival with a paired bootstrap, Holm-correct the p-values, and if it is not a clear winner, take the simplest tied learner in a fixed order (S, then T, then X, then transformed outcome). On the full 13,979,592-row Criteo run with 1,000 bootstrap draws, the transformed outcome had the highest Qini point estimate, but its paired differences against S, T and X all covered 0 (Holm-adjusted p 0.35, 0.65, 0.35), so the rule registered the S-learner.

I should be precise about the timing. The rule was committed before that 1,000-draw run, but after a first full-data run had already shown the learners within noise. So it was not written blind. What it does give is a rule that is fixed in code, with its reasoning, and applied the same way to every future run, instead of a judgement call each time. The principle is the same as fixing tau in advance: decide how you will decide, ideally before the data can influence you.

Takeaways

If anyone looks at a running experiment, assume they will act on it. A fixed-horizon t-test checked daily produced 24.8% false positives in my simulation, and 37.7% with 100 looks.
If peeking is unavoidable, use a method built for it. mSPRT held 0.98% in the same setting and keeps its guarantee for any inspection schedule.
Plan the cost up front. At the same cap, power was 0.465 against 0.787. Getting to 80% power needed about 1.8x the cap, and the average sample under the alternative was about 12% larger than the fixed test's.
Fix tau at the MDE before the data arrives. It does not affect validity, but choosing it after looking would defeat the point.
Measure before you promise CUPED. Variance falls by about the share of outcome variance the covariate explains: 2.6% on Hillstrom visits, 49% when rho is 0.7.
Write down the decision rule before you see the results, whether that is a stopping rule or a model-selection rule.

The code, the seeded scripts that regenerate every number above, and the evidence table mapping each README claim to its source are at https://github.com/tantn89/uplift-experimentation.

Proving exactly-once by killing the stream mid-backlog

Tan Tran — Sun, 01 Nov 2026 15:46:28 +0000

Most pipelines I have reviewed claim "exactly-once" somewhere: a README line, an architecture slide, a job title. Very few have a test that fails if the claim stops being true.

Exactly-once is a property of a failure, not of a happy path. To test it you have to cause the failure at an awkward moment, restart, and count. That is more work than a unit test, so it rarely gets written.

So I wrote a script that kills the streaming job with SIGKILL mid-backlog, restarts it, and checks every layer. It runs in CI on every push.

The pipeline

The repo is realtime-lakehouse. The flow is:

Wikimedia recentchange events arrive over server-sent events.
A Python producer validates each event against a JSON schema and publishes it to Redpanda. Events that break the contract go to a dead-letter topic.
Spark Structured Streaming lands the raw records in an Iceberg bronze table, with the Kafka partition and offset kept as columns.
A second stream parses and deduplicates into silver.edits. A third builds editor sessions.
dbt on DuckDB reads the Iceberg tables and builds four gold marts. Dagster schedules dbt, runs four asset checks and compacts tables.

Each hop has its own guarantee, and they are not all the same.

Where duplicates and gaps come from

Replay after a crash. A micro-batch reads a range of Kafka offsets, writes files, and commits. If the process dies after the write but before the checkpoint is updated, the restart reads the same range again. A sink that simply appends will write those rows twice.

Offsets against commits. The opposite failure is a gap. If the checkpoint says "batch 12 is done" but the table commit never landed, the restart skips rows that were never stored. The checkpoint and the table commit have to agree, or the sink has to be able to tell that it is ahead of the checkpoint.

A non-idempotent sink and an at-least-once source. My producer is at-least-once on purpose. It saves its resume position (Last-Event-ID) only after Kafka acknowledges everything before it. After a producer crash, a few events are sent again. Those carry the same Wikimedia event id, but they are new Kafka messages with new offsets.

So there are two different problems. Replayed batches must not double-write. Resent events must not double-count. I handle them in two different places.

The mechanism the repo uses

For Kafka to bronze, I rely on Spark's checkpoint plus the Iceberg sink. The docstring in streaming_queries.py says:

each query has its own checkpoint, and the Iceberg sink commits a micro-batch atomically and records the batch id, so a batch replayed after a crash is not committed twice.

The bronze query is small:

def start_bronze(spark, bootstrap):
    kafka = (
        spark.readStream.format("kafka")
        .option("kafka.bootstrap.servers", bootstrap)
        .option("subscribe", TOPIC)
        .option("startingOffsets", "earliest")
        .option("maxOffsetsPerTrigger", os.environ.get("LAKEHOUSE_MAX_OFFSETS", "100000"))
        .load()
    )
    return (
        kafka_to_bronze(kafka).writeStream.format("iceberg").outputMode("append")
        .queryName("bronze").trigger(processingTime=TRIGGER)
        .option("checkpointLocation", f"{CHECKPOINTS}/bronze")
        .toTable(f"lake.{BRONZE}")
    )

There is no foreachBatch, no hand-written MERGE and no dedup in bronze. The guarantee comes from how the two halves fit together:

Before a micro-batch runs, Spark writes its offset range to the checkpoint's offset log. It marks the batch done in the commit log only after the sink's commit returns. A restart re-runs any batch that has offsets but no commit marker, with the same batch id and the same offsets. That closes gaps.
Iceberg's streaming sink stores the query id and the batch id in the summary of the snapshot it commits. Before committing, it looks up the last batch id that query committed and skips the commit if this one is not newer. So a batch that reached the table before the crash, but not the commit log, is not written twice. That closes duplicates.

I did not write that logic; I use it, and the test is what tells me whether I am using it correctly. It also depends on the checkpoint surviving: the query id lives there, so a lost or reset checkpoint starts a new query that Iceberg cannot match to the old commits.

For silver, the problem is the resent events, and a checkpoint cannot help with those because they are genuinely different Kafka messages. Silver deduplicates on the Wikimedia event id:

def deduplicate_edits(edits_df):
    return edits_df.withWatermark("event_time", WATERMARK).dropDuplicatesWithinWatermark(
        ["event_id"])

WATERMARK is 10 minutes: the newest event time seen, minus 10 minutes. The dedup state for an event id is kept until the watermark has moved 10 minutes past that event's time, which keeps state bounded. A resent copy carries the same event time as the original, so by the time its state is gone the copy is already behind the watermark and is dropped as late rather than written twice. That is my reading of how Spark orders the two steps; the crash test only resends within the window.

The cost is lateness. The watermark drops every event more than 10 minutes behind, duplicate or not, from silver only. Bronze keeps them, and ops.stream_progress counts them in rows_dropped_by_watermark.

So the claim has two parts, and I keep them separate:

Kafka to bronze: every Kafka offset appears once.
Bronze to silver: every unique on-time edit appears once, within the watermark.

The crash test

scripts/prove_exactly_once.py runs in its own compose project with no published ports. An overlay file sets small batches (LAKEHOUSE_MAX_OFFSETS 500, a 2 second trigger) so the backlog takes many micro-batches and the kill lands in the middle of it, not after everything is committed.

The scenario, in order:

Publish 50 loops of a 400-event recorded sample, each with unique ids.
Resend the 400 events of loop 0, as a resume would.
Publish 25 contract violations.
Wait 12 seconds, then SIGKILL the stream container, and read the committed counts.
Restart the same container on the same checkpoint volume.
Publish 100 events stamped 30 minutes in the past.
Run the verifier.

Trimmed to the part that matters:

publish("base", "--loops", str(LOOPS))
publish("duplicates")
publish("poison", "--count", str(POISON))

time.sleep(12)  # let a few micro-batches commit, then crash mid-backlog
compose("kill", "-s", "SIGKILL", "stream")
at_crash = marker_json(compose("run", "--rm", "--no-deps", "spark", "python", "-m",
    "wiki_lakehouse.reliability.verify_exactly_once", "--counts-only",
    capture=True), "COUNTS ")
total = LOOPS * sample_size + sample_size  # every event reaches bronze, duplicates too
if not 0 < at_crash["bronze_rows"] < total:
    sys.exit("inconclusive: the crash did not land in the middle of the backlog")

compose("start", "stream")  # same container and checkpoint volume
wait_for_log("stream", "started queries", after=1)
publish("late", "--count", str(LATE), "--minutes", "30")
compose("run", "--rm", "--no-deps", "spark", "python", "-m",
        "wiki_lakehouse.reliability.verify_exactly_once",
        "--expect-kafka", str(total + LATE),
        "--expect-silver", str(LOOPS * silver_per_loop),
        "--expect-dlq", str(POISON))

One guard in there is worth the effort. After the kill, the script reads the committed counts and refuses to continue unless bronze has more than zero rows and fewer than the total. If the crash lands before anything is written, or after everything is written, the test reports "inconclusive" instead of passing. A crash test that can pass without crashing anything is worse than none.

The verifier reads the same Iceberg tables with Spark and compares them with Kafka itself:

checks = {
    "kafka_has_expected_messages": observed["kafka_main_messages"] == args.expect_kafka,
    "bronze_has_every_offset_once": observed["bronze_rows"] == observed["kafka_main_messages"]
    == observed["bronze_distinct_offsets"],
    "silver_has_every_event_once": observed["silver_rows"] == args.expect_silver
    == observed["silver_distinct_events"],
    "late_events_dropped": observed["late_events_in_silver"] == 0,
    "contract_violations_in_dlq": observed["kafka_dlq_messages"] == args.expect_dlq,
}

Look at what is compared. Duplicates are bronze_rows against count(DISTINCT kafka_partition, kafka_offset). Gaps are bronze_rows against the number of messages Kafka actually holds, read with an endingOffsets=latest batch read. A gap shows up as fewer rows than offsets; a duplicate shows up as more rows than distinct offsets.

The expectations come from the scenario, not from numbers I typed in. Kafka should hold 50 x 400 + 400 resent + 100 late = 20,500 messages. The script counts 196 edit or new events in the recording, so silver should hold 50 x 196 = 9,800.

The latest CI run on main killed the stream with 4,484 of the 20,400 events published before the crash committed to bronze. After the restart the verifier saw 20,500 Kafka messages, 20,500 bronze rows and 20,500 distinct offsets, 9,800 silver rows and 9,800 distinct event ids, no late events in silver and 25 messages in the dead-letter topic. Bronze had taken 43 commits to get there. Every check passed: 0 duplicates and 0 gaps.

The numbers around it

The same repo has two measured runs.

Freshness, from the benchmark (benchmarks/results.md, 45 seconds per phase, 10 second trigger, local run). Freshness is edit time to Iceberg commit:

Publish rate	In silver	p95
100 ev/s	2,156	20.2 s
1,000 ev/s	21,952	20.2 s
5,000 ev/s	110,152	20.6 s
9,596 ev/s (target was 15,000)	330,652	21.0 s

Every phase had all expected edit and new events in silver. The producer, one Python process, topped out near 10k events/s, so this does not saturate the pipeline.

The AWS run (benchmarks/aws-run.md): one t3.xlarge in ap-southeast-2 with S3 and Glue, live Wikimedia stream, 2 h 10 min. 868,073 recentchange events ingested, 0 duplicate Kafka offsets in bronze and 0 duplicate events in silver, freshness p95 20.0 s measured as processed_at - event_time in Athena, and all 42 Dagster runs green. The estimated cost was about $0.52. This run predates the change that keeps only edit and new in silver, so its counts cover every event type.

The AWS run checked for duplicates over real traffic, but nothing crashed during it. It answers a different question from the crash proof.

Lessons

Make the test refuse to pass vacuously. Any fault-injection test needs a check that the fault happened where you meant it to.

Verify against the source of truth, not against the pipeline. The verifier counts offsets in Kafka and compares them with bronze. If I had compared bronze with my own earlier count, a bug in my counting would have hidden a bug in the pipeline.

Say which hop each guarantee belongs to. "The pipeline is exactly-once" is not a statement I can defend. "Each Kafka offset lands once in bronze" and "each on-time event id lands once in silver" are, and they have different mechanisms.

A reader can lie too. While building the verifier, a polling reader stalled at 5,479 rows. Iceberg's caching catalog expires an entry after it was last accessed, so a verifier that polled every few seconds kept seeing the same snapshot. Readers of tables that other processes write now disable the cache. A wrong verifier would have looked like a pipeline bug.

What the test does not prove

I would rather list these than have a reviewer find them.

It kills the Spark stream, not the producer. The producer's resume path is imitated by resending loop 0, not exercised by a real producer crash.
One kill, one crash point. The kill lands somewhere in the backlog, 12 seconds in. I did not kill during the commit itself, repeat the kill several times, or kill Kafka, the catalog or the object store.
Dedup is bounded by the watermark. The test does not resend an event after its dedup state has expired. Late events are dropped by design and counted, and replay from bronze is manual.
It assumes the checkpoint survives. The restart reuses the same checkpoint volume. Losing the checkpoint is a different failure, and this test does not cover it.
Small data, small batches. The test deliberately uses 500-offset batches and a 2 second trigger to force a mid-backlog crash. The default is 100,000 offsets per trigger.
It proves one scenario. CI shows that this scenario passes on every push. It is not a proof that no scenario fails.
Gold is not part of this test. A dbt reconciliation test and a Dagster duplicate check cover gold and silver separately.

Closing

The point of the exercise was not the number. It was to turn a sentence in a README into an assertion that can fail, and to write down where it stops being true.

To run it: python scripts/prove_exactly_once.py. It needs Docker and nothing else.

Code, tests, benchmarks and the AWS write-up: https://github.com/tantn89/realtime-lakehouse

An alias that walked past my SQL guard: red-teaming an MCP server for warehouse access

Tan Tran — Sun, 01 Nov 2026 15:46:25 +0000

I wrote an MCP server that lets an AI agent query a warehouse. A second model reviewed it and found a way past my SQL guard with a one-line alias. This post covers the design, the bug, the fix, and the other gaps a red team found.

The repo is warehouse-mcp. It serves DuckDB, Postgres and BigQuery.

The problem

Agents are good at writing SQL and bad at knowing what they should not read. Giving one a warehouse connection raises three worries:

It reads personal data it should not see.
It writes, or reads files and system state, because a prompt told it to.
It runs a query that scans terabytes and sends you the bill.

You cannot fix this by asking the model to behave. A prompt-injected agent does what the injected text says. The controls have to sit outside the model.

The design

Three decisions shaped the server.

Parse, do not pattern-match. The guard parses agent SQL with sqlglot in the backend's dialect. It accepts one statement, a SELECT only, over declared datasets only, with a function allowlist and a capped LIMIT. A function the parser does not model is denied by default, because dialect-specific functions are where pg_read_file and sleeps hide.

Pseudonymize at the source. The guard rewrites every dataset reference into a policy subquery. That subquery projects only declared columns, replaces personal columns with a salted hash, and applies the role's row filter. All of it runs inside the warehouse.

def pseudonym(column: str, salt: str) -> exp.Expr:
    """A stable, salted pseudonym: equal inputs stay equal (joins, counts), names never leave."""
    template = (f"CASE WHEN {column} IS NULL THEN NULL ELSE CONCAT('pii_', "
                f"SUBSTRING(MD5(CONCAT('{salt}', CAST({column} AS TEXT))), 1, 12)) END")
    return sqlglot.parse_one(template, read="duckdb")

count(DISTINCT editor) stays exact and joins on the pseudonym work. A probe like WHERE editor = 'Alice' matches nothing, because the comparison runs against the hash. Masking the output after lineage analysis can be bypassed with aliases and expressions. Here the raw column is not reachable.

The guard is not the boundary. Each backend is locked down on its own:

DuckDB: read-only, no file access.
Postgres: read-only transaction plus grants.
BigQuery: dry run plus a byte cap.

On Postgres, the tests send writes straight to the backend with the guard out of the way and expect the engine to refuse them: first the read-only transaction, then, with that bypassed too, the grants. A parser gap alone should not be a breach. That goal mattered, because I then found a parser gap.

The bug

An independent review by a second model read the guard and found something my own suite had missed. Here is the shape of it, taken from a regression test:

WITH x AS (SELECT wiki FROM edits)
SELECT editor FROM edits_raw x, x

edits_raw is a physical table, not a declared dataset.

The guard has to tell two kinds of table node apart. A reference to a real dataset must be rewritten into the policy subquery. A reference to a CTE must be left alone, because the CTE body was already guarded.

My version decided this by looking up the table's alias in the scope's sources. This is the line that the fix (commit d1369b3) removed:

if isinstance(scope.sources.get(table.alias_or_name), Scope):
    cte_refs.add(id(table))

alias_or_name returns the alias when there is one. In the query above, the physical table edits_raw is aliased x, and the scope has a CTE named x. The lookup says "this is a CTE" and the node is skipped. No allowlist check, no policy rewrite. The physical table reaches the warehouse as written.

The fix commit states the impact: raw PII, undeclared columns, and rows outside the role filter.

I had fixed a neighbouring case before. In WITH t AS (SELECT * FROM t) the inner t is the real table, so a global name match would wave it through. I moved to scope-based resolution and thought that was the end of it. I had swapped a name match for an alias match, and the alias match has the same flaw.

The fix

A table refers to a CTE only if its name is a CTE visible in its own scope. If the name does not match but the alias does, the query is refused.

for scope in scopes:
    visible = {name.lower() for name in scope.cte_sources}
    for table in scope.tables:
        if table.args.get("db") or table.args.get("catalog"):
            continue  # qualified: never a CTE; rejected later
        if table.name.lower() in visible:
            cte_refs.add(id(table))
        elif table.alias and table.alias.lower() in visible:
            raise QueryRejected(f"Alias {table.alias!r} hides the CTE of the same name; "
                                "use a different alias.")

Refusing is simpler than resolving. No legitimate query needs to alias a real table to the name of a CTE in scope, so the guard removes the ambiguity instead of interpreting it.

The commit adds three cases that must be rejected:

"WITH x AS (SELECT wiki FROM edits) SELECT editor FROM edits_raw x, x",
"WITH x AS (SELECT wiki FROM edits) SELECT x.editor FROM edits x JOIN x y ON true",
"WITH x AS (SELECT wiki FROM edits) SELECT * FROM salaries AS x",

It also adds a positive test: a CTE referenced by its own name still works, including a self-join like FROM a AS b JOIN a. A guard that rejects everything is easy to write and useless to ship.

What else the red team caught

The adversarial suite is separate from the review. It runs attempts through the real service on a snapshot of about 100k Wikipedia edits. Each attempt must be refused, or come back with no raw editor names and no rows outside the role's filter. It found three gaps outside the SQL guard.

ReDoS in redaction. The email regex used unbounded repeats and backtracked quadratically on a long string with no @. The README's estimate: a 1 MB string from repeat('x', 1000000) would have held the server for about 12 minutes. Redaction ran after the warehouse timeout, so no timeout covered it. The fix bounds the repeats to RFC 5321 lengths and cuts cells before redaction:

_EMAIL = re.compile(r"[A-Za-z0-9._%+-]{1,64}@[A-Za-z0-9.-]{1,255}\.[A-Za-z]{2,24}")

The regression test asserts that a 40,000 character cell finishes in under half a second.

A single huge row skipped the result cap. The cap trimmed rows, so a result of one oversized row went out whole. The fix halves the longest text cells until the row fits, and the test checks that a 5,000 character cell comes back under a 1,000 character cap.

Pseudonyms relinked through titles. On Wikipedia, editing your own User: page produces a row where the editor column holds a pseudonym and the page title holds the real name. That pairs the two. In the snapshot where I found it, this re-identified 5.7% of editors. The fix adds conditional PII, pii_when: "namespace IN (2, 3)", so user-page titles are pseudonymized, and edit summaries are pseudonymized too. The regression test takes ground truth from a cleared role and checks that no pseudonymized row still carries the name in its title.

The independent review also found a leak the suite had missed.

Engine errors quoted data. An error like Could not convert string '<value>' skipped result redaction, because errors take a different path than rows. It reached both the agent and the audit log. The fix scrubs quoted values, redacts what is left, and caps the length:

_QUOTED = re.compile(r"'(?:[^']|'')*'|\"(?:[^\"]|\"\")*\"")

def scrub_error(message: str, max_chars: int = 300) -> str:
    text = _QUOTED.sub("'…'", message)
    text, _ = redact(text, phones=True)
    return text if len(text) <= max_chars else text[:max_chars] + "…"

The agent still sees the error kind and where it happened, which is enough to fix a query. My earlier red-team test had asserted that a cast error showed the pseudonym, and I had treated that as acceptable. The test now asserts that no value comes back, for the analyst role and for the cleared role.

The numbers

104 red-team cases per engine, 0 leaks. 103 are attacks; one is a positive control that checks the cleared role still sees real names, so the PII checks are not passing vacuously. The attacks cover exfiltration, row-filter escapes, PII probes, file and system access, statement smuggling and resource abuse. Locally the suite runs on DuckDB, and on Postgres when a DSN is set. CI sets the DSN, so every push runs all 208 cases.
Parity. The same 15 queries return identical results on DuckDB, Postgres and BigQuery. The BigQuery run was live and is not part of CI, because it needs credentials. docs/bigquery-run.md has the details, including a query that would scan 4.65 GB. The server refused it before billing: "above the 1.00 GB cap".
Agent eval. A Claude Haiku 5.5 agent answered 26 cases, 3 trials each, graded against ground-truth SQL with no LLM judge. Result: 78/78 correct, including 6 governance cases, at $0.0016 per question, with p95 latency of 6.3 s. All 10 rejected tool calls were self-corrected from the error message.

The BigQuery run also found one more bug. It was a correctness problem, not a leak: time literals were cast to DATETIME against TIMESTAMP columns.

Lessons

Layers matter when one fails. The engines were locked down separately, and the Postgres tests prove it with the guard bypassed. I do not want to overstate that. The alias bypass still skipped the row filter and the pseudonymization, because those live in the guard. Defense in depth limited what a parser gap could do. It did not make the gap harmless.

Every finding gets a regression test. Each fix above came with a test that fails without it. Those regressions are part of the 104 cases above, so a fix cannot quietly come undone.

Independent review finds what your own suite cannot. I wrote the guard and the red-team suite with the same assumptions. The reviewer did not share them, and it found the alias gap and the error leak. A passing suite tells you nothing about attempts you never thought of.

Prefer refusing over interpreting. When a construct is ambiguous and never needed, reject it. The alias rule is a few lines and removes a whole class of confusion.

To try it, the repo has a demo that needs no keys: uvx warehouse-mcp --demo.

Repo: github.com/tantn89/warehouse-mcp
PyPI: pypi.org/project/warehouse-mcp

Questions or findings are welcome on Telegram: @tanngoctr.