FF-442: Enforce issue branch matches workspace on wake/checkout#6425
FF-442: Enforce issue branch matches workspace on wake/checkout#6425daveliddlefreddies wants to merge 1 commit into
Conversation
Fail heartbeats before adapter execution when the git branch in PAPERCLIP_WORKSPACE_CWD does not match the checked-out issue, inject PAPERCLIP_EXPECTED_BRANCH, refuse shared project_primary cwd conflicts, and document Step 5.5 branch preflight for operators. Co-Authored-By: Paperclip <noreply@paperclip.ing> Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThis PR introduces a
Confidence Score: 3/5The preflight guard is a safety-critical path; the auto-checkout branch has an exception-handling gap that surfaces as a confusing adapter_failed error, and the env injection in workspace-runtime.ts is wired up but never passed to the call site. The auto-checkout path in checkoutGitBranch propagates a raw execFile error on failure, bypassing the WorkspaceBranchPreflightError wrapper the heartbeat outer-catch relies on. Additionally, buildWorkspaceCommandEnv exposes expectedBranchName but the sole call site never passes it, leaving PAPERCLIP_EXPECTED_BRANCH unset for workspace provision scripts. workspace-branch-preflight.ts (auto-checkout error handling) and workspace-runtime.ts (missing call-site wiring for expectedBranchName) need attention before deployment. Important Files Changed
|
| if (input.expectedBranchName) { | ||
| env.PAPERCLIP_EXPECTED_BRANCH = input.expectedBranchName; | ||
| } |
There was a problem hiding this comment.
expectedBranchName is never passed at the only call site
The new expectedBranchName field is added to the buildWorkspaceCommandEnv input type and the env-injection logic is wired up, but the sole call site (workspace provision command, ~line 922) does not pass expectedBranchName. As a result, PAPERCLIP_EXPECTED_BRANCH will never be set via this code path. If workspace provision scripts are intended to read this variable (e.g., scripts/paperclip_verify_branch.sh mentioned in the docs), they will receive an empty/unset value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/workspace-runtime.ts
Line: 718-720
Comment:
**`expectedBranchName` is never passed at the only call site**
The new `expectedBranchName` field is added to the `buildWorkspaceCommandEnv` input type and the env-injection logic is wired up, but the sole call site (workspace provision command, ~line 922) does not pass `expectedBranchName`. As a result, `PAPERCLIP_EXPECTED_BRANCH` will never be set via this code path. If workspace provision scripts are intended to read this variable (e.g., `scripts/paperclip_verify_branch.sh` mentioned in the docs), they will receive an empty/unset value.
How can I resolve this? If you propose a fix, please make it concise.| expectedBranch, | ||
| expectedSpec, | ||
| currentBranch, |
There was a problem hiding this comment.
Case-sensitive prefix matching may silently reject valid branches
The ff-<num>- prefix is derived by lowercasing the identifier (e.g., "FF-442" → ff-442-). But branchMatchesSpec uses String.prototype.startsWith, which is case-sensitive. A branch created as FF-442-my-feature (uppercase project prefix) would fail the check and block the run, even though it is the correct branch for the issue. Consider normalizing both sides to lowercase before the prefix comparison, or documenting that branch names must use lowercase project prefixes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/workspace-branch-preflight.ts
Line: 460-462
Comment:
**Case-sensitive prefix matching may silently reject valid branches**
The `ff-<num>-` prefix is derived by lowercasing the identifier (e.g., "FF-442" → `ff-442-`). But `branchMatchesSpec` uses `String.prototype.startsWith`, which is case-sensitive. A branch created as `FF-442-my-feature` (uppercase project prefix) would fail the check and block the run, even though it is the correct branch for the issue. Consider normalizing both sides to lowercase before the prefix comparison, or documenting that branch names must use lowercase project prefixes.
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,471 @@ | |||
| import { execFile as execFileCallback } from "node:child_process"; | |||
There was a problem hiding this comment.
PR description is missing required CONTRIBUTING.md template sections
Per CONTRIBUTING.md, every PR must follow the PR template and include: a Thinking Path (top-down reasoning from the product down to the specific change), explicit Risks, and a Model Used section with provider, exact model ID/version, context window size, and capability details. The current description has a summary and test plan but omits the Thinking Path entirely, has no Risks section, and "Made with Cursor" does not satisfy the required model detail (no model ID, no context window size). Please fill out the full PR template.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/workspace-branch-preflight.ts
Line: 1
Comment:
**PR description is missing required CONTRIBUTING.md template sections**
Per `CONTRIBUTING.md`, every PR must follow the PR template and include: a **Thinking Path** (top-down reasoning from the product down to the specific change), explicit **Risks**, and a **Model Used** section with provider, exact model ID/version, context window size, and capability details. The current description has a summary and test plan but omits the Thinking Path entirely, has no Risks section, and "Made with Cursor" does not satisfy the required model detail (no model ID, no context window size). Please fill out the full PR template.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents run inside git worktrees; the heartbeat system establishes a workspace branch and tracks it through checkout, realization, restore, and finalization > - When the live git branch diverges from the recorded workspace branch mid-change (branch incoherence), the heartbeat must fail closed with `workspace_validation_failed` and block the source issue with a recovery action > - There were no embedded-Postgres tests covering this fail-closed behavior across the three interlock call sites: fresh git-worktree realization, persisted workspace restore, and heartbeat finalization > - This PR adds a single test file covering all three call sites with an embedded-Postgres heartbeat test harness and asserts the exact fail-closed outcome and evidence fields > - The benefit is confidence that branch-incoherence containment is correct and regressions in the interlock chain are caught before they silently corrupt workspace state ## Linked Issues or Issue Description Refs: #6425 (related: enforce issue branch matches workspace on wake/checkout) No pre-existing public GitHub issue for this specific reproduction test gap. The underlying problem: **Bug / gap:** The heartbeat's branch-incoherence containment was untested by any embedded-Postgres integration test. All three call sites — fresh git-worktree realization, persisted workspace restore, and finalization — could regress without detection. The fail-closed path (`workspace_validation_failed` + source-issue block + deduped recovery action) and the evidence fields surfaced to operators were unverified. ## What Changed - Added `server/src/__tests__/heartbeat-workspace-branch-containment.test.ts` with embedded-Postgres integration tests covering: - **Fresh git-worktree realization** — heartbeat detects branch divergence at workspace setup and fails closed - **Persisted workspace restore** — re-entering a previously-established workspace with a diverged branch fails closed instead of being silently coerced into a generic reuse-failure path - **Heartbeat finalization** — any late-stage branch incoherence detected at finalization fails closed - Asserts fail-closed behavior in all three cases: run status = `workspace_validation_failed`, source issue status = `blocked`, exactly one deduped workspace-validation recovery action on the blocked issue, sibling issues on same/other workspaces retain their status - Asserts evidence completeness: `expectedBranch`, `liveBranch`, `expectedHead`, `liveHead`, `cleanliness`, `ancestryVerdict`, `plainLanguageReason`, and `recoveryGuidance` fields are present and correct on the run - Ensures release/promotion errors after setup failures are logged (not silently swallowed), making cleanup failures observable ## Verification ```bash pnpm exec vitest run server/src/__tests__/heartbeat-workspace-branch-containment.test.ts pnpm --filter @paperclipai/server typecheck ``` All 3 tests pass, typecheck clean. ## Risks Low. Test-only change — no production code paths are modified. The tests use an embedded-Postgres harness and do not touch any shared or live database. ## Model Used Claude Sonnet 4.6 (`claude-sonnet-4-6`) via Claude Code — tool use mode, standard context window. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
Summary
Prevents coding agents from running on the wrong git branch when using shared
project_primaryworkspaces (incident: FF-425 woke on FF-438's branch).workspace-branch-preflightservice: derive expected branch from persisted workspace, template, description links, orff-<issueNumber>-*prefix; detect shared-cwd conflicts; optional auto-checkout; fail heartbeat before adapter withworkspace_branch_preflight.PAPERCLIP_EXPECTED_BRANCHfor adapters.Test plan
vitest run server/src/__tests__/workspace-branch-preflight.test.tspnpm --filter @paperclipai/server typecheckworkspace_branch_preflightFF tracking
[FF-442](https://paperclip instance issue) — Paperclip platform work for Freddie's Flowers.
Made with Cursor