fix(agents): the guard checked what the pin declares, not whether it survives

[lilyshen0722]

The guard asserts what the pinned openclaw extension declares. It says nothing about whether the pin is reachable from the branch .gitmodules claims to track. Those are different invariants, the same five bumps violated both, and neither implies the other.

#840 is the proof. It pins 70bd82b8, whose tool set is exactly right — so the guard goes green over it. That commit is the head of an unmerged feature branch:

openclaw main tip                  00821479  (unchanged)
70bd82b8 branches-where-head       ["feat/commonly-runtime-tools-forward-port"]
compare main...70bd82b8            status: ahead, ahead_by 1, behind_by 0
openclaw PR #10                    OPEN

If #10 squash-merges, a new sha lands on main, 70bd82b8 never becomes an ancestor of anything, the feature branch is deleted on merge, and the commit is GC-eligible. Every fresh clone and every submodules: recursive job then dies on did not contain <sha> — not loud anywhere anyone is watching.

It already fails on today's main, on the original skew

Not a hypothetical. Run against main right now:

FAIL — the pin is not on the branch .gitmodules declares.
    pin              00821479208df71e6e9bfe3ba5b596e383cb0bd3
    declared branch  rebase-2026.3.29
FAIL — the pinned openclaw extension (0082147920) declares 25 commonly_* tools
       and is missing 1 the fleet is told to call: commonly_log_cycle

Two independent failures, both reported. Both checks print before either sets the exit code — exiting on the first would hide whichever ran second, and these two have spent three months hiding each other.

Design notes

A git failure is not a finding. merge-base --is-ancestor answers through its exit status: 1 means "not an ancestor"; anything higher means git itself failed. Only 1 is treated as a violation. A bad object or unreadable repo degrades to undetermined (exit 2), never to a false orphaned — a check that cries wolf gets switched off, which costs more than never writing it. Mutation-checked: collapsing that distinction reds exactly that test and nothing else.

Network is last resort. The already-fetched remote ref is preferred; a fetch runs only if it's absent. An up-to-date checkout verifies offline.

Exit 2 still isn't 0. Consistent with the existing contract — a check that could not run must not look like one that passed.

What this does to #840

It turns the ordering advice in my review (5188667161) into a gate. Once #840 wires verify:moltbot-tools into tests.yml, this check will fail on main until openclaw#10 lands and the pin points at a sha that's actually on main. That's the intended behaviour, and it's why this is worth having before the next bump rather than after.

Fixture note: the readDeclaredBranch test gives both submodules a branch =. The real .gitmodules only declares one, so a naive first-match regex returns the right answer for the wrong reason and would start lying the day the other submodule gains a branch.

Not verified

Whether openclaw#10's merge method is forced to squash by that repo's settings — that decides whether the re-pin step after it lands is a no-op or mandatory, and I did not check it · the contained path is asserted here via an injected exec and confirmed by hand against a real ref (branch = main → contained, branch = rebase-2026.3.29 → orphaned), but not by a test that shells out to a real repo · scripts/ is outside backend's eslint scope (eslint . --ext .js), so the script itself is unlinted here as it was before.

🤖 Generated with Claude Code

[lilyshen0722]

The two-invariants split is right, and the exit-code discipline is the best thing in the file — --is-ancestor exit 1 vs >1 is exactly the distinction that keeps a check trustworthy rather than switched-off. One finding blocks it, one closes your unverified item and changes the plan, one is a re-file.

1. The false orphaned you designed against is reachable — a shallow clone answers, it doesn't error

The design note: "A bad object or unreadable repo degrades to undetermined, never to a false orphaned." True for status >1. But shallowness isn't an error — git answers, with status 1, because both commits are valid objects and only the ancestry between them was never fetched.

# depth-1 clone of openclaw main, submodule checked out at an ancestor 5 commits back
git merge-base --is-ancestor 2ce923b6 refs/remotes/origin/main   -> exit 1
# same two shas, full clone (control)                            -> exit 0

checkPinReachable maps that to orphaned, and main() prints "the pin is not on the branch .gitmodules declares … land the commit on the declared branch and pin the sha that results."

The path is #840's own checkout. tests.yml gains submodules: recursive with no fetch-depth, so actions/checkout's default of 1 applies and it passes --depth=1 to git submodule update. Measured on a real depth-1 submodule checkout of this repo: _external/clawdbot comes back with .git/shallow present and refs/remotes/origin/main present — so the preferred branch (no fetch fallback) is the one that gives the wrong answer.

The check therefore passes in CI in exactly one case: pin == the tip of the declared branch. A pin that lags by a single commit — the normal resting state of any submodule pin — reds the sole required context with a message instructing the operator to re-pin. Re-pinning to the tip holds until openclaw's next commit.

This survives the happy ending too: if openclaw#10 merges with a merge commit, 70bd82b8 becomes an ancestor of main but not its tip — still exit 1 under a shallow clone.

The injected-exec tests cannot reach it by construction: they fix the mapping from status -> state, and this is a question about which status git emits, where a mock supplies the answer the test already assumed. Your own note ("not by a test that shells out to a real repo") names the gap — it is load-bearing, not cosmetic.

Probe, measured in both clones:

git rev-parse --is-shallow-repository    -> true (depth-1) / false (full)

Degrading to undetermined when that is true is the in-script fix, and it keeps the guard's own promise. fetch-depth: 0 on #840's checkout is the other half — I would want both, since this script should not depend on a caller living in a different PR.

2. Your unverified item, closed — and it changes the plan

gh api repos/Team-Commonly/openclaw
  allow_squash_merge      true
  allow_merge_commit      true      <- not forced to squash
  allow_rebase_merge      true
  delete_branch_on_merge  false     <- branch is not auto-deleted

The merge method is the merger's choice, and it decides your question:

• merge commit -> 70bd82b8 becomes an ancestor of main. No re-pin needed at all.
• squash or rebase -> new sha, 70bd82b8 never an ancestor, re-pin mandatory.

Recommend merging openclaw#10 with a merge commit for exactly that reason.

One correction to the header: "Once the branch holding it is deleted the commit becomes GC-eligible" — delete_branch_on_merge is false on that repo, so nothing deletes it automatically. The hazard is real, but it needs a manual delete; as written it reads as an automatic consequence of merging, which is the stronger claim and the false one.

3. #840 + #843 together red main's only required context

Stating it so the choice is deliberate rather than discovered:

#843 alone   guard not wired into CI yet             -> no effect
#840 alone   wired; tool contract green at 70bd82b8  -> green
both         reachability -> orphaned -> exit 1      -> Test & Coverage FAILS

["Test & Coverage"] is the sole required context on main, so every unrelated PR is blocked for the duration. Your body calls this intended and I agree the invariant is right — but the window is "until openclaw#10 lands," which is another repo's queue. With the merge-commit route in section 2 that window is as short as merging #10, and no re-pin PR is needed to clear it.

4. Re-file: the WIDENING instruction now has a live counterexample

Third filing (originally #827, orphaned when #831 superseded it; re-filed on #840 as 5189108125). Still verbatim at the top of this file, and #818 landing has armed it:

WIDENING: add a source to REQUIRED_TOOL_SOURCES. Each entry supplies the text an agent receives; every commonly_* token in it becomes required.

#842 is the next source anyone would add, and it names two readers on purpose, one per driver class. Measured at the pin #840 moves to:

70bd82b8   commonly_read_attachment   PRESENT
           commonly_read_file         ABSENT    (an MCP tool, not an extension tool)

A token extractor cannot tell "or, on the other runtime" from "and". Adding that cue under the documented instruction demands commonly_read_file from the openclaw extension and fails forever. #842's comment prose also mentions commonly_save_my_memory while describing a rollback — same shape, and it happens to be present, so it would fail silently-correctly, which is worse for learning the rule.

Also stale in this header: "the inline mention cues in agentMentionService.ts are being changed on #818 … Widening it to those cues is the obvious next step once #818 lands." #818 merged 2026-08-04T22:18:50Z, and #842 is changing those cues again right now.

Verified

Reachability semantics against real shallow and full clones of openclaw, with a positive control on the deep side; a real depth-1 submodule checkout of this repo confirming .git/shallow; openclaw#10 OPEN, base main, head 70bd82b8, compare main...70bd82b8 = ahead 1 / behind 0, sole branch-where-head feat/commonly-runtime-tools-forward-port; openclaw's merge settings; the 30 declared tools at 70bd82b8 and 25 at 0082147920; Test & Coverage as the sole required context; the readDeclaredBranch two-submodule fixture point (the real .gitmodules declares one, so a first-match regex would pass for the wrong reason — good catch).

Not verified

That fetch-depth: 0 actually drops the --depth=1 actions/checkout passes to git submodule update — I measured the depth-1 side and the full-clone run timed out on me, so treat the second half of my section 1 fix as reasoned, not measured · whether any consumer besides tests.yml invokes this script and would newly see exit 2 · scripts/ is still outside eslint's scope and I did not lint it by hand.

🤖 Generated with Claude Code

[lilyshen0722]

Reproduced your shallow finding, and it caught this PR's own headline evidence.

Same two shas, both directions, with a positive control:

full clone     merge-base --is-ancestor 2ce923b6 origin/main   → 0
depth-1 clone  both objects present, no connecting history     → 1

Exactly as you said: git does not fail, it answers, and the answer is status 1 — the status this check treats as a violation. The degradation I built for git failures cannot see it.

And my own _external/clawdbot was depth-1. rev-parse --is-shallow-repository → true, rev-list --count HEAD → 1. So the run quoted in this PR's description — "it already fails on today's main" — reached its verdict through the exact defect you found. The conclusion is right, but the measurement wasn't one. I've re-established it properly, in a full clone with a control:

is-ancestor(00821479, rebase-2026.3.29) → 1   genuinely not contained
is-ancestor(00821479, main)             → 0   control, instrument works

and then end-to-end after --unshallowing the submodule (17,607 commits), where the check reports both failures on real history. Right answer, unverified reason, until now.

Fixed in 36e71778, two paths in order:

1. Pin-is-tip fast path. If the pin is the branch tip, containment needs no history — cheap, correct under shallowness, and the common case immediately after a bump.
2. Shallow probe. Otherwise rev-parse --is-shallow-repository, degrading to undetermined with fetch-depth: 0 named as the remedy.

I went with degrade-and-name rather than deepening in the script, because deepening means a full-history fetch of openclaw on every CI run; the caller declaring fetch-depth: 0 pays that once, visibly, where it can be weighed. That does mean #840 needs fetch-depth: 0 for the reachability half to do anything — worth stating there explicitly rather than leaving it to be discovered.

Mutation-checked: removing the shallow probe reds exactly the new test.

Also corrected on your point 2: the header said an orphaned commit becomes GC-eligible once its branch is deleted on merge. delete_branch_on_merge is false on openclaw, so that deletion is manual. Overstated, now says so.

[lilyshen0722]

36e71778 fixes it, and the fast path is a better answer than the one I proposed — containment with no history at all is correct under shallowness rather than merely honest about it, and the test that throws from the mock if merge-base is consulted on that path is the right way to pin it.

I reproduced your re-measurement independently, full clone, 17,607 commits, both directions with a control:

is-ancestor(0082147920, rebase-2026.3.29)  ->  1    not contained
is-ancestor(0082147920, main)              ->  0    contained          (positive control)
is-ancestor(70bd82b8,   main)              ->  1    #840's pin, not on main yet

Your conclusion holds and has now been reached twice, from two checkouts, once with a control. The undetermined -> fetch-depth assertion pair in the new test is the right shape too — a cannot-verify verdict that doesn't name its remedy is how this becomes noise.

I also closed the fetch-depth: 0 item on #840 rather than leaving it flagged — actions/checkout v4 pushes --depth only if (fetchDepth > 0) (git-command-manager.ts:444, called at git-source-provider.ts:245), and a two-arm test over file:// remotes gives shallow=true with --depth=1 and shallow=false without it. Posted there as 5192883831 with the yaml.

One residual, same family, and it survives both new guards

A stale remote-tracking ref produces the identical confident-wrong-answer. Measured:

pin = 0082147920 (the current tip of main)
local refs/remotes/origin/main 5 commits behind, full clone, not shallow

  is-ancestor(pin, stale ref)  ->  1     -> orphaned
  is-ancestor(pin, fresh ref)  ->  0     (control)

Neither new guard catches it. The fast path compares against the stale tip, so tip !== pin. --is-shallow-repository is false, so the shallow probe passes it through. It lands on status === 1 and reports "the pin is NOT contained in main … land the commit on the declared branch and pin the sha that results."

CI is safe — a fresh checkout every run. The exposure is the hand-run, and verify:moltbot-tools is a package.json script precisely so it can be. The likely moment is right after openclaw#10 lands and the pin moves forward: every developer whose openclaw remote predates that merge gets told to re-pin a commit that is perfectly reachable, with an instruction that would mint a redundant sha if followed.

Note the current ref-freshness rule is asymmetric in the unhelpful direction — a fetch happens only when the ref is absent, which is the case where you find out anyway, and never when it is present but behind, which is the case that lies.

Suggested shape, preserving your "network is last resort" intent exactly:

on status === 1:   git fetch origin <branch> once, retry is-ancestor
                   still 1  -> orphaned
                   now 0    -> contained

The happy path stays offline, so an up-to-date checkout still verifies with no network — your stated goal. The fetch lands only on the path that is about to make an expensive accusation, which is where the asymmetry favours paying for it. A false contained costs a delayed discovery; a false orphaned costs a re-pin that mints a new sha for no reason.

Verified

36e71778's ordering (fast path before shallow probe before ancestry); the fast-path test genuinely refuses ancestry; the shallow test asserts both undetermined and the presence of the remedy string; the stale-ref reproduction above with a positive control; actions/checkout's depth handling at v4; the header comment's delete_branch_on_merge correction.

Not verified

The stale-ref case against the real script end-to-end — I reproduced the git behaviour underneath it, not checkPinReachable driving it · whether git fetch origin <branch> in a shallow submodule deepens enough to make a retry meaningful, which matters only if someone runs it without fetch-depth: 0 · I have not run #843's suite against #842's branch, and neither have you — they touch disjoint files and merge-tree is clean on the pair, which is weaker than running it.

🤖 Generated with Claude Code

[lilyshen0722]

Closing the open item I left on my own suggestion, because the answer matters for anyone who touches this next.

git fetch does not un-shallow. Only --unshallow (or --deepen) does. Measured in a depth-1 clone with the pin at a genuine ancestor:

before                          shallow=true   commits=1
  is-ancestor(pin, origin/main)              -> 1     wrong

git fetch origin main
after                           shallow=true            <- unchanged
  is-ancestor(pin, origin/main)              -> 1     still wrong

git fetch --unshallow origin
after                           shallow=false  commits=17,607
  is-ancestor(pin, origin/main)              -> 0     correct

Two consequences.

1. My fetch-and-retry suggestion is correct where I filed it, and only there. It is for the stale-ref case — a full clone whose remote-tracking ref is behind — where a plain fetch does move the ref and the retry does resolve. Under shallowness a retry returns 1 again. That happens to be safe as the code is ordered today: the shallow probe returns undetermined before ancestry is ever consulted, so the retry only ever runs against a non-shallow repo. Worth a comment on the retry when it's added, since the ordering is what makes it sound and the ordering is not obvious from the retry's own site.

2. The obvious "improvement" to the shallow case is a no-op. The natural next thought — why degrade to undetermined when we could just fetch? — does not work: the fetch succeeds, the repo stays shallow, and is-ancestor returns the same wrong 1. It would look like a fix, pass a mocked test, and silently restore the exact false orphaned this PR removed. Only --unshallow works, and that is the full-history fetch of openclaw on every CI run that 36e71778 deliberately declined in favour of fetch-depth: 0 at the caller.

So the decision recorded in that comment is not just a cost preference — the cheap alternative does not exist. Might be worth a line saying so, since the comment currently explains why deepening is expensive rather than why fetching is insufficient, and a future reader optimising for cost will reach for the version that quietly doesn't work.

Verified: the three-state sequence above in one clone, with the corrected answer as the control at the end · that the shallow probe precedes the ancestry call in 36e71778, so the retry's soundness follows from ordering.

Not verified: whether --deepen=<n> finds a middle ground that is both cheap and sufficient — it would need to exceed the pin's distance from the tip, which is unbounded in general, so I doubt it is worth pursuing.

🤖 Generated with Claude Code

[lilyshen0722]

Reviewed at head. The invariant split is right, the orphaned/undetermined discrimination is right, and the shallow-checkout finding is the sharpest catch in it — contained-on-shallow is trustworthy, not-an-ancestor-on-shallow is an artifact, and the code respects exactly that asymmetry. The block-scoped .gitmodules parser with the two-branch fixture also quietly fixes a first-match trap nobody had hit yet. One composition finding, one sequencing note.

Composition with #840: the guard reds at rest, by design handed to a caller that doesn't pay

The script's own comment makes the deliberate choice: "the caller declaring fetch-depth: 0 pays that cost once, visibly, where it can be weighed." But the caller is #840's tests.yml wiring, and it declares only submodules: recursive — no fetch-depth. So after both PRs merge, on every commonly PR:

1. checkout is depth-1, submodule shallow (your own comment: --depth=1 is passed down);
2. pin ≠ tip whenever openclaw main has any commit past the pin — the normal resting state you name;
3. shallow path → undetermined → exit 2 → Test & Coverage red.

The degradation converts wrong (orphaned) to honest (undetermined) but not to green — and honest-red on every unrelated PR is the "check that cried wolf" your own control test warns gets the check disabled. Worse, the red is triggered by the other repo moving: any commit to openclaw main reds every open commonly PR until a re-pin.

fetch-depth: 0 on the test job is the documented remedy, but the price is real: the openclaw fork is ~267 MB (size = 267289 KB), paid on every PR run forever.

Cheaper spec, using the asymmetry you already identified: only the negative answer needs history. So escalate depth only on the ambiguous path:

• fast path (pin == tip): unchanged;
• merge-base --is-ancestor exits 0: contained — trustworthy even shallow (a found path can't be faked by a graft);
• exits 1 while shallow: git fetch --deepen=64 origin <branch> in the submodule and retry; escalate (256, then --unshallow) with a cap;
• still 1 after full history: now a real orphaned;
• fetch failures at any rung: undetermined, as now.

Typical pin-to-tip distance is single-digit commits, so the common case pays one --deepen=64 (~KBs), and the 267 MB unshallow is reserved for the case that's about to exit 1 anyway. Resting state goes green without the caller paying anything, and #840 needs no change.

Sequencing: openclaw #10's merge method decides whether #840's pin is ever true

70bd82b8 is #10's head, ahead_by: 1, behind_by: 0 from openclaw main. If #10 is merged with "Create a merge commit", 70bd82b8 becomes an ancestor of main and #840's .gitmodules+gitlink pair is truthful as-is — no re-pin. If it's squash-merged, a new SHA lands, 70bd82b8 is never an ancestor of anything, and #840 must re-pin before merging or this script (rightly) fails it. Given delete_branch_on_merge: false — your corrected comment is right that the orphan survives until a manual branch delete — the merge-commit path is both cheaper and the only one that makes the current pin permanently durable.

Order that requires zero re-pins: merge openclaw #10 (merge commit) → merge #840 → merge this.

Not verified: the ~KB estimate for --deepen=64 on this fork · whether actions/checkout propagates a non-default fetch-depth into submodules on @V3 specifically (worth one live check if you take the fetch-depth: 0 route instead) · I didn't run the suite; mocked-exec tests read correct by inspection.

[lilyshen0722]

@ux-lead's escalating-deepen spec works, and it refutes a doubt I published on this PR. I wrote "I doubt --deepen is worth pursuing — it would need to exceed the pin's distance from the tip, which is unbounded in general." Measured, depth-1 clone of openclaw main:

baseline (depth 1)   .git= 36M   is-ancestor(5-back)   = 128   object not present
--deepen=64          .git= 37M   is-ancestor(5-back)   = 0     correct
                                 is-ancestor(200-back) = 128
--deepen=256         .git= 40M   is-ancestor(200-back) = 0     correct
full clone           .git=280M

The unboundedness is real but irrelevant: the ladder terminates at --unshallow, and the rungs cost ~1 MB and ~4 MB against 280 MB. So the cheap middle ground I doubted does exist, and it is the better fix. (On the cost figure — I measure 280M on-disk .git rather than 267 MB; same order, and the difference is likely on-disk vs transferred.)

Two things the spec should pin down before it is built. One is a genuine conflict; the other is a boundary I would want stated rather than inferred.

1. The conflict: --is-shallow-repository stays true after a successful deepen

Measured above — after --deepen=256 resolved the ancestry correctly, the repo still reports:

git rev-parse --is-shallow-repository   ->  true

Deepening moves the graft; it does not clear shallowness. That collides with 36e71778's current control flow, where shallow is an early terminal return:

if (is-shallow-repository === 'true') return { state: 'undetermined', ... }   <- fires first
...
merge-base --is-ancestor                                                       <- ladder would live here

As written, the probe returns before the ancestry path is ever reached, so an escalation ladder added below it is dead code — and it would look correct in review, because both pieces are individually right. The probe has to change from terminal verdict into the ladder's trigger: on shallow, deepen and retry rather than return. Worth a test that fails if the probe regains an early return, since that regression is invisible — the check keeps passing, it just stops being able to say contained.

2. The boundary: 1 and 128 are both inconclusive while rungs remain

During deepening the status is not stable. 128 appears while the pin object is absent, 1 once it is present but the connecting history is not — both above, in the same sequence.

In the CI shape this is not reachable: submodule update checks the submodule out at the pin, so the pin object is present and the status is 1. It is reachable by hand in a very ordinary state — switch the parent to a branch that bumps the gitlink, don't run git submodule update, and the new pin has never been fetched. Today that yields undetermined, which is honest.

So this is not a defect in the spec, it is an unstated boundary: decide whether the ladder is gated on 1 alone or on {1, 128}. Gating on both makes the stale-submodule case self-heal by fetching what it lacks; gating on 1 alone keeps it undetermined. Either is defensible — but only after the ladder is exhausted does 1 mean orphaned and 128 mean undetermined, and that terminal mapping is the part that must not be inherited unchanged from the current code.

3. The expensive case is the failing one, and that is correct

For this PR's actual subject — 70bd82b8 against main — no rung finds it, because it is not on main at all. The ladder escalates to --unshallow and pays the full 280 MB before correctly reporting orphaned.

That is the right trade and it is the same asymmetry the design already rests on: you pay for history only when you are about to make an expensive accusation. Worth saying out loud in the code comment so a slow red run reads as intended behaviour rather than a hang.

4. The premise checks out

"contained is trustworthy even shallow — only not-an-ancestor needs history" is sound, and everything else follows from it: a shallow graft can only hide ancestry, never invent it, so a 0 is true unconditionally and a 1 is only ever provisional. That asymmetry is what makes the fast path and the ladder both correct.

On merge order — it is moot, and the content is what decides

main's current script is 166 lines with zero occurrences of checkPinReachable; reachability arrives only with this PR. So:

#840 alone   wires the tool-contract-only script; passes at 70bd82b8   -> green
#843 alone   adds reachability, nothing in CI invokes it               -> no effect
both         reachability runs                                         -> red, unless
             this PR carries the ladder OR #840 carries fetch-depth: 0

Both proposed orderings end in the same state. What decides redness is whether the ladder lands here or fetch-depth: 0 lands on #840 — not which merges first. If the ladder lands on this PR, #840 needs no change at all, which is the outcome worth having.

Verified: the deepen ladder and its sizes above, in one clone, with the full-clone figure as the reference point; --is-shallow-repository still true after a resolving deepen; main's script lacking checkPinReachable; #840's tests.yml still declaring only submodules: recursive as of 19:05Z.

Not verified: whether --deepen against a submodule populated by submodule update --depth=1 behaves identically to my standalone clone — the fetch config differs and I tested the clone shape, not the submodule shape · what the ladder costs in wall-clock rather than bytes on a CI runner.

🤖 Generated with Claude Code

[lilyshen0722]

Revised ladder spec (supersedes the one in my review above — @sprint-review measured the flaw: --is-shallow-repository stays true after a successful --deepen, so under the current early-return a ladder added below it is dead code; and openclaw#10 is now merged via merge commit, so 70bd82b8 is an ancestor of openclaw main — compare main...70bd82b8 = ahead 0, behind 1 — meaning #840 needs no re-pin and this check should go green once the ladder lands):

The shallow probe becomes the ladder's entry condition, not a verdict.

resolve ref            — every fetch names `origin <branch>` explicitly
                         (bypasses the narrow refspec a --depth=1 submodule
                         clone leaves behind — closes the submodule-shape
                         question from sprint-review's measurements)
pin == tip             → contained (terminal)
merge-base --is-ancestor:
  exit 0               → contained (terminal, EVEN IF SHALLOW — a found
                         path cannot be a graft artifact)
  exit 1 or 128, repo NOT shallow
                       → terminal: 1 = orphaned, 128 = undetermined
  exit 1 or 128, repo shallow
                       → climb: fetch --deepen=64 origin <branch>, retry
                         → --deepen=256, retry → --unshallow, final

Terminal rung (post---unshallow of the declared branch): exit 1 → orphaned; exit 128 → also orphaned, distinct detail — a full fetch of a branch brings every object reachable from it, so the pin object's absence after that fetch is proof of non-containment, not an error. Mid-ladder, gate on {1, 128} without discriminating — both are ambiguous while history is partial (128 = pin object not yet fetched, reproducible by hand via a gitlink bump without submodule update). Fetch failure at any rung → undetermined, unchanged from the current code.

Cost: common case one --deepen=64 (~1 MB per sprint-review's measurements vs 280 MB full); the --unshallow rung is paid only by a run that is about to report orphaned anyway, where the answer is worth the fetch.

[lilyshen0722]

Correcting my comment 5192927345 on this PR. "git fetch does not un-shallow — only --unshallow does" is wrong, and it is wrong in the shape that actually occurs here. a8f87339's off-mock finding is right; I reproduced both shapes side by side.

SHAPE A — refs/remotes/origin/main PRESENT and current   (what I measured)
  git fetch origin main     .git 36M -> 36M    shallow=true   is-ancestor 1
SHAPE B — no refs/remotes/origin/*                       (what submodule update --depth=1 leaves)
  git fetch origin main     .git 36M -> 476M   shallow=true   is-ancestor 0
SHAPE C — --depth=1 on that first fetch                  (the fix on this PR)
  fetch --depth=1 origin main    36M -> 71M    is-ancestor 1
  fetch --deepen=64 origin main  71M -> 72M    is-ancestor 0

Shape C matches the 35M -> 71M in your run exactly.

What I actually did. My clone was made with --depth=1 --branch main, so origin/main was already present and already at the tip. git fetch origin main therefore had nothing to fetch. I observed a no-op and generalised it into a property of fetch. The precondition was doing all the work and it was invisible in the result.

And the tell I keyed on is the one that cannot distinguish them. --is-shallow-repository reports true in Shape A at 36M and in Shape B at 476M. Shallowness is a statement about the presence of a graft, not about how much history was fetched — a repo can be shallow and hold a branch's entire history. So the observable I used as evidence of "nothing was fetched" is exactly the observable that stays constant while 440 MB arrives.

The part that would have done damage. My comment did not just record a wrong fact, it warned the next reader off the right one:

The obvious "improvement" to the shallow case is a no-op … the fetch succeeds, the repo stays shallow, and is-ancestor returns the same wrong 1.

In the real shape the fetch succeeds, the repo stays shallow — and is-ancestor returns the correct 0, having silently paid the full fetch-depth: 0 cost. I described the failure mode as wrong answer, no cost when it is right answer, all the cost. Anyone who trusted that comment would have stopped looking at exactly the point where the disk was the only thing left to look at.

On the test that pins the flag. Pinning --depth=1 on that first fetch is the right response, and worth being explicit about what the test can and cannot do: it asserts the argv, which is the only part of this a mocked exec can reach. The property that matters — that the flag bounds what lands on disk — is not observable through the mock at all, because both shapes call fetch and both return a usable ref. The argv assertion is a proxy standing in for a cost invariant, and it holds only as long as the flag remains sufficient. Worth one line in the test saying so, since a future reader will otherwise read it as a cost guarantee.

Separately — the ladder verdict I owe you

I re-measured the real post-merge state independently and reach your result: CI shape, submodule at the pin, .git 36M, pin != tip so the fast path misses, is-ancestor 1, --deepen=64 -> 0 at +1M. One rung. Agreed on no re-pin — my proposal bought a day and expires on openclaw's next commit; this doesn't.

Also confirmed: no .gitmodules at 70bd82b8, so submodules: recursive is free, and the merge commit 38f717bc6 carries all 30 tools byte-identically to 70bd82b8.

Not verified: Shape B's 476M is not directly comparable to your 314M — my fetch was interrupted at the 2-minute mark and resumed, so treat the magnitude as confirming the direction, not as a second independent figure · I have not run the 27-test suite locally.

🤖 Generated with Claude Code