chore: record the fork's conventions where agents will read them - #53
Merged
Merged
Conversation
Nothing in this repo said it was a fork. `AGENTS.md` and `README.md` are upstream's and describe the project, so an agent arriving here saw an ordinary liquid-cache checkout and had to learn the rest by breaking something: branching off a stale local `main`, cutting an upstream PR from our `main` and carrying the whole patch stack into it, or re-reporting the reclaim race as new. `CLAUDE.md` is a file upstream does not have, which is the point. A convention recorded in an upstream-owned file conflicts on every sync, and this fork has already paid enough for history problems. Adding a file upstream will never create cannot conflict. It states what differs here and nothing else: that `main` is upstream plus our patches, that upstream PRs branch from upstream under `upstream/<topic>`, that the toolchain is `cargo +1.96.0`, that the shuttle feature has to be run with a `shuttle_` filter or ~49 unrelated tests fail by design, that `dev-tools` needs a generated asset, and that the disk reclaim race is known and open. The guard workflow enforces the branch rule rather than trusting it. `main` is upstream plus our patches, so a correct `upstream/**` branch cannot have `main` in its ancestry; one `merge-base --is-ancestor` tests exactly that, and tests the real property rather than a proxy like which files are present. It runs on push, so the author learns before a PR exists. Its failure output explains rather than rejects: what was detected, why it matters — the reviewer sees a diff far larger than the change, mixed with work never meant for them — and the commands to redo the branch from upstream and to check before pushing. Both paths were run against real branches: this one fails, and `upstream/keep-column-free-conjuncts` passes.
anoop-narang
requested review from
eddietejeda
and removed request for
a team
September 24, 2026 13:26
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — Blocking Issues: (1) .github/workflows/upstream-branch-guard.yml:28: The ancestry test checks only the current tip of main. A branch cut from main before a later merge passes the guard, even though the branch carries fork patches. (2) .github/workflows/upstream-branch-guard.yml:71: The "must print nothing" self-check excludes every commit in our main. The check never shows fork patches, and it always prints the author's own commits.
Action Required: Test whether git merge-base HEAD origin/main is an ancestor of upstream's main. Fetch upstream in the job to run the test. Replace the self-check command with the same merge-base test, or with git log FETCH_HEAD..HEAD plus correct wording.
CI checks were still queued or in progress when this review started. This review makes no claim about CI results.
…s tip Review found the guard passes the case it exists for. It asked `git merge-base --is-ancestor main HEAD`, which tests whether a branch contains `main`'s *current* tip. `main` moves with every merge, so a branch cut from `main` last week has today's tip nowhere in its history and the check reported "OK". Verified: a branch at `9298d97`, our own main a few hours ago, passed while carrying 19 fork commits. Every case I tested cut from the tip as it stood, which is the one arrangement where the wrong test gives the right answer. Test the merge base instead. A branch cut from upstream shares only upstream history with `main`, so that point is a commit upstream has; a branch cut from `main` shares a fork commit, and upstream never contains one. Run against six refs — upstream's tip, a branch off upstream, our main's tip, our main as of two merges ago, a commit mid fork history, and this branch — it is right on all six. The self-check printed in the failure was wrong in the same spirit. `git log origin/main..HEAD ^FETCH_HEAD` excludes everything reachable from `main`, which is every fork patch, so it lists the author's own commits on a clean branch and a contaminated one alike. It can never be empty, and the instruction said it must be. Now `git log FETCH_HEAD..HEAD`, described as what it is: a list that should contain only your own commits. The failure also now names the branch point, the upstream tip, how many fork commits are carried and the first five of them, rather than asserting that some are. `CLAUDE.md` said `git fetch fork`, which is one local remote naming and not what a fresh clone has. It says `origin` and notes the alternative, and carries the corrected self-check.
`FETCH_HEAD` holds whatever the last fetch wrote. The bullet above tells the author to fetch their own remote, and after that `FETCH_HEAD` is our `main` — so `git log FETCH_HEAD..HEAD` would exclude every fork patch and show a contaminated branch as clean. The workflow's copy already repeated the fetch; this one now does too.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
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
1 participant
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.
Nothing in this repo says it is a fork.
AGENTS.mdandREADME.mdare upstream's and describe the project itself, so an agent arriving here sees an ordinary liquid-cache checkout and learns the rest by breaking something.Two files, neither of which upstream has — which is the point. A convention written into an upstream-owned file conflicts on every sync; a file upstream will never create cannot.
CLAUDE.md— read automatically, states only what differs here:mainis upstream0033b15plus our patchesmain, andgit fetchfirst — a stale localmaingives no signalupstream/<topic>, never frommaincargo +1.96.0; the default fails resolution on the vortex depsshuttle_filter, or ~49 unrelated tests fail by designdev-toolsneeds a generatedassets/tailwind.css, and habitually passing--exclude dev-toolshides real failures#[cfg(feature = "shuttle")]test can be deleted without moving the default total.github/workflows/upstream-branch-guard.yml— enforces the branch rule instead of trusting it.mainis upstream plus our patches, so a correctupstream/**branch cannot havemainin its ancestry. Onemerge-base --is-ancestortests that directly, rather than a proxy like which files are present. Runs on push, so the author finds out before a PR exists.The failure output explains rather than rejects — what was detected, why it matters (the reviewer gets a diff far larger than the change, mixed with work never meant for them), and the commands to redo the branch and to self-check before pushing.
Both paths were executed against real branches, not assumed: