Skip to content

fix: keep shallow sync finalization blob-filtered - #2854

Merged
steipete merged 2 commits into
mainfrom
steipete/sync-finalize-child-stall
Oct 15, 2026
Merged

steipete merged 2 commits into
mainfrom
steipete/sync-finalize-child-stall

Conversation

@steipete

@steipete steipete commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Finalization of an incremental sync can download full historical file contents after workflow setup or a command replaces Git metadata with a shallow checkout. Cancelling the local SSH client can leave that fetch alive remotely; owner release then correctly refuses the witnessed child, but previously described the known refusal as ambiguous state.

Keep both coherence and hydration fetches blob-filtered, including unshallow and fallback fetches. Full commit ancestry, advertised-branch membership, tree/index checks, claim fencing, and live-child takeover refusal remain intact. Distinguish a confirmed CHILD release refusal from transport ambiguity and explain waiting or stopping the lease before retrying on a fresh lease. There is no force path, protocol change, new configuration, or secret requirement.

This is separate from #2841, which addresses local object enumeration bounds before transfer.

Validation:

  • The new regression failed on the original code in coherence-only, hydration-only, and combined modes. It passes with this change and verifies omitted historical blobs, complete commit ancestry, and successful explicit historical file retrieval.
  • Initial focused CLI race tests passed in 110.166s. After CI exposed a transport-size overflow, the failed transport test and affected finalization/coherence tests passed with race detection: ok github.com/openclaw/crabbox/internal/cli 45.999s.
  • go vet ./internal/cli and the candidate CLI build passed.
  • Isolated Codex autoreview completed scoped-clean through P2.
  • An AWS c7a.4xlarge proof used a 55,990-file, 639.4 MiB checkout. Ordinary initial and repeated syncs completed, including owner release. A controlled unfiltered depth-2 history fetch hit a 120-second proof limit while Git metadata grew from 183 MB to 2.05 GB. The blob-filtered equivalent completed in 58 seconds and retained full ancestry and promisor configuration.
  • AWS execution proof at 610370eb (before the transport-size helper refactor) against reconstructed shallow metadata passed: finalize 64.111 seconds, workload exit 0, completed transaction markers, expected HEAD/index tree, full ancestry, matching file content, and zero remaining owner/child records.
  • Proof lease cbx_e08e185308ab was stopped and independently read back as released.
  • The initial PR CI found a witnessed finalizer 141 bytes over the existing 30,000-byte SSH limit. Factoring the repeated hydration fetch into one helper fixes the overflow without raising the limit. The older full local CLI run was stopped after this failure was diagnosed; it has no clean result. Only the failed transport and affected finalization tests were rerun locally after the failure.
  • All four Go CLI race shards and all three CLI coverage shards passed on final head 8fa814fbb7e0c8297c26d2d896641b62db6217d7: https://github.com/openclaw/crabbox/actions/runs/38704375167. Other workflow checks remain pending at the latest read; no failures are reported on this head.

The original reported leases were already released, so their process trees could not be recovered. The controlled shallow-history reproduction establishes the excessive-history-fetch mechanism rather than claiming a live capture of every original occurrence. Origins that ignore Git filtering may still download historical blobs.

@clawsweeper

clawsweeper Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T15:34:41.991479Z 226941f New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 610370eb20

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/cli/ssh.go
` + remoteGitOriginTransportFunctions() + `
coherence_fetch() {
set -- --quiet --no-tags
set -- --quiet --filter=blob:none --no-tags

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep sync working when Git fetch lacks filters

On a POSIX target running Git 2.18, this unconditional option makes every coherence fetch fail because that version's documented fetch options include --unshallow but not --filter. The shell converts this into exit 69, and gitOriginRuntimeFallbackResult does not classify the resulting unknown-option diagnostic, so an otherwise supported origin-based sync now aborts instead of falling back to plain-manifest sync. Please detect filter support or handle this diagnostic as a safe fallback without performing the problematic unfiltered history fetch.

Useful? React with 👍 / 👎.

Comment thread docs/features/sync.md
Comment on lines +364 to +366
Finalization requests blob-filtered Git fetches, including when completing a
shallow repository left by workflow setup or a previous command. On origins
that support filtering, commit ancestry becomes available without downloading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply blob filtering to native Windows finalization

This statement is false for native Windows targets: syncWindowsNative still finalizes through windowsGitCoherence, whose fetch at internal/cli/sync_windows_target.go:264 has neither --filter=blob:none nor shallow-history completion. Consequently a native Windows workspace can still download all fetched blobs, and a shallow checkout does not gain the promised ancestry. Git documents that --filter=blob:none omits file contents until needed; mirror the filtering and shallow handling in the Windows finalizer, or scope the documented fix explicitly to POSIX/WSL targets.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 15, 2026
@clawsweeper

clawsweeper Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

This PR filters POSIX and WSL finalization fetches and clarifies recovery when a live remote child prevents workspace-owner release.

Example: An incremental sync encounters a depth-2 checkout with substantial historical file contents.

  • Before: Completing ancestry also downloads historical blobs; the reported AWS reproduction grew Git metadata from 183 MB to 2.05 GB.
  • After: Filtering-capable origins provide ancestry while deferring historical file contents until explicitly requested.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The supplied AWS account and real-Git regression support the mechanism, but inspectable runtime evidence is limited and the compatibility regression blocks landing.
Proof confidence 🦐 gold shrimp (3/6) The host identifies a maintainer-authored PR outside the ordinary contributor proof gate; its AWS report covers shallow finalization, command completion, ancestry/content verification, and cleanup at 610370e, before the hydration-helper refactor.
Patch quality 🦪 silver shellfish (2/6) 2 actionable review findings remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Incremental sync can spend substantial bandwidth and time downloading historical file contents before the requested command starts.
Reason: The host identifies this as maintainer-authored work, establishing direction; the small shared finalizer change addresses a concrete execution bottleneck without a new option or protocol.

Merge readiness

⛔ Needs changes before merge - 2 items remain

Keep this PR open: it addresses excessive history downloads that remain on main, but needs a legacy-Git compatibility repair and narrower documentation.

Priority: P2
Reviewed head: 8fa814fbb7e0c8297c26d2d896641b62db6217d7

Before merge

  • Preserve sync recovery for Git's partial-clone prerequisite refusal (P1) - On a reused ordinary repository with Git 2.18 and no extensions.partialClone setting, the newly unconditional --filter makes fetch terminate with '--filter can only be used when extensions.partialClone is set'. The previous unfiltered coherence fetch worked for this input. Finalization converts the refusal to exit 69, but gitOriginRuntimeFallbackResult does not recognize it, so crabbox run aborts before executing the workload. Git 2.18 actually accepts the option; the earlier review identified the compatibility concern but misstated its cause. Detect this unsupported filtering state or classify its exact refusal for safe plain-manifest recovery, without restoring an unfiltered historical fetch.
  • Qualify the filtering guarantee as POSIX and WSL behavior (P3) - The sync page explicitly covers native Windows targets, but this new paragraph promises blob-filtered finalization without qualifying the target. Native Windows still uses windowsGitCoherence, whose fetch has neither --filter nor shallow-history completion. A Windows user following this guidance can therefore still download historical blobs or retain an insufficient shallow boundary. Scope this paragraph to POSIX/WSL rather than expanding this focused repair into the Windows implementation.

Findings

  • [P1] Preserve sync recovery for Git's partial-clone prerequisite refusal — internal/cli/ssh.go:3062
  • [P3] Qualify the filtering guarantee as POSIX and WSL behavior — docs/features/sync.md:364-366
Agent review details

How this fits together

The Go CLI generates remote finalization scripts after workspace transfer; these fetch Git history, verify branch membership and tree identity, and publish sync completion under workspace ownership.

flowchart LR
  A[CLI run] --> B[Workspace owner]
  B --> C[File transfer]
  C --> D[Git finalization]
  D --> E[Git origin]
  D --> F[Verified workspace]
  F --> G[User command]
Loading

Technical review

Best possible solution:

Keep filtered history completion, preserve plain-manifest recovery for incompatible Git clients, and document its POSIX/WSL scope.

Do we have a high-confidence way to reproduce the issue?

Main's unfiltered --unshallow fetch directly explains historical-blob downloads, and the author reports a controlled AWS reproduction; no target code was executed during this read-only review.

Is this the best way to solve the issue?

Filtering at the shared POSIX finalizer addresses the download mechanism while preserving full ancestry and ownership checks, provided incompatible clients retain safe recovery.

Full review comments:

  • [P1] Preserve sync recovery for Git's partial-clone prerequisite refusal — internal/cli/ssh.go:3062
    On a reused ordinary repository with Git 2.18 and no extensions.partialClone setting, the newly unconditional --filter makes fetch terminate with '--filter can only be used when extensions.partialClone is set'. The previous unfiltered coherence fetch worked for this input. Finalization converts the refusal to exit 69, but gitOriginRuntimeFallbackResult does not recognize it, so crabbox run aborts before executing the workload. Git 2.18 actually accepts the option; the earlier review identified the compatibility concern but misstated its cause. Detect this unsupported filtering state or classify its exact refusal for safe plain-manifest recovery, without restoring an unfiltered historical fetch.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.
  • [P3] Qualify the filtering guarantee as POSIX and WSL behavior — docs/features/sync.md:364-366
    The sync page explicitly covers native Windows targets, but this new paragraph promises blob-filtered finalization without qualifying the target. Native Windows still uses windowsGitCoherence, whose fetch has neither --filter nor shallow-history completion. A Windows user following this guidance can therefore still download historical blobs or retain an insufficient shallow boundary. Scope this paragraph to POSIX/WSL rather than expanding this focused repair into the Windows implementation.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against e36baddc79ec.

Provenance checked

  • Coherence history fetching keeps the original intent (0097f1c: Complete shallow history so valid older commits pass advertised-branch ancestry verification.)
  • Hydration depth handling keeps the original intent (40449c5: Avoid --unshallow on complete repositories and retain repeatable finalization.)
  • Workspace-owner release refusal keeps the original intent (784092e: Retain ownership while a witnessed child remains active and fail closed on ambiguous state.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

  • go test -race -timeout=20m ./internal/cli -run 'TestRemoteFinalizeSync|TestRemoteGitCoherence|TestWorkspaceOwnerReleaseChildDiagnostic'
  • go vet ./internal/cli
  • git diff --check

What I checked:

Likely related people:

  • Peter Steinberger: Raw commit 0097f1c adds internal/cli/ssh.go:2891 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 0097f1c90b6c; files: internal/cli/ssh.go)
  • Vincent Koc: Raw commit 784092e adds internal/cli/workspace_owner.go:308 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 784092eeae1d; files: internal/cli/workspace_owner.go)

Labels

Label changes:

  • add P2: Excessive history transfer affects incremental remote execution, with a focused repair and limited subsystem scope.
  • add merge-risk: 🚨 compatibility: Unconditional filtering makes existing ordinary repositories on Git 2.18 fail coherence finalization instead of completing their previously working fetch.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: Excessive history transfer affects incremental remote execution, with a focused repair and limited subsystem scope.
  • merge-risk: 🚨 compatibility: Unconditional filtering makes existing ordinary repositories on Git 2.18 fail coherence finalization instead of completing their previously working fetch.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 15, 2026, 4:44 AM ET / 08:44 UTC.

Complete commit ancestry without eagerly fetching historical file blobs. Preserve owner fencing and fail-closed release semantics while distinguishing a witnessed live child from ambiguous transport state.
Factor the repeated hydration fetch invocation into one shell helper. Preserve every fallback and the existing 30000-byte witnessed-command limit.
@steipete
steipete force-pushed the steipete/sync-finalize-child-stall branch from 8fa814f to 226941f Compare October 15, 2026 15:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 226941fee5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/cli/ssh.go
if !opts.GitOverlay && !opts.PlainManifest && opts.HydrateGit && opts.BaseRef != "" {
refspec := "+refs/heads/" + opts.BaseRef + ":refs/remotes/origin/" + opts.BaseRef
script += `if exact_git_root && git remote get-url origin >/dev/null 2>&1; then
hydrate_fetch() { git fetch --quiet --filter=blob:none origin "$@"; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back when hydration Git lacks filtering

On a POSIX target running Git 2.18, a hydration-only sync (sync.gitSeed: false, so coherence is disabled while HydrateGit remains enabled) now sends every fetch attempt through this helper, and each fails on the unsupported --filter option. The trailing || true then reports a successful sync and records hydration metadata even though the repository was never unshallowed or updated; previously these fetches worked without filtering. The Git 2.18 fetch manual documents --unshallow but has no --filter option, so this path should detect filter support or retry hydration without it.

Useful? React with 👍 / 👎.

@steipete
steipete merged commit 05eded3 into main Oct 15, 2026
64 of 66 checks passed
@steipete
steipete deleted the steipete/sync-finalize-child-stall branch October 15, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant