Fix workspace fetch into the current branch

[NriotHrreion]

close #66

Description

Proval fetches the remote branch directly into a local branch with the same name (like ref/heads/master). When that branch matched the default branch created by git init, git will reject the fetch because it would update the currently checked out branch.

What changed

Fetch the remote branch into a remote ref refs/remotes/origin/... instead of the branch ref itself. When loading from branch, just checkout with the new ref name returned by fetch().

Added apps/api/src/git-provider/workspace.test.ts.

[proval-0e6d07]

Thanks for this change — the refspec remapping is a clean fix, and the new end-to-end fixture tests drive real git rather than mocks.

Overview

Summary

This PR makes Workspace.fetch map refs/heads/* wants to refs/remotes/origin/* targets and return that target, so loadFromBranch checks out exactly what the fetch wrote instead of relying on a local-branch side effect. It also adds a real-git test fixture covering PR-ref loads (GitHub and GitLab ref shapes), branch loads, and the adopted-repository path. The main risk is that the two loadFromPullRequest tests fail before their final assertion: previousSha never lands in the fixture workspace, so the push-diff call throws (see Main Issue).

Good points

• 🟢 Mapping the fetch refspec to want:refs/remotes/origin/* (apps/api/src/git-provider/workspace.ts:114) is the correct git idiom — it avoids writing into local refs/heads/* and lets loadFromBranch check out the same target value the fetch produced, removing a fragile coupling.
• 🟢 The branch-load tests cover both arms of the new ternary in loadFromBranch — the fetched path via init() and the adopted path where authHeader is null (apps/api/src/git-provider/workspace.test.ts:99 and line 106) — so both fallback behaviors are exercised.

Main Issues

• ⚫️ apps/api/src/git-provider/workspace.test.ts:92 — previousSha is fetched as a raw SHA against a plain git init fixture remote that rejects unadvertised wants; the failure is swallowed (workspace.ts:168-171), so getPushFileDiff later runs git diff against an unknown revision (workspace.ts:235) and throws before the assertion executes. Both PR-ref tests fail in CI. Fix: set uploadpack.allowReachableSHA1InWant=true on the fixture remote, or advertise previousSha as a ref tip.

Additional Notes

• apps/api/src/git-provider/workspace.ts:124 — the comment says the method "Returns the fetched remote ref name (refs/remotes/origin/...)", but for raw-SHA and PR-ref inputs target is the SHA or the ref unchanged; only refs/heads/* input produces a remote-tracking name. Today only loadFromBranch consumes the return value, so the risk is latent, but rewording the comment would keep the contract accurate.

[NriotHrreion]

@proval-0e6d07 review

[proval-0e6d07]

@NriotHrreion

Reviewed — full findings are in the review above; here's the verdict for this thread:

Verdict

The core change is correct: remapping refs/heads/* wants to refs/remotes/origin/* in fetch() (apps/api/src/git-provider/workspace.ts:114) is the right git idiom, and having loadFromBranch check out the exact ref the fetch returned (apps/api/src/git-provider/workspace.ts:187) removes the fragile reliance on a local-branch side effect.

Blocking issue

• ⚫️ apps/api/src/git-provider/workspace.test.ts:92 — both PR-ref tests fail before their final assertion. previousSha is fetched as a raw SHA against the fixture remote, which rejects unadvertised wants by default (uploadpack.allowReachableSHA1InWant is off). The failure is swallowed at apps/api/src/git-provider/workspace.ts:166-171, so getPushFileDiff later runs git diff against a revision that never landed in the workspace (apps/api/src/git-provider/workspace.ts:235) and throws. Fix: set git config uploadpack.allowReachableSHA1InWant true in the fixture repo setup, or advertise previousSha as a ref tip (e.g. git update-ref refs/heads/previous <sha>).

Minor

• apps/api/src/git-provider/workspace.ts:125 — the comment says the method returns "the fetched remote ref name (refs/remotes/origin/...)", but for raw-SHA and PR-ref inputs target is returned unchanged. Only refs/heads/* input produces a remote-tracking name. Today only loadFromBranch consumes the return value, so the risk is latent, but the comment should be reworded to keep the contract accurate.

Once the fixture setup allows the previousSha want, this is good to merge.

[proval-0e6d07]

@NriotHrreion

Thanks for the follow-up — this push addresses the blocking fixture issue from the last review and restructures the suite for isolation. The change set is small and focused.

Overview

a. Summary
This push reworks apps/api/src/git-provider/workspace.test.ts (the only changed file): the suite now runs in an isolated child bun test process gated by PROVAL_WORKSPACE_TEST_CHILD, and the fixture advertises previousSha as a ref tip. The latter directly fixes the prior review's blocking issue — the raw-SHA previousSha fetch is now accepted by the fixture remote, so the getPushFileDiff -previous assertion can pass. All seven test bodies are otherwise preserved, and the harness matches the already-accepted sibling pattern in webhook-signing.test.ts. No blocking issues remain in this push.

b. Good points

• 🟢 The fixture now runs git update-ref refs/heads/previous previousSha (apps/api/src/git-provider/workspace.test.ts:81), advertising previousSha as a ref tip so Workspace.fetch's raw-SHA want (workspace.ts:111-112) is accepted and the previously failing -previous assertion in both PR-ref tests can succeed.
• 🟢 The parent process imports Workspace type-only and loads the real class via dynamic import only in the child (workspace.test.ts:7, :36), so it never pulls in the util/log.ts:22 → ActivityService chain that other test files mock — and the spawn sets DB_FILE_NAME: ":memory:" (workspace.test.ts:26), matching src/db/index.ts:5, so the child never touches a real database file.
• 🟢 The harness is byte-for-byte aligned with the accepted sibling pattern in apps/api/src/webhook/webhook-signing.test.ts:9-19 (same spawn, env, throw-on-failure, 30s timeout), and child failures surface both stdout and stderr in the parent (workspace.test.ts:31-33).

Main Issues

None in this push — the prior blocking issue is fixed.

Additional Notes

On success, CI will report the seven inner tests as a single wrapper test named "passes workspace integration in an isolated process" (workspace.test.ts:24); this is the same observability trade-off already accepted for the sibling harnesses.

Once CI confirms the suite is green, this is good to merge.

[seoes]

Thanks for this job @NriotHrreion!

I reviewed it and you made it in best way

I'll merge it immediately