From f748548f22cab0b60d8338100b4e59fa130081e8 Mon Sep 17 00:00:00 2001 From: Nishad <133812901+nishu-builder@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:50:30 +0000 Subject: [PATCH] Keep snapshot fetches from making client repositories shallow --- design/flake-inputs.md | 33 ++++---- rust/crates/caos/src/lib.rs | 162 +++++++++++++++--------------------- tests/remote-ref/worker.sh | 10 +++ 3 files changed, 95 insertions(+), 110 deletions(-) diff --git a/design/flake-inputs.md b/design/flake-inputs.md index cdbb1f303..ba969e396 100644 --- a/design/flake-inputs.md +++ b/design/flake-inputs.md @@ -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 ` is the +(`allowReachableSHA1InWant`), so `git fetch ` is the fetchable granularity; `dir` selects within it. The commit is also the pin. -Resolution (stage 4, client, eval time): `git fetch --depth 1 ` → +Resolution (stage 4, client, eval time): `git fetch ` → 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 @@ -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 ` (the granularity a host will serve), peel the + `git fetch ` (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 @@ -182,8 +188,9 @@ 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/. 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 @@ -191,17 +198,11 @@ Client-side only, so it landed with **no redeploy**; 34/34 with a new — 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 ` 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. + --- diff --git a/rust/crates/caos/src/lib.rs b/rust/crates/caos/src/lib.rs index 48ae2754e..0a0c3f630 100644 --- a/rust/crates/caos/src/lib.rs +++ b/rust/crates/caos/src/lib.rs @@ -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 @@ -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, String> { + fn fetch_git_ref(&self, _url: &str, _rev: &str) -> Result, String> { Ok(None) } @@ -894,68 +893,69 @@ impl Transport for GitTransport { parse_oid(out.trim()).map(Some) } - fn fetch_git_ref(&self, url: &str, rev: &str) -> Result, 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 ` 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, 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 { @@ -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(|_| ()) @@ -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(""); diff --git a/tests/remote-ref/worker.sh b/tests/remote-ref/worker.sh index f8f47ab2c..27f23095e 100644 --- a/tests/remote-ref/worker.sh +++ b/tests/remote-ref/worker.sh @@ -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" @@ -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"