refactor(core)!: prepareContext decides the start commit once (start: base | reuse | prHead)

[SebaBoler]

Closes #446 (candidate 4 of the 2026-10-08 architecture review).

What changed

"Which commit does a task start from" was decided at six call sites, three of which (review-pr, review-mr, research) passed no base and silently fell back to 'main' inside prepareContext. It is now decided once.

PrepareOptions takes two required inputs:

• baseBranch: string — the branch the task is for (the cut point of a 'base'/'reuse' start, and what the PR/MR targets).
• start: TaskStart = 'base' | 'reuse' | { prHead: string } — which commit the task starts from.

prepareContext resolves the start ref ONCE (resolveStartRef, in core/vanguard.ts) to a SHA and records it as ctx.startRef, alongside ctx.baseBranch. It is logged once on the existing run start line. resolveRemoteBaseRef is unchanged. baseBranch is validated (assertSafeBaseBranch) for every start kind.

start	resolution
'base'	resolveRemoteBaseRef(repo, baseBranch, { keepLocalIfAhead }) — origin's copy when ahead, the local copy when kept (existing semantics)
'reuse'	never fetched — refs/heads/<base>, then refs/remotes/origin/<base>, then the bare name. When none is a commit here, an existing run branch is still picked up and measured from its own tip (logged); a fresh cut fails in WorktreeManager.create as it always did
{ prHead }	git fetch --end-of-options origin refs/heads/<prHead> → FETCH_HEAD (what revise-pr did inline), now with GIT_TERMINAL_PROMPT=0, a 60 s timeout and a masked WorktreeError on failure

Whatever the kind, the result is rev-parsed to a commit SHA; a ref that is not a commit fails with a WorktreeError naming the task and the ref — there is no fallback.

Downstream readers now use the context instead of re-deriving:

• the worktree cut: wm.create(taskId, ctx.startRef, …)
• deliverChange: closingKeywordBase?: string → closingKeywordScan?: boolean, scanning ctx.startRef..HEAD; DeliveryTarget.new-pr.baseBranch removed
• publishForReview: PublishOptions.baseBranch (and its ?? 'main' default, three occurrences) removed; the fetch/rebase target and --base/--target-branch come from ctx.baseBranch
• rebaseOntoRemoteBase is otherwise unchanged: it still counts HEAD..FETCH_HEAD (not startRef..FETCH_HEAD), because a reused branch may be cut from an older base than the one its run resolved and it is the branch's own distance that decides whether the push would be stale; the doc comment says so

DEFAULT_BASE_BRANCH = 'main' (exported from core/base-branch.ts) is the one named default for callers that take --base; baseBranchOrDefault(base, what) is the loud fallback for a PR/MR record that carries no base (it warns). WorktreeManager.create's own = 'main' parameter default remains but is always overridden by prepareContext.

Caller → start mapping

caller	baseBranch	start	notes
runSourcedIssue (source-adapter)	deps.baseBranch ?? DEFAULT_BASE_BRANCH	reuse ? 'reuse' : 'base'	keepLocalIfAhead passed through; unchanged behaviour
core run() (vanguard run)	opts.baseBranch ?? DEFAULT_BASE_BRANCH	reuse ? 'reuse' : 'base'	RunOptions unchanged
runSpecGenerator (spec)	deps.baseBranch ?? DEFAULT_BASE_BRANCH	'base', keepLocalIfAhead: false	resolveSpecBaseRef wrapper removed (only the spec: log label is lost)
reviseGithubPr	baseBranchOrDefault(pr.baseRefName)	{ prHead: pr.headRefName }	the inline fetch moved into prepareContext; the _baseBranch test hook maps to start: 'base', keepLocalIfAhead: true on a local branch
review-pr	baseBranchOrDefault(pr.baseRefName)	'base'	was local 'main', no fetch
review-mr	baseBranchOrDefault(mr.targetBranch)	'base'	was local 'main', no fetch
research	DEFAULT_BASE_BRANCH	'base'	was local 'main', no fetch; research has no --base

review-pr / review-mr / research

These read-only runs previously cut their worktree from the local main whatever the PR targeted. They now state the base explicitly — the PR's base / the MR's target branch (research has no PR, so the named default) — resolved the same way as every other run (resolveRemoteBaseRef: origin's copy when ahead, best-effort local fallback when offline). For a PR against main on a CI checkout this is the same tree as before plus one fetch; for a PR against another branch the reviewer now reads the tree the change actually lands on.

Trade-off, stated: for a stacked PR (base = another contributor's unmerged branch) the reviewer's worktree — and with it CLAUDE.md, AGENTS.md and skills/ as the agent sees them — now comes from that base branch rather than from main. The diff under review was always untrusted; the instruction files around it previously came from the default branch. Reviewing against the real base was judged the better default (it is what the change lands on); pinning instruction files to the default branch is a possible follow-up if stacked PRs from untrusted authors become a concern.

Tests

• vanguard.test.ts: ctx.startRef asserted directly against a real origin+clone (origin tip for 'base', local tip for 'reuse', fetched head for { prHead }; the run start log carries it exactly once; an unresolvable start throws instead of defaulting; a 'reuse' start resumes a run branch through the tracking ref and then through the branch's own tip when the base is gone; an unsafe baseBranch is rejected for a prHead start).
• source-adapter.test.ts: the resolveRemoteBaseRef mock is gone; tests assert the start/baseBranch handed to prepareContext and that the closing-keyword scan uses ctx.startRef.
• spec.test.ts: the four resolveSpecBaseRef unit tests (duplicates of base-branch.test.ts) are replaced by one runner-level test proving the spec worktree is cut from origin's commit even when the local base is ahead.
• review-pr.test.ts / review-mr.test.ts: the default reviewer hands prepareContext the PR base / MR target with start: 'base', and DEFAULT_BASE_BRANCH plus a warning when the record carries none.
• remote-branch.test.ts: a new case checks the fetch target and --base come from the context.
• All other prepareContext call sites in tests (and examples/validate-phase3.ts) gained baseBranch: 'main', start: 'base' (mechanical).

Gates

• pnpm lint — clean
• pnpm typecheck — 0 errors
• pnpm test — 123 files, 2512 passed, 3 skipped

Rebased onto main after #458 (pipeline.ts shim removal); no conflicts.

BREAKING CHANGE: PrepareOptions.baseBranch and PrepareOptions.start are required (PrepareOptions.reuse is replaced by start: 'reuse'); PublishOptions.baseBranch, DeliveryTarget.new-pr.baseBranch and DeliverChangeOptions.closingKeywordBase (now closingKeywordScan: boolean) are removed; resolveSpecBaseRef is removed. RunContext gains baseBranch and startRef; TaskStart and DEFAULT_BASE_BRANCH are new exports.

[github-actions]

Vanguard Review

Verdict: BLOCKING

Repo guidelines read: CLAUDE.md (no AGENTS.md, and the "Review guidance" section points to no further review doc). It defines no severity scale, so I use High/Medium/Low. No CI-config path is touched by this diff, so the CI-config rules in §"Review guidance" don't apply.

Blocking

1. --reuse resume now hard-fails when the base has no local branch (High, correctness/regression)
prepareContext resolves startRef before wm.create, and the 'reuse' arm of resolveStartRef only tries local refs/heads/<baseBranch> (never fetched, no fallback) and throws WorktreeError otherwise. Previously reuse: true reached WorktreeManager.create, which early-returns the existing run branch at src/worktree/manager.ts:60-63 without ever using the base. So vanguard run --reuse --base release/2.0 in a clone that tracks but has no local release/2.0 now throws Cannot resolve the start commit for … and loses the resume, where it previously succeeded. The new test fails loudly when the start cannot be resolved… (start: 'reuse', baseBranch: 'no-such-branch') codifies exactly this case. Fix: for 'reuse', fall back (or resolve lazily) when an existing branch will be reused.

2. pr.baseRefName / mr.targetBranch are ?? ''-defaulted and go straight into a throwing validator (High, correctness)
fetchPullRequestForReview sets baseRefName: view.baseRefName ?? '' (src/runners/pr-review.ts:142) and fetchMergeRequestForReview sets targetBranch: view.target_branch ?? '' (src/runners/mr-review.ts:115). The new review-pr.ts / review-mr.ts call sites pass that value as baseBranch with start: 'base', so an empty value reaches assertSafeBaseBranch (src/core/base-branch.ts:14) and aborts the entire review run before any agent work. The old literal 'main' made this unreachable. Guard with pr.baseRefName || DEFAULT_BASE_BRANCH.

Non-blocking

3. startRef..FETCH_HEAD is not the same set as HEAD..FETCH_HEAD under start: 'reuse' (Medium)
The PR body's justification ("the same set since task commits are never on the remote base") holds only when startRef is an ancestor of HEAD. For a reuse start the worktree is a pre-existing branch cut from an older base while startRef is the current local base tip; if the local base has since advanced to origin's tip the count is 0, so the #423 pre-push rebase is silently skipped on a stale branch. Narrow in practice — rebaseOntoRemoteBase already early-returns for a branch that exists on the remote (src/pipeline/remote-branch.ts:132-137) — so it only bites a reuse run whose branch was never pushed. Worth a comment at minimum.

4. examples/validate-phase3.ts still calls prepareContext without the now-required baseBranch/start (Medium, maintainability)
Three call sites (lines 50, 84, 117). At runtime start === undefined falls through to the { prHead } arm and throws a TypeError on start.prHead. Neither gate catches it: tsconfig.json has "include": ["src"] and pnpm lint globs src/**/*.ts only. README was updated; these weren't.

5. The three behaviour-changing callers have no test (Medium, tests)
src/cli/review-pr.test.ts and src/cli/review-mr.test.ts exist and are untouched; nothing asserts the base now comes from pr.baseRefName / mr.targetBranch, nor that a fetch now happens. Per the PR's own table these three (review-pr, review-mr, research) are the only callers whose behaviour changes, and they are the only ones without coverage.

6. baseBranch is validated on two of three start kinds (Low, defence in depth)
'reuse' calls assertSafeBaseBranch, 'base' gets it via resolveRemoteBaseRef, but the { prHead } arm validates neither baseBranch nor prHead. So ctx.baseBranch — documented as "the pre-push rebase fetches it and the PR/MR targets it" — can hold an unvalidated, PR-sourced value. Harmless today (revise-pr never publishes, and rebaseOntoRemoteBase re-asserts), but the refactor's whole point is that ctx is the single chokepoint; assert there. (prHead itself remains protected as before by --end-of-options plus the full refs/heads/ source.)

7. Stale doc on the _baseBranch test hook (Low)
src/runners/revise-pr.ts: "Start the worktree from this local branch instead of the PR head on origin (no fetch)". The hook now maps to start: 'base', which does fetch origin via resolveRemoteBaseRef and will prefer origin's commit when the local branch is behind — so the hook no longer reliably pins the local branch.

Checked and clean

• assertSafeBaseBranch accepts the 40-hex SHA now handed to wm.create — no new rejection there.
• closingKeywordScan + ctx.startRef is equivalent to the old closingKeywordBase: baseRef (commitMessages runs git log <ref>..HEAD, src/worktree/manager.ts:151-161).
• publishForReview base via ctx.baseBranch matches the old target.baseBranch ?? 'main' for runSourcedIssue.
• The four deleted resolveSpecBaseRef tests are genuinely duplicated by src/core/base-branch.test.ts (no-remote fallback at :95, unsafe-base rejection at :159); resolveSpecBaseRef was not re-exported from src/index.ts, so its removal is internal.

Not covered

I did not run pnpm typecheck / pnpm test (the working tree is at the pre-PR baseline, not this PR's head, so the diff could not be executed), and I did not audit the ~60 mechanical baseBranch: 'main', start: 'base' test edits beyond spot checks.

[SebaBoler]

Review round 1 addressed in b8202dd.

1. --reuse hard-fails without a local base (High) — fixed. The 'reuse' arm now resolves refs/heads/<base>, then refs/remotes/origin/<base> (the tracking ref a clone holds for a branch it never checked out — your release/2.0 case), then the bare name. If none is a commit, resolveStartRef returns undefined, wm.create gets the bare base exactly as before (an existing run branch is picked up without touching it; a fresh cut fails in WorktreeManager.create as it always did), and the context's startRef becomes the reused branch's tip with a warn saying so. New test: a reuse start still resumes a run branch when the base has no local branch covers both the tracking-ref and the base-gone cases.

2. Empty pr.baseRefName / mr.targetBranch reach the validator (High) — fixed. baseBranchOrDefault(base, what) in core/base-branch.ts returns the value or DEFAULT_BASE_BRANCH with a console.warn naming the PR/MR — a loud default, not a silent one. Used by review-pr, review-mr and revise-pr (whose baseBranch is now validated too, see 6).

3. startRef..FETCH_HEAD vs HEAD..FETCH_HEAD under reuse (Medium) — you are right; reverted. The count is HEAD..FETCH_HEAD again and the startRef option is gone from RebaseOntoRemoteBaseOptions; the doc comment explains why HEAD (a reused branch may be cut from an older base than the run resolved). The pre-push rebase reads ctx.baseBranch from the context; that is the part of #446 it keeps.

4. examples/validate-phase3.ts (Medium) — fixed, three call sites.

5. No tests for the behaviour-changing callers (Medium) — fixed. review-pr.test.ts and review-mr.test.ts each gain a case driving the default reviewer and asserting prepareContext receives [pr.baseRefName | mr.targetBranch, 'base'], and ['main', 'base'] plus the warning when the record carries none. research has no test file and no fixture to drive the default researcher; its call site is the one-liner baseBranch: DEFAULT_BASE_BRANCH, start: 'base'.

6. baseBranch unvalidated for { prHead } (Low) — fixed. assertSafeBaseBranch(baseBranch) runs first in resolveStartRef for every start kind; test added (--upload-pack=x with a prHead start rejects).

7. Stale _baseBranch doc (Low) — fixed: it now says it is a 'base' start that keeps a local copy that is ahead, for repos without an origin.

Gates: pnpm lint clean, pnpm typecheck 0 errors, pnpm test 123 files / 2512 passed / 3 skipped.

[github-actions]

Vanguard Review

Verdict: NO BLOCKING FINDINGS

No blocking findings.

No CI-config paths (.github/workflows/, .github/actions/, .gitlab-ci.yml, .gitlab/**.yml) are touched, so CLAUDE.md's hard constraint and its author-gate carve-out don't apply here. CLAUDE.md defines no severity scale beyond blocking / not-blocking, so each item is labelled that way.

I verified the refactor against the base tree: resolveRemoteBaseRef already returned SHAs, WorktreeManager.create re-asserts assertSafeBaseBranch and accepts a SHA identically, the reuse path still short-circuits on an existing run branch, publishForReview's ctx.baseBranch equals the old opts.baseBranch ?? 'main' at the only caller (deliverChange), closingKeywordScan + ctx.startRef reproduces the old baseRef scan range, and revise-pr's delivery goes through pushToExistingBranch (no ctx.baseBranch, so no new force-push/rebase risk). All seven prepareContext call sites are updated.

1. Non-blocking — the relocated PR-head fetch drops the hardening of its new neighbour

src/core/vanguard.ts (resolveStartRef, { prHead } branch):

await execa('git', ['fetch', '--end-of-options', 'origin', `refs/heads/${start.prHead}`], { cwd });

runFetch in src/core/base-branch.ts:134 deliberately sets env: FETCH_ENV (GIT_TERMINAL_PROMPT=0) and timeout: FETCH_TIMEOUT_MS (60s) — "Git never prompts for credentials from a daemon; an unreachable origin fails fast instead." This fetch has neither, so an origin whose credentials aren't usable can hang prepareContext on a credential prompt instead of failing. It's also the only git failure on these paths that escapes raw: everything around it goes through WorktreeError / redactGitError (base-branch.ts:81, manager.ts:70, remote-branch.ts errorMessage), and a raw execa rejection carries unredacted stderr. Suggest { cwd, env: { GIT_TERMINAL_PROMPT: '0' }, timeout: 60_000 } and wrapping the failure in WorktreeError with redactGitError(cause). Pre-existing in revise-pr.ts, but this PR promotes it to "the one place" start refs are resolved, so it now applies to every future { prHead } caller.

2. Non-blocking — review-pr/review-mr now read repo instructions from an unmerged branch

src/cli/review-pr.ts:115, src/cli/review-mr.ts:105. The review worktree is now cut from the PR's base / MR's target branch rather than local main. The agent's cwd therefore mounts CLAUDE.md, AGENTS.md and skills/ from that branch — which for a stacked PR (base = another contributor's branch) was never reviewed or merged. The diff under review was always untrusted; the instruction files around it previously came from main. The PR description argues only the upside ("the reviewer now reads the tree the change actually lands on") and doesn't mention this. Either note the trade-off, or keep the reviewer's instruction files pinned to the default branch.

3. Non-blocking — PR description contradicts the diff in two places

The body claims rebaseOntoRemoteBase "takes startRef and counts startRef..FETCH_HEAD", and that remote-branch.test.ts's count expectation now reads ${ctx.startRef}..FETCH_HEAD. Neither is in the diff: RebaseOntoRemoteBaseOptions is unchanged, the body still counts HEAD..FETCH_HEAD, and the new doc comment correctly states the opposite ("Compares HEAD, not startRef, against FETCH_HEAD"). The code is right — the description is wrong, and it becomes the squash-merge body.

4. Non-blocking — breaking public API under a non-breaking commit subject

PrepareOptions.baseBranch and .start are now required; PublishOptions.baseBranch, DeliveryTarget.new-pr.baseBranch and the resolveSpecBaseRef export are gone. PrepareOptions is re-exported from src/index.ts for package vanguard (no private: true, main: dist/index.js, release-please release-type: node), so refactor(core): cuts a non-major release. Add ! or a BREAKING CHANGE: footer. (TaskStart being added to index.ts is correct.)

Nits (non-blocking)

• Every revise-pr.test.ts case passes _baseBranch, which this PR re-routes to start: 'base', keepLocalIfAhead: true, so revise-pr's production start: { prHead } path still has no coverage in that file. vanguard.test.ts does cover resolveStartRef's prHead branch against a real origin, which is the better test — just worth knowing the runner-level wiring isn't asserted.
• baseBranchOrDefault is called inside runDefaultReviewer/runDefaultMrReviewer, which re-run on retry, so the "carries no base branch" warning prints twice per review.
• "no bare 'main' literal remains on the path" isn't quite true: WorktreeManager.create(taskId, baseBranch = 'main') still carries the default (now always overridden by prepareContext).

Nothing I found requires a fix before merge. I did not independently run pnpm test/pnpm typecheck (the checked-out tree is the base commit, not the PR head), so the gate results in the description are taken as reported.

[SebaBoler]

Round 2 (NO BLOCKING FINDINGS) — the non-blocking items, addressed in ae81058 and the PR text:

1. Unhardened PR-head fetch — fixed (ae81058). The { prHead } fetch in resolveStartRef now runs with GIT_TERMINAL_PROMPT=0, a 60 s timeout and reject: false; a non-zero exit becomes WorktreeError("Cannot fetch origin/<head> for <task>: <redactGitError(…)>"), so nothing on this path escapes as raw execa stderr any more.

2. Reviewer worktree from the PR base vs. instruction files from main — trade-off now stated in the PR body (section "review-pr / review-mr / research"). Kept as is: reviewing against the branch the change lands on is the right default, and pinning CLAUDE.md/AGENTS.md/skills/ to the default branch is a possible follow-up if stacked PRs from untrusted authors become a concern.

3. PR description contradicted the diff — fixed. The body now says the opposite, matching the code: rebaseOntoRemoteBase still counts HEAD..FETCH_HEAD, with the reuse rationale.

4. Breaking public API under a non-breaking subject — fixed. Title is refactor(core)!: … and the body ends with a BREAKING CHANGE: footer listing every removed/required member, so release-please cuts a major.

Nits: revise-pr's production { prHead } wiring is exercised by vanguard.test.ts against a real origin (as you noted); the double warning on a retried review is a cosmetic duplicate of one line and left as is; WorktreeManager.create's = 'main' parameter default is now mentioned in the body as always overridden by prepareContext.

Gates on ae81058: pnpm lint clean, pnpm typecheck 0 errors, pnpm test 123 files / 2512 passed / 3 skipped.

[github-actions]

Vanguard Review

Verdict: NO BLOCKING FINDINGS

No blocking findings.

The repo documents only a binary severity (blocking = CLAUDE.md's hard constraints, correctness, or security must be fixed before merge; otherwise not), so each item below is labelled non-blocking with a Medium/Low impact note. This diff touches no .github/workflows/, .github/actions/, or .gitlab* path, so the CI-config constraint doesn't apply. I verified the resolution contract against src/core/base-branch.ts (resolveRemoteBaseRef may return a SHA, refs/heads/<base>, or the bare name — all of which revParseCommit handles), WorktreeManager.create (assertSafeBaseBranch accepts a SHA, reuse ignores the base), commitMessages (<ref>..HEAD, so a SHA is fine), and that all 8 production prepareContext call sites are updated. publishForReview's --base/--target-branch and rebaseOntoRemoteBase's fetch target are behaviour-preserving for run/runSourcedIssue (deps.baseBranch ?? 'main' before and after).

1. Reviewer instruction files now come from the PR's base branch (Medium, non-blocking, security)
src/cli/review-pr.ts:116 / src/cli/review-mr.ts:107 — the review worktree is now cut from pr.baseRefName / mr.targetBranch, so CLAUDE.md, AGENTS.md and skills/ as the reviewer agent sees them are whatever that branch holds. Copy-back protects CI config but not instruction files, so any branch in the repo (including a branch a prior Vanguard run pushed) can steer the reviewer that gates it. The PR body discloses and justifies this, and creating a base branch needs repo write, so it isn't blocking — but the follow-up it names (pin instruction files to the default branch, or only honour a non-default base when the API-sourced author passes the author gate) is the mitigation worth scheduling, not deferring indefinitely. The reviewed diff itself still comes from gh pr diff (src/runners/pr-review.ts:132), so the diff content is unaffected.

2. The { prHead } start resolves through the shared FETCH_HEAD (Low, non-blocking)
resolveStartRef in src/core/vanguard.ts fetches then rev-parses FETCH_HEAD, which lives in the common git dir — every concurrent resolveRemoteBaseRef fetch against the same repoPath rewrites it too (runFetch in src/core/base-branch.ts, the fan-out that module's #434 lock-retry logic exists for). The window is much narrower than the old inline fetch in revise-pr, but the new doc comment ("a concurrent run's fetch cannot move it") only holds after the rev-parse. Fix matching the rest of the module: fetch +refs/heads/<prHead>:refs/vanguard/<taskId>/prhead and rev-parse that named ref. Nit in the same block: baseBranch is run through assertSafeBaseBranch for every start kind, but prHead is not — it relies on GitHub refname validity plus --end-of-options/full refs/heads/ source; a cheap assertSafeBaseBranch(start.prHead) would make that defence explicit rather than argued in a comment.

3. Read-only reviews now hard-fail when the base is unresolvable (Low, non-blocking)
With a base that origin no longer carries and no local branch, resolveRemoteBaseRef returns the bare name, revParseCommit returns '', and prepareContext throws — e.g. a PR/MR whose base branch was deleted gets no review at all, where previously it was reviewed against local main. "Fail loudly" is right for a run that will push; for the read-only reviewers a logged fall back to DEFAULT_BASE_BRANCH would keep the review happening.

4. DEFAULT_BASE_BRANCH is not reachable from the package root (Low, non-blocking)
src/index.ts exports type TaskStart but nothing from core/base-branch.js, so the BREAKING CHANGE note's "DEFAULT_BASE_BRANCH … new export" only holds via a deep dist/core/base-branch.js import. Since PrepareOptions.baseBranch is now required, every barrel consumer must write the bare 'main' literal the constant exists to prevent — as the updated README.md:180 example itself does. Add it (and baseBranchOrDefault) to src/index.ts and use it in the README snippet.

5. revise-pr's production start path is untested; the test hook diverges further (Low, non-blocking)
src/runners/revise-pr.ts:243 — no runner-level test covers start: { prHead } or baseBranchOrDefault(pr.baseRefName, …); all revise-pr tests go through _baseBranch, whose meaning changed from "cut directly from this ref" to "a 'base' start through resolveRemoteBaseRef with keepLocalIfAhead: true" (so it now fetches if the test repo has an origin, and sets ctx.baseBranch to the local test branch). The core test in vanguard.test.ts covers the mechanism; a revise-pr-level assertion that prepareContext receives { prHead: pr.headRefName } and the PR's base would close the gap at the call site that actually pushes.

Not covered: I did not run pnpm test/typecheck (gate results are taken from the PR body), and I only spot-checked the ~60 mechanical baseBranch: 'main', start: 'base' test updates rather than reading each.