Skip to content

Prevent :@@= loads from (falsely) indicating to the server they have missing commit ancestry #260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nishu-builder merged 1 commit into from
Sep 18, 2026
Merged

Prevent :@@= loads from (falsely) indicating to the server they have missing commit ancestry #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions design/flake-inputs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,20 @@ Rules:
- `dir=` selects a subtree within the repo.

Why commit-then-path (not a bare subtree hash): **GitHub only serves *commits***
(`allowReachableSHA1InWant`), so `git fetch --depth 1 <url> <rev>` is the
(`allowReachableSHA1InWant`), so `git fetch <url> <rev>` is the
fetchable granularity; `dir` selects within it. The commit is also the pin.

Resolution (stage 4, client, eval time): `git fetch --depth 1 <url> <rev>` →
Resolution (stage 4, client, eval time): `git fetch <url> <rev>` →
verify rev → peel → descend `dir` → ingest closure → **the oid is the arg
entry**, byte-for-byte as if from a local `:@=`. URL/rev are fetch coordinates
and provenance (trace), never part of the key.

Fetch with --depth=1 into a temporary bare repository, then pack only the
root tree and its trees/blobs into the caller's object store. The shallow commit
and boundary file stay in the temporary repository. This bounds the transfer
to the snapshot without making later CAOS uploads shallow. Full source-history
imports use the server import endpoint instead.

Parser safety (verified against current code): the token is whitespace-free;
`split_once('=')` takes the **first** `=`, so `?rev=…&dir=…` survives in the
value; the `:`-type split runs on the **key** only, so `://` is untouched; and
Expand All @@ -172,7 +178,7 @@ Client-side only, so it landed with **no redeploy**; 34/34 with a new
the time a worker sees a `:@@=` arg it is an ordinary oid. `tests/remote-ref/check.sh` asserts the refusal from inside a
worker.
- **Pin, fetch, then select** — the order is the content-addressing argument.
`git fetch --depth 1 <url> <rev>` (the granularity a host will serve), peel the
`git fetch <url> <rev>` (the granularity a host will serve), peel the
commit to its tree, descend `dir=`. A `path:` skips the fetch and ingests a
live local directory, exactly as `:@=` does.
- **Wired in three places, each behaving like its `:@=` sibling** — that is the
Expand All @@ -182,26 +188,21 @@ Client-side only, so it landed with **no redeploy**; 34/34 with a new
evaluator (evaluated if it carries a `.caos-expr`, raw if not — the
worker-vs-data rule, now factored out as `eval_if_evaluable` and shared).
- **A rev is a pin, so re-resolving is free.** `fetch_git_ref` returns early when
the commit is already local; the test proves it by DELETING the source repo
and resolving again.
the commit is already local or a previous fetch retained its tree under
refs/caos/locator-trees/<commit>. That ref is both a commit-to-tree cache
and a GC root. The test deletes the source repository and resolves again.

### ⚠️ `git fetch` and a partial ALTERNATE object store

`git fetch` runs a connectivity check — `rev-list --not --all --alternate-refs`
— which walks the tips of every **alternate** object store as well. A repo whose
alternate holds a deliberate SUBSET then fails with `missing blob object <x>`
naming an object that has nothing to do with the fetch, blamed on the fetch.
The test harness creates exactly that shape (`dev/cli-test/run-test.sh` points the
client at `/tmp/seed-git/objects`, "exactly what this test declared"), so
`tests/remote-ref` failed the first time it ran — on a fetch that succeeds in
any ordinary repo. The difference is the alternate, not the command.

Fixed with `-c core.alternateRefsCommand=true` (a command that prints nothing)
on **this fetch only**. Alternate tips are an EXCLUSION set, so dropping them
makes the check verify our closure and only ours — stricter, not looser — which
is safe precisely because a `--depth 1` fetch is self-contained. Do NOT copy it
to `fetch_object_negotiated`: a chat commit's history may legitimately live in
an alternate, and there the tips are doing real work.
The old test harness created this shape; it no longer gives client repositories
an alternate. Locator fetching now uses a fresh temporary repository, so it
cannot accidentally walk the caller's alternate tips. Other fetches must decide
whether alternate history is part of the closure they are completing.


---

Expand Down
162 changes: 68 additions & 94 deletions rust/crates/caos/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,8 @@ pub trait Transport {
Ok(None)
}

/// Fetch commit `rev` (and its tree) from the FOREIGN repo at `url` into
/// local storage, for a `:@@=` arg — or `Ok(None)` if this transport has no
/// repo to fetch into.
/// Fetch the tree of a pinned commit into local storage and return its OID,
/// or None if this transport has no repository to fetch into.
///
/// `None` is the default because the worker's [`HttpTransport`] speaks only
/// `/object`: there is no working repo and no `caos` remote to negotiate
Expand All @@ -449,7 +448,7 @@ pub trait Transport {
/// different URLs get different keys for identical content. By the time a
/// worker sees this arg it is an ordinary oid, resolved before the request
/// existed (design/flake-inputs.md).
fn fetch_git_ref(&self, _url: &str, _rev: &str) -> Result<Option<()>, String> {
fn fetch_git_ref(&self, _url: &str, _rev: &str) -> Result<Option<gix::ObjectId>, String> {
Ok(None)
}

Expand Down Expand Up @@ -894,68 +893,69 @@ impl Transport for GitTransport {
parse_oid(out.trim()).map(Some)
}

fn fetch_git_ref(&self, url: &str, rev: &str) -> Result<Option<()>, String> {
// ALREADY HAVE IT? A rev is a full commit sha, so local presence is
// authoritative — the bytes cannot have changed under the name. This is
// the whole memo: re-evaluating the same locator costs nothing and
// touches no network.
//
// Two honest limits. (1) It tests the COMMIT, and the fetch below pulls
// that commit's whole tree, so a hit we ourselves put there is complete
// — but a commit already present for some OTHER reason (a partial or
// filtered clone) can have a missing tree, and that surfaces as a
// missing-object error while descending, not as a re-fetch. (2) The
// objects are unreferenced by design (see `--no-tags` below), so a
// `git gc` may prune them and the next resolve pays the fetch again;
// that is a cache miss, never a wrong answer.
if self.have_commit(rev) {
return Ok(Some(()));
}
// `--depth 1`: we want ONE commit's tree, not a repo's history. That is
// also the granularity a host will serve — GitHub answers a sha in a
// want only when it is reachable (`uploadpack.allowReachableSHA1InWant`),
// which is exactly why the locator pins a COMMIT and selects within it
// with `dir=` rather than naming a subtree hash (design/flake-inputs.md).
//
// `--no-tags` and `--no-write-fetch-head` keep a foreign repo from
// leaving anything behind in the caller's: nothing is referenced, so the
// objects are ordinary unreachable ones the next `git gc` may drop —
// re-fetching them is a cache miss, never a correctness problem.
//
// `core.alternateRefsCommand=true` (a command that prints nothing) is
// load-bearing, and cost a debugging session. git's post-fetch
// connectivity check runs `rev-list --not --all --alternate-refs`, so it
// walks the tips of every ALTERNATE object store as well — and a repo
// whose alternate holds a deliberate SUBSET (the test harness points the
// client at exactly that, tests/lib/run-test.sh) then dies with
// `missing blob object <x>` naming an object that has nothing to do with
// the fetch, blamed on the fetch. Dropping those tips makes the check
// verify OUR closure and only ours, which is stricter, not looser.
//
// Narrow to this fetch on purpose: alternate tips are an exclusion set,
// so suppressing them is only safe when the fetched closure stands
// alone. It does here (`--depth 1` — a commit and its tree), and it does
// NOT for `fetch_object_negotiated`, where a chat commit's history may
// legitimately live in an alternate.
self.run_git(&[
"-c",
"core.alternateRefsCommand=true",
"-c",
"fetch.negotiationAlgorithm=noop",
"fetch",
"--quiet",
"--no-tags",
"--no-write-fetch-head",
"--depth",
"1",
url,
rev,
])
.map_err(|e| format!("fetching {rev} from {url}: {e}"))?;
if !self.have_commit(rev) {
return Err(format!("{url} did not deliver commit {rev}"));
fn fetch_git_ref(&self, url: &str, rev: &str) -> Result<Option<gix::ObjectId>, String> {
let tree_ref = format!("refs/caos/locator-trees/{rev}");
for name in [rev, tree_ref.as_str()] {
if let Ok(tree) = self.git_capture(
&["rev-parse", "--verify", &format!("{name}^{{tree}}")],
None,
) {
return parse_oid(tree.trim()).map(Some);
}
}
Ok(Some(()))
// A locator needs a snapshot, not history. Keep the shallow boundary
// and incomplete commit in a disposable repo; copy only its tree closure.
let scratch = scratch_dir()?;
let result = (|| {
let git = |args: &[&str]| git_capture_in(args, None, &scratch);
git(&["init", "--bare", "--quiet"])?;
git(&[
"fetch",
"--quiet",
"--depth=1",
"--no-tags",
"--no-write-fetch-head",
"--",
url,
rev,
])?;
let tree = git(&[
"rev-parse",
"--verify",
&format!("{rev}^{{commit}}^{{tree}}"),
])?;
let tree = tree.trim();
let pack = scratch.join("tree.pack");
let mut writer = std::process::Command::new("git")
.current_dir(&scratch)
.args(["pack-objects", "--stdout", "--revs"])
.stdin(std::process::Stdio::piped())
.stdout(std::fs::File::create(&pack).map_err(|e| e.to_string())?)
.spawn()
.map_err(|e| e.to_string())?;
let written = writeln!(writer.stdin.take().unwrap(), "{tree}");
let status = writer.wait().map_err(|e| e.to_string())?;
written.map_err(|e| e.to_string())?;
if !status.success() {
return Err("packing locator tree failed".into());
}
let status = std::process::Command::new("git")
.current_dir(&self.work_dir)
.args(["index-pack", "--stdin"])
.stdin(std::fs::File::open(&pack).map_err(|e| e.to_string())?)
.stdout(std::process::Stdio::null())
.status()
.map_err(|e| e.to_string())?;
if !status.success() {
return Err("storing locator tree failed".into());
}
// The ref maps the pin to its tree across processes and retains the
// complete tree for GC. No foreign commit enters the caller's store.
self.run_git(&["update-ref", &tree_ref, tree])?;
parse_oid(tree).map(Some)
})();
let _ = std::fs::remove_dir_all(scratch);
result.map_err(|e| format!("fetching {rev} from {url}: {e}"))
}

fn server_url(&self) -> Result<String, String> {
Expand Down Expand Up @@ -1422,19 +1422,6 @@ impl GitTransport {
Ok(())
}

/// Does the local repo already hold `rev` as a COMMIT?
///
/// A SUBPROCESS on purpose, rather than the cached `self.repo`: a `git fetch`
/// lands a new pack that the already-open odb handle will not see, and this
/// is called on both sides of exactly that fetch. (`get_object` solves the
/// same problem by re-opening; here one `cat-file` is simpler and is already
/// a process boundary.) `^{commit}` also does the type check for us — a rev
/// that names a tree or a tag object is not a pin we can peel.
fn have_commit(&self, rev: &str) -> bool {
self.git_capture(&["cat-file", "-e", &format!("{rev}^{{commit}}")], None)
.is_ok()
}

/// Run a network Git command in this transport's bound working tree.
fn run_git(&self, args: &[&str]) -> Result<(), String> {
self.git_capture(args, None).map(|_| ())
Expand Down Expand Up @@ -3317,23 +3304,10 @@ fn resolve_remote_arg(
let url = git_ref.fetch_url();
t.fetch_git_ref(&url, rev)?.ok_or_else(|| {
format!(
"cannot fetch {value:?}: resolving a remote ref is a CLIENT capability, \
so a `:@@=` arg must already be an oid by the time a worker sees it"
"cannot fetch {value:?}: resolving a remote ref is a CLIENT capability; \\
a remote locator must already be an oid by the time a worker sees it"
)
})?;
// Peel the pinned commit to its tree. Everything below is ordinary
// object reading, against objects the fetch just made local.
let (kind, content) = t.get_object(rev)?;
if kind != "commit" {
return Err(format!(
"git ref {value:?}: {rev} is a {kind}, not a commit"
));
}
// Bound rather than returned inline: the parsed commit BORROWS `content`,
// and a block's tail temporaries outlive its locals.
let commit = gix::objs::CommitRef::from_bytes(&content, gix::hash::Kind::Sha1)
.map_err(|e| format!("git ref {value:?}: malformed commit {rev}: {e}"))?;
commit.tree()
})?
};

let dir = git_ref.dir.as_deref().unwrap_or("");
Expand Down
10 changes: 10 additions & 0 deletions tests/remote-ref/worker.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ SRC=/tmp/remote-ref-src
rm -rf "$SRC"; mkdir -p "$SRC/payload" "$SRC/tool"
git init -q "$SRC"
git -C "$SRC" config uploadpack.allowReachableSHA1InWant true
git -C "$SRC" -c user.email=test@caos -c user.name=caos commit --allow-empty -qm remote-ref-parent
PARENT=$(git -C "$SRC" rev-parse HEAD)
# The same BYTES as ./payload, reached the other way. Two files and a subtree, so
# the run below covers a subtree, a blob and the whole root in one container.
printf 'from another repo\n' > "$SRC/payload/note.txt"
Expand Down Expand Up @@ -69,6 +71,14 @@ bound=$(git cat-file -p "$node" | awk '$4=="args"{print $3}')
|| fail "the bound arg is not the source repo's own tree:
$(git cat-file -p "$bound")"
echo " ok: the ArgTree carries the foreign repo's oid, and no URL" >&2
[ "$(git rev-parse --is-shallow-repository)" = false ] \
|| fail "resolving a locator made the caller shallow"
if git cat-file -e "$PARENT^{commit}" 2>/dev/null; then
fail "the locator fetched unnecessary ancestor history"
fi
if git cat-file -e "$SHA^{commit}" 2>/dev/null; then
fail "the locator copied a shallow commit into the caller"
fi

args=(--base:@=DEEP-DEPS/bash --worker1:@=test/check.sh
"--tree:@@=$REPO?rev=$SHA&dir=payload"
Expand Down
Loading