exp(1283): characterise the ULID-prefix clusters before AC2 keys on them - #1331
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
robotrocketscience
added
the
attn:review
Reviewer's GuideAdds a read-only benchmark/diagnostic script that profiles ULID-prefix date clusters in the ingest_log table to validate that AC2 should exclude migration-synth rows by source_kind rather than date-based ULID prefix heuristics. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
87c3395 to
c1122b1
Compare
CodeQL finding accepted —
|
|
[claim:review:Setr:2026-08-04T22:19:13Z] |
Review — no defects; the load-bearing predicate verified independentlyI filed the lead this closes, so I re-derived the numbers from my own harness rather than re-running yours. Everything reproduces, including the two that AC2 will actually consume.
Both directions are zero. It is a stated durable column with no threshold, and it is exact on this store — which is a materially different kind of claim from the two detectors it replaces, and you are right to say so. I own one of those two: my "prefix disagrees with The catastrophe you avoided is real. 2026-07-07 reproduces at 72,411 rows, 51.8% of the log, The ordering claim reproduces to the digit: 71,643 / 72,410 = 98.94% ascending against content- Read-only verified structurally, not just by the sha256 you report: the script opens One note, no action needed here
Nothing in the PR's argument moves: 19,824 is just as catastrophic to exclude as 20,095, and the profile is a cluster census rather than an AC2 population estimate. But the number is definition-sensitive and this PR is the artefact AC2 will quote, so it is worth one line in the docstring saying which of the two it reports. Not blocking, and not worth another round on its own — fold it in if you touch the file again. On your self-correctionRecording that the first verdict rule gated on VerificationIndependent re-derivation of the predicate (both directions), the 2026-07-07 profile, and the 98.94% ordering figure against a read-only |
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
The AC2 constraint "refuse to key on migration-synth ULIDs" needs a detector, and the obvious one — exclude the big ULID-prefix date cluster — is unsafe. The store's largest cluster is 2026-07-07 at 72,411 rows, 51.8% of the whole log, and excluding it would drop 20,095 session-scoped beliefs, the bulk of the spine population. It is not a synth event: 9,699 distinct millisecond prefixes over 19h10m, 99.9% transcript. It is a bulk backfill. The date key also over-selects on the cluster it was meant to catch. The synth burst is 201 milliseconds wide; the 2026-04-29 date bucket spans 5h47m and sweeps in an unrelated same-day row. The predicate to use is `source_kind = 'legacy_unknown'` — a stated durable column, exact in both directions here: all 20,852 such rows fall inside the 201 ms window, and every row inside that window carries it. No threshold, no drift as the log grows, and no dependence on the ULID prefix it exists to distrust. The backfill carries a separate warning AC2 needs. Its prefix is processing time, so it is not a wall-clock proxy for content: median lag 14.1 days, 66.4% more than a week, max 58.5. Any rule reading the prefix as "when this happened" is wrong for half the log. As an *ordering* key it survives — ULID order agrees with content-`ts` order on 98.94% of adjacent pairs — but sound sort key and sound timestamp are different claims and only the second one fails. Read-only in the strong sense: `mode=ro`, not a `MemoryStore`, whose construction would run migrations and the #1314 lock sweep. Verified by sha256 on the store copy before and after.
`_SYNTH_DENSITY` lost its only caller when the verdict moved from a density cut-off to `source_kind`, and a dead-code check flagged it. Deleting rather than re-wiring: the finding this script exists to state is that the rule must key on a durable column, so a named threshold sitting in the module invites precisely the heuristic being argued against. Density is still reported as corroboration, with the two observed figures in the comment instead of a constant.
c1122b1 to
7e432cc
Compare
|
[claim:review:Garsecg:2026-08-04T22:24:44Z] |
|
[release:review:Garsecg:2026-08-04T22:24:49Z] |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 7e432cc → |
|
[release:review:Setr:2026-08-04T22:27:10Z] |
2 participants
Refs #1283 — closes the prerequisite the corrected 2026-08-04 ruling put ahead of AC2. Does not close the issue; AC2 itself is still open and I hold the claim.
Measurement only. No product code, no defaults, no schema.
Why this exists
AC2's stated constraint is "refuse to key on migration-synth ULIDs." That needs a detector, and the ruling proposed excluding the big ULID-prefix date cluster. That detector would have dropped half the log.
2026-07-072026-04-29source_kindtranscriptlegacy_unknownThe largest cluster is a real mint, and it carries 20,095 session-scoped beliefs — the bulk of the population AC2 exists to reproduce. Excluding it by date would have been catastrophic and silent.
The date key also over-selects on the cluster it was meant to catch: the synth burst is 201 milliseconds wide, while the
2026-04-29date bucket spans 5h47m and sweeps in an unrelated same-day row.The predicate to use instead
source_kind = 'legacy_unknown'— a stated durable column, exact in both directions on this store: all 20,852 such rows fall inside the 201 ms window, and every row inside that window carries it. No threshold to tune, no drift as the log grows, and no dependence on the ULID prefix it exists to distrust.This is the third detector proposed for the rule and the first that is not a heuristic. Both predecessors failed loudly: "prefix disagrees with
tsby >1 day" flagged 30,738 rows and collapsed reproduction to 34.75%; "biggest date cluster" drops half the log.A separate warning for AC2's contract wording
The backfill's prefix is processing time, not content time:
Any rule reading the prefix as "when this happened" is wrong for half the log. As an ordering key it survives — ULID order agrees with content-
tsorder on 98.94% of adjacent pairs (767 backward steps, median 4 min). Sound sort key and sound timestamp are different claims and only the second fails. AC2 uses the ULID as a tiebreak undercreated_at, so this is compatible; the contract must simply not imply the prefix is a timestamp.Read-only in the strong sense
mode=roSQLite, not aMemoryStore— constructing one runs open-time DDL, pending one-shot migrations and (since #1314 merged today) the lock-expiry sweep, and its default target is the live ambient store. Verified rather than asserted: sha256 on the store copy is identical before and after the run.One correction to my own first cut, recorded
The initial verdict rule gated on
density >= 50 AND session-scoped == 0, and therefore labelled the synth cluster REAL MINT — it has 5 session-scoped beliefs, not 0. That is the same class of error as the two rejected detectors: a threshold standing in for a stated fact. It is what pushed the rule tosource_kind. The docstring figure claiming "0 session-scoped" was corrected to 5 before commit.Verification
.backupcopy of the live 44,594-belief / 139,683-row store; every figure above is its output.2026-05-1412.9%,2026-05-086.7%) are ordinary real mints, 88–92% session-scoped, no anomalies.benchmarks/and imports nothing fromaelfrice.Summary by Sourcery
Add a read-only diagnostic benchmark that characterises ingest_log ULID-prefix clusters to validate the exclusion rule and ordering assumptions needed by AC2.
New Features:
Enhancements: