Cross-fork PRs with fork-unique commits have no object bridge (open 503s, merge/diff fail) #456

New issue

Closed

opened 2026-09-13 14:19:19 +00:00 by crueber · 3 comments

crueber commented

2026-09-13 14:19:19 +00:00

Owner

Child of #449 (audit finding F1 High, comment 4555; audited e8bc499c). Cross-fork PRs whose head contains fork-unique commits have no fork-to-base object bridge: OpenPR maps the rev-list error to 503 (service.go:375-378 — missing objects are an error, not unreachable), and mergeability/merge/diff run git in baseDir where fork objects can never appear (fallback is child-to-ancestor only). Only base-contained heads work end to end. Fix: bridge fork objects into the base serving copy for open/mergeability/merge/diff (or fetch them on demand), with tests proving a fork-unique-commit PR opens, diffs, and merges.

Child of #449 (audit finding F1 High, comment 4555; audited e8bc499c). Cross-fork PRs whose head contains fork-unique commits have no fork-to-base object bridge: OpenPR maps the rev-list error to 503 (service.go:375-378 — missing objects are an error, not unreachable), and mergeability/merge/diff run git in baseDir where fork objects can never appear (fallback is child-to-ancestor only). Only base-contained heads work end to end. Fix: bridge fork objects into the base serving copy for open/mergeability/merge/diff (or fetch them on demand), with tests proving a fork-unique-commit PR opens, diffs, and merges.

crueber referenced this issue

2026-09-13 14:19:25 +00:00

Audit: fork feature vs git directly — clone/fetch/push, object completeness, ref copying, parent deletion/GC, provenance (findings as comments) #449

crueber referenced this issue from a commit

2026-09-13 15:38:00 +00:00

Fix #456: fork-to-base object bridge for cross-fork PRs (03 §7, 04 Decisions)

crueber referenced this issue from a pull request that will close it,

2026-09-13 15:38:08 +00:00

Fix #456: cross-fork object bridge #467

crueber commented

2026-09-13 15:38:17 +00:00

Author

Owner

Fixed by #467: fork-to-base object bridge (fetch fork head into base serving copy on demand). Fork-unique PRs now open fork-local, diff, compute mergeability, and merge end-to-end (real-git e2e included). Do NOT merge this comment's PR without review.

crueber referenced this issue from a commit

2026-09-13 15:40:16 +00:00

Fix double-space typo in 03 §7 bridge decision note (#456 review)

crueber commented

2026-09-13 15:40:32 +00:00

Author

Owner

Review of PR #467 (fix/issue-456, cross-fork object bridge) — adversarial pass, verified in scratch worktree /tmp/pr467 (removed afterward). Main worktree untouched (still clean on main).

FINDINGS (all 8 checks pass):

Fetch argv exact + pinned (law 2): code internal/pulls/git.go FetchInto builds {-c gc.auto=0, fetch, --no-tags, --no-write-fetch-head, --quiet, srcDir, sha} — verbatim match to docs/go/04_git.md Decisions entry added in same change. No refs (no FETCH_HEAD write), gc disabled, full history (merge-base/trial-merge need ancestry), fetch-by-sha exact under moved branches / loud under force-pushed-away shas. Verified live: TestBridgeFetchIntoReal asserts for-each-ref == refs/heads/main only after bridge, unknown-sha fails, garbage-sha fails fast via validateSHA, cancelled ctx propagates unavailable.
Open path: headDir resolved before bridge (service.go:358-361); bridgeForkHead pre-probes, fetches only on miss for cross-repo, same-repo short-circuits with zero fetch (unit test: same-repo 422 no-fetch; base-contained cross-fork opens with zero fetch). Bridge failure -> 503 preserved (ErrUnavailable wrap, unit-tested).
Diff path: diffDir bridges then re-probes (mergeable.go:365-378); still-missing head keeps fork-dir fallback, bridge failure -> 503. Fallback preserved, never a guessed diff. Commits rides the same dir choice (proven in e2e).
Mergeable/merge: unconditional FetchInto for cross-repo heads only (mergeable.go:138-142, merge.go:129+), same-repo untouched; bridge failure fails loud (mergeable ErrUnavailable; merge task TaskError naming the bridge, narrated via rec.notice). headDir resolved before fetch in both, so fork-deleted (#451 interplay) -> 503/unknown-ref, never silent wrong-answer. No unconditional-fetch on same-repo.
refs/pull/N/head publish rule: gated on pre (service.go: pre && Refs != nil) — bridged-only heads stay fork-local (e2e asserts !HeadPublished + no pull-head publish call). No dangling base refs: manifest never written, bridged objects are serving-copy warmth.
Eviction/GC: base manifest untouched (serving-copy warmth only, re-bridges after eviction); fetch is ref-free + idempotent so concurrent bridges converge with no locks (13 §2 rule 4, pool-gated subprocess); mid-task eviction surfaces as loud task error, not wrong merge. Merge durability: bridged objects ride the UpdateRefWithPack pack atomically (e2e asserts update-pack with pack + ancestry of both parents in base copy).
Law-6 cost: open/diff pre-probe (common/base-contained case costs zero extra subprocesses); bridge is local disk-to-disk, zero bucket trips, on the failure path. Residual nit (non-blocking): mergeable/merge fetch unconditionally for cross-repo (one extra local no-op subprocess even when objects present) — acceptable, not a hot path, no store trips.
Coverage/tests/hygiene: pulls 96.0% (>=95%), -race green incl. -count=3 on TestBridge*, real-git e2e open->diff->commits->mergeable->merge green, gofmt/vet clean, go build ./... clean, no new deps (go.mod/go.sum untouched; bridge.go imports context only). Docs: 03 §7 + Decisions and 04 Decisions updated in same change (law 12). No browser needed: no browser-facing changes (pure git/serving-copy layer + docs); stating explicitly per instructions.

FIX APPLIED: double-space typo in 03 §7 decision note ('event); repair' -> 'event); repair'), pushed to origin/fix/issue-456 (c1c6b0f), pulls -race re-run green after push.

Note: PR description mentions internal/server web-asset test failures pre-existing on pristine main — out of scope for this change (untouched packages), not verified here.

MERGE RECOMMENDATION: ready to merge.

Review of PR #467 (fix/issue-456, cross-fork object bridge) — adversarial pass, verified in scratch worktree /tmp/pr467 (removed afterward). Main worktree untouched (still clean on main). FINDINGS (all 8 checks pass): 1. Fetch argv exact + pinned (law 2): code internal/pulls/git.go FetchInto builds {-c gc.auto=0, fetch, --no-tags, --no-write-fetch-head, --quiet, srcDir, sha} — verbatim match to docs/go/04_git.md Decisions entry added in same change. No refs (no FETCH_HEAD write), gc disabled, full history (merge-base/trial-merge need ancestry), fetch-by-sha exact under moved branches / loud under force-pushed-away shas. Verified live: TestBridgeFetchIntoReal asserts for-each-ref == refs/heads/main only after bridge, unknown-sha fails, garbage-sha fails fast via validateSHA, cancelled ctx propagates unavailable. 2. Open path: headDir resolved before bridge (service.go:358-361); bridgeForkHead pre-probes, fetches only on miss for cross-repo, same-repo short-circuits with zero fetch (unit test: same-repo 422 no-fetch; base-contained cross-fork opens with zero fetch). Bridge failure -> 503 preserved (ErrUnavailable wrap, unit-tested). 3. Diff path: diffDir bridges then re-probes (mergeable.go:365-378); still-missing head keeps fork-dir fallback, bridge failure -> 503. Fallback preserved, never a guessed diff. Commits rides the same dir choice (proven in e2e). 4. Mergeable/merge: unconditional FetchInto for cross-repo heads only (mergeable.go:138-142, merge.go:129+), same-repo untouched; bridge failure fails loud (mergeable ErrUnavailable; merge task TaskError naming the bridge, narrated via rec.notice). headDir resolved before fetch in both, so fork-deleted (#451 interplay) -> 503/unknown-ref, never silent wrong-answer. No unconditional-fetch on same-repo. 5. refs/pull/N/head publish rule: gated on pre (service.go: pre \&\& Refs != nil) — bridged-only heads stay fork-local (e2e asserts !HeadPublished + no pull-head publish call). No dangling base refs: manifest never written, bridged objects are serving-copy warmth. 6. Eviction/GC: base manifest untouched (serving-copy warmth only, re-bridges after eviction); fetch is ref-free + idempotent so concurrent bridges converge with no locks (13 §2 rule 4, pool-gated subprocess); mid-task eviction surfaces as loud task error, not wrong merge. Merge durability: bridged objects ride the UpdateRefWithPack pack atomically (e2e asserts update-pack with pack + ancestry of both parents in base copy). 7. Law-6 cost: open/diff pre-probe (common/base-contained case costs zero extra subprocesses); bridge is local disk-to-disk, zero bucket trips, on the failure path. Residual nit (non-blocking): mergeable/merge fetch unconditionally for cross-repo (one extra local no-op subprocess even when objects present) — acceptable, not a hot path, no store trips. 8. Coverage/tests/hygiene: pulls 96.0% (>=95%), -race green incl. -count=3 on TestBridge*, real-git e2e open->diff->commits->mergeable->merge green, gofmt/vet clean, go build ./... clean, no new deps (go.mod/go.sum untouched; bridge.go imports context only). Docs: 03 §7 + Decisions and 04 Decisions updated in same change (law 12). No browser needed: no browser-facing changes (pure git/serving-copy layer + docs); stating explicitly per instructions. FIX APPLIED: double-space typo in 03 §7 decision note ('event); repair' -> 'event); repair'), pushed to origin/fix/issue-456 (c1c6b0f), pulls -race re-run green after push. Note: PR description mentions internal/server web-asset test failures pre-existing on pristine main — out of scope for this change (untouched packages), not verified here. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-13 15:40:54 +00:00

crueber referenced this issue from a commit

2026-09-13 15:40:54 +00:00

Merge pull request 'Fix #456: cross-fork object bridge' (#467) from fix/issue-456 into main

crueber commented

2026-09-13 15:40:55 +00:00

Author

Owner

Fixed by PR #467 (review clean + one typo fix by reviewer; all 8 bridge-safety checks pass, e2e open→merge green), merged. Closing.

crueber referenced this issue

2026-09-13 17:22:04 +00:00